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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 26/04/2022 09:17, Roger Pau Monné wrote:
> 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:
> > > > 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.
The current approach is actually not correct. On production build, 
ASSERT() will turn to NOP. IOW mem_sharing_fork_reset() *will* not be 
called.
So the call needs to move outside of the ASSERT() and use a construct 
similar to what you suggested:
if ( .... && mem_sharing_fork_reset(...) )
{
  ASSERT_UNREACHABLE();
  break;
}

Cheers,

--
Julien Grall