On Fri, Jan 08, 2021, Borislav Petkov wrote: > On Fri, Jan 08, 2021 at 08:03:50PM +1300, Kai Huang wrote: > > > > I am not sure changing reverse lookup to handle dynamic would be acceptable. To > > > > me it is ugly, and I don't have a first glance on how to do it. KVM can query > > > > host CPUID when dealing with SGX w/o X86_FEATURE_SGX1/2, but it is not as > > > > straightforward as having X86_FEATURE_SGX1/2. > > > > > > So, Boris was pretty direct here. Could you please go spend a bit of > > > time to see what it would take to make these dynamic? You can check > > > what our (Intel) plans are for this leaf, but if it's going to remain > > > sparsely-used, we need to look into making the leaves a bit more dynamic. To be fair, this is the third time we've got conflicting, direct feedback on this exact issue. I do agree that it doesn't make sense to burn a whole word for just two features, I guess I just feel like whining. [*] https://lore.kernel.org/kvm/20180828102140.GA31102@xxxxxxxxxxx/ [*] https://lore.kernel.org/linux-sgx/20190924162520.GJ19317@xxxxxxx/ > > I don't think reverse lookup can be made dyanmic, but like I said if we don't > > have X86_FEATURE_SGX1/2, KVM needs to query raw CPUID when dealing with SGX. > > How about before you go and say that "it is ugly" and "don't think can > be made" you actually go and *really* try it first? Because actually > trying is sometimes faster than trying to find arguments against it. :) > > Because I just did it and unless I'm missing something obvious - I > haven't actually tested it - this is not ugly at all and in the long run > it will become one big switch-case, which is perfectly fine. > > --- > diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h > index 59bf91c57aa8..0bf5cb5441f8 100644 > --- a/arch/x86/include/asm/cpufeature.h > +++ b/arch/x86/include/asm/cpufeature.h > @@ -30,6 +30,7 @@ enum cpuid_leafs > CPUID_7_ECX, > CPUID_8000_0007_EBX, > CPUID_7_EDX, > + CPUID_12_EAX, /* used only by KVM for now */ > }; > > #ifdef CONFIG_X86_FEATURE_NAMES > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h > index 84b887825f12..1bc1ade64489 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -292,6 +292,8 @@ > #define X86_FEATURE_FENCE_SWAPGS_KERNEL (11*32+ 5) /* "" LFENCE in kernel entry SWAPGS path */ > #define X86_FEATURE_SPLIT_LOCK_DETECT (11*32+ 6) /* #AC for split lock */ > #define X86_FEATURE_PER_THREAD_MBA (11*32+ 7) /* "" Per-thread Memory Bandwidth Allocation */ > +#define X86_FEATURE_SGX1 (11*32+ 8) /* SGX1 leaf functions */ > +#define X86_FEATURE_SGX2 (11*32+ 9) /* SGX2 leaf functions */ > > /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ > #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index dc921d76e42e..33c53a7411a1 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -63,8 +63,27 @@ static const struct cpuid_reg reverse_cpuid[] = { > [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX}, > [CPUID_7_EDX] = { 7, 0, CPUID_EDX}, > [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX}, > + [CPUID_12_EAX] = { 0x12, 0, CPUID_EAX}, > }; As is, this won't build (if KVM uses the features) because KVM cares about where the feature actually lives in Linux's words. The addition of CPUID_12_EAX is unnecessary, and the new entry in reverse_cpuid would need to be s/CPUID_12_EAX/CPUID_LNX_4. That being said, I dislike this approach as it introduces fragility into KVM's CPUID shenanigans. E.g. fixing the above will make guest_cpuid_has() functional, but kvm_cpu_cap_mask() and cpuid_entry_override() will not work as expected. Here's a more involved approach that I believe will work (compile tested only) and retains KVM's build magic. Idea is to allocate a word kvm_cpu_caps for the hardware-defined, Linux-scattered features, and use boot_cpu_has() to bridge the gap when populating kvm_cpu_caps. Another alternative would be to have KVM use boot_cpu_has() for everything, and omit the memcpy from boot_cpu_data.x86_capability -> kvm_cpu_caps. That would eliminate some of the special logic for scattered features, but it adds nearly 3k bytes to kvm_set_cpu_caps(), which is hard to stomach even though it's effectively one-and-done code. >From 6bdd61e23f1c0bd7519a3a6391c95cde5456f79d Mon Sep 17 00:00:00 2001 From: Sean Christopherson <seanjc@xxxxxxxxxx> Date: Fri, 8 Jan 2021 15:46:11 -0800 Subject: [PATCH] KVM: x86: Add support for reverse CPUID lookup of scattered features Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> --- arch/x86/include/asm/cpufeatures.h | 2 ++ arch/x86/kvm/cpuid.c | 36 ++++++++++++++++++--- arch/x86/kvm/cpuid.h | 50 +++++++++++++++++++++++++++--- 3 files changed, 78 insertions(+), 10 deletions(-) diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 9f9e9511f7cd..2fe57736d644 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -291,6 +291,8 @@ #define X86_FEATURE_FENCE_SWAPGS_KERNEL (11*32+ 5) /* "" LFENCE in kernel entry SWAPGS path */ #define X86_FEATURE_SPLIT_LOCK_DETECT (11*32+ 6) /* #AC for split lock */ #define X86_FEATURE_PER_THREAD_MBA (11*32+ 7) /* "" Per-thread Memory Bandwidth Allocation */ +#define X86_FEATURE_SGX1 (11*32+ 8) /* SGX1 leafs */ +#define X86_FEATURE_SGX2 (11*32+ 9) /* SGX2 leafs */ /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ #define X86_FEATURE_AVX512_BF16 (12*32+ 5) /* AVX512 BFLOAT16 instructions */ diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 13036cf0b912..4e647524f302 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -28,7 +28,7 @@ * Unlike "struct cpuinfo_x86.x86_capability", kvm_cpu_caps doesn't need to be * aligned to sizeof(unsigned long) because it's not accessed via bitops. */ -u32 kvm_cpu_caps[NCAPINTS] __read_mostly; +u32 kvm_cpu_caps[NR_KVM_CPU_CAPS] __read_mostly; EXPORT_SYMBOL_GPL(kvm_cpu_caps); static u32 xstate_required_size(u64 xstate_bv, bool compacted) @@ -53,6 +53,7 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted) } #define F feature_bit +#define SF(name) (boot_cpu_has(X86_FEATURE_##name) ? F(name) : 0) static inline struct kvm_cpuid_entry2 *cpuid_entry2_find( struct kvm_cpuid_entry2 *entries, int nent, u32 function, u32 index) @@ -331,13 +332,13 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu, return r; } -static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask) +/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ +static __always_inline void __kvm_cpu_cap_mask(enum cpuid_leafs leaf) { const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); struct kvm_cpuid_entry2 entry; reverse_cpuid_check(leaf); - kvm_cpu_caps[leaf] &= mask; cpuid_count(cpuid.function, cpuid.index, &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); @@ -345,6 +346,26 @@ static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask) kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg); } +static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask) +{ + /* Use the "init" variant for scattered leafs. */ + BUILD_BUG_ON(leaf >= NCAPINTS); + + kvm_cpu_caps[leaf] &= mask; + + __kvm_cpu_cap_mask(leaf); +} + +static __always_inline void kvm_cpu_cap_init(enum cpuid_leafs leaf, u32 mask) +{ + /* Use the "mask" variant for hardwared-defined leafs. */ + BUILD_BUG_ON(leaf < NCAPINTS); + + kvm_cpu_caps[leaf] = mask; + + __kvm_cpu_cap_mask(leaf); +} + void kvm_set_cpu_caps(void) { unsigned int f_nx = is_efer_nx() ? F(NX) : 0; @@ -355,12 +376,13 @@ void kvm_set_cpu_caps(void) unsigned int f_gbpages = 0; unsigned int f_lm = 0; #endif + memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); - BUILD_BUG_ON(sizeof(kvm_cpu_caps) > + BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > sizeof(boot_cpu_data.x86_capability)); memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, - sizeof(kvm_cpu_caps)); + sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); kvm_cpu_cap_mask(CPUID_1_ECX, /* @@ -503,6 +525,10 @@ void kvm_set_cpu_caps(void) F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) | F(PMM) | F(PMM_EN) ); + + kvm_cpu_cap_init(CPUID_12_EAX, + SF(SGX1) | SF(SGX2) + ); } EXPORT_SYMBOL_GPL(kvm_set_cpu_caps); diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index dc921d76e42e..21f92d81d5a5 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -7,7 +7,25 @@ #include <asm/processor.h> #include <uapi/asm/kvm_para.h> -extern u32 kvm_cpu_caps[NCAPINTS] __read_mostly; +/* + * Hardware-defined CPUID leafs that are scattered in the kernel, but need to + * be directly by KVM. Note, these word values conflict with the kernel's + * "bug" caps, but KVM doesn't use those. + */ +enum kvm_only_cpuid_leafs { + CPUID_12_EAX = NCAPINTS, + NR_KVM_CPU_CAPS, + + NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS, +}; + +#define X86_KVM_FEATURE(w, f) ((w)*32 + (f)) + +/* Intel-defined SGX sub-features, CPUID level 0x12 (EAX). */ +#define __X86_FEATURE_SGX1 X86_KVM_FEATURE(CPUID_12_EAX, 0) +#define __X86_FEATURE_SGX2 X86_KVM_FEATURE(CPUID_12_EAX, 1) + +extern u32 kvm_cpu_caps[NR_KVM_CPU_CAPS] __read_mostly; void kvm_set_cpu_caps(void); void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu); @@ -63,6 +81,7 @@ static const struct cpuid_reg reverse_cpuid[] = { [CPUID_8000_0007_EBX] = {0x80000007, 0, CPUID_EBX}, [CPUID_7_EDX] = { 7, 0, CPUID_EDX}, [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX}, + [CPUID_12_EAX] = {0x00000012, 0, CPUID_EAX}, }; /* @@ -83,6 +102,25 @@ static __always_inline void reverse_cpuid_check(unsigned int x86_leaf) BUILD_BUG_ON(reverse_cpuid[x86_leaf].function == 0); } +/* + * A handful of feature bits are scattered in the kernel's cpufeatures word, + * translate them to KVM features that align with the hardware definitions. + */ +static __always_inline u32 __feature_translate(int x86_feature) +{ + if (x86_feature == X86_FEATURE_SGX1) + return __X86_FEATURE_SGX1; + else if (x86_feature == X86_FEATURE_SGX2) + return __X86_FEATURE_SGX2; + + return x86_feature; +} + +static __always_inline u32 __feature_leaf(int x86_feature) +{ + return __feature_translate(x86_feature) / 32; +} + /* * Retrieve the bit mask from an X86_FEATURE_* definition. Features contain * the hardware defined bit number (stored in bits 4:0) and a software defined @@ -91,6 +129,8 @@ static __always_inline void reverse_cpuid_check(unsigned int x86_leaf) */ static __always_inline u32 __feature_bit(int x86_feature) { + x86_feature = __feature_translate(x86_feature); + reverse_cpuid_check(x86_feature / 32); return 1 << (x86_feature & 31); } @@ -99,7 +139,7 @@ static __always_inline u32 __feature_bit(int x86_feature) static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned int x86_feature) { - unsigned int x86_leaf = x86_feature / 32; + unsigned int x86_leaf = __feature_leaf(x86_feature); reverse_cpuid_check(x86_leaf); return reverse_cpuid[x86_leaf]; @@ -291,7 +331,7 @@ static inline bool cpuid_fault_enabled(struct kvm_vcpu *vcpu) static __always_inline void kvm_cpu_cap_clear(unsigned int x86_feature) { - unsigned int x86_leaf = x86_feature / 32; + unsigned int x86_leaf = __feature_leaf(x86_feature); reverse_cpuid_check(x86_leaf); kvm_cpu_caps[x86_leaf] &= ~__feature_bit(x86_feature); @@ -299,7 +339,7 @@ static __always_inline void kvm_cpu_cap_clear(unsigned int x86_feature) static __always_inline void kvm_cpu_cap_set(unsigned int x86_feature) { - unsigned int x86_leaf = x86_feature / 32; + unsigned int x86_leaf = __feature_leaf(x86_feature); reverse_cpuid_check(x86_leaf); kvm_cpu_caps[x86_leaf] |= __feature_bit(x86_feature); @@ -307,7 +347,7 @@ static __always_inline void kvm_cpu_cap_set(unsigned int x86_feature) static __always_inline u32 kvm_cpu_cap_get(unsigned int x86_feature) { - unsigned int x86_leaf = x86_feature / 32; + unsigned int x86_leaf = __feature_leaf(x86_feature); reverse_cpuid_check(x86_leaf); return kvm_cpu_caps[x86_leaf] & __feature_bit(x86_feature); -- 2.30.0.284.gd98b1dd5eaa7-goog
