Re: [PATCH v4 2/2] x86/monitor: Add new monitor event to catch all vmexits

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

On Tue, Apr 26, 2022 at 02:53:54PM -0400, Tamas K Lengyel wrote:
> On Tue, Apr 26, 2022 at 4:50 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Mon, Apr 25, 2022 at 11:40:11AM -0400, Tamas K Lengyel wrote:
> > > On Mon, Apr 25, 2022 at 10:41 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> 
> > > wrote:
> > > >
> > > > On Wed, Apr 13, 2022 at 09:41:52AM -0400, Tamas K Lengyel wrote:
> > > > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> > > > > index 3079726a8b..30ca71432c 100644
> > > > > --- a/xen/arch/x86/monitor.c
> > > > > +++ b/xen/arch/x86/monitor.c
> > > > > @@ -332,6 +332,20 @@ int arch_monitor_domctl_event(struct domain *d,
> > > > >          break;
> > > > >      }
> > > > >
> > > > > +    case XEN_DOMCTL_MONITOR_EVENT_VMEXIT:
> > > > > +    {
> > > > > +        bool old_status = ad->monitor.vmexit_enabled;
> > > > > +
> > > > > +        if ( unlikely(old_status == requested_status) )
> > > > > +            return -EEXIST;
> > > >
> > > > What about if the requested status is the same as the current one, but
> > > > vmexit sync is not?
> > >
> > > You need to clear the currently registered event first, then register
> > > the new one.
> > >
> > > > IOW, I'm not sure this check is helpful, and you could likely avoid
> > > > the old_status local variable.
> > >
> > > It is helpful on the callee side. Usually the callee needs to keep
> > > track of the state of what events are enabled so that it can clean up
> > > after itself and if it ever runs into trying to set the event to
> > > something it's already set to then that indicates its state being
> > > out-of-sync.
> >
> > Hm, right.  I wonder if you should also check that the ring is empty
> > before changing the status?  So that the callee doesn't change the
> > status while requests are still pending on the ring from the previous
> > type?
> 
> No, that becomes tricky because really the only way to ensure the ring
> remains empty from the userspace is to pause the domain, which is very
> heavy handed. There is nothing wrong with asking Xen not to produce
> more of a certain type of request while still being able to handle the
> ones that are already on the ring. For setups where the two should
> happen at the same time is where the toolstack first pauses the
> domain, clears the ring, then disables the event. Both are valid
> approaches.

OK, so we rely on the callee to drain the ring properly when wanting
to change VMEXIT events.

Thanks, Roger.