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

From: Yang Weijiang <weijiang.yang@intel.com>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Yang Weijiang <weijiang.yang@intel.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	pbonzini@redhat.com, jmattson@google.com,
	yu.c.zhang@linux.intel.com, yu-cheng.yu@intel.com
Subject: Re: [PATCH v8 3/7] KVM: VMX: Pass through CET related MSRs
Date: Wed, 18 Dec 2019 21:55:13 +0800	[thread overview]
Message-ID: <20191218135513.GB7926@local-michael-cet-test> (raw)
In-Reply-To: <20191218003455.GP11771@linux.intel.com>

On Tue, Dec 17, 2019 at 04:34:55PM -0800, Sean Christopherson wrote:
> On Mon, Dec 16, 2019 at 10:18:16AM +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.
> > > > 
> >  > +
> > > >  static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> > > >  {
> > > >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > > > @@ -7025,6 +7087,9 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> > > >  	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
> > > >  			guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT))
> > > >  		update_intel_pt_cfg(vcpu);
> > > > +
> > > > +	if (!is_guest_mode(vcpu))
> > > > +		vmx_pass_cet_msrs(vcpu);
> > > 
> > > Hmm, this looks insufficent, e.g. deliberately toggling CET from on->off
> > > while in guest mode would put KVM in a weird state as the msr bitmap for
> > > L1 would still allow L1 to access the CET MSRs.
> > >
> > Hi, Sean,
> > I don't get you, there's guest mode check before access CET msrs, it'll
> > fail if it's in guest mode.
> 
> KVM can exit to userspae while L2 is active.  If userspace then did a
> KVM_SET_CPUID2, e.g. instead of KVM_RUN, vmx_cpuid_update() would skip
> vmx_pass_cet_msrs() and KVM would never update L1's MSR bitmaps.
>
Thanks, it makes sense to me. Given current implementation, how about
removing above check and adding it in CET CPUID
enumeration for L2 so that no CET msrs passed through to L2 when
is_guest_mode() is true?

> > > Allowing KVM_SET_CPUID{2} while running a nested guest seems bogus, can we
> > > kill that path entirely with -EINVAL?
> > >
> > Do you mean don't expose CET cpuids to L2 guest?
> 
> I mean completely disallow KVM_SET_CPUID and KVM_SET_CPUID2 if
> is_guest_mode() is true.  My question is mostly directed at Paolo and
> anyone else that has an opinion on whether we can massage the ABI to
> retroactively change KVM_SET_CPUID{2} behavior.
This sounds like something deserving an individual patch after get
agreement in community. I'll put it aside right now.