Re: [PATCH v4 1/2] x86/mem_sharing: make fork_reset more configurable

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 25, 2022 at 11:24:37AM -0400, Tamas K Lengyel wrote:
> On Mon, Apr 25, 2022 at 10:12 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Wed, Apr 13, 2022 at 09:41:51AM -0400, Tamas K Lengyel wrote:
> > > 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@xxxxxxxxx>
> >
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thank you!
> 
> > > ---
> > > v4: No change
> > > v3: Rebase on simpler approach after dropping empty_p2m feature
> > > 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          | 24 +++++++++++++++++++-----
> > >  xen/common/vm_event.c                  | 15 +++++++++++++++
> > >  xen/include/public/memory.h            |  4 +++-
> > >  xen/include/public/vm_event.h          |  8 ++++++++
> > >  7 files changed, 62 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > > index 95bd5eca67..1b089a2c02 100644
> > > --- a/tools/include/xenctrl.h
> > > +++ b/tools/include/xenctrl.h
> > > @@ -2290,7 +2290,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 a6cfd7dccf..a0d0b894e2 100644
> > > --- a/tools/libs/ctrl/xc_memshr.c
> > > +++ b/tools/libs/ctrl/xc_memshr.c
> > > @@ -257,12 +257,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;
> >
> > IMO would be clearer to init mso fields at definition.
> 
> Not sure what you mean exactly, mso = { ... }; ? I think the logic is
> pretty clear as-is and I don't have any preference for one style vs
> the other.

IMO it's clearer to initialize the fields at declaration using
mso = { ... } because then you avoid the memset.

> >
> > > 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));
> >
> > Might be appropriate to destroy the domain in case fork reset fails?
> > ASSERT will only help in debug builds.
> 
> No, I would prefer not destroying the domain here. If it ever becomes
> necessary the right way would be to introduce a new monitor event to
> signal an error and let the listener decide what to do. At the moment
> I don't see that being necessary as there are no known scenarios where
> we would be able to setup a fork but fail to reset is.

My concern for raising this was what would happen on non-debug
builds if mem_sharing_fork_reset() failed, and hence my request to
crash the domain.  I would have used something like:

if ( (reset_state || reset_mem) &&
     mem_sharing_fork_reset(d, reset_state, reset_mem) )
{
    ASSERT_UNREACHABLE();
    domain_crash(d);
    break;
}

But if you and other vm_event maintainers are fine with the current
approach and don't think it's a problem that's OK with me.

Thanks, Roger.