Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as to-be-saved

From: Sean Christopherson <seanjc@google.com>
To: Chao Gao <chao.gao@intel.com>
Cc: Weijiang Yang <weijiang.yang@intel.com>,
	pbonzini@redhat.com, peterz@infradead.org, john.allen@amd.com,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	rick.p.edgecombe@intel.com, binbin.wu@linux.intel.com
Subject: Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as to-be-saved
Date: Fri, 4 Aug 2023 11:51:00 -0700	[thread overview]
Message-ID: <ZM1IlPrWz/R6D0O5@google.com> (raw)
In-Reply-To: <ZMyR5Ztfjd9EMgIR@chao-email>

On Fri, Aug 04, 2023, Chao Gao wrote:
> On Fri, Aug 04, 2023 at 11:13:36AM +0800, Yang, Weijiang wrote:
> >> > @@ -7214,6 +7217,13 @@ static void kvm_probe_msr_to_save(u32 msr_index)
> >> > 		if (!kvm_caps.supported_xss)
> >> > 			return;
> >> > 		break;
> >> > +	case MSR_IA32_U_CET:
> >> > +	case MSR_IA32_S_CET:
> >> > +	case MSR_KVM_GUEST_SSP:
> >> > +	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
> >> > +		if (!kvm_is_cet_supported())
> >> shall we consider the case where IBT is supported while SS isn't
> >> (e.g., in L1 guest)?
> >Yes, but userspace should be able to access SHSTK MSRs even only IBT is exposed to guest so
> >far as KVM can support SHSTK MSRs.
> 
> Why should userspace be allowed to access SHSTK MSRs in this case? L1 may not
> even enumerate SHSTK (qemu removes -shstk explicitly but keeps IBT), how KVM in
> L1 can allow its userspace to do that?

+1.  And specifically, this isn't about SHSTK being exposed to the guest, it's about
SHSTK being _supported by KVM_.  This is all about KVM telling userspace what MSRs
are valid and/or need to be saved+restored.  If KVM doesn't support a feature,
then the MSRs are invalid and there is no reason for userspace to save+restore
the MSRs on live migration.

> >> > +static inline bool kvm_is_cet_supported(void)
> >> > +{
> >> > +	return (kvm_caps.supported_xss & CET_XSTATE_MASK) == CET_XSTATE_MASK;
> >> why not just check if SHSTK or IBT is supported explicitly, i.e.,
> >> 
> >> 	return kvm_cpu_cap_has(X86_FEATURE_SHSTK) ||
> >> 	       kvm_cpu_cap_has(X86_FEATURE_IBT);
> >> 
> >> this is straightforward. And strictly speaking, the support of a feature and
> >> the support of managing a feature's state via XSAVE(S) are two different things.x
> >I think using exiting check implies two things:
> >1. Platform/KVM can support CET features.
> >2. CET user mode MSRs are backed by host thus are guaranteed to be valid.
> >i.e., the purpose is to check guest CET dependencies instead of features' availability.
> 
> When KVM claims a feature is supported, it should ensure all its dependencies are
> met. that's, KVM's support of a feature also imples all dependencies are met.
> Function-wise, the two approaches have no difference. I just think checking
> KVM's support of SHSTK/IBT is more clear because the function name is
> kvm_is_cet_supported() rather than e.g., kvm_is_cet_state_managed_by_xsave().

+1, one of the big reasons kvm_cpu_cap_has() came about was being KVM had a giant
mess of one-off helpers.