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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Apr 6, 2022 at 11:14 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 06.04.2022 16:58, Tamas K Lengyel wrote:
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -328,6 +328,24 @@ bool hvm_monitor_check_p2m(unsigned long gla, gfn_t 
> > gfn, uint32_t pfec,
> >      return monitor_traps(curr, true, &req) >= 0;
> >  }
> >
> > +int hvm_monitor_vmexit(unsigned long exit_reason,
> > +                       unsigned long exit_qualification)
> > +{
> > +    struct vcpu *curr = current;
> > +    struct arch_domain *ad = &curr->domain->arch;
> > +    vm_event_request_t req = {};
> > +
> > +    ASSERT(ad->monitor.vmexit_enabled);
> > +
> > +    req.reason = VM_EVENT_REASON_VMEXIT;
> > +    req.u.vmexit.reason = exit_reason;
> > +    req.u.vmexit.qualification = exit_qualification;
> > +
> > +    set_npt_base(curr, &req);
> > +
> > +    return monitor_traps(curr, !!ad->monitor.vmexit_sync, &req);
>
> vmexit_sync is a single-bit bitfield; I don't see the need for using
> !! here, even more so that the respective parameter of monitor_traps()
> is "bool" anyway.

Ack.

>
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -4008,6 +4008,18 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >          }
> >      }
> >
> > +    if ( unlikely(currd->arch.monitor.vmexit_enabled) )
> > +    {
> > +        int rc;
> > +        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> > +
>
> Nit: Please swap blank and non-blank lines here.

Ack.

>
> > +        rc = hvm_monitor_vmexit(exit_reason, exit_qualification);
> > +        if ( rc < 0 )
> > +            goto exit_and_crash;
> > +        else if ( rc )
>
> Nit: No need for "else" here, just if() suffices after "goto".

Ack.

>
> > +            return;
> > +    }
> > +
> >      /* XXX: This looks ugly, but we need a mechanism to ensure
> >       * any pending vmresume has really happened
> >       */
>
> A few lines down from here failed VM entry is being handled? Wouldn't
> you want to place your code after that? And wouldn't you want to avoid
> invoking the monitor for e.g. EXIT_REASON_EXTERNAL_INTERRUPT,
> EXIT_REASON_MCE_DURING_VMENTRY, and at least the NMI sub-case of
> EXIT_REASON_EXCEPTION_NMI?

No, the placement is necessary to be where it is to be able to collect
information about all vmexits, no matter the root cause. Failed
vmentry & mce during vmentry would be interesting exits to see if we
can induce during fuzzing from within the guest (indicating some
serious state corruption) while the external interrupt and nmi cases
are for the most part just ignored and the fuzz-case restarted, but
could be still interesting to collect their frequencies. So in effect,
we want to avoid Xen hiding any of the events from the monitoring
agent by handling it one way or another and just let the agent decide
what to do next. We most certainly want to avoid Xen crashing the VM
for us.

Tamas