Skip to contentSkip to conversation

x86/mm: Switch paging/sharing/access per-page ops to memops

PATCH In review Andres Lagar-Cavilla 3 participants 11 messages Feb 1, 2012 – Feb 3, 2012

lore
Share & tools
lore

Andres Lagar-Cavilla

author
#0 lore raw

PATCH 1/1 x86/mm: Switch paging/sharing/access per-page ops to memops

Per page operations in the paging, sharing, and access tracking subsystems are all implemented with domctls (e.g. a domctl to evict one page, or to share one page).

Under heavy load, the domctl path reveals a lack of scalability. The domctl lock serializes dom0's vcpus in the hypervisor. When performing thousands of per-page operations on dozens of domains, these vcpus will spin in the hypervisor. Beyond the aggressive locking, an added inefficiency of blocking vcpus in the domctl lock is that dom0 is prevented from re-scheduling any of its other work-starved processes.

We retain the domctl interface for setting up and tearing down paging/sharing/mem access for a domain. But we migrate all the per page operations to use the memory_op hypercalls (e.g XENMEM_*).

A new tools-only section is added to the public memory.h header.

This is a backwards-incompatible ABI change. It's been floating on the list for a couple weeks now, with no nacks thus far.

Signed-off-by Andres Lagar-Cavilla <andres@lagarcavilla>
Signed-off-by Adin Scannell <[email protected]>
 tools/libxc/xc_mem_access.c          |   12 +-
 tools/libxc/xc_mem_event.c           |   23 +++-
 tools/libxc/xc_mem_paging.c          |   44 ++++----
 tools/libxc/xc_memshr.c              |  182 ++++++++++++++++------------------
 tools/libxc/xenctrl.h                |    6 +-
 tools/memshr/interface.c             |    4 +-
 tools/tests/mem-sharing/memshrtool.c |    4 +-
 xen/arch/x86/domctl.c                |    1 -
 xen/arch/x86/mm/mem_access.c         |    7 +-
 xen/arch/x86/mm/mem_event.c          |   68 ++++++++++--
 xen/arch/x86/mm/mem_paging.c         |   13 +-
 xen/arch/x86/mm/mem_sharing.c        |   99 +++++++++++-------
 xen/arch/x86/x86_64/compat/mm.c      |   23 ++++
 xen/arch/x86/x86_64/mm.c             |   23 ++++
 xen/include/asm-x86/mem_access.h     |    3 +-
 xen/include/asm-x86/mem_event.h      |    2 +
 xen/include/asm-x86/mem_paging.h     |    3 +-
 xen/include/asm-x86/mem_sharing.h    |    3 +
 xen/include/public/domctl.h          |   90 +++-------------
 xen/include/public/memory.h          |   87 ++++++++++++++++
 tools/libxc/xc_memshr.c              |   11 ++
 tools/libxc/xenctrl.h                |    1 +
 tools/tests/mem-sharing/memshrtool.c |   11 ++
 xen/arch/x86/mm/mem_sharing.c        |   13 +-
 xen/arch/x86/x86_64/compat/mm.c      |    3 +
 xen/arch/x86/x86_64/mm.c             |    2 +
 xen/include/asm-x86/mem_sharing.h    |    3 +-
 xen/include/public/memory.h          |    1 +
 28 files changed, 458 insertions(+), 284 deletions(-)
author
#1 lore raw

PATCH 1/1 Use memops for mem paging, sharing, and access, instead of domctls

 tools/libxc/xc_mem_access.c          |   12 +-
 tools/libxc/xc_mem_event.c           |   23 +++-
 tools/libxc/xc_mem_paging.c          |   44 ++++----
 tools/libxc/xc_memshr.c              |  182 ++++++++++++++++------------------
 tools/libxc/xenctrl.h                |    6 +-
 tools/memshr/interface.c             |    4 +-
 tools/tests/mem-sharing/memshrtool.c |    4 +-
 xen/arch/x86/domctl.c                |    1 -
 xen/arch/x86/mm/mem_access.c         |    7 +-
 xen/arch/x86/mm/mem_event.c          |   68 ++++++++++--
 xen/arch/x86/mm/mem_paging.c         |   13 +-
 xen/arch/x86/mm/mem_sharing.c        |   99 +++++++++++-------
 xen/arch/x86/x86_64/compat/mm.c      |   23 ++++
 xen/arch/x86/x86_64/mm.c             |   23 ++++
 xen/include/asm-x86/mem_access.h     |    3 +-
 xen/include/asm-x86/mem_event.h      |    2 +
 xen/include/asm-x86/mem_paging.h     |    3 +-
 xen/include/asm-x86/mem_sharing.h    |    3 +
 xen/include/public/domctl.h          |   90 +++-------------
 xen/include/public/memory.h          |   87 ++++++++++++++++
 20 files changed, 421 insertions(+), 276 deletions(-)

Per page operations in the paging, sharing, and access tracking subsystems are all implemented with domctls (e.g. a domctl to evict one page, or to share one page).

Under heavy load, the domctl path reveals a lack of scalability. The domctl lock serializes dom0's vcpus in the hypervisor. When performing thousands of per-page operations on dozens of domains, these vcpus will spin in the hypervisor. Beyond the aggressive locking, an added inefficiency of blocking vcpus in the domctl lock is that dom0 is prevented from re-scheduling any of its other work-starved processes.

We retain the domctl interface for setting up and tearing down paging/sharing/mem access for a domain. But we migrate all the per page operations to use the memory_op hypercalls (e.g XENMEM_*).

Signed-off-by Andres Lagar-Cavilla <andres@lagarcavilla>
Signed-off-by Adin Scannell <[email protected]>
tools/libxc/xc_mem_access.c +6 -6
diff -r abfc492fd04d -r 8e64acc49aa8 tools/libxc/xc_mem_access.c
@@ -30,7 +30,7 @@ int xc_mem_access_enable(xc_interface *x
     return xc_mem_event_control(xch, domain_id,
                                 XEN_DOMCTL_MEM_EVENT_OP_ACCESS_ENABLE,
                                 XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
-                                shared_page, ring_page, INVALID_MFN);
+                                shared_page, ring_page);
 }
 
 int xc_mem_access_disable(xc_interface *xch, domid_t domain_id)

@@ -38,15 +38,15 @@ int xc_mem_access_disable(xc_interface *
     return xc_mem_event_control(xch, domain_id,
                                 XEN_DOMCTL_MEM_EVENT_OP_ACCESS_DISABLE,
                                 XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
-                                NULL, NULL, INVALID_MFN);
+                                NULL, NULL);
 }
 
 int xc_mem_access_resume(xc_interface *xch, domid_t domain_id, unsigned long gfn)
 {
-    return xc_mem_event_control(xch, domain_id,
-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS_RESUME,
-                                XEN_DOMCTL_MEM_EVENT_OP_ACCESS,
-                                NULL, NULL, gfn);
+    return xc_mem_event_memop(xch, domain_id,
+                                XENMEM_access_op_resume,
+                                XENMEM_access_op,
+                                gfn, NULL);
 }
 
 /*
tools/libxc/xc_mem_event.c +18 -5
diff -r abfc492fd04d -r 8e64acc49aa8 tools/libxc/xc_mem_event.c
@@ -24,8 +24,7 @@
 #include "xc_private.h"
 
 int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned int op,
-                         unsigned int mode, void *page,
-                         void *ring_page, unsigned long gfn)
+                         unsigned int mode, void *page, void *ring_page)
 {
     DECLARE_DOMCTL;
 

@@ -34,11 +33,25 @@ int xc_mem_event_control(xc_interface *x
     domctl.u.mem_event_op.op = op;
     domctl.u.mem_event_op.mode = mode;
 
-    domctl.u.mem_event_op.u.shared_addr = (unsigned long)page;
+    domctl.u.mem_event_op.shared_addr = (unsigned long)page;
     domctl.u.mem_event_op.ring_addr = (unsigned long)ring_page;
-
-    domctl.u.mem_event_op.gfn = gfn;
     
     return do_domctl(xch, &domctl);
 }
 
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id, 
+                        unsigned int op, unsigned int mode,
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;
+
+    return do_memory_op(xch, mode, &meo, sizeof(meo));
+}
+
tools/libxc/xc_mem_paging.c +22 -22

80 lines not on this page · raw message

tools/libxc/xc_memshr.c +84 -98

265 lines not on this page · raw message

tools/libxc/xenctrl.h +4 -2
diff -r abfc492fd04d -r 8e64acc49aa8 tools/libxc/xenctrl.h
@@ -1879,8 +1879,10 @@ int xc_tmem_restore_extra(xc_interface *
  * mem_event operations
  */
 int xc_mem_event_control(xc_interface *xch, domid_t domain_id, unsigned int op,
-                         unsigned int mode, void *shared_page,
-                          void *ring_page, unsigned long gfn);
+                         unsigned int mode, void *shared_page, void *ring_page);
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id, 
+                        unsigned int op, unsigned int mode,
+                        uint64_t gfn, void *buffer);
 
 int xc_mem_paging_enable(xc_interface *xch, domid_t domain_id,
                         void *shared_page, void *ring_page);
tools/memshr/interface.c +2 -2
diff -r abfc492fd04d -r 8e64acc49aa8 tools/memshr/interface.c
@@ -186,12 +186,12 @@ int memshr_vbd_issue_ro_request(char *bu
            remove the relevant ones from the map */
         switch(ret)
         {
-            case XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID:
+            case XENMEM_SHARING_OP_S_HANDLE_INVALID:
                 ret = blockshr_shrhnd_remove(memshr.blks, source_st, NULL);
                 if(ret) DPRINTF("Could not rm invl s_hnd: %u %"PRId64" %"PRId64"\n", 
                                     source_st.domain, source_st.frame, source_st.handle);
                 break;
-            case XEN_DOMCTL_MEM_SHARING_C_HANDLE_INVALID:
+            case XENMEM_SHARING_OP_C_HANDLE_INVALID:
                 ret = blockshr_shrhnd_remove(memshr.blks, client_st, NULL);
                 if(ret) DPRINTF("Could not rm invl c_hnd: %u %"PRId64" %"PRId64"\n", 
                                     client_st.domain, client_st.frame, client_st.handle);
tools/tests/mem-sharing/memshrtool.c +2 -2
diff -r abfc492fd04d -r 8e64acc49aa8 tools/tests/mem-sharing/memshrtool.c
@@ -34,9 +34,9 @@ static int usage(const char* prog)
     int rc = f; \
     if ( rc < 0 ) { \
         printf("error executing %s: %s\n", #f, \
-                ((errno * -1) == XEN_DOMCTL_MEM_SHARING_C_HANDLE_INVALID) ? \
+                ((errno * -1) == XENMEM_SHARING_OP_S_HANDLE_INVALID) ? \
                 "problem with client handle" :\
-                ((errno * -1) == XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID) ? \
+                ((errno * -1) == XENMEM_SHARING_OP_C_HANDLE_INVALID) ? \
                 "problem with source handle" : strerror(errno)); \
         return rc; \
     } \
xen/arch/x86/domctl.c +0 -1
diff -r abfc492fd04d -r 8e64acc49aa8 xen/arch/x86/domctl.c
@@ -1463,7 +1463,6 @@ long arch_do_domctl(
             if ( !ret )
                 ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
             rcu_unlock_domain(d);
-            copy_to_guest(u_domctl, domctl, 1);
         } 
     }
     break;
xen/arch/x86/mm/mem_access.c +3 -4
diff -r abfc492fd04d -r 8e64acc49aa8 xen/arch/x86/mm/mem_access.c
@@ -25,14 +25,13 @@
 #include <asm/mem_event.h>
 
 
-int mem_access_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
-                      XEN_GUEST_HANDLE(void) u_domctl)
+int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo)
 {
     int rc;
 
-    switch( mec->op )
+    switch( meo->op )
     {
-    case XEN_DOMCTL_MEM_EVENT_OP_ACCESS_RESUME:
+    case XENMEM_access_op_resume:
     {
         p2m_mem_access_resume(d);
         rc = 0;
xen/arch/x86/mm/mem_event.c +57 -11

101 lines not on this page · raw message

xen/arch/x86/mm/mem_paging.c +6 -7
diff -r abfc492fd04d -r 8e64acc49aa8 xen/arch/x86/mm/mem_paging.c
@@ -25,33 +25,32 @@
 #include <asm/mem_event.h>
 
 
-int mem_paging_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
-                      XEN_GUEST_HANDLE(void) u_domctl)
+int mem_paging_memop(struct domain *d, xen_mem_event_op_t *mec)
 {
     switch( mec->op )
     {
-    case XEN_DOMCTL_MEM_EVENT_OP_PAGING_NOMINATE:
+    case XENMEM_paging_op_nominate:
     {
         unsigned long gfn = mec->gfn;
         return p2m_mem_paging_nominate(d, gfn);
     }
     break;
 
-    case XEN_DOMCTL_MEM_EVENT_OP_PAGING_EVICT:
+    case XENMEM_paging_op_evict:
     {
         unsigned long gfn = mec->gfn;
         return p2m_mem_paging_evict(d, gfn);
     }
     break;
 
-    case XEN_DOMCTL_MEM_EVENT_OP_PAGING_PREP:
+    case XENMEM_paging_op_prep:
     {
         unsigned long gfn = mec->gfn;
-        return p2m_mem_paging_prep(d, gfn, mec->u.buffer);
+        return p2m_mem_paging_prep(d, gfn, mec->buffer);
     }
     break;
 
-    case XEN_DOMCTL_MEM_EVENT_OP_PAGING_RESUME:
+    case XENMEM_paging_op_resume:
     {
         p2m_mem_paging_resume(d);
         return 0;
xen/arch/x86/mm/mem_sharing.c +58 -41

254 lines not on this page · raw message

xen/arch/x86/x86_64/compat/mm.c +23 -0
diff -r abfc492fd04d -r 8e64acc49aa8 xen/arch/x86/x86_64/compat/mm.c
@@ -2,6 +2,7 @@
 #include <xen/multicall.h>
 #include <compat/memory.h>
 #include <compat/xen.h>
+#include <asm/mem_event.h>
 
 int compat_set_gdt(XEN_GUEST_HANDLE(uint) frame_list, unsigned int entries)
 {

@@ -211,6 +212,28 @@ int compat_arch_memory_op(int op, XEN_GU
     case XENMEM_get_sharing_shared_pages:
         return mem_sharing_get_nr_shared_mfns();
 
+    case XENMEM_paging_op:
+    case XENMEM_access_op:
+    {
+        xen_mem_event_op_t meo;
+        if ( copy_from_guest(&meo, arg, 1) )
+            return -EFAULT;
+        rc = do_mem_event_op(op, meo.domain, (void *) &meo);
+        if ( !rc && copy_to_guest(arg, &meo, 1) )
+            return -EFAULT;
+        break;
+    }
+    case XENMEM_sharing_op:
+    {
+        xen_mem_sharing_op_t mso;
+        if ( copy_from_guest(&mso, arg, 1) )
+            return -EFAULT;
+        rc = do_mem_event_op(op, mso.domain, (void *) &mso);
+        if ( !rc && copy_to_guest(arg, &mso, 1) )
+            return -EFAULT;
+        break;
+    }
+
     default:
         rc = -ENOSYS;
         break;
xen/arch/x86/x86_64/mm.c +23 -0
diff -r abfc492fd04d -r 8e64acc49aa8 xen/arch/x86/x86_64/mm.c
@@ -34,6 +34,7 @@
 #include <asm/msr.h>
 #include <asm/setup.h>
 #include <asm/numa.h>
+#include <asm/mem_event.h>
 #include <asm/mem_sharing.h>
 #include <public/memory.h>
 

@@ -1100,6 +1101,28 @@ long subarch_memory_op(int op, XEN_GUEST
     case XENMEM_get_sharing_shared_pages:
         return mem_sharing_get_nr_shared_mfns();
 
+    case XENMEM_paging_op:
+    case XENMEM_access_op:
+    {
+        xen_mem_event_op_t meo;
+        if ( copy_from_guest(&meo, arg, 1) )
+            return -EFAULT;
+        rc = do_mem_event_op(op, meo.domain, (void *) &meo);
+        if ( !rc && copy_to_guest(arg, &meo, 1) )
+            return -EFAULT;
+        break;
+    }
+    case XENMEM_sharing_op:
+    {
+        xen_mem_sharing_op_t mso;
+        if ( copy_from_guest(&mso, arg, 1) )
+            return -EFAULT;
+        rc = do_mem_event_op(op, mso.domain, (void *) &mso);
+        if ( !rc && copy_to_guest(arg, &mso, 1) )
+            return -EFAULT;
+        break;
+    }
+
     default:
         rc = -ENOSYS;
         break;
xen/include/asm-x86/mem_access.h +1 -2
diff -r abfc492fd04d -r 8e64acc49aa8 xen/include/asm-x86/mem_access.h
@@ -21,8 +21,7 @@
  */
 
 
-int mem_access_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
-                      XEN_GUEST_HANDLE(void) u_domctl);
+int mem_access_memop(struct domain *d, xen_mem_event_op_t *meo);
 int mem_access_send_req(struct domain *d, mem_event_request_t *req);
xen/include/asm-x86/mem_event.h +2 -0
diff -r abfc492fd04d -r 8e64acc49aa8 xen/include/asm-x86/mem_event.h
@@ -42,6 +42,8 @@ void mem_event_put_request(struct domain
 int mem_event_get_response(struct domain *d, struct mem_event_domain *med,
                            mem_event_response_t *rsp);
 
+struct domain *get_mem_event_op_target(uint32_t domain, int *rc);
+int do_mem_event_op(int op, uint32_t domain, void *arg);
 int mem_event_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
                      XEN_GUEST_HANDLE(void) u_domctl);
xen/include/asm-x86/mem_paging.h +1 -2
diff -r abfc492fd04d -r 8e64acc49aa8 xen/include/asm-x86/mem_paging.h
@@ -21,8 +21,7 @@
  */
 
 
-int mem_paging_domctl(struct domain *d, xen_domctl_mem_event_op_t *mec,
-                      XEN_GUEST_HANDLE(void) u_domctl);
+int mem_paging_memop(struct domain *d, xen_mem_event_op_t *meo);
 
 
 /*
xen/include/asm-x86/mem_sharing.h +3 -0
diff -r abfc492fd04d -r 8e64acc49aa8 xen/include/asm-x86/mem_sharing.h
@@ -23,6 +23,7 @@
 #define __MEM_SHARING_H__
 
 #include <public/domctl.h>
+#include <public/memory.h>
 
 /* Auditing of memory sharing code? */
 #define MEM_SHARING_AUDIT 0

@@ -56,6 +57,8 @@ int mem_sharing_unshare_page(struct doma
                              unsigned long gfn, 
                              uint16_t flags);
 int mem_sharing_sharing_resume(struct domain *d);
+int mem_sharing_memop(struct domain *d, 
+                       xen_mem_sharing_op_t *mec);
 int mem_sharing_domctl(struct domain *d, 
                        xen_domctl_mem_sharing_op_t *mec);
 void mem_sharing_init(void);
xen/include/public/domctl.h +19 -71

129 lines not on this page · raw message

xen/include/public/memory.h +87 -0

99 lines not on this page · raw message

#2 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 4 lines · Andres Lagar-Cavilla
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in order to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Or does the mem sharing code make it's own arrangements for these sorts of things somewhere?

Ian.

Quoted 3 lines
tools/libxc/xc_mem_event.c +3 -0
+
+    return do_memory_op(xch, mode, &meo, sizeof(meo));
+}
author
#3 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 21 lines · Andres Lagar-Cavilla
Quoted 4 lines
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in order to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

Quoted 7 lines

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Or does the mem sharing code make it's own arrangements for these sorts of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a separate patch.

There is a far more insidious problem that I'm not sure how to solve. Say the pager dies unexpectedly. The page hosting the ring is returned to dom0's free page pool, and then re-assigned to some other process. Xen never gets to find out. At best, the domain crashes before an event is generated. Perhaps, Xen posts at least one event. Worst-case scenario, the guest generates a spate of events. Cases 2 & 3 corrupt the memory of some other dom0 process.

I'm glad I got that off my chest :) While being a very rare race condition (haven't actually experienced it), I am quite unsure of how to tackle it cleanly.

Andres

Quoted 10 lines

Ian.

Quoted 3 lines
tools/libxc/xc_mem_event.c +3 -0
+
+    return do_memory_op(xch, mode, &meo, sizeof(meo));
+}
#4 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 26 lines · Andres Lagar-Cavilla
Quoted 26 lines · Andres Lagar-Cavilla
Quoted 4 lines
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in order to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Quoted 7 lines
Quoted 2 lines

Or does the mem sharing code make it's own arrangements for these sorts of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a separate patch.

If you are reworking this It Would Be Nice If (tm) you could make it use xc_hypercall_buffer_alloc and friends to allocate and manage the buffer while you are there e.g. the void *buffer above would become struct xc_hypercall_buffer *buffer -- see xc_perfc_query for example.

It happens that mlock has roughly the semantics we need (and so it mostly works) but it is not guaranteed by the specs. At some point we may wish to do something more robust and using the common interface will mean that the sharing code will benefit from that too.

Quoted 7 lines

There is a far more insidious problem that I'm not sure how to solve. Say the pager dies unexpectedly. The page hosting the ring is returned to dom0's free page pool, and then re-assigned to some other process. Xen never gets to find out. At best, the domain crashes before an event is generated. Perhaps, Xen posts at least one event. Worst-case scenario, the guest generates a spate of events. Cases 2 & 3 corrupt the memory of some other dom0 process.

How does the memory actually get mapped? Or is it actually memory belonging to the user process?

The gntalloc driver presumably has a similar sort of problem, might be interesting to take inspiration from there?

There's the possibility of having a paging specific driver which takes care of this and all the userspace does is mmap /dev/xen/pager or whatever.

Quoted 17 lines

I'm glad I got that off my chest :) While being a very rare race condition (haven't actually experienced it), I am quite unsure of how to tackle it cleanly.

Andres

Quoted 10 lines

Ian.

Quoted 3 lines
tools/libxc/xc_mem_event.c +3 -0
+
+    return do_memory_op(xch, mode, &meo, sizeof(meo));
+}
author
#5 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 36 lines · Andres Lagar-Cavilla
Quoted 26 lines
Quoted 26 lines · Andres Lagar-Cavilla
Quoted 4 lines
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in order to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Quoted 8 lines
Quoted 1 line

Or does the mem sharing code make it's own arrangements for these

sorts

Quoted 1 line

of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a separate patch.

Quoted 4 lines

If you are reworking this It Would Be Nice If (tm) you could make it use xc_hypercall_buffer_alloc and friends to allocate and manage the buffer while you are there e.g. the void *buffer above would become struct xc_hypercall_buffer *buffer -- see xc_perfc_query for example.

Took me a while to grok those hypercall buffer macros. Conclusion: that would need to change callers of paging_load (xenpaging in tree).

Can we keep this separate? domctl interface also suffers from all these problems. domctl->memops switch is orthogonal to the buffer badness. I want to tackle one problem at a time.

Quoted 17 lines

It happens that mlock has roughly the semantics we need (and so it mostly works) but it is not guaranteed by the specs. At some point we may wish to do something more robust and using the common interface will mean that the sharing code will benefit from that too.

Quoted 10 lines

There is a far more insidious problem that I'm not sure how to solve. Say
the pager dies unexpectedly. The page hosting the ring is returned to dom0's free page pool, and then re-assigned to some other process. Xen never gets to find out. At best, the domain crashes before an event is generated. Perhaps, Xen posts at least one event. Worst-case scenario, the
guest generates a spate of events. Cases 2 & 3 corrupt the memory of some
other dom0 process.

Quoted 2 lines

How does the memory actually get mapped? Or is it actually memory belonging to the user process?

The user process memory is mapped by Xen: user virtual address -> mfn -> map_domain_page. Ugly.

Agree with you that the best way to solve it is with a dom0 kernel driver.

Thanks!
Andres

Quoted 29 lines

The gntalloc driver presumably has a similar sort of problem, might be interesting to take inspiration from there?

There's the possibility of having a paging specific driver which takes care of this and all the userspace does is mmap /dev/xen/pager or whatever.

Quoted 18 lines

I'm glad I got that off my chest :) While being a very rare race condition
(haven't actually experienced it), I am quite unsure of how to tackle it cleanly.

Andres

Quoted 10 lines

Ian.

Quoted 3 lines
tools/libxc/xc_mem_event.c +3 -0
+
+    return do_memory_op(xch, mode, &meo, sizeof(meo));
+}
#6 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 36 lines · Andres Lagar-Cavilla
Quoted 36 lines · Andres Lagar-Cavilla
Quoted 34 lines
Quoted 21 lines · Andres Lagar-Cavilla
Quoted 4 lines
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in order to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

Quoted 6 lines

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Or does the mem sharing code make it's own arrangements for these

sorts

Quoted 1 line

of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a separate patch.

Quoted 7 lines
Quoted 4 lines

If you are reworking this It Would Be Nice If (tm) you could make it use xc_hypercall_buffer_alloc and friends to allocate and manage the buffer while you are there e.g. the void *buffer above would become struct xc_hypercall_buffer *buffer -- see xc_perfc_query for example.

Took me a while to grok those hypercall buffer macros. Conclusion: that would need to change callers of paging_load (xenpaging in tree).

Yes, in this case you probably want to bubble their use right up to the top level rather than bouncing in the internals like we do for smaller and.or more transient data.

Quoted 3 lines

Can we keep this separate? domctl interface also suffers from all these problems. domctl->memops switch is orthogonal to the buffer badness. I want to tackle one problem at a time.

Sure, read "IWBNI" as "in the fullness of time" ;-)

Ian.

author
#7 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 53 lines · Andres Lagar-Cavilla
Quoted 39 lines
Quoted 18 lines · Andres Lagar-Cavilla
Quoted 17 lines
Quoted 17 lines · Andres Lagar-Cavilla
Quoted 4 lines
tools/libxc/xc_mem_event.c +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

tools/libxc/xc_mem_event.c int xc_mem_event_control(xc_interface *x · line 53

Quoted 10 lines
tools/libxc/xc_mem_event.c +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in

order

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Quoted 7 lines
Quoted 7 lines
Quoted 1 line

Or does the mem sharing code make it's own arrangements for these

sorts

Quoted 1 line

of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a

separate

Quoted 3 lines
Quoted 1 line

patch.

If you are reworking this It Would Be Nice If (tm) you could make it

Quoted 8 lines

use

Quoted 1 line

xc_hypercall_buffer_alloc and friends to allocate and manage the

buffer

Quoted 2 lines

while you are there e.g. the void *buffer above would become struct xc_hypercall_buffer *buffer -- see xc_perfc_query for example.

Took me a while to grok those hypercall buffer macros. Conclusion: that would need to change callers of paging_load (xenpaging in tree).

Yes, in this case you probably want to bubble their use right up to the top level rather than bouncing in the internals like we do for smaller and.or more transient data.

Quoted 5 lines
Quoted 3 lines

Can we keep this separate? domctl interface also suffers from all these problems. domctl->memops switch is orthogonal to the buffer badness. I want to tackle one problem at a time.

Sure, read "IWBNI" as "in the fullness of time" ;-)

Ok, trying to mb() what's needed now and what will take place in the fullness of time:
- ring creation when initializing mem event is ugly, but cannot be fixed short of a kernel driver. I will however produce a patch now that at least mlock's these guys.
- All libxc consumers of these interfaces should be updated to init these magic pages (and the paging load buffer) using xc_hypercall_buffer_alloc in their own code.

Cheers,
Andres

Quoted 5 lines

Ian.

#8 lore raw

Re: [PATCH 1 of 2] Use memops for mem paging, sharing, and access, instead of domctls

Quoted 57 lines · Andres Lagar-Cavilla
Quoted 57 lines · Andres Lagar-Cavilla
Quoted 39 lines
Quoted 18 lines · Andres Lagar-Cavilla
Quoted 17 lines
Quoted 17 lines · Andres Lagar-Cavilla
Quoted 4 lines
(quoted diff) +2 -0
+int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
+                        unsigned int op, unsigned int mode,

Review

Quoted 10 lines
(quoted diff) +10 -0
+                        uint64_t gfn, void *buffer)
+{
+    xen_mem_event_op_t meo;
+
+    memset(&meo, 0, sizeof(meo));
+
+    meo.op      = op;
+    meo.domain  = domain_id;
+    meo.gfn     = gfn;
+    meo.buffer  = (unsigned long) buffer;

Hypercall arguments need to use the xc_hypercall_buffer stuff in

order

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

to ensure that the memory is safe to access from a hypercall (in particular that it is mlocked or similar)-- I don't see where that happens for this buffer.

meo itself is bounced by do_memory_op so that is OK.

I was also a little suspicious of ring_addr and shared_addr in xc_mem_event_control in this regard.

Quoted 7 lines
Quoted 7 lines
Quoted 1 line

Or does the mem sharing code make it's own arrangements for these

sorts

Quoted 1 line

of things somewhere?

It's rather unclean. They do not get mlock'ed (and there is no sanity check to ensure they're page-aligned). They should follow the pattern established in xc_mem_paging_load. I'll get on it, probably a

separate

Quoted 3 lines
Quoted 1 line

patch.

If you are reworking this It Would Be Nice If (tm) you could make it

Quoted 8 lines

use

Quoted 1 line

xc_hypercall_buffer_alloc and friends to allocate and manage the

buffer

Quoted 2 lines

while you are there e.g. the void *buffer above would become struct xc_hypercall_buffer *buffer -- see xc_perfc_query for example.

Took me a while to grok those hypercall buffer macros. Conclusion: that would need to change callers of paging_load (xenpaging in tree).

Yes, in this case you probably want to bubble their use right up to the top level rather than bouncing in the internals like we do for smaller and.or more transient data.

Quoted 3 lines

Can we keep this separate? domctl interface also suffers from all these problems. domctl->memops switch is orthogonal to the buffer badness. I want to tackle one problem at a time.

Quoted 7 lines
Quoted 1 line

Sure, read "IWBNI" as "in the fullness of time" ;-)

Ok, trying to mb() what's needed now and what will take place in the fullness of time:
- ring creation when initializing mem event is ugly, but cannot be fixed short of a kernel driver. I will however produce a patch now that at least mlock's these guys.

Sounds good.

mb() goes here ;-)

Quoted 3 lines

- All libxc consumers of these interfaces should be updated to init these magic pages (and the paging load buffer) using xc_hypercall_buffer_alloc in their own code.

Correct, this can be done whenever though.

Ian.

author
#9 lore raw

PATCH 1/1 x86/mm: New sharing audit memop

In reply to Andres Lagar-Cavilla
 tools/libxc/xc_memshr.c              |  11 +++++++++++
 tools/libxc/xenctrl.h                |   1 +
 tools/tests/mem-sharing/memshrtool.c |  11 +++++++++++
 xen/arch/x86/mm/mem_sharing.c        |  13 ++++++-------
 xen/arch/x86/x86_64/compat/mm.c      |   3 +++
 xen/arch/x86/x86_64/mm.c             |   2 ++
 xen/include/asm-x86/mem_sharing.h    |   3 ++-
 xen/include/public/memory.h          |   1 +
 8 files changed, 37 insertions(+), 8 deletions(-)

Remove costly mem_sharting audits from the inline path, and instead make them callable as a memop.

Have the audit function return the number of errors detected.

Update memshrtool to be able to trigger audits.

Set sharing audits as enabled by default.

Signed-off-by Andres Lagar-Cavilla <[email protected]>
Signed-off-by Adin Scannell <[email protected]>
tools/libxc/xc_memshr.c +11 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 tools/libxc/xc_memshr.c
@@ -211,6 +211,17 @@ int xc_memshr_debug_gref(xc_interface *x
     return xc_memshr_memop(xch, domid, &mso);
 }
 
+int xc_memshr_audit(xc_interface *xch)
+{
+    xen_mem_sharing_op_t mso;
+
+    memset(&mso, 0, sizeof(mso));
+
+    mso.op = XENMEM_sharing_op_audit;
+
+    return do_memory_op(xch, XENMEM_sharing_op, &mso, sizeof(mso));
+}
+
 long xc_sharing_freed_pages(xc_interface *xch)
 {
     return do_memory_op(xch, XENMEM_get_sharing_freed_pages, NULL, 0);
tools/libxc/xenctrl.h +1 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 tools/libxc/xenctrl.h
@@ -1947,6 +1947,7 @@ int xc_memshr_debug_mfn(xc_interface *xc
 int xc_memshr_debug_gref(xc_interface *xch,
                          domid_t domid,
                          grant_ref_t gref);
+int xc_memshr_audit(xc_interface *xch);
 
 int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size);
 int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid);
tools/tests/mem-sharing/memshrtool.c +11 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 tools/tests/mem-sharing/memshrtool.c
@@ -27,6 +27,7 @@ static int usage(const char* prog)
     printf("  add-to-physmap <domid> <gfn> <source> <source-gfn> <source-handle>\n");
     printf("                          - Populate a page in a domain with a shared page.\n");
     printf("  debug-gfn <domid> <gfn> - Debug a particular domain and gfn.\n");
+    printf("  audit                   - Audit the sharing subsytem in Xen.\n");
     return 1;
 }
 

@@ -160,6 +161,16 @@ int main(int argc, const char** argv)
         gfn = strtol(argv[3], NULL, 0);
         R(xc_memshr_debug_gfn(xch, domid, gfn));
     }
+    else if( !strcasecmp(cmd, "audit") )
+    {
+        int rc = xc_memshr_audit(xch);
+        if ( rc < 0 )
+        {
+            printf("error executing xc_memshr_audit: %s\n", strerror(errno));
+            return rc;
+        }
+        printf("Audit returned %d errors.\n", rc);
+    }
 
     return 0;
 }
xen/arch/x86/mm/mem_sharing.c +6 -7
diff -r 8e64acc49aa8 -r f0aec2898b83 xen/arch/x86/mm/mem_sharing.c
@@ -50,8 +50,6 @@ DEFINE_PER_CPU(pg_lock_data_t, __pld);
 
 #if MEM_SHARING_AUDIT
 
-static void mem_sharing_audit(void);
-
 static struct list_head shr_audit_list;
 static spinlock_t shr_audit_lock;
 DEFINE_RCU_READ_LOCK(shr_audit_read_lock);

@@ -81,7 +79,10 @@ static inline void audit_del_list(struct
 
 #else
 
-#define mem_sharing_audit() ((void)0)
+int mem_sharing_audit(void)
+{
+    return -ENOSYS;
+}
 
 #define audit_add_list(p)  ((void)0)
 static inline void audit_del_list(struct page_info *page)

@@ -212,7 +213,7 @@ static struct page_info* mem_sharing_loo
 }
 
 #if MEM_SHARING_AUDIT
-static void mem_sharing_audit(void)
+int mem_sharing_audit(void)
 {
     int errors = 0;
     unsigned long count_expected;

@@ -338,6 +339,7 @@ static void mem_sharing_audit(void)
         errors++;
     }
 
+    return errors;
 }
 #endif
 

@@ -918,7 +920,6 @@ int mem_sharing_unshare_page(struct doma
     gfn_info_t *gfn_info = NULL;
     struct list_head *le;
    
-    mem_sharing_audit();
     mfn = get_gfn(d, gfn, &p2mt);
     
     /* Has someone already unshared it? */

@@ -1182,8 +1183,6 @@ int mem_sharing_memop(struct domain *d, 
             break;
     }
 
-    mem_sharing_audit();
-
     return rc;
 }
xen/arch/x86/x86_64/compat/mm.c +3 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 xen/arch/x86/x86_64/compat/mm.c
@@ -3,6 +3,7 @@
 #include <compat/memory.h>
 #include <compat/xen.h>
 #include <asm/mem_event.h>
+#include <asm/mem_sharing.h>
 
 int compat_set_gdt(XEN_GUEST_HANDLE(uint) frame_list, unsigned int entries)
 {

@@ -228,6 +229,8 @@ int compat_arch_memory_op(int op, XEN_GU
         xen_mem_sharing_op_t mso;
         if ( copy_from_guest(&mso, arg, 1) )
             return -EFAULT;
+        if ( mso.op == XENMEM_sharing_op_audit )
+            return mem_sharing_audit(); 
         rc = do_mem_event_op(op, mso.domain, (void *) &mso);
         if ( !rc && copy_to_guest(arg, &mso, 1) )
             return -EFAULT;
xen/arch/x86/x86_64/mm.c +2 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 xen/arch/x86/x86_64/mm.c
@@ -1117,6 +1117,8 @@ long subarch_memory_op(int op, XEN_GUEST
         xen_mem_sharing_op_t mso;
         if ( copy_from_guest(&mso, arg, 1) )
             return -EFAULT;
+        if ( mso.op == XENMEM_sharing_op_audit )
+            return mem_sharing_audit(); 
         rc = do_mem_event_op(op, mso.domain, (void *) &mso);
         if ( !rc && copy_to_guest(arg, &mso, 1) )
             return -EFAULT;
xen/include/asm-x86/mem_sharing.h +2 -1
diff -r 8e64acc49aa8 -r f0aec2898b83 xen/include/asm-x86/mem_sharing.h
@@ -26,7 +26,7 @@
 #include <public/memory.h>
 
 /* Auditing of memory sharing code? */
-#define MEM_SHARING_AUDIT 0
+#define MEM_SHARING_AUDIT 1
 
 typedef uint64_t shr_handle_t; 
 

@@ -61,6 +61,7 @@ int mem_sharing_memop(struct domain *d,
                        xen_mem_sharing_op_t *mec);
 int mem_sharing_domctl(struct domain *d, 
                        xen_domctl_mem_sharing_op_t *mec);
+int mem_sharing_audit(void);
 void mem_sharing_init(void);
 
 #else
xen/include/public/memory.h +1 -0
diff -r 8e64acc49aa8 -r f0aec2898b83 xen/include/public/memory.h
@@ -333,6 +333,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_event_op
 #define XENMEM_sharing_op_debug_mfn         5
 #define XENMEM_sharing_op_debug_gref        6
 #define XENMEM_sharing_op_add_physmap       7
+#define XENMEM_sharing_op_audit             8
 
 #define XENMEM_SHARING_OP_S_HANDLE_INVALID  (-10)
 #define XENMEM_SHARING_OP_C_HANDLE_INVALID  (-9)
Acked-by on v1 1/1
#10 lore raw
In reply to Andres Lagar-Cavilla
Quoted 15 lines · Andres Lagar-Cavilla

Per page operations in the paging, sharing, and access tracking subsystems are all implemented with domctls (e.g. a domctl to evict one page, or to share one page).

Under heavy load, the domctl path reveals a lack of scalability. The domctl lock serializes dom0's vcpus in the hypervisor. When performing thousands of per-page operations on dozens of domains, these vcpus will spin in the hypervisor. Beyond the aggressive locking, an added inefficiency of blocking vcpus in the domctl lock is that dom0 is prevented from re-scheduling any of its other work-starved processes.

We retain the domctl interface for setting up and tearing down paging/sharing/mem access for a domain. But we migrate all the per page operations to use the memory_op hypercalls (e.g XENMEM_*).

Quoted 7 lines

A new tools-only section is added to the public memory.h header.

This is a backwards-incompatible ABI change. It's been floating on the list for a couple weeks now, with no nacks thus far.

Signed-off-by Andres Lagar-Cavilla <andres@lagarcavilla>
Signed-off-by Adin Scannell <[email protected]>

For the hypervisor parts,

Acked-by Tim Deegan <[email protected]>

so this can all go in once the tools parts have been reviewed.

Tim.