Re: [PATCH 10/11] x86/fpu: Remove unnecessary CPUID level check

Dave Hansen
2024-11-22T17:46:43Z

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On 11/21/24 07:45, Sean Christopherson wrote:
> On Wed, Nov 20, 2024, Dave Hansen wrote:
>> The CPUID level dependency table will entirely zap X86_FEATURE_XSAVE
>
> What table is that? XSAVE depends on FXSR, but I can't find anything that clears
> X86_FEATURE_XSAVE if cpuid_level < XSTATE_CPUID. Even if it did, dropping a
> sanity check in a one-time path adds risk for almost no reward.

arch/x86/kernel/cpu/common.c::cpuid_dependent_features[]

It's hard to find because it hard-codes the leaf number:

{ X86_FEATURE_XSAVE, 0x0000000d },

Fixing that was my initial motivation for this series.

As for removing the checks, I'd much rather have a super generic check
in the CPUID helpers that have all the callers code something.
Something like the attached patch?

[-- Attachment #2: cpuid_count-warn.patch --]
[-- Type: text/x-patch, Size: 2247 bytes --]

---

b/arch/x86/include/asm/cpuid.h | 6 ++++++
b/arch/x86/kernel/cpu/common.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+)

diff -puN arch/x86/include/asm/cpuid.h~cpuid_count-warn arch/x86/include/asm/cpuid.h
--- a/arch/x86/include/asm/cpuid.h~cpuid_count-warn 2024-11-22 08:50:12.618186610 -0800
+++ b/arch/x86/include/asm/cpuid.h 2024-11-22 09:10:12.112217942 -0800
@@ -64,6 +64,8 @@ native_cpuid_reg(edx)
#define __cpuid native_cpuid
#endif

+extern void check_cpuid_level(unsigned int level);
+
/*
* Generic CPUID function
* clear %ecx since some cpus (Cyrix MII) do not set or clear %ecx
@@ -73,6 +75,8 @@ static inline void cpuid(unsigned int op
unsigned int *eax, unsigned int *ebx,
unsigned int *ecx, unsigned int *edx)
{
+ check_cpuid_level(op);
+
*eax = op;
*ecx = 0;
__cpuid(eax, ebx, ecx, edx);
@@ -83,6 +87,8 @@ static inline void cpuid_count(unsigned
unsigned int *eax, unsigned int *ebx,
unsigned int *ecx, unsigned int *edx)
{
+ check_cpuid_level(op);
+
*eax = op;
*ecx = count;
__cpuid(eax, ebx, ecx, edx);
diff -puN arch/x86/kernel/cpu/common.c~cpuid_count-warn arch/x86/kernel/cpu/common.c
--- a/arch/x86/kernel/cpu/common.c~cpuid_count-warn 2024-11-22 09:07:43.922591720 -0800
+++ b/arch/x86/kernel/cpu/common.c 2024-11-22 09:42:48.950952538 -0800
@@ -2425,3 +2425,41 @@ void __init arch_cpu_finalize_init(void)
*/
mem_encrypt_init();
}
+
+void check_cpuid_level(unsigned int leaf)
+{
+ unsigned int region = leaf >> 4;
+ int max_leaf;
+
+ /*
+ * The max leaf in a region is discovered from the first
+ * leaf. Allow this kind of discovery without checks:
+ */
+ if (!(leaf & GENMASK(3, 0)))
+ return;
+
+ switch (region) {
+ case 0x0000:
+ max_leaf = boot_cpu_data.cpuid_level;
+ break;
+ case 0x8000:
+ max_leaf = boot_cpu_data.extended_cpuid_level;
+ break;
+ default:
+ /* Only check the basic and extended regions: */
+ return;
+ }
+
+ /*
+ * Skip checks before ->cpuid_level is populated
+ * and on CPUs without CPUID support:
+ */
+ if (!max_leaf)
+ return;
+
+ if (leaf <= max_leaf)
+ return;
+
+ WARN_ONCE(1, "CPUID read leaf 0x%x above max supported leaf: 0x%x",
+ leaf, max_leaf);
+}
_