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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Mar 25, 2022 at 9:42 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index a21c781452..bfa6082f13 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1892,15 +1892,19 @@ 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;
> >
> >      domain_pause(d);
>
> I would assert that at least one of reset_sate or reset_memory is set
> here, as callers already do the checks.

Sure.

>
> >
> > +    if ( !reset_memory )
> > +        goto state;
>
> I don't like using labels and goto like this as I think it makes the
> code harder to follow, and so more likely to introduce bugs. I would
> rather place the memory reset parts inside of an if ( reset_memory ) {
> ... }, but that's my taste.

I did that first but because it requires shifting everything to the
right it requires odd line breaks.

>
> > +
> >      /* 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)
> > @@ -1933,7 +1937,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.skip_special_pages);
> > + state:
> > +    if ( reset_state )
> > +        rc = copy_settings(d, pd, 
> > d->arch.hvm.mem_sharing.skip_special_pages);
> >
> >      domain_unpause(d);
> >
> > @@ -2239,15 +2245,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..a7b192be0d 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,15 @@ 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
> > +            do {
> > +                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 )
> > +                    mem_sharing_fork_reset(d, reset_state, reset_mem);
>
> You seem to drop the error code returned by mem_sharing_fork_reset.

Yes, there is no response that could be sent to the toolstack from
here. I could add an ASSERT that rc is 0 though. If the fork() op
successfully finished then fork_reset() couldn't reasonably end up in
a path where it fails.

>
> > +            } while(0);
> > +#endif
>
> I think you can avoid the do {} while(0); just using the braces will
> allow you to define local variables in the inner block.

Sure.

>
> >              /*
> >               * 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 208d8dcbd9..30ce23c5a7 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_SKIP_SPECIAL_PAGES (1u << 2)
> > +#define XENMEM_FORK_RESET_STATE        (1u << 3)
> > +#define XENMEM_FORK_RESET_MEMORY       (1u << 4)
>
> For backward compatibility purposes should the flags be added
> backwards, ie:
>
> #define XENMEM_FORK_KEEP_STATE        (1u << 3)
> #define XENMEM_FORK_KEEP_MEMORY       (1u << 4)
>
> So that existing callers of XENMEM_sharing_op_fork_reset will continue
> working as expected?
>
> Or we don't care about that as the interface is protected with
> __XEN_TOOLS__?

I would say don't care, we are updating the only toolstack that uses
this in lock-step with Xen.

Tamas