Peter!

On Mon, May 27 2024 at 23:15, Peter Schneider wrote:

Thanks for providing all the information!

I want to add one thing: there is a log entry in the dmesg output of a "bad" kernel, which I initially overlooked, because it is way up, and I noticed this just now. I guess this might be relevant:

[ 1.683564] [Firmware Bug]: CPU0: Topology domain 0 shift 1 != 5

Yes. That's absolutely related. I can see what goes wrong, but I have absolutely no idea how that happens.

Can you please apply the debug patch below ad provide the full dmesg after boot?

Thanks,

tglx --- --- a/arch/x86/kernel/cpu/topology_common.c +++ b/arch/x86/kernel/cpu/topology_common.c @@ -65,6 +65,7 @@ static void parse_legacy(struct topo_sca cores <<= smt_shift; }

+ pr_info("Legacy: %u %u %u\n", c->cpuid_level, smt_shift, core_shift); topology_set_dom(tscan, TOPO_SMT_DOMAIN, smt_shift, 1U << smt_shift); topology_set_dom(tscan, TOPO_CORE_DOMAIN, core_shift, cores); } --- a/arch/x86/kernel/cpu/topology_ext.c +++ b/arch/x86/kernel/cpu/topology_ext.c @@ -72,6 +72,9 @@ static inline bool topo_subleaf(struct t

cpuid_subleaf(leaf, subleaf, &sl);

+ pr_info("L:%0x %0x %0x S:%u N:%u T:%u\n", leaf, subleaf, sl.level, sl.x2apic_shift, + sl.num_processors, sl.type); + if (!sl.num_processors || sl.type == INVALID_TYPE) return false;

@@ -97,6 +100,7 @@ static inline bool topo_subleaf(struct t leaf, subleaf, tscan->c->topo.initial_apicid, sl.x2apic_id); }

+ pr_info("D: %u\n", dom); topology_set_dom(tscan, dom, sl.x2apic_shift, sl.num_processors); return true; }

Peter!

On Thu, May 30 2024 at 12:06, Peter Schneider wrote:

Am 30.05.24 um 10:30 schrieb Thomas Gleixner:

...

Can you please apply the debug patch below ad provide the full dmesg after boot?

Here you go... The patch applied cleanly against 6.9.3, which I saw was just released by Greg, so I used that. If you want, I can repeat the test against 6.9.2, too.

.3 is fine

Please note: to be able to boot any kernel >= 6.8.4 on my machine, I also had to apply this patch by Martin Petersen, fixing another (unrelated SCSI) regression I reported some time ago, see here:

https://lore.kernel.org/all/20240521023040.2703884-1-martin.petersen@oracle....

But I think these two issues are not connected in any way. It was during testing the above patch by Martin that I noticed this new issue in 6.9 BTW.

Right. It's a seperate problem.

I have attached resulting file dmesg_6.9.3-dirty_Bad_wDebugInfo.txt, and I hope you can make some sense of it.

It's exactly what I expected but it does not make any sense at all.

[    0.000000] Legacy: 2 5 5

So that means that during early boot where the topology parameters are decoded from CPUID the CPUID evaluation code sees that the maximum supported CPUID leaf is 0x02 and it therefore reads complete non-sense.

Later on when the full CPUID evaluation happens it sees the full space and uses leaf 0xb.

[    1.687649] L:b 0 0 S:1 N:2 T:1
[    1.687652] D: 0
[    1.687653] L:b 1 1 S:5 N:24 T:2
[    1.687655] D: 1
[    1.687656] L:b 2 2 S:0 N:0 T:0
[    1.687658] [Firmware Bug]: CPU0: Topology domain 0 shift 1 != 5

And this obviously sees the proper numbers and complains about the inconsistency.

So something on this CPU is broken. The same problem exists on all APs:

[    1.790035] .... node  #0, CPUs:        #4
[    1.790312] .... node  #1, CPUs:   #12 #16
[    0.011992] Legacy: 2 5 5
[    0.011992] Legacy: 2 5 5
[    0.011992] Legacy: 2 5 5
[    0.011992] Legacy: 2 5 5

.....

Now the million-dollar question is what unlocks CPUID to read the proper value of EAX of leaf 0. All I could come up with is to sprinkle a dozen of printks into that code. Updated debug patch below.

Thanks,

tglx --- --- a/arch/x86/kernel/cpu/topology_common.c +++ b/arch/x86/kernel/cpu/topology_common.c @@ -65,6 +65,7 @@ static void parse_legacy(struct topo_sca cores <<= smt_shift; }

+ pr_info("Legacy: %u %u %u\n", c->cpuid_level, smt_shift, core_shift); topology_set_dom(tscan, TOPO_SMT_DOMAIN, smt_shift, 1U << smt_shift); topology_set_dom(tscan, TOPO_CORE_DOMAIN, core_shift, cores); } --- a/arch/x86/kernel/cpu/topology_ext.c +++ b/arch/x86/kernel/cpu/topology_ext.c @@ -72,6 +72,9 @@ static inline bool topo_subleaf(struct t

cpuid_subleaf(leaf, subleaf, &sl);

+ pr_info("L:%0x %0x %0x S:%u N:%u T:%u\n", leaf, subleaf, sl.level, sl.x2apic_shift, + sl.num_processors, sl.type); + if (!sl.num_processors || sl.type == INVALID_TYPE) return false;

@@ -97,6 +100,7 @@ static inline bool topo_subleaf(struct t leaf, subleaf, tscan->c->topo.initial_apicid, sl.x2apic_id); }

+ pr_info("D: %u\n", dom); topology_set_dom(tscan, dom, sl.x2apic_shift, sl.num_processors); return true; } --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -1584,22 +1584,30 @@ static void __init early_identify_cpu(st /* cyrix could have cpuid enabled via c_identify()*/ if (have_cpuid_p()) { cpu_detect(c); + pr_info("MAXL1: %x\n", cpuid_eax(0)); get_cpu_vendor(c); + pr_info("MAXL2: %x\n", cpuid_eax(0)); get_cpu_cap(c); + pr_info("MAXL3: %x\n", cpuid_eax(0)); setup_force_cpu_cap(X86_FEATURE_CPUID); get_cpu_address_sizes(c); + pr_info("MAXL4: %x\n", cpuid_eax(0)); cpu_parse_early_param(); + pr_info("MAXL5: %x\n", cpuid_eax(0));

cpu_init_topology(c); + pr_info("MAXL6: %x\n", cpuid_eax(0));

if (this_cpu->c_early_init) this_cpu->c_early_init(c); + pr_info("MAXL7: %x\n", cpuid_eax(0));

c->cpu_index = 0; filter_cpuid_features(c, false);

if (this_cpu->c_bsp_init) this_cpu->c_bsp_init(c); + pr_info("MAXL8: %x\n", cpuid_eax(0)); } else { setup_clear_cpu_cap(X86_FEATURE_CPUID); get_cpu_address_sizes(c); @@ -1797,9 +1805,12 @@ static void identify_cpu(struct cpuinfo_ #ifdef CONFIG_X86_VMX_FEATURE_NAMES memset(&c->vmx_capability, 0, sizeof(c->vmx_capability)); #endif + pr_info("MAXLG1: %x\n", cpuid_eax(0));

generic_identify(c);

+ pr_info("MAXLG2: %x\n", cpuid_eax(0)); + cpu_parse_topology(c);

if (this_cpu->c_identify)

On Thu, May 30 2024 at 17:53, Thomas Gleixner wrote:

On Thu, May 30 2024 at 15:35, Thomas Gleixner wrote:

...

On Thu, May 30 2024 at 12:06, Peter Schneider wrote: Now the million-dollar question is what unlocks CPUID to read the proper value of EAX of leaf 0. All I could come up with is to sprinkle a dozen of printks into that code. Updated debug patch below.

Don't bother. Dave pointed out to me that this is unlocked in early_init_intel() via MSR_IA32_MISC_ENABLE_LIMIT_CPUID...

Let me figure out how to fix that sanely.

The original code just worked because it was reevaluating this stuff over and over until it magically became "correct".

The proper fix is obviously to unlock CPUID on Intel _before_ anything which depends on cpuid_level is evaluated.

Thanks,

tglx --- --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -969,7 +969,7 @@ static void init_speculation_control(str } }

-void get_cpu_cap(struct cpuinfo_x86 *c) +static void get_cpu_cap(struct cpuinfo_x86 *c) { u32 eax, ebx, ecx, edx;

@@ -1585,6 +1585,7 @@ static void __init early_identify_cpu(st if (have_cpuid_p()) { cpu_detect(c); get_cpu_vendor(c); + intel_unlock_cpuid_leafs(c); get_cpu_cap(c); setup_force_cpu_cap(X86_FEATURE_CPUID); get_cpu_address_sizes(c); @@ -1744,7 +1745,7 @@ static void generic_identify(struct cpui cpu_detect(c);

get_cpu_vendor(c); - + intel_unlock_cpuid_leafs(c); get_cpu_cap(c);

get_cpu_address_sizes(c); --- a/arch/x86/kernel/cpu/cpu.h +++ b/arch/x86/kernel/cpu/cpu.h @@ -61,14 +61,15 @@ extern __ro_after_init enum tsx_ctrl_sta

extern void __init tsx_init(void); void tsx_ap_init(void); +void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c); #else static inline void tsx_init(void) { } static inline void tsx_ap_init(void) { } +static inline void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c) { } #endif /* CONFIG_CPU_SUP_INTEL */

extern void init_spectral_chicken(struct cpuinfo_x86 *c);

-extern void get_cpu_cap(struct cpuinfo_x86 *c); extern void get_cpu_address_sizes(struct cpuinfo_x86 *c); extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c); extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c); --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -269,19 +269,26 @@ static void detect_tme_early(struct cpui c->x86_phys_bits -= keyid_bits; }

+void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c) +{ + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + return; + + if (c->x86 < 6 || (c->x86 == 6 && c->x86_model < 0xd)) + return; + + /* + * The BIOS can have limited CPUID to leaf 2, which breaks feature + * enumeration. Unlock it and update the maximum leaf info. + */ + if (msr_clear_bit(MSR_IA32_MISC_ENABLE, MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0) + c->cpuid_level = cpuid_eax(0); +} + static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable;

- /* Unmask CPUID levels if masked: */ - if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) { - if (msr_clear_bit(MSR_IA32_MISC_ENABLE, - MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0) { - c->cpuid_level = cpuid_eax(0); - get_cpu_cap(c); - } - } - if ((c->x86 == 0xf && c->x86_model >= 0x03) || (c->x86 == 0x6 && c->x86_model >= 0x0e)) set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

Peter!

On Fri, May 31 2024 at 08:52, Peter Schneider wrote:

Am 30.05.2024 um 18:24 schrieb Thomas Gleixner: With that patch applied, I now get a build error:

arch/x86/xen/enlighten_pv.c: In Funktion »xen_start_kernel«: arch/x86/xen/enlighten_pv.c:1388:9: Fehler: Implizite Deklaration der Funktion »get_cpu_cap«; meinten Sie »set_cpu_cap«? [-Werror=implicit-function-declaration] 1388 | get_cpu_cap(&boot_cpu_data);

Bah. Updated patch below.

Thanks,

tglx --- Subject: x86/topology/intel: Unlock CPUID before evaluating anything From: Thomas Gleixner tglx@linutronix.de Date: Thu, 30 May 2024 17:29:18 +0200

Intel CPUs have a MSR bit to limit CPUID enumeration to leaf two. If this bit is set by the BIOS then CPUID evaluation including topology enumeration does not work correctly as the evaluation code does not try to analyze any leaf greater than two.

This went unnoticed before because the original topology code just repeated evaluation several times and managed to overwrite the initial limited information with the correct one later. The new evaluation code does it once and therefore ends up with the limited and wrong information.

Cure this by unlocking CPUID right before evaluating anything which depends on the maximum CPUID leaf being greater than two instead of rereading stuff after unlock.

Fixes: 22d63660c35e ("x86/cpu: Use common topology code for Intel") Reported-by: Peter Schneider pschneider1968@googlemail.com Signed-off-by: Thomas Gleixner tglx@linutronix.de --- arch/x86/kernel/cpu/common.c | 3 ++- arch/x86/kernel/cpu/intel.c | 25 ++++++++++++++++--------- 2 files changed, 18 insertions(+), 10 deletions(-)

--- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -1585,6 +1585,7 @@ static void __init early_identify_cpu(st if (have_cpuid_p()) { cpu_detect(c); get_cpu_vendor(c); + intel_unlock_cpuid_leafs(c); get_cpu_cap(c); setup_force_cpu_cap(X86_FEATURE_CPUID); get_cpu_address_sizes(c); @@ -1744,7 +1745,7 @@ static void generic_identify(struct cpui cpu_detect(c);

get_cpu_vendor(c); - + intel_unlock_cpuid_leafs(c); get_cpu_cap(c);

get_cpu_address_sizes(c); --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -269,19 +269,26 @@ static void detect_tme_early(struct cpui c->x86_phys_bits -= keyid_bits; }

+void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c) +{ + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) + return; + + if (c->x86 < 6 || (c->x86 == 6 && c->x86_model < 0xd)) + return; + + /* + * The BIOS can have limited CPUID to leaf 2, which breaks feature + * enumeration. Unlock it and update the maximum leaf info. + */ + if (msr_clear_bit(MSR_IA32_MISC_ENABLE, MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0) + c->cpuid_level = cpuid_eax(0); +} + static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable;

- /* Unmask CPUID levels if masked: */ - if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) { - if (msr_clear_bit(MSR_IA32_MISC_ENABLE, - MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0) { - c->cpuid_level = cpuid_eax(0); - get_cpu_cap(c); - } - } - if ((c->x86 == 0xf && c->x86_model >= 0x03) || (c->x86 == 0x6 && c->x86_model >= 0x0e)) set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

On 24/05/30 06:24PM, Thomas Gleixner wrote:

On Thu, May 30 2024 at 17:53, Thomas Gleixner wrote:

...

Let me figure out how to fix that sanely.

The proper fix is obviously to unlock CPUID on Intel _before_ anything which depends on cpuid_level is evaluated.

Thanks,

    tglx

Hey Thomas,

as reported on the other mail the proposed fix broke the build (see below) due to get_cpu_cap() becoming static but still being used in other parts of the code.

One of the reporters in the Arch Bugtracker with an Intel Core i7-7700k has tested a modified version of this fix[0] with the static change reversed on top of the 6.9.2 stable kernel and reports that the patch does not fix the issue for them. I have attached their output for the patched (dmesg6.9.2-1.5.log) and nonpatched (dmesg6.9.2-1.log) kernel.

Should we also get them to test the mainline version or do you need any other debug output?

Cheers, gromit

[0]: https://gitlab.archlinux.org/archlinux/packaging/packages/linux/-/issues/57#...

---

--- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -969,7 +969,7 @@ static void init_speculation_control(str } } -void get_cpu_cap(struct cpuinfo_x86 *c) +static void get_cpu_cap(struct cpuinfo_x86 *c)

making this function static breaks the build for me:

arch/x86/xen/enlighten_pv.c: In function ‘xen_start_kernel’: arch/x86/xen/enlighten_pv.c:1388:9: error: implicit declaration of function ‘get_cpu_cap’; did you mean ‘set_cpu_cap’? [-Wimplicit-function-declaration] 1388 | get_cpu_cap(&boot_cpu_data); ¦ | ^~~~~~~~~~~ ¦ | set_cpu_cap

{ u32 eax, ebx, ecx, edx; @@ -1585,6 +1585,7 @@ static void __init early_identify_cpu(st if (have_cpuid_p()) { cpu_detect(c); get_cpu_vendor(c);

• intel_unlock_cpuid_leafs(c);
  

  get_cpu_cap(c); setup_force_cpu_cap(X86_FEATURE_CPUID); get_cpu_address_sizes(c);

@@ -1744,7 +1745,7 @@ static void generic_identify(struct cpui cpu_detect(c); get_cpu_vendor(c);

• intel_unlock_cpuid_leafs(c); get_cpu_cap(c);

get_cpu_address_sizes(c); --- a/arch/x86/kernel/cpu/cpu.h +++ b/arch/x86/kernel/cpu/cpu.h @@ -61,14 +61,15 @@ extern __ro_after_init enum tsx_ctrl_sta extern void __init tsx_init(void); void tsx_ap_init(void); +void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c); #else static inline void tsx_init(void) { } static inline void tsx_ap_init(void) { } +static inline void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c) { } #endif /* CONFIG_CPU_SUP_INTEL */ extern void init_spectral_chicken(struct cpuinfo_x86 *c); -extern void get_cpu_cap(struct cpuinfo_x86 *c); extern void get_cpu_address_sizes(struct cpuinfo_x86 *c); extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c); extern void init_scattered_cpuid_features(struct cpuinfo_x86 *c); --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -269,19 +269,26 @@ static void detect_tme_early(struct cpui c->x86_phys_bits -= keyid_bits; } +void intel_unlock_cpuid_leafs(struct cpuinfo_x86 *c) +{

• if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)

• return;
  

• if (c->x86 < 6 || (c->x86 == 6 && c->x86_model < 0xd))

• return;
  

• /*

• * The BIOS can have limited CPUID to leaf 2, which breaks feature
  

• * enumeration. Unlock it and update the maximum leaf info.
  

• */
  

• if (msr_clear_bit(MSR_IA32_MISC_ENABLE, MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0)

• c->cpuid_level = cpuid_eax(0);
  

+}

static void early_init_intel(struct cpuinfo_x86 *c) { u64 misc_enable;

• /* Unmask CPUID levels if masked: */
• if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {

• if (msr_clear_bit(MSR_IA32_MISC_ENABLE,
  

• 		  MSR_IA32_MISC_ENABLE_LIMIT_CPUID_BIT) > 0) {
  

• 	c->cpuid_level = cpuid_eax(0);
  

• 	get_cpu_cap(c);
  

• }
  

• }
• if ((c->x86 == 0xf && c->x86_model >= 0x03) || (c->x86 == 0x6 && c->x86_model >= 0x0e)) set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);