Messages in this thread | | | Date | Sun, 6 Aug 2023 16:54:43 +0800 | Subject | Re: [PATCH v5 08/19] KVM:x86: Report KVM supported CET MSRs as to-be-saved | From | "Yang, Weijiang" <> |
| |
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;
| |