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 <weijiang.yang@xxxxxxxxx>
>---
> 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;
