Date:   Thu, 9 Feb 2023 17:18:08 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Wang, Lei" <lei4.wang@intel.com>
Cc:     Yian Chen <yian.chen@intel.com>, linux-kernel@vger.kernel.org,
        x86@kernel.org, Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ravi Shankar <ravi.v.shankar@intel.com>,
        Tony Luck <tony.luck@intel.com>,
        Sohil Mehta <sohil.mehta@intel.com>,
        Paul Lai <paul.c.lai@intel.com>
Subject: Re: [PATCH 7/7] x86/kvm: Expose LASS feature to VM guest
Message-ID: <Y+Uq0JOEmmdI0YwA@google.com>
References: <20230110055204.3227669-1-yian.chen@intel.com>
 <20230110055204.3227669-8-yian.chen@intel.com>
 <faac69f3-3885-8cab-ea33-49922d7a6b6d@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <faac69f3-3885-8cab-ea33-49922d7a6b6d@intel.com>
Precedence: bulk

On Tue, Feb 07, 2023, Wang, Lei wrote:
> Could you also CC the KVM mailing list (kvm@vger.kernel.org) 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@intel.com

> On 1/10/2023 1:52 PM, Yian Chen wrote:
> > From: Paul Lai <paul.c.lai@intel.com>
> > 
> > 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@intel.com>
> > Signed-off-by: Yian Chen <yian.chen@intel.com>
> > Reviewed-by: Tony Luck <tony.luck@intel.com>

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,