Re: [PATCH 7/7] x86/kvm: Expose LASS feature to VM guest

[Sean Christopherson]

On Tue, Feb 07, 2023, Wang, Lei wrote:
> Could you also CC the KVM mailing list (kvm@xxxxxxxxxxxxxxx) for KVM guys to review?

As Sohil pointed out[*], use scripts/get_maintainer.pl.  Cc'ing kvm@ on random
bits of the series isn't sufficient, e.g. I completely missed Sohil's Cc on the
cover letter.  For me personally at least, if I'm Cc'd on one patch then I want
to be Cc'd on all patches.

That's somewhat of a moot point for the future of LASS enabling, because the KVM
enabling needs to be a separate series.

[*] https://lore.kernel.org/all/66857084-fbed-3e9a-ed2c-7167010cbad9@xxxxxxxxx

> On 1/10/2023 1:52 PM, Yian Chen wrote:
> > From: Paul Lai <paul.c.lai@xxxxxxxxx>
> > 
> > Expose LASS feature which is defined in the CPUID.7.1.EAX
> > bit and enabled via the CR4 bit for VM guest.
> > 
> > Signed-off-by: Paul Lai <paul.c.lai@xxxxxxxxx>
> > Signed-off-by: Yian Chen <yian.chen@xxxxxxxxx>
> > Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>

So I'm pretty sure this "review" was to satisfy Intel's requirements to never post
anything to x86@ that wasn't reviewed internally by one of a set of select
individuals.  Please drop that requirement for KVM x86, I truly think it's doing
more harm than good.

This is laughably inadqueate "enabling".  This has architecturally visible effects
on _all_ guest virtual/linear accesses.  That statement alone should set off alarms
left and right that simply advertising support via KVM_GET_SUPPORTED_CPUID and
marking the CR4 bit as not unconditionally reserved is insufficient.

My recommendation is to look at the touchpoints for X86_CR4_LA57 and follow the
various breadcrumbs to identify what all needs to be done to properly support LASS.

More importantly, I want tests.  There's _zero_ chance this was reasonably tested.
E.g. the most basic negative testcase of attempting to set X86_CR4_LASS on a host
without LASS is missed (see __cr4_reserved_bits()).

I will not so much as glance at future versions of LASS enabling if there aren't
testscases in KVM-unit-tests and/or selftests that give me a high level of confidence
that KVM correctly handles the various edge cases, e.g. that KVM correctly handles
an implicit supervisor access from user mode while in the emulator.

That goes a for all new hardware enabling: no tests, no review.  Please relay that
message to managers, leadership, and everyone working on KVM.  Ample warning has
been given that new KVM features need to be accompanied by tests.

> > ---
> >  arch/x86/include/asm/kvm_host.h | 3 ++-
> >  arch/x86/kvm/cpuid.c            | 2 +-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index f35f1ff4427b..bd39f45e9b5a 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -125,7 +125,8 @@
> >  			  | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \
> >  			  | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \
> >  			  | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \
> > -			  | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP))
> > +			  | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \
> > +			  | X86_CR4_LASS))
> >  
> >  #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
> >  
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index b14653b61470..e0f53f85f5ae 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -664,7 +664,7 @@ void kvm_set_cpu_caps(void)
> >  
> >  	kvm_cpu_cap_mask(CPUID_7_1_EAX,
> >  		F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) | F(AMX_FP16) |
> > -		F(AVX_IFMA)
> > +		F(AVX_IFMA) | F(LASS)
> >  	);
> >  
> >  	kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX,