| Message ID | 20221124000449.79014-2-kim.phillips@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu, kvm: Support AMD Automatic IBRS | expand |
On Wed, Nov 23, 2022 at 06:04:48PM -0600, Kim Phillips wrote: > The AMD Zen4 Automatic IBRS feature bit resides in the 0x80000021 leaf, > for which there is already support for exposing Zen3 bits to the guest. > > Add AMD AutoIBRS feature bit support, including for the other bits, > using scattered/synthetic bits. > > Add the corresponding word to KVM's feature machinery so that AutoIBRS > gets advertized into the guest too. > > Co-developed-by: Babu Moger <Babu.Moger@amd.com> verify_tags: WARNING: Co-developed-by Babu Moger <Babu.Moger@amd.com> hasn't signed off on the patch! > Co-developed-by: Borislav Petkov <bp@suse.de> > Signed-off-by: Kim Phillips <kim.phillips@amd.com> ... > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index c92c49a0b35b..61cd33a848cc 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -730,6 +730,25 @@ void kvm_set_cpu_caps(void) > 0 /* SME */ | F(SEV) | 0 /* VM_PAGE_FLUSH */ | F(SEV_ES) | > F(SME_COHERENT)); > > + /* > + * Pass down these bits: > + * EAX 0 NNDBP, Processor ignores nested data breakpoints > + * EAX 2 LAS, LFENCE always serializing > + * EAX 6 NSCB, Null selector clear base > + * EAX 8 Automatic IBRS > + * > + * Other defined bits are for MSRs that KVM does not expose: > + * EAX 3 SPCL, SMM page configuration lock > + * EAX 13 PCMSR, Prefetch control MSR > + */ > + kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX, > + SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | > + SF(NULL_SEL_CLR_BASE) | SF(AUTOIBRS)); > + if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) > + kvm_cpu_cap_set(X86_FEATURE_LFENCE_RDTSC); > + if (!static_cpu_has_bug(X86_BUG_NULL_SEG)) > + kvm_cpu_cap_set(X86_FEATURE_NULL_SEL_CLR_BASE); So this looks backwards: if X86_FEATURE_NULL_SEL_CLR_BASE is set, then X86_BUG_NULL_SEG should not be. Which means, you'd have to update check_null_seg_clears_base() too. Which means, you should make the X86_FEATURE_NULL_SEL_CLR_BASE bit addition a separate patch because this one is clearly doing too many things at once.
On Wed, Nov 23, 2022 at 06:04:48PM -0600, Kim Phillips wrote: > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index c92c49a0b35b..61cd33a848cc 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -730,6 +730,25 @@ void kvm_set_cpu_caps(void) > 0 /* SME */ | F(SEV) | 0 /* VM_PAGE_FLUSH */ | F(SEV_ES) | > F(SME_COHERENT)); > > + /* > + * Pass down these bits: > + * EAX 0 NNDBP, Processor ignores nested data breakpoints > + * EAX 2 LAS, LFENCE always serializing > + * EAX 6 NSCB, Null selector clear base > + * EAX 8 Automatic IBRS > + * > + * Other defined bits are for MSRs that KVM does not expose: > + * EAX 3 SPCL, SMM page configuration lock > + * EAX 13 PCMSR, Prefetch control MSR > + */ > + kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX, > + SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | > + SF(NULL_SEL_CLR_BASE) | SF(AUTOIBRS)); > + if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) Also: diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 61cd33a848cc..acda3883a905 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -744,7 +744,7 @@ void kvm_set_cpu_caps(void) kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX, SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | SF(NULL_SEL_CLR_BASE) | SF(AUTOIBRS)); - if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) + if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC))
On 11/24/22 6:57 AM, Borislav Petkov wrote: > On Wed, Nov 23, 2022 at 06:04:48PM -0600, Kim Phillips wrote: >> The AMD Zen4 Automatic IBRS feature bit resides in the 0x80000021 leaf, >> for which there is already support for exposing Zen3 bits to the guest. >> >> Add AMD AutoIBRS feature bit support, including for the other bits, >> using scattered/synthetic bits. >> >> Add the corresponding word to KVM's feature machinery so that AutoIBRS >> gets advertized into the guest too. >> >> Co-developed-by: Babu Moger <Babu.Moger@amd.com> > > verify_tags: WARNING: Co-developed-by Babu Moger <Babu.Moger@amd.com> hasn't signed off on the patch! OK, I'll add his signed-off-by. >> Co-developed-by: Borislav Petkov <bp@suse.de> >> Signed-off-by: Kim Phillips <kim.phillips@amd.com> > > ... > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index c92c49a0b35b..61cd33a848cc 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -730,6 +730,25 @@ void kvm_set_cpu_caps(void) >> 0 /* SME */ | F(SEV) | 0 /* VM_PAGE_FLUSH */ | F(SEV_ES) | >> F(SME_COHERENT)); >> >> + /* >> + * Pass down these bits: >> + * EAX 0 NNDBP, Processor ignores nested data breakpoints >> + * EAX 2 LAS, LFENCE always serializing >> + * EAX 6 NSCB, Null selector clear base >> + * EAX 8 Automatic IBRS >> + * >> + * Other defined bits are for MSRs that KVM does not expose: >> + * EAX 3 SPCL, SMM page configuration lock >> + * EAX 13 PCMSR, Prefetch control MSR >> + */ >> + kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX, >> + SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | >> + SF(NULL_SEL_CLR_BASE) | SF(AUTOIBRS)); >> + if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) >> + kvm_cpu_cap_set(X86_FEATURE_LFENCE_RDTSC); >> + if (!static_cpu_has_bug(X86_BUG_NULL_SEG)) >> + kvm_cpu_cap_set(X86_FEATURE_NULL_SEL_CLR_BASE); > > So this looks backwards: > > if X86_FEATURE_NULL_SEL_CLR_BASE is set, then X86_BUG_NULL_SEG should > not be. Not sure I follow. That code (originally from commit f144c49e8c39 ("KVM: x86: synthesize CPUID leaf 0x80000021h if useful") doesn't negate that: the code is saying that if we don't have the bug, then set the feature bit that says we don't have the bug. > Which means, you'd have to update check_null_seg_clears_base() too. Like this?: diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 73cc546e024d..bbe96d71ff5e 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -1682,11 +1682,6 @@ void check_null_seg_clears_base(struct cpuinfo_x86 *c) if (!IS_ENABLED(CONFIG_X86_64)) return; - /* Zen3 CPUs advertise Null Selector Clears Base in CPUID. */ - if (c->extended_cpuid_level >= 0x80000021 && - cpuid_eax(0x80000021) & BIT(6)) - return; - /* * CPUID bit above wasn't set. If this kernel is still running * as a HV guest, then the HV has decided not to advertize @@ -1700,11 +1695,13 @@ void check_null_seg_clears_base(struct cpuinfo_x86 *c) } /* + * Zen3+ CPUs advertise Null Selector Clears Base in CPUID. * Zen2 CPUs also have this behaviour, but no CPUID bit. * 0x18 is the respective family for Hygon. */ - if ((c->x86 == 0x17 || c->x86 == 0x18) && - detect_null_seg_behavior()) + if (cpu_has(X86_FEATURE_NULL_SEL_CLR_BASE) || + ((c->x86 == 0x17 || c->x86 == 0x18) && + detect_null_seg_behavior())) return; /* All the remaining ones are affected */ > Which means, you should make the X86_FEATURE_NULL_SEL_CLR_BASE bit > addition a separate patch because this one is clearly doing too many > things at once. OK. Thanks, Kim
On Mon, Nov 28, 2022 at 05:00:43PM -0600, Kim Phillips wrote: > > verify_tags: WARNING: Co-developed-by Babu Moger <Babu.Moger@amd.com> hasn't signed off on the patch! > > OK, I'll add his signed-off-by. You can't just add his SOB - he needs to give it himself. "Certificate of Origin" in Documentation/process/submitting-patches.rst needs brushing up on, it seems. > Not sure I follow. That code (originally from commit f144c49e8c39 > ("KVM: x86: synthesize CPUID leaf 0x80000021h if useful") doesn't > negate that: the code is saying that if we don't have the bug, then > set the feature bit that says we don't have the bug. I was thinking of the case described here: 415de4407664 ("x86/cpu: Fix migration safety with X86_BUG_NULL_SEL") but I guess we can do that on the host. > > Which means, you'd have to update check_null_seg_clears_base() too. > > Like this?: > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index 73cc546e024d..bbe96d71ff5e 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -1682,11 +1682,6 @@ void check_null_seg_clears_base(struct cpuinfo_x86 *c) > if (!IS_ENABLED(CONFIG_X86_64)) > return; > > - /* Zen3 CPUs advertise Null Selector Clears Base in CPUID. */ > - if (c->extended_cpuid_level >= 0x80000021 && > - cpuid_eax(0x80000021) & BIT(6)) > - return; > - No, not like this. The above you've removed needs to be if (cpu_has(c, X86_FEATURE_NULL_SEL_CLR_BASE)) return; so that you exit early. Thx.
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h index 759b71cb4f9a..961eb49532b7 100644 --- a/arch/x86/include/asm/cpufeatures.h +++ b/arch/x86/include/asm/cpufeatures.h @@ -306,9 +306,10 @@ #define X86_FEATURE_RSB_VMEXIT_LITE (11*32+17) /* "" Fill RSB on VM exit when EIBRS is enabled */ #define X86_FEATURE_SGX_EDECCSSA (11*32+18) /* "" SGX EDECCSSA user leaf function */ #define X86_FEATURE_CALL_DEPTH (11*32+19) /* "" Call depth tracking for RSB stuffing */ - - #define X86_FEATURE_MSR_TSX_CTRL (11*32+20) /* "" MSR IA32_TSX_CTRL (Intel) implemented */ +#define X86_FEATURE_NO_NESTED_DATA_BP (11*32+21) /* "" AMD No Nested Data Breakpoints */ +#define X86_FEATURE_NULL_SEL_CLR_BASE (11*32+22) /* "" AMD Null Selector Clears Base */ +#define X86_FEATURE_AUTOIBRS (11*32+23) /* AMD Automatic IBRS */ /* Intel-defined CPU features, CPUID level 0x00000007:1 (EAX), word 12 */ #define X86_FEATURE_AVX_VNNI (12*32+ 4) /* AVX VNNI instructions */ diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c index f53944fb8f7f..7ae7203cd410 100644 --- a/arch/x86/kernel/cpu/scattered.c +++ b/arch/x86/kernel/cpu/scattered.c @@ -45,6 +45,10 @@ static const struct cpuid_bit cpuid_bits[] = { { X86_FEATURE_CPB, CPUID_EDX, 9, 0x80000007, 0 }, { X86_FEATURE_PROC_FEEDBACK, CPUID_EDX, 11, 0x80000007, 0 }, { X86_FEATURE_MBA, CPUID_EBX, 6, 0x80000008, 0 }, + { X86_FEATURE_NO_NESTED_DATA_BP,CPUID_EAX, 0, 0x80000021, 0 }, + { X86_FEATURE_LFENCE_RDTSC, CPUID_EAX, 2, 0x80000021, 0 }, + { X86_FEATURE_NULL_SEL_CLR_BASE,CPUID_EAX, 6, 0x80000021, 0 }, + { X86_FEATURE_AUTOIBRS, CPUID_EAX, 8, 0x80000021, 0 }, { X86_FEATURE_PERFMON_V2, CPUID_EAX, 0, 0x80000022, 0 }, { X86_FEATURE_AMD_LBR_V2, CPUID_EAX, 1, 0x80000022, 0 }, { 0, 0, 0, 0, 0 } diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index c92c49a0b35b..61cd33a848cc 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -730,6 +730,25 @@ void kvm_set_cpu_caps(void) 0 /* SME */ | F(SEV) | 0 /* VM_PAGE_FLUSH */ | F(SEV_ES) | F(SME_COHERENT)); + /* + * Pass down these bits: + * EAX 0 NNDBP, Processor ignores nested data breakpoints + * EAX 2 LAS, LFENCE always serializing + * EAX 6 NSCB, Null selector clear base + * EAX 8 Automatic IBRS + * + * Other defined bits are for MSRs that KVM does not expose: + * EAX 3 SPCL, SMM page configuration lock + * EAX 13 PCMSR, Prefetch control MSR + */ + kvm_cpu_cap_init_scattered(CPUID_8000_0021_EAX, + SF(NO_NESTED_DATA_BP) | SF(LFENCE_RDTSC) | + SF(NULL_SEL_CLR_BASE) | SF(AUTOIBRS)); + if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) + kvm_cpu_cap_set(X86_FEATURE_LFENCE_RDTSC); + if (!static_cpu_has_bug(X86_BUG_NULL_SEG)) + kvm_cpu_cap_set(X86_FEATURE_NULL_SEL_CLR_BASE); + kvm_cpu_cap_mask(CPUID_C000_0001_EDX, F(XSTORE) | F(XSTORE_EN) | F(XCRYPT) | F(XCRYPT_EN) | F(ACE2) | F(ACE2_EN) | F(PHE) | F(PHE_EN) | @@ -1211,21 +1230,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function) break; case 0x80000021: entry->ebx = entry->ecx = entry->edx = 0; - /* - * Pass down these bits: - * EAX 0 NNDBP, Processor ignores nested data breakpoints - * EAX 2 LAS, LFENCE always serializing - * EAX 6 NSCB, Null selector clear base - * - * Other defined bits are for MSRs that KVM does not expose: - * EAX 3 SPCL, SMM page configuration lock - * EAX 13 PCMSR, Prefetch control MSR - */ - entry->eax &= BIT(0) | BIT(2) | BIT(6); - if (static_cpu_has(X86_FEATURE_LFENCE_RDTSC)) - entry->eax |= BIT(2); - if (!static_cpu_has_bug(X86_BUG_NULL_SEG)) - entry->eax |= BIT(6); + cpuid_entry_override(entry, CPUID_8000_0021_EAX); break; /*Add support for Centaur's CPUID instruction*/ case 0xC0000000: diff --git a/arch/x86/kvm/reverse_cpuid.h b/arch/x86/kvm/reverse_cpuid.h index 4e5b8444f161..0bf02c02bb0a 100644 --- a/arch/x86/kvm/reverse_cpuid.h +++ b/arch/x86/kvm/reverse_cpuid.h @@ -13,6 +13,7 @@ */ enum kvm_only_cpuid_leafs { CPUID_12_EAX = NCAPINTS, + CPUID_8000_0021_EAX, NR_KVM_CPU_CAPS, NKVMCAPINTS = NR_KVM_CPU_CAPS - NCAPINTS, @@ -25,6 +26,12 @@ enum kvm_only_cpuid_leafs { #define KVM_X86_FEATURE_SGX2 KVM_X86_FEATURE(CPUID_12_EAX, 1) #define KVM_X86_FEATURE_SGX_EDECCSSA KVM_X86_FEATURE(CPUID_12_EAX, 11) +/* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX) */ +#define KVM_X86_FEATURE_NO_NESTED_DATA_BP KVM_X86_FEATURE(CPUID_8000_0021_EAX, 0) +#define KVM_X86_FEATURE_LFENCE_RDTSC KVM_X86_FEATURE(CPUID_8000_0021_EAX, 2) +#define KVM_X86_FEATURE_NULL_SEL_CLR_BASE KVM_X86_FEATURE(CPUID_8000_0021_EAX, 6) +#define KVM_X86_FEATURE_AUTOIBRS KVM_X86_FEATURE(CPUID_8000_0021_EAX, 8) + struct cpuid_reg { u32 function; u32 index; @@ -49,6 +56,7 @@ static const struct cpuid_reg reverse_cpuid[] = { [CPUID_7_1_EAX] = { 7, 1, CPUID_EAX}, [CPUID_12_EAX] = {0x00000012, 0, CPUID_EAX}, [CPUID_8000_001F_EAX] = {0x8000001f, 0, CPUID_EAX}, + [CPUID_8000_0021_EAX] = {0x80000021, 0, CPUID_EAX}, }; /* @@ -75,12 +83,16 @@ static __always_inline void reverse_cpuid_check(unsigned int x86_leaf) */ static __always_inline u32 __feature_translate(int x86_feature) { - if (x86_feature == X86_FEATURE_SGX1) - return KVM_X86_FEATURE_SGX1; - else if (x86_feature == X86_FEATURE_SGX2) - return KVM_X86_FEATURE_SGX2; - else if (x86_feature == X86_FEATURE_SGX_EDECCSSA) - return KVM_X86_FEATURE_SGX_EDECCSSA; + switch (x86_feature) { + case X86_FEATURE_SGX1: return KVM_X86_FEATURE_SGX1; + case X86_FEATURE_SGX2: return KVM_X86_FEATURE_SGX2; + case X86_FEATURE_SGX_EDECCSSA: return KVM_X86_FEATURE_SGX_EDECCSSA; + case X86_FEATURE_NO_NESTED_DATA_BP: return KVM_X86_FEATURE_NO_NESTED_DATA_BP; + case X86_FEATURE_LFENCE_RDTSC: return KVM_X86_FEATURE_LFENCE_RDTSC; + case X86_FEATURE_NULL_SEL_CLR_BASE: return KVM_X86_FEATURE_NULL_SEL_CLR_BASE; + case X86_FEATURE_AUTOIBRS: return KVM_X86_FEATURE_AUTOIBRS; + default: break; + } return x86_feature; }
The AMD Zen4 Automatic IBRS feature bit resides in the 0x80000021 leaf, for which there is already support for exposing Zen3 bits to the guest. Add AMD AutoIBRS feature bit support, including for the other bits, using scattered/synthetic bits. Add the corresponding word to KVM's feature machinery so that AutoIBRS gets advertized into the guest too. Co-developed-by: Babu Moger <Babu.Moger@amd.com> Co-developed-by: Borislav Petkov <bp@suse.de> Signed-off-by: Kim Phillips <kim.phillips@amd.com> --- v1: https://lore.kernel.org/lkml/20221104213651.141057-2-kim.phillips@amd.com/, and https://lore.kernel.org/lkml/20221104213651.141057-4-kim.phillips@amd.com/ v2: Addressed v1 comments: - Use synthetic/scattered bits instead of introducing new leaf [Boris] - Combine the rest of the leaf's bits being used [Paolo] Note: Bits not used by the host can be moved to kvm/cpuid.c if maintainers do not want them in cpufeatures.h. - Hoist bitsetting code to kvm_set_cpu_caps(), and use cpuid_entry_override() in __do_cpuid_func() [Paolo] arch/x86/include/asm/cpufeatures.h | 5 +++-- arch/x86/kernel/cpu/scattered.c | 4 ++++ arch/x86/kvm/cpuid.c | 35 +++++++++++++++++------------- arch/x86/kvm/reverse_cpuid.h | 24 +++++++++++++++----- 4 files changed, 45 insertions(+), 23 deletions(-)