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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

> +        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".

> +            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?

Jan