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.
