Message-ID: <83d767df-c9ef-1bee-40c0-2360598aafa8@intel.com>
Date:   Fri, 4 Aug 2023 11:13:36 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     Chao Gao <chao.gao@...el.com>
CC:     <seanjc@...gle.com>, <pbonzini@...hat.com>, <peterz@...radead.org>,
        <john.allen@....com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <rick.p.edgecombe@...el.com>,
        <binbin.wu@...ux.intel.com>
Subject: Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as
 to-be-saved

On 8/3/2023 6:39 PM, Chao Gao wrote:
> On Thu, Aug 03, 2023 at 12:27:21AM -0400, Yang Weijiang wrote:
>> Add all CET MSRs including the synthesized GUEST_SSP to report list.
>> PL{0,1,2}_SSP are independent to host XSAVE management with later
>> patches. MSR_IA32_U_CET and MSR_IA32_PL3_SSP are XSAVE-managed on
>> host side. MSR_IA32_S_CET/MSR_IA32_INT_SSP_TAB/MSR_KVM_GUEST_SSP
>> are not XSAVE-managed.
>>
>> When CET IBT/SHSTK are enumerated to guest, both user and supervisor
>> modes should be supported for architechtural integrity, i.e., two
>> modes are supported as both or neither.
> I think whether MSRs are XSAVE-managed or not isn't related or important in
> this patch. And I don't get what's the intent of the last paragraph.
I recalled the original intent to say, although kvm_is_cet_supported() only checks the
host support of user mode states, but user and supervisor mode states are exposed
as a bundle, i.e., both or neither. So it can enforce the same check for both
user and supervisor states support.
> how about:
>
> Add CET MSRs to the list of MSRs reported to userspace if the feature
> i.e., IBT or SHSTK, associated with the MSRs is supported by KVM.
It's OK for me, thanks!
>> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
>> ---
>> arch/x86/include/uapi/asm/kvm_para.h |  1 +
>> arch/x86/kvm/x86.c                   | 10 ++++++++++
>> arch/x86/kvm/x86.h                   | 10 ++++++++++
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h
>> index 6e64b27b2c1e..7af465e4e0bd 100644
>> --- a/arch/x86/include/uapi/asm/kvm_para.h
>> +++ b/arch/x86/include/uapi/asm/kvm_para.h
>> @@ -58,6 +58,7 @@
>> #define MSR_KVM_ASYNC_PF_INT	0x4b564d06
>> #define MSR_KVM_ASYNC_PF_ACK	0x4b564d07
>> #define MSR_KVM_MIGRATION_CONTROL	0x4b564d08
>> +#define MSR_KVM_GUEST_SSP	0x4b564d09
>>
>> struct kvm_steal_time {
>> 	__u64 steal;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 82b9f14990da..d68ef87fe007 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1463,6 +1463,9 @@ static const u32 msrs_to_save_base[] = {
>>
>> 	MSR_IA32_XFD, MSR_IA32_XFD_ERR,
>> 	MSR_IA32_XSS,
>> +	MSR_IA32_U_CET, MSR_IA32_S_CET,
>> +	MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP, MSR_IA32_PL2_SSP,
>> +	MSR_IA32_PL3_SSP, MSR_IA32_INT_SSP_TAB, MSR_KVM_GUEST_SSP,
> MSR_KVM_GUEST_SSP really should be added by a separate patch.
>
> it is incorrect to put MSR_KVM_GUEST_SSP here because the rdmsr_safe() in
> kvm_probe_msr_to_save() will fail since hardware doesn't have this MSR.
>
> IMO, MSR_KVM_GUEST_SSP should go to emulated_msrs_all[].
Nice catch! Will move it to emulated_msrs_all, thanks!
>> };
>>
>> static const u32 msrs_to_save_pmu[] = {
>> @@ -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. And here is to advertise all the supported CET MSRs.
So maybe we don't need to check specific feature support.
> if yes, we should do
> 	case MSR_IA32_U_CET:
> 	case MSR_IA32_S_CET:
> 		if (!kvm_is_cet_supported())
> 			return;
> 	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
> 		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
> 			return;
> 	
>
>
>> +			return;
>> +		break;
>> 	default:
>> 		break;
>> 	}
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 82e3dafc5453..6e6292915f8c 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -362,6 +362,16 @@ static inline bool kvm_mpx_supported(void)
>> 		== (XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
>> }
>>
>> +#define CET_XSTATE_MASK (XFEATURE_MASK_CET_USER)
>> +/*
>> + * Shadow Stack and Indirect Branch Tracking feature enabling depends on
>> + * whether host side CET user xstate bit is supported or not.
>> + */
>> +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.

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;

Powered by blists - more mailing lists