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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Mar 29, 2022 at 11:42 AM Jan Beulich <jbeulich@xxxxxxxx> 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