Messages in this thread Patch in this message | | | From | Carsten Tolkmit <> | Subject | AW: x86/topology: Handle bogus ACPI tables correctly | Date | Fri, 17 May 2024 20:13:37 +0000 |
| |
<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> Hi Thomas,</div> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> <br> </div> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> I can confirm that your revised patch works as well as your first one.</div> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> <br> </div> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> Thanks!</div> <div class="elementToProof" style="font-family: Calibri, Helvetica, sans-serif; font-size: 11pt; color: rgb(0, 0, 0);"> Carsten</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>Von:</b> Thomas Gleixner <tglx@linutronix.de><br> <b>Gesendet:</b> Freitag, 17. Mai 2024 16:40<br> <b>An:</b> Carsten Tolkmit <ctolkmit@tng.de><br> <b>Cc:</b> LKML <linux-kernel@vger.kernel.org>; x86@kernel.org <x86@kernel.org><br> <b>Betreff:</b> x86/topology: Handle bogus ACPI tables correctly</font> <div> </div> </div> <div class="BodyFragment"><font size="2"><span style="font-size:11pt;"> <div class="PlainText">The ACPI specification clearly states how the processors should be<br> enumerated in the MADT:<br> <br> "To ensure that the boot processor is supported post initialization,<br> two guidelines should be followed. The first is that OSPM should<br> initialize processors in the order that they appear in the MADT. The<br> second is that platform firmware should list the boot processor as the<br> first processor entry in the MADT.<br> ...<br> Failure of OSPM implementations and platform firmware to abide by<br> these guidelines can result in both unpredictable and non optimal<br> platform operation."<br> <br> The kernel relies on that ordering to detect the real BSP on crash kernels<br> which is important to avoid sending a INIT IPI to it as that would cause a<br> full machine reset.<br> <br> On a Dell XPS 16 9640 the BIOS ignores this rule and enumerates the CPUs in<br> the wrong order. As a consequence the kernel falsely detects a crash kernel<br> and disables the corresponding CPU.<br> <br> Prevent this by checking the IA32_APICBASE MSR for the BSP bit on the boot<br> CPU. If that bit is set, then the MADT based BSP detection can be safely<br> ignored. If the kernel detects a mismatch between the BSP bit and the first<br> enumerated MADT entry then emit a firmware bug message.<br> <br> This obviously also has to be taken into account when the boot APIC ID and<br> the first enumerated APIC ID match. If the boot CPU does not have the BSP<br> bit set in the APICBASE MSR then there is no way for the boot CPU to<br> determine which of the CPUs is the real BSP. Sending an INIT to the real<br> BSP would reset the machine so the only sane way to deal with that is to<br> limit the number of CPUs to one and emit a corresponding warning message.<br> <br> Fixes: 5c5682b9f87a ("x86/cpu: Detect real BSP on crash kernels")<br> Reported-by: Carsten Tolkmit <ctolkmit@ennit.de><br> Signed-off-by: Thomas Gleixner <tglx@linutronix.de><br> Cc: stable@vger.kernel.org<br> Closes: <a href="https://bugzilla.kernel.org/show_bug.cgi?id=218837">https://bugzilla.kernel.org/show_bug.cgi?id=218837</a><br> ---<br> <br> This is a slightly different solution than the initial patch I provided in<br> the bugzilla. Carsten, can you please test that again?<br> ---<br> arch/x86/kernel/cpu/topology.c | 43 ++++++++++++++++++++++++++++++++++++++---<br> 1 file changed, 40 insertions(+), 3 deletions(-)<br> <br> --- a/arch/x86/kernel/cpu/topology.c<br> +++ b/arch/x86/kernel/cpu/topology.c<br> @@ -128,6 +128,9 @@ static void topo_set_cpuids(unsigned int<br> <br> static __init bool check_for_real_bsp(u32 apic_id)<br> {<br> + bool is_bsp = false, has_apic_base = boot_cpu_data.x86 >= 6;<br> + u64 msr;<br> +<br> /*<br> * There is no real good way to detect whether this a kdump()<br> * kernel, but except on the Voyager SMP monstrosity which is not<br> @@ -144,17 +147,51 @@ static __init bool check_for_real_bsp(u3<br> if (topo_info.real_bsp_apic_id != BAD_APICID)<br> return false;<br> <br> + /*<br> + * Check whether the enumeration order is broken by evaluating the<br> + * BSP bit in the APICBASE MSR. If the CPU does not have the<br> + * APICBASE MSR then the BSP detection is not possible and the<br> + * kernel must rely on the firmware enumeration order.<br> + */<br> + if (has_apic_base) {<br> + rdmsrl(MSR_IA32_APICBASE, msr);<br> + is_bsp = !!(msr & MSR_IA32_APICBASE_BSP);<br> + }<br> +<br> if (apic_id == topo_info.boot_cpu_apic_id) {<br> - topo_info.real_bsp_apic_id = apic_id;<br> - return false;<br> + if (is_bsp || !has_apic_base) {<br> + topo_info.real_bsp_apic_id = apic_id;<br> + return false;<br> + }<br> + /*<br> + * If the boot APIC is enumerated first, but the APICBASE<br> + * MSR does not have the BSP bit set, then there is no way<br> + * to discover the real BSP here. Assume a crash kernel and<br> + * limit the number of CPUs to 1 as an INIT to the real BSP<br> + * would reset the machine.<br> + */<br> + pr_warn("Enumerated BSP APIC %x is not marked in APICBASE MSR\n", apic_id);<br> + pr_warn("Assuming crash kernel. Limiting to one CPU to prevent machine INIT\n");<br> + set_nr_cpu_ids(1);<br> + goto fwbug;<br> }<br> <br> - pr_warn("Boot CPU APIC ID not the first enumerated APIC ID: %x > %x\n",<br> + pr_warn("Boot CPU APIC ID not the first enumerated APIC ID: %x != %x\n",<br> topo_info.boot_cpu_apic_id, apic_id);<br> +<br> + if (is_bsp && has_apic_base) {<br> + topo_info.real_bsp_apic_id = topo_info.boot_cpu_apic_id;<br> + goto fwbug;<br> + }<br> +<br> pr_warn("Crash kernel detected. Disabling real BSP to prevent machine INIT\n");<br> <br> topo_info.real_bsp_apic_id = apic_id;<br> return true;<br> +<br> +fwbug:<br> + pr_warn(FW_BUG "APIC enumeration order not specification compliant\n");<br> + return false;<br> }<br> <br> static unsigned int topo_unit_count(u32 lvlid, enum x86_topology_domains at_level,<br> <br> </div> </span></font></div> <font style="font-family: Calibri, sans-serif; font-size: 11pt;">https://www.tng.de <br> <p>Executive board (Geschäftsführer):<br> Dr. Sven Willert (CEO/Vorsitz), <br> Gunnar Peter, Sven Schade,<br> Carsten Tolkmit, Bernd Sontheimer<br> </p> <p>Amtsgericht Kiel HRB 6002 KI<br> USt-ID: DE225201428<br> Die Information über die Verarbeitung Ihrer Daten<br> gemäß Artikel 12 DSGVO können Sie unter https://www.tng.de/datenschutz/ abrufen.<br> ______________________________________________________________________ </p> </font> </body> </html>[unhandled content-type:application/pkcs7-signature] | |