* [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files
@ 2018-01-30 17:50 Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
` (6 more replies)
0 siblings, 7 replies; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Hi,
This series implements the new xl config entry proposed in [1]. Users can use
the new config entry to statically setup shared memory areas among VMs that
don't have grant table support so that they could communicate with each other
through the static shared memory areas.
After several rounds of reviews, the current design is somewhat different
from what was described in the original proposal. So please refer to the
documentation in [patch 7/7] for the current design.
[1] Proposal to allow setting up shared memory areas between VMs from xl
config file:
https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
v3:
* Added the docs
* Changed the order of patches to reflect their internal dependencies
* Fixed the error handling when memory mapping are done but the xs
trasaction fails
* Changed the xsm hooks to lookup the current domain themselves instead of
getting it as a parameter
v4:
* Place the sshm info, which should not be readable by guests, under the
/libxl/static_shm xs path.
* Add a new subop to xenmem_add_to_physmap() to support the memory sharing,
instead of modifying an old one.
Cheers,
Zhongze Liu (7):
libxc: add xc_domain_remove_from_physmap to wrap
XENMEM_remove_from_physmap
xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
libxl: introduce a new structure to represent static shared memory
regions
libxl: support mapping static shared memory areas during domain
creation
libxl: support unmapping static shared memory areas during domain
destruction
libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config
files
docs: documentation about static shared memory regions
docs/man/xl-static-shm-configuration.pod.5 | 257 +++++++++++++++
docs/man/xl.cfg.pod.5.in | 8 +
docs/misc/xenstore-paths.markdown | 47 +++
tools/flask/policy/modules/xen.if | 2 +
tools/libxc/include/xenctrl.h | 4 +
tools/libxc/xc_domain.c | 11 +
tools/libxl/Makefile | 4 +-
tools/libxl/libxl.h | 4 +
tools/libxl/libxl_arch.h | 6 +
tools/libxl/libxl_arm.c | 15 +
tools/libxl/libxl_create.c | 27 ++
tools/libxl/libxl_domain.c | 5 +
tools/libxl/libxl_internal.h | 16 +
tools/libxl/libxl_sshm.c | 503 +++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 32 +-
tools/libxl/libxl_x86.c | 19 ++
tools/libxl/libxlu_sshm.c | 210 ++++++++++++
tools/libxl/libxlutil.h | 6 +
tools/xl/xl_parse.c | 24 +-
xen/arch/arm/mm.c | 8 +-
xen/arch/x86/mm.c | 4 +
xen/include/public/memory.h | 4 +
xen/include/xsm/dummy.h | 6 +
xen/include/xsm/xsm.h | 6 +
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 7 +
xen/xsm/flask/policy/access_vectors | 5 +
27 files changed, 1235 insertions(+), 6 deletions(-)
create mode 100644 docs/man/xl-static-shm-configuration.pod.5
create mode 100644 tools/libxl/libxl_sshm.c
create mode 100644 tools/libxl/libxlu_sshm.c
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v4 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing Zhongze Liu
` (5 subsequent siblings)
6 siblings, 0 replies; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file". See:
https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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>
Acked-by: Wei Liu <wei.liu2@citrix.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/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 235b8bb847..543abfcb34 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1416,6 +1416,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,
+ uint32_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 da0aa2f6a8..ea3df1ef31 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1090,6 +1090,17 @@ out:
return rc;
}
+int xc_domain_remove_from_physmap(xc_interface *xch,
+ uint32_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.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-02-01 10:23 ` Jan Beulich
2018-02-06 11:04 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
` (4 subsequent siblings)
6 siblings, 2 replies; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Zhongze Liu, Ian Jackson,
Julien Grall, Daniel De Graaf
The existing XENMAPSPACE_gmfn_foreign subop of XENMEM_add_to_physmap forbids
a Dom0 to map memory pages from one DomU to another, which restricts some useful
yet not dangerous use cases -- such as sharing pages among DomU's so that they
can do shm-based communication.
This patch introduces XENMAPSPACE_gmfn_share to address this inconvenience,
which is mostly the same as XENMAPSPACE_gmfn_foreign but has its own xsm check.
Specifically, the patch:
* Introduces a new av permission MMU__SHARE_MEM to denote if two domains can
share memory by using the new subop;
* Introduces xsm_map_gmfn_share() to check if (current) has proper permission
over (t) AND MMU__SHARE_MEM is allowed between (d) and (t);
* Modify the default xen.te to allow MMU__SHARE_MEM for normal domains that
allow grant mapping/event channels.
The new subop is marked unsupported for x86 because calling p2m_add_foregin
on two DomU's is currently not supported on x86.
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file" (see [1]).
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
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
---
V3:
* Change several if statements to the GCC '... = a ?: b' extention.
* Lookup the current domain in the hooks instead of passing it as an arg.
V4:
* Eliminate the duplicated (c) over (d) check.
* Added a new subop instead of modifying the old subop.
---
tools/flask/policy/modules/xen.if | 2 ++
xen/arch/arm/mm.c | 8 +++++++-
xen/arch/x86/mm.c | 4 ++++
xen/include/public/memory.h | 4 ++++
xen/include/xsm/dummy.h | 6 ++++++
xen/include/xsm/xsm.h | 6 ++++++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 7 +++++++
xen/xsm/flask/policy/access_vectors | 5 +++++
9 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 459880bb01..f02d49114f 100644
--- 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;
')
# domain_self_comms(domain)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 3c328e2df5..8e385d62a8 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1251,6 +1251,7 @@ int xenmem_add_to_physmap_one(
break;
case XENMAPSPACE_gmfn_foreign:
+ case XENMAPSPACE_gmfn_share:
{
struct domain *od;
p2m_type_t p2mt;
@@ -1265,7 +1266,12 @@ int xenmem_add_to_physmap_one(
return -EINVAL;
}
- rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+ if ( space == XENMAPSPACE_gmfn_foreign ) {
+ rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+ } else {
+ rc = xsm_map_gmfn_share(XSM_TARGET, d, od);
+ }
+
if ( rc )
{
rcu_unlock_domain(od);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c732734ac1..684403f737 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4126,6 +4126,10 @@ int xenmem_add_to_physmap_one(
}
case XENMAPSPACE_gmfn_foreign:
return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
+ case XENMAPSPACE_gmfn_share:
+ gdprintk(XENLOG_WARNING,
+ "XENMAPSPACE_gmfn_share is currently not supported on x86\n");
+ break;
default:
break;
}
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 29386df98b..ce70788660 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -227,6 +227,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
Stage-2 using the Normal Memory
Inner/Outer Write-Back Cacheable
memory attribute. */
+#define XENMAPSPACE_gmfn_share 6 /* Same as *_gmfn_foreign, but this is
+ for a privileged dom to share pages
+ between two doms. */
+
/* ` } */
/*
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index d6ddadcafd..cda87dea12 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
return xsm_default_action(action, d, t);
}
+static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+{
+ XSM_ASSERT_ACTION(XSM_TARGET);
+ return xsm_default_action(action, current->domain, t);
+}
+
static XSM_INLINE int xsm_hvm_param(XSM_DEFAULT_ARG struct domain *d, unsigned long op)
{
XSM_ASSERT_ACTION(XSM_TARGET);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index e3912bcc9d..1305b16973 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -86,6 +86,7 @@ struct xsm_operations {
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_share) (struct domain *d, struct domain *t);
int (*claim_pages) (struct domain *d);
int (*console_io) (struct domain *d, int cmd);
@@ -375,6 +376,11 @@ static inline int xsm_map_gmfn_foreign (xsm_default_t def, struct domain *d, str
return xsm_ops->map_gmfn_foreign(d, t);
}
+static inline int xsm_map_gmfn_share (xsm_default_t def, struct domain *d, struct domain *t)
+{
+ return xsm_ops->map_gmfn_share(d, t);
+}
+
static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
{
return xsm_ops->claim_pages(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 479b103614..3017dfc9a6 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -124,6 +124,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, add_to_physmap);
set_to_dummy_if_null(ops, remove_from_physmap);
set_to_dummy_if_null(ops, map_gmfn_foreign);
+ set_to_dummy_if_null(ops, map_gmfn_share);
set_to_dummy_if_null(ops, vm_event_control);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 1802d8dfe6..d4a8153301 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
}
+static int flask_map_gmfn_share(struct domain *d, struct domain *t)
+{
+ return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
+}
+
static int flask_hvm_param(struct domain *d, unsigned long op)
{
u32 perm;
@@ -1815,6 +1821,7 @@ static struct xsm_operations flask_ops = {
.add_to_physmap = flask_add_to_physmap,
.remove_from_physmap = flask_remove_from_physmap,
.map_gmfn_foreign = flask_map_gmfn_foreign,
+ .map_gmfn_share = flask_map_gmfn_share,
#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
.get_device_group = flask_get_device_group,
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 89b99966bb..e249a68728 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -383,6 +383,11 @@ class mmu
# Allow a privileged domain to install a map of a page it does not own. Used
# for stub domain device models with the PV framebuffer.
target_hack
+# Checked when using XENMEM_add_to_physmap with XENMAPSPACE_gmfn_share
+# to share memory between two domains:
+# source = domain whose memory is being shared
+# target = client domain
+ share_mem
}
# control of the paging_domctl split by subop
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-02-06 11:27 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
` (3 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add a new structure to the IDL familiy 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]).
And deleted some trailing white spaces.
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Wei Liu <wei.liu2@citrix.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 | 32 ++++++++++++++++++++++++++++++--
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index eca0ea2c50..372ad3cd32 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2365,6 +2365,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 35038120ca..4e9accf168 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -559,10 +559,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),
@@ -816,6 +816,33 @@ libxl_device_vdispl = Struct("device_vdispl", [
("connectors", Array(libxl_connector_param, "num_connectors"))
])
+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),
@@ -835,6 +862,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.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (2 preceding siblings ...)
2018-01-30 17:50 ` [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-02-06 13:07 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
` (2 subsequent siblings)
6 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
process involves the follwing steps:
* Set defaults and check for further errors in the static_shm configs:
overlapping areas, invalid ranges, duplicated master domain,
not page aligned, no master domain etc.
* Use xc_domain_add_to_physmap_batch to do the page sharing.
* When some of the pages can't be successfully mapped, roll back any
successfully mapped pages so that the system stays in a consistent state.
* Write infomation about static shared memory areas into the appropriate
xenstore paths and set the refcount of the shared region accordingly.
Temporarily mark this as unsupported on x86 because 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 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.xen.org/archives/html/xen-devel/2017-08/msg03242.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
---
V3:
* Unmap the successfully mapped pages whenever rc != 0.
V4:
* Use XENMAPSPACE_gmfn_share instead of *_gmfn_foreign.
---
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 | 397 +++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_x86.c | 19 +++
7 files changed, 479 insertions(+), 1 deletion(-)
create mode 100644 tools/libxl/libxl_sshm.c
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 917ceb0e72..e2834d84dc 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -139,7 +139,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_vdispl.o \
+ libxl_9pfs.o libxl_domain.o libxl_vdispl.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 784ec7f609..11433fe466 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -78,6 +78,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 3e46554301..e14879ec08 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1154,6 +1154,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 c498135246..98c16867bf 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -532,6 +532,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;
@@ -957,6 +965,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;
+ }
+
+ 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 shm");
+ goto error_out;
+ }
+ }
+
+ ret = libxl__sshm_check_overlap(gc, domid,
+ d_config->sshms, d_config->num_sshms);
+ if (ret) 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 506687fbe9..2cfe4c08a7 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4415,6 +4415,20 @@ static inline bool libxl__string_is_default(char **s)
}
#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..562f46f299
--- /dev/null
+++ b/tools/libxl/libxl_sshm.c
@@ -0,0 +1,397 @@
+#include "libxl_osdeps.h"
+#include "libxl_internal.h"
+#include "libxl_arch.h"
+
+#define SSHM_PATH(id) GCSPRINTF("/libxl/static_shm/%s", id)
+
+#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 (sshm->role == LIBXL_SSHM_ROLE_SLAVE) {
+ 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");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+ } else {
+ if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
+ SSHM_ERROR(domid, sshm->id,
+ "offset is only applicable to slave domains");
+ rc = ERROR_INVAL;
+ goto out;
+ }
+
+ rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
+ if (rc) {
+ SSHM_ERROR(domid, sshm->id,
+ "cache policy not supported on this platform");
+ goto out;
+ }
+ }
+
+ rc = 0;
+out:
+ return rc;
+}
+
+/* Comparator 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;
+
+ if (!len) return 0;
+
+ slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
+ num_slaves = 0;
+ for (i = 0; i < len; ++i) {
+ if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
+ 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;
+}
+
+/* libxl__sshm_do_map -- map pages into slave's physmap
+ *
+ * This functions maps
+ * master gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)
+ * into
+ * slave gfn: [@sshm->begin, @sshm->end)
+ *
+ * The gfns of the pages that are successfully mapped will be stored
+ * in @mapped, and the number of the gfns will be stored in @nmapped.
+ *
+ * The caller have to guarentee that sshm->begin < sshm->end and all the
+ * values are page-aligned.
+ */
+static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
+ libxl_static_shm *sshm, libxl_static_shm *msshm,
+ xen_pfn_t *mapped, unsigned int *nmapped)
+{
+ int rc;
+ int i;
+ unsigned int num_mpages, num_spages, num_success, offset;
+ int *errs;
+ xen_ulong_t *idxs;
+ xen_pfn_t *gpfns;
+
+ num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
+ num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;
+ offset = sshm->offset >> XC_PAGE_SHIFT;
+
+ /* 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] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
+ gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
+ }
+ rc = xc_domain_add_to_physmap_batch(CTX->xch,
+ sid, mid,
+ XENMAPSPACE_gmfn_share,
+ num_spages,
+ idxs, gpfns, errs);
+
+ num_success = 0;
+ for (i = 0; i < num_spages; i++) {
+ if (errs[i]) {
+ SSHM_ERROR(sid, sshm->id,
+ "can't map at address 0x%"PRIx64".",
+ gpfns[i] << XC_PAGE_SHIFT);
+ rc = ERROR_FAIL;
+ } else {
+ mapped[num_success++] = gpfns[i];
+ }
+ }
+ *nmapped = num_success;
+ if (rc) goto out;
+
+ rc = 0;
+out:
+ return rc;
+}
+
+static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
+ const char *sshm_path)
+{
+ int rc, count;
+ const char *count_path, *count_string;
+
+ count_path = GCSPRINTF("%s/usercnt", sshm_path);
+ rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
+ if (rc) goto out;
+ count = atoi(count_string);
+
+ count_string = GCSPRINTF("%d", count+1);
+ rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
+ 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, i;
+ const char *sshm_path, *slave_path;
+ const char *dom_path, *dom_sshm_path, *dom_role_path;
+ const char *xs_value;
+ char *ents[9];
+ libxl_static_shm master_sshm;
+ uint32_t master_domid;
+ xen_pfn_t *mapped;
+ unsigned int nmapped = 0;
+ xs_transaction_t xt = XBT_NULL;
+ bool isretry;
+
+ sshm_path = SSHM_PATH(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;
+
+ mapped = libxl__calloc(gc, (sshm->end - sshm->begin) >> XC_PAGE_SHIFT,
+ sizeof(xen_pfn_t));
+
+ isretry = false;
+ 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);
+
+ if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
+ sshm->prot = master_sshm.prot;
+
+ /* check if the slave is asking too much permission */
+ 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 */
+ if (!isretry) {
+ rc = libxl__sshm_do_map(gc, master_domid, domid,
+ sshm, &master_sshm,
+ mapped, &nmapped);
+ if (rc) goto out;
+ }
+
+ /* write the result to xenstore and commit */
+ rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
+ if (rc) goto out;
+ rc = libxl__xs_writev(gc, xt, slave_path, ents);
+ if (rc) goto out;
+ rc = libxl__sshm_incref(gc, xt, sshm_path);
+ if (rc) goto out;
+
+ rc = libxl__xs_transaction_commit(gc, &xt);
+ if (!rc) break;
+ if (rc < 0) goto out;
+ isretry = true;
+ }
+
+ rc = 0;
+out:
+ if (rc) {
+ /* roll back successfully mapped pages */
+ SSHM_ERROR(domid, sshm->id, "failed to map some pages, cancelling.");
+ for (i = 0; i < nmapped; i++) {
+ xc_domain_remove_from_physmap(CTX->xch, domid, mapped[i]);
+ }
+ }
+
+ 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;
+ const char *sshm_path, *dom_path, *dom_role_path;
+ char *ents[13];
+ struct xs_permissions noperm;
+ xs_transaction_t xt = XBT_NULL;
+
+ sshm_path = SSHM_PATH(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] = "usercnt";
+ ents[11] = "1";
+ 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;
+
+ for (i = 0; i < len; ++i) {
+ if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE) {
+ 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 5f91fe4f92..8ec5044eb2 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -596,6 +596,25 @@ 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 for 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
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (3 preceding siblings ...)
2018-01-30 17:50 ` [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-02-06 13:24 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 7/7] docs: documentation about static shared memory regions Zhongze Liu
6 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, 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: decrease the refcount of the sshm region, if the refcount
reaches 0, cleanup the whole sshm path.
* For a slave: 1) unmap the shared pages, and cleanup related xs entries. If
the system works normally, all the shared pages will be unmapped, so there
won't be page leaks. In case of errors, the unmapping process will go on and
unmap all the other pages that can be unmapped, so the other pages won't
be leaked, either. 2) Decrease the refcount of the sshm region, if the
refcount reaches 0, cleanup the whole sshm path.
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file" (see [1]).
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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 | 106 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+)
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 13b1c73d40..37f001554b 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1026,6 +1026,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 2cfe4c08a7..c398b6a6b8 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
_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 562f46f299..1bf4d4c2dc 100644
--- a/tools/libxl/libxl_sshm.c
+++ b/tools/libxl/libxl_sshm.c
@@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
return 0;
}
+/* Decrease the refcount of an sshm. When refcount reaches 0,
+ * clean up the whole sshm path.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+ const char *sshm_path)
+{
+ int count;
+ const char *count_path, *count_string;
+
+ count_path = GCSPRINTF("%s/usercnt", sshm_path);
+ if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
+ return;
+ count = atoi(count_string);
+
+ if (--count == 0) {
+ libxl__xs_path_cleanup(gc, xt, sshm_path);
+ return;
+ }
+
+ count_string = GCSPRINTF("%d", count);
+ libxl__xs_write_checked(gc, xt, count_path, count_string);
+
+ return;
+}
+
+static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char *id,
+ uint64_t begin, uint64_t end)
+{
+ begin >>= XC_PAGE_SHIFT;
+ end >>= XC_PAGE_SHIFT;
+ 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)
+{
+ const char *slave_path, *begin_str, *end_str;
+ uint64_t begin, end;
+
+ slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
+}
+
+/* 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;
+ const char *dom_path, *dom_sshm_path, *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);
+
+ libxl__sshm_decref(gc, xt, SSHM_PATH(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;
+}
+
/* libxl__sshm_do_map -- map pages into slave's physmap
*
* This functions maps
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (4 preceding siblings ...)
2018-01-30 17:50 ` [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 7/7] docs: documentation about static shared memory regions Zhongze Liu
6 siblings, 0 replies; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add the parsing utils for the newly introduced libxl_static_sshm struct
to the libxl/libxlu_* family. And add realated parsing code in xl to
parse the struct from xl config files. This is for the proposal "Allow
setting up shared memory areas between VMs from xl config file" (see [1]).
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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/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 e2834d84dc..dfd68bc2df 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -176,7 +176,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) $(CFLAGS_libxentoolcore)
diff --git a/tools/libxl/libxlu_sshm.c b/tools/libxl/libxlu_sshm.c
new file mode 100644
index 0000000000..5276ff9395
--- /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 "xenctrl.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 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);
+ }
+
+ sshm->id = strdup(val);
+ if (!sshm->id) {
+ 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 (val[0] == '0' && val[1] == 'x') { base = 16; }
+ new_addr = strtoull(val, &endptr, base);
+ if (errno == ERANGE || *endptr)
+ RET_INVAL("invalid begin/end/offset", val);
+ if (new_addr & ~XC_PAGE_MASK)
+ RET_INVAL("begin/end/offset is not a multiple of 4K", val);
+
+ /* begin or end */
+ if (key[0] == 'b') {
+ SET_VAL(sshm->begin, "beginning address", RANGE, new_addr, val);
+ } else if(key[0] == 'e'){
+ 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;
+ }
+
+ buf2 = ptr = strdup(spec);
+ if (!buf2) {
+ 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 f6842540ca..3221401dac 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -860,7 +860,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, *vdispls;
+ *usbctrls, *usbdevs, *p9devs, *vdispls, *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;
@@ -1566,6 +1566,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.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v4 7/7] docs: documentation about static shared memory regions
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
` (5 preceding siblings ...)
2018-01-30 17:50 ` [PATCH v4 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
@ 2018-01-30 17:50 ` Zhongze Liu
2018-02-06 13:28 ` Julien Grall
6 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-01-30 17:50 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, Zhongze Liu
Add docs to document the motivation, usage, use cases and other
relevant information about the static shared memory feature.
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file". See:
https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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
---
docs/man/xl-static-shm-configuration.pod.5 | 257 +++++++++++++++++++++++++++++
docs/man/xl.cfg.pod.5.in | 8 +
docs/misc/xenstore-paths.markdown | 47 ++++++
3 files changed, 312 insertions(+)
create mode 100644 docs/man/xl-static-shm-configuration.pod.5
diff --git a/docs/man/xl-static-shm-configuration.pod.5 b/docs/man/xl-static-shm-configuration.pod.5
new file mode 100644
index 0000000000..d68ed0ebf7
--- /dev/null
+++ b/docs/man/xl-static-shm-configuration.pod.5
@@ -0,0 +1,257 @@
+=head1 NAME
+
+xl-static-shm-configuration - XL Static Shared Memeory Configuration Syntax
+
+
+(B<NOTE>: This is currently only available to ARM guests.)
+
+=head1 DESCRIPTION
+
+The static_shm option allows users to statically setup shared memory regions
+among a group of VMs, enabling guests without grant table support to do
+shm-based communication.
+
+Every shared region is:
+
+=over 4
+
+* Uniquely identified by a string that is no longer than 128 characters, which
+is called an B<identifier> in this document.
+
+* Backed by exactely one domain, which is called a B<master> domain, and all
+the other domains who are also sharing this region are called B<slave>s.
+
+=back
+
+=head1 SYNTAX
+
+This document specifies syntax of the static shared memory configuration in
+the xl config file. It has the following form:
+
+ static_shm = [ "SSHM_SPEC", "SSHM_SPEC", ... ]
+
+where each C<SSHM_SPEC> is in this form:
+
+ [<key>=<value>,]*
+
+Valid examples of C<SSHM_SPEC> are:
+
+ id=ID1, begin=0x100000, end=0x200000, role=master, cache_policy=x86_normal
+ id=ID1, offset = 0, begin=0x500000, end=0x600000, role=slave, prot=rw
+ id=ID2, begin=0x300000, end=0x400000, role=master
+ id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
+ id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
+
+These might be specified in the domain config file like this:
+
+ static_shm = ["id=ID2, offset = 0x10000, begin=0x690000, end=0x800000,
+role=slave"]
+
+
+More formally, the string is a series of comma-separated keyword/value
+pairs. Each parameter may be specified at most once. Default values apply if
+the parameter is not specified.
+
+=head1 Parameters
+
+=over 4
+
+=item B<id>
+
+=over 4
+
+=item Description
+
+The unique identifier of the shared memory region.
+
+Every identifier could appear only once in each xl config file.
+
+=item Supported values
+
+A string that contains alphanumerics and "_"s, and is no longer than 128
+characters.
+
+=item Default value
+
+None, this parameter is mandatory.
+
+=back
+
+=item B<begin>/B<end>
+
+=over 4
+
+=item Description
+
+The boundaries of the shared memory area.
+
+=item Supported values
+
+Same with B<offset>.
+
+=item Default Value
+
+None, this parameter is mandatory.
+
+=back
+
+=item B<offset>
+
+=over 4
+
+=item Description
+
+Can only appear when B<role> = slave. If set, the address mapping will not
+start from the beginning the backing memory region, but from the middle
+(B<offet> bytes away from the beginning) of it. See the graph below:
+
+With B<offset> = 0, the mapping will look like:
+
+ backing memory region: #########################################
+ | |
+ | |
+ | |
+ V V
+ slave's shared region: #########################
+
+With B<offset> > 0:
+
+ backing memory region: #########################################
+ |<-- offset -->|| |
+ | |
+ | |
+ V V
+ slave's memory region: #########################
+
+=item Supported values
+
+Decimals or hexadecimals with a prefix "0x", and should be the multiple of the
+hypervisor page granularity (currently 4K on both ARM and x86).
+
+=item Default value
+
+0x0
+
+=back
+
+=item B<role>
+
+=over 4
+
+=item Description
+
+The backing area would be taken from one domain, which we will mark
+as the "master domain", and this domain should be created prior to any
+other slave domains that depend on it.
+
+This arugment specifies the role of this domain.
+
+=item Supported values
+
+master, slave
+
+=item Default value
+
+slave
+
+=back
+
+=item B<prot>
+
+=over 4
+
+=item Description
+
+When B<role> = master, this means the largest set of stage-2 permission flags
+that can be granted to the slave domains. When B<role> = slave, this means the
+stage-2 permission flags of the shared memory area.
+
+=item Supported values
+
+Currently only 'rw' is supported.
+
+=item Default value
+
+rw
+
+=back
+
+=item B<cache_policy>
+
+=over 4
+
+=item Description
+
+The stage-2 cacheability/shareability attributes of the shared memory area.
+This can only appear when B<role> = master.
+
+=item Supported values
+
+Currently, only the following policy is supported:
+
+=over 4
+
+=item B<ARM_normal>
+
+Only applicable to ARM guests. This would mean Inner and Outer Write-Back
+Cacheable, and Inner Shareable.
+
+=back
+
+=item Default value
+
+ARM_normal
+
+=back
+
+=back
+
+=head1 TYPICAL USAGE
+
+A typical procedure of setting up a shared mem region among several VMs is:
+
+=over 4
+
+1. Add a static_shm option to the master domain's xl config file, assign an
+B<ID> to it and mark it's B<role> as master, and set up the boundaries, prot
+flag, and B<cache_policy> appropriately.
+
+2. Add a static_shm option to every slave domain's xl config file, set
+their B<ID> to the same value as the master's, and set up the B<offset>,
+boundaries and prot flag appropriately.
+
+3. Create the master domain.
+
+4. Create the slaves.
+
+=back
+
+Remember that the master domain must be created before any slave domains could
+be created, for the slaves depend on the memory pages backed by their master.
+
+=head1 Example
+
+Suppose that we have 3 domains: vm1~vm3. And we want to setup two shared
+regions, say, ID1 and ID2, among the three domains, with the following address
+mapping:
+
+ ID1: (vm1 : 0x100000~0x200000) <=====> (vm2 : 0x500000~0x600000)
+ ID2: (vm1 : 0x310000~0x400000) <=====> (vm3 : 0x690000~0x800000)
+
+According to the syntax defined above, the xl config files of the three domains
+should contains the following content:
+
+In xl config file of vm1:
+ static_shm = [ "id=ID1, begin=0x100000, end=0x200000, role=master,
+cache_policy=x86_normal, prot=rw",
+"id=ID2, begin=0x300000, end=0x400000, role=master" ]
+
+In xl config file of vm2:
+ static_shm = [ "id=ID1, offset=0, begin=0x500000, end=0x600000,
+role=slave, prot=rw" ]
+
+In xl config file of vm3:
+ static_shm = [ "id=ID2, offset=0x10000, begin=0x690000,
+end=0x800000, role=slave" ]
+
+After that, just create vm1 first, and then create vm2 and vm3 in any order.
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index a699367779..c4d8d5e8ff 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -277,6 +277,14 @@ memory=8096 will report significantly less memory available for use
than a system with maxmem=8096 memory=8096 due to the memory overhead
of having to track the unused pages.
+=item B<static_shm=[ "SSHM_SPEC", "SSHM_SPEC", ... ]>
+
+Specifies the static shared memory regions of this guest. Static shared
+memory regions enables guests to communicate with each other through
+one or more shared memory regions, even without grant table support.
+Currently, this only works on ARM guests.
+See L<xl-static-shm-configuration(5)> for more details.
+
=back
=head3 Guest Virtual NUMA Configuration
diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
index 7be2592c74..33ad123839 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -174,6 +174,14 @@ than this amount of RAM.
The size of the video RAM this domain is configured with.
+#### ~/static_shm/[_a-zA-Z0-9]+/role = ("master"|"slave") []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The role of this domain in the static shared memory region whose id matches
+the `[_a-zA-Z0-9]+` part in the path. (Described in the manpage
+**xl-static-shm-configuration(5)**).
+
#### ~/device/suspend/event-channel = ""|EVTCHN [w]
The domain's suspend event channel. The toolstack will create this
@@ -539,6 +547,45 @@ type. The name of each backend directory is the same as the backend type
Contains the PIDs of the device models running on the domain.
+#### /libxl/static_shm/[_a-zA-Z0-9]+/* []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The following paths contain backing memory parameters of a static shared memory
+whose id matches the `[_a-zA-Z0-9]+` part in the path. Their formats and
+meanings are the same as those in an xl config file, described in the manpage
+**xl-static-shm-configuration(5)**.
+
+* begin/end: the boundary of the backing memory region.
+* prot: the largest set of stage-2 permission flags that can be granted to
+ the slave domains.
+* cache_policy: the stage-2 cacheability/shareability attributes of the backing
+ memory region.
+
+The following paths contain run-time information about the static shared memory
+region.
+
+* master: the domid of the backing domain.
+* slaves: information about the slaves that are sharing the region, see
+ ** /libxl/static_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* ** below.
+* usercnt: An integer. This is the reference count of the backing memory region,
+ including the master domain itself. When this value reachies 0, the backing
+ memory region will be freed.
+
+#### /libxl/staitc_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* []
+
+(Note: Currently, this will only appear on ARM guests.)
+
+The following paths contain static shared memory region parameters of a slave
+domain. Their formats and meanings are the same as those in xl config files,
+described in the manpage **xl-static-shm-configuration(5)**.
+
+* begin/end: the boundary of the shared memory region.
+* prot: the stage-2 permission flags of the shared memory area.
+* offset: when mapping the backing memory region to the slave's memory space,
+ the mapping will start from offset bytes after the beginning of the backing
+ memory region.
+
## Virtual Machine Paths
The /vm/$UUID namespace is used by toolstacks to store various
--
2.16.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-01-30 17:50 ` [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing Zhongze Liu
@ 2018-02-01 10:23 ` Jan Beulich
2018-02-01 18:11 ` Zhongze Liu
2018-02-13 15:15 ` Zhongze Liu
2018-02-06 11:04 ` Julien Grall
1 sibling, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2018-02-01 10:23 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 30.01.18 at 18:50, <blackskygg@gmail.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4126,6 +4126,10 @@ int xenmem_add_to_physmap_one(
> }
> case XENMAPSPACE_gmfn_foreign:
> return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
> + case XENMAPSPACE_gmfn_share:
> + gdprintk(XENLOG_WARNING,
> + "XENMAPSPACE_gmfn_share is currently not supported on x86\n");
> + break;
Please don't - a hypervisor log message isn't really useful here. It
should be the tool stack to disallow respective options on x86
until that's implemented.
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -227,6 +227,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
> Stage-2 using the Normal Memory
> Inner/Outer Write-Back Cacheable
> memory attribute. */
> +#define XENMAPSPACE_gmfn_share 6 /* Same as *_gmfn_foreign, but this is
> + for a privileged dom to share pages
> + between two doms. */
> +
The comment doesn't make clear why XENMAPSPACE_gmfn_foreign
then can't be used. In particular it is left open how _both_ domains
would be specified.
Also XENMAPSPACE_gmfn_foreign is restricted to
XENMEM_add_to_physmap_batch (a comment says so) - how about
this new one? According to the actual code changes you do, there's
no meaningful difference, in which case the restriction should be
named here as well.
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
> return xsm_default_action(action, d, t);
> }
>
> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
Line length.
> +{
> + XSM_ASSERT_ACTION(XSM_TARGET);
> + return xsm_default_action(action, current->domain, t);
How does this represent a proper default equivalent of ...
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
}
+static int flask_map_gmfn_share(struct domain *d, struct domain *t)
+{
+ return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
+ domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
... this?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-01 10:23 ` Jan Beulich
@ 2018-02-01 18:11 ` Zhongze Liu
2018-02-02 8:32 ` Jan Beulich
2018-02-13 15:15 ` Zhongze Liu
1 sibling, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-01 18:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 30.01.18 at 18:50, <blackskygg@gmail.com> wrote:
[...]
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>> return xsm_default_action(action, d, t);
>> }
>>
>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
>
> Line length.
>
>> +{
>> + XSM_ASSERT_ACTION(XSM_TARGET);
>> + return xsm_default_action(action, current->domain, t);
>
> How does this represent a proper default equivalent of ...
>
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
> return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
> }
>
> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
> +{
> + return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>
> ... this?
The full flask check tries to guarantee that:
1) (c) over (d), which will always be checked somewhere higher in
the call stack for these kinds of calls;
2) (c) over (t), namely, MMU__MAP_READ/WRITE;
3) (d) over (t), namely, MMU__SHARE_MEM
In my default builtin actions, checks 1) and 2) are done by the
xsm_default_action
function, but I can't think of a way to do check 3), because we don't
have the proper
equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a choice).
Want to hear your and other maintainers' suggestions about how to do
this properly.
>
> Jan
>
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-01 18:11 ` Zhongze Liu
@ 2018-02-02 8:32 ` Jan Beulich
2018-02-05 9:59 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2018-02-02 8:32 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 01.02.18 at 19:11, <blackskygg@gmail.com> wrote:
> 2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 30.01.18 at 18:50, <blackskygg@gmail.com> wrote:
>>> --- a/xen/include/xsm/dummy.h
>>> +++ b/xen/include/xsm/dummy.h
>>> @@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>>> return xsm_default_action(action, d, t);
>>> }
>>>
>>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
>>> +{
>>> + XSM_ASSERT_ACTION(XSM_TARGET);
>>> + return xsm_default_action(action, current->domain, t);
>>
>> How does this represent a proper default equivalent of ...
>>
>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>> return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
>> }
>>
>> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
>> +{
>> + return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
>> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>
>> ... this?
>
> The full flask check tries to guarantee that:
> 1) (c) over (d), which will always be checked somewhere higher in
> the call stack for these kinds of calls;
This is in no way apparent here. At the very least a comment to
that effect should be added, but perhaps even better would be
if you had an explicit xsm_default_action(..., current->domain, d)
call there. Whether one or the other is preferable I'd leave to
Daniel.
> 2) (c) over (t), namely, MMU__MAP_READ/WRITE;
> 3) (d) over (t), namely, MMU__SHARE_MEM
>
> In my default builtin actions, checks 1) and 2) are done by the
> xsm_default_action
> function, but I can't think of a way to do check 3), because we don't
> have the proper
> equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a
> choice).
> Want to hear your and other maintainers' suggestions about how to do
> this properly.
"(d) over (t)" would suggest xsm_default_action(action, d, t), which
I don't think is appropriate here. In fact aiui d and t are unrelated to
one another in terms of mutual privilege. I don't think 3) needs
expressing in the dummy version; it's really the apparent lack of 1)
which I've been commenting on. But again, I'll leave it to Daniel to
tell you otherwise. What is imperative in any event is that you
extend the description to also reason about the dummy logic, at
least as long as it isn't a clear equivalent of the flask variant.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-02 8:32 ` Jan Beulich
@ 2018-02-05 9:59 ` Zhongze Liu
0 siblings, 0 replies; 45+ messages in thread
From: Zhongze Liu @ 2018-02-05 9:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
(Sorry for the late reply.)
2018-02-02 16:32 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 01.02.18 at 19:11, <blackskygg@gmail.com> wrote:
>> 2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 30.01.18 at 18:50, <blackskygg@gmail.com> wrote:
>>>> --- a/xen/include/xsm/dummy.h
>>>> +++ b/xen/include/xsm/dummy.h
>>>> @@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>>>> return xsm_default_action(action, d, t);
>>>> }
>>>>
>>>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
>>>> +{
>>>> + XSM_ASSERT_ACTION(XSM_TARGET);
>>>> + return xsm_default_action(action, current->domain, t);
>>>
>>> How does this represent a proper default equivalent of ...
>>>
>>> --- a/xen/xsm/flask/hooks.c
>>> +++ b/xen/xsm/flask/hooks.c
>>> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
>>> return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
>>> }
>>>
>>> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
>>> +{
>>> + return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
>>> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>>>
>>> ... this?
>>
>> The full flask check tries to guarantee that:
>> 1) (c) over (d), which will always be checked somewhere higher in
>> the call stack for these kinds of calls;
>
> This is in no way apparent here. At the very least a comment to
> that effect should be added, but perhaps even better would be
> if you had an explicit xsm_default_action(..., current->domain, d)
> call there. Whether one or the other is preferable I'd leave to
> Daniel.
>
During our discussion on v3 of the patch set, Daniel has already expressed
his preference not to duplicate the (c)-over-(d) check here, and that's why
I've eliminated the check in this version. But I do agree with you on adding a
comment here ...
>> 2) (c) over (t), namely, MMU__MAP_READ/WRITE;
>> 3) (d) over (t), namely, MMU__SHARE_MEM
>>
>> In my default builtin actions, checks 1) and 2) are done by the
>> xsm_default_action
>> function, but I can't think of a way to do check 3), because we don't
>> have the proper
>> equivalence of MMU__SHARE_MEM (xsm_default_action is definitely not a
>> choice).
>> Want to hear your and other maintainers' suggestions about how to do
>> this properly.
>
> "(d) over (t)" would suggest xsm_default_action(action, d, t), which
> I don't think is appropriate here. In fact aiui d and t are unrelated to
> one another in terms of mutual privilege. I don't think 3) needs
> expressing in the dummy version; it's really the apparent lack of 1)k
> which I've been commenting on. But again, I'll leave it to Daniel to
> tell you otherwise. What is imperative in any event is that you
> extend the description to also reason about the dummy logic, at
> least as long as it isn't a clear equivalent of the flask variant.
>
... and here.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-01-30 17:50 ` [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing Zhongze Liu
2018-02-01 10:23 ` Jan Beulich
@ 2018-02-06 11:04 ` Julien Grall
1 sibling, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-02-06 11:04 UTC (permalink / raw)
To: Zhongze Liu, xen-devel
Cc: Ian Jackson, Daniel De Graaf, Stefano Stabellini, Wei Liu
Hi,
On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3c328e2df5..8e385d62a8 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1251,6 +1251,7 @@ int xenmem_add_to_physmap_one(
>
> break;
> case XENMAPSPACE_gmfn_foreign:
> + case XENMAPSPACE_gmfn_share:
> {
> struct domain *od;
> p2m_type_t p2mt;
> @@ -1265,7 +1266,12 @@ int xenmem_add_to_physmap_one(
> return -EINVAL;
> }
>
> - rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
> + if ( space == XENMAPSPACE_gmfn_foreign ) {
> + rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
> + } else {
> + rc = xsm_map_gmfn_share(XSM_TARGET, d, od);
> + }
Coding style:
if ( .... )
{
}
else
{
}
But in that case, there braces are not necessary. So you could do
if ( ... )
...
else
...
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-01-30 17:50 ` [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
@ 2018-02-06 11:27 ` Julien Grall
2018-02-06 15:41 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-06 11:27 UTC (permalink / raw)
To: Zhongze Liu, xen-devel; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu
Hi,
On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> Add a new structure to the IDL familiy to represent static shared memory regions
s/familiy/family/
> as proposed in the proposal "Allow setting up shared memory areas between VMs
> from xl config file" (see [1]).
>
> And deleted some trailing white spaces.
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
>
> Signed-off-by: Zhongze Liu <blackskygg@gmail.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Acked-by: Wei Liu <wei.liu2@citrix.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 | 32 ++++++++++++++++++++++++++++++--
> 2 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index eca0ea2c50..372ad3cd32 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -2365,6 +2365,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
> +
You need to add LIBXL_HAVE_* in libxl.h to tell shared memory regions
are available. I wasn't able to find anything within this series.
> #include <libxl_event.h>
>
> #endif /* LIBXL_H */
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 35038120ca..4e9accf168 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -559,10 +559,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),
> @@ -816,6 +816,33 @@ libxl_device_vdispl = Struct("device_vdispl", [
> ("connectors", Array(libxl_connector_param, "num_connectors"))
> ])
>
> +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'}),
We might want to store the size rather than the end. This would allow us
to cover region up to the address 2^64-1.
Also, this would make clearer whether end is included in the region or not.
> + ("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),
> @@ -835,6 +862,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),
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-01-30 17:50 ` [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
@ 2018-02-06 13:07 ` Julien Grall
2018-02-06 15:59 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-06 13:07 UTC (permalink / raw)
To: Zhongze Liu, xen-devel; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu
Hi,
On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> Add libxl__sshm_add to map shared pages from one DomU to another, The mapping
> process involves the follwing steps:
s/follwing/following/
>
> * Set defaults and check for further errors in the static_shm configs:
> overlapping areas, invalid ranges, duplicated master domain,
> not page aligned, no master domain etc.
> * Use xc_domain_add_to_physmap_batch to do the page sharing.
> * When some of the pages can't be successfully mapped, roll back any
> successfully mapped pages so that the system stays in a consistent state.
> * Write infomation about static shared memory areas into the appropriate
s/infomation/information/
> xenstore paths and set the refcount of the shared region accordingly.
>
> Temporarily mark this as unsupported on x86 because 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 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.xen.org/archives/html/xen-devel/2017-08/msg03242.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
> ---
> V3:
> * Unmap the successfully mapped pages whenever rc != 0.
>
> V4:
> * Use XENMAPSPACE_gmfn_share instead of *_gmfn_foreign.
> ---
> 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 | 397 +++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/libxl_x86.c | 19 +++
> 7 files changed, 479 insertions(+), 1 deletion(-)
> create mode 100644 tools/libxl/libxl_sshm.c
>
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 917ceb0e72..e2834d84dc 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -139,7 +139,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_vdispl.o \
> + libxl_9pfs.o libxl_domain.o libxl_vdispl.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 784ec7f609..11433fe466 100644
> --- a/tools/libxl/libxl_arch.h
> +++ b/tools/libxl/libxl_arch.h
> @@ -78,6 +78,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 3e46554301..e14879ec08 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -1154,6 +1154,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 c498135246..98c16867bf 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -532,6 +532,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. */
s/the/The/
> + 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;
> @@ -957,6 +965,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;
> + }
> +
> + 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 shm");
> + goto error_out;
> + }
> + }
> +
> + ret = libxl__sshm_check_overlap(gc, domid,
> + d_config->sshms, d_config->num_sshms);
> + if (ret) 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 506687fbe9..2cfe4c08a7 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4415,6 +4415,20 @@ static inline bool libxl__string_is_default(char **s)
> }
> #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..562f46f299
> --- /dev/null
> +++ b/tools/libxl/libxl_sshm.c
> @@ -0,0 +1,397 @@
> +#include "libxl_osdeps.h"
> +#include "libxl_internal.h"
> +#include "libxl_arch.h"
> +
> +#define SSHM_PATH(id) GCSPRINTF("/libxl/static_shm/%s", id)
> +
> +#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;
What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?
> +
> + /* role-specific checks */
> + if (sshm->role == LIBXL_SSHM_ROLE_SLAVE) {
> + 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");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> + } else {
> + if (sshm->offset != LIBXL_SSHM_RANGE_UNKNOWN) {
> + SSHM_ERROR(domid, sshm->id,
> + "offset is only applicable to slave domains");
> + rc = ERROR_INVAL;
> + goto out;
> + }
> +
> + rc = libxl__arch_domain_sshm_cachepolicy_setdefault(sshm);
> + if (rc) {
> + SSHM_ERROR(domid, sshm->id,
> + "cache policy not supported on this platform");
> + goto out;
> + }
> + }
> +
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +/* Comparator 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 */
s/check/Check/
> +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;
> +
> + if (!len) return 0;
> +
> + slave_sshms = libxl__calloc(gc, len, sizeof(slave_sshms[0]));
> + num_slaves = 0;
> + for (i = 0; i < len; ++i) {
> + if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE)
> + 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;
> +}
> +
> +/* libxl__sshm_do_map -- map pages into slave's physmap
> + *
> + * This functions maps
> + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end + @sshm->offset)
> + * into
> + * slave gfn: [@sshm->begin, @sshm->end)
> + *
> + * The gfns of the pages that are successfully mapped will be stored
> + * in @mapped, and the number of the gfns will be stored in @nmapped.
> + *
> + * The caller have to guarentee that sshm->begin < sshm->end and all the
s/have to/has to/ I think.
s/guarentee/guarantee/
> + * values are page-aligned.
Hmmm, I don't see the alignement check in libxl. So do you rely on the
toolstack to do it?
> + */
> +static int libxl__sshm_do_map(libxl__gc *gc, uint32_t mid, uint32_t sid,
> + libxl_static_shm *sshm, libxl_static_shm *msshm,
> + xen_pfn_t *mapped, unsigned int *nmapped)
> +{
> + int rc;
> + int i;
> + unsigned int num_mpages, num_spages, num_success, offset;
A frame number may not fit into a 32-bit value. You would need to use
xen_pfn_t (64-bit) here.
> + int *errs;
> + xen_ulong_t *idxs;
> + xen_pfn_t *gpfns;
> +
> + num_mpages = (msshm->end - msshm->begin) >> XC_PAGE_SHIFT;
> + num_spages = (sshm->end - sshm->begin) >> XC_PAGE_SHIFT;
> + offset = sshm->offset >> XC_PAGE_SHIFT;
> +
> + /* 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 */
I think you mean gfn here and not pfn.
> + 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] = (msshm->begin >> XC_PAGE_SHIFT) + offset + i;
> + gpfns[i]= (sshm->begin >> XC_PAGE_SHIFT) + i;
> + }
> + rc = xc_domain_add_to_physmap_batch(CTX->xch,
> + sid, mid,
> + XENMAPSPACE_gmfn_share,
> + num_spages,
> + idxs, gpfns, errs);
> +
> + num_success = 0;
> + for (i = 0; i < num_spages; i++) {
> + if (errs[i]) {
> + SSHM_ERROR(sid, sshm->id,
> + "can't map at address 0x%"PRIx64".",
> + gpfns[i] << XC_PAGE_SHIFT);
> + rc = ERROR_FAIL;
> + } else {
> + mapped[num_success++] = gpfns[i];
> + }
> + }
> + *nmapped = num_success;
> + if (rc) goto out;
> +
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +static int libxl__sshm_incref(libxl__gc *gc, xs_transaction_t xt,
> + const char *sshm_path)
> +{
> + int rc, count;
> + const char *count_path, *count_string;
> +
> + count_path = GCSPRINTF("%s/usercnt", sshm_path);
> + rc = libxl__xs_read_checked(gc, xt, count_path, &count_string);
> + if (rc) goto out;
> + count = atoi(count_string);
> +
> + count_string = GCSPRINTF("%d", count+1);
> + rc = libxl__xs_write_checked(gc, xt, count_path, count_string);
> + if (rc) goto out;
I was a about to say the update does not look safe, but then noticed it
was called within a same transation. You might want to add a comment on
top of this function to avoid misunderstanding.
> +
> + rc = 0;
> +out:
> + return rc;
> +}
> +
> +static int libxl__sshm_add_slave(libxl__gc *gc, uint32_t domid,
> + libxl_static_shm *sshm)
> +{
> + int rc, i;
> + const char *sshm_path, *slave_path;
> + const char *dom_path, *dom_sshm_path, *dom_role_path;
> + const char *xs_value;
> + char *ents[9];
> + libxl_static_shm master_sshm;
> + uint32_t master_domid;
> + xen_pfn_t *mapped;
> + unsigned int nmapped = 0;
> + xs_transaction_t xt = XBT_NULL;
> + bool isretry;
> +
> + sshm_path = SSHM_PATH(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;
> +
> + mapped = libxl__calloc(gc, (sshm->end - sshm->begin) >> XC_PAGE_SHIFT,
> + sizeof(xen_pfn_t));
> +
> + isretry = false;
> + 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);
> +
> + if (sshm->prot == LIBXL_SSHM_PROT_UNKNOWN)
> + sshm->prot = master_sshm.prot;
> +
> + /* check if the slave is asking too much permission */
> + 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 */
> + if (!isretry) {
> + rc = libxl__sshm_do_map(gc, master_domid, domid,
> + sshm, &master_sshm,
> + mapped, &nmapped);
> + if (rc) goto out;
> + }
> +
> + /* write the result to xenstore and commit */
> + rc = libxl__xs_write_checked(gc, xt, dom_role_path, "slave");
> + if (rc) goto out;
> + rc = libxl__xs_writev(gc, xt, slave_path, ents);
> + if (rc) goto out;
> + rc = libxl__sshm_incref(gc, xt, sshm_path);
> + if (rc) goto out;
> +
> + rc = libxl__xs_transaction_commit(gc, &xt);
> + if (!rc) break;
> + if (rc < 0) goto out;
> + isretry = true;
> + }
> +
> + rc = 0;
> +out:
> + if (rc) {
> + /* roll back successfully mapped pages */
> + SSHM_ERROR(domid, sshm->id, "failed to map some pages, cancelling.");
> + for (i = 0; i < nmapped; i++) {
> + xc_domain_remove_from_physmap(CTX->xch, domid, mapped[i]);
> + }
> + }
> +
> + 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;
> + const char *sshm_path, *dom_path, *dom_role_path;
> + char *ents[13];
> + struct xs_permissions noperm;
> + xs_transaction_t xt = XBT_NULL;
> +
> + sshm_path = SSHM_PATH(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] = "usercnt";
> + ents[11] = "1";
> + 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;
> +
> + for (i = 0; i < len; ++i) {
> + if (sshms[i].role == LIBXL_SSHM_ROLE_SLAVE) {
> + 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 5f91fe4f92..8ec5044eb2 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -596,6 +596,25 @@ 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 for 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
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-01-30 17:50 ` [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
@ 2018-02-06 13:24 ` Julien Grall
2018-02-06 18:06 ` Wei Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-06 13:24 UTC (permalink / raw)
To: Zhongze Liu, xen-devel; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu
Hi,
On 01/30/2018 05:50 PM, 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: decrease the refcount of the sshm region, if the refcount
> reaches 0, cleanup the whole sshm path.
> * For a slave: 1) unmap the shared pages, and cleanup related xs entries. If
> the system works normally, all the shared pages will be unmapped, so there
> won't be page leaks. In case of errors, the unmapping process will go on and
> unmap all the other pages that can be unmapped, so the other pages won't
> be leaked, either. 2) Decrease the refcount of the sshm region, if the
> refcount reaches 0, cleanup the whole sshm path.
May I ask to add a newline line 1) and 2). This would make clearer the 2
steps.
>
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file" (see [1]).
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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 | 106 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 113 insertions(+)
>
> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> index 13b1c73d40..37f001554b 100644
> --- a/tools/libxl/libxl_domain.c
> +++ b/tools/libxl/libxl_domain.c
> @@ -1026,6 +1026,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 2cfe4c08a7..c398b6a6b8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
> _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 562f46f299..1bf4d4c2dc 100644
> --- a/tools/libxl/libxl_sshm.c
> +++ b/tools/libxl/libxl_sshm.c
> @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
> return 0;
> }
>
> +/* Decrease the refcount of an sshm. When refcount reaches 0,
NIT: Libxl coding style regarding the comment seems to be uncleared
(Ian, Wei?). But I feel keep /* and */ in separate line is nicer.
> + * clean up the whole sshm path.
> + */
> +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
> + const char *sshm_path)
> +{
> + int count;
> + const char *count_path, *count_string;
> +
> + count_path = GCSPRINTF("%s/usercnt", sshm_path);
> + if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
> + return;
> + count = atoi(count_string);
> +
> + if (--count == 0) {
> + libxl__xs_path_cleanup(gc, xt, sshm_path);
> + return;
> + }
> +
> + count_string = GCSPRINTF("%d", count);
> + libxl__xs_write_checked(gc, xt, count_path, count_string);
See my comment about incref in a patch #4.
> +
> + return;
> +}
> +
> +static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char *id,
> + uint64_t begin, uint64_t end)
> +{
> + begin >>= XC_PAGE_SHIFT;
> + end >>= XC_PAGE_SHIFT;
> + 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)
> +{
> + const char *slave_path, *begin_str, *end_str;
> + uint64_t begin, end;
> +
> + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
IHMO, by unmapping the regions in middle of the transaction, you
increase the potential failure of it. I would move that out of the
transaction path.
I would be interested to hear the opinion of the tools maintainers here.
> +
> + libxl__xs_path_cleanup(gc, xt, slave_path);
> +}
> +
> +/* 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;
> + const char *dom_path, *dom_sshm_path, *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) {
Similar comment here. You increase the chance of the transaction failing
by trying to del multiple shared memory regions at the same time. I am
mostly thinking on the libxl__sshm_decref bit.
> + 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);
> +
> + libxl__sshm_decref(gc, xt, SSHM_PATH(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;
> +}
> +
> /* libxl__sshm_do_map -- map pages into slave's physmap
> *
> * This functions maps
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 7/7] docs: documentation about static shared memory regions
2018-01-30 17:50 ` [PATCH v4 7/7] docs: documentation about static shared memory regions Zhongze Liu
@ 2018-02-06 13:28 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-02-06 13:28 UTC (permalink / raw)
To: Zhongze Liu, xen-devel; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu
Hi,
On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> Add docs to document the motivation, usage, use cases and other
> relevant information about the static shared memory feature.
>
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file". See:
>
> https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.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
> ---
> docs/man/xl-static-shm-configuration.pod.5 | 257 +++++++++++++++++++++++++++++
> docs/man/xl.cfg.pod.5.in | 8 +
> docs/misc/xenstore-paths.markdown | 47 ++++++
> 3 files changed, 312 insertions(+)
> create mode 100644 docs/man/xl-static-shm-configuration.pod.5
>
> diff --git a/docs/man/xl-static-shm-configuration.pod.5 b/docs/man/xl-static-shm-configuration.pod.5
> new file mode 100644
> index 0000000000..d68ed0ebf7
> --- /dev/null
> +++ b/docs/man/xl-static-shm-configuration.pod.5
> @@ -0,0 +1,257 @@
> +=head1 NAME
> +
> +xl-static-shm-configuration - XL Static Shared Memeory Configuration Syntax
s/Memeory/memory/
> +
> +
> +(B<NOTE>: This is currently only available to ARM guests.)
> +
> +=head1 DESCRIPTION
> +
> +The static_shm option allows users to statically setup shared memory regions
> +among a group of VMs, enabling guests without grant table support to do
> +shm-based communication.
> +
> +Every shared region is:
> +
> +=over 4
> +
> +* Uniquely identified by a string that is no longer than 128 characters, which
> +is called an B<identifier> in this document.
> +
> +* Backed by exactely one domain, which is called a B<master> domain, and all
s/exactely/exactly/
> +the other domains who are also sharing this region are called B<slave>s.
> +
> +=back
> +
> +=head1 SYNTAX
> +
> +This document specifies syntax of the static shared memory configuration in
> +the xl config file. It has the following form:
> +
> + static_shm = [ "SSHM_SPEC", "SSHM_SPEC", ... ]
> +
> +where each C<SSHM_SPEC> is in this form:
> +
> + [<key>=<value>,]*
> +
> +Valid examples of C<SSHM_SPEC> are:
> +
> + id=ID1, begin=0x100000, end=0x200000, role=master, cache_policy=x86_normal
> + id=ID1, offset = 0, begin=0x500000, end=0x600000, role=slave, prot=rw
> + id=ID2, begin=0x300000, end=0x400000, role=master
> + id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
> + id=ID2, offset = 0x10000, begin=0x690000, end=0x800000, role=slave
> +
> +These might be specified in the domain config file like this:
> +
> + static_shm = ["id=ID2, offset = 0x10000, begin=0x690000, end=0x800000,
> +role=slave"]
> +
> +
> +More formally, the string is a series of comma-separated keyword/value
> +pairs. Each parameter may be specified at most once. Default values apply if
> +the parameter is not specified.
> +
> +=head1 Parameters
> +
> +=over 4
> +
> +=item B<id>
> +
> +=over 4
> +
> +=item Description
> +
> +The unique identifier of the shared memory region.
> +
> +Every identifier could appear only once in each xl config file.
> +
> +=item Supported values
> +
> +A string that contains alphanumerics and "_"s, and is no longer than 128
> +characters.
> +
> +=item Default value
> +
> +None, this parameter is mandatory.
> +
> +=back
> +
> +=item B<begin>/B<end>
> +
> +=over 4
> +
> +=item Description
> +
> +The boundaries of the shared memory area.
> +
> +=item Supported values
> +
> +Same with B<offset>.
> +
> +=item Default Value
> +
> +None, this parameter is mandatory.
> +
> +=back
> +
> +=item B<offset>
> +
> +=over 4
> +
> +=item Description
> +
> +Can only appear when B<role> = slave. If set, the address mapping will not
> +start from the beginning the backing memory region, but from the middle
> +(B<offet> bytes away from the beginning) of it. See the graph below:
s/offet/offset/
> +
> +With B<offset> = 0, the mapping will look like:
> +
> + backing memory region: #########################################
> + | |
> + | |
> + | |
> + V V
> + slave's shared region: #########################
> +
> +With B<offset> > 0:
> +
> + backing memory region: #########################################
> + |<-- offset -->|| |
> + | |
> + | |
> + V V
> + slave's memory region: #########################
> +
> +=item Supported values
> +
> +Decimals or hexadecimals with a prefix "0x", and should be the multiple of the
> +hypervisor page granularity (currently 4K on both ARM and x86).
> +
> +=item Default value
> +
> +0x0
> +
> +=back
> +
> +=item B<role>
> +
> +=over 4
> +
> +=item Description
> +
> +The backing area would be taken from one domain, which we will mark
> +as the "master domain", and this domain should be created prior to any
> +other slave domains that depend on it.
> +
> +This arugment specifies the role of this domain.
s/arugment/argument/
> +
> +=item Supported values
> +
> +master, slave
> +
> +=item Default value
> +
> +slave
> +
> +=back
> +
> +=item B<prot>
> +
> +=over 4
> +
> +=item Description
> +
> +When B<role> = master, this means the largest set of stage-2 permission flags
> +that can be granted to the slave domains. When B<role> = slave, this means the
> +stage-2 permission flags of the shared memory area.
> +
> +=item Supported values
> +
> +Currently only 'rw' is supported.
> +
> +=item Default value
> +
> +rw
> +
> +=back
> +
> +=item B<cache_policy>
> +
> +=over 4
> +
> +=item Description
> +
> +The stage-2 cacheability/shareability attributes of the shared memory area.
> +This can only appear when B<role> = master.
> +
> +=item Supported values
> +
> +Currently, only the following policy is supported:
> +
> +=over 4
> +
> +=item B<ARM_normal>
> +
> +Only applicable to ARM guests. This would mean Inner and Outer Write-Back
> +Cacheable, and Inner Shareable.
> +
> +=back
> +
> +=item Default value
> +
> +ARM_normal
> +
> +=back
> +
> +=back
> +
> +=head1 TYPICAL USAGE
> +
> +A typical procedure of setting up a shared mem region among several VMs is:
> +
> +=over 4
> +
> +1. Add a static_shm option to the master domain's xl config file, assign an
> +B<ID> to it and mark it's B<role> as master, and set up the boundaries, prot
> +flag, and B<cache_policy> appropriately.
> +
> +2. Add a static_shm option to every slave domain's xl config file, set
> +their B<ID> to the same value as the master's, and set up the B<offset>,
> +boundaries and prot flag appropriately.
> +
> +3. Create the master domain.
> +
> +4. Create the slaves.
> +
> +=back
> +
> +Remember that the master domain must be created before any slave domains could
> +be created, for the slaves depend on the memory pages backed by their master.
> +
> +=head1 Example
> +
> +Suppose that we have 3 domains: vm1~vm3. And we want to setup two shared
> +regions, say, ID1 and ID2, among the three domains, with the following address
> +mapping:
> +
> + ID1: (vm1 : 0x100000~0x200000) <=====> (vm2 : 0x500000~0x600000)
> + ID2: (vm1 : 0x310000~0x400000) <=====> (vm3 : 0x690000~0x800000)
> +
> +According to the syntax defined above, the xl config files of the three domains
> +should contains the following content:
> +
> +In xl config file of vm1:
> + static_shm = [ "id=ID1, begin=0x100000, end=0x200000, role=master,
> +cache_policy=x86_normal, prot=rw",
> +"id=ID2, begin=0x300000, end=0x400000, role=master" ]
> +
> +In xl config file of vm2:
> + static_shm = [ "id=ID1, offset=0, begin=0x500000, end=0x600000,
> +role=slave, prot=rw" ]
> +
> +In xl config file of vm3:
> + static_shm = [ "id=ID2, offset=0x10000, begin=0x690000,
> +end=0x800000, role=slave" ]
> +
> +After that, just create vm1 first, and then create vm2 and vm3 in any order.
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index a699367779..c4d8d5e8ff 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -277,6 +277,14 @@ memory=8096 will report significantly less memory available for use
> than a system with maxmem=8096 memory=8096 due to the memory overhead
> of having to track the unused pages.
>
> +=item B<static_shm=[ "SSHM_SPEC", "SSHM_SPEC", ... ]>
> +
> +Specifies the static shared memory regions of this guest. Static shared
> +memory regions enables guests to communicate with each other through
> +one or more shared memory regions, even without grant table support.
> +Currently, this only works on ARM guests.
> +See L<xl-static-shm-configuration(5)> for more details.
> +
> =back
>
> =head3 Guest Virtual NUMA Configuration
> diff --git a/docs/misc/xenstore-paths.markdown b/docs/misc/xenstore-paths.markdown
> index 7be2592c74..33ad123839 100644
> --- a/docs/misc/xenstore-paths.markdown
> +++ b/docs/misc/xenstore-paths.markdown
> @@ -174,6 +174,14 @@ than this amount of RAM.
>
> The size of the video RAM this domain is configured with.
>
> +#### ~/static_shm/[_a-zA-Z0-9]+/role = ("master"|"slave") []
> +
> +(Note: Currently, this will only appear on ARM guests.)
> +
> +The role of this domain in the static shared memory region whose id matches
> +the `[_a-zA-Z0-9]+` part in the path. (Described in the manpage
> +**xl-static-shm-configuration(5)**).
> +
> #### ~/device/suspend/event-channel = ""|EVTCHN [w]
>
> The domain's suspend event channel. The toolstack will create this
> @@ -539,6 +547,45 @@ type. The name of each backend directory is the same as the backend type
>
> Contains the PIDs of the device models running on the domain.
>
> +#### /libxl/static_shm/[_a-zA-Z0-9]+/* []
> +
> +(Note: Currently, this will only appear on ARM guests.)
> +
> +The following paths contain backing memory parameters of a static shared memory
> +whose id matches the `[_a-zA-Z0-9]+` part in the path. Their formats and
> +meanings are the same as those in an xl config file, described in the manpage
> +**xl-static-shm-configuration(5)**.
> +
> +* begin/end: the boundary of the backing memory region.
> +* prot: the largest set of stage-2 permission flags that can be granted to
> + the slave domains.
> +* cache_policy: the stage-2 cacheability/shareability attributes of the backing
> + memory region.
> +
> +The following paths contain run-time information about the static shared memory
> +region.
> +
> +* master: the domid of the backing domain.
> +* slaves: information about the slaves that are sharing the region, see
> + ** /libxl/static_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* ** below.
> +* usercnt: An integer. This is the reference count of the backing memory region,
> + including the master domain itself. When this value reachies 0, the backing
> + memory region will be freed.
> +
> +#### /libxl/staitc_shm/[_a-zA-Z0-9]+/slaves/$DOMID/* []
> +
> +(Note: Currently, this will only appear on ARM guests.)
> +
> +The following paths contain static shared memory region parameters of a slave
> +domain. Their formats and meanings are the same as those in xl config files,
> +described in the manpage **xl-static-shm-configuration(5)**.
> +
> +* begin/end: the boundary of the shared memory region.
> +* prot: the stage-2 permission flags of the shared memory area.
> +* offset: when mapping the backing memory region to the slave's memory space,
> + the mapping will start from offset bytes after the beginning of the backing
> + memory region.
> +
> ## Virtual Machine Paths
>
> The /vm/$UUID namespace is used by toolstacks to store various
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-02-06 11:27 ` Julien Grall
@ 2018-02-06 15:41 ` Zhongze Liu
2018-02-06 15:46 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-06 15:41 UTC (permalink / raw)
To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
Hi Julien,
Thanks for reviewing.
2018-02-06 19:27 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> Hi,
>
> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>
>> Add a new structure to the IDL familiy to represent static shared memory
>> regions
[...]
>> +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'}),
>
>
> We might want to store the size rather than the end. This would allow us to
> cover region up to the address 2^64-1.
>
> Also, this would make clearer whether end is included in the region or not.
>
I think making the range inclusive and documenting it would have the
same effect.
But I'm not sure which syntax is more friendly to the users. What do you think?
Zhongze Liu
Cheers.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-02-06 15:41 ` Zhongze Liu
@ 2018-02-06 15:46 ` Julien Grall
2018-02-06 16:06 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-06 15:46 UTC (permalink / raw)
To: Zhongze Liu; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
Hi,
On 02/06/2018 03:41 PM, Zhongze Liu wrote:
> Thanks for reviewing.
>
> 2018-02-06 19:27 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> Hi,
>>
>> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>>
>>> Add a new structure to the IDL familiy to represent static shared memory
>>> regions
>
> [...]
>
>>> +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'}),
>>
>>
>> We might want to store the size rather than the end. This would allow us to
>> cover region up to the address 2^64-1.
>>
>> Also, this would make clearer whether end is included in the region or not.
>>
>
> I think making the range inclusive and documenting it would have the
> same effect.
> But I'm not sure which syntax is more friendly to the users. What do you think?
You would still run into some problem. Indeed LIBX_SSHM_RANGE_UNKNOWN is
defined as UINT64_MAX. So how would you differentiate them?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-02-06 13:07 ` Julien Grall
@ 2018-02-06 15:59 ` Zhongze Liu
2018-02-06 17:30 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-06 15:59 UTC (permalink / raw)
To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
Hi Julien,
2018-02-06 21:07 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> Hi,
>
> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>
>> Add libxl__sshm_add to map shared pages from one DomU to another, The
>> mapping
>> process involves the follwing steps:
[...]
>> +
>> +/* 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;
>
>
> What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
> ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?
The *_UNKNOWN values are used by the libxlu code to check whether a specific
option was set more than once. Without the default *_UNKNOWN value, I will not
be able to judge if, say, role is set to 'slave' by the user or not,
and therefore, if I
see the user setting role to 'master', I won't be able to tell if role
is specified twice
or not.
I think treating re-specification of options as errors will be good
for the users.
[...]
>> +
>> +/* libxl__sshm_do_map -- map pages into slave's physmap
>> + *
>> + * This functions maps
>> + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
>> @sshm->offset)
>> + * into
>> + * slave gfn: [@sshm->begin, @sshm->end)
>> + *
>> + * The gfns of the pages that are successfully mapped will be stored
>> + * in @mapped, and the number of the gfns will be stored in @nmapped.
>> + *
>> + * The caller have to guarentee that sshm->begin < sshm->end and all
>> the
>
>
> s/have to/has to/ I think.
> s/guarentee/guarantee/
>
>> + * values are page-aligned.
>
>
> Hmmm, I don't see the alignement check in libxl. So do you rely on the
> toolstack to do it?
Yes, This was done in libxlu_sshm.c.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-02-06 15:46 ` Julien Grall
@ 2018-02-06 16:06 ` Zhongze Liu
2018-02-06 17:23 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-06 16:06 UTC (permalink / raw)
To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
Hi,
2018-02-06 23:46 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> Hi,
>
> On 02/06/2018 03:41 PM, Zhongze Liu wrote:
>>
>> Thanks for reviewing.
>>
>> 2018-02-06 19:27 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>>>
>>> Hi,
>>>
>>> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>>>
>>>>
>>>> Add a new structure to the IDL familiy to represent static shared memory
>>>> regions
>>
>>
>> [...]
>>
>>>> +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'}),
>>>
>>>
>>>
>>> We might want to store the size rather than the end. This would allow us
>>> to
>>> cover region up to the address 2^64-1.
>>>
>>> Also, this would make clearer whether end is included in the region or
>>> not.
>>>
>>
>> I think making the range inclusive and documenting it would have the
>> same effect.
>> But I'm not sure which syntax is more friendly to the users. What do you
>> think?
>
>
> You would still run into some problem. Indeed LIBX_SSHM_RANGE_UNKNOWN is
> defined as UINT64_MAX. So how would you differentiate them?
But saying inclusive, I was actually trying to say "including the page
that begins at @end", so
the only possibility when LIBXL_SSHM_RANGE_UNKNOWN would be a valid value for
@end is when the page granularity is 1byte, which, I think, is not
very likely to happen.
But soon I find this might lead to more confusion. Now I agree with
you that we should use
the begin/size syntax instead of the current one.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions
2018-02-06 16:06 ` Zhongze Liu
@ 2018-02-06 17:23 ` Julien Grall
0 siblings, 0 replies; 45+ messages in thread
From: Julien Grall @ 2018-02-06 17:23 UTC (permalink / raw)
To: Zhongze Liu; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On 02/06/2018 04:06 PM, Zhongze Liu wrote:
> Hi,
Hi,
> 2018-02-06 23:46 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> Hi,
>>
>> On 02/06/2018 03:41 PM, Zhongze Liu wrote:
>>>
>>> Thanks for reviewing.
>>>
>>> 2018-02-06 19:27 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>>>>
>>>> Hi,
>>>>
>>>> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>>>>
>>>>>
>>>>> Add a new structure to the IDL familiy to represent static shared memory
>>>>> regions
>>>
>>>
>>> [...]
>>>
>>>>> +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'}),
>>>>
>>>>
>>>>
>>>> We might want to store the size rather than the end. This would allow us
>>>> to
>>>> cover region up to the address 2^64-1.
>>>>
>>>> Also, this would make clearer whether end is included in the region or
>>>> not.
>>>>
>>>
>>> I think making the range inclusive and documenting it would have the
>>> same effect.
>>> But I'm not sure which syntax is more friendly to the users. What do you
>>> think?
>>
>>
>> You would still run into some problem. Indeed LIBX_SSHM_RANGE_UNKNOWN is
>> defined as UINT64_MAX. So how would you differentiate them?
>
> But saying inclusive, I was actually trying to say "including the page
> that begins at @end", so
Which is quite confusing. Usually when I see a range, I assume that the
end will be the actual end. Not PAGE_ALIGN(end).
> the only possibility when LIBXL_SSHM_RANGE_UNKNOWN would be a valid value for
> @end is when the page granularity is 1byte, which, I think, is not
> very likely to happen.
>
> But soon I find this might lead to more confusion. Now I agree with
> you that we should use
> the begin/size syntax instead of the current one.
Yes, begin/size is straightforward. There are no way to lead to map one
less (or extra) page by confusion.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-02-06 15:59 ` Zhongze Liu
@ 2018-02-06 17:30 ` Julien Grall
2018-02-06 17:47 ` Wei Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-06 17:30 UTC (permalink / raw)
To: Zhongze Liu; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On 02/06/2018 03:59 PM, Zhongze Liu wrote:
> Hi Julien,
Hi,
> 2018-02-06 21:07 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> Hi,
>>
>> On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>>>
>>> Add libxl__sshm_add to map shared pages from one DomU to another, The
>>> mapping
>>> process involves the follwing steps:
>
> [...]
>
>>> +
>>> +/* 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;
>>
>>
>> What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
>> ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?
>
> The *_UNKNOWN values are used by the libxlu code to check whether a specific
> option was set more than once.
AFAIK, a toolstack is free to not use libxlu. Someone may implement
their own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.
> Without the default *_UNKNOWN value, I will not
> be able to judge if, say, role is set to 'slave' by the user or not,
> and therefore, if I
> see the user setting role to 'master', I won't be able to tell if role
> is specified twice
> or not.
>
> I think treating re-specification of options as errors will be good
> for the users.
In that case, you should treat that as an error for everyone and not
only xl. This would avoid confusion on other toolstack.
>
> [...]
>
>>> +
>>> +/* libxl__sshm_do_map -- map pages into slave's physmap
>>> + *
>>> + * This functions maps
>>> + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
>>> @sshm->offset)
>>> + * into
>>> + * slave gfn: [@sshm->begin, @sshm->end)
>>> + *
>>> + * The gfns of the pages that are successfully mapped will be stored
>>> + * in @mapped, and the number of the gfns will be stored in @nmapped.
>>> + *
>>> + * The caller have to guarentee that sshm->begin < sshm->end and all
>>> the
>>
>>
>> s/have to/has to/ I think.
>> s/guarentee/guarantee/
>>
>>> + * values are page-aligned.
>>
>>
>> Hmmm, I don't see the alignement check in libxl. So do you rely on the
>> toolstack to do it?
>
> Yes, This was done in libxlu_sshm.c.
Same remark as above regarding libxlu. Note that I am maintaining the
tools. Ian and Wei may have a different opinion here.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-02-06 17:30 ` Julien Grall
@ 2018-02-06 17:47 ` Wei Liu
2018-02-12 15:08 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Wei Liu @ 2018-02-06 17:47 UTC (permalink / raw)
To: Julien Grall
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, Zhongze Liu, xen-devel
On Tue, Feb 06, 2018 at 05:30:50PM +0000, Julien Grall wrote:
>
>
> On 02/06/2018 03:59 PM, Zhongze Liu wrote:
> > Hi Julien,
>
> Hi,
>
>
> > 2018-02-06 21:07 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> > > Hi,
> > >
> > > On 01/30/2018 05:50 PM, Zhongze Liu wrote:
> > > >
> > > > Add libxl__sshm_add to map shared pages from one DomU to another, The
> > > > mapping
> > > > process involves the follwing steps:
> >
> > [...]
> >
> > > > +
> > > > +/* 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;
> > >
> > >
> > > What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
> > > ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?
> >
> > The *_UNKNOWN values are used by the libxlu code to check whether a specific
> > option was set more than once.
>
> AFAIK, a toolstack is free to not use libxlu. Someone may implement their
> own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.
Yes.
>
> > Without the default *_UNKNOWN value, I will not
> > be able to judge if, say, role is set to 'slave' by the user or not,
> > and therefore, if I
> > see the user setting role to 'master', I won't be able to tell if role
> > is specified twice
> > or not.
> >
> > I think treating re-specification of options as errors will be good
> > for the users.
>
> In that case, you should treat that as an error for everyone and not only
> xl. This would avoid confusion on other toolstack.
>
> >
> > [...]
> >
> > > > +
> > > > +/* libxl__sshm_do_map -- map pages into slave's physmap
> > > > + *
> > > > + * This functions maps
> > > > + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
> > > > @sshm->offset)
> > > > + * into
> > > > + * slave gfn: [@sshm->begin, @sshm->end)
> > > > + *
> > > > + * The gfns of the pages that are successfully mapped will be stored
> > > > + * in @mapped, and the number of the gfns will be stored in @nmapped.
> > > > + *
> > > > + * The caller have to guarentee that sshm->begin < sshm->end and all
> > > > the
> > >
> > >
> > > s/have to/has to/ I think.
> > > s/guarentee/guarantee/
> > >
> > > > + * values are page-aligned.
> > >
> > >
> > > Hmmm, I don't see the alignement check in libxl. So do you rely on the
> > > toolstack to do it?
> >
> > Yes, This was done in libxlu_sshm.c.
>
> Same remark as above regarding libxlu. Note that I am maintaining the tools.
> Ian and Wei may have a different opinion here.
>
Please move the check to libxl.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-06 13:24 ` Julien Grall
@ 2018-02-06 18:06 ` Wei Liu
2018-02-07 16:27 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Wei Liu @ 2018-02-06 18:06 UTC (permalink / raw)
To: Julien Grall
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, Zhongze Liu, xen-devel
On Tue, Feb 06, 2018 at 01:24:30PM +0000, Julien Grall wrote:
> > 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 2cfe4c08a7..c398b6a6b8 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
> > _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 562f46f299..1bf4d4c2dc 100644
> > --- a/tools/libxl/libxl_sshm.c
> > +++ b/tools/libxl/libxl_sshm.c
> > @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
> > return 0;
> > }
> > +/* Decrease the refcount of an sshm. When refcount reaches 0,
>
> NIT: Libxl coding style regarding the comment seems to be uncleared (Ian,
> Wei?). But I feel keep /* and */ in separate line is nicer.
I don't have an opinion here.
>
> > + * clean up the whole sshm path.
> > + */
> > +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
> > + const char *sshm_path)
> > +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
> > + uint32_t domid, const char *id, bool isretry)
> > +{
> > + const char *slave_path, *begin_str, *end_str;
> > + uint64_t begin, end;
> > +
> > + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
>
> IHMO, by unmapping the regions in middle of the transaction, you increase
> the potential failure of it. I would move that out of the transaction path.
>
> I would be interested to hear the opinion of the tools maintainers here.
>
If you move the unmap after the loop you create a window in which
the pages are still mapped but the toolstack thinks they are unmapped.
While the code as-is now makes sure (assuming no error in unmap) the
pages are unmapped no later than the transaction is committed. I think
this can be done by moving unmap before the transaction.
Zhongze, do you think the unmap must be done inside the loop? What kind
of invariants do you have in mind?
Then there is the question of "what do we do if unmap fails". Honestly I
don't have an answer. It seems rather screwed up in that case and I
doubt there is much libxl can do to rectify things.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-06 18:06 ` Wei Liu
@ 2018-02-07 16:27 ` Zhongze Liu
2018-02-07 16:54 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-07 16:27 UTC (permalink / raw)
To: Wei Liu, Julien Grall; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
Hi Wei and Julien,
2018-02-07 2:06 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Tue, Feb 06, 2018 at 01:24:30PM +0000, Julien Grall wrote:
>> > 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 2cfe4c08a7..c398b6a6b8 100644
>> > --- a/tools/libxl/libxl_internal.h
>> > +++ b/tools/libxl/libxl_internal.h
>> > @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
>> > _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 562f46f299..1bf4d4c2dc 100644
>> > --- a/tools/libxl/libxl_sshm.c
>> > +++ b/tools/libxl/libxl_sshm.c
>> > @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
>> > return 0;
>> > }
>> > +/* Decrease the refcount of an sshm. When refcount reaches 0,
>>
>> NIT: Libxl coding style regarding the comment seems to be uncleared (Ian,
>> Wei?). But I feel keep /* and */ in separate line is nicer.
>
> I don't have an opinion here.
>
>>
>> > + * clean up the whole sshm path.
>> > + */
>> > +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
>> > + const char *sshm_path)
>> > +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
>> > + uint32_t domid, const char *id, bool isretry)
>> > +{
>> > + const char *slave_path, *begin_str, *end_str;
>> > + uint64_t begin, end;
>> > +
>> > + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
>>
>> IHMO, by unmapping the regions in middle of the transaction, you increase
>> the potential failure of it. I would move that out of the transaction path.
>>
>> I would be interested to hear the opinion of the tools maintainers here.
>>
>
> If you move the unmap after the loop you create a window in which
> the pages are still mapped but the toolstack thinks they are unmapped.
>
> While the code as-is now makes sure (assuming no error in unmap) the
> pages are unmapped no later than the transaction is committed. I think
> this can be done by moving unmap before the transaction.
>
> Zhongze, do you think the unmap must be done inside the loop? What kind
> of invariants do you have in mind?
>
> Then there is the question of "what do we do if unmap fails". Honestly I
> don't have an answer. It seems rather screwed up in that case and I
> doubt there is much libxl can do to rectify things.
>
I put the unmap inside the transaction because I want to make the whole
read_mapping_info->unmap->update_mapping_info process atomic. If
I put unmap outside the transaction: after I read out the information
that I need to do the unmap, and before I do the unmap and decrease the
refcnt, there could be another instance of this code trying to do the same
thing, which might lead to race condition.
And yes, I don't think we can do much in case of something go wrong, so
what I'm doing here is just to do as best as I can -- unmapping all that pages
that can be unmapped and cleanup the xs entries.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-07 16:27 ` Zhongze Liu
@ 2018-02-07 16:54 ` Julien Grall
2018-02-12 14:52 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-07 16:54 UTC (permalink / raw)
To: Zhongze Liu, Wei Liu; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
On 07/02/18 16:27, Zhongze Liu wrote:
> Hi Wei and Julien,
Hi,
> 2018-02-07 2:06 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
>> On Tue, Feb 06, 2018 at 01:24:30PM +0000, Julien Grall wrote:
>>>> 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 2cfe4c08a7..c398b6a6b8 100644
>>>> --- a/tools/libxl/libxl_internal.h
>>>> +++ b/tools/libxl/libxl_internal.h
>>>> @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
>>>> _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 562f46f299..1bf4d4c2dc 100644
>>>> --- a/tools/libxl/libxl_sshm.c
>>>> +++ b/tools/libxl/libxl_sshm.c
>>>> @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
>>>> return 0;
>>>> }
>>>> +/* Decrease the refcount of an sshm. When refcount reaches 0,
>>>
>>> NIT: Libxl coding style regarding the comment seems to be uncleared (Ian,
>>> Wei?). But I feel keep /* and */ in separate line is nicer.
>>
>> I don't have an opinion here.
>>
>>>
>>>> + * clean up the whole sshm path.
>>>> + */
>>>> +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
>>>> + const char *sshm_path)
>>>> +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
>>>> + uint32_t domid, const char *id, bool isretry)
>>>> +{
>>>> + const char *slave_path, *begin_str, *end_str;
>>>> + uint64_t begin, end;
>>>> +
>>>> + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
>>>
>>> IHMO, by unmapping the regions in middle of the transaction, you increase
>>> the potential failure of it. I would move that out of the transaction path.
>>>
>>> I would be interested to hear the opinion of the tools maintainers here.
>>>
>>
>> If you move the unmap after the loop you create a window in which
>> the pages are still mapped but the toolstack thinks they are unmapped.
>>
>> While the code as-is now makes sure (assuming no error in unmap) the
>> pages are unmapped no later than the transaction is committed. I think
>> this can be done by moving unmap before the transaction.
>>
>> Zhongze, do you think the unmap must be done inside the loop? What kind
>> of invariants do you have in mind?
>>
>> Then there is the question of "what do we do if unmap fails". Honestly I
>> don't have an answer. It seems rather screwed up in that case and I
>> doubt there is much libxl can do to rectify things.
>>
>
> I put the unmap inside the transaction because I want to make the whole
> read_mapping_info->unmap->update_mapping_info process atomic. If
> I put unmap outside the transaction: after I read out the information
> that I need to do the unmap, and before I do the unmap and decrease the
> refcnt, there could be another instance of this code trying to do the same
> thing, which might lead to race condition.
AFAIU, the transaction is not a "global" lock. You will just not see the
the change from the others during the transactions. Your changes will be
only be visible at the end. So two transaction can be happily started
concurrently, and try to do the unmap together. Not even your code will
protect against that.
So can you give a bit more details here?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-07 16:54 ` Julien Grall
@ 2018-02-12 14:52 ` Zhongze Liu
2018-02-12 15:09 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-12 14:52 UTC (permalink / raw)
To: Julien Grall, Wei Liu; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
Hi Julien an Wei,
2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> On 07/02/18 16:27, Zhongze Liu wrote:
>>
>> Hi Wei and Julien,
>
>
> Hi,
>
>
>> 2018-02-07 2:06 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
>>>
>>> On Tue, Feb 06, 2018 at 01:24:30PM +0000, Julien Grall wrote:
>>>>>
>>>>> 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 2cfe4c08a7..c398b6a6b8 100644
>>>>> --- a/tools/libxl/libxl_internal.h
>>>>> +++ b/tools/libxl/libxl_internal.h
>>>>> @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char
>>>>> **s)
>>>>> _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 562f46f299..1bf4d4c2dc 100644
>>>>> --- a/tools/libxl/libxl_sshm.c
>>>>> +++ b/tools/libxl/libxl_sshm.c
>>>>> @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc,
>>>>> uint32_t domid,
>>>>> return 0;
>>>>> }
>>>>> +/* Decrease the refcount of an sshm. When refcount reaches 0,
>>>>
>>>>
>>>> NIT: Libxl coding style regarding the comment seems to be uncleared
>>>> (Ian,
>>>> Wei?). But I feel keep /* and */ in separate line is nicer.
>>>
>>>
>>> I don't have an opinion here.
>>>
>>>>
>>>>> + * clean up the whole sshm path.
>>>>> + */
>>>>> +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
>>>>> + const char *sshm_path)
>>>>> +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
>>>>> + uint32_t domid, const char *id, bool
>>>>> isretry)
>>>>> +{
>>>>> + const char *slave_path, *begin_str, *end_str;
>>>>> + uint64_t begin, end;
>>>>> +
>>>>> + slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), 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);
>>>>
>>>>
>>>> IHMO, by unmapping the regions in middle of the transaction, you
>>>> increase
>>>> the potential failure of it. I would move that out of the transaction
>>>> path.
>>>>
>>>> I would be interested to hear the opinion of the tools maintainers here.
>>>>
>>>
>>> If you move the unmap after the loop you create a window in which
>>> the pages are still mapped but the toolstack thinks they are unmapped.
>>>
>>> While the code as-is now makes sure (assuming no error in unmap) the
>>> pages are unmapped no later than the transaction is committed. I think
>>> this can be done by moving unmap before the transaction.
>>>
>>> Zhongze, do you think the unmap must be done inside the loop? What kind
>>> of invariants do you have in mind?
>>>
>>> Then there is the question of "what do we do if unmap fails". Honestly I
>>> don't have an answer. It seems rather screwed up in that case and I
>>> doubt there is much libxl can do to rectify things.
>>>
>>
>> I put the unmap inside the transaction because I want to make the whole
>> read_mapping_info->unmap->update_mapping_info process atomic. If
>> I put unmap outside the transaction: after I read out the information
>> that I need to do the unmap, and before I do the unmap and decrease the
>> refcnt, there could be another instance of this code trying to do the same
>> thing, which might lead to race condition.
>
>
> AFAIU, the transaction is not a "global" lock. You will just not see the the
> change from the others during the transactions. Your changes will be only be
> visible at the end. So two transaction can be happily started concurrently,
> and try to do the unmap together. Not even your code will protect against
> that.
>
> So can you give a bit more details here?
>
It seems that I mistakenly use transaction as a global lock. Now I don't have
any reasons not putting the unmap out of the transaction, but this will break
the original transaction into two, and I do think that we need some explicit
locking here.
@Wei. Do you have any suggestions here?
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-02-06 17:47 ` Wei Liu
@ 2018-02-12 15:08 ` Zhongze Liu
2018-02-14 14:26 ` Wei Liu
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-12 15:08 UTC (permalink / raw)
To: Wei Liu, Julien Grall; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
Hi Wei and Julien,
2018-02-07 1:47 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Tue, Feb 06, 2018 at 05:30:50PM +0000, Julien Grall wrote:
>>
>>
>> On 02/06/2018 03:59 PM, Zhongze Liu wrote:
>> > Hi Julien,
>>
>> Hi,
>>
>>
>> > 2018-02-06 21:07 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> > > Hi,
>> > >
>> > > On 01/30/2018 05:50 PM, Zhongze Liu wrote:
>> > > >
>> > > > Add libxl__sshm_add to map shared pages from one DomU to another, The
>> > > > mapping
>> > > > process involves the follwing steps:
>> >
>> > [...]
>> >
>> > > > +
>> > > > +/* 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;
>> > >
>> > >
>> > > What is the purpose of {ROLE,PROT}_UNKNOWN if you default it resp. to
>> > > ROLE_SLAVE and PROT_RW. Would not it be easier to just drop them?
>> >
>> > The *_UNKNOWN values are used by the libxlu code to check whether a specific
>> > option was set more than once.
>>
>> AFAIK, a toolstack is free to not use libxlu. Someone may implement their
>> own toolstack on top of libxl and may use ROLE_UNKNOWN by mistake.
>
> Yes.
>
>>
>> > Without the default *_UNKNOWN value, I will not
>> > be able to judge if, say, role is set to 'slave' by the user or not,
>> > and therefore, if I
>> > see the user setting role to 'master', I won't be able to tell if role
>> > is specified twice
>> > or not.
>> >
>> > I think treating re-specification of options as errors will be good
>> > for the users.
>>
>> In that case, you should treat that as an error for everyone and not only
>> xl. This would avoid confusion on other toolstack.
>>
>> >
>> > [...]
>> >
>> > > > +
>> > > > +/* libxl__sshm_do_map -- map pages into slave's physmap
>> > > > + *
>> > > > + * This functions maps
>> > > > + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
>> > > > @sshm->offset)
>> > > > + * into
>> > > > + * slave gfn: [@sshm->begin, @sshm->end)
>> > > > + *
>> > > > + * The gfns of the pages that are successfully mapped will be stored
>> > > > + * in @mapped, and the number of the gfns will be stored in @nmapped.
>> > > > + *
>> > > > + * The caller have to guarentee that sshm->begin < sshm->end and all
>> > > > the
>> > >
>> > >
>> > > s/have to/has to/ I think.
>> > > s/guarentee/guarantee/
>> > >
>> > > > + * values are page-aligned.
>> > >
>> > >
>> > > Hmmm, I don't see the alignement check in libxl. So do you rely on the
>> > > toolstack to do it?
>> >
>> > Yes, This was done in libxlu_sshm.c.
>>
>> Same remark as above regarding libxlu. Note that I am maintaining the tools.
>> Ian and Wei may have a different opinion here.
>>
>
> Please move the check to libxl.
I agree that we should move the alignment check to libxl.
But I still think that re-specification checks could only be done in
libxlu, because
this could only be spotted in the parsing phase -- it shouldn't be
libxl's job. For
libxl, an *_UNKNOWN values just indicates that the user of libxl
didn't explicitly
assign a value to the option upon calling the constructor of the sshm struct,
and the code in libxl will set the option to its default value.
However, I do think
I failed to handle the possibilities where an option value is not
not *_UNKNOWN and
not any valid values, which might occur if the user of libxl didn't
use the constructor
to initialize the sshm struct.
Cheers,
Zhongze Liu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-12 14:52 ` Zhongze Liu
@ 2018-02-12 15:09 ` Julien Grall
2018-02-12 15:17 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Julien Grall @ 2018-02-12 15:09 UTC (permalink / raw)
To: Zhongze Liu, Wei Liu; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
Hi,
On 12/02/18 14:52, Zhongze Liu wrote:
> 2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> On 07/02/18 16:27, Zhongze Liu wrote:
> It seems that I mistakenly use transaction as a global lock. Now I don't have
> any reasons not putting the unmap out of the transaction, but this will break
> the original transaction into two, and I do think that we need some explicit
> locking here.
Can you explain why you need specific locking here? What are you trying
to protect? Are you trying to protect against two process doing the unmap?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-12 15:09 ` Julien Grall
@ 2018-02-12 15:17 ` Zhongze Liu
2018-02-12 15:24 ` Julien Grall
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-12 15:17 UTC (permalink / raw)
To: Julien Grall; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
Hi Julien,
2018-02-12 23:09 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> Hi,
>
> On 12/02/18 14:52, Zhongze Liu wrote:
>>
>> 2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>>>
>>> On 07/02/18 16:27, Zhongze Liu wrote:
>>
>> It seems that I mistakenly use transaction as a global lock. Now I don't
>> have
>> any reasons not putting the unmap out of the transaction, but this will
>> break
>> the original transaction into two, and I do think that we need some
>> explicit
>> locking here.
>
>
> Can you explain why you need specific locking here? What are you trying to
> protect? Are you trying to protect against two process doing the unmap?
>
Yes.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-12 15:17 ` Zhongze Liu
@ 2018-02-12 15:24 ` Julien Grall
2018-02-14 14:35 ` Wei Liu
2018-02-14 14:39 ` Wei Liu
0 siblings, 2 replies; 45+ messages in thread
From: Julien Grall @ 2018-02-12 15:24 UTC (permalink / raw)
To: Zhongze Liu; +Cc: Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On 12/02/18 15:17, Zhongze Liu wrote:
> Hi Julien,
Hi,
>
> 2018-02-12 23:09 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>> Hi,
>>
>> On 12/02/18 14:52, Zhongze Liu wrote:
>>>
>>> 2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
>>>>
>>>> On 07/02/18 16:27, Zhongze Liu wrote:
>>>
>>> It seems that I mistakenly use transaction as a global lock. Now I don't
>>> have
>>> any reasons not putting the unmap out of the transaction, but this will
>>> break
>>> the original transaction into two, and I do think that we need some
>>> explicit
>>> locking here.
>>
>>
>> Can you explain why you need specific locking here? What are you trying to
>> protect? Are you trying to protect against two process doing the unmap?
>>
>
> Yes.
I don't think you have to worry about the locking here. With the current
interface, the regions cannot be modified after the guest has booted. So
the addresses will always stay valid.
This code path should never be called concurrently, as a lot of code in
libxl, so I think someone else will take care about that for you (I will
let Wei confirm here).
In any case, the worst that could happen is the unmap is called twice on
the same region. So you would get spurious error message. Not that bad.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-01 10:23 ` Jan Beulich
2018-02-01 18:11 ` Zhongze Liu
@ 2018-02-13 15:15 ` Zhongze Liu
2018-02-13 15:26 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-13 15:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
I've updated the comments according to your previous suggestions,
do they look good to you?
2018-02-01 18:23 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 30.01.18 at 18:50, <blackskygg@gmail.com> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4126,6 +4126,10 @@ int xenmem_add_to_physmap_one(
>> }
>> case XENMAPSPACE_gmfn_foreign:
>> return p2m_add_foreign(d, idx, gfn_x(gpfn), extra.foreign_domid);
>> + case XENMAPSPACE_gmfn_share:
>> + gdprintk(XENLOG_WARNING,
>> + "XENMAPSPACE_gmfn_share is currently not supported on x86\n");
>> + break;
>
> Please don't - a hypervisor log message isn't really useful here. It
> should be the tool stack to disallow respective options on x86
> until that's implemented.
>
>> --- a/xen/include/public/memory.h
>> +++ b/xen/include/public/memory.h
>> @@ -227,6 +227,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>> Stage-2 using the Normal Memory
>> Inner/Outer Write-Back Cacheable
>> memory attribute. */
/*
* GMFN from another dom, but with different privilege requirements.
*
* Suppose that (c), the current domain, is trying to map pages from (t) into
* (d). gmfn_share dosen't require that (d) has privilege over (t). This
* enables some usecases such as dom0 trying to share memory pages between
* two unprivileged guests, which will otherwise be impossible using
* gmfn_foreign. This is XENMEM_add_to_physmap_batch only, and currently ARM
* only.
*
* The exact XSM privilege requirements are as follows:
*
* ================== ============== ===================== =====================
* (c) over (d) (c) over (t) (d) over (t)
* ================== ============== ===================== =====================
* _foreign (dummy) XSM_TARGET NO XSM_TARGET
* _share (dummy) XSM_TARGET XSM_TARGET NO
* _foreign (flask) MMU__PHYSMAP NO MMU__MAP_READ/WRITE
* _share (flask) MMU__PHYSMAP MMU__MAP_READ/WRITE MMU__SHARE
* ================== ============== ===================== =====================
*/
>> +#define XENMAPSPACE_gmfn_share 6 /* Same as *_gmfn_foreign, but this is
>> + for a privileged dom to share pages
>> + between two doms. */
>> +
>
> The comment doesn't make clear why XENMAPSPACE_gmfn_foreign
> then can't be used. In particular it is left open how _both_ domains
> would be specified.
>
> Also XENMAPSPACE_gmfn_foreign is restricted to
> XENMEM_add_to_physmap_batch (a comment says so) - how about
> this new one? According to the actual code changes you do, there's
> no meaningful difference, in which case the restriction should be
> named here as well.
>
>> --- a/xen/include/xsm/dummy.h
>> +++ b/xen/include/xsm/dummy.h
>> @@ -521,6 +521,12 @@ static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, str
>> return xsm_default_action(action, d, t);
>> }
>>
>> +static XSM_INLINE int xsm_map_gmfn_share(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
>
> Line length.
>
>> +{
/*
* This action also requires that @current targets @d, but it has already been
* checked somewhere higher in the call stack.
*
* Be aware that this is not an exact default equivalence of its flask variant
* which also checks if @d and @t "are allowed to share memory pages", for we
* don't have a proper default equivalence of such a check.
*/
>> + XSM_ASSERT_ACTION(XSM_TARGET);
>> + return xsm_default_action(action, current->domain, t);
>
> How does this represent a proper default equivalent of ...
>
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1196,6 +1196,12 @@ static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
> return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
> }
>
> +static int flask_map_gmfn_share(struct domain *d, struct domain *t)
> +{
/*
* This action also requires that @current has MMU__MAP_READ/WRITE over @d,
* but that has already been checked somewhere higher in the call stack (for
* example, by flask_add_to_physmap()).
*/
> + return current_has_perm(t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE) ?:
> + domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
>
> ... this?
Cheers,
Zhongze Liu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-13 15:15 ` Zhongze Liu
@ 2018-02-13 15:26 ` Jan Beulich
2018-02-14 7:15 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2018-02-13 15:26 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
> I've updated the comments according to your previous suggestions,
> do they look good to you?
The one in the public header is way too verbose. I specifically don't
see why you would need to spell out XSM privilege requirements
there. Please make new comments match existing ones in style and
verbosity if at all possible, while still conveying all necessary /
relevant information.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-13 15:26 ` Jan Beulich
@ 2018-02-14 7:15 ` Zhongze Liu
2018-02-14 8:37 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-14 7:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>> I've updated the comments according to your previous suggestions,
>> do they look good to you?
>
> The one in the public header is way too verbose. I specifically don't
> see why you would need to spell out XSM privilege requirements
> there. Please make new comments match existing ones in style and
> verbosity if at all possible, while still conveying all necessary /
> relevant information.
>
I shortened it a little bit, and now it looks like:
#define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
gmfn_foreign,
if (c) tries to map pages from (t) into
(d), this doesn't require that (d) itself
has the privilege to map the pages, but
instead requires that (c) has the
privilege to do so, as long as (d) and (t)
are allowed to share memory pages.
This is XENMEM_add_to_physmap_batch only,
and currently ARM only. */
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-14 7:15 ` Zhongze Liu
@ 2018-02-14 8:37 ` Jan Beulich
2018-02-14 17:02 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2018-02-14 8:37 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
> Hi Jan,
>
> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>> I've updated the comments according to your previous suggestions,
>>> do they look good to you?
>>
>> The one in the public header is way too verbose. I specifically don't
>> see why you would need to spell out XSM privilege requirements
>> there. Please make new comments match existing ones in style and
>> verbosity if at all possible, while still conveying all necessary /
>> relevant information.
>>
>
> I shortened it a little bit, and now it looks like:
>
> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
> gmfn_foreign,
> if (c) tries to map pages from (t) into
> (d), this doesn't require that (d) itself
> has the privilege to map the pages, but
> instead requires that (c) has the
> privilege to do so, as long as (d) and (t)
> are allowed to share memory pages.
> This is XENMEM_add_to_physmap_batch only,
> and currently ARM only. */
Which leaves unclear what (c), (d), and (t) are. How about
"GMFN from another dom, XENMEM_add_to_physmap_batch (and
currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
<explain here what the difference is with a few simple words>."
(You can and should go into further detail in the commit message.)
Without this _properly_ explained, I'll continue to ask why you
can't simply make XENMAPSPACE_gmfn_foreign do what you want
(as it already takes two domid_t-s as input), by suitably adjusting
its XSM check(s).
You'd also need to adjust the comment on the foreign_domid
structure field, as it saying "gmfn_foreign" would otherwise become
stale with your change.
I don't, btw, like the ARM only part here - there's nothing
inherently wrong with the same operation being sensible on x86.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation
2018-02-12 15:08 ` Zhongze Liu
@ 2018-02-14 14:26 ` Wei Liu
0 siblings, 0 replies; 45+ messages in thread
From: Wei Liu @ 2018-02-14 14:26 UTC (permalink / raw)
To: Zhongze Liu
Cc: Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On Mon, Feb 12, 2018 at 11:08:01PM +0800, Zhongze Liu wrote:
> >> > [...]
> >> >
> >> > > > +
> >> > > > +/* libxl__sshm_do_map -- map pages into slave's physmap
> >> > > > + *
> >> > > > + * This functions maps
> >> > > > + * master gfn: [@msshm->begin + @sshm->offset, @msshm->end +
> >> > > > @sshm->offset)
> >> > > > + * into
> >> > > > + * slave gfn: [@sshm->begin, @sshm->end)
> >> > > > + *
> >> > > > + * The gfns of the pages that are successfully mapped will be stored
> >> > > > + * in @mapped, and the number of the gfns will be stored in @nmapped.
> >> > > > + *
> >> > > > + * The caller have to guarentee that sshm->begin < sshm->end and all
> >> > > > the
> >> > >
> >> > >
> >> > > s/have to/has to/ I think.
> >> > > s/guarentee/guarantee/
> >> > >
> >> > > > + * values are page-aligned.
> >> > >
> >> > >
> >> > > Hmmm, I don't see the alignement check in libxl. So do you rely on the
> >> > > toolstack to do it?
> >> >
> >> > Yes, This was done in libxlu_sshm.c.
> >>
> >> Same remark as above regarding libxlu. Note that I am maintaining the tools.
> >> Ian and Wei may have a different opinion here.
> >>
> >
> > Please move the check to libxl.
>
> I agree that we should move the alignment check to libxl.
>
> But I still think that re-specification checks could only be done in
> libxlu, because
> this could only be spotted in the parsing phase -- it shouldn't be
> libxl's job. For
> libxl, an *_UNKNOWN values just indicates that the user of libxl
> didn't explicitly
> assign a value to the option upon calling the constructor of the sshm struct,
> and the code in libxl will set the option to its default value.
> However, I do think
> I failed to handle the possibilities where an option value is not
> not *_UNKNOWN and
> not any valid values, which might occur if the user of libxl didn't
> use the constructor
> to initialize the sshm struct.
So there are two problems under discussion. One is parsing, which should
be done in the parser library (libxlu); the other is checking the
validity of the value in the struct, which should be done in libxl. I
don't think we fundamentally disagree with each other. Sorry for my
terse reply earlier.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-12 15:24 ` Julien Grall
@ 2018-02-14 14:35 ` Wei Liu
2018-02-14 14:39 ` Wei Liu
1 sibling, 0 replies; 45+ messages in thread
From: Wei Liu @ 2018-02-14 14:35 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, Zhongze Liu, xen-devel
On Mon, Feb 12, 2018 at 03:24:26PM +0000, Julien Grall wrote:
>
>
> On 12/02/18 15:17, Zhongze Liu wrote:
> > Hi Julien,
>
> Hi,
>
> >
> > 2018-02-12 23:09 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> > > Hi,
> > >
> > > On 12/02/18 14:52, Zhongze Liu wrote:
> > > >
> > > > 2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> > > > >
> > > > > On 07/02/18 16:27, Zhongze Liu wrote:
> > > >
> > > > It seems that I mistakenly use transaction as a global lock. Now I don't
> > > > have
> > > > any reasons not putting the unmap out of the transaction, but this will
> > > > break
> > > > the original transaction into two, and I do think that we need some
> > > > explicit
> > > > locking here.
> > >
> > >
> > > Can you explain why you need specific locking here? What are you trying to
> > > protect? Are you trying to protect against two process doing the unmap?
> > >
> >
> > Yes.
>
> I don't think you have to worry about the locking here. With the current
> interface, the regions cannot be modified after the guest has booted. So the
> addresses will always stay valid.
>
> This code path should never be called concurrently, as a lot of code in
> libxl, so I think someone else will take care about that for you (I will let
> Wei confirm here).
I think you mean "can be called concurrently but locking is taken care
of".
If two processes / threads with different libxl ctx (not just xl. xl has
a global lock) try to manipulate the same domain, libxl APIs use file
locks. For the same same process, multiple threads but sharing one
libxl_ctx there is pthread mutex in libxl_ctx.
In a way, "someone else" (in libxl code) has already taken care of the
locking.
Fundamentally We need to assume the code can be called concurrently. It
would be good thing if this is taken into consideration if new code is
added.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-12 15:24 ` Julien Grall
2018-02-14 14:35 ` Wei Liu
@ 2018-02-14 14:39 ` Wei Liu
2018-02-26 12:08 ` Ian Jackson
1 sibling, 1 reply; 45+ messages in thread
From: Wei Liu @ 2018-02-14 14:39 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Stefano Stabellini, Ian Jackson, Zhongze Liu, xen-devel
On Mon, Feb 12, 2018 at 03:24:26PM +0000, Julien Grall wrote:
>
>
> On 12/02/18 15:17, Zhongze Liu wrote:
> > Hi Julien,
>
> Hi,
>
> >
> > 2018-02-12 23:09 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> > > Hi,
> > >
> > > On 12/02/18 14:52, Zhongze Liu wrote:
> > > >
> > > > 2018-02-08 0:54 GMT+08:00 Julien Grall <julien.grall@arm.com>:
> > > > >
> > > > > On 07/02/18 16:27, Zhongze Liu wrote:
> > > >
> > > > It seems that I mistakenly use transaction as a global lock. Now I don't
> > > > have
> > > > any reasons not putting the unmap out of the transaction, but this will
> > > > break
> > > > the original transaction into two, and I do think that we need some
> > > > explicit
> > > > locking here.
> > >
> > >
> > > Can you explain why you need specific locking here? What are you trying to
> > > protect? Are you trying to protect against two process doing the unmap?
> > >
> >
> > Yes.
>
> I don't think you have to worry about the locking here. With the current
> interface, the regions cannot be modified after the guest has booted. So the
> addresses will always stay valid.
>
> This code path should never be called concurrently, as a lot of code in
> libxl, so I think someone else will take care about that for you (I will let
> Wei confirm here).
>
> In any case, the worst that could happen is the unmap is called twice on the
> same region. So you would get spurious error message. Not that bad.
Yeah, not that bad. Not going to be a security issue, not going to leak
resources in the end.
To avoid spurious unmap, can we maybe unmap the pages after the xenstore
transaction is committed? In that case, only the successful one gets to
unmap, the ones that aren't committed will bail.
(Just tossing around ideas)
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-14 8:37 ` Jan Beulich
@ 2018-02-14 17:02 ` Zhongze Liu
2018-02-15 8:58 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-14 17:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
>> Hi Jan,
>>
>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>>> I've updated the comments according to your previous suggestions,
>>>> do they look good to you?
>>>
>>> The one in the public header is way too verbose. I specifically don't
>>> see why you would need to spell out XSM privilege requirements
>>> there. Please make new comments match existing ones in style and
>>> verbosity if at all possible, while still conveying all necessary /
>>> relevant information.
>>>
>>
>> I shortened it a little bit, and now it looks like:
>>
>> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
>> gmfn_foreign,
>> if (c) tries to map pages from (t) into
>> (d), this doesn't require that (d) itself
>> has the privilege to map the pages, but
>> instead requires that (c) has the
>> privilege to do so, as long as (d) and (t)
>> are allowed to share memory pages.
>> This is XENMEM_add_to_physmap_batch only,
>> and currently ARM only. */
>
> Which leaves unclear what (c), (d), and (t) are. How about
>
> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
> <explain here what the difference is with a few simple words>."
>
> (You can and should go into further detail in the commit message.)
> Without this _properly_ explained, I'll continue to ask why you
> can't simply make XENMAPSPACE_gmfn_foreign do what you want
> (as it already takes two domid_t-s as input), by suitably adjusting
> its XSM check(s).
I'm sorry that I failed to see the reason why you say "which leaves
unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
from (t) into (d)" has already included the necessary information
about this: (c) is the caller of the hypercall (current), (d) is the
dest domain, and (t) the source domain.
I think I still need more of your explanation here.
>
> You'd also need to adjust the comment on the foreign_domid
> structure field, as it saying "gmfn_foreign" would otherwise become
> stale with your change.
Thanks for pointing out that. I've already updated it.
>
> I don't, btw, like the ARM only part here - there's nothing
> inherently wrong with the same operation being sensible on x86.
>
I agree that we should make this also available to x86 guests, but
we have to fix the FIXME in x86/mm/p2m.c: p2m_add_foreign() first.
And that, I think, should be done in another patch set. And "currently"
here also means that it's planned to be fixed. I just don't want to
disappoint the users who are eager to try this new subop out but end
up getting weired errors.
Cheers,
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-14 17:02 ` Zhongze Liu
@ 2018-02-15 8:58 ` Jan Beulich
2018-02-24 2:50 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2018-02-15 8:58 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 14.02.18 at 18:02, <blackskygg@gmail.com> wrote:
> 2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
>>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>>>> I've updated the comments according to your previous suggestions,
>>>>> do they look good to you?
>>>>
>>>> The one in the public header is way too verbose. I specifically don't
>>>> see why you would need to spell out XSM privilege requirements
>>>> there. Please make new comments match existing ones in style and
>>>> verbosity if at all possible, while still conveying all necessary /
>>>> relevant information.
>>>>
>>>
>>> I shortened it a little bit, and now it looks like:
>>>
>>> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
>>> gmfn_foreign,
>>> if (c) tries to map pages from (t) into
>>> (d), this doesn't require that (d) itself
>>> has the privilege to map the pages, but
>>> instead requires that (c) has the
>>> privilege to do so, as long as (d) and (t)
>>> are allowed to share memory pages.
>>> This is XENMEM_add_to_physmap_batch only,
>>> and currently ARM only. */
>>
>> Which leaves unclear what (c), (d), and (t) are. How about
>>
>> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
>> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
>> <explain here what the difference is with a few simple words>."
>>
>> (You can and should go into further detail in the commit message.)
>> Without this _properly_ explained, I'll continue to ask why you
>> can't simply make XENMAPSPACE_gmfn_foreign do what you want
>> (as it already takes two domid_t-s as input), by suitably adjusting
>> its XSM check(s).
>
> I'm sorry that I failed to see the reason why you say "which leaves
> unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
> from (t) into (d)" has already included the necessary information
> about this: (c) is the caller of the hypercall (current), (d) is the
> dest domain, and (t) the source domain.
> I think I still need more of your explanation here.
Someone coming across _just_ this comment (while reading the
public header) will not necessarily know what (c), (d), and (t)
stand for, and (s)he shouldn't be forced to dig into git history to
find the patch description. But anyway - all this should go away
from the header anyway, as explained before. All that's needed
here is a terse but understandable explanation of what's different
from XENMAPSPACE_gmfn_foreign.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-15 8:58 ` Jan Beulich
@ 2018-02-24 2:50 ` Zhongze Liu
2018-02-24 5:37 ` Zhongze Liu
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-24 2:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
Hi Jan,
(Last week was the Chinese Spring Festival, so I failed to follow up
timely. Sorry for that.)
2018-02-15 16:58 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>> On 14.02.18 at 18:02, <blackskygg@gmail.com> wrote:
>> 2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
>>>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>>>>> I've updated the comments according to your previous suggestions,
>>>>>> do they look good to you?
>>>>>
>>>>> The one in the public header is way too verbose. I specifically don't
>>>>> see why you would need to spell out XSM privilege requirements
>>>>> there. Please make new comments match existing ones in style and
>>>>> verbosity if at all possible, while still conveying all necessary /
>>>>> relevant information.
>>>>>
>>>>
>>>> I shortened it a little bit, and now it looks like:
>>>>
>>>> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
>>>> gmfn_foreign,
>>>> if (c) tries to map pages from (t) into
>>>> (d), this doesn't require that (d) itself
>>>> has the privilege to map the pages, but
>>>> instead requires that (c) has the
>>>> privilege to do so, as long as (d) and (t)
>>>> are allowed to share memory pages.
>>>> This is XENMEM_add_to_physmap_batch only,
>>>> and currently ARM only. */
>>>
>>> Which leaves unclear what (c), (d), and (t) are. How about
>>>
>>> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
>>> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
>>> <explain here what the difference is with a few simple words>."
>>>
>>> (You can and should go into further detail in the commit message.)
>>> Without this _properly_ explained, I'll continue to ask why you
>>> can't simply make XENMAPSPACE_gmfn_foreign do what you want
>>> (as it already takes two domid_t-s as input), by suitably adjusting
>>> its XSM check(s).
>>
>> I'm sorry that I failed to see the reason why you say "which leaves
>> unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
>> from (t) into (d)" has already included the necessary information
>> about this: (c) is the caller of the hypercall (current), (d) is the
>> dest domain, and (t) the source domain.
>> I think I still need more of your explanation here.
>
> Someone coming across _just_ this comment (while reading the
> public header) will not necessarily know what (c), (d), and (t)
> stand for, and (s)he shouldn't be forced to dig into git history to
> find the patch description. But anyway - all this should go away
> from the header anyway, as explained before. All that's needed
> here is a terse but understandable explanation of what's different
> from XENMAPSPACE_gmfn_foreign.
>
I think before we can reach a consensus on how the final comment
should look like, we need to reach an agreement on what should
be included in the <differences> part. And according to our previous
discussion, below is what I think is necessary so far:
1. Different privilege requirements
2. Why we can't just modify the original hypercall to meet our needs.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-24 2:50 ` Zhongze Liu
@ 2018-02-24 5:37 ` Zhongze Liu
2018-02-26 7:53 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Zhongze Liu @ 2018-02-24 5:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
... Sorry for the incomplete mail. I somehow hit the "send" button before I
finish composing the previous mail. And now it continues...
2018-02-24 10:50 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
> Hi Jan,
>
> (Last week was the Chinese Spring Festival, so I failed to follow up
> timely. Sorry for that.)
>
> 2018-02-15 16:58 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>> On 14.02.18 at 18:02, <blackskygg@gmail.com> wrote:
>>> 2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
>>>>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>>>>>> I've updated the comments according to your previous suggestions,
>>>>>>> do they look good to you?
>>>>>>
>>>>>> The one in the public header is way too verbose. I specifically don't
>>>>>> see why you would need to spell out XSM privilege requirements
>>>>>> there. Please make new comments match existing ones in style and
>>>>>> verbosity if at all possible, while still conveying all necessary /
>>>>>> relevant information.
>>>>>>
>>>>>
>>>>> I shortened it a little bit, and now it looks like:
>>>>>
>>>>> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
>>>>> gmfn_foreign,
>>>>> if (c) tries to map pages from (t) into
>>>>> (d), this doesn't require that (d) itself
>>>>> has the privilege to map the pages, but
>>>>> instead requires that (c) has the
>>>>> privilege to do so, as long as (d) and (t)
>>>>> are allowed to share memory pages.
>>>>> This is XENMEM_add_to_physmap_batch only,
>>>>> and currently ARM only. */
>>>>
>>>> Which leaves unclear what (c), (d), and (t) are. How about
>>>>
>>>> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
>>>> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
>>>> <explain here what the difference is with a few simple words>."
>>>>
>>>> (You can and should go into further detail in the commit message.)
>>>> Without this _properly_ explained, I'll continue to ask why you
>>>> can't simply make XENMAPSPACE_gmfn_foreign do what you want
>>>> (as it already takes two domid_t-s as input), by suitably adjusting
>>>> its XSM check(s).
>>>
>>> I'm sorry that I failed to see the reason why you say "which leaves
>>> unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
>>> from (t) into (d)" has already included the necessary information
>>> about this: (c) is the caller of the hypercall (current), (d) is the
>>> dest domain, and (t) the source domain.
>>> I think I still need more of your explanation here.
>>
>> Someone coming across _just_ this comment (while reading the
>> public header) will not necessarily know what (c), (d), and (t)
>> stand for, and (s)he shouldn't be forced to dig into git history to
>> find the patch description. But anyway - all this should go away
>> from the header anyway, as explained before. All that's needed
>> here is a terse but understandable explanation of what's different
>> from XENMAPSPACE_gmfn_foreign.
>>
>
> I think before we can reach a consensus on how the final comment
> should look like, we need to reach an agreement on what should
> be included in the <differences> part. And according to our previous
> discussion, below is what I think is necessary so far:
>
> 1. Different privilege requirements
>
This is what I've been trying to convey in the comment. But it seems that
I failed to make it "terse yet understandable". Now my question is: Am I *not*
supposed to spell out the detail of the exact difference between their
privilege requirements? If so, do you have a rough picture of how it
should look like? Are we going to give an example of a use case where
the old subop is not applicable?
>
> 2. Why we can't just modify the original hypercall to meet our needs.
According to our discussion on the previous versions of this patch, we
can't fit the checks needed by both subop into one because doing so will
either regress the original one or permit something that shouldn't be
allowed in the new use cases. Should we clarify this in the comment, too?
Cheers,
and Best Wishes.
Zhongze Liu
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing
2018-02-24 5:37 ` Zhongze Liu
@ 2018-02-26 7:53 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2018-02-26 7:53 UTC (permalink / raw)
To: Zhongze Liu
Cc: Stefano Stabellini, Wei Liu, Ian Jackson, xen-devel,
Julien Grall, Daniel De Graaf
>>> On 24.02.18 at 06:37, <blackskygg@gmail.com> wrote:
> ... Sorry for the incomplete mail. I somehow hit the "send" button before I
> finish composing the previous mail. And now it continues...
>
> 2018-02-24 10:50 GMT+08:00 Zhongze Liu <blackskygg@gmail.com>:
>> Hi Jan,
>>
>> (Last week was the Chinese Spring Festival, so I failed to follow up
>> timely. Sorry for that.)
>>
>> 2018-02-15 16:58 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>> On 14.02.18 at 18:02, <blackskygg@gmail.com> wrote:
>>>> 2018-02-14 16:37 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>> On 14.02.18 at 08:15, <blackskygg@gmail.com> wrote:
>>>>>> 2018-02-13 23:26 GMT+08:00 Jan Beulich <JBeulich@suse.com>:
>>>>>>>>>> On 13.02.18 at 16:15, <blackskygg@gmail.com> wrote:
>>>>>>>> I've updated the comments according to your previous suggestions,
>>>>>>>> do they look good to you?
>>>>>>>
>>>>>>> The one in the public header is way too verbose. I specifically don't
>>>>>>> see why you would need to spell out XSM privilege requirements
>>>>>>> there. Please make new comments match existing ones in style and
>>>>>>> verbosity if at all possible, while still conveying all necessary /
>>>>>>> relevant information.
>>>>>>>
>>>>>>
>>>>>> I shortened it a little bit, and now it looks like:
>>>>>>
>>>>>> #define XENMAPSPACE_gmfn_share 6 /* GMFN from another dom. Unlike
>>>>>> gmfn_foreign,
>>>>>> if (c) tries to map pages from (t)
> into
>>>>>> (d), this doesn't require that (d)
> itself
>>>>>> has the privilege to map the pages,
> but
>>>>>> instead requires that (c) has the
>>>>>> privilege to do so, as long as (d) and
> (t)
>>>>>> are allowed to share memory pages.
>>>>>> This is XENMEM_add_to_physmap_batch
> only,
>>>>>> and currently ARM only. */
>>>>>
>>>>> Which leaves unclear what (c), (d), and (t) are. How about
>>>>>
>>>>> "GMFN from another dom, XENMEM_add_to_physmap_batch (and
>>>>> currently ARM) only. Other than XENMAPSPACE_gmfn_foreign this
>>>>> <explain here what the difference is with a few simple words>."
>>>>>
>>>>> (You can and should go into further detail in the commit message.)
>>>>> Without this _properly_ explained, I'll continue to ask why you
>>>>> can't simply make XENMAPSPACE_gmfn_foreign do what you want
>>>>> (as it already takes two domid_t-s as input), by suitably adjusting
>>>>> its XSM check(s).
>>>>
>>>> I'm sorry that I failed to see the reason why you say "which leaves
>>>> unclear what (c), (d), and (t) are". I think "if (c) tries to map pages
>>>> from (t) into (d)" has already included the necessary information
>>>> about this: (c) is the caller of the hypercall (current), (d) is the
>>>> dest domain, and (t) the source domain.
>>>> I think I still need more of your explanation here.
>>>
>>> Someone coming across _just_ this comment (while reading the
>>> public header) will not necessarily know what (c), (d), and (t)
>>> stand for, and (s)he shouldn't be forced to dig into git history to
>>> find the patch description. But anyway - all this should go away
>>> from the header anyway, as explained before. All that's needed
>>> here is a terse but understandable explanation of what's different
>>> from XENMAPSPACE_gmfn_foreign.
>>>
>>
>> I think before we can reach a consensus on how the final comment
>> should look like, we need to reach an agreement on what should
>> be included in the <differences> part. And according to our previous
>> discussion, below is what I think is necessary so far:
>>
>> 1. Different privilege requirements
>>
>
> This is what I've been trying to convey in the comment. But it seems that
> I failed to make it "terse yet understandable". Now my question is: Am I *not*
> supposed to spell out the detail of the exact difference between their
> privilege requirements? If so, do you have a rough picture of how it
> should look like? Are we going to give an example of a use case where
> the old subop is not applicable?
>
>>
>> 2. Why we can't just modify the original hypercall to meet our needs.
>
> According to our discussion on the previous versions of this patch, we
> can't fit the checks needed by both subop into one because doing so will
> either regress the original one or permit something that shouldn't be
> allowed in the new use cases. Should we clarify this in the comment, too?
My view on this is: The comment ought to describe the behavior,
nothing else. The commit message ought to explain the difference
to the existing sub-op, to help understand / explain the reason
why the existing one can't be suitably extended.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction
2018-02-14 14:39 ` Wei Liu
@ 2018-02-26 12:08 ` Ian Jackson
0 siblings, 0 replies; 45+ messages in thread
From: Ian Jackson @ 2018-02-26 12:08 UTC (permalink / raw)
To: Wei Liu; +Cc: Julien Grall, Stefano Stabellini, Zhongze Liu, xen-devel
Wei Liu writes ("Re: [Xen-devel] [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction"):
> On Mon, Feb 12, 2018 at 03:24:26PM +0000, Julien Grall wrote:
> > In any case, the worst that could happen is the unmap is called twice on the
> > same region. So you would get spurious error message. Not that bad.
>
> Yeah, not that bad. Not going to be a security issue, not going to leak
> resources in the end.
>
> To avoid spurious unmap, can we maybe unmap the pages after the xenstore
> transaction is committed? In that case, only the successful one gets to
> unmap, the ones that aren't committed will bail.
>
> (Just tossing around ideas)
It should be the other way around. Because, your way, if your process
crashes for some reason between the xenstore commit and the unmap, the
memory is leaked.
Instead, do the unmap first. Check the error code to see if it means
"this was already unmapped" and if so report that only via a debug log
message.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2018-02-26 12:08 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-30 17:50 [PATCH v4 0/7] Allow setting up shared memory areas between VMs from xl config files Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 1/7] libxc: add xc_domain_remove_from_physmap to wrap XENMEM_remove_from_physmap Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 2/7] xen: xsm: flask: introduce XENMAPSPACE_gmfn_share for memory sharing Zhongze Liu
2018-02-01 10:23 ` Jan Beulich
2018-02-01 18:11 ` Zhongze Liu
2018-02-02 8:32 ` Jan Beulich
2018-02-05 9:59 ` Zhongze Liu
2018-02-13 15:15 ` Zhongze Liu
2018-02-13 15:26 ` Jan Beulich
2018-02-14 7:15 ` Zhongze Liu
2018-02-14 8:37 ` Jan Beulich
2018-02-14 17:02 ` Zhongze Liu
2018-02-15 8:58 ` Jan Beulich
2018-02-24 2:50 ` Zhongze Liu
2018-02-24 5:37 ` Zhongze Liu
2018-02-26 7:53 ` Jan Beulich
2018-02-06 11:04 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 3/7] libxl: introduce a new structure to represent static shared memory regions Zhongze Liu
2018-02-06 11:27 ` Julien Grall
2018-02-06 15:41 ` Zhongze Liu
2018-02-06 15:46 ` Julien Grall
2018-02-06 16:06 ` Zhongze Liu
2018-02-06 17:23 ` Julien Grall
2018-01-30 17:50 ` [PATCH v4 4/7] libxl: support mapping static shared memory areas during domain creation Zhongze Liu
2018-02-06 13:07 ` Julien Grall
2018-02-06 15:59 ` Zhongze Liu
2018-02-06 17:30 ` Julien Grall
2018-02-06 17:47 ` Wei Liu
2018-02-12 15:08 ` Zhongze Liu
2018-02-14 14:26 ` Wei Liu
2018-01-30 17:50 ` [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction Zhongze Liu
2018-02-06 13:24 ` Julien Grall
2018-02-06 18:06 ` Wei Liu
2018-02-07 16:27 ` Zhongze Liu
2018-02-07 16:54 ` Julien Grall
2018-02-12 14:52 ` Zhongze Liu
2018-02-12 15:09 ` Julien Grall
2018-02-12 15:17 ` Zhongze Liu
2018-02-12 15:24 ` Julien Grall
2018-02-14 14:35 ` Wei Liu
2018-02-14 14:39 ` Wei Liu
2018-02-26 12:08 ` Ian Jackson
2018-01-30 17:50 ` [PATCH v4 6/7] libxl:xl: add parsing code to parse "libxl_static_sshm" from xl config files Zhongze Liu
2018-01-30 17:50 ` [PATCH v4 7/7] docs: documentation about static shared memory regions Zhongze Liu
2018-02-06 13:28 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.