Re: [PATCH v10 3/9] KVM: x86: Use KVM-governed feature framework to track "LAM enabled"

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 17, 2023, Binbin Wu wrote:
> 
> 
> On 8/17/2023 5:33 AM, Sean Christopherson wrote:
> > On Wed, Aug 16, 2023, Kai Huang wrote:
> > > > > > --- a/arch/x86/kvm/vmx/vmx.c
> > > > > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > > > > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > > > >    		vmx->msr_ia32_feature_control_valid_bits &=
> > > > > >    			~FEAT_CTL_SGX_LC_ENABLED;
> > > > > > +	if (boot_cpu_has(X86_FEATURE_LAM))
> > > > > > +		kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM);
> > > > > > +
> > > > > If you want to use boot_cpu_has(), it's better to be done at your last patch to
> > > > > only set the cap bit when boot_cpu_has() is true, I suppose.
> > > > Yes, but new version of kvm_governed_feature_check_and_set() of
> > > > KVM-governed feature framework will check against kvm_cpu_cap_has() as well.
> > > > I will remove the if statement and call
> > > > kvm_governed_feature_check_and_set()  directly.
> > > > https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/
> > > > 
> > > I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you
> > > are using boot_cpu_has().  They are not the same.
> > > 
> > > If LAM should be only supported when boot_cpu_has() is true then it seems you
> > > can just only set the LAM cap bit when boot_cpu_has() is true.  As you also
> > > mentioned above the kvm_governed_feature_check_and_set() here internally does
> > > kvm_cpu_cap_has().
> > That's covered by the last patch:
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index e961e9a05847..06061c11d74d 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -677,7 +677,7 @@ void kvm_set_cpu_caps(void)
> >          kvm_cpu_cap_mask(CPUID_7_1_EAX,
> >                  F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) |
> >                  F(FZRM) | F(FSRS) | F(FSRC) |
> > -               F(AMX_FP16) | F(AVX_IFMA)
> > +               F(AMX_FP16) | F(AVX_IFMA) | F(LAM)
> >          );
> >          kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX,
> > 
> > 
> > Which highlights a problem with activating a goverened feature before said feature
> > is actually supported by KVM: it's all kinds of confusing.
> > 
> > It'll generate a more churn in git history, but I think we should first enable
> > LAM without a goverened feature, and then activate a goverened feature later on.
> > Using a goverened feature is purely an optimization, i.e. the series needs to be
> > function without using a governed feature.
> OK, then how about the second option which has been listed in your v9 patch
> series discussion.
> https://lore.kernel.org/kvm/20230606091842.13123-1-binbin.wu@linux.intel.com/T/#m16ee5cec4a46954f985cb6afedb5f5a3435373a1
> 
> Temporarily add a bool can_use_lam in kvm_vcpu_arch and use the bool
> "can_use_lam" instead of guest_can_use(vcpu, X86_FEATURE_LAM).
> and then put the patch of adopting "KVM-governed feature framework" to the
> last.

No, just do the completely unoptimized, but functionally obvious thing:

	if (kvm_cpu_cap_has(x86_FEATURE_LAM) &&
	    guest_cpuid_has(vcpu, x86_FEATURE_LAM))
		...

I don't expect anyone to push back on using a governed feature, i.e. I don't expect
to ever see a kernel release with the unoptimized code.  If someone is bisecting
or doing something *really* weird with their kernel management, then yes, they
might see suboptimal performance.

Again, the goal is to separate the addition of functionality from the optimization
of that functionality, e.g. to make it easier to review and understand each change.