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

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 <[email protected]>

Cc Andrew Cooper <[email protected]>

Cc George Dunlap <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Tim Deegan <[email protected]>

Cc Wei Liu <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

Quoted 5 lines

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]).

Quoted 3 lines

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

Acked-by Wei Liu <[email protected]>

I am wondering if there is a chance to extract the key=value parsing into standalone function and use it everywhere though.

Quoted 12 lines

Cc Andrew Cooper <[email protected]>

Cc George Dunlap <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Tim Deegan <[email protected]>

Cc Wei Liu <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

Correctly trim your CC list next time please. A lot of hypervisor people don't need to get CCed on this patch.

2017-09-02 0:03 GMT+08:00 Wei Liu <[email protected]>:

Quoted 8 lines · Zhongze Liu

Quoted 7 lines

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

Quoted 6 lines

Quoted 1 line

Signed-off-by Zhongze Liu <[email protected]>

Acked-by Wei Liu <[email protected]>

I am wondering if there is a chance to extract the key=value parsing into standalone function and use it everywhere though.

I think this is a good idea.

Quoted 14 lines

Quoted 13 lines

Cc Andrew Cooper <[email protected]>

Cc George Dunlap <[email protected]>

Cc Ian Jackson <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Stefano Stabellini <[email protected]>

Cc Tim Deegan <[email protected]>

Cc Wei Liu <[email protected]>

Cc Julien Grall <[email protected]>

Cc [email protected]

Quoted 2 lines

Correctly trim your CC list next time please. A lot of hypervisor people don't need to get CCed on this patch.

Actually, this list is generated by get_maintainer.pl. Maybe next time I should go carefully through the list and remove some of the names manually.

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 <[email protected]>

Quoted 1 line

Quoted 1 line · On 27.08.17 at 10:36, <[email protected]> wrote:

xen/arch/arm/mm.c +1 -1

@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);

xen/xsm/flask/hooks.c +1 -1

@@ -1165,9 +1165,15 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,

Quoted 15 lines

Quoted 1 line

Quoted 1 line · On 27.08.17 at 10:36, <[email protected]> wrote:

xen/arch/arm/mm.c +1 -1

@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);

xen/arch/x86/mm/p2m.c int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, · line 2548

Quoted 5 lines

xen/arch/x86/mm/p2m.c +1 -1

@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 
-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);

I continue to dislike the added arguments here, as being pointless to pass. I'm not the maintainer of either of the modified files, so I won't (and can't) veto the change though.

Quoted 6 lines

xen/include/xsm/dummy.h +0 -0

@@ -525,10 +525,14 @@ static XSM_INLINE int xsm_remove_from_physmap(XSM_DEFAULT_ARG struct domain *d1,
     return xsm_default_action(action, d1, d2);
 }
 

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

Quoted 6 lines

xen/include/xsm/dummy.h +3 -1

-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
+    int rc;
     XSM_ASSERT_ACTION(XSM_TARGET);

Missing blank line between declaration and statements.

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

Quoted 4 lines

xen/include/xsm/dummy.h +2 -1

-    return xsm_default_action(action, d, t);
+    rc = xsm_default_action(action, cd, d);
+    if (rc) return rc;

Coding style. In any event, as suggested before the whole thing is easier to write as

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

Quoted 1 line

xen/include/xsm/dummy.h +1 -0

+    return xsm_default_action(action, cd, t);

    return xsm_default_action(action, cd, d) ?: xsm_default_action(action, cd, t);

anyway imo (suitably split across lines if needed, of course).

Quoted 8 lines

xen/xsm/flask/hooks.c +1 -1

@@ -1165,9 +1165,15 @@ static int flask_remove_from_physmap(struct domain *d1, struct domain *d2)
     return domain_has_perm(d1, d2, SECCLASS_MMU, MMU__PHYSMAP);
 }
 
-static int flask_map_gmfn_foreign(struct domain *d, struct domain *t)
+static int flask_map_gmfn_foreign(struct domain *cd,

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

Quoted 10 lines

xen/xsm/flask/hooks.c +7 -1

+                                  struct domain *d, struct domain *t)
 {
-    return domain_has_perm(d, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    int rc;
+    rc = domain_has_perm(cd, d, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    if (rc) return rc;
+    rc = domain_has_perm(cd, t, SECCLASS_MMU, MMU__MAP_READ | MMU__MAP_WRITE);
+    if (rc) return rc;
+    return domain_has_perm(d, t, SECCLASS_MMU, MMU__SHARE_MEM);
 }

At least the style problems mentioned above apply here too.

Jan

Quoted 1 line

Quoted 1 line · On 28.08.17 at 13:01, <[email protected]> wrote:

2017-08-28 16:29 GMT+08:00 Jan Beulich <[email protected]>:

Quoted 9 lines

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

Quoted 7 lines

Quoted 1 line

Quoted 1 line · On 27.08.17 at 10:36, <[email protected]> wrote:

xen/include/xsm/dummy.h +3 -1

-static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *d, struct domain *t)
+static XSM_INLINE int xsm_map_gmfn_foreign(XSM_DEFAULT_ARG struct domain *cd,
+                                           struct domain *d, struct domain *t)
 {
+    int rc;
     XSM_ASSERT_ACTION(XSM_TARGET);

Missing blank line between declaration and statements.

Sorry. Will fix this.

Quoted 5 lines

Quoted 4 lines

xen/include/xsm/dummy.h +2 -1

-    return xsm_default_action(action, d, t);
+    rc = xsm_default_action(action, cd, d);
+    if (rc) return rc;

Quoted 6 lines

Coding style. In any event, as suggested before the whole thing is easier to write as

Quoted 1 line

+ return xsm_default_action(action, cd, t);

    return xsm_default_action(action, cd, d) ?: xsm_default_action(action, cd, t);

But aren't we going to preserve the error code here?

Quoted 18 lines

Quoted 1 line

Quoted 1 line · On 27.08.17 at 10:36, <[email protected]> wrote:

xen/arch/arm/mm.c +1 -1

@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);

xen/arch/x86/mm/p2m.c +0 -0

@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 

xen/arch/x86/mm/p2m.c int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, · line 2548

Quoted 2 lines

xen/arch/x86/mm/p2m.c +1 -1

-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);

I continue to dislike the added arguments here, as being pointless to pass. I'm not the maintainer of either of the modified files, so I won't (and can't) veto the change though.

Quoted 1 line

Quoted 1 line · On 28.08.17 at 16:24, <[email protected]> wrote:

Quoted 18 lines · Jan Beulich

Quoted 18 lines

Quoted 1 line

Quoted 1 line · On 27.08.17 at 10:36, <[email protected]> wrote:

xen/arch/arm/mm.c +1 -1

@@ -1284,7 +1284,7 @@ int xenmem_add_to_physmap_one(
             return -EINVAL;
         }
 
-        rc = xsm_map_gmfn_foreign(XSM_TARGET, d, od);
+        rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, d, od);
         if ( rc )
         {
             rcu_unlock_domain(od);

xen/arch/x86/mm/p2m.c +0 -0

@@ -2545,7 +2545,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,
     if ( tdom == fdom )
         goto out;
 

Quoted 6 lines

xen/arch/x86/mm/p2m.c int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, · line 2548

Quoted 2 lines

xen/arch/x86/mm/p2m.c +1 -1

-    rc = xsm_map_gmfn_foreign(XSM_TARGET, tdom, fdom);
+    rc = xsm_map_gmfn_foreign(XSM_TARGET, current->domain, tdom, fdom);

I continue to dislike the added arguments here, as being pointless to pass. I'm not the maintainer of either of the modified files, so I won't (and can't) veto the change though.

You mean, you think xsm_map_gmfn_foreign() can look up 'current' itself?

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

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

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

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

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

tools/libxl/libxl_sshm.c new file +12 -0

+
+        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) {

tools/libxl/libxl_sshm.c new file +16 -0

+        }
+
+        /* 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;
+    }
+

Quoted 21 lines

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

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

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

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

[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html

Signed-off-by Zhongze Liu <[email protected]>

[...]

Quoted 17 lines

tools/libxl/libxl_sshm.c new file +17 -0

+
+        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;
+        }
+

tools/libxl/libxl_sshm.c line 260

Quoted 5 lines

tools/libxl/libxl_sshm.c new file +5 -0

+        /* all checks passed, do the job */
+        if (!isretry) {
+            rc = libxl__sshm_do_map(gc, master_domid, domid, sshm,
+                                    master_sshm.begin, master_sshm.end);
+            if (rc) goto out;

You probably need to roll back the mapping should the transaction gets aborted in this loop.

Quoted 21 lines

tools/libxl/libxl_sshm.c new file +20 -0

+        }
+
+        /* 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:
+    libxl__xs_transaction_abort(gc, &xt);
+    return rc;

tools/libxl/libxl_sshm.c line 291

Quoted 6 lines

tools/libxl/libxl_sshm.c new file +6 -0

+}
+
+static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
+                                  libxl_static_shm *sshm)
+{
+

No refcount increment here?

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.

[1] Proposla 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

v2:
* fixed several code style issues.
* introduce MMU__SHARE_MEM in flask av permissions, and add a check to this. permission in the flask hook for xsm_map_gmfn_foreign.
* support rolling back during creation on partial failure.
* refcounting the sshm path instead of using "alive" and "zombie" to label the master and counting the slaves.

Cheers,

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

 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        |  15 ++
 tools/libxl/libxl_sshm.c            | 480 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl         |  34 ++-
 tools/libxl/libxl_x86.c             |  18 ++
 tools/libxl/libxlu_sshm.c           | 210 ++++++++++++++++
 tools/libxl/libxlutil.h             |   6 +
 tools/xl/xl_parse.c                 |  24 +-
 xen/arch/arm/mm.c                   |   2 +-

 xen/arch/x86/mm/p2m.c               |   2 +-
 xen/include/xsm/dummy.h             |   8 +-
 xen/include/xsm/xsm.h               |   7 +-
 xen/xsm/flask/hooks.c               |  10 +-
 xen/xsm/flask/policy/access_vectors |   4 +
 22 files changed, 883 insertions(+), 15 deletions(-)
 create mode 100644 tools/libxl/libxl_sshm.c
 create mode 100644 tools/libxl/libxlu_sshm.c

Signature · 1 line

2.14.0

Quoted 8 lines

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.

[1] Proposla 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

Quoted 7 lines

v2:
* fixed several code style issues.
* introduce MMU__SHARE_MEM in flask av permissions, and add a check to this. permission in the flask hook for xsm_map_gmfn_foreign.
* support rolling back during creation on partial failure.
* refcounting the sshm path instead of using "alive" and "zombie" to label the master and counting the slaves.

Hey Zhongze,

any plans on sending an update of this series?

Quoted 45 lines

Cheers,

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

 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        |  15 ++
 tools/libxl/libxl_sshm.c            | 480 ++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl         |  34 ++-
 tools/libxl/libxl_x86.c             |  18 ++
 tools/libxl/libxlu_sshm.c           | 210 ++++++++++++++++
 tools/libxl/libxlutil.h             |   6 +
 tools/xl/xl_parse.c                 |  24 +-
 xen/arch/arm/mm.c                   |   2 +-
 xen/arch/x86/mm/p2m.c               |   2 +-
 xen/include/xsm/dummy.h             |   8 +-
 xen/include/xsm/xsm.h               |   7 +-
 xen/xsm/flask/hooks.c               |  10 +-
 xen/xsm/flask/policy/access_vectors |   4 +
 22 files changed, 883 insertions(+), 15 deletions(-)
 create mode 100644 tools/libxl/libxl_sshm.c
 create mode 100644 tools/libxl/libxlu_sshm.c

--
2.14.0