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

From: Chao Gao <chao.gao@intel.com>
To: "Yang, Weijiang" <weijiang.yang@intel.com>
Cc: <seanjc@google.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 13:51:33 +0800	[thread overview]
Message-ID: <ZMyR5Ztfjd9EMgIR@chao-email> (raw)
In-Reply-To: <83d767df-c9ef-1bee-40c0-2360598aafa8@intel.com>

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?

>> > +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().

>
>kvm_cpu_cap_has(X86_FEATURE_SHSTK) || kvm_cpu_cap_has(X86_FEATURE_IBT)
>
>only tells at least one of the CET features is supported by KVM.
>
>> then patch 16 has no need to do
>> 
>> +	/*
>> +	 * If SHSTK and IBT are not available in KVM, clear CET user bit in
>> +	 * kvm_caps.supported_xss so that kvm_is_cet__supported() returns
>> +	 * false when called.
>> +	 */
>> +	if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK) &&
>> +	    !kvm_cpu_cap_has(X86_FEATURE_IBT))
>> +		kvm_caps.supported_xss &= ~CET_XSTATE_MASK;
>