Re: [PATCH v8 3/7] KVM: VMX: Pass through CET related MSRs

[Sean Christopherson]

On Wed, Dec 11, 2019 at 09:32:07AM +0800, Yang Weijiang wrote:
> On Tue, Dec 10, 2019 at 01:18:21PM -0800, Sean Christopherson wrote:
> > On Fri, Nov 01, 2019 at 04:52:18PM +0800, Yang Weijiang wrote:
> > > CET MSRs pass through Guest directly to enhance performance.
> > > CET runtime control settings are stored in MSR_IA32_{U,S}_CET,
> > > Shadow Stack Pointer(SSP) are stored in MSR_IA32_PL{0,1,2,3}_SSP,
> > > SSP table base address is stored in MSR_IA32_INT_SSP_TAB,
> > > these MSRs are defined in kernel and re-used here.
> > > 
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index dd387a785c1e..4166c4fcad1e 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -371,13 +371,13 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index)
> > >  		F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
> > >  		F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
> > >  		F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
> > > -		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
> > > +		F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | F(SHSTK);
> > >  
> > >  	/* cpuid 7.0.edx*/
> > >  	const u32 kvm_cpuid_7_0_edx_x86_features =
> > >  		F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
> > >  		F(SPEC_CTRL_SSBD) | F(ARCH_CAPABILITIES) | F(INTEL_STIBP) |
> > > -		F(MD_CLEAR);
> > > +		F(MD_CLEAR) | F(IBT);
> > 
> > Advertising CET to userspace/guest needs to be done at the end of the
> > series, or at least after CR4.CET is no longer reserved, e.g. KVM_SET_SREGS
> > will fail and the guest will get a #GP when trying to set CR4.CET.
> > 
> > I'm pretty sure I've said this at least twice in previous versions of
> > this series...
> 
> Thanks Sean for picking these up!
> The reason is, starting from this patch, I'm using guest_cpuid_has(CET)
> to check the availability of guest CET CPUID, so logically I would like to let
> the readers understand CET related CPUID word is
> defined as above. But no problem, I can move these definitions to a
> latter patch as the patchset only meaningful as a whole. 

Adding usage of guest_cpuid_has(CET) without advertising CET is perfectly
ok from a functionality perspective.  Having a user without a consumer
isn't ideal, but it's better than having one gigantic patch.

The problem with advertising CET when it's not fully supported is that it
will break bisection, e.g. trying to boot a CET-enabled guest would get a
#GP during boot and likely crash.  Whether or not a series is useful when
taken as a whole is orthogonal to the integrity of each invidiual patch.