x86/smp: Cure stop_other_cpus() and kexec() troubles

Tony reported intermittent lockups on poweroff. His analysis identified the wbinvd() in stop_this_cpu() as the culprit. This was added to ensure that on SME enabled machines a kexec() does not leave any stale data in the caches when switching from encrypted to non-encrypted mode or vice versa.

That wbindv() is conditional on the SME feature bit which is read directly from CPUID. But that readout does not check whether the CPUID leaf is available or not. If it's not available the CPU will return the value of the highest supported leaf instead. Depending on the content the "SME" bit might be set or not.

That's incorrect but harmless. Making the CPUID readout conditional makes the observed hangs go away, but it does not fix the underlying problem:

CPU0 CPU1

stop_other_cpus()
send_IPIs(REBOOT); stop_this_cpu()

   while (num_online_cpus() > 1);         set_online(false);
   proceed... -> hang
				          wbinvd()

WBINVD is an expensive operation and if multiple CPUs issue it at the same time the resulting delays are even larger.

But CPU0 already observed num_online_cpus() going down to 1 and proceeds which causes the system to hang.

This issue exists independent of WBINVD, but the delays caused by WBINVD make it more prominent.

Make this more robust by adding a cpumask which is initialized to the online CPU mask before sending the IPIs and CPUs clear their bit in stop_this_cpu() after the WBINVD completed. Check for that cpumask to become empty in stop_other_cpus() instead of watching num_online_cpus().

The cpumask cannot plug all holes either, but it's better than a raw counter and allows to restrict the NMI fallback IPI to be sent only to the CPUs which have not reported within the timeout window.

Fixes 08f253ec3767 ("x86/cpu: Clear SME feature flag when not in use")

Reported-by Tony Battersby <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Link https://lore.kernel.org/all/[email protected] open here

---
V3: Use a cpumask to make the NMI case slightly safer - Ashok

---
 arch/x86/include/asm/cpu.h |    2 +
 arch/x86/kernel/process.c  |   23 +++++++++++++-
 arch/x86/kernel/smp.c      |   71 +++++++++++++++++++++++++++++++--------------
 3 files changed, 73 insertions(+), 23 deletions(-)

arch/x86/kernel/smp.c +3 -1

 	/* if the REBOOT_VECTOR didn't work, try with the NMI */
-	if (num_online_cpus() > 1) {
+	if (!cpumask_empty(&cpus_stop_mask)) {
 		/*
 		 * If NMI IPI is enabled, try to register the stop handler
 		 * and send the IPI. In any case try to wait for the other
 		 * CPUs to stop.
 		 */
 		if (!smp_no_nmi_ipi && !register_stop_handler()) {
+			u32 dm;
+
 			/* Sync above data before sending IRQ */
 			wmb();
 
 			pr_emerg("Shutting down cpus with NMI\n");

arch/x86/kernel/smp.c static void native_stop_other_cpus(int w · line 225

Quoted 8 lines · Thomas Gleixner

arch/x86/kernel/smp.c +8 -0

+			dm = apic->dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
+			dm |= APIC_DM_NMI;
+
+			for_each_cpu(cpu, &cpus_stop_mask) {
+				u32 apicid = apic->cpu_present_to_apicid(cpu);
+
+				apic_icr_write(dm, apicid);
+				apic_wait_icr_idle();

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??

Quoted 11 lines · Ashok Raj

arch/x86/kernel/smp.c static void native_stop_other_cpus(int w · line 225

Quoted 8 lines · Thomas Gleixner

arch/x86/kernel/smp.c +8 -0

+			dm = apic->dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
+			dm |= APIC_DM_NMI;
+
+			for_each_cpu(cpu, &cpus_stop_mask) {
+				u32 apicid = apic->cpu_present_to_apicid(cpu);
+
+				apic_icr_write(dm, apicid);
+				apic_wait_icr_idle();

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??

That would not set APIC_DM_NMI in delivery mode and the IPI would be sent with APIC_DM_FIXED.

Quoted 4 lines · Thomas Gleixner

Quoted 1 line

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??

That would not set APIC_DM_NMI in delivery mode and the IPI would be sent with APIC_DM_FIXED.

That's correct.

Maybe if we use apic->send_IPI_mask(cpumask_of(cpu), NMI_VECTOR)

apic->send_IPI_mask(cpumask_of(cpu),NMI_VECTOR)
__x2apic_send_IPI_mask()

      __x2apic_send_IPI_dest()
	  unsigned long cfg = __prepare_ICR(0, vector, dest);
	  native_x2apic_icr_write(cfg, apicid);

__prepare_ICR() seems to have the magic for APIC_DM_NMI?

Quoted 10 lines · Ashok Raj

Quoted 4 lines · Thomas Gleixner

Quoted 1 line

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??

That would not set APIC_DM_NMI in delivery mode and the IPI would be sent with APIC_DM_FIXED.

That's correct.

Maybe if we use apic->send_IPI_mask(cpumask_of(cpu), NMI_VECTOR)

Quoted 7 lines

apic->send_IPI_mask(cpumask_of(cpu),NMI_VECTOR)
__x2apic_send_IPI_mask()

      __x2apic_send_IPI_dest()
	  unsigned long cfg = __prepare_ICR(0, vector, dest);
	  native_x2apic_icr_write(cfg, apicid);

__prepare_ICR() seems to have the magic for APIC_DM_NMI?

Bah. I clearly neither can read nor can remember any of the internals of that code:

apic->send_IPI()
x2apic_send_IPI()

    __x2apic_send_IPI_dest()

It also works for all other APIC incarnations.

Quoted 22 lines · Thomas Gleixner

Quoted 10 lines · Ashok Raj

Quoted 4 lines · Thomas Gleixner

Quoted 1 line

can we simplify this by just apic->send_IPI(cpu, NMI_VECTOR); ??

That would not set APIC_DM_NMI in delivery mode and the IPI would be sent with APIC_DM_FIXED.

That's correct.

Maybe if we use apic->send_IPI_mask(cpumask_of(cpu), NMI_VECTOR)

Quoted 7 lines

apic->send_IPI_mask(cpumask_of(cpu),NMI_VECTOR)
__x2apic_send_IPI_mask()

      __x2apic_send_IPI_dest()
	  unsigned long cfg = __prepare_ICR(0, vector, dest);
	  native_x2apic_icr_write(cfg, apicid);

__prepare_ICR() seems to have the magic for APIC_DM_NMI?

Bah. I clearly neither can read nor can remember any of the internals of that code:

Quoted 5 lines

apic->send_IPI()
x2apic_send_IPI()

    __x2apic_send_IPI_dest()

It also works for all other APIC incarnations.

Works!

From: Thomas Gleixner <[email protected]>
Subject: x86/smp: Make stop_other_cpus() more robust
Date: Wed, 26 Apr 2023 18:37:00 +0200

Tony reported intermittent lockups on poweroff. His analysis identified the wbinvd() in stop_this_cpu() as the culprit. This was added to ensure that on SME enabled machines a kexec() does not leave any stale data in the caches when switching from encrypted to non-encrypted mode or vice versa.

That wbindv() is conditional on the SME feature bit which is read directly

Hi Thomas,

Quoted 44 lines · Thomas Gleixner

Tony reported intermittent lockups on poweroff. His analysis identified the wbinvd() in stop_this_cpu() as the culprit. This was added to ensure that on SME enabled machines a kexec() does not leave any stale data in the caches when switching from encrypted to non-encrypted mode or vice versa.

That wbindv() is conditional on the SME feature bit which is read directly from CPUID. But that readout does not check whether the CPUID leaf is available or not. If it's not available the CPU will return the value of the highest supported leaf instead. Depending on the content the "SME" bit might be set or not.

That's incorrect but harmless. Making the CPUID readout conditional makes the observed hangs go away, but it does not fix the underlying problem:

CPU0 CPU1

stop_other_cpus()
send_IPIs(REBOOT); stop_this_cpu()

   while (num_online_cpus() > 1);         set_online(false);
   proceed... -> hang
				          wbinvd()

WBINVD is an expensive operation and if multiple CPUs issue it at the same time the resulting delays are even larger.

But CPU0 already observed num_online_cpus() going down to 1 and proceeds which causes the system to hang.

This issue exists independent of WBINVD, but the delays caused by WBINVD make it more prominent.

Make this more robust by adding a cpumask which is initialized to the online CPU mask before sending the IPIs and CPUs clear their bit in stop_this_cpu() after the WBINVD completed. Check for that cpumask to become empty in stop_other_cpus() instead of watching num_online_cpus().

The cpumask cannot plug all holes either, but it's better than a raw counter and allows to restrict the NMI fallback IPI to be sent only to the CPUs which have not reported within the timeout window.

Fixes 08f253ec3767 ("x86/cpu: Clear SME feature flag when not in use")

Reported-by Tony Battersby <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Link https://lore.kernel.org/all/[email protected] open here

Quoted 7 lines

---
V3: Use a cpumask to make the NMI case slightly safer - Ashok

---
 arch/x86/include/asm/cpu.h |    2 +
 arch/x86/kernel/process.c  |   23 +++++++++++++-
 arch/x86/kernel/smp.c      |   71 +++++++++++++++++++++++++++++++--------------
 3 files changed, 73 insertions(+), 23 deletions(-)

I tested them and seems to work fine on my system.

Maybe Tony can check in his setup would be great.

From: Tony Battersby <[email protected]>

stop_this_cpu() tests CPUID leaf 0x8000001f::EAX unconditionally. CPUs return the content of the highest supported leaf when a non-existing leaf is read. So the result of the test is lottery except on AMD CPUs which support that leaf.

While harmless it's incorrect and causes the conditional wbinvd() to be issued where not required.

Check whether the leaf is supported before reading it.

[ tglx: Adjusted changelog ]

Fixes 08f253ec3767 ("x86/cpu: Clear SME feature flag when not in use")

Signed-off-by Tony Battersby <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Link https://lore.kernel.org/r/[email protected] open here

---
  arch/x86/kernel/process.c |    5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

arch/x86/kernel/process.c +3 -2

@@ -763,6 +763,7 @@ struct cpumask cpus_stop_mask;
 
  void __noreturn stop_this_cpu(void *dummy)
  {
+	struct cpuinfo_x86 *c = this_cpu_ptr(&cpu_info);
  	unsigned int cpu = smp_processor_id();
 
  	local_irq_disable();

@@ -777,7 +778,7 @@ void __noreturn stop_this_cpu(void *dumm
  	 */
  	set_cpu_online(cpu, false);
  	disable_local_APIC();
-	mcheck_cpu_clear(this_cpu_ptr(&cpu_info));
+	mcheck_cpu_clear(c);
 
  	/*
  	 * Use wbinvd on processors that support SME. This provides support

@@ -791,7 +792,7 @@ void __noreturn stop_this_cpu(void *dumm
  	 * Test the CPUID bit directly because the machine might've cleared
  	 * X86_FEATURE_SME due to cmdline options.
  	 */
-	if (cpuid_eax(0x8000001f) & BIT(0))
+	if (c->extended_cpuid_level >= 0x8000001f && (cpuid_eax(0x8000001f) & BIT(0)))
  		native_wbinvd();
 
  	/*

Quoted 10 lines · Thomas Gleixner

From: Tony Battersby <[email protected]>

stop_this_cpu() tests CPUID leaf 0x8000001f::EAX unconditionally. CPUs return the content of the highest supported leaf when a non-existing leaf is read. So the result of the test is lottery except on AMD CPUs which support that leaf.

While harmless it's incorrect and causes the conditional wbinvd() to be issued where not required.

Quoted 5 lines

Check whether the leaf is supported before reading it.

[ tglx: Adjusted changelog ]

Fixes 08f253ec3767 ("x86/cpu: Clear SME feature flag when not in use")

Thanks for this fix.
This particular patch should probably also CC to stable.

Subject: Re: [patch v3 2/7] x86/smp: Dont access non-existing CPUID leaf

From: Tony Battersby <[email protected]>

stop_this_cpu() tests CPUID leaf 0x8000001f::EAX unconditionally. CPUs return the content of the highest supported leaf when a non-existing leaf is read.

The wmb()s before sending the IPIs are not synchronizing anything.

If at all then the apic IPI functions have to provide or act as appropriate barriers.

Remove these cargo cult barriers which have no explanation of what they are synchronizing.

Signed-off-by Thomas Gleixner <[email protected]>

---
V3: Remove second instance and reword changelog - PeterZ

---
 arch/x86/kernel/smp.c |    6 ------
 1 file changed, 6 deletions(-)

Monitoring idletask::thread_info::flags in mwait_play_dead() has been an obvious choice as all what is needed is a cache line which is not written by other CPUs.

But there is a use case where a "dead" CPU needs to be brought out of that mwait(): kexec().

The CPU needs to be brought out of mwait before kexec() as kexec() can

overwrite text, pagetables, stacks and the monitored cacheline of the original kernel.

The latter causes mwait to resume execution which
obviously causes havoc on the kexec kernel which results usually in triple faults.

TLDR: It's a mess.

When kexec() is executed on a system with "offline" CPUs, which are parked

in mwait_play_dead() it can end up in a triple fault during the bootup of the kexec kernel or cause hard to diagnose data corruption.

The reason is that kexec() eventually overwrites the previous kernels text,

page tables, data and stack, If it writes to the cache line which is

monitored by an previously offlined CPU, MWAIT resumes execution and ends

up executing the wrong text, dereferencing overwritten page tables or corrupting the kexec kernels data.

Cure this by bringing the offline CPUs out of MWAIT into HLT.

Write to the monitored cache line of each offline CPU, which makes MWAIT

resume execution. The written control word tells the offline CPUs to issue

HLT, which does not have the MWAIT problem.

That does not help, if a stray NMI, MCE or SMI hits the offline CPUs as those make it come out of HLT.

A follow up change will put them into INIT, which protects at least against NMI and SMI.

arch/x86/kernel/smpboot.c +18 -0

 		mb();
 		__mwait(eax, 0);
 
+		if (READ_ONCE(md->control) == CPUDEAD_MWAIT_KEXEC_HLT) {
+			/*
+			 * Kexec is about to happen. Don't go back into mwait() as
+			 * the kexec kernel might overwrite text and data including
+			 * page tables and stack. So mwait() would resume when the
+			 * monitor cache line is written to and then the CPU goes
+			 * south due to overwritten text, page tables and stack.
+			 *
+			 * Note: This does _NOT_ protect against a stray MCE, NMI,
+			 * SMI. They will resume execution at the instruction
+			 * following the HLT instruction and run into the problem
+			 * which this is trying to prevent.
+			 */
+			WRITE_ONCE(md->status, CPUDEAD_MWAIT_KEXEC_HLT);
+			while(1)
+				native_halt();
+		}
+
 		cond_wakeup_cpu0();

arch/x86/kernel/smpboot.c static inline void mwait_play_dead(void)

Quoted 3 lines · Thomas Gleixner

TLDR: It's a mess.

arch/x86/kernel/smpboot.c +0 -0

 	while (1) {

@@ -1824,10 +1836,57 @@ static inline void mwait_play_dead(void)

JFYI: that last hunk has some conflicts applying to latest tip/master. Might need merge resolving...

arch/x86/kernel/smpboot.c static inline void mwait_play_dead(void) · line 1867

Quoted 7 lines

arch/x86/kernel/smpboot.c +7 -0

+/*
+ * Kick all "offline" CPUs out of mwait on kexec(). See comment in
+ * mwait_play_dead().
+ */
+void smp_kick_mwait_play_dead(void)
+{
+	u32 newstate = CPUDEAD_MWAIT_KEXEC_HLT;

Do you even need this newstate thing?

arch/x86/kernel/smpboot.c static inline void mwait_play_dead(void) · line 1886

Quoted 3 lines

arch/x86/kernel/smpboot.c +3 -0

+
+		if (READ_ONCE(md->status) != newstate)
+			pr_err("CPU%u is stuck in mwait_play_dead()\n", cpu);

Shouldn't this be a pr_err_once thing so that it doesn't flood the console unnecessarily?

Putting CPUs into INIT is a safer place during kexec() to park CPUs.

Split the INIT assert/deassert sequence out so it can be reused.

Signed-off-by Thomas Gleixner <[email protected]>

Reviewed-by Ashok Raj <[email protected]>

---
V2: Fix rebase screwup

---
 arch/x86/kernel/smpboot.c |   49 ++++++++++++++++++----------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

arch/x86/kernel/smpboot.c +0 -0

@@ -853,47 +853,38 @@ wakeup_secondary_cpu_via_nmi(int apicid,
 	return (send_status | accept_status);
 }

arch/x86/kernel/smpboot.c wakeup_secondary_cpu_via_nmi(int apicid, · line 858

Quoted 10 lines · Thomas Gleixner

arch/x86/kernel/smpboot.c +2 -6

-static int
-wakeup_secondary_cpu_via_init(int phys_apicid, unsigned long start_eip)
+static void send_init_sequence(int phys_apicid)
 {
-	unsigned long send_status = 0, accept_status = 0;
-	int maxlvt, num_starts, j;
-
-	maxlvt = lapic_get_maxlvt();
+	int maxlvt = lapic_get_maxlvt();

Whoops:

arch/x86/kernel/smpboot.c: In function ‘send_init_sequence’:
arch/x86/kernel/smpboot.c:860:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

  860 |         int maxlvt = lapic_get_maxlvt();
      |         ^~~

arch/x86/kernel/smp.c +0 -0

@@ -202,7 +206,27 @@ static void native_stop_other_cpus(int w
 			udelay(1);
 	}
 

When I manually trigger panic in a qume x86 VM with

       echo c > /proc/sysrq-trigger,

 I find that the VM will probably reboot directly, but the
PANIC_TIMEOUT is 0.
This prevents us from exporting the vmcore via panic, and even if we succeed in panic exporting the vmcore, the processes in the vmcore are mostly stop_this_cpu(). By dichotomizing we found the patch that introduced the behavior change

   45e34c8af58f ("x86/smp: Put CPUs into INIT on shutdown if possible"),

Quoted 4 lines · Baokun Li

When I manually trigger panic in a qume x86 VM with

       echo c > /proc/sysrq-trigger,

Quoted 8 lines

 I find that the VM will probably reboot directly, but the
PANIC_TIMEOUT is 0.
This prevents us from exporting the vmcore via panic, and even if we succeed in panic exporting the vmcore, the processes in the vmcore are mostly stop_this_cpu(). By dichotomizing we found the patch that introduced the behavior change

   45e34c8af58f ("x86/smp: Put CPUs into INIT on shutdown if possible"),

Bah, I missed that this is used by crash too. So if this happens to be invoked on an AP, i.e. not on CPU 0, then the INIT will reset the machine. Fix below.

Thanks,

tglx
---

arch/x86/kernel/smpboot.c +1 -0

index ed2d51960a7d..e1aa2cd7734b 100644
@@ -1348,6 +1348,14 @@ bool smp_park_other_cpus_in_init(void)
  	if (apic->wakeup_secondary_cpu_64 || apic->wakeup_secondary_cpu)
  		return false;
 
+	/*

arch/x86/kernel/smpboot.c bool smp_park_other_cpus_in_init(void) · line 1357

Quoted 7 lines · Thomas Gleixner

arch/x86/kernel/smpboot.c +7 -0

+	/*
+	 * If this is a crash stop which does not execute on the boot CPU,
+	 * then this cannot use the INIT mechanism because INIT to the boot
+	 * CPU will reset the machine.
+	 */
+	if (this_cpu)
+		return false;

This patch does fix the problem of rebooting at panic, but the exported stack stays at stop_this_cpu() like below, instead of showing what the corresponding process is doing as before.

PID: 681      TASK: ffff9ac2429d3080  CPU: 2    COMMAND: "fsstress"
 #0 [ffffb00200184fd0] stop_this_cpu at ffffffff89a4ffd8 #1 [ffffb00200184fe8] __sysvec_reboot at ffffffff89a94213 #2 [ffffb00200184ff0] sysvec_reboot at ffffffff8aee7491
--- <IRQ stack> ---

     RIP: 0000000000000010  RSP: 0000000000000018  RFLAGS: ffffb00200f8bd08
     RAX: ffff9ac256fda9d8  RBX: 0000000009973a85  RCX: ffff9ac256fda078
     RDX: ffff9ac24416e300  RSI: ffff9ac256fda9e0  RDI: ffffffffffffffff
     RBP: ffff9ac2443a5f88   R8: 0000000000000000   R9: ffff9ac2422eeea0
     R10: ffff9ac256fda9d8  R11: 0000000000549921  R12: ffff9ac2422eeea0
     R13: ffff9ac251cd23c8  R14: ffff9ac24269a800  R15: ffff9ac251cd2150
     ORIG_RAX: ffffffff8a1719e4  CS: 0206  SS: ffffffff8a1719c8

bt: WARNING: possibly bogus exception frame

Do you know how this happened? I would be grateful if you could fix it.

bt: WARNING: possibly bogus exception frame

Quoted 8 lines · Baokun Li

arch/x86/kernel/smpboot.c bool smp_park_other_cpus_in_init(void) · line 1357

Quoted 7 lines · Thomas Gleixner

arch/x86/kernel/smpboot.c +7 -0

+	/*
+	 * If this is a crash stop which does not execute on the boot CPU,
+	 * then this cannot use the INIT mechanism because INIT to the boot
+	 * CPU will reset the machine.
+	 */
+	if (this_cpu)
+		return false;

Quoted 16 lines

This patch does fix the problem of rebooting at panic, but the exported stack stays at stop_this_cpu() like below, instead of showing what the corresponding process is doing as before.

PID: 681      TASK: ffff9ac2429d3080  CPU: 2    COMMAND: "fsstress"
 #0 [ffffb00200184fd0] stop_this_cpu at ffffffff89a4ffd8 #1 [ffffb00200184fe8] __sysvec_reboot at ffffffff89a94213 #2 [ffffb00200184ff0] sysvec_reboot at ffffffff8aee7491
--- <IRQ stack> ---

      RIP: 0000000000000010  RSP: 0000000000000018  RFLAGS: ffffb00200f8bd08
      RAX: ffff9ac256fda9d8  RBX: 0000000009973a85  RCX: ffff9ac256fda078
      RDX: ffff9ac24416e300  RSI: ffff9ac256fda9e0  RDI: ffffffffffffffff
      RBP: ffff9ac2443a5f88   R8: 0000000000000000   R9: ffff9ac2422eeea0
      R10: ffff9ac256fda9d8  R11: 0000000000549921  R12: ffff9ac2422eeea0
      R13: ffff9ac251cd23c8  R14: ffff9ac24269a800  R15: ffff9ac251cd2150
      ORIG_RAX: ffffffff8a1719e4  CS: 0206  SS: ffffffff8a1719c8

Quoted 3 lines

bt: WARNING: possibly bogus exception frame

Do you know how this happened? I would be grateful if you could fix it.

No, I don't. But there is clearly a hint:

Quoted 1 line

bt: WARNING: possibly bogus exception frame

So the exception frame seems to be corrupted. I have no idea why.

The question is, whether this goes away when you revert that commit or not. I can't oracle that out from your report.

Can you please revert 45e34c8af58f on top of Linus tree and verify that it makes the issue go away?

Thanks,

tglx