Re: [PATCH 2/2] x86/vmx: implement Notify VM Exit

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, May 19, 2022 at 08:50:55AM +0200, Jan Beulich wrote:
> On 17.05.2022 15:21, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -67,6 +67,9 @@ integer_param("ple_gap", ple_gap);
> >  static unsigned int __read_mostly ple_window = 4096;
> >  integer_param("ple_window", ple_window);
> >  
> > +static int __read_mostly vm_notify_window;
> 
> __ro_after_init?

Yes, I tend to forget we have this now.

> > @@ -1333,6 +1338,19 @@ static int construct_vmcs(struct vcpu *v)
> >          rc = vmx_add_msr(v, MSR_FLUSH_CMD, FLUSH_CMD_L1D,
> >                           VMX_MSR_GUEST_LOADONLY);
> >  
> > +    if ( cpu_has_vmx_notify_vm_exiting && vm_notify_window >= 0 )
> 
> The assumption then is that values >= 2^^31 are nonsense? Generally
> I'd think we want to special case merely ~0u, giving the variable
> unsigned type.

OK, I really don't know whether >= 2^31 makes sense or not, I would
think that using such values the window would be too big to be
helpful.  In any case I don't see a point in preventing >= 2^31 so
will adjust the type and check.

> However, I also don't see where you disable use of
> the feature in that case: Merely skipping the VMCS update here isn't
> enough, is it? The field itself doesn't know any special case
> values (like ~0) as per the doc I'm looking at. So I guess the OR-ing
> in of SECONDARY_EXEC_NOTIFY_VM_EXITING in vmx_init_vmcs_config()
> wants to be conditional.

I've sent the disabling chunk as a followup, forgot to add it here,
sorry.

> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -4567,6 +4567,30 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> >           */
> >          break;
> >  
> > +    case EXIT_REASON_NOTIFY:
> > +        __vmread(EXIT_QUALIFICATION, &exit_qualification);
> > +
> > +        if ( exit_qualification & NOTIFY_VM_CONTEXT_INVALID )
> > +        {
> > +            perfc_incr(vmnotify_crash);
> 
> Is this a useful event to count? We don't count other crash causes,
> iirc.

I thought it was helpful information from an admin PoV, but maybe I'm
mistaken.  I know we don't count other crash reasons, but that doesn't
mean it won't be helpful to do so.  Given that users have to opt-in to
enable counters I suggest to leave the counter there.

Thanks, Roger.