Date:   Wed, 1 Feb 2023 00:39:07 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Sandipan Das <sandipan.das@amd.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        Jiaxi Chen <jiaxi.chen@linux.intel.com>,
        Babu Moger <babu.moger@amd.com>, linux-kernel@vger.kernel.org,
        Jing Liu <jing2.liu@intel.com>,
        Wyes Karny <wyes.karny@amd.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>,
        Santosh Shukla <santosh.shukla@amd.com>
Subject: Re: [PATCH v2 10/11] KVM: SVM: implement support for vNMI
Message-ID: <Y9m0q31NBmsnhVGD@google.com>
References: <20221129193717.513824-1-mlevitsk@redhat.com>
 <20221129193717.513824-11-mlevitsk@redhat.com>
 <Y9mwz/G6+G8NSX3+@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9mwz/G6+G8NSX3+@google.com>
Precedence: bulk

On Wed, Feb 01, 2023, Sean Christopherson wrote:
> On Tue, Nov 29, 2022, Maxim Levitsky wrote:
> > @@ -553,6 +554,15 @@ static inline bool is_x2apic_msrpm_offset(u32 offset)
> >  	       (msr < (APIC_BASE_MSR + 0x100));
> >  }
> >  
> > +static inline bool is_vnmi_enabled(struct vcpu_svm *svm)
> > +{
> > +	/* L1's vNMI is inhibited while nested guest is running */
> > +	if (is_guest_mode(&svm->vcpu))
> 
> I would rather check the current VMCB.  I don't see any value in hardcoding the
> "KVM doesn't support vNMI in L2" in multiple places.  And I find the above comment
> about "L1's vNMI is inhibited" confusing.  vNMI isn't inhibited/blocked, KVM just
> doesn't utilize vNMI while L2 is active (IIUC, as proposed).

Oof.  Scratch that, code is correct as proposed, but the comment and function name
are confusing.

After looking at the nested support, is_vnmi_enabled() actually means "is vNMI
currently enabled _for L1_".  And it has less to do with vNMI being "inhibited" and
everything to do with KVM choosing not to utilize vNMI for L1 while running L2.

"inhibited" in quotes because "inhibited" is a synonym of "blocked", i.e. I read
that as L1 NMIs are blocked.

So regardless of whether or not KVM decides to utilize vNMI for L2 if L1's NMIs
are passed through, the function name needs to clarify that it's referring to
L1.  E.g. is_vnmi_enabled_for_l1() or so.

And if KVM decides not to use vNMI for L1 while running L2, state that more
explicitly instead of saying it's inhibited.

And if KVM does decide to use vNMI while running L2 if NMIs are passed through,
then the comment goes away and KVM toggles the flag in vmcb01 on nested enter
and exit.

> > +		return false;
> > +
> > +	return !!(svm->vmcb01.ptr->control.int_ctl & V_NMI_ENABLE);
> > +}
> > +
> >  /* svm.c */
> >  #define MSR_INVALID				0xffffffffU
> >  
> > -- 
> > 2.26.3
> >