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

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

On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote:
> > > On Thu, Mar 24, 2022 at 11:46 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/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)
> > > > >              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)
> > > >
> > > > I'm confused about why two different interfaces are added to do this
> > > > kind of selective resets, one to vm_event and one to xenmem_fork?
> > > >
> > > > I thin k the natural place for the option to live would be
> > > > XENMEM_FORK?
> > >
> > > Yes, that's the natural place for it. But we are adding it to both for
> > > a reason. In our use-case the reset operation will happen after a
> > > vm_event is received to which we already must send a reply. Setting
> > > the flag on the vm_event reply saves us having to issue an extra memop
> > > hypercall afterwards.
> >
> > Can you do a multicall and batch both operations in a single
> > hypercall?
> >
> > That would seem more natural than adding duplicated interfaces.
> 
> Not in a straight forward way, no. There is no exposed API in libxc to
> do a multicall. Even if that was an option it is still easier for me
> to just flip a bit in the response field than having to construct a
> whole standalone hypercall structure to be sent as part of a
> multicall.

Right, I can see it being easier, but it seems like a bad choice from
an interface PoV. You are the maintainer of both subsystems, but it
would seem to me it's in your best interest to try to keep the
interfaces separated and clean.

Would it be possible for the reset XENMEM_FORK op to have the side
effect of performing what you would instead do with the vm_event
hypercall?

Thanks, Roger.