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

[Yang Weijiang]

On Tue, Dec 10, 2019 at 05:50:52PM -0800, Sean Christopherson wrote:
> 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.

Oh, I omitted case likes bisection, you're right, I'll change it, thanks
a lot!