* [PATCH 1/6] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-23 16:14 ` Wei Liu
2017-08-22 18:08 ` [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
` (4 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson,
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:
commit 20e725e9364cff4a29945f66986ecd88cca8743d
Now add the wrapper to XENMEM_remove_from_physmap.
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxc/include/xenctrl.h | 4 ++++
tools/libxc/xc_domain.c | 11 +++++++++++
2 files changed, 15 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c7710b8f36..0ff15a9255 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -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,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 3bab4e8bab..e6b32792c0 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -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)
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2017-08-22 18:08 ` [PATCH 1/6] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-22 20:05 ` Stefano Stabellini
2017-08-22 18:08 ` [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
` (3 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
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
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/libxl.h | 4 ++++
tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 229e289750..3ee788642f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -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 */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 6e80d36256..6c9e79c05d 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -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),
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions
2017-08-22 18:08 ` [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
@ 2017-08-22 20:05 ` Stefano Stabellini
2017-08-23 2:05 ` Zhongze Liu
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-22 20:05 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On Wed, 23 Aug 2017, Zhongze Liu wrote:
> Add a new structure to the IDL famliy to represent static shared memory regions,
^ family
> 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
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> tools/libxl/libxl.h | 4 ++++
> tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
> 2 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 229e289750..3ee788642f 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -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 */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 6e80d36256..6c9e79c05d 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -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:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ("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),
> --
> 2.14.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions
2017-08-22 20:05 ` Stefano Stabellini
@ 2017-08-23 2:05 ` Zhongze Liu
0 siblings, 0 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-23 2:05 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Ian Jackson, Julien Grall, Wei Liu, xen-devel
Hi Stefano,
2017-08-23 4:05 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Wed, 23 Aug 2017, Zhongze Liu wrote:
>> Add a new structure to the IDL famliy to represent static shared memory regions,
> ^ family
>
>
>> 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
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> tools/libxl/libxl.h | 4 ++++
>> tools/libxl/libxl_types.idl | 36 ++++++++++++++++++++++++++++++++----
>> 2 files changed, 36 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
>> index 229e289750..3ee788642f 100644
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -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 */
>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>> index 6e80d36256..6c9e79c05d 100644
>> --- a/tools/libxl/libxl_types.idl
>> +++ b/tools/libxl/libxl_types.idl
>> @@ -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:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
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
>
>
>> ("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),
>> --
>> 2.14.0
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2017-08-22 18:08 ` [PATCH 1/6] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
2017-08-22 18:08 ` [PATCH 2/6] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-22 20:36 ` Stefano Stabellini
2017-08-22 18:08 ` [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin Zhongze Liu
` (2 subsequent siblings)
5 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Zhongze Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich
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
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
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
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 082af8f716..3b63fb2cad 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -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)
diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
new file mode 100644
index 0000000000..8647665213
--- /dev/null
+++ b/tools/libxl/libxlu_sshm.c
@@ -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)
+
+#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:
+ */
diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
index e81b644c01..ee39cb5bdc 100644
--- a/tools/libxl/libxlutil.h
+++ b/tools/libxl/libxlutil.h
@@ -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 */
/*
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 5c2bf17222..82d955b8b9 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -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;
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files
2017-08-22 18:08 ` [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
@ 2017-08-22 20:36 ` Stefano Stabellini
2017-08-23 10:58 ` Julien Grall
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-22 20:36 UTC (permalink / raw)
To: Zhongze Liu
Cc: Tim Deegan, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, xen-devel, Julien Grall, Jan Beulich
On Wed, 23 Aug 2017, Zhongze Liu wrote:
> 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
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> 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
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 082af8f716..3b63fb2cad 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -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)
> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
> new file mode 100644
> index 0000000000..8647665213
> --- /dev/null
> +++ b/tools/libxl/libxlu_sshm.c
> @@ -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.
> +#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:
> + */
> diff --git a/tools/libxl/libxlutil.h b/tools/libxl/libxlutil.h
> index e81b644c01..ee39cb5bdc 100644
> --- a/tools/libxl/libxlutil.h
> +++ b/tools/libxl/libxlutil.h
> @@ -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 */
>
> /*
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 5c2bf17222..82d955b8b9 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -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;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files
2017-08-22 20:36 ` Stefano Stabellini
@ 2017-08-23 10:58 ` Julien Grall
0 siblings, 0 replies; 31+ messages in thread
From: Julien Grall @ 2017-08-23 10:58 UTC (permalink / raw)
To: Stefano Stabellini, Zhongze Liu
Cc: Tim Deegan, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
xen-devel, Jan Beulich
Hi,
On 22/08/17 21:36, Stefano Stabellini wrote:
> On Wed, 23 Aug 2017, Zhongze Liu wrote:
>> 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
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> 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
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 082af8f716..3b63fb2cad 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -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)
>> diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
>> new file mode 100644
>> index 0000000000..8647665213
>> --- /dev/null
>> +++ b/tools/libxl/libxlu_sshm.c
>> @@ -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 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,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (2 preceding siblings ...)
2017-08-22 18:08 ` [PATCH 3/6] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-22 19:58 ` Stefano Stabellini
2017-08-23 9:55 ` Jan Beulich
2017-08-22 18:08 ` [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
2017-08-22 18:08 ` [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
5 siblings, 2 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Zhongze Liu, George Dunlap, Andrew Cooper,
Julien Grall, Jan Beulich, Daniel De Graaf
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
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: xen-devel@lists.xen.org
---
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(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a810a056d7..9ec78d8c03 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -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);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e8a57d118c..a547fd00c0 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -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;
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 62fcea6f04..28dbc6f2a2 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -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)
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 60c0fd6a62..a20654a803 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -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)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 91146275bb..3408b6b9e1 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -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)
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-22 18:08 ` [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin Zhongze Liu
@ 2017-08-22 19:58 ` Stefano Stabellini
2017-08-23 2:18 ` Zhongze Liu
2017-08-23 9:55 ` Jan Beulich
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-22 19:58 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Jan Beulich, Daniel De Graaf
On Wed, 23 Aug 2017, Zhongze Liu 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
^ IFF
> 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
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: xen-devel@lists.xen.org
> ---
> 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(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a810a056d7..9ec78d8c03 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -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);
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index e8a57d118c..a547fd00c0 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -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;
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 62fcea6f04..28dbc6f2a2 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -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);
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);
> }
>
> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index 60c0fd6a62..a20654a803 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -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)
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 91146275bb..3408b6b9e1 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -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);
> }
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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-22 19:58 ` Stefano Stabellini
@ 2017-08-23 2:18 ` Zhongze Liu
2017-08-23 15:56 ` Daniel De Graaf
0 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-23 2:18 UTC (permalink / raw)
To: Stefano Stabellini
Cc: George Dunlap, Andrew Cooper, xen-devel, Julien Grall,
Jan Beulich, Daniel De Graaf
Hi Stefano,
2017-08-23 3:58 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Wed, 23 Aug 2017, Zhongze Liu 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
> ^ IFF
>
>
>> 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
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: xen-devel@lists.xen.org
>> ---
>> 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(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index a810a056d7..9ec78d8c03 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -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);
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index e8a57d118c..a547fd00c0 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -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;
>>
>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>> index 62fcea6f04..28dbc6f2a2 100644
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -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);
>
> 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);
OK, will correct this.
>
>
>
>> }
>>
>> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>> index 60c0fd6a62..a20654a803 100644
>> --- a/xen/include/xsm/xsm.h
>> +++ b/xen/include/xsm/xsm.h
>> @@ -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)
>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>> index 91146275bb..3408b6b9e1 100644
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -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);
>> }
>
> 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.
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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-23 2:18 ` Zhongze Liu
@ 2017-08-23 15:56 ` Daniel De Graaf
0 siblings, 0 replies; 31+ messages in thread
From: Daniel De Graaf @ 2017-08-23 15:56 UTC (permalink / raw)
To: Zhongze Liu, Stefano Stabellini
Cc: George Dunlap, Andrew Cooper, Julien Grall, Jan Beulich,
xen-devel
On 08/22/2017 10:18 PM, Zhongze Liu wrote:
> Hi Stefano,
>
> 2017-08-23 3:58 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
>> On Wed, 23 Aug 2017, Zhongze Liu 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
>> ^ IFF
>>
>>
>>> 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
>>>
>>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>>
>>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> Cc: xen-devel@lists.xen.org
>>> ---
>>> 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(-)
>>>
>>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>>> index a810a056d7..9ec78d8c03 100644
>>> --- a/xen/arch/arm/mm.c
>>> +++ b/xen/arch/arm/mm.c
>>> @@ -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);
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index e8a57d118c..a547fd00c0 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -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;
>>>
>>> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
>>> index 62fcea6f04..28dbc6f2a2 100644
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -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);
>>
>> 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);
>
> OK, will correct this.
>
>>
>>
>>
>>> }
>>>
>>> static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>> index 60c0fd6a62..a20654a803 100644
>>> --- a/xen/include/xsm/xsm.h
>>> +++ b/xen/include/xsm/xsm.h
>>> @@ -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)
>>> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
>>> index 91146275bb..3408b6b9e1 100644
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -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);
>>> }
>>
>> 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.
>
> 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:
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-22 18:08 ` [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin Zhongze Liu
2017-08-22 19:58 ` Stefano Stabellini
@ 2017-08-23 9:55 ` Jan Beulich
2017-08-23 17:16 ` Stefano Stabellini
2017-08-24 0:51 ` Zhongze Liu
1 sibling, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2017-08-23 9:55 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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 ...
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -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);
> }
... you use "or" here and ...
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -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);
> }
... 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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-23 9:55 ` Jan Beulich
@ 2017-08-23 17:16 ` Stefano Stabellini
2017-08-24 6:32 ` Jan Beulich
2017-08-24 0:51 ` Zhongze Liu
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-23 17:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Zhongze Liu, George Dunlap, Andrew Cooper,
xen-devel, Julien Grall, Daniel De Graaf
On Wed, 23 Aug 2017, Jan Beulich wrote:
> >>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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 ...
>
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -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);
> > }
>
> ... you use "or" here and ...
>
> > --- a/xen/xsm/flask/hooks.c
> > +++ b/xen/xsm/flask/hooks.c
> > @@ -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);
> > }
>
> ... 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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-23 17:16 ` Stefano Stabellini
@ 2017-08-24 6:32 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2017-08-24 6:32 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Zhongze Liu, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 23.08.17 at 19:16, <sstabellini@kernel.org> wrote:
> On Wed, 23 Aug 2017, Jan Beulich wrote:
>> >>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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 ...
>>
>> > --- a/xen/include/xsm/dummy.h
>> > +++ b/xen/include/xsm/dummy.h
>> > @@ -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);
>> > }
>>
>> ... you use "or" here and ...
>>
>> > --- a/xen/xsm/flask/hooks.c
>> > +++ b/xen/xsm/flask/hooks.c
>> > @@ -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);
>> > }
>>
>> ... 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?
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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-23 9:55 ` Jan Beulich
2017-08-23 17:16 ` Stefano Stabellini
@ 2017-08-24 0:51 ` Zhongze Liu
2017-08-24 6:37 ` Jan Beulich
1 sibling, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-24 0:51 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
Thanks for reviewing my patch.
2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> 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 ...
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -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);
>> }
>
> ... 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.
>
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -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);
>> }
>
> ... 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.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-24 0:51 ` Zhongze Liu
@ 2017-08-24 6:37 ` Jan Beulich
2017-08-24 11:33 ` Zhongze Liu
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-08-24 6:37 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote:
> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> wrote:
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -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);
>>> }
>>
>> ... 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.
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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-24 6:37 ` Jan Beulich
@ 2017-08-24 11:33 ` Zhongze Liu
2017-08-24 12:39 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-24 11:33 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote:
>> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> wrote:
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -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);
>>>> }
>>>
>>> ... 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.
>
> 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
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-24 11:33 ` Zhongze Liu
@ 2017-08-24 12:39 ` Jan Beulich
2017-08-24 14:30 ` Daniel De Graaf
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2017-08-24 12:39 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, George Dunlap, Andrew Cooper, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 24.08.17 at 13:33, <blackskygg@gmail.com> wrote:
> Hi Jan,
>
> 2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote:
>>> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> wrote:
>>>>> --- a/xen/include/xsm/dummy.h
>>>>> +++ b/xen/include/xsm/dummy.h
>>>>> @@ -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);
>>>>> }
>>>>
>>>> ... 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.
>>
>> 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.
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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin
2017-08-24 12:39 ` Jan Beulich
@ 2017-08-24 14:30 ` Daniel De Graaf
0 siblings, 0 replies; 31+ messages in thread
From: Daniel De Graaf @ 2017-08-24 14:30 UTC (permalink / raw)
To: Jan Beulich, Zhongze Liu
Cc: George Dunlap, Andrew Cooper, Julien Grall, Stefano Stabellini,
xen-devel
On 08/24/2017 08:39 AM, Jan Beulich wrote:
>>>> On 24.08.17 at 13:33, <blackskygg@gmail.com> wrote:
>> Hi Jan,
>>
>> 2017-08-24 14:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 24.08.17 at 02:51, <blackskygg@gmail.com> wrote:
>>>> 2017-08-23 17:55 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>> On 22.08.17 at 20:08, <blackskygg@gmail.com> wrote:
>>>>>> --- a/xen/include/xsm/dummy.h
>>>>>> +++ b/xen/include/xsm/dummy.h
>>>>>> @@ -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);
>>>>>> }
>>>>>
>>>>> ... 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.
>>>
>>> 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.
>
> 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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (3 preceding siblings ...)
2017-08-22 18:08 ` [PATCH 4/6] xsm: flask: change the interface and default policy for xsm_map_gmfn_foregin Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-22 21:42 ` Stefano Stabellini
2017-08-25 11:05 ` Wei Liu
2017-08-22 18:08 ` [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
5 siblings, 2 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Zhongze Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich
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
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
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
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 3b63fb2cad..fd624b28f3 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -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
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 5e1fc6060e..1d681d8863 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -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
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d888eb..0975109c0c 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -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
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1158303e1a..8e5ec486d2 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -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);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 724750967c..74bc0acb21 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -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
diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
new file mode 100644
index 0000000000..e16c24ccb9
--- /dev/null
+++ b/tools/libxl/libxl_sshm.c
@@ -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;
+ 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".",
+ 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,
+ 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 */
+ 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);
+ 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:
+ */
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 455f6f0bed..8dd34fd7ba 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -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
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index c4a18df353..d91fbf5fda 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -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)
{
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-22 18:08 ` [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2017-08-22 21:42 ` Stefano Stabellini
2017-08-23 2:37 ` Zhongze Liu
2017-08-25 11:05 ` Wei Liu
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-22 21:42 UTC (permalink / raw)
To: Zhongze Liu
Cc: Tim Deegan, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, xen-devel, Julien Grall, Jan Beulich
On Wed, 23 Aug 2017, Zhongze Liu wrote:
> 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
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> 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
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 3b63fb2cad..fd624b28f3 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -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
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc6060e..1d681d8863 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -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
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index d842d888eb..0975109c0c 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -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
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1158303e1a..8e5ec486d2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -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;
I think it makes sense to call libxl__sshm_check_overlap only if
num_sshms != 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);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 724750967c..74bc0acb21 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -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
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> new file mode 100644
> index 0000000000..e16c24ccb9
> --- /dev/null
> +++ b/tools/libxl/libxl_sshm.c
> @@ -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;
> + 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).
> + 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,
> + 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
> + 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?
> + 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:
> + */
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 455f6f0bed..8dd34fd7ba 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -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
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index c4a18df353..d91fbf5fda 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -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)
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-22 21:42 ` Stefano Stabellini
@ 2017-08-23 2:37 ` Zhongze Liu
0 siblings, 0 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-23 2:37 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Tim Deegan, Wei Liu, George Dunlap, Andrew Cooper, Ian Jackson,
xen-devel, Julien Grall, Jan Beulich
Hi Stefano,
2017-08-23 5:42 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Wed, 23 Aug 2017, Zhongze Liu wrote:
>> 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
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> 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
>>
>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
>> index 3b63fb2cad..fd624b28f3 100644
>> --- a/tools/libxl/Makefile
>> +++ b/tools/libxl/Makefile
>> @@ -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
>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>> index 5e1fc6060e..1d681d8863 100644
>> --- a/tools/libxl/libxl_arch.h
>> +++ b/tools/libxl/libxl_arch.h
>> @@ -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
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index d842d888eb..0975109c0c 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -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
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 1158303e1a..8e5ec486d2 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -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;
>
> 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.
>
>
>> + 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);
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 724750967c..74bc0acb21 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -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
>> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
>> new file mode 100644
>> index 0000000000..e16c24ccb9
>> --- /dev/null
>> +++ b/tools/libxl/libxl_sshm.c
>> @@ -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;
>> + 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.
>
>
>> + 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,
>> + 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.
>
>
>> + 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?
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
>
>
>
>> + 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:
>> + */
>> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
>> index 455f6f0bed..8dd34fd7ba 100644
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -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
>> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
>> index c4a18df353..d91fbf5fda 100644
>> --- a/tools/libxl/libxl_xshelp.c
>> +++ b/tools/libxl/libxl_xshelp.c
>> @@ -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)
>> {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-22 18:08 ` [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
2017-08-22 21:42 ` Stefano Stabellini
@ 2017-08-25 11:05 ` Wei Liu
2017-08-25 12:02 ` Zhongze Liu
1 sibling, 1 reply; 31+ messages in thread
From: Wei Liu @ 2017-08-25 11:05 UTC (permalink / raw)
To: Zhongze Liu
Cc: Tim Deegan, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, xen-devel, Julien Grall, Jan Beulich
On Wed, Aug 23, 2017 at 02:08:39AM +0800, Zhongze Liu wrote:
[...]
> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
> index 5e1fc6060e..1d681d8863 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -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.
[...]
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 1158303e1a..8e5ec486d2 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -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;
Better move this after setting default.
> +
> + 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);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 724750967c..74bc0acb21 100644
[...]
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> new file mode 100644
> index 0000000000..e16c24ccb9
> --- /dev/null
> +++ b/tools/libxl/libxl_sshm.c
> @@ -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)
Indentation.
> +{
> + 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.
> + /* role-specific checks */
> + if (LIBXL_SSHM_ROLE_SLAVE == sshm->role) {
Style.
> + 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;
> + }
> + }
Please use goto style error handling.
> +
> + 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)
Style.
> +/* 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".",
> + sshm->begin + (offset << 12) );
> + rc = ERROR_FAIL;
> + }
> + }
> + if (rc) goto out;
> +
How is partial failure handled?
> + rc = 0;
> +
> +out:
> + return rc;
> +}
> +
> +static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm)
> +{
[...]
> +
> + /* check if the slave is asking too much permission */
> + if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
> + sshm->prot = master_sshm.prot;
> + }
Style.
> +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.
> +
> + 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:
> + */
[...]
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index c4a18df353..d91fbf5fda 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -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.
> + 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
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-25 11:05 ` Wei Liu
@ 2017-08-25 12:02 ` Zhongze Liu
2017-08-25 12:09 ` Wei Liu
0 siblings, 1 reply; 31+ messages in thread
From: Zhongze Liu @ 2017-08-25 12:02 UTC (permalink / raw)
To: Wei Liu
Cc: Tim Deegan, Stefano Stabellini, George Dunlap, Andrew Cooper,
Ian Jackson, xen-devel, Julien Grall, Jan Beulich
Hi Wei,
2017-08-25 19:05 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Wed, Aug 23, 2017 at 02:08:39AM +0800, Zhongze Liu wrote:
> [...]
>> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
>> index 5e1fc6060e..1d681d8863 100644
>> --- a/tools/libxl/libxl_arch.h
>> +++ b/tools/libxl/libxl_arch.h
>> @@ -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.
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.
>
> [...]
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 1158303e1a..8e5ec486d2 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -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;
>
> Better move this after setting default.
OK.
>
>> +
>> + 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);
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 724750967c..74bc0acb21 100644
> [...]
>> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
>> new file mode 100644
>> index 0000000000..e16c24ccb9
>> --- /dev/null
>> +++ b/tools/libxl/libxl_sshm.c
>> @@ -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)
>
> Indentation.
>
>> +{
>> + 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.
>
>> + /* 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.
>
>> + 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;
>> + }
>> + }
>
> Please use goto style error handling.
>
>> +
>> + 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)
>
> Style.
>
>> +/* 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".",
>> + 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?
>
>> + rc = 0;
>> +
>> +out:
>> + return rc;
>> +}
>> +
>> +static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
>> + libxl_static_shm *sshm)
>> +{
> [...]
>> +
>> + /* check if the slave is asking too much permission */
>> + if (LIBXL_SSHM_PROT_UNKNOWN == sshm->prot) {
>> + sshm->prot = master_sshm.prot;
>> + }
>
> Style.
>
>> +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.
>
>> +
>> + 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:
>> + */
> [...]
>> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
>> index c4a18df353..d91fbf5fda 100644
>> --- a/tools/libxl/libxl_xshelp.c
>> +++ b/tools/libxl/libxl_xshelp.c
>> @@ -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.
>
>> + 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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation
2017-08-25 12:02 ` Zhongze Liu
@ 2017-08-25 12:09 ` Wei Liu
0 siblings, 0 replies; 31+ messages in thread
From: Wei Liu @ 2017-08-25 12:09 UTC (permalink / raw)
To: Zhongze Liu
Cc: Tim Deegan, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, xen-devel, Julien Grall, Jan Beulich
On Fri, Aug 25, 2017 at 08:02:55PM +0800, Zhongze Liu wrote:
> Hi Wei,
>
> >> +/* 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".",
> >> + 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?
You should be able to roll back by calling remove_from_physmap if
necessary.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction
2017-08-22 18:08 [PATCH 0/6] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (4 preceding siblings ...)
2017-08-22 18:08 ` [PATCH 5/6] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2017-08-22 18:08 ` Zhongze Liu
2017-08-22 21:31 ` Stefano Stabellini
2017-08-25 11:05 ` Wei Liu
5 siblings, 2 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-22 18:08 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson,
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
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: xen-devel@lists.xen.org
---
tools/libxl/libxl_domain.c | 5 ++
tools/libxl/libxl_internal.h | 2 +
tools/libxl/libxl_sshm.c | 125 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 132 insertions(+)
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 08eccd082b..73ac856fb4 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -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);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 74bc0acb21..648eaee8c2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -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,
diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
index e16c24ccb9..417a4cd0a4 100644
--- a/tools/libxl/libxl_sshm.c
+++ b/tools/libxl/libxl_sshm.c
@@ -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);
+
+ /* 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))) {
+ 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 */
+ 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)))
+ {
+ 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,
--
2.14.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction
2017-08-22 18:08 ` [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
@ 2017-08-22 21:31 ` Stefano Stabellini
2017-08-23 2:43 ` Zhongze Liu
2017-08-25 11:05 ` Wei Liu
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2017-08-22 21:31 UTC (permalink / raw)
To: Zhongze Liu
Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson, xen-devel
On Wed, 23 Aug 2017, Zhongze Liu wrote:
> 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
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: xen-devel@lists.xen.org
> ---
> tools/libxl/libxl_domain.c | 5 ++
> tools/libxl/libxl_internal.h | 2 +
> tools/libxl/libxl_sshm.c | 125 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 132 insertions(+)
>
> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> index 08eccd082b..73ac856fb4 100644
> --- a/tools/libxl/libxl_domain.c
> +++ b/tools/libxl/libxl_domain.c
> @@ -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);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 74bc0acb21..648eaee8c2 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -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,
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> index e16c24ccb9..417a4cd0a4 100644
> --- a/tools/libxl/libxl_sshm.c
> +++ b/tools/libxl/libxl_sshm.c
> @@ -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);
> +
> + /* 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?
> + 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 */
> + 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
> + {
> + 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,
> --
> 2.14.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction
2017-08-22 21:31 ` Stefano Stabellini
@ 2017-08-23 2:43 ` Zhongze Liu
0 siblings, 0 replies; 31+ messages in thread
From: Zhongze Liu @ 2017-08-23 2:43 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Wei Liu, Julien Grall, Ian Jackson, xen-devel
Hi Stefano,
2017-08-23 5:31 GMT+08:00 Stefano Stabellini <sstabellini@kernel.org>:
> On Wed, 23 Aug 2017, Zhongze Liu wrote:
>> 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
>>
>> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
>>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: xen-devel@lists.xen.org
>> ---
>> tools/libxl/libxl_domain.c | 5 ++
>> tools/libxl/libxl_internal.h | 2 +
>> tools/libxl/libxl_sshm.c | 125 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 132 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
>> index 08eccd082b..73ac856fb4 100644
>> --- a/tools/libxl/libxl_domain.c
>> +++ b/tools/libxl/libxl_domain.c
>> @@ -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);
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 74bc0acb21..648eaee8c2 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -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,
>> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
>> index e16c24ccb9..417a4cd0a4 100644
>> --- a/tools/libxl/libxl_sshm.c
>> +++ b/tools/libxl/libxl_sshm.c
>> @@ -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);
>> +
>> + /* 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
>
>
>> + 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 */
>> + 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
>
>
>> + {
>> + 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,
>> --
>> 2.14.0
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction
2017-08-22 18:08 ` [PATCH 6/6] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
2017-08-22 21:31 ` Stefano Stabellini
@ 2017-08-25 11:05 ` Wei Liu
1 sibling, 0 replies; 31+ messages in thread
From: Wei Liu @ 2017-08-25 11:05 UTC (permalink / raw)
To: Zhongze Liu
Cc: Wei Liu, Julien Grall, Stefano Stabellini, Ian Jackson, xen-devel
On Wed, Aug 23, 2017 at 02:08:40AM +0800, Zhongze Liu wrote:
> Add libxl__sshm_del to Unmap static shared memory areas mapped by
Unmap -> unmap
> 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
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread