[PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Tamas K Lengyel]

Add option to the fork memop to enforce a start with an empty p2m.
Pre-populating special pages to the fork tend to be necessary only when setting
up forks to be fully functional with a toolstack or if the fork makes use of
them in some way. For short-lived forks these pages are optional and starting
with an empty p2m has advantages both in terms of reset performance as well as
easier reasoning about the state of the fork after creation.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
v2: rename flag to empty_p2m, add assert at the end and move
     vAPIC page mapping skipping logic into where its mapped
---
 tools/include/xenctrl.h               |  3 ++-
 tools/libs/ctrl/xc_memshr.c           |  5 +++-
 xen/arch/x86/hvm/vmx/vmx.c            |  5 ++++
 xen/arch/x86/include/asm/hvm/domain.h |  4 +++-
 xen/arch/x86/mm/mem_sharing.c         | 33 +++++++++++++++++----------
 xen/include/public/memory.h           |  4 ++--
 6 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 95bd5eca67..26766ec19f 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2281,7 +2281,8 @@ int xc_memshr_fork(xc_interface *xch,
                    uint32_t source_domain,
                    uint32_t client_domain,
                    bool allow_with_iommu,
-                   bool block_interrupts);
+                   bool block_interrupts,
+                   bool empty_p2m);
 
 /*
  * Note: this function is only intended to be used on short-lived forks that
diff --git a/tools/libs/ctrl/xc_memshr.c b/tools/libs/ctrl/xc_memshr.c
index a6cfd7dccf..0143f9ddea 100644
--- a/tools/libs/ctrl/xc_memshr.c
+++ b/tools/libs/ctrl/xc_memshr.c
@@ -240,7 +240,8 @@ int xc_memshr_debug_gref(xc_interface *xch,
 }
 
 int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
-                   bool allow_with_iommu, bool block_interrupts)
+                   bool allow_with_iommu, bool block_interrupts,
+                   bool empty_p2m)
 {
     xen_mem_sharing_op_t mso;
 
@@ -253,6 +254,8 @@ int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
         mso.u.fork.flags |= XENMEM_FORK_WITH_IOMMU_ALLOWED;
     if ( block_interrupts )
         mso.u.fork.flags |= XENMEM_FORK_BLOCK_INTERRUPTS;
+    if ( empty_p2m )
+        mso.u.fork.flags |= XENMEM_FORK_EMPTY_P2M;
 
     return xc_memshr_memop(xch, domid, &mso);
 }
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c075370f64..5e60c92d5c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -424,6 +424,11 @@ static void cf_check domain_creation_finished(struct domain *d)
     if ( !has_vlapic(d) || mfn_eq(apic_access_mfn, INVALID_MFN) )
         return;
 
+#ifdef CONFIG_MEM_SHARING
+    if ( d->arch.hvm.mem_sharing.empty_p2m )
+        return;
+#endif
+
     ASSERT(epte_get_entry_emt(d, gfn, apic_access_mfn, 0, &ipat,
                               p2m_mmio_direct) == MTRR_TYPE_WRBACK);
     ASSERT(ipat);
diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index 698455444e..22a17c36c5 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -31,7 +31,9 @@
 #ifdef CONFIG_MEM_SHARING
 struct mem_sharing_domain
 {
-    bool enabled, block_interrupts;
+    bool enabled;
+    bool block_interrupts;
+    bool empty_p2m;
 
     /*
      * When releasing shared gfn's in a preemptible manner, recall where
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 15e6a7ed81..ef67285a98 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1643,7 +1643,8 @@ static int bring_up_vcpus(struct domain *cd, struct domain *d)
     return 0;
 }
 
-static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
+static int copy_vcpu_settings(struct domain *cd, const struct domain *d,
+                              bool empty_p2m)
 {
     unsigned int i;
     struct p2m_domain *p2m = p2m_get_hostp2m(cd);
@@ -1660,7 +1661,7 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
 
         /* Copy & map in the vcpu_info page if the guest uses one */
         vcpu_info_mfn = d_vcpu->vcpu_info_mfn;
-        if ( !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
+        if ( !empty_p2m && !mfn_eq(vcpu_info_mfn, INVALID_MFN) )
         {
             mfn_t new_vcpu_info_mfn = cd_vcpu->vcpu_info_mfn;
 
@@ -1807,17 +1808,18 @@ static int copy_special_pages(struct domain *cd, struct domain *d)
     return 0;
 }
 
-static int copy_settings(struct domain *cd, struct domain *d)
+static int copy_settings(struct domain *cd, struct domain *d,
+                         bool empty_p2m)
 {
     int rc;
 
-    if ( (rc = copy_vcpu_settings(cd, d)) )
+    if ( (rc = copy_vcpu_settings(cd, d, empty_p2m)) )
         return rc;
 
     if ( (rc = hvm_copy_context_and_params(cd, d)) )
         return rc;
 
-    if ( (rc = copy_special_pages(cd, d)) )
+    if ( !empty_p2m && (rc = copy_special_pages(cd, d)) )
         return rc;
 
     copy_tsc(cd, d);
@@ -1826,9 +1828,11 @@ static int copy_settings(struct domain *cd, struct domain *d)
     return rc;
 }
 
-static int fork(struct domain *cd, struct domain *d)
+static int fork(struct domain *cd, struct domain *d, uint16_t flags)
 {
     int rc = -EBUSY;
+    bool block_interrupts = flags & XENMEM_FORK_BLOCK_INTERRUPTS;
+    bool empty_p2m = flags & XENMEM_FORK_EMPTY_P2M;
 
     if ( !cd->controller_pause_count )
         return rc;
@@ -1856,7 +1860,13 @@ static int fork(struct domain *cd, struct domain *d)
     if ( (rc = bring_up_vcpus(cd, d)) )
         goto done;
 
-    rc = copy_settings(cd, d);
+    if ( !(rc = copy_settings(cd, d, empty_p2m)) )
+    {
+        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
+
+        if ( (cd->arch.hvm.mem_sharing.empty_p2m = empty_p2m) )
+            ASSERT(page_list_empty(&cd->page_list));
+    }
 
  done:
     if ( rc && rc != -ERESTART )
@@ -1920,7 +1930,7 @@ static int mem_sharing_fork_reset(struct domain *d)
     }
     spin_unlock_recursive(&d->page_alloc_lock);
 
-    rc = copy_settings(d, pd);
+    rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.empty_p2m);
 
     domain_unpause(d);
 
@@ -2190,7 +2200,8 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
         if ( mso.u.fork.pad )
             goto out;
         if ( mso.u.fork.flags &
-             ~(XENMEM_FORK_WITH_IOMMU_ALLOWED | XENMEM_FORK_BLOCK_INTERRUPTS) )
+             ~(XENMEM_FORK_WITH_IOMMU_ALLOWED | XENMEM_FORK_BLOCK_INTERRUPTS |
+               XENMEM_FORK_EMPTY_P2M) )
             goto out;
 
         rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
@@ -2212,14 +2223,12 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
             goto out;
         }
 
-        rc = fork(d, pd);
+        rc = fork(d, pd, mso.u.fork.flags);
 
         if ( rc == -ERESTART )
             rc = hypercall_create_continuation(__HYPERVISOR_memory_op,
                                                "lh", XENMEM_sharing_op,
                                                arg);
-        else if ( !rc && (mso.u.fork.flags & XENMEM_FORK_BLOCK_INTERRUPTS) )
-            d->arch.hvm.mem_sharing.block_interrupts = true;
 
         rcu_unlock_domain(pd);
         break;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index a1a0f0233a..d44c256b3c 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -543,10 +543,10 @@ struct xen_mem_sharing_op {
         } debug;
         struct mem_sharing_op_fork {      /* OP_FORK */
             domid_t parent_domain;        /* IN: parent's domain id */
-/* Only makes sense for short-lived forks */
+/* These flags only makes sense for short-lived forks */
 #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
-/* Only makes sense for short-lived forks */
 #define XENMEM_FORK_BLOCK_INTERRUPTS   (1u << 1)
+#define XENMEM_FORK_EMPTY_P2M          (1u << 2)
             uint16_t flags;               /* IN: optional settings */
             uint32_t pad;                 /* Must be set to 0 */
         } fork;
-- 
2.25.1

[PATCH v2 2/3] x86/mem_sharing: add fork_complete field to mem_sharing_domain

[Tamas K Lengyel]

The fork's physmap should only be populated with select special pages during
the setup of the fork. The rest of the fork's physmap should only be populated
as needed after the fork is complete. Add a field to specify when the fork is
complete so fork_page() can determine whether it's time to start adding entries
to the physmap.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
v2: replace previous patch that set parent to dom_cow as a placeholder
---
 xen/arch/x86/include/asm/hvm/domain.h | 1 +
 xen/arch/x86/mm/mem_sharing.c         | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/include/asm/hvm/domain.h b/xen/arch/x86/include/asm/hvm/domain.h
index 22a17c36c5..7078d041bd 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -32,6 +32,7 @@
 struct mem_sharing_domain
 {
     bool enabled;
+    bool fork_complete;
     bool block_interrupts;
     bool empty_p2m;
 
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index ef67285a98..bfde342a38 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1555,7 +1555,7 @@ int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool unsharing)
     p2m_type_t p2mt;
     struct page_info *page;
 
-    if ( !mem_sharing_is_fork(d) )
+    if ( !d->arch.hvm.mem_sharing.fork_complete )
         return -ENOENT;
 
     if ( !unsharing )
@@ -1862,6 +1862,7 @@ static int fork(struct domain *cd, struct domain *d, uint16_t flags)
 
     if ( !(rc = copy_settings(cd, d, empty_p2m)) )
     {
+        cd->arch.hvm.mem_sharing.fork_complete = true;
         cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
 
         if ( (cd->arch.hvm.mem_sharing.empty_p2m = empty_p2m) )
-- 
2.25.1

[PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Tamas K Lengyel]

Allow specify distinct parts of the fork VM to be reset. This is useful when a
fuzzing operation involves mapping in only a handful of pages that are known
ahead of time. Throwing these pages away just to be re-copied immediately is
expensive, thus allowing to specify partial resets can speed things up.

Also allow resetting to be initiated from vm_event responses as an
optiomization.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
v2: address review comments and add more sanity checking
---
 tools/include/xenctrl.h                |  3 ++-
 tools/libs/ctrl/xc_memshr.c            |  7 ++++++-
 xen/arch/x86/include/asm/mem_sharing.h |  9 +++++++++
 xen/arch/x86/mm/mem_sharing.c          | 27 +++++++++++++++++++++-----
 xen/common/vm_event.c                  | 15 ++++++++++++++
 xen/include/public/memory.h            |  4 +++-
 xen/include/public/vm_event.h          |  8 ++++++++
 7 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 26766ec19f..8a5b125aae 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2291,7 +2291,8 @@ int xc_memshr_fork(xc_interface *xch,
  *
  * With VMs that have a lot of memory this call may block for a long time.
  */
-int xc_memshr_fork_reset(xc_interface *xch, uint32_t forked_domain);
+int xc_memshr_fork_reset(xc_interface *xch, uint32_t forked_domain,
+                         bool reset_state, bool reset_memory);
 
 /* Debug calls: return the number of pages referencing the shared frame backing
  * the input argument. Should be one or greater.
diff --git a/tools/libs/ctrl/xc_memshr.c b/tools/libs/ctrl/xc_memshr.c
index 0143f9ddea..5044d5b83e 100644
--- a/tools/libs/ctrl/xc_memshr.c
+++ b/tools/libs/ctrl/xc_memshr.c
@@ -260,12 +260,17 @@ int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
     return xc_memshr_memop(xch, domid, &mso);
 }
 
-int xc_memshr_fork_reset(xc_interface *xch, uint32_t domid)
+int xc_memshr_fork_reset(xc_interface *xch, uint32_t domid, bool reset_state,
+                         bool reset_memory)
 {
     xen_mem_sharing_op_t mso;
 
     memset(&mso, 0, sizeof(mso));
     mso.op = XENMEM_sharing_op_fork_reset;
+    if ( reset_state )
+        mso.u.fork.flags |= XENMEM_FORK_RESET_STATE;
+    if ( reset_memory )
+        mso.u.fork.flags |= XENMEM_FORK_RESET_MEMORY;
 
     return xc_memshr_memop(xch, domid, &mso);
 }
diff --git a/xen/arch/x86/include/asm/mem_sharing.h b/xen/arch/x86/include/asm/mem_sharing.h
index cf7a12f4d2..2c00069bc9 100644
--- a/xen/arch/x86/include/asm/mem_sharing.h
+++ b/xen/arch/x86/include/asm/mem_sharing.h
@@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct domain *d)
 int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
                           bool unsharing);
 
+int mem_sharing_fork_reset(struct domain *d, bool reset_state,
+                           bool reset_memory);
+
 /*
  * If called by a foreign domain, possible errors are
  *   -EBUSY -> ring full
@@ -148,6 +151,12 @@ static inline int mem_sharing_fork_page(struct domain *d, gfn_t gfn, bool lock)
     return -EOPNOTSUPP;
 }
 
+static inline int mem_sharing_fork_reset(struct domain *d, bool reset_state,
+                                         bool reset_memory)
+{
+    return -EOPNOTSUPP;
+}
+
 #endif
 
 #endif /* __MEM_SHARING_H__ */
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index bfde342a38..11c74e3905 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1890,15 +1890,24 @@ static int fork(struct domain *cd, struct domain *d, uint16_t flags)
  * footprints the hypercall continuation should be implemented (or if this
  * feature needs to be become "stable").
  */
-static int mem_sharing_fork_reset(struct domain *d)
+int mem_sharing_fork_reset(struct domain *d, bool reset_state,
+                           bool reset_memory)
 {
-    int rc;
+    int rc = 0;
     struct domain *pd = d->parent;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     struct page_info *page, *tmp;
 
+    ASSERT(reset_state || reset_memory);
+
+    if ( !d->arch.hvm.mem_sharing.fork_complete )
+        return -ENOSYS;
+
     domain_pause(d);
 
+    if ( !reset_memory )
+        goto state;
+
     /* need recursive lock because we will free pages */
     spin_lock_recursive(&d->page_alloc_lock);
     page_list_for_each_safe(page, tmp, &d->page_list)
@@ -1931,7 +1940,9 @@ static int mem_sharing_fork_reset(struct domain *d)
     }
     spin_unlock_recursive(&d->page_alloc_lock);
 
-    rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.empty_p2m);
+ state:
+    if ( reset_state )
+        rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.empty_p2m);
 
     domain_unpause(d);
 
@@ -2237,15 +2248,21 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
 
     case XENMEM_sharing_op_fork_reset:
     {
+        bool reset_state = mso.u.fork.flags & XENMEM_FORK_RESET_STATE;
+        bool reset_memory = mso.u.fork.flags & XENMEM_FORK_RESET_MEMORY;
+
         rc = -EINVAL;
-        if ( mso.u.fork.pad || mso.u.fork.flags )
+        if ( mso.u.fork.pad || (!reset_state && !reset_memory) )
+            goto out;
+        if ( mso.u.fork.flags &
+             ~(XENMEM_FORK_RESET_STATE | XENMEM_FORK_RESET_MEMORY) )
             goto out;
 
         rc = -ENOSYS;
         if ( !d->parent )
             goto out;
 
-        rc = mem_sharing_fork_reset(d);
+        rc = mem_sharing_fork_reset(d, reset_state, reset_memory);
         break;
     }
 
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 84cf52636b..d26a6699fc 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -28,6 +28,11 @@
 #include <asm/p2m.h>
 #include <asm/monitor.h>
 #include <asm/vm_event.h>
+
+#ifdef CONFIG_MEM_SHARING
+#include <asm/mem_sharing.h>
+#endif
+
 #include <xsm/xsm.h>
 #include <public/hvm/params.h>
 
@@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct vm_event_domain *ved)
             if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
                 p2m_mem_paging_resume(d, &rsp);
 #endif
+#ifdef CONFIG_MEM_SHARING
+            if ( mem_sharing_is_fork(d) )
+            {
+                bool reset_state = rsp.flags & VM_EVENT_FLAG_RESET_FORK_STATE;
+                bool reset_mem = rsp.flags & VM_EVENT_FLAG_RESET_FORK_MEMORY;
+
+                if ( reset_state || reset_mem )
+                    ASSERT(!mem_sharing_fork_reset(d, reset_state, reset_mem));
+            }
+#endif
 
             /*
              * Check emulation flags in the arch-specific handler only, as it
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index d44c256b3c..2a4edfc84b 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
                 uint32_t gref;     /* IN: gref to debug         */
             } u;
         } debug;
-        struct mem_sharing_op_fork {      /* OP_FORK */
+        struct mem_sharing_op_fork {      /* OP_FORK/_RESET */
             domid_t parent_domain;        /* IN: parent's domain id */
 /* These flags only makes sense for short-lived forks */
 #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
 #define XENMEM_FORK_BLOCK_INTERRUPTS   (1u << 1)
 #define XENMEM_FORK_EMPTY_P2M          (1u << 2)
+#define XENMEM_FORK_RESET_STATE        (1u << 3)
+#define XENMEM_FORK_RESET_MEMORY       (1u << 4)
             uint16_t flags;               /* IN: optional settings */
             uint32_t pad;                 /* Must be set to 0 */
         } fork;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index bb003d21d0..81c2ee28cc 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -127,6 +127,14 @@
  * Reset the vmtrace buffer (if vmtrace is enabled)
  */
 #define VM_EVENT_FLAG_RESET_VMTRACE      (1 << 13)
+/*
+ * Reset the VM state (if VM is fork)
+ */
+#define VM_EVENT_FLAG_RESET_FORK_STATE   (1 << 14)
+/*
+ * Remove unshared entried from physmap (if VM is fork)
+ */
+#define VM_EVENT_FLAG_RESET_FORK_MEMORY  (1 << 15)
 
 /*
  * Reasons for the vm event request
-- 
2.25.1

Re: [PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Jan Beulich]

On 29.03.2022 16:03, Tamas K Lengyel wrote:
> Add option to the fork memop to enforce a start with an empty p2m.
> Pre-populating special pages to the fork tend to be necessary only when setting
> up forks to be fully functional with a toolstack or if the fork makes use of
> them in some way. For short-lived forks these pages are optional and starting
> with an empty p2m has advantages both in terms of reset performance as well as
> easier reasoning about the state of the fork after creation.

I'm afraid I don't consider this enough of an explanation: Why would these
page be optional? Where does the apriori knowledge come from that the guest
wouldn't manage to access the vCPU info pages or the APIC access one?

> --- a/xen/arch/x86/include/asm/hvm/domain.h
> +++ b/xen/arch/x86/include/asm/hvm/domain.h
> @@ -31,7 +31,9 @@
>  #ifdef CONFIG_MEM_SHARING
>  struct mem_sharing_domain
>  {
> -    bool enabled, block_interrupts;
> +    bool enabled;
> +    bool block_interrupts;
> +    bool empty_p2m;

While the name of the field is perhaps fine as is, it would be helpful to
have a comment here clarifying that this is only about the guest's initial
and reset state; this specifically does not indicate the p2m has to remain
empty (aiui).

> @@ -1856,7 +1860,13 @@ static int fork(struct domain *cd, struct domain *d)
>      if ( (rc = bring_up_vcpus(cd, d)) )
>          goto done;
>  
> -    rc = copy_settings(cd, d);
> +    if ( !(rc = copy_settings(cd, d, empty_p2m)) )
> +    {
> +        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> +
> +        if ( (cd->arch.hvm.mem_sharing.empty_p2m = empty_p2m) )

Is there a reason you don't do the assignment earlier, thus avoiding the
need to pass around the extra function argument?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -543,10 +543,10 @@ struct xen_mem_sharing_op {
>          } debug;
>          struct mem_sharing_op_fork {      /* OP_FORK */
>              domid_t parent_domain;        /* IN: parent's domain id */
> -/* Only makes sense for short-lived forks */
> +/* These flags only makes sense for short-lived forks */

Nit: s/makes/make/.

Jan

Re: [PATCH v2 2/3] x86/mem_sharing: add fork_complete field to mem_sharing_domain

[Jan Beulich]

On 29.03.2022 16:03, Tamas K Lengyel wrote:
> The fork's physmap should only be populated with select special pages during
> the setup of the fork. The rest of the fork's physmap should only be populated
> as needed after the fork is complete. Add a field to specify when the fork is
> complete so fork_page() can determine whether it's time to start adding entries
> to the physmap.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Tamas K Lengyel]

On Tue, Mar 29, 2022 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> > Add option to the fork memop to enforce a start with an empty p2m.
> > Pre-populating special pages to the fork tend to be necessary only when setting
> > up forks to be fully functional with a toolstack or if the fork makes use of
> > them in some way. For short-lived forks these pages are optional and starting
> > with an empty p2m has advantages both in terms of reset performance as well as
> > easier reasoning about the state of the fork after creation.
>
> I'm afraid I don't consider this enough of an explanation: Why would these
> page be optional? Where does the apriori knowledge come from that the guest
> wouldn't manage to access the vCPU info pages or the APIC access one?

By knowing what code you are fuzzing. The code you are fuzzing is
clearly marked by harnesses and that's the only code you execute while
fuzzing. If you know the code doesn't use them, no need to map them
in. They haven't been needed in any of the fuzzing setups we had so
far so I'm planning to be this the default when fuzzing.

> > --- a/xen/arch/x86/include/asm/hvm/domain.h
> > +++ b/xen/arch/x86/include/asm/hvm/domain.h
> > @@ -31,7 +31,9 @@
> >  #ifdef CONFIG_MEM_SHARING
> >  struct mem_sharing_domain
> >  {
> > -    bool enabled, block_interrupts;
> > +    bool enabled;
> > +    bool block_interrupts;
> > +    bool empty_p2m;
>
> While the name of the field is perhaps fine as is, it would be helpful to
> have a comment here clarifying that this is only about the guest's initial
> and reset state; this specifically does not indicate the p2m has to remain
> empty (aiui).

Sure.

>
> > @@ -1856,7 +1860,13 @@ static int fork(struct domain *cd, struct domain *d)
> >      if ( (rc = bring_up_vcpus(cd, d)) )
> >          goto done;
> >
> > -    rc = copy_settings(cd, d);
> > +    if ( !(rc = copy_settings(cd, d, empty_p2m)) )
> > +    {
> > +        cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> > +
> > +        if ( (cd->arch.hvm.mem_sharing.empty_p2m = empty_p2m) )
>
> Is there a reason you don't do the assignment earlier, thus avoiding the
> need to pass around the extra function argument?

Yes, I prefer only setting these values when the fork is complete, to
avoid having these be dangling in case the fork failed. It's
ultimately not a requirement since if the fork failed we just destroy
the domain that was destined to be the fork from the toolstack. If the
fork failed half-way through all bets are off anyway since we don't do
any "unfork" to roll back the changes that were already applied, so
having these also set early wouldn't make things worse then it already
is. But still, I prefer not adding more things that would need to be
cleaned up if I don't have to.

>
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -543,10 +543,10 @@ struct xen_mem_sharing_op {
> >          } debug;
> >          struct mem_sharing_op_fork {      /* OP_FORK */
> >              domid_t parent_domain;        /* IN: parent's domain id */
> > -/* Only makes sense for short-lived forks */
> > +/* These flags only makes sense for short-lived forks */
>
> Nit: s/makes/make/.

Ack.

Tamas

Re: [PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Jan Beulich]

On 29.03.2022 18:10, Tamas K Lengyel wrote:
> On Tue, Mar 29, 2022 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 29.03.2022 16:03, Tamas K Lengyel wrote:
>>> Add option to the fork memop to enforce a start with an empty p2m.
>>> Pre-populating special pages to the fork tend to be necessary only when setting
>>> up forks to be fully functional with a toolstack or if the fork makes use of
>>> them in some way. For short-lived forks these pages are optional and starting
>>> with an empty p2m has advantages both in terms of reset performance as well as
>>> easier reasoning about the state of the fork after creation.
>>
>> I'm afraid I don't consider this enough of an explanation: Why would these
>> page be optional? Where does the apriori knowledge come from that the guest
>> wouldn't manage to access the vCPU info pages or the APIC access one?
> 
> By knowing what code you are fuzzing. The code you are fuzzing is
> clearly marked by harnesses and that's the only code you execute while
> fuzzing. If you know the code doesn't use them, no need to map them
> in. They haven't been needed in any of the fuzzing setups we had so
> far so I'm planning to be this the default when fuzzing.

But isn't it the very nature of what you do fuzzing for that unexpected
code paths may be taken? By not having in place what is expected to be
there, yet more unexpected behavior might then result.

Plus - how do you bound how far the guest executes in a single attempt?

Jan

Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Jan Beulich]

On 29.03.2022 16:03, Tamas K Lengyel wrote:
> --- a/xen/arch/x86/include/asm/mem_sharing.h
> +++ b/xen/arch/x86/include/asm/mem_sharing.h
> @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct domain *d)
>  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
>                            bool unsharing);
>  
> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> +                           bool reset_memory);

Please avoid passing multiple booleans, even more so when you already
derive them from a single "flags" value. This would likely be easiest
if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
prove they're the same.

> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1890,15 +1890,24 @@ static int fork(struct domain *cd, struct domain *d, uint16_t flags)
>   * footprints the hypercall continuation should be implemented (or if this
>   * feature needs to be become "stable").
>   */
> -static int mem_sharing_fork_reset(struct domain *d)
> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> +                           bool reset_memory)
>  {
> -    int rc;
> +    int rc = 0;
>      struct domain *pd = d->parent;
>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>      struct page_info *page, *tmp;
>  
> +    ASSERT(reset_state || reset_memory);
> +
> +    if ( !d->arch.hvm.mem_sharing.fork_complete )
> +        return -ENOSYS;

Either EOPNOTSUPP (in case you think this operation could make sense
to implement for incomplete forks) or e.g. EINVAL, but please not
ENOSYS.

> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct vm_event_domain *ved)
>              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>                  p2m_mem_paging_resume(d, &rsp);
>  #endif
> +#ifdef CONFIG_MEM_SHARING
> +            if ( mem_sharing_is_fork(d) )
> +            {
> +                bool reset_state = rsp.flags & VM_EVENT_FLAG_RESET_FORK_STATE;
> +                bool reset_mem = rsp.flags & VM_EVENT_FLAG_RESET_FORK_MEMORY;
> +
> +                if ( reset_state || reset_mem )
> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state, reset_mem));
> +            }
> +#endif

Should the two flags be rejected in the "else" case, rather than
being silently ignored?

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
>                  uint32_t gref;     /* IN: gref to debug         */
>              } u;
>          } debug;
> -        struct mem_sharing_op_fork {      /* OP_FORK */
> +        struct mem_sharing_op_fork {      /* OP_FORK/_RESET */

I consider the notation in the comment misleading - I would read it to
mean OP_FORK and OP_RESET, supported by the earlier
OP_SHARE/ADD_PHYSMAP. Commonly we write OP_FORK{,_RESET} in such cases.

Jan

Re: [PATCH v2 1/3] x86/mem_sharing: option to enforce fork starting with empty p2m

[Tamas K Lengyel]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

On Wed, Mar 30, 2022, 2:47 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 29.03.2022 18:10, Tamas K Lengyel wrote:
> > On Tue, Mar 29, 2022 at 11:42 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> >>> Add option to the fork memop to enforce a start with an empty p2m.
> >>> Pre-populating special pages to the fork tend to be necessary only
> when setting
> >>> up forks to be fully functional with a toolstack or if the fork makes
> use of
> >>> them in some way. For short-lived forks these pages are optional and
> starting
> >>> with an empty p2m has advantages both in terms of reset performance as
> well as
> >>> easier reasoning about the state of the fork after creation.
> >>
> >> I'm afraid I don't consider this enough of an explanation: Why would
> these
> >> page be optional? Where does the apriori knowledge come from that the
> guest
> >> wouldn't manage to access the vCPU info pages or the APIC access one?
> >
> > By knowing what code you are fuzzing. The code you are fuzzing is
> > clearly marked by harnesses and that's the only code you execute while
> > fuzzing. If you know the code doesn't use them, no need to map them
> > in. They haven't been needed in any of the fuzzing setups we had so
> > far so I'm planning to be this the default when fuzzing.
>
> But isn't it the very nature of what you do fuzzing for that unexpected
> code paths may be taken? By not having in place what is expected to be
> there, yet more unexpected behavior might then result.
>

You don't get totally arbitrary execution, no. If you do then that means
having instability and non-reproducible runs which makes the fuzzing
inefficient. So if you know that the part of code has no reasonable path to
reach code using these pages then you can get rid of them. This is an
option for cases where you can make that call. That's all, just an option.


> Plus - how do you bound how far the guest executes in a single attempt?
>

We use a cpuid or breakpoint to signal where the code reached the end
point. The start point is where the parent got paused (also usually using a
magic cpuid).

Tamas

>

[-- Attachment #2: Type: text/html, Size: 3219 bytes --]

Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Tamas K Lengyel]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/include/asm/mem_sharing.h
> > +++ b/xen/arch/x86/include/asm/mem_sharing.h
> > @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct
> domain *d)
> >  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
> >                            bool unsharing);
> >
> > +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> > +                           bool reset_memory);
>
> Please avoid passing multiple booleans, even more so when you already
> derive them from a single "flags" value. This would likely be easiest
> if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
> XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
> prove they're the same.
>

I don't see why that would be an improvement in any way. I also don't want
to make VM_EVENT flags tied to the XENMEM ones as they are separate
interfaces. I rather just drop the changes to the XENMEM interface then do
that.


> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1890,15 +1890,24 @@ static int fork(struct domain *cd, struct domain
> *d, uint16_t flags)
> >   * footprints the hypercall continuation should be implemented (or if
> this
> >   * feature needs to be become "stable").
> >   */
> > -static int mem_sharing_fork_reset(struct domain *d)
> > +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> > +                           bool reset_memory)
> >  {
> > -    int rc;
> > +    int rc = 0;
> >      struct domain *pd = d->parent;
> >      struct p2m_domain *p2m = p2m_get_hostp2m(d);
> >      struct page_info *page, *tmp;
> >
> > +    ASSERT(reset_state || reset_memory);
> > +
> > +    if ( !d->arch.hvm.mem_sharing.fork_complete )
> > +        return -ENOSYS;
>
> Either EOPNOTSUPP (in case you think this operation could make sense
> to implement for incomplete forks) or e.g. EINVAL, but please not
> ENOSYS.
>
> > @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> >              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
> >                  p2m_mem_paging_resume(d, &rsp);
> >  #endif
> > +#ifdef CONFIG_MEM_SHARING
> > +            if ( mem_sharing_is_fork(d) )
> > +            {
> > +                bool reset_state = rsp.flags &
> VM_EVENT_FLAG_RESET_FORK_STATE;
> > +                bool reset_mem = rsp.flags &
> VM_EVENT_FLAG_RESET_FORK_MEMORY;
> > +
> > +                if ( reset_state || reset_mem )
> > +                    ASSERT(!mem_sharing_fork_reset(d, reset_state,
> reset_mem));
> > +            }
> > +#endif
>
> Should the two flags be rejected in the "else" case, rather than
> being silently ignored?
>

What do you mean by rejected? There is no feasible "rejection" that could
be done in this path other then skipping it.


> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> >                  uint32_t gref;     /* IN: gref to debug         */
> >              } u;
> >          } debug;
> > -        struct mem_sharing_op_fork {      /* OP_FORK */
> > +        struct mem_sharing_op_fork {      /* OP_FORK/_RESET */
>
> I consider the notation in the comment misleading - I would read it to
> mean OP_FORK and OP_RESET, supported by the earlier
> OP_SHARE/ADD_PHYSMAP. Commonly we write OP_FORK{,_RESET} in such cases.


Ack.

Tamas

>

[-- Attachment #2: Type: text/html, Size: 5138 bytes --]

Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Jan Beulich]

On 30.03.2022 15:19, Tamas K Lengyel wrote:
> On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 29.03.2022 16:03, Tamas K Lengyel wrote:
>>> --- a/xen/arch/x86/include/asm/mem_sharing.h
>>> +++ b/xen/arch/x86/include/asm/mem_sharing.h
>>> @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct
>> domain *d)
>>>  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
>>>                            bool unsharing);
>>>
>>> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
>>> +                           bool reset_memory);
>>
>> Please avoid passing multiple booleans, even more so when you already
>> derive them from a single "flags" value. This would likely be easiest
>> if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
>> XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
>> prove they're the same.
> 
> I don't see why that would be an improvement in any way. I also don't want
> to make VM_EVENT flags tied to the XENMEM ones as they are separate
> interfaces. I rather just drop the changes to the XENMEM interface then do
> that.

If the function gained two or three more flags, how would that look to
you? And how would you easily identify the correct order of all the
booleans?

>>> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d, struct
>> vm_event_domain *ved)
>>>              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
>>>                  p2m_mem_paging_resume(d, &rsp);
>>>  #endif
>>> +#ifdef CONFIG_MEM_SHARING
>>> +            if ( mem_sharing_is_fork(d) )
>>> +            {
>>> +                bool reset_state = rsp.flags &
>> VM_EVENT_FLAG_RESET_FORK_STATE;
>>> +                bool reset_mem = rsp.flags &
>> VM_EVENT_FLAG_RESET_FORK_MEMORY;
>>> +
>>> +                if ( reset_state || reset_mem )
>>> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state,
>> reset_mem));
>>> +            }
>>> +#endif
>>
>> Should the two flags be rejected in the "else" case, rather than
>> being silently ignored?
> 
> What do you mean by rejected? There is no feasible "rejection" that could
> be done in this path other then skipping it.

IOW no return value being handed back to the requester? The function
does have an error return path, so I don't see why you couldn't return
-EINVAL.

Jan

Re: [PATCH v2 3/3] x86/mem_sharing: make fork_reset more configurable

[Tamas K Lengyel]

[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]

On Wed, Mar 30, 2022, 9:34 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 30.03.2022 15:19, Tamas K Lengyel wrote:
> > On Wed, Mar 30, 2022, 6:31 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> >>> --- a/xen/arch/x86/include/asm/mem_sharing.h
> >>> +++ b/xen/arch/x86/include/asm/mem_sharing.h
> >>> @@ -85,6 +85,9 @@ static inline bool mem_sharing_is_fork(const struct
> >> domain *d)
> >>>  int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
> >>>                            bool unsharing);
> >>>
> >>> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> >>> +                           bool reset_memory);
> >>
> >> Please avoid passing multiple booleans, even more so when you already
> >> derive them from a single "flags" value. This would likely be easiest
> >> if you re-used the VM_EVENT_FLAG_RESET_FORK_* values also for
> >> XENMEM_FORK_RESET_*, with suitable BUILD_BUG_ON() put in place to
> >> prove they're the same.
> >
> > I don't see why that would be an improvement in any way. I also don't
> want
> > to make VM_EVENT flags tied to the XENMEM ones as they are separate
> > interfaces. I rather just drop the changes to the XENMEM interface then
> do
> > that.
>
> If the function gained two or three more flags, how would that look to
> you? And how would you easily identify the correct order of all the
> booleans?
>

IMHO we can cross that bridge when and if that becomes necessary. Also not
sure what you mean by the order of the booleans - that's irrelevant since
the booleans are named?


> >>> @@ -394,6 +399,16 @@ static int vm_event_resume(struct domain *d,
> struct
> >> vm_event_domain *ved)
> >>>              if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
> >>>                  p2m_mem_paging_resume(d, &rsp);
> >>>  #endif
> >>> +#ifdef CONFIG_MEM_SHARING
> >>> +            if ( mem_sharing_is_fork(d) )
> >>> +            {
> >>> +                bool reset_state = rsp.flags &
> >> VM_EVENT_FLAG_RESET_FORK_STATE;
> >>> +                bool reset_mem = rsp.flags &
> >> VM_EVENT_FLAG_RESET_FORK_MEMORY;
> >>> +
> >>> +                if ( reset_state || reset_mem )
> >>> +                    ASSERT(!mem_sharing_fork_reset(d, reset_state,
> >> reset_mem));
> >>> +            }
> >>> +#endif
> >>
> >> Should the two flags be rejected in the "else" case, rather than
> >> being silently ignored?
> >
> > What do you mean by rejected? There is no feasible "rejection" that could
> > be done in this path other then skipping it.
>
> IOW no return value being handed back to the requester? The function
> does have an error return path, so I don't see why you couldn't return
> -EINVAL.
>

The way vm_event response flags are right now is "best effort". Error code
from this path never reaches the caller, which are usually callback
functions that return these flags. The best way for an error code to reach
the caller would be by making a separate vm_event on the ring and sending
that, but that's non-existent today and also hasn't been needed. It
certainly isn't needed here since there should be no feasable way for a
fork to fail a reset request (hence the assert).

Tamas

>

[-- Attachment #2: Type: text/html, Size: 4702 bytes --]

Re: [PATCH v2 2/3] x86/mem_sharing: add fork_complete field to mem_sharing_domain

[Tamas K Lengyel]

On Tue, Mar 29, 2022 at 11:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.03.2022 16:03, Tamas K Lengyel wrote:
> > The fork's physmap should only be populated with select special pages during
> > the setup of the fork. The rest of the fork's physmap should only be populated
> > as needed after the fork is complete. Add a field to specify when the fork is
> > complete so fork_page() can determine whether it's time to start adding entries
> > to the physmap.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks! After further consideration I'll drop this and the empty_p2m
patch from the series as there is a corner-case with PAE-mode guests
which doesn't play nice with enforcing a start with an empty p2m.
Working around that issue leads to a cascading changeset which
ultimately isn't a worthwhile effort.

Tamas