Re: [PATCH v2 1/2] x86/cpu, kvm: Use CPU capabilities for CPUID[0x80000021].EAX

From: Kim Phillips
Date: Mon Nov 28 2022 - 18:00:57 EST


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@xxxxxxx>

verify_tags: WARNING: Co-developed-by Babu Moger <Babu.Moger@xxxxxxx> hasn't signed off on the patch!

OK, I'll add his signed-off-by.

Co-developed-by: Borislav Petkov <bp@xxxxxxx>
Signed-off-by: Kim Phillips <kim.phillips@xxxxxxx>

...

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