[v6,06/10] x86/mem-sharing: copy GADDR based shared guest areas

Commit Message

Roger Pau Monné Oct. 4, 2023, 1:53 p.m. UTC

From: Jan Beulich <[e-mail]>

In preparation of the introduction of new vCPU operations allowing to
register the respective areas (one of the two is x86-specific) by
guest-physical address, add the necessary fork handling (with the
backing function yet to be filled in).

Signed-off-by: Jan Beulich <[e-mail]>
Signed-off-by: Roger Pau Monné <[e-mail]>
---
Changes since v5:
 - Convert -ERESTART to -EAGAIN.

Changes since v4:
 - Rely on map_guest_area() to populate the child p2m if necessary.
---
 xen/arch/x86/mm/mem_sharing.c | 36 +++++++++++++++++++++++++++++++++++
 xen/common/domain.c           |  7 +++++++
 2 files changed, 43 insertions(+)

Comments

Tamas K Lengyel Oct. 4, 2023, 9:36 p.m. UTC | #1

On Wed, Oct 4, 2023 at 9:54 AM Roger Pau Monne <[e-mail]> wrote:
>
> From: Jan Beulich <[e-mail]>
>
> In preparation of the introduction of new vCPU operations allowing to
> register the respective areas (one of the two is x86-specific) by
> guest-physical address, add the necessary fork handling (with the
> backing function yet to be filled in).
>
> Signed-off-by: Jan Beulich <[e-mail]>
> Signed-off-by: Roger Pau Monné <[e-mail]>

Acked-by: Tamas K Lengyel <[e-mail]>

Jan Beulich Oct. 16, 2023, 9:55 a.m. UTC | #2

On 04.10.2023 15:53, Roger Pau Monne wrote:
> @@ -1950,7 +1978,15 @@ int mem_sharing_fork_reset(struct domain *d, bool reset_state,
>  
>   state:
>      if ( reset_state )
> +    {
>          rc = copy_settings(d, pd);
> +        if ( rc == -ERESTART )
> +            /*
> +             * Translate to -EAGAIN, see TODO comment at top of function about
> +             * hypercall continuations.
> +             */
> +            rc = -EAGAIN;
> +    }

Are existing callers known to properly handle EAGAIN? I'm worried of the
verbosity that was no lost here.

Jan

Roger Pau Monné Oct. 16, 2023, 10:59 a.m. UTC | #3

On Mon, Oct 16, 2023 at 11:55:25AM +0200, Jan Beulich wrote:
> On 04.10.2023 15:53, Roger Pau Monne wrote:
> > @@ -1950,7 +1978,15 @@ int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> >  
> >   state:
> >      if ( reset_state )
> > +    {
> >          rc = copy_settings(d, pd);
> > +        if ( rc == -ERESTART )
> > +            /*
> > +             * Translate to -EAGAIN, see TODO comment at top of function about
> > +             * hypercall continuations.
> > +             */
> > +            rc = -EAGAIN;
> > +    }
> 
> Are existing callers known to properly handle EAGAIN? I'm worried of the
> verbosity that was no lost here.

No idea about the callers using XENMEM_sharing_op_fork_reset, but it
did seem the best option rather than leaking -ERESTART to callers.  We
have no callers of xc_memshr_fork_reset() in the tree.

vm_event_resume() will trigger an assert if mem_sharing_fork_reset()
fails with any error code, so doesn't make much difference there if
the return is -EAGAIN or -ERESTART.

My initial proposal had -EBUSY IIRC.

Thanks, Roger.

Patch

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 5f8f1fb4d871..445947b6a918 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1641,6 +1641,24 @@  static void copy_vcpu_nonreg_state(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
     hvm_set_nonreg_state(cd_vcpu, &nrs);
 }
 
+static int copy_guest_area(struct guest_area *cd_area,
+                           const struct guest_area *d_area,
+                           struct vcpu *cd_vcpu,
+                           const struct domain *d)
+{
+    unsigned int offset;
+
+    /* Check if no area to map, or already mapped. */
+    if ( !d_area->pg || cd_area->pg )
+        return 0;
+
+    offset = PAGE_OFFSET(d_area->map);
+    return map_guest_area(cd_vcpu, gfn_to_gaddr(
+                                       mfn_to_gfn(d, page_to_mfn(d_area->pg))) +
+                                   offset,
+                          PAGE_SIZE - offset, cd_area, NULL);
+}
+
 static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
 {
     struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
@@ -1709,6 +1727,16 @@  static int copy_vcpu_settings(struct domain *cd, const struct domain *d)
                 return ret;
         }
 
+        /* Same for the (physically registered) runstate and time info areas. */
+        ret = copy_guest_area(&cd_vcpu->runstate_guest_area,
+                              &d_vcpu->runstate_guest_area, cd_vcpu, d);
+        if ( ret )
+            return ret;
+        ret = copy_guest_area(&cd_vcpu->arch.time_guest_area,
+                              &d_vcpu->arch.time_guest_area, cd_vcpu, d);
+        if ( ret )
+            return ret;
+
         ret = copy_vpmu(d_vcpu, cd_vcpu);
         if ( ret )
             return ret;
@@ -1950,7 +1978,15 @@  int mem_sharing_fork_reset(struct domain *d, bool reset_state,
 
  state:
     if ( reset_state )
+    {
         rc = copy_settings(d, pd);
+        if ( rc == -ERESTART )
+            /*
+             * Translate to -EAGAIN, see TODO comment at top of function about
+             * hypercall continuations.
+             */
+            rc = -EAGAIN;
+    }
 
     domain_unpause(d);
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index d4958ec5e149..47fc90271901 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1601,6 +1601,13 @@  void unmap_vcpu_info(struct vcpu *v)
     put_page_and_type(mfn_to_page(mfn));
 }
 
+int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
+                   struct guest_area *area,
+                   void (*populate)(void *dst, struct vcpu *v))
+{
+    return -EOPNOTSUPP;
+}
+
 /*
  * This is only intended to be used for domain cleanup (or more generally only
  * with at least the respective vCPU, if it's not the current one, reliably