[PATCH 1/2] x86/mpparse: avoid overwriting boot_cpu_physical_apicid

[PATCH 1/2] x86/mpparse: avoid overwriting boot_cpu_physical_apicid

[Kevin Mitchell]

When booting with ACPI unavailable or disabled, get_smp_config() ends up
calling MP_processor_info() for each CPU found in the MPS
table. Previously, this resulted in boot_cpu_physical_apicid getting
unconditionally overwritten by the apicid of whatever processor had the
CPU_BOOTPROCESSOR flag. This occurred even if boot_cpu_physical_apicid
had already been more reliably determined in register_lapic_address() by
calling read_apic_id() from the actual boot processor.

Ordinariliy, this is not a problem because the boot processor really is
the one with the CPU_BOOTPROCESSOR flag. However, kexec is an exception
in which the kernel may be booted from any processor regardless of the
MPS table contents. In this case, boot_cpu_physical_apicid may not
indicate the actual boot processor.

This was particularly problematic when the second kernel was booted with
NR_CPUS fewer than the number of physical processors. It's the job of
generic_processor_info() to decide which CPUs to bring up in this case.
That obviously must include the real boot processor which it takes care
to save a slot for. It relies upon the contents of
boot_cpu_physical_apicid to do this, which if incorrect, may result in
the boot processor getting left out.

This condition can be discovered by smp_sanity_check() and rectified by
adding the boot processor to the phys_cpu_present_map with the warning
"weird, boot CPU (#%d) not listed by the BIOS". However, commit
3e730dad3b6da ("x86/apic: Unify interrupt mode setup for SMP-capable
system") caused setup_local_APIC() to be called before this could happen
resulting in a BUG_ON(!apic->apic_id_registered()):

[    0.655452] ------------[ cut here ]------------
[    0.660610] Kernel BUG at setup_local_APIC+0x74/0x280 [verbose debug info unavailable]
[    0.669466] invalid opcode: 0000 [#1] SMP
[    0.673948] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.19.109.Ar-16509018.eostrunkkernel419 #1
[    0.683670] Hardware name: Quanta Quanta LY6 (1LY6UZZ0FBC), BIOS 1.0.6.0-e7d6a55 11/26/2015
[    0.693007] RIP: 0010:setup_local_APIC+0x74/0x280
[    0.698264] Code: 80 e4 fe bf f0 00 00 00 89 c6 48 8b 05 0f 1a 8e 00 ff 50 10 e8 12 53 fd ff 48 8b 05 00 1a 8e 00 ff 90 a0 00 00 00 85 c0 75 02 <0f> 0b 48 8b 05 ed 19 8e 00 41 be 00 02 00 00 ff 90 b0 00 00 00 48
[    0.719251] RSP: 0000:ffffffff81a03e20 EFLAGS: 00010246
[    0.725091] RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
[    0.733066] RDX: 0000000000000000 RSI: 000000000000000f RDI: 0000000000000020
[    0.741041] RBP: ffffffff81a03e98 R08: 0000000000000002 R09: 0000000000000000
[    0.749014] R10: ffffffff81a204e0 R11: ffffffff81b50ea7 R12: 0000000000000000
[    0.756989] R13: ffffffff81aef920 R14: ffffffff81af60a0 R15: 0000000000000000
[    0.764965] FS:  0000000000000000(0000) GS:ffff888036800000(0000) knlGS:0000000000000000
[    0.774007] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    0.780427] CR2: ffff888035c01000 CR3: 0000000035a08000 CR4: 00000000000006b0
[    0.788401] Call Trace:
[    0.791137]  ? amd_iommu_prepare+0x15/0x2a
[    0.795717]  apic_bsp_setup+0x55/0x75
[    0.799808]  apic_intr_mode_init+0x169/0x16e
[    0.804579]  x86_late_time_init+0x10/0x17
[    0.809062]  start_kernel+0x37e/0x3fe
[    0.813154]  x86_64_start_reservations+0x2a/0x2c
[    0.818316]  x86_64_start_kernel+0x72/0x75
[    0.822886]  secondary_startup_64+0xa4/0xb0
[    0.827564] ---[ end trace 237b64da0fd9b22e ]---

This change avoids these issues by only setting boot_cpu_physical_apicid
from the MPS table if it is not already set, which can occur in the
construct_default_ISA_mptable() path. Otherwise,
boot_cpu_physical_apicid will already have been set in
register_lapic_address() and should therefore remain untouched.

Looking through all the places where boot_cpu_physical_apicid is
accessed, nearly all of them assume that boot_cpu_physical_apicid should
match read_apic_id() on the booting processor. The only place that might
intend to use the BSP apicid listed in the MPS table is amd_numa_init(),
which explicitly requires boot_cpu_physical_apicid to be the lowest
apicid of all processors. Ironically, due to the early exit short
circuit in early_get_smp_config(), it instead gets
boot_cpu_physical_apicid = read_apic_id() rather than the MPS table
BSP. The behaviour of amd_numa_init() is therefore unaffected by this
change.

Fixes: 3e730dad3b6da ("x86/apic: Unify interrupt mode setup for SMP-capable system")
Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
Cc: <stable@vger.kernel.org>
---
 arch/x86/kernel/mpparse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index afac7ccce72f..6f22f09bfe11 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -64,7 +64,8 @@ static void __init MP_processor_info(struct mpc_cpu *m)
 
 	if (m->cpuflag & CPU_BOOTPROCESSOR) {
 		bootup_cpu = " (Bootup-CPU)";
-		boot_cpu_physical_apicid = m->apicid;
+		if (boot_cpu_physical_apicid == -1U)
+			boot_cpu_physical_apicid = m->apicid;
 	}
 
 	pr_info("Processor #%d%s\n", m->apicid, bootup_cpu);
-- 
2.26.2

[PATCH 2/2] x86/apic: remove boot_cpu_physical_apicid hacks

[Kevin Mitchell]

Now that mpparse can be relied upon to not sabotage
boot_cpu_physical_apicid with nonsense, the hacks that tried to work
around that nonsense can be removed.

Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
---
 arch/x86/kernel/apic/apic.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 4b1d31be50b4..c1722a71aca5 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2361,27 +2361,8 @@ int generic_processor_info(int apicid, int version)
 	bool boot_cpu_detected = physid_isset(boot_cpu_physical_apicid,
 				phys_cpu_present_map);
 
-	/*
-	 * boot_cpu_physical_apicid is designed to have the apicid
-	 * returned by read_apic_id(), i.e, the apicid of the
-	 * currently booting-up processor. However, on some platforms,
-	 * it is temporarily modified by the apicid reported as BSP
-	 * through MP table. Concretely:
-	 *
-	 * - arch/x86/kernel/mpparse.c: MP_processor_info()
-	 * - arch/x86/mm/amdtopology.c: amd_numa_init()
-	 *
-	 * This function is executed with the modified
-	 * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
-	 * parameter doesn't work to disable APs on kdump 2nd kernel.
-	 *
-	 * Since fixing handling of boot_cpu_physical_apicid requires
-	 * another discussion and tests on each platform, we leave it
-	 * for now and here we use read_apic_id() directly in this
-	 * function, generic_processor_info().
-	 */
 	if (disabled_cpu_apicid != BAD_APICID &&
-	    disabled_cpu_apicid != read_apic_id() &&
+	    disabled_cpu_apicid != boot_cpu_physical_apicid &&
 	    disabled_cpu_apicid == apicid) {
 		int thiscpu = num_processors + disabled_cpus;
 
@@ -2498,15 +2479,6 @@ static void __init apic_bsp_up_setup(void)
 {
 #ifdef CONFIG_X86_64
 	apic_write(APIC_ID, apic->set_apic_id(boot_cpu_physical_apicid));
-#else
-	/*
-	 * Hack: In case of kdump, after a crash, kernel might be booting
-	 * on a cpu with non-zero lapic id. But boot_cpu_physical_apicid
-	 * might be zero if read from MP tables. Get it from LAPIC.
-	 */
-# ifdef CONFIG_CRASH_DUMP
-	boot_cpu_physical_apicid = read_apic_id();
-# endif
 #endif
 	physid_set_mask_of_physid(boot_cpu_physical_apicid, &phys_cpu_present_map);
 }
-- 
2.26.2

Re: [PATCH 1/2] x86/mpparse: avoid overwriting boot_cpu_physical_apicid

[Kevin Mitchell]

On Mon, Jun 08, 2020 at 05:44:48PM -0700, Kevin Mitchell wrote:
> When booting with ACPI unavailable or disabled, get_smp_config() ends up
> calling MP_processor_info() for each CPU found in the MPS
> table. Previously, this resulted in boot_cpu_physical_apicid getting
> unconditionally overwritten by the apicid of whatever processor had the
> CPU_BOOTPROCESSOR flag. This occurred even if boot_cpu_physical_apicid
> had already been more reliably determined in register_lapic_address() by
> calling read_apic_id() from the actual boot processor.
> 
> Ordinariliy, this is not a problem because the boot processor really is
> the one with the CPU_BOOTPROCESSOR flag. However, kexec is an exception
> in which the kernel may be booted from any processor regardless of the
> MPS table contents. In this case, boot_cpu_physical_apicid may not
> indicate the actual boot processor.
> 
> This was particularly problematic when the second kernel was booted with
> NR_CPUS fewer than the number of physical processors. It's the job of
> generic_processor_info() to decide which CPUs to bring up in this case.
> That obviously must include the real boot processor which it takes care
> to save a slot for. It relies upon the contents of
> boot_cpu_physical_apicid to do this, which if incorrect, may result in
> the boot processor getting left out.
> 
> This condition can be discovered by smp_sanity_check() and rectified by
> adding the boot processor to the phys_cpu_present_map with the warning
> "weird, boot CPU (#%d) not listed by the BIOS". However, commit
> 3e730dad3b6da ("x86/apic: Unify interrupt mode setup for SMP-capable
> system") caused setup_local_APIC() to be called before this could happen
> resulting in a BUG_ON(!apic->apic_id_registered()):
> 
> [    0.655452] ------------[ cut here ]------------
> [    0.660610] Kernel BUG at setup_local_APIC+0x74/0x280 [verbose debug info unavailable]
> [    0.669466] invalid opcode: 0000 [#1] SMP
> [    0.673948] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.19.109.Ar-16509018.eostrunkkernel419 #1
> [    0.683670] Hardware name: Quanta Quanta LY6 (1LY6UZZ0FBC), BIOS 1.0.6.0-e7d6a55 11/26/2015
> [    0.693007] RIP: 0010:setup_local_APIC+0x74/0x280
> [    0.698264] Code: 80 e4 fe bf f0 00 00 00 89 c6 48 8b 05 0f 1a 8e 00 ff 50 10 e8 12 53 fd ff 48 8b 05 00 1a 8e 00 ff 90 a0 00 00 00 85 c0 75 02 <0f> 0b 48 8b 05 ed 19 8e 00 41 be 00 02 00 00 ff 90 b0 00 00 00 48
> [    0.719251] RSP: 0000:ffffffff81a03e20 EFLAGS: 00010246
> [    0.725091] RAX: 0000000000000000 RBX: 0000000000000003 RCX: 0000000000000000
> [    0.733066] RDX: 0000000000000000 RSI: 000000000000000f RDI: 0000000000000020
> [    0.741041] RBP: ffffffff81a03e98 R08: 0000000000000002 R09: 0000000000000000
> [    0.749014] R10: ffffffff81a204e0 R11: ffffffff81b50ea7 R12: 0000000000000000
> [    0.756989] R13: ffffffff81aef920 R14: ffffffff81af60a0 R15: 0000000000000000
> [    0.764965] FS:  0000000000000000(0000) GS:ffff888036800000(0000) knlGS:0000000000000000
> [    0.774007] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.780427] CR2: ffff888035c01000 CR3: 0000000035a08000 CR4: 00000000000006b0
> [    0.788401] Call Trace:
> [    0.791137]  ? amd_iommu_prepare+0x15/0x2a
> [    0.795717]  apic_bsp_setup+0x55/0x75
> [    0.799808]  apic_intr_mode_init+0x169/0x16e
> [    0.804579]  x86_late_time_init+0x10/0x17
> [    0.809062]  start_kernel+0x37e/0x3fe
> [    0.813154]  x86_64_start_reservations+0x2a/0x2c
> [    0.818316]  x86_64_start_kernel+0x72/0x75
> [    0.822886]  secondary_startup_64+0xa4/0xb0
> [    0.827564] ---[ end trace 237b64da0fd9b22e ]---
> 
> This change avoids these issues by only setting boot_cpu_physical_apicid
> from the MPS table if it is not already set, which can occur in the
> construct_default_ISA_mptable() path. Otherwise,
> boot_cpu_physical_apicid will already have been set in
> register_lapic_address() and should therefore remain untouched.
> 
> Looking through all the places where boot_cpu_physical_apicid is
> accessed, nearly all of them assume that boot_cpu_physical_apicid should
> match read_apic_id() on the booting processor. The only place that might
> intend to use the BSP apicid listed in the MPS table is amd_numa_init(),
> which explicitly requires boot_cpu_physical_apicid to be the lowest
> apicid of all processors. Ironically, due to the early exit short
> circuit in early_get_smp_config(), it instead gets
> boot_cpu_physical_apicid = read_apic_id() rather than the MPS table
> BSP. The behaviour of amd_numa_init() is therefore unaffected by this
> change.
> 
> Fixes: 3e730dad3b6da ("x86/apic: Unify interrupt mode setup for SMP-capable system")
> Signed-off-by: Kevin Mitchell <kevmitch@arista.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/x86/kernel/mpparse.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index afac7ccce72f..6f22f09bfe11 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -64,7 +64,8 @@ static void __init MP_processor_info(struct mpc_cpu *m)
>  
>  	if (m->cpuflag & CPU_BOOTPROCESSOR) {
>  		bootup_cpu = " (Bootup-CPU)";
> -		boot_cpu_physical_apicid = m->apicid;
> +		if (boot_cpu_physical_apicid == -1U)
> +			boot_cpu_physical_apicid = m->apicid;
>  	}
>  
>  	pr_info("Processor #%d%s\n", m->apicid, bootup_cpu);
> -- 
> 2.26.2
> 

We've moved on to our next kernel upgrade to linux-5.10 and are still seeing
this same issue with the upstream kernel. We will therefore be porting this
patch forward, but still wondering if there is any interest in getting this into
the mainline kernel so more people get (more) correct code? Both patches still
apply to the mainline (linux-5.18-rc7 right now). Are there any
alternative suggestions for avoiding this BUG_ON on kexec?