Code in v.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 = 0 },
[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 = 0 },
[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 changing X86_VENDOR_INTEL to a non-zero value (4 was lowest
unused value, so I picked that).
Fixes: 644e9cbbe3fc ("Add driver auto probing for x86 features v4")
Signed-off-by: Tony Luck <[email protected]>
---
Changes since v1:
1) More detailed commit description.
2) Changed "Fixes" tag. Commit 4db64279bc2b merely revealed a twelve
year old gap in the implementation of x86_match_cpu().
arch/x86/include/asm/processor.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index cb4f6c513c48..271c4c95bc37 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -175,10 +175,10 @@ struct cpuinfo_x86 {
unsigned initialized : 1;
} __randomize_layout;
-#define X86_VENDOR_INTEL 0
#define X86_VENDOR_CYRIX 1
#define X86_VENDOR_AMD 2
#define X86_VENDOR_UMC 3
+#define X86_VENDOR_INTEL 4
#define X86_VENDOR_CENTAUR 5
#define X86_VENDOR_TRANSMETA 7
#define X86_VENDOR_NSC 8
--
2.44.0
Couple of typos in this:
> Code in v.9 arch/x86/kernel/smpboot.c was changed by commit 4db64279bc2b
Should say v6.9
> [2] = { .vendor = 0, .family = 6, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 0 },
driver_data = 1 (not zero)
> [2] = { .vendor = 0, .family = 0, .model = 0x00, .steppings = 0, .feature = 0, .driver_data = 0 },
ditto.
-Tony
On Thu, May 16, 2024 at 09:29:25AM -0700, Tony Luck wrote:
> -#define X86_VENDOR_INTEL 0
> #define X86_VENDOR_CYRIX 1
> #define X86_VENDOR_AMD 2
> #define X86_VENDOR_UMC 3
> +#define X86_VENDOR_INTEL 4
> #define X86_VENDOR_CENTAUR 5
> #define X86_VENDOR_TRANSMETA 7
> #define X86_VENDOR_NSC 8
> --
From my last review:
> But the Intel vendor has been 0 for what, 30 years?
> Are you sure no other code in the tree relies on that? Audited?
But nope, apparently that's too much to ask. :-(
modinfo ./arch/x86/events/intel/intel-uncore.ko
filename: ./arch/x86/events/intel/intel-uncore.ko
license: GPL
srcversion: ECE38449B18DD83223B93FD
alias: cpu:type:x86,ven0000fam0006mod00B6:feature:*
alias: cpu:type:x86,ven0000fam0006mod00AF:feature:*
alias: cpu:type:x86,ven0000fam0006mod00BE:feature:*
^^^^^^^^^
Would everything still work if it said "ven0004" now?
So tglx and I just did some poking and we think the best solution would
be to add a __u16 flags field to struct x86_cpu_id right...
struct x86_cpu_id {
__u16 vendor; /* 0 2 */
__u16 family; /* 2 2 */
__u16 model; /* 4 2 */
__u16 steppings; /* 6 2 */
__u16 feature; /* 8 2 */
/* XXX 6 bytes hole, try to pack */
<--- HERE
kernel_ulong_t driver_data; /* 16 8 */
/* size: 24, cachelines: 1, members: 6 */
/* sum members: 18, holes: 1, sum holes: 6 */
/* last cacheline: 24 bytes */
};
and the 32-bit version has the same hole:
struct x86_cpu_id {
__u16 vendor; /* 0 2 */
__u16 family; /* 2 2 */
__u16 model; /* 4 2 */
__u16 steppings; /* 6 2 */
__u16 feature; /* 8 2 */
/* XXX 2 bytes hole, try to pack */
<--- HERE
kernel_ulong_t driver_data; /* 12 4 */
/* size: 16, cachelines: 1, members: 6 */
/* sum members: 14, holes: 1, sum holes: 2 */
/* last cacheline: 16 bytes */
};
And then do:
struct x86_cpu_id {
__u16 vendor;
__u16 family;
__u16 model;
__u16 steppings;
__u16 feature; /* bit index */
__u16 flags;
kernel_ulong_t driver_data;
};
#define X86_CPU_ID_FLAG_VENDOR_VALID BIT(0)
and then have the macros in arch/x86/include/asm/cpu_device_id.h set
that valid flag and then have x86_match_cpu() check it.
Then you don't risk a userspace breakage and that x86_match_cpu() crap
thing is fixed.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On May 17, 2024 7:43:12 AM PDT, Borislav Petkov <[email protected]> wrote:
>On Thu, May 16, 2024 at 09:29:25AM -0700, Tony Luck wrote:
>> -#define X86_VENDOR_INTEL 0
>> #define X86_VENDOR_CYRIX 1
>> #define X86_VENDOR_AMD 2
>> #define X86_VENDOR_UMC 3
>> +#define X86_VENDOR_INTEL 4
>> #define X86_VENDOR_CENTAUR 5
>> #define X86_VENDOR_TRANSMETA 7
>> #define X86_VENDOR_NSC 8
>> --
>
>From my last review:
>
>> But the Intel vendor has been 0 for what, 30 years?
>
>> Are you sure no other code in the tree relies on that? Audited?
>
>But nope, apparently that's too much to ask. :-(
>
>modinfo ./arch/x86/events/intel/intel-uncore.ko
>filename: ./arch/x86/events/intel/intel-uncore.ko
>license: GPL
>srcversion: ECE38449B18DD83223B93FD
>alias: cpu:type:x86,ven0000fam0006mod00B6:feature:*
>alias: cpu:type:x86,ven0000fam0006mod00AF:feature:*
>alias: cpu:type:x86,ven0000fam0006mod00BE:feature:*
> ^^^^^^^^^
>
>Would everything still work if it said "ven0004" now?
>
>So tglx and I just did some poking and we think the best solution would
>be to add a __u16 flags field to struct x86_cpu_id right...
>
>struct x86_cpu_id {
> __u16 vendor; /* 0 2 */
> __u16 family; /* 2 2 */
> __u16 model; /* 4 2 */
> __u16 steppings; /* 6 2 */
> __u16 feature; /* 8 2 */
>
> /* XXX 6 bytes hole, try to pack */
>
><--- HERE
>
> kernel_ulong_t driver_data; /* 16 8 */
>
> /* size: 24, cachelines: 1, members: 6 */
> /* sum members: 18, holes: 1, sum holes: 6 */
> /* last cacheline: 24 bytes */
>};
>
>and the 32-bit version has the same hole:
>
>struct x86_cpu_id {
> __u16 vendor; /* 0 2 */
> __u16 family; /* 2 2 */
> __u16 model; /* 4 2 */
> __u16 steppings; /* 6 2 */
> __u16 feature; /* 8 2 */
>
> /* XXX 2 bytes hole, try to pack */
>
><--- HERE
>
> kernel_ulong_t driver_data; /* 12 4 */
>
> /* size: 16, cachelines: 1, members: 6 */
> /* sum members: 14, holes: 1, sum holes: 2 */
> /* last cacheline: 16 bytes */
>};
>
>And then do:
>
>struct x86_cpu_id {
> __u16 vendor;
> __u16 family;
> __u16 model;
> __u16 steppings;
> __u16 feature; /* bit index */
> __u16 flags;
> kernel_ulong_t driver_data;
>};
>
>#define X86_CPU_ID_FLAG_VENDOR_VALID BIT(0)
>
>and then have the macros in arch/x86/include/asm/cpu_device_id.h set
>that valid flag and then have x86_match_cpu() check it.
>
>Then you don't risk a userspace breakage and that x86_match_cpu() crap
>thing is fixed.
>
>Thx.
>
I'm confused. Why not simply use say -1 for wildcard vendor match, -2 for no vendor ID (no CPUID or other known probing mechanism) and -3 for unrecognized vendor (vendor detectable but not known.)
If we have a match/valid mask, I would suggest that we have *separate* match and valid masks, so that we can explicitly encode the condition of "no valid vendor", but in the specific case of vendor, there are two conditions (see above.)
I *hate* these strings with the passion of a thousand suns: they are a classic case of how just blindly converting binary information to hex adds absolutely no value, and often makes the result worse than what one started with. And yes, I complained about that when they first went in as a classic case of exposing what was always simply intended as a kernel internal interface to user space.
This is particularly pathetic as there already is a canonical string representation of the vendor ID!
Similarly, kernel internal CPUID feature numbers shouldn't be an API, we once again have canonical strings for the API level.
But of course, now the module utilities depend on them, and although they are relatively tightly coupled to the kernel, it still would require a staged transition where the legacy strings are included for some time.
> I'm confused. Why not simply use say -1 for wildcard vendor match, -2 for no vendor ID (no CPUID or other known probing mechanism) and -3 for unrecognized vendor (vendor detectable but not known.)
It was really convenient to have "0" be the wildcard for all of vendor, family, model, stepping, feature because users of x86_match_cpu() could just initialize the fields they cared about in the struct x86_cpu_id and have the compiler make the rest be 0 automagically.
But X86_VENDOR_INTEL being zero has always been a thorn in that scheme.
-Tony
On Fri, May 17 2024 at 10:35, H. Peter Anvin wrote:
> On May 17, 2024 7:43:12 AM PDT, Borislav Petkov <[email protected]> wrote:
>>And then do:
>>
>>struct x86_cpu_id {
>> __u16 vendor;
>> __u16 family;
>> __u16 model;
>> __u16 steppings;
>> __u16 feature; /* bit index */
>> __u16 flags;
>> kernel_ulong_t driver_data;
>>};
>>
>>#define X86_CPU_ID_FLAG_VENDOR_VALID BIT(0)
>>
>>and then have the macros in arch/x86/include/asm/cpu_device_id.h set
>>that valid flag and then have x86_match_cpu() check it.
>>
>>Then you don't risk a userspace breakage and that x86_match_cpu() crap
>>thing is fixed.
>
> I'm confused. Why not simply use say -1 for wildcard vendor match, -2
> for no vendor ID (no CPUID or other known probing mechanism) and -3
> for unrecognized vendor (vendor detectable but not known.)
This has nothing to do with wildcards.
The problem at hand is about loop termination. Making that explicit by
having a valid bit in the existing struct hole is trivial, straight
forward and just works obviously correct.
> I *hate* these strings with the passion of a thousand suns: they are a
> classic case of how just blindly converting binary information to hex
> adds absolutely no value, and often makes the result worse than what
> one started with. And yes, I complained about that when they first
> went in as a classic case of exposing what was always simply intended
> as a kernel internal interface to user space.
You obviously did not complain loud enough :)
> This is particularly pathetic as there already is a canonical string
> representation of the vendor ID!
I agree, but that train has left the station long ago,
Thanks,
tglx
On Fri, May 17 2024 at 18:17, Luck, Tony wrote:
>> I'm confused. Why not simply use say -1 for wildcard vendor match, -2
>> for no vendor ID (no CPUID or other known probing mechanism) and -3
>> for unrecognized vendor (vendor detectable but not known.)
>
> It was really convenient to have "0" be the wildcard for all of
> vendor, family, model, stepping, feature because users of
> x86_match_cpu() could just initialize the fields they cared about in
> the struct x86_cpu_id and have the compiler make the rest be 0
> automagically.
>
> But X86_VENDOR_INTEL being zero has always been a thorn in that scheme.
As all initializers are macro based, that's really a non-problem.
Thanks,
tglx
>> This is particularly pathetic as there already is a canonical string
>> representation of the vendor ID!
>
> I agree, but that train has left the station long ago,
Is that "GenuineIntel" and "AuthenticAMD"? While canonical, 96-bit values seem less
easy to use than simple small integers.
-Tony
On May 17, 2024 1:54:33 PM PDT, "Luck, Tony" <[email protected]> wrote:
>>> This is particularly pathetic as there already is a canonical string
>>> representation of the vendor ID!
>>
>> I agree, but that train has left the station long ago,
>
>Is that "GenuineIntel" and "AuthenticAMD"? While canonical, 96-bit values seem less
>easy to use than simple small integers.
>
>-Tony
>
Not for internal use, but for exporting in strings.
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 93022482b2948a9a7e9b5a2bb685f2e1cb4c3348
Gitweb: https://git.kernel.org/tip/93022482b2948a9a7e9b5a2bb685f2e1cb4c3348
Author: Tony Luck <[email protected]>
AuthorDate: Mon, 20 May 2024 15:45:33 -07:00
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Wed, 22 May 2024 11:31:10 +02:00
x86/cpu: Fix x86_match_cpu() to match just X86_VENDOR_INTEL
Code in v6.9 arch/x86/kernel/smpboot.c was changed by commit
4db64279bc2b ("x86/cpu: Switch to new Intel CPU model defines") from:
static const struct x86_cpu_id intel_cod_cpu[] = {
X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0), /* COD */
X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0), /* COD */
X86_MATCH_INTEL_FAM6_MODEL(ANY, 1), /* SNC */ <--- 443
{}
};
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
to:
static const struct x86_cpu_id intel_cod_cpu[] = {
X86_MATCH_VFM(INTEL_HASWELL_X, 0), /* COD */
X86_MATCH_VFM(INTEL_BROADWELL_X, 0), /* COD */
X86_MATCH_VFM(INTEL_ANY, 1), /* SNC */
{}
};
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
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():
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++) {
...
}
return NULL;
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.
Add 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.
Change the end-marker check in x86_match_cpu() to just check the flags field
for this bit.
Backporter notes: The commit in Fixes is really the one that is broken:
you can't have m->vendor as part of the loop termination conditional in
x86_match_cpu() because it can happen - as it has happened above
- that that whole conditional is 0 albeit vendor == 0 is a valid case
- X86_VENDOR_INTEL is 0.
However, the only case where the above happens is the SNC check added by
4db64279bc2b1 so you only need this fix if you have backported that
other commit
4db64279bc2b ("x86/cpu: Switch to new Intel CPU model defines")
Fixes: 644e9cbbe3fc ("Add driver auto probing for x86 features v4")
Suggested-by: Thomas Gleixner <[email protected]>
Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Tony Luck <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Cc: <[email protected]> # see above
Link: https://lore.kernel.org/r/20240517144312.GBZkdtAOuJZCvxhFbJ@fat_crate.local
---
arch/x86/include/asm/cpu_device_id.h | 5 +++++
arch/x86/kernel/cpu/match.c | 4 +---
include/linux/mod_devicetable.h | 2 ++
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index 970a232..b6325ee 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
@@ -79,6 +82,7 @@
.model = _model, \
.steppings = _steppings, \
.feature = _feature, \
+ .flags = X86_CPU_ID_FLAG_ENTRY_VALID, \
.driver_data = (unsigned long) _data \
}
@@ -89,6 +93,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 8651643..8e7de73 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)
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7a9a07e..4338b1b 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -690,6 +690,8 @@ 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;
};