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.
Signed-off-by: Yang Weijiang <[email protected]>
---
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,
};
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())
+ 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;
+}
+
extern unsigned int min_timer_period_us;
extern bool enable_vmware_backdoor;
--
2.27.0
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.
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.
>
>Signed-off-by: Yang Weijiang <[email protected]>
>---
> 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[].
> };
>
> 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)?
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.
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;
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 <[email protected]>
>> ---
>> 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;
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;
>
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.
On Thu, Aug 03, 2023, 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.
>
> Signed-off-by: Yang Weijiang <[email protected]>
> ---
> 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,
> };
>
> 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())
> + 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)
This is funky. As of this patch, KVM reports MSR_IA32_S_CET, a supervisor MSR,
but does not require XFEATURE_MASK_CET_KERNEL. That eventually comes along with
"KVM:x86: Enable guest CET supervisor xstate bit support", but as of this patch
KVM is busted.
The whole cpuid_count() code in that patch shouldn't exist, so the easiest thing
is to just fold the KVM_SUPPORTED_XSS and CET_XSTATE_MASK changes from that patch
into this one.
On 8/3/23 06:27, 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.
MSR_KVM_GUEST_SSP -> MSR_KVM_SSP
Also please add a comment,
/*
* SSP can only be read via RDSSP; writing even requires
* destructive and potentially faulting operations such as
* SAVEPREVSSP/RSTORSSP or SETSSBSY/CLRSSBSY. Let the host
* use a pseudo-MSR that is just a wrapper for the GUEST_SSP
* field of the VMCS.
*/
Paolo
On 8/4/23 20:51, Sean Christopherson wrote:
>>>>> + 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.
I think you three are talking past each other.
There are four cases:
- U_CET/S_CET supported by the host and exposed (obvious).
- U_CET/S_CET supported by the host, IBT or SHSTK partially exposed. The
MSRs should still be guest-accessible and bits that apply to absent features
should be reserved (bits 0-1 for SHSTK, bits 2-63 for IBT).
- U_CET/S_CET supported by the host, IBT or SHSTK not exposed. The MSRs
should still be host-accessible and writable to the default value. This is
clearer if you think that KVM_GET_MSR_INDEX_LIST is a system ioctl. Whether
to allow writing 0 from the guest is debatable.
- U_CET/S_CET not supported by the host. Then the MSRs should not be
enabled and should not be in KVM_GET_MSR_INDEX_LIST, and also IBT/SHSTK
should not be in KVM_GET_SUPPORTED_CPUID.
In my opinion it is reasonable to require both U_CET and S_CET to be
supported from the beginning in the host in order to support CET. It
is simpler and keeps the feature matrix at bay.
Paolo
On 8/4/2023 1:51 PM, 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?
Hold on until host_initiated access is finalized.
>>>> +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().
OK, maybe the helper is not necessary anymore, I will remove it, thank you!
>> 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;
On 8/5/2023 2:51 AM, Sean Christopherson wrote:
> 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.
OK, will use kvm_cpu_cap_has() to check KVM support before add CET MSRs to the lists.
>>>>> +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.
I see, thanks!
On 8/5/2023 2:55 AM, Sean Christopherson wrote:
> On Thu, Aug 03, 2023, 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.
>>
>> [...]
>> }
>> 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)
> This is funky. As of this patch, KVM reports MSR_IA32_S_CET, a supervisor MSR,
> but does not require XFEATURE_MASK_CET_KERNEL. That eventually comes along with
> "KVM:x86: Enable guest CET supervisor xstate bit support", but as of this patch
> KVM is busted.
>
> The whole cpuid_count() code in that patch shouldn't exist, so the easiest thing
> is to just fold the KVM_SUPPORTED_XSS and CET_XSTATE_MASK changes from that patch
> into this one.
I screwed it up when tried to make it clearer :-/
Will do it, thanks!
On 8/5/2023 5:47 AM, Paolo Bonzini wrote:
> On 8/3/23 06:27, 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.
>
> MSR_KVM_GUEST_SSP -> MSR_KVM_SSP
>
> Also please add a comment,
>
> /*
> * SSP can only be read via RDSSP; writing even requires
> * destructive and potentially faulting operations such as
> * SAVEPREVSSP/RSTORSSP or SETSSBSY/CLRSSBSY. Let the host
> * use a pseudo-MSR that is just a wrapper for the GUEST_SSP
> * field of the VMCS.
> */
>
OK, will take it, thanks!
> Paolo
>