Code in v6.9 arch/x86/kernel/smpboot.c was changed by commit 4db64279bc2b
("x86/cpu: Switch to new Intel CPU model defines") from old code:
440 static const struct x86_cpu_id intel_cod_cpu[] = {
441 X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0), /* COD */
442 X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0), /* COD */
443 X86_MATCH_INTEL_FAM6_MODEL(ANY, 1), /* SNC */
444 {}
445 };
446
447 static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
448 {
449 const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
new code:
440 static const struct x86_cpu_id intel_cod_cpu[] = {
441 X86_MATCH_VFM(INTEL_HASWELL_X, 0), /* COD */
442 X86_MATCH_VFM(INTEL_BROADWELL_X, 0), /* COD */
443 X86_MATCH_VFM(INTEL_ANY, 1), /* SNC */
444 {}
445 };
446
447 static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
448 {
449 const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
On an Intel CPU with SNC enabled this code previously matched the rule
on line 443 to avoid printing messages about insane cache configuration.
The new code did not match any rules.
Expanding the macros for the intel_cod_cpu[] array shows that the old
is equivalent to:
static const struct x86_cpu_id intel_cod_cpu[] = {
[0] = { .vendor = 0, .family = 6, .model = 0x3F, .steppings = 0, .feature = 0, .driver_data = 0 },
[1] = { .vendor = 0, .family = 6, .model = 0x4F, .steppings = 0, .feature = 0, .driver_data = 0 },
[2] = { .vendor = 0, .family = 6, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 1 },
[3] = { .vendor = 0, .family = 0, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 0 }
}
while the new code expands to:
static const struct x86_cpu_id intel_cod_cpu[] = {
[0] = { .vendor = 0, .family = 6, .model = 0x3F, .steppings = 0, .feature = 0, .driver_data = 0 },
[1] = { .vendor = 0, .family = 6, .model = 0x4F, .steppings = 0, .feature = 0, .driver_data = 0 },
[2] = { .vendor = 0, .family = 0, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 1 },
[3] = { .vendor = 0, .family = 0, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 0 }
}
Looking at the code for x86_match_cpu():
36 const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
37 {
38 const struct x86_cpu_id *m;
39 struct cpuinfo_x86 *c = &boot_cpu_data;
40
41 for (m = match;
42 m->vendor | m->family | m->model | m->steppings | m->feature;
43 m++) {
...
56 }
57 return NULL;
58 }
59 EXPORT_SYMBOL(x86_match_cpu);
it is clear that there was no match because the ANY entry in the table
(array index 2) is now the loop termination condition (all of vendor,
family, model, steppings, and feature are zero).
So this code was working before because the "ANY" check was looking for
any Intel CPU in family 6. But fails now because the family is a wild
card. So the root cause is that x86_match_cpu() has never been able to
match on a rule with just X86_VENDOR_INTEL and all other fields set to
wildcards.
Fix by adding a new flags field to struct x86_cpu_id that has a bit set
to indicate that this entry in the array is valid. Update X86_MATCH*()
macros to set that bit. Chenge the end-marker check in x86_match_cpu()
to just check the flags field for this bit.
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Fixes: 644e9cbbe3fc ("Add driver auto probing for x86 features v4")
Signed-off-by: Tony Luck <[email protected]>
---
---
include/linux/mod_devicetable.h | 4 ++++
arch/x86/include/asm/cpu_device_id.h | 2 ++
arch/x86/kernel/cpu/match.c | 4 +---
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7a9a07ea451b..ca3468ad06ff 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -690,6 +690,7 @@ struct x86_cpu_id {
__u16 model;
__u16 steppings;
__u16 feature; /* bit index */
+ __u16 flags;
kernel_ulong_t driver_data;
};
@@ -700,6 +701,9 @@ struct x86_cpu_id {
#define X86_STEPPING_ANY 0
#define X86_FEATURE_ANY 0 /* Same as FPU, you can't test for that */
+/* x86_cpu_id::flags */
+#define X86_CPU_ID_FLAG_ENTRY_VALID BIT(0)
+
/*
* Generic table type for matching CPU features.
* @feature: the bit number of the feature (0 - 65535)
diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index 970a232009c3..54a71c669ce9 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -79,6 +79,7 @@
.model = _model, \
.steppings = _steppings, \
.feature = _feature, \
+ .flags = X86_CPU_ID_FLAG_ENTRY_VALID, \
.driver_data = (unsigned long) _data \
}
@@ -89,6 +90,7 @@
.model = _model, \
.steppings = _steppings, \
.feature = _feature, \
+ .flags = X86_CPU_ID_FLAG_ENTRY_VALID, \
.driver_data = (unsigned long) _data \
}
diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
index 8651643bddae..8e7de733320a 100644
--- a/arch/x86/kernel/cpu/match.c
+++ b/arch/x86/kernel/cpu/match.c
@@ -38,9 +38,7 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
const struct x86_cpu_id *m;
struct cpuinfo_x86 *c = &boot_cpu_data;
- for (m = match;
- m->vendor | m->family | m->model | m->steppings | m->feature;
- m++) {
+ for (m = match; m->flags & X86_CPU_ID_FLAG_ENTRY_VALID; m++) {
if (m->vendor != X86_VENDOR_ANY && c->x86_vendor != m->vendor)
continue;
if (m->family != X86_FAMILY_ANY && c->x86 != m->family)
--
2.45.0
On Mon, May 20, 2024 at 03:45:33PM -0700, Tony Luck wrote:
> Fixes: 644e9cbbe3fc ("Add driver auto probing for x86 features v4")
Do you really want to backport this to everything since 2012, as that
patch is from then?
> @@ -690,6 +690,7 @@ struct x86_cpu_id {
> __u16 model;
> __u16 steppings;
> __u16 feature; /* bit index */
> + __u16 flags;
> kernel_ulong_t driver_data;
> };
>
> @@ -700,6 +701,9 @@ struct x86_cpu_id {
> #define X86_STEPPING_ANY 0
> #define X86_FEATURE_ANY 0 /* Same as FPU, you can't test for that */
>
> +/* x86_cpu_id::flags */
> +#define X86_CPU_ID_FLAG_ENTRY_VALID BIT(0)
I would definitely not want to have those visible in userspace.
IOW, something like this:
diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index 54a71c669ce9..b6325ee30871 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -53,6 +53,9 @@
#define X86_CENTAUR_FAM6_C7_D 0xd
#define X86_CENTAUR_FAM6_NANO 0xf
+/* x86_cpu_id::flags */
+#define X86_CPU_ID_FLAG_ENTRY_VALID BIT(0)
+
#define X86_STEPPINGS(mins, maxs) GENMASK(maxs, mins)
/**
* X86_MATCH_VENDOR_FAM_MODEL_STEPPINGS_FEATURE - Base macro for CPU matching
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index ca3468ad06ff..4338b1b4ac44 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -690,6 +690,7 @@ struct x86_cpu_id {
__u16 model;
__u16 steppings;
__u16 feature; /* bit index */
+ /* Solely for kernel-internal use: DO NOT EXPORT to userspace! */
__u16 flags;
kernel_ulong_t driver_data;
};
@@ -701,9 +702,6 @@ struct x86_cpu_id {
#define X86_STEPPING_ANY 0
#define X86_FEATURE_ANY 0 /* Same as FPU, you can't test for that */
-/* x86_cpu_id::flags */
-#define X86_CPU_ID_FLAG_ENTRY_VALID BIT(0)
-
/*
* Generic table type for matching CPU features.
* @feature: the bit number of the feature (0 - 65535)
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, May 21, 2024 at 09:49:47AM +0200, Borislav Petkov wrote:
> On Mon, May 20, 2024 at 03:45:33PM -0700, Tony Luck wrote:
> > Fixes: 644e9cbbe3fc ("Add driver auto probing for x86 features v4")
>
> Do you really want to backport this to everything since 2012, as that
> patch is from then?
I didn't include a Cc: stable. Is there some better way to report
the source commit for a problem without triggering a backport?
>
> > @@ -690,6 +690,7 @@ struct x86_cpu_id {
> > __u16 model;
> > __u16 steppings;
> > __u16 feature; /* bit index */
> > + __u16 flags;
> > kernel_ulong_t driver_data;
> > };
> >
> > @@ -700,6 +701,9 @@ struct x86_cpu_id {
> > #define X86_STEPPING_ANY 0
> > #define X86_FEATURE_ANY 0 /* Same as FPU, you can't test for that */
> >
> > +/* x86_cpu_id::flags */
> > +#define X86_CPU_ID_FLAG_ENTRY_VALID BIT(0)
>
> I would definitely not want to have those visible in userspace.
>
> IOW, something like this:
Agreed. Looks better to keep the define out of a <linux/*.h> file.
Do you want me to spin a new patch? Or can you fold your change into
my patch when applying?
-Tony
On Tue, May 21, 2024 at 08:48:34AM -0700, Tony Luck wrote:
> I didn't include a Cc: stable. Is there some better way to report
> the source commit for a problem without triggering a backport?
Looking at:
Documentation/process/stable-kernel-rules.rst
I guess this:
"There furthermore is a variant of the stable tag you can use to make the stable
team's backporting tools (e.g AUTOSEL or scripts that look for commits
containing a 'Fixes:' tag) ignore a change::
Cc: <[email protected]> # reason goes here, and must be present"
Might want to explain in that reason there what the situation is and
that this patch should be backported only when the SNC change is in the
tree or so, yadda yadda.
Or the crypto one - your patch 1 in this thread.
> Agreed. Looks better to keep the define out of a <linux/*.h> file.
>
> Do you want me to spin a new patch? Or can you fold your change into
> my patch when applying?
Nah, I can fold everything in.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette