Skip to contentSkip to conversation

Allow setting up shared memory areas between VMs from xl config files

PATCH Superseded Zhongze Liu 6 participants 31 messages Aug 22, 2017 – Aug 25, 2017

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover Allow setting up shared memory areas between VMs from xl config files

This series implements the new xl config entry proposed in [1]. Users can use the new config entry to statically setup shared memory areas among VMs so that they could communicate with each other through the static shared memory areas.

[1] Proposla to allow setting up shared memory areas between VMs from xl config file:

    https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cheers,

Zhongze Liu (6):
libxc: add xc_domain_remove_from_physmap to wrap
XENMEM_remove_from_physmap
libxl: introduce a new structure to represent static shared memory
regions
libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config
files
xsm: flask: change the interface and default policy for
xsm_map_gmfn_foregin
libxl: support mapping static shared memory areas during domain
creation
libxl: support unmapping static shared memory areas during domain
destruction

 tools/libxc/include/xenctrl.h |   4 +
 tools/libxc/xc_domain.c       |  11 +
 tools/libxl/Makefile          |   4 +-
 tools/libxl/libxl.h           |   4 +
 tools/libxl/libxl_arch.h      |   6 +
 tools/libxl/libxl_arm.c       |  15 ++
 tools/libxl/libxl_create.c    |  27 +++
 tools/libxl/libxl_domain.c    |   5 +
 tools/libxl/libxl_internal.h  |  16 ++
 tools/libxl/libxl_sshm.c      | 461 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl   |  36 +++-
 tools/libxl/libxl_x86.c       |  18 ++
 tools/libxl/libxl_xshelp.c    |   8 +
 tools/libxl/libxlu_sshm.c     | 210 +++++++++++++++++++
 tools/libxl/libxlutil.h       |   6 +
 tools/xl/xl_parse.c           |  24 ++-
 xen/arch/arm/mm.c             |   2 +-
 xen/arch/x86/mm/p2m.c         |   2 +-
 xen/include/xsm/dummy.h       |   6 +-
 xen/include/xsm/xsm.h         |   7 +-
 xen/xsm/flask/hooks.c         |   6 +-
 21 files changed, 862 insertions(+), 16 deletions(-)
 create mode 100644 tools/libxl/libxl_sshm.c
 create mode 100644 tools/libxl/libxlu_sshm.c
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#1 lore raw

PATCH 1/6 libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file". See:

https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Then plan is to use XENMEM_add_to_physmap_batch to map the shared pages from one domU to another and use XENMEM_remove_from_physmap to cancel the sharing. A wrapper to XENMEM_add_to_physmap_batch was added in the following commit:

commit 20e725e9364cff4a29945f66986ecd88cca8743d

Now add the wrapper to XENMEM_remove_from_physmap.

Cc Ian Jackson <[email protected]>
Cc Wei Liu <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxc/include/xenctrl.h |  4 ++++
 tools/libxc/xc_domain.c       | 11 +++++++++++
 2 files changed, 15 insertions(+)
tools/libxc/include/xenctrl.h +4 -0
index c7710b8f36..0ff15a9255 100644
@@ -1381,6 +1381,10 @@ int xc_domain_add_to_physmap_batch(xc_interface *xch,
                                    xen_pfn_t *gfpns,
                                    int *errs);
 
+int xc_domain_remove_from_physmap(xc_interface *xch,
+                                  domid_t domid,
+                                  xen_pfn_t gpfn);
+
 int xc_domain_populate_physmap(xc_interface *xch,
                                uint32_t domid,
                                unsigned long nr_extents,
tools/libxc/xc_domain.c +11 -0
index 3bab4e8bab..e6b32792c0 100644
@@ -1077,6 +1077,17 @@ out:
     return rc;
 }
 
+int xc_domain_remove_from_physmap(xc_interface *xch,
+                                  domid_t domid,
+                                  xen_pfn_t gpfn)
+{
+    struct xen_remove_from_physmap xrfp = {
+        .domid = domid,
+        .gpfn = gpfn,
+    };
+    return do_memory_op(xch, XENMEM_remove_from_physmap, &xrfp, sizeof(xrfp));
+}
+
 int xc_domain_claim_pages(xc_interface *xch,
                                uint32_t domid,
                                unsigned long nr_pages)
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Acked-by on v1 1/6
#2 lore raw

Re: [PATCH 1/6] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap

Quoted 9 lines · Zhongze Liu

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file". See:

https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Then plan is to use XENMEM_add_to_physmap_batch to map the shared pages from one domU to another and use XENMEM_remove_from_physmap to cancel the sharing. A wrapper to XENMEM_add_to_physmap_batch was added in the following commit:

Quoted 6 lines

commit 20e725e9364cff4a29945f66986ecd88cca8743d

Now add the wrapper to XENMEM_remove_from_physmap.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#3 lore raw

PATCH 2/6 libxl: introduce a new structure to represent static shared memory regions

In reply to Zhongze Liu

Add a new structure to the IDL famliy to represent static shared memory regions, as proposed in the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Wei Liu <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl.h         |  4 ++++
 tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)
tools/libxl/libxl.h +4 -0
index 229e289750..3ee788642f 100644
@@ -2237,6 +2237,10 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
 int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
                                const char *command_line, char **output);
 
+/* Constants for libxl_static_shm */
+#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
+#define LIBXL_SSHM_ID_MAXLEN    128
+
 #include <libxl_event.h>
 
 #endif /* LIBXL_H */
tools/libxl/libxl_types.idl +32 -4
index 6e80d36256..6c9e79c05d 100644
@@ -472,7 +472,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("blkdev_start",    string),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-    
+
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     # if you set device_model you must set device_model_version too
@@ -494,7 +494,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("ioports",          Array(libxl_ioport_range, "num_ioports")),
     ("irqs",             Array(uint32, "num_irqs")),
     ("iomem",            Array(libxl_iomem_range, "num_iomem")),
-    ("claim_mode",	     libxl_defbool),
+    ("claim_mode",           libxl_defbool),
     ("event_channels",   uint32),
     ("kernel",           string),
     ("cmdline",          string),
@@ -543,10 +543,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("keymap",           string),
                                        ("sdl",              libxl_sdl_info),
                                        ("spice",            libxl_spice_info),
-                                       
+
                                        ("gfx_passthru",     libxl_defbool),
                                        ("gfx_passthru_kind", libxl_gfx_passthru_kind),
-                                       
+
                                        ("serial",           string),
                                        ("boot",             string),
                                        ("usb",              libxl_defbool),
@@ -779,6 +779,33 @@ libxl_device_channel = Struct("device_channel", [
            ])),
 ])
 
+libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
+    (-1, "UNKNOWN"),
+    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
+    (32,  "X86_NORMAL"), # X86 policies should be >= 32
+    ], init_val = "LIBXL_SSHM_CHCHE_POLICY_UNKNOWN")
+
+libxl_sshm_prot = Enumeration("sshm_prot", [
+    (-1, "UNKNOWN"),
+    (3,  "RW"),
+    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
+
+libxl_sshm_role = Enumeration("sshm_role", [
+    (-1, "UNKNOWN"),
+    (0,  "MASTER"),
+    (1,  "SLAVE"),
+    ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
+
+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("prot", libxl_sshm_prot, {'init_val': 'LIBXL_SSHM_PROT_UNKNOWN'}),
+    ("cache_policy", libxl_sshm_cachepolicy, {'init_val': 'LIBXL_SSHM_CACHEPOLICY_UNKNOWN'}),
+    ("role", libxl_sshm_role, {'init_val': 'LIBXL_SSHM_ROLE_UNKNOWN'}),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -797,6 +824,7 @@ libxl_domain_config = Struct("domain_config", [
     ("channels", Array(libxl_device_channel, "num_channels")),
     ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
     ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
+    ("sshms", Array(libxl_static_shm, "num_sshms")),
 
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
Reviewed-by on v1 2/6
#4 lore raw

Re: [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions

Quoted 1 line · Zhongze Liu

Add a new structure to the IDL famliy to represent static shared memory regions,

^ family

Quoted 37 lines

as proposed in the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Wei Liu <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl.h         |  4 ++++
 tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)
tools/libxl/libxl.h +4 -0
index 229e289750..3ee788642f 100644
@@ -2237,6 +2237,10 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
 int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
                                const char *command_line, char **output);
 
+/* Constants for libxl_static_shm */
+#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
+#define LIBXL_SSHM_ID_MAXLEN    128
+
 #include <libxl_event.h>
 
 #endif /* LIBXL_H */
tools/libxl/libxl_types.idl +0 -0
index 6e80d36256..6c9e79c05d 100644

tools/libxl/libxl_types.idl libxl_domain_build_info = Struct("domain_build_info",[ · line 475

Quoted 5 lines
tools/libxl/libxl_types.idl +1 -1
@@ -472,7 +472,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("blkdev_start",    string),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-    
+

Although your code style corrections are appropriate, usually we do them in separate patches to separate them out from more meaningful changes. However, different maintainers have different styles, so Wei might be OK with this.

In any case:

Quoted 70 lines
tools/libxl/libxl_types.idl +31 -3
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     # if you set device_model you must set device_model_version too
@@ -494,7 +494,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("ioports",          Array(libxl_ioport_range, "num_ioports")),
     ("irqs",             Array(uint32, "num_irqs")),
     ("iomem",            Array(libxl_iomem_range, "num_iomem")),
-    ("claim_mode",	     libxl_defbool),
+    ("claim_mode",           libxl_defbool),
     ("event_channels",   uint32),
     ("kernel",           string),
     ("cmdline",          string),
@@ -543,10 +543,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("keymap",           string),
                                        ("sdl",              libxl_sdl_info),
                                        ("spice",            libxl_spice_info),
-                                       
+
                                        ("gfx_passthru",     libxl_defbool),
                                        ("gfx_passthru_kind", libxl_gfx_passthru_kind),
-                                       
+
                                        ("serial",           string),
                                        ("boot",             string),
                                        ("usb",              libxl_defbool),
@@ -779,6 +779,33 @@ libxl_device_channel = Struct("device_channel", [
            ])),
 ])
 
+libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
+    (-1, "UNKNOWN"),
+    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
+    (32,  "X86_NORMAL"), # X86 policies should be >= 32
+    ], init_val = "LIBXL_SSHM_CHCHE_POLICY_UNKNOWN")
+
+libxl_sshm_prot = Enumeration("sshm_prot", [
+    (-1, "UNKNOWN"),
+    (3,  "RW"),
+    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
+
+libxl_sshm_role = Enumeration("sshm_role", [
+    (-1, "UNKNOWN"),
+    (0,  "MASTER"),
+    (1,  "SLAVE"),
+    ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
+
+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("prot", libxl_sshm_prot, {'init_val': 'LIBXL_SSHM_PROT_UNKNOWN'}),
+    ("cache_policy", libxl_sshm_cachepolicy, {'init_val': 'LIBXL_SSHM_CACHEPOLICY_UNKNOWN'}),
+    ("role", libxl_sshm_role, {'init_val': 'LIBXL_SSHM_ROLE_UNKNOWN'}),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -797,6 +824,7 @@ libxl_domain_config = Struct("domain_config", [
     ("channels", Array(libxl_device_channel, "num_channels")),
     ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
     ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
+    ("sshms", Array(libxl_static_shm, "num_sshms")),
 
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
Signature · 1 line
2.14.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#5 lore raw

Re: [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions

Hi Stefano,

2017-08-23 4:05 GMT+08:00 Stefano Stabellini <[email protected]>:

Quoted 53 lines · Zhongze Liu
Quoted 1 line

Add a new structure to the IDL famliy to represent static shared memory regions,

^ family

Quoted 37 lines

as proposed in the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Wei Liu <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl.h         |  4 ++++
 tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
 2 files changed, 36 insertions(+), 4 deletions(-)
tools/libxl/libxl.h +4 -0
index 229e289750..3ee788642f 100644
@@ -2237,6 +2237,10 @@ int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock);
 int libxl_qemu_monitor_command(libxl_ctx *ctx, uint32_t domid,
                                const char *command_line, char **output);
 
+/* Constants for libxl_static_shm */
+#define LIBXL_SSHM_RANGE_UNKNOWN UINT64_MAX
+#define LIBXL_SSHM_ID_MAXLEN    128
+
 #include <libxl_event.h>
 
 #endif /* LIBXL_H */
tools/libxl/libxl_types.idl +0 -0
index 6e80d36256..6c9e79c05d 100644

tools/libxl/libxl_types.idl libxl_domain_build_info = Struct("domain_build_info",[ · line 475

Quoted 5 lines
tools/libxl/libxl_types.idl +1 -1
@@ -472,7 +472,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("blkdev_start",    string),
 
     ("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-
+

Although your code style corrections are appropriate, usually we do them in separate patches to separate them out from more meaningful changes. However, different maintainers have different styles, so Wei might be OK with this.

Quoted 3 lines

In any case:

Thanks for reviewing. This is actually a 'clean-trailing-white-space-on-save' feature
of my editor. I always turn it off when modifying the toolstack code, because there
are trailing white spaces at many unexpected corners and this will screw up the diff. I don't know why I somehow missed this file. But I think it's not a big problem
though. I will wait for Wei's comments on this. And it won't be too hard to restore
the style corrections.

But I think we really should do a big white spaces cleanup in the toolstack code.

Cheers,

Zhongze Liu

Quoted 67 lines
Quoted 67 lines
tools/libxl/libxl_types.idl +31 -3
     ("device_model_version", libxl_device_model_version),
     ("device_model_stubdomain", libxl_defbool),
     # if you set device_model you must set device_model_version too
@@ -494,7 +494,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("ioports",          Array(libxl_ioport_range, "num_ioports")),
     ("irqs",             Array(uint32, "num_irqs")),
     ("iomem",            Array(libxl_iomem_range, "num_iomem")),
-    ("claim_mode",        libxl_defbool),
+    ("claim_mode",           libxl_defbool),
     ("event_channels",   uint32),
     ("kernel",           string),
     ("cmdline",          string),
@@ -543,10 +543,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("keymap",           string),
                                        ("sdl",              libxl_sdl_info),
                                        ("spice",            libxl_spice_info),
-
+
                                        ("gfx_passthru",     libxl_defbool),
                                        ("gfx_passthru_kind", libxl_gfx_passthru_kind),
-
+
                                        ("serial",           string),
                                        ("boot",             string),
                                        ("usb",              libxl_defbool),
@@ -779,6 +779,33 @@ libxl_device_channel = Struct("device_channel", [
            ])),
 ])
 
+libxl_sshm_cachepolicy = Enumeration("sshm_cachepolicy", [
+    (-1, "UNKNOWN"),
+    (0,  "ARM_NORMAL"),  # ARM policies should be < 32
+    (32,  "X86_NORMAL"), # X86 policies should be >= 32
+    ], init_val = "LIBXL_SSHM_CHCHE_POLICY_UNKNOWN")
+
+libxl_sshm_prot = Enumeration("sshm_prot", [
+    (-1, "UNKNOWN"),
+    (3,  "RW"),
+    ], init_val = "LIBXL_SSHM_PROT_UNKNOWN")
+
+libxl_sshm_role = Enumeration("sshm_role", [
+    (-1, "UNKNOWN"),
+    (0,  "MASTER"),
+    (1,  "SLAVE"),
+    ], init_val = "LIBXL_SSHM_ROLE_UNKNOWN")
+
+libxl_static_shm = Struct("static_shm", [
+    ("id", string),
+    ("offset", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("begin", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("end", uint64, {'init_val': 'LIBXL_SSHM_RANGE_UNKNOWN'}),
+    ("prot", libxl_sshm_prot, {'init_val': 'LIBXL_SSHM_PROT_UNKNOWN'}),
+    ("cache_policy", libxl_sshm_cachepolicy, {'init_val': 'LIBXL_SSHM_CACHEPOLICY_UNKNOWN'}),
+    ("role", libxl_sshm_role, {'init_val': 'LIBXL_SSHM_ROLE_UNKNOWN'}),
+])
+
 libxl_domain_config = Struct("domain_config", [
     ("c_info", libxl_domain_create_info),
     ("b_info", libxl_domain_build_info),
@@ -797,6 +824,7 @@ libxl_domain_config = Struct("domain_config", [
     ("channels", Array(libxl_device_channel, "num_channels")),
     ("usbctrls", Array(libxl_device_usbctrl, "num_usbctrls")),
     ("usbdevs", Array(libxl_device_usbdev, "num_usbdevs")),
+    ("sshms", Array(libxl_static_shm, "num_sshms")),
 
Quoted 5 lines
Quoted 5 lines
tools/libxl/libxl_types.idl +0 -1
     ("on_poweroff", libxl_action_on_shutdown),
     ("on_reboot", libxl_action_on_shutdown),
--

2.14.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#6 lore raw

PATCH 3/6 libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files

In reply to Zhongze Liu

Add the parsing utils for the newly introduced libxl_static_sshm struct to the libxl/libxlu_* family. And add realated parsing code in xl to parse the struct from xl config files. This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile      |   2 +-
 tools/libxl/libxlu_sshm.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxlutil.h   |   6 ++
 tools/xl/xl_parse.c       |  24 +++++-
 4 files changed, 240 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxlu_sshm.c
tools/libxl/Makefile +1 -1
index 082af8f716..3b63fb2cad 100644
@@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
tools/libxl/libxlu_sshm.c new file +210 -0

210 lines not on this page · raw message

tools/libxl/libxlutil.h +6 -0
index e81b644c01..ee39cb5bdc 100644
@@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
 int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
                        libxl_device_nic *nic);
 
+/*
+ * static shared memory specification parsing
+ */
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+                   libxl_static_shm *sshm);
+
 #endif /* LIBXLUTIL_H */
 
 /*
tools/xl/xl_parse.c +23 -1
index 5c2bf17222..82d955b8b9 100644
@@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
     long l, vcpus = 0;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
-                   *usbctrls, *usbdevs, *p9devs;
+                   *usbctrls, *usbdevs, *p9devs, *sshms;
     XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
                    *mca_caps;
     int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
@@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
+        d_config->num_sshms = 0;
+        d_config->sshms = NULL;
+        while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
+            libxl_static_shm *sshm;
+            char *buf2 = strdup(buf);
+            int ret;
+
+            sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
+                                             d_config->num_sshms,
+                                             libxl_static_shm_init);
+            ret = xlu_sshm_parse(config, buf2, sshm);
+            if (ret) {
+                fprintf(stderr,
+                        "xl: Invalid argument for static_shm: %s", buf2);
+                exit(EXIT_FAILURE);
+            }
+
+            free(buf2);
+        }
+    }
+
     if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
         libxl_device_p9 *p9;
         char *security_model = NULL;
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#7 lore raw

Re: [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files

Quoted 46 lines · Zhongze Liu

Add the parsing utils for the newly introduced libxl_static_sshm struct to the libxl/libxlu_* family. And add realated parsing code in xl to parse the struct from xl config files. This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile      |   2 +-
 tools/libxl/libxlu_sshm.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxlutil.h   |   6 ++
 tools/xl/xl_parse.c       |  24 +++++-
 4 files changed, 240 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxlu_sshm.c
tools/libxl/Makefile +1 -1
index 082af8f716..3b63fb2cad 100644
@@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
tools/libxl/libxlu_sshm.c new file +0 -0
new file mode 100644
index 0000000000..8647665213

tools/libxl/libxlu_sshm.c line 10

Quoted 10 lines
tools/libxl/libxlu_sshm.c new file +10 -0
@@ -0,0 +1,210 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+
+#include <ctype.h>
+
+#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
+#define WORD_RE         "([_a-zA-Z0-9]+)"
+#define EQU_RE         PARAM_RE(WORD_RE "\\s*=\\s*" WORD_RE)
+
+#define PAGE_SIZE_MASK ((uint64_t)0xfff)

You can probably use XC_PAGE_MASK that is already defined? Otherwise I would name this XLU_PAGE_MASK and move the definition to one of the libxlu headers.

I am not the most qualified person to review xlu code, but this patch looks OK to me.

Quoted 248 lines
tools/libxl/libxlu_sshm.c new file +199 -0
+#define RET_INVAL(msg, curr_str)  do {              \
+        xlu__sshm_err(cfg, msg, curr_str);          \
+        rc = EINVAL;                                \
+        goto out;                                   \
+    } while(0)
+
+/* set a member in libxl_static_shm and report an error if it's respecified,
+ * @curr_str indicates the head of the remaining string. */
+#define SET_VAL(var, name, type, value, curr_str)  do {                 \
+        if ((var) != LIBXL_SSHM_##type##_UNKNOWN && (var) != value) {   \
+            RET_INVAL("\"" name "\" respecified", curr_str);            \
+        }                                                               \
+        (var) = value;                                                  \
+    } while(0)
+
+
+static void xlu__sshm_err(XLU_Config *cfg, const char *msg,
+                          const char *curr_str) {
+    fprintf(cfg->report,
+            "%s: config parsing error in shared_memory: %s at '%s'\n",
+            cfg->config_source, msg, curr_str);
+}
+
+static int parse_prot(XLU_Config *cfg, char *str, libxl_sshm_prot *prot)
+{
+    int rc;
+    libxl_sshm_prot new_prot;
+
+    if (!strcmp(str, "rw")) {
+        new_prot = LIBXL_SSHM_PROT_RW;
+    } else {
+        RET_INVAL("invalid permission flags", str);
+    }
+
+    SET_VAL(*prot, "permission flags", PROT, new_prot, str);
+
+    rc = 0;
+
+ out:
+    return rc;
+}
+
+static int parse_cachepolicy(XLU_Config *cfg, char *str,
+                             libxl_sshm_cachepolicy *policy)
+{
+    int rc;
+    libxl_sshm_cachepolicy new_policy;
+
+    if (!strcmp(str, "ARM_normal")) {
+        new_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+    } else if (!strcmp(str, "x86_normal")) {
+        new_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    } else {
+        RET_INVAL("invalid cache policy", str);
+    }
+
+    SET_VAL(*policy, "cache policy", CACHEPOLICY, new_policy, str);
+    rc = 0;
+
+ out:
+    return rc;
+}
+
+/* handle key = value pairs */
+static int handle_equ(XLU_Config *cfg, char *key, char *val,
+                      libxl_static_shm *sshm)
+{
+    int rc;
+
+    if (!strcmp(key, "id")) {
+        if (strlen(val) > LIBXL_SSHM_ID_MAXLEN) { RET_INVAL("id too long", val); }
+        if (sshm->id && !strcmp(sshm->id, val)) {
+            RET_INVAL("id respecified", val);
+        }
+
+        if (NULL == (sshm->id = strdup(val))) {
+            fprintf(stderr, "sshm parser out of memory\n");
+            rc = ENOMEM;
+            goto out;
+        }
+    } else if (!strcmp(key, "role")) {
+        libxl_sshm_role new_role;
+
+        if (!strcmp("master", val)) {
+            new_role = LIBXL_SSHM_ROLE_MASTER;
+        } else if (!strcmp("slave", val)) {
+            new_role = LIBXL_SSHM_ROLE_SLAVE;
+        } else {
+            RET_INVAL("invalid role", val);
+        }
+
+        SET_VAL(sshm->role, "role", ROLE, new_role, val);
+    } else if (!strcmp(key, "begin") ||
+               !strcmp(key, "end") ||
+               !strcmp(key, "offset")) {
+        char *endptr;
+        int base = 10;
+        uint64_t new_addr;
+
+        /* Could be in hex form. Note that we don't need to check the length here,
+         * for val[] is NULL-terminated */
+        if ('0' == val[0] && 'x' == val[1]) { base = 16; }
+        new_addr = strtoull(val, &endptr, base);
+        if (ERANGE == errno || *endptr) {
+            RET_INVAL("invalid begin/end/offset", val);
+        }
+        if (new_addr & PAGE_SIZE_MASK)
+            RET_INVAL("begin/end/offset is not a multiple of 4K", val);
+
+        /* begin or end */
+        if ('b' == key[0]) {
+            SET_VAL(sshm->begin, "beginning address", RANGE, new_addr, val);
+        } else if('e' == key[0]){
+            SET_VAL(sshm->end, "ending address", RANGE, new_addr, val);
+        } else {
+            SET_VAL(sshm->offset, "offset", RANGE, new_addr, val);
+        }
+    } else if (!strcmp(key, "prot")) {
+        rc = parse_prot(cfg, val, &sshm->prot);
+        if (rc) { goto out; }
+    } else if (!strcmp(key, "cache_policy")) {
+        rc = parse_cachepolicy(cfg, val, &sshm->cache_policy);
+        if (rc) { goto out; }
+    } else {
+        RET_INVAL("invalid option", key);
+    }
+
+    rc = 0;
+
+ out:
+    return rc;
+}
+
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+                   libxl_static_shm *sshm)
+{
+    int rc;
+    regex_t equ_rec;
+    char *buf2 = NULL, *ptr = NULL;
+    regmatch_t pmatch[3];
+
+    rc = regcomp(&equ_rec, EQU_RE, REG_EXTENDED);
+    if (rc) {
+        fprintf(stderr, "sshm parser failed to initialize\n");
+        goto out;
+    }
+
+    if (NULL == (buf2 = ptr = strdup(spec))) {
+        fprintf(stderr, "sshm parser out of memory\n");
+        rc = ENOMEM;
+        goto out;
+    }
+
+    /* main parsing loop */
+    while (true) {
+        if (!*ptr) { break; }
+        if (regexec(&equ_rec, ptr, 3, pmatch, 0))
+            RET_INVAL("unrecognized token", ptr);
+
+        ptr[pmatch[1].rm_eo] = '\0';
+        ptr[pmatch[2].rm_eo] = '\0';
+        rc = handle_equ(cfg, ptr + pmatch[1].rm_so,
+                        ptr + pmatch[2].rm_so, sshm);
+        if (rc) { goto out; }
+
+        ptr += pmatch[0].rm_eo;
+    }
+
+    if (*ptr) { RET_INVAL("invalid syntax", ptr); }
+
+    /* do some early checks */
+    if (!sshm->id) {
+        RET_INVAL("id not specified", spec);
+    }
+    if (sshm->begin == LIBXL_SSHM_RANGE_UNKNOWN) {
+        RET_INVAL("begin address not specified", spec);
+    }
+    if (sshm->end == LIBXL_SSHM_RANGE_UNKNOWN) {
+        RET_INVAL("end address not specified", spec);
+    }
+    if (sshm->begin > sshm->end) {
+        RET_INVAL("begin address larger that end address", spec);
+    }
+
+    rc = 0;
+
+ out:
+    if (buf2) { free(buf2); }
+    regfree(&equ_rec);
+    return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
tools/libxl/libxlutil.h +6 -0
index e81b644c01..ee39cb5bdc 100644
@@ -118,6 +118,12 @@ int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char *str);
 int xlu_vif_parse_rate(XLU_Config *cfg, const char *rate,
                        libxl_device_nic *nic);
 
+/*
+ * static shared memory specification parsing
+ */
+int xlu_sshm_parse(XLU_Config *cfg, const char *spec,
+                   libxl_static_shm *sshm);
+
 #endif /* LIBXLUTIL_H */
 
 /*
tools/xl/xl_parse.c +16 -1
index 5c2bf17222..82d955b8b9 100644
@@ -813,7 +813,7 @@ void parse_config_data(const char *config_source,
     long l, vcpus = 0;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms,
-                   *usbctrls, *usbdevs, *p9devs;
+                   *usbctrls, *usbdevs, *p9devs, *sshms;
     XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs,
                    *mca_caps;
     int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian, num_mca_caps;
@@ -1392,6 +1392,28 @@ void parse_config_data(const char *config_source,
         }
     }
 
+    if (!xlu_cfg_get_list (config, "static_shm", &sshms, 0, 0)) {
+        d_config->num_sshms = 0;
+        d_config->sshms = NULL;
+        while ((buf = xlu_cfg_get_listitem (sshms, d_config->num_sshms)) != NULL) {
+            libxl_static_shm *sshm;
+            char *buf2 = strdup(buf);
+            int ret;
+
+            sshm = ARRAY_EXTEND_INIT_NODEVID(d_config->sshms,
+                                             d_config->num_sshms,
+                                             libxl_static_shm_init);
+            ret = xlu_sshm_parse(config, buf2, sshm);
+            if (ret) {
+                fprintf(stderr,
+                        "xl: Invalid argument for static_shm: %s", buf2);

tools/xl/xl_parse.c void parse_config_data(const char *config_source, · line 1419

Quoted 10 lines
tools/xl/xl_parse.c +7 -0
+                exit(EXIT_FAILURE);
+            }
+
+            free(buf2);
+        }
+    }
+
     if (!xlu_cfg_get_list(config, "p9", &p9devs, 0, 0)) {
         libxl_device_p9 *p9;
         char *security_model = NULL;

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#8 lore raw

Re: [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files

Hi,

Quoted 53 lines · Stefano Stabellini
Quoted 52 lines · Zhongze Liu

Add the parsing utils for the newly introduced libxl_static_sshm struct to the libxl/libxlu_* family. And add realated parsing code in xl to parse the struct from xl config files. This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile      |   2 +-
 tools/libxl/libxlu_sshm.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxlutil.h   |   6 ++
 tools/xl/xl_parse.c       |  24 +++++-
 4 files changed, 240 insertions(+), 2 deletions(-)
 create mode 100644 tools/libxl/libxlu_sshm.c
tools/libxl/Makefile +1 -1
index 082af8f716..3b63fb2cad 100644
@@ -175,7 +175,7 @@ AUTOINCS= libxlu_cfg_y.h libxlu_cfg_l.h _libxl_list.h _paths.h \
 AUTOSRCS= libxlu_cfg_y.c libxlu_cfg_l.c
 AUTOSRCS += _libxl_save_msgs_callout.c _libxl_save_msgs_helper.c
 LIBXLU_OBJS = libxlu_cfg_y.o libxlu_cfg_l.o libxlu_cfg.o \
-	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o
+	libxlu_disk_l.o libxlu_disk.o libxlu_vif.o libxlu_pci.o libxlu_sshm.o
 $(LIBXLU_OBJS): CFLAGS += $(CFLAGS_libxenctrl) # For xentoollog.h
 
 $(TEST_PROG_OBJS) _libxl.api-for-check: CFLAGS += $(CFLAGS_libxentoollog)
tools/libxl/libxlu_sshm.c new file +6 -0
new file mode 100644
index 0000000000..8647665213
@@ -0,0 +1,210 @@
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxlu_internal.h"
+
+#include <ctype.h>
+
+#define PARAM_RE(EXPR) "^\\s*" EXPR "\\s*(,|$)"
Quoted 8 lines

tools/libxl/libxlu_sshm.c line 10

Quoted 4 lines
tools/libxl/libxlu_sshm.c new file +4 -0
+#define WORD_RE         "([_a-zA-Z0-9]+)"
+#define EQU_RE         PARAM_RE(WORD_RE "\\s*=\\s*" WORD_RE)
+
+#define PAGE_SIZE_MASK ((uint64_t)0xfff)

You can probably use XC_PAGE_MASK that is already defined? Otherwise I would name this XLU_PAGE_MASK and move the definition to one of the libxlu headers.

I am not maintaining this code, but could we please avoid introducing new definition of PAGE_MASK?

This would make more difficult to support an hypervisor using a different page granularity.

Furthermore, 0xfff is not obvious to read. It makes more sense to define it in term of PAGE_SIZE.

Cheers,

Signature · 6 lines
Julien Grall

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#9 lore raw

PATCH 4/6 xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

In reply to Zhongze Liu

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the target domain and source domain, grant the access.

References to this xsm check have also been updated to pass the current domain as a new parameter.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
Cc George Dunlap <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Andrew Cooper <[email protected]>
Cc Daniel De Graaf <[email protected]>
---
 xen/arch/arm/mm.c       | 2 +-
 xen/arch/x86/mm/p2m.c   | 2 +-
 xen/include/xsm/dummy.h | 6 ++++--
 xen/include/xsm/xsm.h   | 7 ++++---
 xen/xsm/flask/hooks.c   | 6 ++++--
 5 files changed, 14 insertions(+), 9 deletions(-)
xen/arch/arm/mm.c +1 -1
index a810a056d7..9ec78d8c03 100644
@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);
xen/arch/x86/mm/p2m.c +1 -1
index e8a57d118c..a547fd00c0 100644
@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 
-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);
     if ( rc )
         goto out;
 
xen/include/xsm/dummy.h +4 -2
index 62fcea6f04..28dbc6f2a2 100644
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }
 
 static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
xen/include/xsm/xsm.h +4 -3
index 60c0fd6a62..a20654a803 100644
@@ -85,7 +85,7 @@ struct xsm_operations {
     int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page);
     int (*add_to_physmap) (struct domain *d1, struct domain *d2);
     int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
-    int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+    int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t);
     int (*claim_pages) (struct domain *d);
 
     int (*console_io) (struct domain *d, int cmd);
@@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1,
     return xsm_ops->remove_from_physmap(d1, d2);
 }
 
-static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t)
+static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd,
+                                        struct domain *d, struct domain *t)
 {
-    return xsm_ops->map_gmfn_foreign(d, t);
+    return xsm_ops->map_gmfn_foreign(cd, d, t);
 }
 
 static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
xen/xsm/flask/hooks.c +4 -2
index 91146275bb..3408b6b9e1 100644
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }
 
 static int flask_hvm_param(struct domain *d, unsigned long op)
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#10 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 7 lines · Zhongze Liu

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the

^ IFF

Quoted 59 lines

target domain and source domain, grant the access.

References to this xsm check have also been updated to pass the current domain as a new parameter.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
Cc George Dunlap <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Andrew Cooper <[email protected]>
Cc Daniel De Graaf <[email protected]>
---
 xen/arch/arm/mm.c       | 2 +-
 xen/arch/x86/mm/p2m.c   | 2 +-
 xen/include/xsm/dummy.h | 6 ++++--
 xen/include/xsm/xsm.h   | 7 ++++---
 xen/xsm/flask/hooks.c   | 6 ++++--
 5 files changed, 14 insertions(+), 9 deletions(-)
xen/arch/arm/mm.c +1 -1
index a810a056d7..9ec78d8c03 100644
@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);
xen/arch/x86/mm/p2m.c +1 -1
index e8a57d118c..a547fd00c0 100644
@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 
-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);
     if ( rc )
         goto out;
 
xen/include/xsm/dummy.h +0 -0
index 62fcea6f04..28dbc6f2a2 100644
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 533

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
 }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);

We need to preserve the returned errors:

  rc = xsm_default_action(action, cd, d);
  if (rc) return rc;
  return xsm_default_action(action, cd, t);
Quoted 35 lines
xen/include/xsm/dummy.h +0 -0
 }
 
 static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
xen/include/xsm/xsm.h +4 -3
index 60c0fd6a62..a20654a803 100644
@@ -85,7 +85,7 @@ struct xsm_operations {
     int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page);
     int (*add_to_physmap) (struct domain *d1, struct domain *d2);
     int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
-    int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+    int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t);
     int (*claim_pages) (struct domain *d);
 
     int (*console_io) (struct domain *d, int cmd);
@@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1,
     return xsm_ops->remove_from_physmap(d1, d2);
 }
 
-static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t)
+static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd,
+                                        struct domain *d, struct domain *t)
 {
-    return xsm_ops->map_gmfn_foreign(d, t);
+    return xsm_ops->map_gmfn_foreign(cd, d, t);
 }
 
 static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
xen/xsm/flask/hooks.c +0 -0
index 91146275bb..3408b6b9e1 100644
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 10 lines
xen/xsm/flask/hooks.c +4 -2
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

Same here:

  rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
  if (rc) return rc;
  return domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);

Also, I just want to point out that in the regular case cd and d are one and the same. The code assumes that domain_has_perm returns 0 in that case. I think that is correct, but I don't know enough about XSM to be sure about it.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#11 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Hi Stefano,

2017-08-23 3:58 GMT+08:00 Stefano Stabellini <[email protected]>:

Quoted 83 lines · Zhongze Liu
Quoted 7 lines

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the

^ IFF

Quoted 59 lines

target domain and source domain, grant the access.

References to this xsm check have also been updated to pass the current domain as a new parameter.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
Cc George Dunlap <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Andrew Cooper <[email protected]>
Cc Daniel De Graaf <[email protected]>
---
 xen/arch/arm/mm.c       | 2 +-
 xen/arch/x86/mm/p2m.c   | 2 +-
 xen/include/xsm/dummy.h | 6 ++++--
 xen/include/xsm/xsm.h   | 7 ++++---
 xen/xsm/flask/hooks.c   | 6 ++++--
 5 files changed, 14 insertions(+), 9 deletions(-)
xen/arch/arm/mm.c +1 -1
index a810a056d7..9ec78d8c03 100644
@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);
xen/arch/x86/mm/p2m.c +1 -1
index e8a57d118c..a547fd00c0 100644
@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 
-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);
     if ( rc )
         goto out;
 
xen/include/xsm/dummy.h +0 -0
index 62fcea6f04..28dbc6f2a2 100644
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 533

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
 }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);

We need to preserve the returned errors:

Quoted 3 lines
  rc = xsm_default_action(action, cd, d);
  if (rc) return rc;
  return xsm_default_action(action, cd, t);

OK, will correct this.

Quoted 51 lines
Quoted 38 lines
xen/include/xsm/dummy.h +0 -0
 }
 
 static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
xen/include/xsm/xsm.h +4 -3
index 60c0fd6a62..a20654a803 100644
@@ -85,7 +85,7 @@ struct xsm_operations {
     int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page);
     int (*add_to_physmap) (struct domain *d1, struct domain *d2);
     int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
-    int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+    int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t);
     int (*claim_pages) (struct domain *d);
 
     int (*console_io) (struct domain *d, int cmd);
@@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1,
     return xsm_ops->remove_from_physmap(d1, d2);
 }
 
-static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t)
+static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd,
+                                        struct domain *d, struct domain *t)
 {
-    return xsm_ops->map_gmfn_foreign(d, t);
+    return xsm_ops->map_gmfn_foreign(cd, d, t);
 }
 
 static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
xen/xsm/flask/hooks.c +0 -0
index 91146275bb..3408b6b9e1 100644
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 10 lines
xen/xsm/flask/hooks.c +4 -2
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

Same here:

Quoted 8 lines
  rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
  if (rc) return rc;
  return domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);

Also, I just want to point out that in the regular case cd and d are one and the same. The code assumes that domain_has_perm returns 0 in that case. I think that is correct, but I don't know enough about XSM to be sure about it.

I also assume that domain_has_perm returns 0 when cd == d, but let's wait for other
maintainers' comments.

Cheers,

Zhongze Liu

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#12 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 147 lines · Zhongze Liu

Hi Stefano,

2017-08-23 3:58 GMT+08:00 Stefano Stabellini <[email protected]>:

Quoted 83 lines · Zhongze Liu
Quoted 7 lines

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the

^ IFF

Quoted 59 lines

target domain and source domain, grant the access.

References to this xsm check have also been updated to pass the current domain as a new parameter.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
Cc George Dunlap <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Andrew Cooper <[email protected]>
Cc Daniel De Graaf <[email protected]>
---
  xen/arch/arm/mm.c       | 2 +-
  xen/arch/x86/mm/p2m.c   | 2 +-
  xen/include/xsm/dummy.h | 6 ++++--
  xen/include/xsm/xsm.h   | 7 ++++---
  xen/xsm/flask/hooks.c   | 6 ++++--
  5 files changed, 14 insertions(+), 9 deletions(-)
xen/arch/arm/mm.c +1 -1
index a810a056d7..9ec78d8c03 100644
@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
              return -EINVAL;
          }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
          if ( rc )
          {
              rcu_unlock_domain(od);
xen/arch/x86/mm/p2m.c +1 -1
index e8a57d118c..a547fd00c0 100644
@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
      if ( tdom == fdom )
          goto out;
 
-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);
      if ( rc )
          goto out;
 
xen/include/xsm/dummy.h +0 -0
index 62fcea6f04..28dbc6f2a2 100644
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
      return xsm_default_action(action, d1, d2);

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 533

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
  }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);

We need to preserve the returned errors:

Quoted 3 lines
   rc = xsm_default_action(action, cd, d);
   if (rc) return rc;
   return xsm_default_action(action, cd, t);

OK, will correct this.

Quoted 55 lines
Quoted 38 lines
xen/include/xsm/dummy.h +0 -0
  }
 
  static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
xen/include/xsm/xsm.h +4 -3
index 60c0fd6a62..a20654a803 100644
@@ -85,7 +85,7 @@ struct xsm_operations {
      int (*memory_pin_page) (struct domain *d1, struct domain *d2, struct page_info *page);
      int (*add_to_physmap) (struct domain *d1, struct domain *d2);
      int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
-    int (*map_gmfn_foreign) (struct domain *d, struct domain *t);
+    int (*map_gmfn_foreign) (struct domain *cd, struct domain *d, struct domain *t);
      int (*claim_pages) (struct domain *d);
 
      int (*console_io) (struct domain *d, int cmd);
@@ -372,9 +372,10 @@ static inline int xsm_remove_from_physmap(xsm_default_t def, struct domain *d1,
      return xsm_ops->remove_from_physmap(d1, d2);
  }
 
-static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, struct domain *t)
+static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *cd,
+                                        struct domain *d, struct domain *t)
  {
-    return xsm_ops->map_gmfn_foreign(d, t);
+    return xsm_ops->map_gmfn_foreign(cd, d, t);
  }
 
  static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
xen/xsm/flask/hooks.c +0 -0
index 91146275bb..3408b6b9e1 100644
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
      return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 10 lines
xen/xsm/flask/hooks.c +4 -2
  }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
  {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
  }

Same here:

   rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
   if (rc) return rc;
   return domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
Quoted 8 lines
Quoted 4 lines

Also, I just want to point out that in the regular case cd and d are one and the same. The code assumes that domain_has_perm returns 0 in that case. I think that is correct, but I don't know enough about XSM to be sure about it.

I also assume that domain_has_perm returns 0 when cd == d, but let's wait for other
maintainers' comments.

While this permission check with (cd == d) should succeed in all sane policies, it's faster to compare for equality than to look up the access vector.

In addition, I think it would be useful to have a check that (d) and (t) can share memory (so that a security policy could be written preventing them from communicating directly). Normally, this would be allowed between all domains that allow grant mapping/event channels.

    rc = domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
    if (rc) return rc;

To allow this in the policy the same as grants:

tools/flask/policy/modules/xen.if +2 -0
@@ -127,6 +127,8 @@ define(`domain_comms', `
        domain_event_comms($1, $2)
        allow $1 $2:grant { map_read map_write copy unmap };
        allow $2 $1:grant { map_read map_write copy unmap };
+       allow $1 $2:mmu share_mem;
+       allow $2 $1:mmu share_mem;
 ')
 
 

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#13 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

In reply to Zhongze Liu
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the target domain and source domain, grant the access.

You say "and here", yet ...

Quoted 5 lines
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

Quoted 4 lines
xen/xsm/flask/hooks.c +0 -0
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 10 lines
xen/xsm/flask/hooks.c +4 -2
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

... here. A domain can't have XSM_TARGET permission over two other domains, so what you want to do here can't work at all, afaict.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#14 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 36 lines · Jan Beulich
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the target domain and source domain, grant the access.

You say "and here", yet ...

Quoted 5 lines
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

Quoted 6 lines
xen/xsm/flask/hooks.c +0 -0
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
Quoted 12 lines

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 8 lines
xen/xsm/flask/hooks.c +4 -2
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

... here. A domain can't have XSM_TARGET permission over two other domains, so what you want to do here can't work at all, afaict.

It would work with XSM_TARGET if cd == d, and cd has XSM_TARGET permission over t (current case). Otherwise, it would work if cd is XSM_PRIV (Zhongze's case). Did I get it wrong?

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#15 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 51 lines
Quoted 1 line
Quoted 1 line · On 23.08.17 at 19:16, <[email protected]> wrote:
Quoted 36 lines · Jan Beulich
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the target domain and source domain, grant the access.

You say "and here", yet ...

Quoted 5 lines
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 10 lines
xen/include/xsm/dummy.h +4 -2
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

Quoted 6 lines
xen/xsm/flask/hooks.c +0 -0
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
Quoted 12 lines

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 8 lines
xen/xsm/flask/hooks.c +4 -2
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

... here. A domain can't have XSM_TARGET permission over two other domains, so what you want to do here can't work at all, afaict.

Quoted 3 lines

It would work with XSM_TARGET if cd == d, and cd has XSM_TARGET permission over t (current case). Otherwise, it would work if cd is XSM_PRIV (Zhongze's case). Did I get it wrong?

I think so, yes, but besides the "and" vs "or" discrepancy the patch description suggests a three-way operation (i.e. including the case of all three domains being different ones). The case you describe doesn't require three domains to be passed into the hook, it would
- afaict - just be the traditional "cd is XSM_TARGET over t" case (matching for example xsm_evtchn_{status,unbound}()).

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#16 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

In reply to Jan Beulich

Hi Jan,

Thanks for reviewing my patch.

2017-08-23 17:55 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 18 lines
Quoted 9 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:

The original xsm_map_gmfn_foregin policy checks if source domain has the proper privileges over the target domain. Under this policy, it's not allowed if a Dom0 wants to map pages from one DomU to another, this restricts some useful yet not dangerous usages of the API, such as sharing pages among DomU's by calling XENMEM_add_to_physmap from Dom0.

Change the policy to: IIF current domain has the proper privilege on the target domain and source domain, grant the access.

You say "and here", yet ...

Quoted 6 lines
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }
 
Quoted 11 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 9 lines
xen/include/xsm/dummy.h +4 -2
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.)

But as Stefano has pointed out, I should preserve the error code. And as Daniel has pointed out, I should also check if d and t can share memory.

Quoted 7 lines
Quoted 7 lines
xen/xsm/flask/hooks.c +0 -0
@@ -1165,9 +1165,11 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
Quoted 12 lines

xen/xsm/flask/hooks.c static int flask_remove_from_physmap(struct domain *d1, struct domain *d2) · line 1173

Quoted 8 lines
xen/xsm/flask/hooks.c +4 -2
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,
+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    return domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ||
+        domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
 }

... here. A domain can't have XSM_TARGET permission over two other domains, so what you want to do here can't work at all, afaict.

I agree with what Stefano has said below.

Cheers,

Zhongze Liu.

Quoted 3 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#17 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 21 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 02:51, <[email protected]> wrote:

2017-08-23 17:55 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }
 
Quoted 11 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 9 lines
xen/include/xsm/dummy.h +4 -2
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

Quoted 6 lines

This might be confusing. But think of returning 0 as "allowed", the only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.)

But as Stefano has pointed out, I should preserve the error code.

Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)?

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#18 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Hi Jan,

2017-08-24 14:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 26 lines
Quoted 26 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 02:51, <[email protected]> wrote:

2017-08-23 17:55 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }
 
Quoted 11 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 9 lines
xen/include/xsm/dummy.h +4 -2
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.)

Quoted 6 lines
Quoted 1 line

But as Stefano has pointed out, I should preserve the error code.

Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)?

This was my original plan, but I'm not very sure about this, so I turn to Julien for help, and...
Here is part of the irc log from a discussion with Julien on #xendevel, where Julien said:

blackskygg: I think you want to pass the current domain in
parameter, i.e having 3 domains argument.
because your solution only works when XSM is not enabled (this is
the dummy callback)
when XSM is enabled, the policy would be specificed by the administrator he needs to be able to know which domain was doing the configuration.

Cheers,

Zhongze Liu

Quoted 3 lines

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#19 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 46 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 13:33, <[email protected]> wrote:

Hi Jan,

2017-08-24 14:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 02:51, <[email protected]> wrote:

2017-08-23 17:55 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 4 lines

xen/include/xsm/dummy.h static XSM_INLINE int

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int 

xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 4 lines
xen/include/xsm/dummy.h +0 -1
     return xsm_default_action(action, d1, d2);
 }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 

*d, struct domain *t)

Quoted 1 line
Quoted 1 line
Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 528

Quoted 1 line
xen/include/xsm/dummy.h +1 -0
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain 

*cd,

Quoted 1 line
Quoted 1 line
Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 529

Quoted 1 line
xen/include/xsm/dummy.h +1 -0
+                                           struct domain *d, struct domain 

*t)

Quoted 14 lines
Quoted 14 lines
Quoted 8 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 6 lines
xen/include/xsm/dummy.h +2 -1
 {
     XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
 }

... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.)

Quoted 6 lines
Quoted 1 line

But as Stefano has pointed out, I should preserve the error code.

Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)?

This was my original plan, but I'm not very sure about this, so I turn to Julien for help, and...
Here is part of the irc log from a discussion with Julien on #xendevel, where Julien said:

Quoted 6 lines

blackskygg: I think you want to pass the current domain in
parameter, i.e having 3 domains argument.
because your solution only works when XSM is not enabled (this is
the dummy callback)
when XSM is enabled, the policy would be specificed by the administrator he needs to be able to know which domain was doing the configuration.

in flask/hooks.c there are quite a few uses of avc_current_has_perm() in such cases, so I would think not handing current->domain through the hook should be fine. But of course Daniel may tell you I'm completely wrong here.

Jan

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#20 lore raw

Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin

Quoted 53 lines · Jan Beulich
Quoted 46 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 13:33, <[email protected]> wrote:

Hi Jan,

2017-08-24 14:37 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Quoted 1 line · On 24.08.17 at 02:51, <[email protected]> wrote:

2017-08-23 17:55 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 4 lines

xen/include/xsm/dummy.h static XSM_INLINE int

Quoted 4 lines
Quoted 1 line
Quoted 1 line · On 22.08.17 at 20:08, <[email protected]> wrote:
xen/include/xsm/dummy.h +0 -0
@@ -525,10 +525,12 @@ static XSM_INLINE int

xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,

Quoted 4 lines
xen/include/xsm/dummy.h +0 -1
      return xsm_default_action(action, d1, d2);
  }
 
-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain

*d, struct domain *t)

Quoted 1 line
Quoted 1 line
Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 528

Quoted 1 line
xen/include/xsm/dummy.h +1 -0
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain

*cd,

Quoted 1 line
Quoted 1 line
Quoted 1 line

xen/include/xsm/dummy.h static XSM_INLINE int · line 529

Quoted 1 line
xen/include/xsm/dummy.h +1 -0
+                                           struct domain *d, struct domain

*t)

Quoted 14 lines
Quoted 14 lines
Quoted 8 lines

xen/include/xsm/dummy.h static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1, · line 534

Quoted 6 lines
xen/include/xsm/dummy.h +2 -1
  {
      XSM_ASSERT_ACTION(XSM_TARGET);
-    return xsm_default_action(action, d, t);
+    return xsm_default_action(action, cd, d) ||
+        xsm_default_action(action, cd, t);
  }

... you use "or" here and ...

This might be confusing. But think of returning 0 as "allowed", the only condition where this
statement returns a 0 is when both calls return 0 -- so it's actually an "and". (Think of de-morgan's law.)

Quoted 6 lines
Quoted 1 line

But as Stefano has pointed out, I should preserve the error code.

Ah, right - the code as written suggests boolean return values, which gives it the wrong look. You really mean ?: instead of ||. Why do you, btw, pass in current->domain (as cd) instead of obtaining it here (just like various other hooks do)?

This was my original plan, but I'm not very sure about this, so I turn to Julien for help, and...
Here is part of the irc log from a discussion with Julien on #xendevel, where Julien said:

Quoted 6 lines

blackskygg: I think you want to pass the current domain in
parameter, i.e having 3 domains argument.
because your solution only works when XSM is not enabled (this is
the dummy callback)
when XSM is enabled, the policy would be specificed by the administrator he needs to be able to know which domain was doing the configuration.

Quoted 6 lines

in flask/hooks.c there are quite a few uses of avc_current_has_perm() in such cases, so I would think not handing current->domain through the hook should be fine. But of course Daniel may tell you I'm completely wrong here.

Jan

This is really just a choice of whatever looks better. There's a very minor performance penalty from not calling current->domain over and over, but there might also be a performance gain if current_has_perm is not inlined and the call results in smaller code size.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#21 lore raw

PATCH 5/6 libxl: support mapping static shared memory areas during domain creation

In reply to Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, no master domain etc.
* Write infomation of static shared memory areas into the appropriate xenstore paths.
* use xc_domain_add_to_physmap_batch to do the page sharing.

Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foregin for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile         |   2 +-
 tools/libxl/libxl_arch.h     |   6 +
 tools/libxl/libxl_arm.c      |  15 ++
 tools/libxl/libxl_create.c   |  27 ++++
 tools/libxl/libxl_internal.h |  14 ++
 tools/libxl/libxl_sshm.c     | 336 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_x86.c      |  18 +++
 tools/libxl/libxl_xshelp.c   |   8 ++
 8 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_sshm.c
tools/libxl/Makefile +1 -1
index 3b63fb2cad..fd624b28f3 100644
@@ -138,7 +138,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
 			libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \
 			libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \
-			libxl_9pfs.o libxl_domain.o \
+			libxl_9pfs.o libxl_domain.o libxl_sshm.o \
                         $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
tools/libxl/libxl_arch.h +6 -0
index 5e1fc6060e..1d681d8863 100644
@@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
 
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);
+
+_hidden
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm);
+
 #if defined(__i386__) || defined(__x86_64__)
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
tools/libxl/libxl_arm.c +15 -0
index d842d888eb..0975109c0c 100644
@@ -1065,6 +1065,21 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, false);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    return true;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+    if (sshm->cache_policy >= LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_create.c +27 -0
index 1158303e1a..8e5ec486d2 100644
@@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
         ret = ERROR_INVAL;
         goto out;
     }
+
+    /* the p2m has been setup, we could map the static shared memory now. */
+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
     return ret;
@@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;
+
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
+            goto error_out;
+        }
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
tools/libxl/libxl_internal.h +14 -0
index 724750967c..74bc0acb21 100644
@@ -721,6 +721,7 @@ _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
                                    const char *path, unsigned int *nb);
    /* On error: returns NULL, sets errno (no logging) */
 _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+_hidden char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id);
 
 _hidden int libxl__backendpath_parse_domid(libxl__gc *gc, const char *be_path,
                                            libxl_domid *domid_out);
@@ -4352,6 +4353,19 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 }
 #endif
 
+/*
+ * Set up static shared ram pages for HVM domains to communicate
+ *
+ * This function should only be called after the memory map is constructed
+ * and before any further memory access. */
+_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
+                            libxl_static_shm *sshm, int len);
+
+_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                      libxl_static_shm *sshms, int len);
+_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                   libxl_static_shm *sshm);
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_sshm.c new file +336 -0

336 lines not on this page · raw message

tools/libxl/libxl_x86.c +18 -0
index 455f6f0bed..8dd34fd7ba 100644
@@ -587,6 +587,24 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    /* FIXME: Mark this as unsupported since calling p2m_add_foreign on two
+     * DomU's is currently not allowd on x86, see the comments in 
+     * x86/mm/p2m.c: p2m_add_foreign */
+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_xshelp.c +8 -0
index c4a18df353..d91fbf5fda 100644
@@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
     return s;
 }
 
+char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
+{
+    char *s = GCSPRINTF("/local/static_shm/%s", id);
+    if (!s)
+        LOGE(ERROR, "cannot allocate static shm path");
+    return s;
+}
+
 int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char **result_out)
 {
Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#22 lore raw

Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

Quoted 122 lines · Zhongze Liu

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, no master domain etc.
* Write infomation of static shared memory areas into the appropriate xenstore paths.
* use xc_domain_add_to_physmap_batch to do the page sharing.

Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foregin for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile         |   2 +-
 tools/libxl/libxl_arch.h     |   6 +
 tools/libxl/libxl_arm.c      |  15 ++
 tools/libxl/libxl_create.c   |  27 ++++
 tools/libxl/libxl_internal.h |  14 ++
 tools/libxl/libxl_sshm.c     | 336 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_x86.c      |  18 +++
 tools/libxl/libxl_xshelp.c   |   8 ++
 8 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_sshm.c
tools/libxl/Makefile +1 -1
index 3b63fb2cad..fd624b28f3 100644
@@ -138,7 +138,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
 			libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
 			libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \
 			libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \
-			libxl_9pfs.o libxl_domain.o \
+			libxl_9pfs.o libxl_domain.o libxl_sshm.o \
                         $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
tools/libxl/libxl_arch.h +6 -0
index 5e1fc6060e..1d681d8863 100644
@@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
 
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);
+
+_hidden
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm);
+
 #if defined(__i386__) || defined(__x86_64__)
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
tools/libxl/libxl_arm.c +15 -0
index d842d888eb..0975109c0c 100644
@@ -1065,6 +1065,21 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, false);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    return true;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+    if (sshm->cache_policy >= LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_create.c +8 -0
index 1158303e1a..8e5ec486d2 100644
@@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
         ret = ERROR_INVAL;
         goto out;
     }
+
+    /* the p2m has been setup, we could map the static shared memory now. */
+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
     return ret;
@@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 

tools/libxl/libxl_create.c static void initiate_domain_create(libxl__egc *egc, · line 938

Quoted 10 lines
tools/libxl/libxl_create.c +10 -0
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;

I think it makes sense to call libxl__sshm_check_overlap only if num_sshms != 0.

Quoted 133 lines
tools/libxl/libxl_create.c +8 -0
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
+            goto error_out;
+        }
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
tools/libxl/libxl_internal.h +14 -0
index 724750967c..74bc0acb21 100644
@@ -721,6 +721,7 @@ _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
                                    const char *path, unsigned int *nb);
    /* On error: returns NULL, sets errno (no logging) */
 _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+_hidden char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id);
 
 _hidden int libxl__backendpath_parse_domid(libxl__gc *gc, const char *be_path,
                                            libxl_domid *domid_out);
@@ -4352,6 +4353,19 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 }
 #endif
 
+/*
+ * Set up static shared ram pages for HVM domains to communicate
+ *
+ * This function should only be called after the memory map is constructed
+ * and before any further memory access. */
+_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
+                            libxl_static_shm *sshm, int len);
+
+_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                      libxl_static_shm *sshms, int len);
+_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                   libxl_static_shm *sshm);
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_sshm.c new file +84 -0
new file mode 100644
index 0000000000..e16c24ccb9
@@ -0,0 +1,336 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_ERROR(domid, sshmid, f, ...)                               \
+    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN) {
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    }
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN) {
+        sshm->prot = LIBXL_SSHM_PROT_RW;
+    }
+    /* role-specific checks */
+    if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {
+        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN) {
+            sshm->offset = 0;
+        }
+        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache_policy is only applicable to master domains");
+            return ERROR_INVAL;
+        }
+    } else {
+        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "offset is only applicable to slave domains");
+            return ERROR_INVAL;
+        }
+        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+        if (rc) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache policy not supported on this platform");
+            return ERROR_INVAL;
+        }
+    }
+
+    return 0;
+}
+
+/* Compare function for sorting sshm ranges by sshm->begin */
+static int sshm_range_cmp(const void *a, const void *b)
+{
+    libxl_static_shm *const *sshma = a, *const *sshmb = b;
+    return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
+}
+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role)
+            slave_sshms[num_slaves++] = sshms + i;
+    }
+    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
+
+    for (i = 0; i < num_slaves - 1; ++i) {
+        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
+            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges overlap.");
+            return ERROR_INVAL;
+        }
+    }
+
+    return 0;
+}
+
+/* The caller have to guarentee that sshm->begin < sshm->end */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm,

tools/libxl/libxl_sshm.c line 94

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+                              uint64_t mbegin, uint64_t mend)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (mend - mbegin) >> 12;

Please don't hardcode 12, please use XC_PAGE_SHIFT (also in other libxl patches).

Quoted 104 lines
tools/libxl/libxl_sshm.c new file +104 -0
+    num_spages = (sshm->end - sshm->begin) >> 12;
+    offset = sshm->offset >> 12;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (mbegin >> 12) + offset + i;
+        gpfns[i]= (sshm->begin >> 12) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    for (i = 0; i< num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       sshm->begin + (offset << 12) );
+            rc = ERROR_FAIL;
+        }
+    }
+    if (rc) goto out;
+
+    rc = 0;
+
+out:
+    return rc;
+}
+
+static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
+                                 libxl_static_shm *sshm)
+{
+    int rc;
+    char *sshm_path, *slave_path, *dom_path, *dom_sshm_path, *dom_role_path;
+    char *ents[9];
+    const char *xs_value;
+    libxl_static_shm master_sshm;
+    uint32_t master_domid;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, sshm->id);
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_sshm_path = GCSPRINTF("%s/static_shm/%s", dom_path, sshm->id);
+    dom_role_path = GCSPRINTF("%s/role", dom_sshm_path);
+
+    /* prepare the slave xenstore entries */
+    ents[0] = "begin";
+    ents[1] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[2] = "end";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[4] = "offset";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->offset);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = NULL;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            SSHM_ERROR(domid, sshm->id, "no master found.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* every ID can appear in each domain at most once */
+        if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+            SSHM_ERROR(domid, sshm->id,
+                       "domain tried to map the same ID twice.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* look at the master info and see if we could do the mapping */
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/prot", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        libxl_sshm_prot_from_string(xs_value, &master_sshm.prot);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/begin", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_sshm.begin = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/end", sshm_path),
+                                    &xs_value);

tools/libxl/libxl_sshm.c line 208

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+        if (rc) goto out;
+        master_sshm.end = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/master", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_domid = strtoull(xs_value, NULL, 16);
+
+        /* check if the slave is asking too much permission */

this comment doesn't seem to match the check below

tools/libxl/libxl_sshm.c line 220

Quoted 12 lines
tools/libxl/libxl_sshm.c new file +12 -0
+        if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
+            sshm->prot = master_sshm.prot;
+        }
+        if (master_sshm.prot < sshm->prot) {
+            SSHM_ERROR(domid, sshm->id, "slave is asking too much permission.");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        /* all checks passed, do the job */
+        rc = libxl__sshm_do_map(gc, master_domid, domid, sshm,
+                                master_sshm.begin, master_sshm.end);

Doesn't libxl__sshm_do_map risk being called twice on xenstore transaction retries?

Quoted 154 lines
tools/libxl/libxl_sshm.c new file +116 -0
+        if (rc) {
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        /* write the result to xenstore and commit */
+        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+        if (rc) goto out;
+        libxl__xs_writev(gc, xt, slave_path, ents);
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+    char *sshm_path, *dom_path, *dom_role_path;
+    char *ents[13];
+    struct xs_permissions noperm;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, sshm->id);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
+
+    /* prepare the xenstore entries */
+    ents[0] = "master";
+    ents[1] = GCSPRINTF("%"PRIu32, domid);
+    ents[2] = "begin";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[4] = "end";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = "cache_policy";
+    ents[9] = libxl__strdup(gc,
+                            libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
+    ents[10] = "status";
+    ents[11] = "alive";
+    ents[12] = NULL;
+
+    /* could only be accessed by Dom0 */
+    noperm.id = 0;
+    noperm.perms = XS_PERM_NONE;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            /* every ID can appear in each domain at most once */
+            if (libxl__xs_read(gc, xt, dom_role_path)) {
+                SSHM_ERROR(domid, sshm->id,
+                           "domain tried to map the same ID twice.");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
+            if (rc) goto out;;
+
+            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
+            libxl__xs_writev(gc, xt, sshm_path, ents);
+        } else {
+            SSHM_ERROR(domid, sshm->id, "can only have one master.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
+int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
+                    libxl_static_shm *sshms, int len)
+{
+    int rc, i;
+
+    if (!len) return 0;
+
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role) {
+            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
+        } else {
+            rc = libxl__sshm_add_master(gc, domid, sshms+i);
+        }
+        if (rc)  return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
tools/libxl/libxl_x86.c +18 -0
index 455f6f0bed..8dd34fd7ba 100644
@@ -587,6 +587,24 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    /* FIXME: Mark this as unsupported since calling p2m_add_foreign on two
+     * DomU's is currently not allowd on x86, see the comments in 
+     * x86/mm/p2m.c: p2m_add_foreign */
+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_xshelp.c +1 -0
index c4a18df353..d91fbf5fda 100644
@@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
     return s;
 }
 
+char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)

tools/libxl/libxl_xshelp.c char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid) · line 206

Quoted 10 lines
tools/libxl/libxl_xshelp.c +7 -0
+{
+    char *s = GCSPRINTF("/local/static_shm/%s", id);
+    if (!s)
+        LOGE(ERROR, "cannot allocate static shm path");
+    return s;
+}
+
 int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char **result_out)
 {

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#23 lore raw

Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

Hi Stefano,

2017-08-23 5:42 GMT+08:00 Stefano Stabellini <[email protected]>:

Quoted 123 lines · Zhongze Liu
Quoted 122 lines

Add libxl__sshm_add to map shared pages from one DomU to another, The mapping process involves the follwing steps:

* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain, no master domain etc.
* Write infomation of static shared memory areas into the appropriate xenstore paths.
* use xc_domain_add_to_physmap_batch to do the page sharing.

Temporarily mark this as unsupported on x86 because calling p2m_add_foregin on two domU's is currently not allowd on x86 (see the comments in x86/mm/p2m.c:p2m_add_foregin for more details).

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Andrew Cooper <[email protected]>
Cc George Dunlap <[email protected]>
Cc Ian Jackson <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Tim Deegan <[email protected]>
Cc Wei Liu <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/Makefile         |   2 +-
 tools/libxl/libxl_arch.h     |   6 +
 tools/libxl/libxl_arm.c      |  15 ++
 tools/libxl/libxl_create.c   |  27 ++++
 tools/libxl/libxl_internal.h |  14 ++
 tools/libxl/libxl_sshm.c     | 336 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_x86.c      |  18 +++
 tools/libxl/libxl_xshelp.c   |   8 ++
 8 files changed, 425 insertions(+), 1 deletion(-)
 create mode 100644 tools/libxl/libxl_sshm.c
tools/libxl/Makefile +1 -1
index 3b63fb2cad..fd624b28f3 100644
@@ -138,7 +138,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o libxl_pci.o \
                      libxl_dom_suspend.o libxl_dom_save.o libxl_usb.o \
                      libxl_vtpm.o libxl_nic.o libxl_disk.o libxl_console.o \
                      libxl_cpupool.o libxl_mem.o libxl_sched.o libxl_tmem.o \
-                     libxl_9pfs.o libxl_domain.o \
+                     libxl_9pfs.o libxl_domain.o libxl_sshm.o \
                         $(LIBXL_OBJS-y)
 LIBXL_OBJS += libxl_genid.o
 LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
tools/libxl/libxl_arch.h +6 -0
index 5e1fc6060e..1d681d8863 100644
@@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
 
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);
+
+_hidden
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm);
+
 #if defined(__i386__) || defined(__x86_64__)
 
 #define LAPIC_BASE_ADDRESS  0xfee00000
tools/libxl/libxl_arm.c +15 -0
index d842d888eb..0975109c0c 100644
@@ -1065,6 +1065,21 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, false);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    return true;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_ARM_NORMAL;
+    if (sshm->cache_policy >= LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_create.c +8 -0
index 1158303e1a..8e5ec486d2 100644
@@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
         ret = ERROR_INVAL;
         goto out;
     }
+
+    /* the p2m has been setup, we could map the static shared memory now. */
+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
     return ret;
@@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
Quoted 13 lines

tools/libxl/libxl_create.c static void initiate_domain_create(libxl__egc *egc, · line 938

Quoted 10 lines
tools/libxl/libxl_create.c +10 -0
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;

I think it makes sense to call libxl__sshm_check_overlap only if num_sshms != 0.

Yes.
But I prefer to do the check inside libxl__sshm_check_overlap. Just like what libxl__sshm_add does.

Quoted 140 lines
Quoted 140 lines
tools/libxl/libxl_create.c +8 -0
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
+            goto error_out;
+        }
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
tools/libxl/libxl_internal.h +14 -0
index 724750967c..74bc0acb21 100644
@@ -721,6 +721,7 @@ _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
                                    const char *path, unsigned int *nb);
    /* On error: returns NULL, sets errno (no logging) */
 _hidden char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid);
+_hidden char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id);
 
 _hidden int libxl__backendpath_parse_domid(libxl__gc *gc, const char *be_path,
                                            libxl_domid *domid_out);
@@ -4352,6 +4353,19 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 }
 #endif
 
+/*
+ * Set up static shared ram pages for HVM domains to communicate
+ *
+ * This function should only be called after the memory map is constructed
+ * and before any further memory access. */
+_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
+                            libxl_static_shm *sshm, int len);
+
+_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                      libxl_static_shm *sshms, int len);
+_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                   libxl_static_shm *sshm);
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_sshm.c new file +89 -0
new file mode 100644
index 0000000000..e16c24ccb9
@@ -0,0 +1,336 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_ERROR(domid, sshmid, f, ...)                               \
+    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN) {
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    }
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN) {
+        sshm->prot = LIBXL_SSHM_PROT_RW;
+    }
+    /* role-specific checks */
+    if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {
+        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN) {
+            sshm->offset = 0;
+        }
+        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache_policy is only applicable to master domains");
+            return ERROR_INVAL;
+        }
+    } else {
+        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "offset is only applicable to slave domains");
+            return ERROR_INVAL;
+        }
+        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+        if (rc) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache policy not supported on this platform");
+            return ERROR_INVAL;
+        }
+    }
+
+    return 0;
+}
+
+/* Compare function for sorting sshm ranges by sshm->begin */
+static int sshm_range_cmp(const void *a, const void *b)
+{
+    libxl_static_shm *const *sshma = a, *const *sshmb = b;
+    return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
+}
+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role)
+            slave_sshms[num_slaves++] = sshms + i;
+    }
+    qsort(slave_sshms, num_slaves, sizeof(slave_sshms[0]), sshm_range_cmp);
+
+    for (i = 0; i < num_slaves - 1; ++i) {
+        if (slave_sshms[i+1]->begin < slave_sshms[i]->end) {
+            SSHM_ERROR(domid, slave_sshms[i+1]->id, "slave ranges overlap.");
+            return ERROR_INVAL;
+        }
+    }
+
+    return 0;
+}
+
+/* The caller have to guarentee that sshm->begin < sshm->end */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm,
+                              uint64_t mbegin, uint64_t mend)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, offset;
Quoted 8 lines

tools/libxl/libxl_sshm.c line 94

Quoted 5 lines
tools/libxl/libxl_sshm.c new file +5 -0
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (mend - mbegin) >> 12;

Please don't hardcode 12, please use XC_PAGE_SHIFT (also in other libxl patches).

OK.

Quoted 110 lines
Quoted 110 lines
tools/libxl/libxl_sshm.c new file +108 -0
+    num_spages = (sshm->end - sshm->begin) >> 12;
+    offset = sshm->offset >> 12;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (mbegin >> 12) + offset + i;
+        gpfns[i]= (sshm->begin >> 12) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    for (i = 0; i< num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       sshm->begin + (offset << 12) );
+            rc = ERROR_FAIL;
+        }
+    }
+    if (rc) goto out;
+
+    rc = 0;
+
+out:
+    return rc;
+}
+
+static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
+                                 libxl_static_shm *sshm)
+{
+    int rc;
+    char *sshm_path, *slave_path, *dom_path, *dom_sshm_path, *dom_role_path;
+    char *ents[9];
+    const char *xs_value;
+    libxl_static_shm master_sshm;
+    uint32_t master_domid;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, sshm->id);
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_sshm_path = GCSPRINTF("%s/static_shm/%s", dom_path, sshm->id);
+    dom_role_path = GCSPRINTF("%s/role", dom_sshm_path);
+
+    /* prepare the slave xenstore entries */
+    ents[0] = "begin";
+    ents[1] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[2] = "end";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[4] = "offset";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->offset);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = NULL;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            SSHM_ERROR(domid, sshm->id, "no master found.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* every ID can appear in each domain at most once */
+        if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+            SSHM_ERROR(domid, sshm->id,
+                       "domain tried to map the same ID twice.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        /* look at the master info and see if we could do the mapping */
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/prot", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        libxl_sshm_prot_from_string(xs_value, &master_sshm.prot);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/begin", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_sshm.begin = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
+                                    GCSPRINTF("%s/end", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_sshm.end = strtoull(xs_value, NULL, 16);
+
+        rc = libxl__xs_read_checked(gc, xt,
Quoted 8 lines

tools/libxl/libxl_sshm.c line 208

Quoted 6 lines
tools/libxl/libxl_sshm.c new file +6 -0
+                                    GCSPRINTF("%s/master", sshm_path),
+                                    &xs_value);
+        if (rc) goto out;
+        master_domid = strtoull(xs_value, NULL, 16);
+
+        /* check if the slave is asking too much permission */

this comment doesn't seem to match the check below

I see this setdefault-and-check as a whole, but if you think it's confusing, I'll move it down to the exact check.

Quoted 15 lines
Quoted 4 lines
tools/libxl/libxl_sshm.c new file +2 -0
+        if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
+            sshm->prot = master_sshm.prot;

tools/libxl/libxl_sshm.c line 220

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+        }
+        if (master_sshm.prot < sshm->prot) {
+            SSHM_ERROR(domid, sshm->id, "slave is asking too much permission.");
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        /* all checks passed, do the job */
+        rc = libxl__sshm_do_map(gc, master_domid, domid, sshm,
+                                master_sshm.begin, master_sshm.end);
Quoted 2 lines

Doesn't libxl__sshm_do_map risk being called twice on xenstore transaction retries?

Oops. I think I should skip libxl__sshm_do_map when within a retry, just like what I did in the libxl__sshm_del patch.

Cheers,

Zhongze Liu

Quoted 156 lines
Quoted 156 lines
tools/libxl/libxl_sshm.c new file +116 -0
+        if (rc) {
+            rc = ERROR_INVAL;
+            goto out;
+        }
+
+        /* write the result to xenstore and commit */
+        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+        if (rc) goto out;
+        libxl__xs_writev(gc, xt, slave_path, ents);
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+    int rc;
+    char *sshm_path, *dom_path, *dom_role_path;
+    char *ents[13];
+    struct xs_permissions noperm;
+    xs_transaction_t xt = XBT_NULL;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, sshm->id);
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    /* the domain should be in xenstore by now */
+    assert(dom_path);
+    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
+
+    /* prepare the xenstore entries */
+    ents[0] = "master";
+    ents[1] = GCSPRINTF("%"PRIu32, domid);
+    ents[2] = "begin";
+    ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
+    ents[4] = "end";
+    ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
+    ents[6] = "prot";
+    ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
+    ents[8] = "cache_policy";
+    ents[9] = libxl__strdup(gc,
+                            libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
+    ents[10] = "status";
+    ents[11] = "alive";
+    ents[12] = NULL;
+
+    /* could only be accessed by Dom0 */
+    noperm.id = 0;
+    noperm.perms = XS_PERM_NONE;
+
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (!libxl__xs_read(gc, xt, sshm_path)) {
+            /* every ID can appear in each domain at most once */
+            if (libxl__xs_read(gc, xt, dom_role_path)) {
+                SSHM_ERROR(domid, sshm->id,
+                           "domain tried to map the same ID twice.");
+                rc = ERROR_FAIL;
+                goto out;
+            }
+            rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
+            if (rc) goto out;;
+
+            libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
+            libxl__xs_writev(gc, xt, sshm_path, ents);
+        } else {
+            SSHM_ERROR(domid, sshm->id, "can only have one master.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
+int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
+                    libxl_static_shm *sshms, int len)
+{
+    int rc, i;
+
+    if (!len) return 0;
+
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role) {
+            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
+        } else {
+            rc = libxl__sshm_add_master(gc, domid, sshms+i);
+        }
+        if (rc)  return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
tools/libxl/libxl_x86.c +18 -0
index 455f6f0bed..8dd34fd7ba 100644
@@ -587,6 +587,24 @@ void libxl__arch_domain_build_info_acpi_setdefault(
     libxl_defbool_setdefault(&b_info->acpi, true);
 }
 
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info)
+{
+    /* FIXME: Mark this as unsupported since calling p2m_add_foreign on two
+     * DomU's is currently not allowd on x86, see the comments in
+     * x86/mm/p2m.c: p2m_add_foreign */
+     return false;
+}
+
+int libxl__arch_domain_sshm_cachepolicy_setdefault(libxl_static_shm *sshm)
+{
+    if (sshm->cache_policy == LIBXL_SSHM_CACHEPOLICY_UNKNOWN)
+        sshm->cache_policy = LIBXL_SSHM_CACHEPOLICY_X86_NORMAL;
+    if (sshm->cache_policy < LIBXL_SSHM_CACHEPOLICY_X86_NORMAL)
+        return ERROR_INVAL;
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
tools/libxl/libxl_xshelp.c +0 -0
index c4a18df353..d91fbf5fda 100644
@@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
     return s;
 }
 
Quoted 11 lines

tools/libxl/libxl_xshelp.c char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid) · line 206

Quoted 11 lines
tools/libxl/libxl_xshelp.c +8 -0
+char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
+{
+    char *s = GCSPRINTF("/local/static_shm/%s", id);
+    if (!s)
+        LOGE(ERROR, "cannot allocate static shm path");
+    return s;
+}
+
 int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char **result_out)
 {

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#24 lore raw

Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

In reply to Zhongze Liu

On Wed, Aug 23, 2017 at 02:08:39AM +0800, Zhongze Liu wrote:
[...]

tools/libxl/libxl_arch.h int libxl__arch_extra_memory(libxl__gc *gc, · line 75

Quoted 10 lines
tools/libxl/libxl_arch.h +2 -0
index 5e1fc6060e..1d681d8863 100644
@@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
 
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);

No need to take any argument afaict.

[...]

Quoted 23 lines
tools/libxl/libxl_create.c +8 -0
index 1158303e1a..8e5ec486d2 100644
@@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
         ret = ERROR_INVAL;
         goto out;
     }
+
+    /* the p2m has been setup, we could map the static shared memory now. */
+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
     return ret;
@@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 

tools/libxl/libxl_create.c static void initiate_domain_create(libxl__egc *egc, · line 938

Quoted 10 lines
tools/libxl/libxl_create.c +10 -0
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;

Better move this after setting default.

Quoted 14 lines
tools/libxl/libxl_create.c +9 -0
+
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
+            goto error_out;
+        }
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
tools/libxl/libxl_internal.h +0 -0
index 724750967c..74bc0acb21 100644

[...]

Quoted 7 lines
tools/libxl/libxl_sshm.c new file +1 -0
new file mode 100644
index 0000000000..e16c24ccb9
@@ -0,0 +1,336 @@
+#include "libxl_osdeps.h"

tools/libxl/libxl_sshm.c line 11

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_ERROR(domid, sshmid, f, ...)                               \
+    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)

Indentation.

tools/libxl/libxl_sshm.c line 20

Quoted 9 lines
tools/libxl/libxl_sshm.c new file +9 -0
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN) {
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    }
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN) {
+        sshm->prot = LIBXL_SSHM_PROT_RW;
+    }

You can avoid using {} when there is only one statement.

tools/libxl/libxl_sshm.c line 22

Quoted 2 lines
tools/libxl/libxl_sshm.c new file +2 -0
+    /* role-specific checks */
+    if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {

Style.

Quoted 11 lines
tools/libxl/libxl_sshm.c new file +11 -0
+        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN) {
+            sshm->offset = 0;
+        }
+        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache_policy is only applicable to master domains");
+            return ERROR_INVAL;
+        }
+    } else {
+        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,

tools/libxl/libxl_sshm.c line 43

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+                       "offset is only applicable to slave domains");
+            return ERROR_INVAL;
+        }
+        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+        if (rc) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache policy not supported on this platform");
+            return ERROR_INVAL;
+        }
+    }

Please use goto style error handling.

Quoted 14 lines
tools/libxl/libxl_sshm.c new file +14 -0
+
+    return 0;
+}
+
+/* Compare function for sorting sshm ranges by sshm->begin */
+static int sshm_range_cmp(const void *a, const void *b)
+{
+    libxl_static_shm *const *sshma = a, *const *sshmb = b;
+    return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
+}
+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)

tools/libxl/libxl_sshm.c line 67

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role)

Style.

Quoted 37 lines
tools/libxl/libxl_sshm.c new file +37 -0
+/* The caller have to guarentee that sshm->begin < sshm->end */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm,
+                              uint64_t mbegin, uint64_t mend)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (mend - mbegin) >> 12;
+    num_spages = (sshm->end - sshm->begin) >> 12;
+    offset = sshm->offset >> 12;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (mbegin >> 12) + offset + i;
+        gpfns[i]= (sshm->begin >> 12) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+

tools/libxl/libxl_sshm.c line 128

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+    for (i = 0; i< num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
+                       sshm->begin + (offset << 12) );
+            rc = ERROR_FAIL;
+        }
+    }
+    if (rc) goto out;
+

How is partial failure handled?

Quoted 9 lines
tools/libxl/libxl_sshm.c new file +9 -0
+    rc = 0;
+
+out:
+    return rc;
+}
+
+static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
+                                 libxl_static_shm *sshm)
+{

[...]

tools/libxl/libxl_sshm.c line 211

Quoted 5 lines
tools/libxl/libxl_sshm.c new file +5 -0
+
+        /* check if the slave is asking too much permission */
+        if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
+            sshm->prot = master_sshm.prot;
+        }

Style.

tools/libxl/libxl_sshm.c line 316

Quoted 6 lines
tools/libxl/libxl_sshm.c new file +6 -0
+int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
+                    libxl_static_shm *sshms, int len)
+{
+    int rc, i;
+
+    if (!len) return 0;

Unneeded check.

Quoted 20 lines
tools/libxl/libxl_sshm.c new file +20 -0
+
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role) {
+            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
+        } else {
+            rc = libxl__sshm_add_master(gc, domid, sshms+i);
+        }
+        if (rc)  return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

[...]

tools/libxl/libxl_xshelp.c char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid) · line 198

Quoted 11 lines
tools/libxl/libxl_xshelp.c +3 -0
index c4a18df353..d91fbf5fda 100644
@@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
     return s;
 }
 
+char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
+{
+    char *s = GCSPRINTF("/local/static_shm/%s", id);

This can't fail.

Quoted 11 lines
tools/libxl/libxl_xshelp.c +5 -0
+    if (!s)
+        LOGE(ERROR, "cannot allocate static shm path");
+    return s;
+}
+
 int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char **result_out)
 {
Signature · 1 line
2.14.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#25 lore raw

Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

Hi Wei,

2017-08-25 19:05 GMT+08:00 Wei Liu <[email protected]>:

Quoted 10 lines · Zhongze Liu

[...]

Quoted 8 lines
tools/libxl/libxl_arch.h +0 -0
index 5e1fc6060e..1d681d8863 100644
@@ -71,6 +71,12 @@ int libxl__arch_extra_memory(libxl__gc *gc,
                              const libxl_domain_build_info *info,
                              uint64_t *out);
 
Quoted 4 lines

tools/libxl/libxl_arch.h int libxl__arch_extra_memory(libxl__gc *gc, · line 75

Quoted 2 lines
tools/libxl/libxl_arch.h +2 -0
+_hidden
+bool libxl__arch_domain_support_sshm(const libxl_domain_build_info *b_info);

No need to take any argument afaict.

This is needed because later on, we will restrict this to HVM only on x86, we need b_info
to tell if a domain is HVM or not.

Quoted 25 lines

[...]

Quoted 23 lines
tools/libxl/libxl_create.c +8 -0
index 1158303e1a..8e5ec486d2 100644
@@ -501,6 +501,14 @@ int libxl__domain_build(libxl__gc *gc,
         ret = ERROR_INVAL;
         goto out;
     }
+
+    /* the p2m has been setup, we could map the static shared memory now. */
+    ret = libxl__sshm_add(gc, domid, d_config->sshms, d_config->num_sshms);
+    if (ret != 0) {
+        LOG(ERROR, "failed to map static shared memory");
+        goto out;
+    }
+
     ret = libxl__build_post(gc, domid, info, state, vments, localents);
 out:
     return ret;
@@ -918,6 +926,25 @@ static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
Quoted 12 lines

tools/libxl/libxl_create.c static void initiate_domain_create(libxl__egc *egc, · line 938

Quoted 10 lines
tools/libxl/libxl_create.c +10 -0
+    if (d_config->num_sshms != 0 &&
+        !libxl__arch_domain_support_sshm(&d_config->b_info)) {
+        LOGD(ERROR, domid, "static_shm is not supported by this domain type.");
+        ret = ERROR_INVAL;
+        goto error_out;
+    }
+
+    ret = libxl__sshm_check_overlap(gc, domid,
+                                    d_config->sshms, d_config->num_sshms);
+    if (ret) goto error_out;

Better move this after setting default.

OK.

Quoted 48 lines
Quoted 15 lines
tools/libxl/libxl_create.c +9 -0
+
+    for (i = 0; i < d_config->num_sshms; ++i) {
+        ret = libxl__sshm_setdefault(gc, domid, &d_config->sshms[i]);
+        if (ret) {
+            LOGD(ERROR, domid, "Unable to set defaults for static sshm");
+            goto error_out;
+        }
+    }
+
     ret = libxl__domain_make(gc, d_config, &domid, &state->config);
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
tools/libxl/libxl_internal.h +0 -0
index 724750967c..74bc0acb21 100644

[...]

Quoted 7 lines
tools/libxl/libxl_sshm.c new file +1 -0
new file mode 100644
index 0000000000..e16c24ccb9
@@ -0,0 +1,336 @@
+#include "libxl_osdeps.h"

tools/libxl/libxl_sshm.c line 11

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_ERROR(domid, sshmid, f, ...)                               \
+    LOGD(ERROR, domid, "static_shm id = %s: " f, sshmid, ##__VA_ARGS__)
+
+
+/* Set default values for libxl_static_shm */
+int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)

Indentation.

tools/libxl/libxl_sshm.c line 20

Quoted 9 lines
tools/libxl/libxl_sshm.c new file +9 -0
+{
+    int rc;
+
+    if (sshm->role == LIBXL_SSHM_ROLE_UNKNOWN) {
+        sshm->role = LIBXL_SSHM_ROLE_SLAVE;
+    }
+    if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN) {
+        sshm->prot = LIBXL_SSHM_PROT_RW;
+    }

You can avoid using {} when there is only one statement.

Quoted 4 lines

tools/libxl/libxl_sshm.c line 22

Quoted 2 lines
tools/libxl/libxl_sshm.c new file +2 -0
+    /* role-specific checks */
+    if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {

Style.

Sorry about this. I've realized this and have removed all the Yoda conditions in my new draft.

Quoted 93 lines
Quoted 12 lines
tools/libxl/libxl_sshm.c new file +11 -0
+        if (sshm->offset == LIBXL_SSHM_RANGE_UNKNOWN) {
+            sshm->offset = 0;
+        }
+        if (sshm->cache_policy != LIBXL_SSHM_CACHEPOLICY_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache_policy is only applicable to master domains");
+            return ERROR_INVAL;
+        }
+    } else {
+        if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+            SSHM_ERROR(domid, sshm->id,

tools/libxl/libxl_sshm.c line 43

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+                       "offset is only applicable to slave domains");
+            return ERROR_INVAL;
+        }
+        rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+        if (rc) {
+            SSHM_ERROR(domid, sshm->id,
+                       "cache policy not supported on this platform");
+            return ERROR_INVAL;
+        }
+    }

Please use goto style error handling.

Quoted 14 lines
tools/libxl/libxl_sshm.c new file +14 -0
+
+    return 0;
+}
+
+/* Compare function for sorting sshm ranges by sshm->begin */
+static int sshm_range_cmp(const void *a, const void *b)
+{
+    libxl_static_shm *const *sshma = a, *const *sshmb = b;
+    return (*sshma)->begin > (*sshmb)->begin ? 1 : -1;
+}
+
+/* check if the sshm slave configs in @sshm overlap */
+int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
+                                     libxl_static_shm *sshms, int len)

tools/libxl/libxl_sshm.c line 67

Quoted 10 lines
tools/libxl/libxl_sshm.c new file +10 -0
+{
+
+    const libxl_static_shm **slave_sshms = NULL;
+    int num_slaves;
+    int i;
+
+    slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+    num_slaves = 0;
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role)

Style.

Quoted 41 lines
tools/libxl/libxl_sshm.c new file +41 -0
+/* The caller have to guarentee that sshm->begin < sshm->end */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm,
+                              uint64_t mbegin, uint64_t mend)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (mend - mbegin) >> 12;
+    num_spages = (sshm->end - sshm->begin) >> 12;
+    offset = sshm->offset >> 12;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (mbegin >> 12) + offset + i;
+        gpfns[i]= (sshm->begin >> 12) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    for (i = 0; i< num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
Quoted 8 lines

tools/libxl/libxl_sshm.c line 128

Quoted 6 lines
tools/libxl/libxl_sshm.c new file +6 -0
+                       sshm->begin + (offset << 12) );
+            rc = ERROR_FAIL;
+        }
+    }
+    if (rc) goto out;
+

How is partial failure handled?

Em...
If one of the pages failed, the whole domain won't be constructed. But the mapped pages are still occupying the refcount.
Do you think I should continue or just throw out a warning and let to user to decide whether she is going to destroy it or not?

Quoted 17 lines
Quoted 10 lines
tools/libxl/libxl_sshm.c new file +9 -0
+    rc = 0;
+
+out:
+    return rc;
+}
+
+static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
+                                 libxl_static_shm *sshm)
+{

[...]

tools/libxl/libxl_sshm.c line 211

Quoted 5 lines
tools/libxl/libxl_sshm.c new file +5 -0
+
+        /* check if the slave is asking too much permission */
+        if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
+            sshm->prot = master_sshm.prot;
+        }
Quoted 10 lines

Style.

tools/libxl/libxl_sshm.c line 316

Quoted 6 lines
tools/libxl/libxl_sshm.c new file +6 -0
+int libxl__sshm_add(libxl__gc *gc,  uint32_t domid,
+                    libxl_static_shm *sshms, int len)
+{
+    int rc, i;
+
+    if (!len) return 0;

will remove this.

Quoted 37 lines
Quoted 21 lines
tools/libxl/libxl_sshm.c new file +20 -0
+
+    for (i = 0; i < len; ++i) {
+        if (LIBXL_SSHM_ROLE_SLAVE == sshms[i].role) {
+            rc = libxl__sshm_add_slave(gc, domid, sshms+i);
+        } else {
+            rc = libxl__sshm_add_master(gc, domid, sshms+i);
+        }
+        if (rc)  return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */

[...]

tools/libxl/libxl_xshelp.c char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid) · line 198

Quoted 11 lines
tools/libxl/libxl_xshelp.c +3 -0
index c4a18df353..d91fbf5fda 100644
@@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
     return s;
 }
 
+char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
+{
+    char *s = GCSPRINTF("/local/static_shm/%s", id);

This can't fail.

Quoted 11 lines
Quoted 11 lines
tools/libxl/libxl_xshelp.c +5 -1
+    if (!s)
+        LOGE(ERROR, "cannot allocate static shm path");
+    return s;
+}
+
 int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
                              const char *path, const char **result_out)
 {
--

2.14.0

Cheers,

Zhongze Liu.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#26 lore raw

Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation

Quoted 52 lines · Zhongze Liu

Hi Wei,

Quoted 41 lines
Quoted 41 lines
tools/libxl/libxl_sshm.c new file +41 -0
+/* The caller have to guarentee that sshm->begin < sshm->end */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+                              libxl_static_shm *sshm,
+                              uint64_t mbegin, uint64_t mend)
+{
+    int rc;
+    int i;
+    unsigned int num_mpages, num_spages, offset;
+    int *errs;
+    xen_ulong_t *idxs;
+    xen_pfn_t *gpfns;
+
+    num_mpages = (mend - mbegin) >> 12;
+    num_spages = (sshm->end - sshm->begin) >> 12;
+    offset = sshm->offset >> 12;
+
+    /* Check range. Test offset < mpages first to avoid overflow */
+    if ((offset >= num_mpages) || (num_mpages - offset < num_spages)) {
+        SSHM_ERROR(sid, sshm->id, "exceeds master's address space.");
+        rc = ERROR_INVAL;
+        goto out;
+    }
+
+    /* fill out the pfn's and do the mapping */
+    errs = libxl__calloc(gc, num_spages, sizeof(int));
+    idxs = libxl__calloc(gc, num_spages, sizeof(xen_ulong_t));
+    gpfns = libxl__calloc(gc, num_spages, sizeof(xen_pfn_t));
+    for (i = 0; i < num_spages; i++) {
+        idxs[i] = (mbegin >> 12) + offset + i;
+        gpfns[i]= (sshm->begin >> 12) + i;
+    }
+    rc = xc_domain_add_to_physmap_batch(CTX->xch,
+                                        sid, mid,
+                                        XENMAPSPACE_gmfn_foreign,
+                                        num_spages,
+                                        idxs, gpfns, errs);
+
+    for (i = 0; i< num_spages; i++) {
+        if (errs[i]) {
+            SSHM_ERROR(sid, sshm->id,
+                       "can't map at address 0x%"PRIx64".",
Quoted 8 lines

tools/libxl/libxl_sshm.c line 128

Quoted 6 lines
tools/libxl/libxl_sshm.c new file +6 -0
+                       sshm->begin + (offset << 12) );
+            rc = ERROR_FAIL;
+        }
+    }
+    if (rc) goto out;
+

How is partial failure handled?

Quoted 5 lines

Em...
If one of the pages failed, the whole domain won't be constructed. But the mapped pages are still occupying the refcount.
Do you think I should continue or just throw out a warning and let to user to decide whether she is going to destroy it or not?

You should be able to roll back by calling remove_from_physmap if necessary.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#27 lore raw

PATCH 6/6 libxl: support unmapping static shared memory areas during domain destruction

In reply to Zhongze Liu

Add libxl__sshm_del to Unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: check if it still has living slaves: 1) if yes, mark its status as "zombie", and don't destroy the information under /local/static_shm; 2) if no, simply cleanup related xs entries
* For a slave: unmap the shared pages, and cleanup related xs entries. If this is the only slave, and it's master is in zombie state, also cleanup the master entries.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Ian Jackson <[email protected]>
Cc Wei Liu <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl_domain.c   |   5 ++
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_sshm.c     | 125 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
tools/libxl/libxl_domain.c +5 -0
index 08eccd082b..73ac856fb4 100644
@@ -1028,6 +1028,11 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         goto out;
     }
 
+    rc = libxl__sshm_del(gc, domid);
+    if (rc) {
+        LOGD(ERROR, domid, "Deleting static shm failed.");
+    }
+
     if (libxl__device_pci_destroy_all(gc, domid) < 0)
         LOGD(ERROR, domid, "Pci shutdown failed");
     rc = xc_domain_pause(ctx->xch, domid);
tools/libxl/libxl_internal.h +2 -0
index 74bc0acb21..648eaee8c2 100644
@@ -4361,6 +4361,8 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                             libxl_static_shm *sshm, int len);
 
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
 _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                       libxl_static_shm *sshms, int len);
 _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
tools/libxl/libxl_sshm.c +125 -0

131 lines not on this page · raw message

Signature · 7 lines
2.14.0


_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel
#28 lore raw

Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction

Quoted 70 lines · Zhongze Liu

Add libxl__sshm_del to Unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: check if it still has living slaves: 1) if yes, mark its status as "zombie", and don't destroy the information under /local/static_shm; 2) if no, simply cleanup related xs entries
* For a slave: unmap the shared pages, and cleanup related xs entries. If this is the only slave, and it's master is in zombie state, also cleanup the master entries.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Ian Jackson <[email protected]>
Cc Wei Liu <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl_domain.c   |   5 ++
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_sshm.c     | 125 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
tools/libxl/libxl_domain.c +5 -0
index 08eccd082b..73ac856fb4 100644
@@ -1028,6 +1028,11 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         goto out;
     }
 
+    rc = libxl__sshm_del(gc, domid);
+    if (rc) {
+        LOGD(ERROR, domid, "Deleting static shm failed.");
+    }
+
     if (libxl__device_pci_destroy_all(gc, domid) < 0)
         LOGD(ERROR, domid, "Pci shutdown failed");
     rc = xc_domain_pause(ctx->xch, domid);
tools/libxl/libxl_internal.h +2 -0
index 74bc0acb21..648eaee8c2 100644
@@ -4361,6 +4361,8 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                             libxl_static_shm *sshm, int len);
 
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
 _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                       libxl_static_shm *sshms, int len);
 _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
tools/libxl/libxl_sshm.c +5 -0
index e16c24ccb9..417a4cd0a4 100644
@@ -79,6 +79,131 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+/* Delete sshm master infomation from xenstore. If there are still living
+ * slaves, mark the master as in zombie state, but do not delete it,
+ * and it will be totally deleted later after all slaves are gone */
+static void libxl__sshm_del_master(libxl__gc *gc, xs_transaction_t xt,
+                                   uint32_t domid, const char *id)

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 96

Quoted 10 lines
tools/libxl/libxl_sshm.c +10 -0
+{
+    char *sshm_path;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, id);
+
+    /* we know that domid can't be both a master and a slave for one id
+     * (enforced in the *_add_master() and *_add_slave() code),
+     * so the number of slaves won't change during iteration. Simply check
+     * sshm_path/slaves to tell if there are still living slaves. */
+    if (libxl__xs_read(gc, xt, GCSPRINTF("%s/slaves", sshm_path))) {

Is it possible that "slaves" is present but empty? What does libxl__xs_read return in that case?

Quoted 44 lines
tools/libxl/libxl_sshm.c +44 -0
+        SSHM_ERROR(domid, id,
+                   "there are living slaves, won't be totally destroyed");
+
+        libxl__xs_write_checked(gc, xt,
+                                GCSPRINTF("%s/status", sshm_path),
+                                "zombie");
+    } else {
+        libxl__xs_path_cleanup(gc, xt, sshm_path);
+    }
+
+    return;
+}
+
+static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char *id,
+                                 uint64_t begin, uint64_t end)
+{
+    begin >>= 12;
+    end >>= 12;
+    for (; begin < end; ++begin) {
+        if (xc_domain_remove_from_physmap(CTX->xch, domid, begin)) {
+            SSHM_ERROR(domid, id,
+                       "unable to unmap shared page at 0x%"PRIx64".",
+                       begin);
+        }
+    }
+}
+
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    char *sshm_path, *slave_path;
+    char *master_stat, *mdomid_str, *begin_str, *end_str;
+    uint64_t begin, end;
+    uint32_t master_domid;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, id);
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction retry */

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 150

Quoted 10 lines
tools/libxl/libxl_sshm.c +10 -0
+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);
+
+    libxl__xs_path_cleanup(gc, xt, slave_path);
+
+    /* check if master is in zombie state and has no slaves now,
+     * if yes, now it's the time to destroy it */
+    master_stat = libxl__xs_read(gc, xt, GCSPRINTF("%s/status", sshm_path));
+    if (!strncmp(master_stat, "zombie", 6) &&
+        !libxl__xs_read(gc, xt, GCSPRINTF("%s/slaves", sshm_path)))

same here

Quoted 62 lines
tools/libxl/libxl_sshm.c +56 -0
+    {
+        mdomid_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/master", sshm_path));
+        master_domid = strtoul(mdomid_str, NULL, 10);
+        libxl__sshm_del_master(gc, xt, master_domid, id);
+    }
+}
+
+/* Delete static_shm entries in the xensotre. */
+int libxl__sshm_del(libxl__gc *gc,  uint32_t domid)
+{
+    int rc, i;
+    bool isretry;
+    xs_transaction_t xt = XBT_NULL;
+    char *dom_path, *dom_sshm_path;
+    const char *role;
+    char **sshm_ents;
+    unsigned int sshm_num;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    dom_sshm_path = GCSPRINTF("%s/static_shm", dom_path);
+
+    isretry = false;
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+            sshm_ents = libxl__xs_directory(gc, xt, dom_sshm_path, &sshm_num);
+            if (!sshm_ents) continue;
+
+            for (i = 0; i < sshm_num; ++i) {
+                role = libxl__xs_read(gc, xt,
+                                      GCSPRINTF("%s/%s/role",
+                                                dom_sshm_path,
+                                                sshm_ents[i]));
+                assert(role);
+                if (!strncmp(role, "slave", 5)) {
+                    libxl__sshm_del_slave(gc, xt, domid, sshm_ents[i], isretry);
+                } else {
+                    libxl__sshm_del_master(gc, xt, domid, sshm_ents[i]);
+                }
+            }
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+	 isretry = true;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
+}
+
 /* The caller have to guarentee that sshm->begin < sshm->end */
 static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
                               libxl_static_shm *sshm,
Signature · 1 line
2.14.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#29 lore raw

Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction

Hi Stefano,

2017-08-23 5:31 GMT+08:00 Stefano Stabellini <[email protected]>:

Quoted 76 lines · Zhongze Liu
Quoted 75 lines

Add libxl__sshm_del to Unmap static shared memory areas mapped by libxl__sshm_add during domain creation. The unmapping process is:

* For a master: check if it still has living slaves: 1) if yes, mark its status as "zombie", and don't destroy the information under /local/static_shm; 2) if no, simply cleanup related xs entries
* For a slave: unmap the shared pages, and cleanup related xs entries. If this is the only slave, and it's master is in zombie state, also cleanup the master entries.

This is for the proposal "Allow setting up shared memory areas between VMs from xl config file" (see [1]).

[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03047.html

Cc Ian Jackson <[email protected]>
Cc Wei Liu <[email protected]>
Cc Stefano Stabellini <[email protected]>
Cc Julien Grall <[email protected]>
---
 tools/libxl/libxl_domain.c   |   5 ++
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_sshm.c     | 125 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)
tools/libxl/libxl_domain.c +5 -0
index 08eccd082b..73ac856fb4 100644
@@ -1028,6 +1028,11 @@ void libxl__destroy_domid(libxl__egc *egc, libxl__destroy_domid_state *dis)
         goto out;
     }
 
+    rc = libxl__sshm_del(gc, domid);
+    if (rc) {
+        LOGD(ERROR, domid, "Deleting static shm failed.");
+    }
+
     if (libxl__device_pci_destroy_all(gc, domid) < 0)
         LOGD(ERROR, domid, "Pci shutdown failed");
     rc = xc_domain_pause(ctx->xch, domid);
tools/libxl/libxl_internal.h +2 -0
index 74bc0acb21..648eaee8c2 100644
@@ -4361,6 +4361,8 @@ static inline bool libxl__acpi_defbool_val(const libxl_domain_build_info *b_info
 _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
                             libxl_static_shm *sshm, int len);
 
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
 _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
                                       libxl_static_shm *sshms, int len);
 _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
tools/libxl/libxl_sshm.c +10 -0
index e16c24ccb9..417a4cd0a4 100644
@@ -79,6 +79,131 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
     return 0;
 }
 
+/* Delete sshm master infomation from xenstore. If there are still living
+ * slaves, mark the master as in zombie state, but do not delete it,
+ * and it will be totally deleted later after all slaves are gone */
+static void libxl__sshm_del_master(libxl__gc *gc, xs_transaction_t xt,
+                                   uint32_t domid, const char *id)
+{
+    char *sshm_path;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, id);
+
Quoted 8 lines

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 96

Quoted 5 lines
tools/libxl/libxl_sshm.c +5 -0
+    /* we know that domid can't be both a master and a slave for one id
+     * (enforced in the *_add_master() and *_add_slave() code),
+     * so the number of slaves won't change during iteration. Simply check
+     * sshm_path/slaves to tell if there are still living slaves. */
+    if (libxl__xs_read(gc, xt, GCSPRINTF("%s/slaves", sshm_path))) {

Is it possible that "slaves" is present but empty? What does libxl__xs_read return in that case?

Because I didn't create the "slaves" entry directly, it's automatically created when I
write things to slaves/*. So when all the slaves are gone, it will go away. This is what I observed during a local test, but not very sure if it's the expected behavior.

Cheer,

Zhongze Liu

Quoted 114 lines
Quoted 46 lines
tools/libxl/libxl_sshm.c +44 -0
+        SSHM_ERROR(domid, id,
+                   "there are living slaves, won't be totally destroyed");
+
+        libxl__xs_write_checked(gc, xt,
+                                GCSPRINTF("%s/status", sshm_path),
+                                "zombie");
+    } else {
+        libxl__xs_path_cleanup(gc, xt, sshm_path);
+    }
+
+    return;
+}
+
+static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char *id,
+                                 uint64_t begin, uint64_t end)
+{
+    begin >>= 12;
+    end >>= 12;
+    for (; begin < end; ++begin) {
+        if (xc_domain_remove_from_physmap(CTX->xch, domid, begin)) {
+            SSHM_ERROR(domid, id,
+                       "unable to unmap shared page at 0x%"PRIx64".",
+                       begin);
+        }
+    }
+}
+
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+                                  uint32_t domid, const char *id, bool isretry)
+{
+    char *sshm_path, *slave_path;
+    char *master_stat, *mdomid_str, *begin_str, *end_str;
+    uint64_t begin, end;
+    uint32_t master_domid;
+
+    sshm_path = libxl__xs_get_sshmpath(gc, id);
+    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, sshm_path, domid);
+
+    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
+    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
+    begin = strtoull(begin_str, NULL, 16);
+    end = strtoull(end_str, NULL, 16);
+
+    /* Avoid calling do_unmap many times in case of xs transaction retry */

tools/libxl/libxl_sshm.c int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid, · line 150

Quoted 10 lines
tools/libxl/libxl_sshm.c +10 -0
+    if (!isretry)
+        libxl__sshm_do_unmap(gc, domid, id, begin, end);
+
+    libxl__xs_path_cleanup(gc, xt, slave_path);
+
+    /* check if master is in zombie state and has no slaves now,
+     * if yes, now it's the time to destroy it */
+    master_stat = libxl__xs_read(gc, xt, GCSPRINTF("%s/status", sshm_path));
+    if (!strncmp(master_stat, "zombie", 6) &&
+        !libxl__xs_read(gc, xt, GCSPRINTF("%s/slaves", sshm_path)))

same here

Quoted 54 lines
tools/libxl/libxl_sshm.c +54 -0
+    {
+        mdomid_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/master", sshm_path));
+        master_domid = strtoul(mdomid_str, NULL, 10);
+        libxl__sshm_del_master(gc, xt, master_domid, id);
+    }
+}
+
+/* Delete static_shm entries in the xensotre. */
+int libxl__sshm_del(libxl__gc *gc,  uint32_t domid)
+{
+    int rc, i;
+    bool isretry;
+    xs_transaction_t xt = XBT_NULL;
+    char *dom_path, *dom_sshm_path;
+    const char *role;
+    char **sshm_ents;
+    unsigned int sshm_num;
+
+    dom_path = libxl__xs_get_dompath(gc, domid);
+    dom_sshm_path = GCSPRINTF("%s/static_shm", dom_path);
+
+    isretry = false;
+    for (;;) {
+        rc = libxl__xs_transaction_start(gc, &xt);
+        if (rc) goto out;
+
+        if (libxl__xs_read(gc, xt, dom_sshm_path)) {
+            sshm_ents = libxl__xs_directory(gc, xt, dom_sshm_path, &sshm_num);
+            if (!sshm_ents) continue;
+
+            for (i = 0; i < sshm_num; ++i) {
+                role = libxl__xs_read(gc, xt,
+                                      GCSPRINTF("%s/%s/role",
+                                                dom_sshm_path,
+                                                sshm_ents[i]));
+                assert(role);
+                if (!strncmp(role, "slave", 5)) {
+                    libxl__sshm_del_slave(gc, xt, domid, sshm_ents[i], isretry);
+                } else {
+                    libxl__sshm_del_master(gc, xt, domid, sshm_ents[i]);
+                }
+            }
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &xt);
+        if (!rc) break;
+        if (rc < 0) goto out;
+      isretry = true;
+    }
+
+    rc = 0;
+out:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;
Quoted 8 lines
Quoted 8 lines
tools/libxl/libxl_sshm.c +2 -1
+}
+
 /* The caller have to guarentee that sshm->begin < sshm->end */
 static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
                               libxl_static_shm *sshm,
--

2.14.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel

#30 lore raw

Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction

In reply to Zhongze Liu
Quoted 1 line · Zhongze Liu

Add libxl__sshm_del to Unmap static shared memory areas mapped by

Unmap -> unmap

Quoted 8 lines

libxl__sshm_add during domain creation. The unmapping process is:

* For a master: check if it still has living slaves: 1) if yes, mark its status as "zombie", and don't destroy the information under /local/static_shm; 2) if no, simply cleanup related xs entries
* For a slave: unmap the shared pages, and cleanup related xs entries. If this is the only slave, and it's master is in zombie state, also cleanup the master entries.

I still think refcounting the node is better than this customised protocol.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xen.org/xen-devel