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

[Dave Hansen <dave.hansen@intel.com>]

[-- 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);
+}
_