On Tue, Apr 26, 2022, 4:33 AM Julien Grall <jul...@xen.org> wrote:
> 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....@citrix.com>
> 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;
> }
>
Ah, good call. Thanks!
Tamas
>
