[RFC PATCH v2 00/12] Unify SMP stop generic logic to common code

[RFC PATCH v2 00/12] Unify SMP stop generic logic to common code

[Cristian Marussi]

Hi all,

the logic underlying SMP stop and kexec crash procedures, beside containing
some arch-specific bits, is mostly generic and common across all archs:
despite this fact, such logic is now scattered across all architectures and
on some of them is flawed, in such a way that, under some specific
conditions, you can end up with a CPU left still running after a panic and
possibly lost across a subsequent kexec crash reboot. [1]

Beside the flaws on some archs, there is anyway lots of code duplication,
so this patch series attempts to move into common code all the generic SMP
stop and crash logic, fixing observed issues, and leaving only the arch
specific bits inside properly provided arch-specific helpers.

An architecture willing to rely on this SMP common logic has to define its
own helpers and set CONFIG_ARCH_USE_COMMON_SMP_STOP=y.

This series wire this up for arm64, x86, arm, sparc64.

Behaviour is not changed for architectures not adopting this new common
logic.

In v2 the SMP common stop/crash code is generalized a bit more to address
the needs of the newly ported architectures (x86, arm, sparc64).

Tested as follows:

- arm64:
 1. boot/reboot/emergency reboot
 2. panic on a starting CPU within a 2 CPUs system (freezing properly)
 3. kexec reboot after a panic like 2. (not losing any CPU on reboot)
 4. kexec reboot after a panic like 2. and a simultaneous reboot
    (instrumenting code to delay the stop messages transmission
     to have time to inject a reboot -f)

- x86:
 1. boot/reboot/emergency reboot/emergency reboot with VMX enabled
 2. panic on a starting CPU within a 2 CPUs system
 2. panic sysrq 4-CPus
 3. kexec reboot after a panic

- arm:
1. boot

- sparc64:
1. build

A couple more of still to be done potential enhancements have been noted
on specific commits, and a lot more of testing remains surely to be done
as of now, but, in the context of this RFC, any thoughts on this approach
in general ?

Thanks,

Cristian


Changes:
--------

v1-->v2:
- rebased on v5.3-rc8
- moved common Kconfig bits to arch/Kconfig
- extended SMP common stop/crash generic code to address new architectures
  needs: custom wait/timeouts, max_retries, attempt numbers.
- ported x86 to SMP common stop/crash code
- ported arm to SMP common stop/crash code
- ported sparc64 to SMP common stop code


[1]

[root@arch ~]# echo 1 > /sys/devices/system/cpu/cpu1/online
[root@arch ~]# [  152.583368] ------------[ cut here ]------------
[  152.583872] kernel BUG at arch/arm64/kernel/cpufeature.c:852!
[  152.584693] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[  152.585228] Modules linked in:
[  152.586040] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.3.0-rc5-00001-gcabd12118c4a-dirty #2
[  152.586218] Hardware name: Foundation-v8A (DT)
[  152.586478] pstate: 000001c5 (nzcv dAIF -PAN -UAO)
[  152.587260] pc : has_cpuid_feature+0x35c/0x360
[  152.587398] lr : verify_local_elf_hwcaps+0x6c/0xf0
[  152.587520] sp : ffff0000118bbf60
[  152.587605] x29: ffff0000118bbf60 x28: 0000000000000000
[  152.587784] x27: 0000000000000000 x26: 0000000000000000
[  152.587882] x25: ffff00001167a010 x24: ffff0000112f59f8
[  152.587992] x23: 0000000000000000 x22: 0000000000000000
[  152.588085] x21: ffff0000112ea018 x20: ffff000010fe5518
[  152.588180] x19: ffff000010ba3f30 x18: 0000000000000036
[  152.588285] x17: 0000000000000000 x16: 0000000000000000
[  152.588380] x15: 0000000000000000 x14: ffff80087a821210
[  152.588481] x13: 0000000000000000 x12: 0000000000000000
[  152.588599] x11: 0000000000000080 x10: 00400032b5503510
[  152.588709] x9 : 0000000000000000 x8 : ffff000010b93204
[  152.588810] x7 : 00000000800001d8 x6 : 0000000000000005
[  152.588910] x5 : 0000000000000000 x4 : 0000000000000000
[  152.589021] x3 : 0000000000000000 x2 : 0000000000008000
[  152.589121] x1 : 0000000000180480 x0 : 0000000000180480
[  152.589379] Call trace:
[  152.589646]  has_cpuid_feature+0x35c/0x360
[  152.589763]  verify_local_elf_hwcaps+0x6c/0xf0
[  152.589858]  check_local_cpu_capabilities+0x88/0x118
[  152.589968]  secondary_start_kernel+0xc4/0x168
[  152.590530] Code: d53801e0 17ffff58 d5380600 17ffff56 (d4210000)
[  152.592215] ---[ end trace 80ea98416149c87e ]---
[  152.592734] Kernel panic - not syncing: Attempted to kill the idle task!
[  152.593173] Kernel Offset: disabled
[  152.593501] CPU features: 0x0004,20c02008
[  152.593678] Memory Limit: none
[  152.594208] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
[root@arch ~]# bash: echo: write error: Input/output error
[root@arch ~]#
[root@arch ~]#
[root@arch ~]# echo HELO
HELO


Cristian Marussi (12):
  smp: add generic SMP-stop support to common code
  smp: unify crash_ and smp_send_stop() logic
  smp: coordinate concurrent crash/smp stop calls
  smp: address races of starting CPUs while stopping
  arm64: smp: use generic SMP stop common code
  arm64: smp: use SMP crash-stop common code
  arm64: smp: add arch specific cpu parking helper
  x86: smp: use generic SMP stop common code
  x86: smp: use SMP crash-stop common code
  arm: smp: use generic SMP stop common code
  arm: smp: use SMP crash-stop common code
  sparc64: smp: use generic SMP stop common code

 arch/Kconfig                    |   7 ++
 arch/arm/Kconfig                |   1 +
 arch/arm/kernel/machine_kexec.c |  27 ++---
 arch/arm/kernel/smp.c           |  18 +---
 arch/arm64/Kconfig              |   1 +
 arch/arm64/include/asm/smp.h    |   2 -
 arch/arm64/kernel/smp.c         | 127 +++++++-----------------
 arch/sparc/Kconfig              |   1 +
 arch/sparc/kernel/smp_64.c      |  15 +--
 arch/x86/Kconfig                |   1 +
 arch/x86/include/asm/reboot.h   |   2 +
 arch/x86/include/asm/smp.h      |   6 --
 arch/x86/kernel/crash.c         |  27 +----
 arch/x86/kernel/reboot.c        |  50 ++++++----
 arch/x86/kernel/smp.c           |  98 ++++++++----------
 include/linux/smp.h             | 109 ++++++++++++++++++++
 kernel/panic.c                  |  26 -----
 kernel/smp.c                    | 170 ++++++++++++++++++++++++++++++++
 18 files changed, 418 insertions(+), 270 deletions(-)

-- 
2.17.1

[RFC PATCH v2 01/12] smp: add generic SMP-stop support to common code

[Cristian Marussi]

There was a lot of code duplication across architectures regarding the
SMP stop procedures' logic; moreover some of this duplicated code logic
happened to be similarly faulty across a few architectures: while fixing
such logic, move such generic logic as much as possible inside common
code.

Collect all the common logic related to SMP stop operations into the
common SMP code; any architecture willing to use such centralized logic
can select CONFIG_ARCH_USE_COMMON_STOP=y and provide the related
arch-specific helpers: in such a scenario, those architectures will
transparently start using the common code provided by smp_send_stop()
common function.

On the other side, Architectures not willing to use common code SMP stop
logic will simply leave CONFIG_ARCH_USE_COMMON_STOP undefined and carry
on executing their local arch-specific smp_send_stop() as before.

Suggested-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- moved related Kconfig to common code inside arch/Kconfig
- introduced additional CONFIG_USE_COMMON_STOP selected by
  CONFIG_ARCH_USE_COMMON_STOP
- introduced helpers to let architectures optionally alter
  the default common code behaviour while waiting for CPUs:
  change timeout or wait for ever. (will be needed by x86)
---
 arch/Kconfig        |  7 +++++
 include/linux/smp.h | 55 +++++++++++++++++++++++++++++++++++++
 kernel/smp.c        | 67 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 129 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index a7b57dd42c26..89766e6c0ac8 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -166,6 +166,13 @@ config ARCH_USE_BUILTIN_BSWAP
 	 instructions should set this. And it shouldn't hurt to set it
 	 on architectures that don't have such instructions.
 
+config ARCH_USE_COMMON_SMP_STOP
+       def_bool n
+
+config USE_COMMON_SMP_STOP
+       depends on SMP && ARCH_USE_COMMON_SMP_STOP
+       def_bool y
+
 config KRETPROBES
 	def_bool y
 	depends on KPROBES && HAVE_KRETPROBES
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 6fc856c9eda5..381a14bfcd96 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -77,6 +77,61 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd);
  */
 extern void smp_send_stop(void);
 
+#ifdef CONFIG_USE_COMMON_SMP_STOP
+static atomic_t wait_forever;
+static atomic_t wait_timeout = ATOMIC_INIT(USEC_PER_SEC);
+
+/*
+ * An Architecture can optionally decide to use this helper to change the
+ * waiting behaviour of common STOP logic, forcing to wait forever for
+ * all CPUs to be stopped.
+ */
+static inline void smp_stop_set_wait_forever(int wait)
+{
+	atomic_set(&wait_forever, wait);
+	/* ensure wait atomic-op is visible */
+	smp_mb__after_atomic();
+}
+
+/*
+ * An Architecture can optionally decide to use this helper to change the
+ * waiting timeout of common STOP logic. A ZERO timeout means no timeout
+ * at all as long as wait_forever was not previously set.
+ *
+ * Note that wait_forever and timeout must remain individually selectable:
+ * so you can temporarily request wait_forever while keeping the same timeout
+ * settings.
+ */
+static inline void smp_stop_set_wait_timeout_us(unsigned long timeout)
+{
+	atomic_set(&wait_timeout, timeout);
+	/* ensure timeout atomic-op is visible */
+	smp_mb__after_atomic();
+}
+
+/* Retrieve the current wait settings. */
+static inline bool smp_stop_get_wait_timeout_us(unsigned long *timeout)
+{
+	if (timeout)
+		*timeout = atomic_read(&wait_timeout);
+	return atomic_read(&wait_forever);
+}
+
+/*
+ * Any Architecture willing to use STOP common logic implementation
+ * MUST at least provide the arch_smp_stop_call() helper which is in
+ * charge of its own arch-specific CPU-stop mechanism.
+ */
+extern void arch_smp_stop_call(cpumask_t *cpus);
+
+/*
+ * An Architecture CAN also provide the arch_smp_cpus_stop_complete()
+ * dedicated helper, to perform any final arch-specific operation on
+ * the local CPU once the other CPUs have been successfully stopped.
+ */
+void arch_smp_cpus_stop_complete(void);
+#endif
+
 /*
  * sends a 'reschedule' event to another CPU:
  */
diff --git a/kernel/smp.c b/kernel/smp.c
index 7dbcb402c2fc..72f99bf13fd0 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -20,6 +20,7 @@
 #include <linux/sched.h>
 #include <linux/sched/idle.h>
 #include <linux/hypervisor.h>
+#include <linux/delay.h>
 
 #include "smpboot.h"
 
@@ -817,3 +818,69 @@ int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par, bool phys)
 	return sscs.ret;
 }
 EXPORT_SYMBOL_GPL(smp_call_on_cpu);
+
+#ifdef CONFIG_USE_COMMON_SMP_STOP
+void __weak arch_smp_cpus_stop_complete(void) { }
+
+static inline bool any_other_cpus_online(cpumask_t *mask,
+					 unsigned int this_cpu_id)
+{
+	cpumask_copy(mask, cpu_online_mask);
+	cpumask_clear_cpu(this_cpu_id, mask);
+
+	return !cpumask_empty(mask);
+}
+
+/*
+ * This centralizes the common logic to:
+ *
+ *  - evaluate which CPUs are online and needs to be notified for stop,
+ *    while considering properly the status of the calling CPU
+ *
+ *  - call the arch-specific helpers to request the effective stop
+ *
+ *  - wait for the stop operation to be completed across all involved CPUs
+ *    monitoring the cpu_online_mask
+ */
+void smp_send_stop(void)
+{
+	unsigned int this_cpu_id;
+	cpumask_t mask;
+
+	this_cpu_id = smp_processor_id();
+	if (any_other_cpus_online(&mask, this_cpu_id)) {
+		bool wait;
+		unsigned long timeout;
+		unsigned int this_cpu_online = cpu_online(this_cpu_id);
+
+		if (system_state <= SYSTEM_RUNNING)
+			pr_crit("stopping secondary CPUs\n");
+		arch_smp_stop_call(&mask);
+
+		/*
+		 * Defaults to wait up to one second for other CPUs to stop;
+		 * architectures can modify the default timeout or request
+		 * to wait forever.
+		 *
+		 * Here we rely simply on cpu_online_mask to sync with
+		 * arch-specific stop code without bloating the code with an
+		 * additional atomic_t ad-hoc counter.
+		 *
+		 * As a consequence we'll need proper explicit memory barriers
+		 * in case the other CPUs running the arch-specific stop-code
+		 * would need to commit to memory some data (like saved_regs).
+		 */
+		wait = smp_stop_get_wait_timeout_us(&timeout);
+		while (num_online_cpus() > this_cpu_online &&
+		       (wait || timeout--))
+			udelay(1);
+		/* ensure any stopping-CPUs memory access is made visible */
+		smp_rmb();
+		if (num_online_cpus() > this_cpu_online)
+			pr_warn("failed to stop secondary CPUs %*pbl\n",
+				cpumask_pr_args(cpu_online_mask));
+	}
+	/* Perform final (possibly arch-specific) work on this CPU */
+	arch_smp_cpus_stop_complete();
+}
+#endif
-- 
2.17.1

[RFC PATCH v2 02/12] smp: unify crash_ and smp_send_stop() logic

[Cristian Marussi]

crash_smp_send_stop() logic was fairly similar to smp_send_stop():
a lot of logic and code was duplicated between the two code paths and
across a few different architectures.

Unify this underlying common logic into the existing SMP common stop
code: use a common workhorse function for both paths to perform the
common tasks while taking care to propagate to the underlying
architecture code the intent of the stop operation: a simple stop or
a crash dump stop.

Relocate the __weak crash_smp_send_stop() function from panic.c to smp.c,
since it is the crash dump entry point for the crash stop process and now
calls into this new common logic (only if this latter is enabled by
ARCH_USE_COMMON_SMP_STOP=y).

Introduce a few more helpers so that the architectures willing to use
the common code logic can provide their arch-specific bits to handle
the differences between a stop and a crash stop; architectures can
anyway decide to override as a whole the common logic providing their
own custom solution in crash_smp_send_stop() (as it was before).

Provide also a new common code method to inquiry on the outcome of an
ongoing crash_stop procedure: smp_crash_stop_failed().

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- using new CONFIG_USE_COMMON_SMP_STOP
- added arch_smp_cpus_crash_complete()
---
 include/linux/smp.h | 34 +++++++++++++++++++++++
 kernel/panic.c      | 26 -----------------
 kernel/smp.c        | 68 ++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 98 insertions(+), 30 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 381a14bfcd96..0f95bbd2cb5c 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -130,8 +130,36 @@ extern void arch_smp_stop_call(cpumask_t *cpus);
  * the local CPU once the other CPUs have been successfully stopped.
  */
 void arch_smp_cpus_stop_complete(void);
+
+/*
+ * An Architecture CAN also provide the arch_smp_cpus_crash_complete()
+ * dedicated helper, to perform any final arch-specific operation on
+ * the local CPU once the other CPUs have been successfully crash stopped.
+ * When not overridden by the user, this defaults to call straight away
+ * arch_smp_cpus_stop_complete()
+ */
+void arch_smp_cpus_crash_complete(void);
+
+/*
+ * An Architecture CAN additionally provide the arch_smp_crash_call()
+ * helper which implements the arch-specific crash dump related operations.
+ *
+ * If such arch wants to fully support crash dump, this MUST be provided;
+ * when not provided the crash dump procedure will fallback to behave like
+ * a normal stop. (no saved regs, no arch-specific features disabled)
+ */
+extern void arch_smp_crash_call(cpumask_t *cpus);
+
+/* Helper to query the outcome of an ongoing crash_stop operation */
+bool smp_crash_stop_failed(void);
 #endif
 
+/*
+ * stops all CPUs but the current one propagating to all other CPUs
+ * the information that a crash_kexec is ongoing:
+ */
+void crash_smp_send_stop(void);
+
 /*
  * sends a 'reschedule' event to another CPU:
  */
@@ -195,6 +223,12 @@ static inline int get_boot_cpu_id(void)
 
 static inline void smp_send_stop(void) { }
 
+static inline void crash_smp_send_stop(void) { }
+
+#ifdef CONFIG_USE_COMMON_SMP_STOP
+static inline bool smp_crash_stop_failed(void) { }
+#endif
+
 /*
  *	These macros fold the SMP functionality into a single CPU system
  */
diff --git a/kernel/panic.c b/kernel/panic.c
index 057540b6eee9..bc0dbf9c9b75 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -86,32 +86,6 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
 	panic_smp_self_stop();
 }
 
-/*
- * Stop other CPUs in panic.  Architecture dependent code may override this
- * with more suitable version.  For example, if the architecture supports
- * crash dump, it should save registers of each stopped CPU and disable
- * per-CPU features such as virtualization extensions.
- */
-void __weak crash_smp_send_stop(void)
-{
-	static int cpus_stopped;
-
-	/*
-	 * This function can be called twice in panic path, but obviously
-	 * we execute this only once.
-	 */
-	if (cpus_stopped)
-		return;
-
-	/*
-	 * Note smp_send_stop is the usual smp shutdown function, which
-	 * unfortunately means it may not be hardened to work in a panic
-	 * situation.
-	 */
-	smp_send_stop();
-	cpus_stopped = 1;
-}
-
 atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
 
 /*
diff --git a/kernel/smp.c b/kernel/smp.c
index 72f99bf13fd0..b05d2648a168 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -820,8 +820,14 @@ int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par, bool phys)
 EXPORT_SYMBOL_GPL(smp_call_on_cpu);
 
 #ifdef CONFIG_USE_COMMON_SMP_STOP
+
 void __weak arch_smp_cpus_stop_complete(void) { }
 
+void __weak arch_smp_cpus_crash_complete(void)
+{
+	arch_smp_cpus_stop_complete();
+}
+
 static inline bool any_other_cpus_online(cpumask_t *mask,
 					 unsigned int this_cpu_id)
 {
@@ -831,6 +837,12 @@ static inline bool any_other_cpus_online(cpumask_t *mask,
 	return !cpumask_empty(mask);
 }
 
+void __weak arch_smp_crash_call(cpumask_t *cpus)
+{
+	pr_debug("SMP: Using generic %s() as SMP crash call.\n", __func__);
+	arch_smp_stop_call(cpus);
+}
+
 /*
  * This centralizes the common logic to:
  *
@@ -842,7 +854,7 @@ static inline bool any_other_cpus_online(cpumask_t *mask,
  *  - wait for the stop operation to be completed across all involved CPUs
  *    monitoring the cpu_online_mask
  */
-void smp_send_stop(void)
+static inline void __smp_send_stop_all(bool reason_crash)
 {
 	unsigned int this_cpu_id;
 	cpumask_t mask;
@@ -855,8 +867,11 @@ void smp_send_stop(void)
 
 		if (system_state <= SYSTEM_RUNNING)
 			pr_crit("stopping secondary CPUs\n");
-		arch_smp_stop_call(&mask);
-
+		/* smp and crash arch-backends helpers are kept distinct */
+		if (!reason_crash)
+			arch_smp_stop_call(&mask);
+		else
+			arch_smp_crash_call(&mask);
 		/*
 		 * Defaults to wait up to one second for other CPUs to stop;
 		 * architectures can modify the default timeout or request
@@ -881,6 +896,51 @@ void smp_send_stop(void)
 				cpumask_pr_args(cpu_online_mask));
 	}
 	/* Perform final (possibly arch-specific) work on this CPU */
-	arch_smp_cpus_stop_complete();
+	if (!reason_crash)
+		arch_smp_cpus_stop_complete();
+	else
+		arch_smp_cpus_crash_complete();
+}
+
+void smp_send_stop(void)
+{
+	__smp_send_stop_all(false);
 }
+
+bool __weak smp_crash_stop_failed(void)
+{
+	return (num_online_cpus() > cpu_online(smp_processor_id()));
+}
+#endif
+
+/*
+ * Stop other CPUs while passing down the additional information that a
+ * crash_kexec is ongoing: it's up to the architecture implementation
+ * decide what to do.
+ *
+ * For example, Architectures supporting crash dump should provide
+ * specialized support for saving registers and disabling per-CPU features
+ * like virtualization extensions.
+ *
+ * Behaviour in the CONFIG_USE_COMMON_SMP_STOP=n case is preserved
+ * as it was before.
+ */
+void __weak crash_smp_send_stop(void)
+{
+	static int cpus_stopped;
+
+	/*
+	 * This function can be called twice in panic path, but obviously
+	 * we execute this only once.
+	 */
+	if (cpus_stopped)
+		return;
+
+#ifdef CONFIG_USE_COMMON_SMP_STOP
+	__smp_send_stop_all(true);
+#else
+	smp_send_stop();
 #endif
+
+	cpus_stopped = 1;
+}
-- 
2.17.1

[RFC PATCH v2 03/12] smp: coordinate concurrent crash/smp stop calls

[Cristian Marussi]

Once a stop request is in progress on one CPU, it must be carefully
evaluated what to do if another stop request is issued concurrently
on another CPU.

Given that panic and crash dump code flows are mutually exclusive,
the only alternative possible scenario which instead could lead to
concurrent stop requests, is represented by the simultaneous stop
requests possibly triggered by a concurrent halt/reboot/shutdown.

In such a case, prioritize the panic/crash procedure and most importantly
immediately park the offending CPU that was attempting the concurrent stop
request: force it to idle quietly, waiting for the ongoing stop/dump
requests to arrive.

Failing to do so would result in the offending CPU being effectively lost
on the next possible reboot triggered by the crash dump. [1]

Another scenario, where the crash/stop code path was known to be executed
twice, was during a normal panic/crash with crash_kexec_post_notifiers=1;
since this issue is similar, fold also this case-handling into this new
logic.

[1]
<<<<<---------- TRIGGERED PANIC
[  225.676014] ------------[ cut here ]------------
[  225.676656] kernel BUG at arch/arm64/kernel/cpufeature.c:852!
[  225.677253] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[  225.677660] Modules linked in:
[  225.678458] CPU: 3 PID: 0 Comm: swapper/3 Kdump: loaded Not tainted 5.3.0-rc5-00004-gb8a210cd3c32-dirty #2
[  225.678621] Hardware name: Foundation-v8A (DT)
[  225.678868] pstate: 000001c5 (nzcv dAIF -PAN -UAO)
[  225.679523] pc : has_cpuid_feature+0x35c/0x360
[  225.679649] lr : verify_local_elf_hwcaps+0x6c/0xf0
[  225.679759] sp : ffff0000118cbf60
[  225.679855] x29: ffff0000118cbf60 x28: 0000000000000000
[  225.680026] x27: 0000000000000000 x26: 0000000000000000
[  225.680115] x25: ffff00001167a010 x24: ffff0000112f59f8
[  225.680207] x23: 0000000000000000 x22: 0000000000000000
[  225.680290] x21: ffff0000112ea018 x20: ffff000010fe5538
[  225.680372] x19: ffff000010ba3f30 x18: 000000000000001e
[  225.680465] x17: 0000000000000000 x16: 0000000000000000
[  225.680546] x15: 0000000000000000 x14: 0000000000000008
[  225.680629] x13: 0209018b7a9404f4 x12: 0000000000000001
[  225.680719] x11: 0000000000000080 x10: 00400032b5503510
[  225.680803] x9 : 0000000000000000 x8 : ffff000010b93204
[  225.680884] x7 : 00000000800001d8 x6 : 0000000000000005
[  225.680975] x5 : 0000000000000000 x4 : 0000000000000000
[  225.681056] x3 : 0000000000000000 x2 : 0000000000008000
[  225.681139] x1 : 0000000000180480 x0 : 0000000000180480
[  225.681423] Call trace:
[  225.681669]  has_cpuid_feature+0x35c/0x360
[  225.681762]  verify_local_elf_hwcaps+0x6c/0xf0
[  225.681836]  check_local_cpu_capabilities+0x88/0x118
[  225.681939]  secondary_start_kernel+0xc4/0x168
[  225.682432] Code: d53801e0 17ffff58 d5380600 17ffff56 (d4210000)
[  225.683998] smp: stopping secondary CPUs
[  225.684130] Delaying stop....   <<<<<------ INSTRUMENTED DEBUG_DELAY

Rebooting.                         <<<<<------ MANUAL SIMULTANEOUS REBOOT
[  232.647472] reboot: Restarting system
[  232.648363] Reboot failed -- System halted
[  239.552413] smp: failed to stop secondary CPUs 0
[  239.554730] Starting crashdump kernel...
[  239.555194] ------------[ cut here ]------------
[  239.555406] Some CPUs may be stale, kdump will be unreliable.
[  239.555802] WARNING: CPU: 3 PID: 0 at arch/arm64/kernel/machine_kexec.c:156 machine_kexec+0x3c/0x2b0
[  239.556044] Modules linked in:
[  239.556244] CPU: 3 PID: 0 Comm: swapper/3 Kdump: loaded Not tainted 5.3.0-rc5-00004-gb8a210cd3c32-dirty #2
[  239.556340] Hardware name: Foundation-v8A (DT)
[  239.556459] pstate: 600003c5 (nZCv DAIF -PAN -UAO)
[  239.556587] pc : machine_kexec+0x3c/0x2b0
[  239.556700] lr : machine_kexec+0x3c/0x2b0
[  239.556775] sp : ffff0000118cbad0
[  239.556876] x29: ffff0000118cbad0 x28: ffff80087a8c3700
[  239.557012] x27: 0000000000000000 x26: 0000000000000000
[  239.557278] x25: ffff000010fe3ef0 x24: 00000000000003c0
....
[  239.561568] Bye!
[    0.000000] Booting Linux on physical CPU 0x0000000003 [0x410fd0f0]
[    0.000000] Linux version 5.2.0-rc4-00001-g93bd4bc234d5-dirty
[    0.000000] Machine model: Foundation-v8A
...
[    0.197991] smp: Bringing up secondary CPUs ...
[    0.232643] psci: failed to boot CPU1 (-22)  <<<<--- LOST CPU ON REBOOT
[    0.232861] CPU1: failed to boot: -22
[    0.306291] Detected PIPT I-cache on CPU2
[    0.310524] GICv3: CPU2: found redistributor 1 region 0:0x000000002f120000
[    0.315618] CPU2: Booted secondary processor 0x0000000001 [0x410fd0f0]
[    0.395576] Detected PIPT I-cache on CPU3
[    0.400431] GICv3: CPU3: found redistributor 2 region 0:0x000000002f140000
[    0.407252] CPU3: Booted secondary processor 0x0000000002 [0x410fd0f0]
[    0.431811] smp: Brought up 1 node, 3 CPUs
[    0.439898] SMP: Total of 3 processors activated.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- using new CONFIG_USE_COMMON_SMP_STOP
---
 include/linux/smp.h |  3 +++
 kernel/smp.c        | 48 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 0f95bbd2cb5c..2ae7dd48b33c 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -152,6 +152,9 @@ extern void arch_smp_crash_call(cpumask_t *cpus);
 
 /* Helper to query the outcome of an ongoing crash_stop operation */
 bool smp_crash_stop_failed(void);
+
+/* A generic cpu parking helper, possibly overridden by architecture code */
+void arch_smp_cpu_park(void) __noreturn;
 #endif
 
 /*
diff --git a/kernel/smp.c b/kernel/smp.c
index b05d2648a168..d92ce59ae538 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -821,6 +821,12 @@ EXPORT_SYMBOL_GPL(smp_call_on_cpu);
 
 #ifdef CONFIG_USE_COMMON_SMP_STOP
 
+void __weak arch_smp_cpu_park(void)
+{
+	while (1)
+		cpu_relax();
+}
+
 void __weak arch_smp_cpus_stop_complete(void) { }
 
 void __weak arch_smp_cpus_crash_complete(void)
@@ -843,6 +849,9 @@ void __weak arch_smp_crash_call(cpumask_t *cpus)
 	arch_smp_stop_call(cpus);
 }
 
+#define	REASON_STOP	1
+#define	REASON_CRASH	2
+
 /*
  * This centralizes the common logic to:
  *
@@ -858,8 +867,38 @@ static inline void __smp_send_stop_all(bool reason_crash)
 {
 	unsigned int this_cpu_id;
 	cpumask_t mask;
+	static atomic_t stopping;
+	int was_reason;
 
 	this_cpu_id = smp_processor_id();
+	/* make sure that concurrent stop requests are handled properly */
+	was_reason = atomic_cmpxchg(&stopping, 0,
+				    reason_crash ? REASON_CRASH : REASON_STOP);
+	if (was_reason) {
+		/*
+		 * This function can be called twice in panic path if
+		 * crash_kexec is called with crash_kexec_post_notifiers=1,
+		 * but obviously we execute this only once.
+		 */
+		if (reason_crash && was_reason == REASON_CRASH)
+			return;
+		/*
+		 * In case of other concurrent STOP calls (like in a REBOOT
+		 * started simultaneously as an ongoing PANIC/CRASH/REBOOT)
+		 * we want to prioritize the ongoing PANIC/KEXEC call and
+		 * force here the offending CPU that was attempting the
+		 * concurrent STOP to just sit idle waiting to die.
+		 * Failing to do so would result in a lost CPU on the next
+		 * possible reboot triggered by crash_kexec().
+		 */
+		if (!reason_crash) {
+			pr_crit("CPU%d - kernel already stopping, parking myself.\n",
+				this_cpu_id);
+			local_irq_enable();
+			/* does not return */
+			arch_smp_cpu_park();
+		}
+	}
 	if (any_other_cpus_online(&mask, this_cpu_id)) {
 		bool wait;
 		unsigned long timeout;
@@ -927,6 +966,9 @@ bool __weak smp_crash_stop_failed(void)
  */
 void __weak crash_smp_send_stop(void)
 {
+#ifdef CONFIG_USE_COMMON_SMP_STOP
+	__smp_send_stop_all(true);
+#else
 	static int cpus_stopped;
 
 	/*
@@ -936,11 +978,7 @@ void __weak crash_smp_send_stop(void)
 	if (cpus_stopped)
 		return;
 
-#ifdef CONFIG_USE_COMMON_SMP_STOP
-	__smp_send_stop_all(true);
-#else
 	smp_send_stop();
-#endif
-
 	cpus_stopped = 1;
+#endif
 }
-- 
2.17.1

[RFC PATCH v2 04/12] smp: address races of starting CPUs while stopping

[Cristian Marussi]

Add to SMP stop common code a best-effort retry logic, re-issuing a stop
request when any CPU is detected to be still online after the first
stop request cycle has completed.

While retrying provide to architectures' helpers an 'attempt' argument
so that, after a possible first failed attempt, they can autonomously
decide to adopt different approaches in subsequent retries.

Address the case in which some CPUs could still be starting up when the
stop process is initiated, remaining so undetected and coming fully online
only after the SMP stop procedure was already started: such officially
still offline CPUs would be missed by an ongoing stop procedure.

Being a best effort approach, though, it is not always guaranteed to be
able to stop any CPU that happened to finally come online after the whole
SMP stop retry cycle has completed.
(i.e. the race-window is reduced but not eliminated)

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- added attempt_num param to arch helpers, to let arch implementation
  know if a retry is ongoing because some CPU failed to stop.
  (some arch like x86 attempts the retry with different means like NMI)
- added some helpers to let archs decide on the number of retries

---
A more deterministic approach has been also attempted in order to catch
any concurrently starting CPUs at the very last moment and make them
kill themselves by:

- adding a new set_cpus_stopping() in cpumask.h used to set a
  __cpus_stopping atomic internal flag

- modifying set_cpu_online() to check on __cpus_stopping only when
  coming online, and force the offending CPU to kill itself in that case

Anyway it has proved tricky and complex (beside faulty) to implement the
above 'kill-myself' phase in a reliable way while remaining architecture
agnostic and still distingushing properly regular stops from crash kexec.

So given that the main idea underlying this patch series was instead of
simplifying and unifying code and the residual races not caught by the
best-effort logic seemed not very likely, this more deterministic approach
has been dropped in favour of the best effort retry logic.

Moreover, the current retry logic will be anyway needed to support some
architectures, like x86, that prefer to use different CPU's stopping
methods in subsequent retries.
---
 include/linux/smp.h | 21 +++++++++++++++++++--
 kernel/smp.c        | 19 ++++++++++++-------
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/include/linux/smp.h b/include/linux/smp.h
index 2ae7dd48b33c..63e4322d52e1 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -78,8 +78,11 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd);
 extern void smp_send_stop(void);
 
 #ifdef CONFIG_USE_COMMON_SMP_STOP
+#define	DEFAULT_MAX_STOP_RETRIES	2
+
 static atomic_t wait_forever;
 static atomic_t wait_timeout = ATOMIC_INIT(USEC_PER_SEC);
+static atomic_t max_stop_retries = ATOMIC_INIT(DEFAULT_MAX_STOP_RETRIES);
 
 /*
  * An Architecture can optionally decide to use this helper to change the
@@ -117,12 +120,26 @@ static inline bool smp_stop_get_wait_timeout_us(unsigned long *timeout)
 	return atomic_read(&wait_forever);
 }
 
+/* Change common SMP STOP logic maximum retries */
+static inline void smp_stop_set_max_retries(unsigned int max_retries)
+{
+	atomic_set(&max_stop_retries, max_retries);
+	/* ensure retries atomics are visible */
+	smp_mb__after_atomic();
+}
+
+/* Get currently set maximum retries attempt */
+static inline unsigned int smp_stop_get_max_retries(void)
+{
+	return atomic_read(&max_stop_retries);
+}
+
 /*
  * Any Architecture willing to use STOP common logic implementation
  * MUST at least provide the arch_smp_stop_call() helper which is in
  * charge of its own arch-specific CPU-stop mechanism.
  */
-extern void arch_smp_stop_call(cpumask_t *cpus);
+extern void arch_smp_stop_call(cpumask_t *cpus, unsigned int attempt_num);
 
 /*
  * An Architecture CAN also provide the arch_smp_cpus_stop_complete()
@@ -148,7 +165,7 @@ void arch_smp_cpus_crash_complete(void);
  * when not provided the crash dump procedure will fallback to behave like
  * a normal stop. (no saved regs, no arch-specific features disabled)
  */
-extern void arch_smp_crash_call(cpumask_t *cpus);
+extern void arch_smp_crash_call(cpumask_t *cpus, unsigned int attempt_num);
 
 /* Helper to query the outcome of an ongoing crash_stop operation */
 bool smp_crash_stop_failed(void);
diff --git a/kernel/smp.c b/kernel/smp.c
index d92ce59ae538..5b966c62d557 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -843,10 +843,10 @@ static inline bool any_other_cpus_online(cpumask_t *mask,
 	return !cpumask_empty(mask);
 }
 
-void __weak arch_smp_crash_call(cpumask_t *cpus)
+void __weak arch_smp_crash_call(cpumask_t *cpus, unsigned int attempt_num)
 {
 	pr_debug("SMP: Using generic %s() as SMP crash call.\n", __func__);
-	arch_smp_stop_call(cpus);
+	arch_smp_stop_call(cpus, attempt_num);
 }
 
 #define	REASON_STOP	1
@@ -865,10 +865,10 @@ void __weak arch_smp_crash_call(cpumask_t *cpus)
  */
 static inline void __smp_send_stop_all(bool reason_crash)
 {
-	unsigned int this_cpu_id;
 	cpumask_t mask;
 	static atomic_t stopping;
 	int was_reason;
+	unsigned int this_cpu_id, max_retries, attempt = 0;
 
 	this_cpu_id = smp_processor_id();
 	/* make sure that concurrent stop requests are handled properly */
@@ -899,7 +899,9 @@ static inline void __smp_send_stop_all(bool reason_crash)
 			arch_smp_cpu_park();
 		}
 	}
-	if (any_other_cpus_online(&mask, this_cpu_id)) {
+	max_retries = smp_stop_get_max_retries();
+	while (++attempt <= max_retries &&
+	       any_other_cpus_online(&mask, this_cpu_id)) {
 		bool wait;
 		unsigned long timeout;
 		unsigned int this_cpu_online = cpu_online(this_cpu_id);
@@ -908,9 +910,9 @@ static inline void __smp_send_stop_all(bool reason_crash)
 			pr_crit("stopping secondary CPUs\n");
 		/* smp and crash arch-backends helpers are kept distinct */
 		if (!reason_crash)
-			arch_smp_stop_call(&mask);
+			arch_smp_stop_call(&mask, attempt);
 		else
-			arch_smp_crash_call(&mask);
+			arch_smp_crash_call(&mask, attempt);
 		/*
 		 * Defaults to wait up to one second for other CPUs to stop;
 		 * architectures can modify the default timeout or request
@@ -930,9 +932,12 @@ static inline void __smp_send_stop_all(bool reason_crash)
 			udelay(1);
 		/* ensure any stopping-CPUs memory access is made visible */
 		smp_rmb();
-		if (num_online_cpus() > this_cpu_online)
+		if (num_online_cpus() > this_cpu_online) {
 			pr_warn("failed to stop secondary CPUs %*pbl\n",
 				cpumask_pr_args(cpu_online_mask));
+			if (attempt < max_retries)
+				pr_warn("Retrying SMP stop call...\n");
+		}
 	}
 	/* Perform final (possibly arch-specific) work on this CPU */
 	if (!reason_crash)
-- 
2.17.1

[RFC PATCH v2 05/12] arm64: smp: use generic SMP stop common code

[Cristian Marussi]

Make arm64 use the generic SMP-stop logic provided by common code
unified smp_send_stop() function.

arm64 smp_send_stop() logic had a bug in it: it failed to consider the
online status of the calling CPU when evaluating if any stop message
needed to be sent to other CPus at all: this resulted, on a 2-CPUs
system, in the failure to stop all cpus if one paniced while starting
up, leaving such system in an unexpected lively state.

[root@arch ~]# echo 1 > /sys/devices/system/cpu/cpu1/online
[root@arch ~]# [  152.583368] ------------[ cut here ]------------
[  152.583872] kernel BUG at arch/arm64/kernel/cpufeature.c:852!
[  152.584693] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[  152.585228] Modules linked in:
[  152.586040] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.3.0-rc5-00001-gcabd12118c4a-dirty #2
[  152.586218] Hardware name: Foundation-v8A (DT)
[  152.586478] pstate: 000001c5 (nzcv dAIF -PAN -UAO)
[  152.587260] pc : has_cpuid_feature+0x35c/0x360
[  152.587398] lr : verify_local_elf_hwcaps+0x6c/0xf0
[  152.587520] sp : ffff0000118bbf60
[  152.587605] x29: ffff0000118bbf60 x28: 0000000000000000
[  152.587784] x27: 0000000000000000 x26: 0000000000000000
[  152.587882] x25: ffff00001167a010 x24: ffff0000112f59f8
[  152.587992] x23: 0000000000000000 x22: 0000000000000000
[  152.588085] x21: ffff0000112ea018 x20: ffff000010fe5518
[  152.588180] x19: ffff000010ba3f30 x18: 0000000000000036
[  152.588285] x17: 0000000000000000 x16: 0000000000000000
[  152.588380] x15: 0000000000000000 x14: ffff80087a821210
[  152.588481] x13: 0000000000000000 x12: 0000000000000000
[  152.588599] x11: 0000000000000080 x10: 00400032b5503510
[  152.588709] x9 : 0000000000000000 x8 : ffff000010b93204
[  152.588810] x7 : 00000000800001d8 x6 : 0000000000000005
[  152.588910] x5 : 0000000000000000 x4 : 0000000000000000
[  152.589021] x3 : 0000000000000000 x2 : 0000000000008000
[  152.589121] x1 : 0000000000180480 x0 : 0000000000180480
[  152.589379] Call trace:
[  152.589646]  has_cpuid_feature+0x35c/0x360
[  152.589763]  verify_local_elf_hwcaps+0x6c/0xf0
[  152.589858]  check_local_cpu_capabilities+0x88/0x118
[  152.589968]  secondary_start_kernel+0xc4/0x168
[  152.590530] Code: d53801e0 17ffff58 d5380600 17ffff56 (d4210000)
[  152.592215] ---[ end trace 80ea98416149c87e ]---
[  152.592734] Kernel panic - not syncing: Attempted to kill the idle task!
[  152.593173] Kernel Offset: disabled
[  152.593501] CPU features: 0x0004,20c02008
[  152.593678] Memory Limit: none
[  152.594208] ---[ end Kernel panic - not syncing: Attempted to kill the idle
task! ]---
[root@arch ~]# bash: echo: write error: Input/output error
[root@arch ~]#
[root@arch ~]#
[root@arch ~]# echo HELO
HELO

Get rid of such bug, switching arm64 to use the common SMP stop code.

Reported-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
v1 --> v2
- now selecting arch/Kconfig ARCH_USE_COMMON_SMP_STOP
- added attempt_num arch_smp_stop_call() helper
---
 arch/arm64/Kconfig      |  1 +
 arch/arm64/kernel/smp.c | 29 ++++++-----------------------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3adcec05b1f6..33f88381702b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -67,6 +67,7 @@ config ARM64
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
+	select ARCH_USE_COMMON_SMP_STOP
 	select ARCH_SUPPORTS_MEMORY_FAILURE
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_SUPPORTS_INT128 if GCC_VERSION >= 50000 || CC_IS_CLANG
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 018a33e01b0e..3e87fdbb9f74 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -953,33 +953,16 @@ void tick_broadcast(const struct cpumask *mask)
 }
 #endif
 
-void smp_send_stop(void)
+void arch_smp_cpus_stop_complete(void)
 {
-	unsigned long timeout;
-
-	if (num_online_cpus() > 1) {
-		cpumask_t mask;
-
-		cpumask_copy(&mask, cpu_online_mask);
-		cpumask_clear_cpu(smp_processor_id(), &mask);
-
-		if (system_state <= SYSTEM_RUNNING)
-			pr_crit("SMP: stopping secondary CPUs\n");
-		smp_cross_call(&mask, IPI_CPU_STOP);
-	}
-
-	/* Wait up to one second for other CPUs to stop */
-	timeout = USEC_PER_SEC;
-	while (num_online_cpus() > 1 && timeout--)
-		udelay(1);
-
-	if (num_online_cpus() > 1)
-		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
-			   cpumask_pr_args(cpu_online_mask));
-
 	sdei_mask_local_cpu();
 }
 
+void arch_smp_stop_call(cpumask_t *cpus, unsigned int __unused)
+{
+	smp_cross_call(cpus, IPI_CPU_STOP);
+}
+
 #ifdef CONFIG_KEXEC_CORE
 void crash_smp_send_stop(void)
 {
-- 
2.17.1

[RFC PATCH v2 06/12] arm64: smp: use SMP crash-stop common code

[Cristian Marussi]

Make arm64 use the SMP common implementation of crash_smp_send_stop() and
its generic logic, by removing the arm64 crash_smp_send_stop() definition
and providing the needed arch specific helpers.

Additionally, simplify the arch-specific stop and crash dump ISRs backends
(which are in charge of effectively receiving and interpreting the
stop/crash messages) and unify them as much as possible.

Using the SMP common code, it is no more needed to make use of an atomic_t
counter to make sure that each CPU had time to perform its crash dump
related shutdown-ops before the world ends: simply take care to synchronize
on cpu_online_mask, and add proper explicit memory barriers where needed.

Moreover, remove arm64 specific smp_crash_stop_failed() helper as a whole
and rely on the common code provided homonym function to lookup the state
of an ongoing crash_stop operation.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
v1 --> v2
- added attempt_num param to arch_smp_crash_call()
---
 arch/arm64/include/asm/smp.h |   2 -
 arch/arm64/kernel/smp.c      | 100 +++++++++--------------------------
 2 files changed, 26 insertions(+), 76 deletions(-)

diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index a0c8a0b65259..d98c409f9225 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -150,8 +150,6 @@ static inline void cpu_panic_kernel(void)
  */
 bool cpus_are_stuck_in_kernel(void);
 
-extern void crash_smp_send_stop(void);
-extern bool smp_crash_stop_failed(void);
 
 #endif /* ifndef __ASSEMBLY__ */
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 3e87fdbb9f74..f0cc2bf84aaa 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -825,12 +825,30 @@ void arch_irq_work_raise(void)
 }
 #endif
 
-static void local_cpu_stop(void)
+static void local_cpu_crash_or_stop(struct pt_regs *crash_regs)
 {
-	set_cpu_online(smp_processor_id(), false);
+	unsigned int cpu = smp_processor_id();
 
-	local_daif_mask();
+	if (IS_ENABLED(CONFIG_KEXEC_CORE) && crash_regs) {
+#ifdef CONFIG_KEXEC_CORE
+		/* crash stop requested: save regs before going offline */
+		crash_save_cpu(crash_regs, cpu);
+#endif
+		local_irq_disable();
+	} else {
+		local_daif_mask();
+	}
 	sdei_mask_local_cpu();
+	/* ensure dumped regs are visible once cpu is seen offline */
+	smp_wmb();
+	set_cpu_online(cpu, false);
+	/* ensure all writes are globally visible before cpu parks */
+	wmb();
+#if defined(CONFIG_KEXEC_CORE) && defined(CONFIG_HOTPLUG_CPU)
+	if (cpu_ops[cpu]->cpu_die)
+		cpu_ops[cpu]->cpu_die(cpu);
+#endif
+	/* just in case */
 	cpu_park_loop();
 }
 
@@ -841,31 +859,7 @@ static void local_cpu_stop(void)
  */
 void panic_smp_self_stop(void)
 {
-	local_cpu_stop();
-}
-
-#ifdef CONFIG_KEXEC_CORE
-static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
-#endif
-
-static void ipi_cpu_crash_stop(unsigned int cpu, struct pt_regs *regs)
-{
-#ifdef CONFIG_KEXEC_CORE
-	crash_save_cpu(regs, cpu);
-
-	atomic_dec(&waiting_for_crash_ipi);
-
-	local_irq_disable();
-	sdei_mask_local_cpu();
-
-#ifdef CONFIG_HOTPLUG_CPU
-	if (cpu_ops[cpu]->cpu_die)
-		cpu_ops[cpu]->cpu_die(cpu);
-#endif
-
-	/* just in case */
-	cpu_park_loop();
-#endif
+	local_cpu_crash_or_stop(NULL);
 }
 
 /*
@@ -894,14 +888,14 @@ void handle_IPI(int ipinr, struct pt_regs *regs)
 
 	case IPI_CPU_STOP:
 		irq_enter();
-		local_cpu_stop();
+		local_cpu_crash_or_stop(NULL);
 		irq_exit();
 		break;
 
 	case IPI_CPU_CRASH_STOP:
 		if (IS_ENABLED(CONFIG_KEXEC_CORE)) {
 			irq_enter();
-			ipi_cpu_crash_stop(cpu, regs);
+			local_cpu_crash_or_stop(regs);
 
 			unreachable();
 		}
@@ -963,52 +957,10 @@ void arch_smp_stop_call(cpumask_t *cpus, unsigned int __unused)
 	smp_cross_call(cpus, IPI_CPU_STOP);
 }
 
-#ifdef CONFIG_KEXEC_CORE
-void crash_smp_send_stop(void)
+void arch_smp_crash_call(cpumask_t *cpus, unsigned int __unused)
 {
-	static int cpus_stopped;
-	cpumask_t mask;
-	unsigned long timeout;
-
-	/*
-	 * This function can be called twice in panic path, but obviously
-	 * we execute this only once.
-	 */
-	if (cpus_stopped)
-		return;
-
-	cpus_stopped = 1;
-
-	if (num_online_cpus() == 1) {
-		sdei_mask_local_cpu();
-		return;
-	}
-
-	cpumask_copy(&mask, cpu_online_mask);
-	cpumask_clear_cpu(smp_processor_id(), &mask);
-
-	atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
-
-	pr_crit("SMP: stopping secondary CPUs\n");
-	smp_cross_call(&mask, IPI_CPU_CRASH_STOP);
-
-	/* Wait up to one second for other CPUs to stop */
-	timeout = USEC_PER_SEC;
-	while ((atomic_read(&waiting_for_crash_ipi) > 0) && timeout--)
-		udelay(1);
-
-	if (atomic_read(&waiting_for_crash_ipi) > 0)
-		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
-			   cpumask_pr_args(&mask));
-
-	sdei_mask_local_cpu();
-}
-
-bool smp_crash_stop_failed(void)
-{
-	return (atomic_read(&waiting_for_crash_ipi) > 0);
+	smp_cross_call(cpus, IPI_CPU_CRASH_STOP);
 }
-#endif
 
 /*
  * not supported here
-- 
2.17.1

[RFC PATCH v2 07/12] arm64: smp: add arch specific cpu parking helper

[Cristian Marussi]

Add an arm64 specific helper which parks the cpu in a more architecture
efficient way.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 arch/arm64/kernel/smp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index f0cc2bf84aaa..539e8db5c1ba 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -947,6 +947,12 @@ void tick_broadcast(const struct cpumask *mask)
 }
 #endif
 
+void arch_smp_cpu_park(void)
+{
+	while (1)
+		cpu_park_loop();
+}
+
 void arch_smp_cpus_stop_complete(void)
 {
 	sdei_mask_local_cpu();
-- 
2.17.1

[RFC PATCH v2 08/12] x86: smp: use generic SMP stop common code

[Cristian Marussi]

Make x86 use the generic SMP-stop logic provided by common code unified
smp_send_stop() function.

Introduce needed arch_smp_stop_call()/arch_smp_cpus_stop_complete()
helpers that implement the backend architectures specific functionalities
previously provided by native_stop_other_cpus(): common logic is now
delegated to common SMP stop code.

Remove arch-specific smp_send_stop(), and redefine original function
native_stop_other_cpus() to rely instead on the unified common code
version of smp_send_stop(): native_stop_other_cpus() is anyway kept
since it is wired to smp_ops.stop_other_cpus() which get called at
reboot time with particular waiting settings.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Note that in this patch we kept in use the original x86 local handling
of 'stopping_cpu' variable:

	atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id());

Instead, common SMP stop code could have been easily extended to keep and
expose to architectures backends such value using some additional helper
like smp_stop_get_stopping_cpu_id().

This has not been addressed in this series.
---
 arch/x86/Kconfig           |  1 +
 arch/x86/include/asm/smp.h |  5 --
 arch/x86/kernel/smp.c      | 95 ++++++++++++++++----------------------
 3 files changed, 41 insertions(+), 60 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 222855cc0158..0fcee2f03a5b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -92,6 +92,7 @@ config X86
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
+	select ARCH_USE_COMMON_SMP_STOP
 	select ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
 	select ARCH_WANTS_DYNAMIC_TASK_STRUCT
 	select ARCH_WANT_HUGE_PMD_SHARE
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index e1356a3b8223..5cf590259d68 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -67,11 +67,6 @@ extern void set_cpu_sibling_map(int cpu);
 #ifdef CONFIG_SMP
 extern struct smp_ops smp_ops;
 
-static inline void smp_send_stop(void)
-{
-	smp_ops.stop_other_cpus(0);
-}
-
 static inline void stop_other_cpus(void)
 {
 	smp_ops.stop_other_cpus(1);
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 96421f97e75c..0942cae46fee 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -179,78 +179,63 @@ asmlinkage __visible void smp_reboot_interrupt(void)
 	irq_exit();
 }
 
-static void native_stop_other_cpus(int wait)
+void arch_smp_stop_call(cpumask_t *cpus, unsigned int attempt_num)
 {
-	unsigned long flags;
-	unsigned long timeout;
-
 	if (reboot_force)
 		return;
 
-	/*
-	 * Use an own vector here because smp_call_function
-	 * does lots of things not suitable in a panic situation.
-	 */
-
-	/*
-	 * We start by using the REBOOT_VECTOR irq.
-	 * The irq is treated as a sync point to allow critical
-	 * regions of code on other cpus to release their spin locks
-	 * and re-enable irqs.  Jumping straight to an NMI might
-	 * accidentally cause deadlocks with further shutdown/panic
-	 * code.  By syncing, we give the cpus up to one second to
-	 * finish their work before we force them off with the NMI.
-	 */
-	if (num_online_cpus() > 1) {
-		/* did someone beat us here? */
-		if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1)
-			return;
-
-		/* sync above data before sending IRQ */
-		wmb();
-
-		apic->send_IPI_allbutself(REBOOT_VECTOR);
-
+	if (attempt_num == 1) {
 		/*
-		 * Don't wait longer than a second if the caller
-		 * didn't ask us to wait.
+		 * We start by using the REBOOT_VECTOR irq.
+		 * The irq is treated as a sync point to allow critical
+		 * regions of code on other cpus to release their spin locks
+		 * and re-enable irqs.  Jumping straight to an NMI might
+		 * accidentally cause deadlocks with further shutdown/panic
+		 * code.  By syncing, we give the cpus up to one second to
+		 * finish their work before we force them off with the NMI.
 		 */
-		timeout = USEC_PER_SEC;
-		while (num_online_cpus() > 1 && (wait || timeout--))
-			udelay(1);
-	}
-	
-	/* if the REBOOT_VECTOR didn't work, try with the NMI */
-	if ((num_online_cpus() > 1) && (!smp_no_nmi_ipi))  {
-		if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
-					 NMI_FLAG_FIRST, "smp_stop"))
-			/* Note: we ignore failures here */
-			/* Hope the REBOOT_IRQ is good enough */
-			goto finish;
+
+		/* Used by NMI handler callback to skip the stopping_cpu. */
+		atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id());
 
 		/* sync above data before sending IRQ */
 		wmb();
-
-		pr_emerg("Shutting down cpus with NMI\n");
-
-		apic->send_IPI_allbutself(NMI_VECTOR);
-
-		/*
-		 * Don't wait longer than a 10 ms if the caller
-		 * didn't ask us to wait.
-		 */
-		timeout = USEC_PER_MSEC * 10;
-		while (num_online_cpus() > 1 && (wait || timeout--))
-			udelay(1);
+		apic->send_IPI_mask(cpus, REBOOT_VECTOR);
+	} else if (attempt_num > 1 && !smp_no_nmi_ipi) {
+		/* if the REBOOT_VECTOR didn't work, try with the NMI */
+
+		/* Don't wait longer than 10 ms when not asked to wait */
+		smp_stop_set_wait_timeout_us(USEC_PER_MSEC * 10);
+
+		/* Note: we ignore failures here */
+		/* Hope the REBOOT_IRQ was good enough */
+		if (!register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback,
+					  NMI_FLAG_FIRST, "smp_stop")) {
+			/* sync above data before sending IRQ */
+			wmb();
+			pr_emerg("Shutting down cpus with NMI\n");
+			apic->send_IPI_mask(cpus, NMI_VECTOR);
+		}
 	}
+}
+
+void arch_smp_cpus_stop_complete(void)
+{
+	unsigned long flags;
 
-finish:
 	local_irq_save(flags);
 	disable_local_APIC();
 	mcheck_cpu_clear(this_cpu_ptr(&cpu_info));
 	local_irq_restore(flags);
 }
 
+static void native_stop_other_cpus(int wait)
+{
+	smp_stop_set_wait_forever(wait);
+	/* use common SMP stop code */
+	smp_send_stop();
+}
+
 /*
  * Reschedule call back. KVM uses this interrupt to force a cpu out of
  * guest mode
-- 
2.17.1

[RFC PATCH v2 09/12] x86: smp: use SMP crash-stop common code

[Cristian Marussi]

Make x86 use the SMP common implementation of crash_smp_send_stop() and
its generic logic, by removing the x86 crash_smp_send_stop() definition
and providing the needed arch specific helpers.

Remove also redundant smp_ops.crash_stop_other_cpus(); add shared util
function common_nmi_shootdown_cpus(), which is a generalization of the
previous nmi_shootdown_cpus(), and it is used by architecture backend
helper arch_smp_crash_call().

Modify original crash_nmi_callback() to properly set cpu offline flag
and adding needed memory barriers.

Modify original nmi_shootdown_cpus() to rely on common code logic
provided by generic crash_smp_send_stop(): this was needed because the
original nmi_shootdown_cpus() was used also on the emergency reboot
path, employing a different callback. Reuse the same shootdown_callback
mechanism to properly handle both a crash and an emergency reboot through
the same common code crash path.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Note that in this patch we kept in use the original x86 local handling
of 'crashing_cpu' variable:

	crashing_cpu = safe_smp_processor_id();

Instead, common SMP stop code could have been easily extended to keep and
expose to architectures backends such value using some additional helper
like smp_stop_get_stopping_cpu_id().

This has not been addressed in this series.
---
 arch/x86/include/asm/reboot.h |  2 ++
 arch/x86/include/asm/smp.h    |  1 -
 arch/x86/kernel/crash.c       | 27 ++++---------------
 arch/x86/kernel/reboot.c      | 50 ++++++++++++++++++++++-------------
 arch/x86/kernel/smp.c         |  3 ---
 5 files changed, 38 insertions(+), 45 deletions(-)

diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.h
index 04c17be9b5fd..a1a9cbed6df5 100644
--- a/arch/x86/include/asm/reboot.h
+++ b/arch/x86/include/asm/reboot.h
@@ -3,6 +3,7 @@
 #define _ASM_X86_REBOOT_H
 
 #include <linux/kdebug.h>
+#include <linux/cpumask.h>
 
 struct pt_regs;
 
@@ -28,6 +29,7 @@ void __noreturn machine_real_restart(unsigned int type);
 typedef void (*nmi_shootdown_cb)(int, struct pt_regs*);
 void nmi_panic_self_stop(struct pt_regs *regs);
 void nmi_shootdown_cpus(nmi_shootdown_cb callback);
+void common_nmi_shootdown_cpus(cpumask_t *cpus, nmi_shootdown_cb callback);
 void run_crash_ipi_callback(struct pt_regs *regs);
 
 #endif /* _ASM_X86_REBOOT_H */
diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index 5cf590259d68..684643ad71e4 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -49,7 +49,6 @@ struct smp_ops {
 	void (*smp_cpus_done)(unsigned max_cpus);
 
 	void (*stop_other_cpus)(int wait);
-	void (*crash_stop_other_cpus)(void);
 	void (*smp_send_reschedule)(int cpu);
 
 	int (*cpu_up)(unsigned cpu, struct task_struct *tidle);
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 2bf70a2fed90..8fff06c7cd26 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -96,34 +96,16 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 	disable_local_APIC();
 }
 
-void kdump_nmi_shootdown_cpus(void)
+void arch_smp_crash_call(cpumask_t *cpus, unsigned int __unused)
 {
-	nmi_shootdown_cpus(kdump_nmi_callback);
-
-	disable_local_APIC();
+	common_nmi_shootdown_cpus(cpus, kdump_nmi_callback);
 }
 
-/* Override the weak function in kernel/panic.c */
-void crash_smp_send_stop(void)
+void arch_smp_cpus_crash_complete(void)
 {
-	static int cpus_stopped;
-
-	if (cpus_stopped)
-		return;
-
-	if (smp_ops.crash_stop_other_cpus)
-		smp_ops.crash_stop_other_cpus();
-	else
-		smp_send_stop();
-
-	cpus_stopped = 1;
+	disable_local_APIC();
 }
 
-#else
-void crash_smp_send_stop(void)
-{
-	/* There are no cpus to shootdown */
-}
 #endif
 
 void native_machine_crash_shutdown(struct pt_regs *regs)
@@ -139,6 +121,7 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 	/* The kernel is broken so disable interrupts */
 	local_irq_disable();
 
+	/* calling into SMP common stop code */
 	crash_smp_send_stop();
 
 	/*
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 09d6bded3c1e..69f894e28fec 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -799,7 +799,6 @@ int crashing_cpu = -1;
 
 static nmi_shootdown_cb shootdown_callback;
 
-static atomic_t waiting_for_crash_ipi;
 static int crash_ipi_issued;
 
 static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
@@ -819,7 +818,12 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 
 	shootdown_callback(cpu, regs);
 
-	atomic_dec(&waiting_for_crash_ipi);
+	/* ensure all shootdown writes are visible once cpu is seen offline */
+	smp_wmb();
+	set_cpu_online(cpu, false);
+	/* ensure all writes are globally visible before cpu parks */
+	wmb();
+
 	/* Assume hlt works */
 	halt();
 	for (;;)
@@ -828,29 +832,44 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 	return NMI_HANDLED;
 }
 
-static void smp_send_nmi_allbutself(void)
-{
-	apic->send_IPI_allbutself(NMI_VECTOR);
-}
-
 /*
  * Halt all other CPUs, calling the specified function on each of them
  *
  * This function can be used to halt all other CPUs on crash
  * or emergency reboot time. The function passed as parameter
  * will be called inside a NMI handler on all CPUs.
+ *
+ * It relies on crash_smp_send_stop() common code logic to shutdown CPUs.
  */
 void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 {
-	unsigned long msecs;
+	local_irq_disable();
+
+	shootdown_callback = callback;
+	/* ensure callback in place before calling into common crash code */
+	wmb();
+	/* call into common SMP Crash Stop to reuse the logic */
+	crash_smp_send_stop();
+}
+
+/*
+ * Halt the specified @cpus, calling the provided @callback on each of them
+ * unless a shootdown_callback was already installed previously: this way
+ * we can handle here also the emergency reboot requests issued via
+ * nmi_shootdown_cpus() and routed via usual common code crash_smp_send_stop()
+ *
+ * Called by arch_smp_crash_call() arch-helper.
+ */
+void common_nmi_shootdown_cpus(cpumask_t *cpus, nmi_shootdown_cb callback)
+{
 	local_irq_disable();
 
 	/* Make a note of crashing cpu. Will be used in NMI callback. */
 	crashing_cpu = safe_smp_processor_id();
 
-	shootdown_callback = callback;
-
-	atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
+	/* skip when the callback has been already set by nmi_shootdown_cpus */
+	if (!shootdown_callback)
+		shootdown_callback = callback;
 	/* Would it be better to replace the trap vector here? */
 	if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback,
 				 NMI_FLAG_FIRST, "crash"))
@@ -860,18 +879,11 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	 * out the NMI
 	 */
 	wmb();
-
-	smp_send_nmi_allbutself();
+	apic->send_IPI_mask(cpus, NMI_VECTOR);
 
 	/* Kick CPUs looping in NMI context. */
 	WRITE_ONCE(crash_ipi_issued, 1);
 
-	msecs = 1000; /* Wait at most a second for the other cpus to stop */
-	while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
-		mdelay(1);
-		msecs--;
-	}
-
 	/* Leave the nmi callback set */
 }
 
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 0942cae46fee..d718b185c6a9 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -295,9 +295,6 @@ struct smp_ops smp_ops = {
 	.smp_cpus_done		= native_smp_cpus_done,
 
 	.stop_other_cpus	= native_stop_other_cpus,
-#if defined(CONFIG_KEXEC_CORE)
-	.crash_stop_other_cpus	= kdump_nmi_shootdown_cpus,
-#endif
 	.smp_send_reschedule	= native_smp_send_reschedule,
 
 	.cpu_up			= native_cpu_up,
-- 
2.17.1

[RFC PATCH v2 10/12] arm: smp: use generic SMP stop common code

[Cristian Marussi]

Make arm use the generic SMP-stop logic provided by common code
unified smp_send_stop() function.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 arch/arm/Kconfig      |  1 +
 arch/arm/kernel/smp.c | 18 ++----------------
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 24360211534a..0a3a9c9a0b2e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -34,6 +34,7 @@ config ARM
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF
+	select ARCH_USE_COMMON_SMP_STOP
 	select ARCH_WANT_IPC_PARSE_VERSION
 	select BINFMT_FLAT_ARGVP_ENVP_ON_STACK
 	select BUILDTIME_EXTABLE_SORT if MMU
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index aab8ba40ce38..bf5c04691fab 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -703,23 +703,9 @@ void smp_send_reschedule(int cpu)
 	smp_cross_call(cpumask_of(cpu), IPI_RESCHEDULE);
 }
 
-void smp_send_stop(void)
+void arch_smp_stop_call(cpumask_t *cpus, unsigned int __unused)
 {
-	unsigned long timeout;
-	struct cpumask mask;
-
-	cpumask_copy(&mask, cpu_online_mask);
-	cpumask_clear_cpu(smp_processor_id(), &mask);
-	if (!cpumask_empty(&mask))
-		smp_cross_call(&mask, IPI_CPU_STOP);
-
-	/* Wait up to one second for other CPUs to stop */
-	timeout = USEC_PER_SEC;
-	while (num_online_cpus() > 1 && timeout--)
-		udelay(1);
-
-	if (num_online_cpus() > 1)
-		pr_warn("SMP: failed to stop secondary CPUs\n");
+	smp_cross_call(cpus, IPI_CPU_STOP);
 }
 
 /* In case panic() and panic() called at the same time on CPU1 and CPU2,
-- 
2.17.1

[RFC PATCH v2 11/12] arm: smp: use SMP crash-stop common code

[Cristian Marussi]

Make arm use the SMP common implementation of crash_smp_send_stop() and
its generic logic, by removing the arm crash_smp_send_stop() definition
and providing the needed arch specific helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 arch/arm/kernel/machine_kexec.c | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
index 76300f3813e8..d53973cae362 100644
--- a/arch/arm/kernel/machine_kexec.c
+++ b/arch/arm/kernel/machine_kexec.c
@@ -29,8 +29,6 @@ extern unsigned long kexec_indirection_page;
 extern unsigned long kexec_mach_type;
 extern unsigned long kexec_boot_atags;
 
-static atomic_t waiting_for_crash_ipi;
-
 /*
  * Provide a dummy crash_notes definition while crash dump arrives to arm.
  * This prevents breakage of crash_notes attribute in kernel/ksysfs.c.
@@ -89,34 +87,21 @@ void machine_crash_nonpanic_core(void *unused)
 	crash_save_cpu(&regs, smp_processor_id());
 	flush_cache_all();
 
+	/* ensure saved regs writes are visible before going offline */
+	smp_wmb();
 	set_cpu_online(smp_processor_id(), false);
-	atomic_dec(&waiting_for_crash_ipi);
 
+	/* ensure all writes visible before parking */
+	wmb();
 	while (1) {
 		cpu_relax();
 		wfe();
 	}
 }
 
-void crash_smp_send_stop(void)
+void arch_smp_crash_call(cpumask_t *cpus, unsigned int __unused)
 {
-	static int cpus_stopped;
-	unsigned long msecs;
-
-	if (cpus_stopped)
-		return;
-
-	atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
-	smp_call_function(machine_crash_nonpanic_core, NULL, false);
-	msecs = 1000; /* Wait at most a second for the other cpus to stop */
-	while ((atomic_read(&waiting_for_crash_ipi) > 0) && msecs) {
-		mdelay(1);
-		msecs--;
-	}
-	if (atomic_read(&waiting_for_crash_ipi) > 0)
-		pr_warn("Non-crashing CPUs did not react to IPI\n");
-
-	cpus_stopped = 1;
+	smp_call_function_many(cpus, machine_crash_nonpanic_core, NULL, false);
 }
 
 static void machine_kexec_mask_interrupts(void)
-- 
2.17.1

[RFC PATCH v2 12/12] sparc64: smp: use generic SMP stop common code

[Cristian Marussi]

Make sparc64 use the generic SMP-stop logic provided by common code
unified smp_send_stop() function.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 arch/sparc/Kconfig         |  1 +
 arch/sparc/kernel/smp_64.c | 15 ++++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 7926a2e11bdc..67d8bb741378 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -95,6 +95,7 @@ config SPARC64
 	select ARCH_HAS_PTE_SPECIAL
 	select PCI_DOMAINS if PCI
 	select ARCH_HAS_GIGANTIC_PAGE
+	select ARCH_USE_COMMON_SMP_STOP
 
 config ARCH_DEFCONFIG
 	string
diff --git a/arch/sparc/kernel/smp_64.c b/arch/sparc/kernel/smp_64.c
index a8275fea4b70..759e5fd867c5 100644
--- a/arch/sparc/kernel/smp_64.c
+++ b/arch/sparc/kernel/smp_64.c
@@ -1537,7 +1537,12 @@ static void stop_this_cpu(void *dummy)
 	prom_stopself();
 }
 
-void smp_send_stop(void)
+void arch_smp_cpus_stop_complete(void)
+{
+	smp_call_function(stop_this_cpu, NULL, 0);
+}
+
+void arch_smp_stop_call(cpumask_t *cpus, unsigned int __unused)
 {
 	int cpu;
 
@@ -1546,10 +1551,7 @@ void smp_send_stop(void)
 #ifdef CONFIG_SERIAL_SUNHV
 		sunhv_migrate_hvcons_irq(this_cpu);
 #endif
-		for_each_online_cpu(cpu) {
-			if (cpu == this_cpu)
-				continue;
-
+		for_each_cpu(cpu, cpus) {
 			set_cpu_online(cpu, false);
 #ifdef CONFIG_SUN_LDOMS
 			if (ldom_domaining_enabled) {
@@ -1562,8 +1564,7 @@ void smp_send_stop(void)
 #endif
 				prom_stopcpu_cpuid(cpu);
 		}
-	} else
-		smp_call_function(stop_this_cpu, NULL, 0);
+	}
 }
 
 /**
-- 
2.17.1

Re: [RFC PATCH v2 00/12] Unify SMP stop generic logic to common code

[Russell King - ARM Linux admin]

On Fri, Sep 13, 2019 at 07:19:41PM +0100, Cristian Marussi wrote:
> Tested as follows:
> 
> - arm:
> 1. boot

So this basically means the code paths you're touching are untested on
ARM... given that, and the variety of systems we have out there, why
should the patches touching ARM be taken?

Given that you're an ARM Ltd employee, I'm sure you can find 32-bit
systems to test - or have ARM Ltd got rid of everything that isn't
64-bit? ;)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH v2 00/12] Unify SMP stop generic logic to common code

[Cristian Marussi]

On 13/09/2019 19:27, Russell King - ARM Linux admin wrote:
> On Fri, Sep 13, 2019 at 07:19:41PM +0100, Cristian Marussi wrote:
>> Tested as follows:
>>
>> - arm:
>> 1. boot
> 
> So this basically means the code paths you're touching are untested on
> ARM... given that, and the variety of systems we have out there, why
> should the patches touching ARM be taken?
> 

Yes, but sincerely it's an RFC, so I was not expecting any change to be picked up
by anyone at this stage: the expectation was to have some feedback on the general
approach used in the common code side of the series (patches 01-02-03-04):

is it worth ? is it over-engineered ? is it badly coded ? is it complete crap ?

In fact in the cover letter I stated:

> A couple more of still to be done potential enhancements have been noted
> on specific commits, and a lot more of testing remains surely to be done
> as of now, but, in the context of this RFC, any thoughts on this approach
> in general ?

I didn't want to port and test a lot of architectures before having some basic
feedback: in fact I did port more than one arch just to verify if they could
easily all fit into the new common code logic/layout I introduced, and, also,
to show that it could be generally useful to more than on arch. (as asked in V1)

As you noticed, though, I did certainly test as of now a lot more on some of them:

- arm64: because is where the initial bug was observed, so I had to verify if all
  of the above at least also fixed something at the end

- x86: because the original x86 SMP stop code differs more than other archs and so
  it was a good challenge to see if it could fit inside the new common SMP code logic
  (and in fact I had to extend the common framework to fit also x86 SMP stop needs)

Moreover within this series structure it is not mandatory for all archs to switch to the
new common logic: if not deemed important they can simply stick to their old code, while
other archs can switch to it.

So testing and porting to further archs is certainly work in progress at this time,
but in this RFC stage, I could be wrong, but I considered the arch-patches in this series more
as an example to showcase the usefulness (or not) of the series related to the common code
changes: I did not extensively tested all archs to the their full extent, so more fixes
could come in V3 (if ever) together with more testing and archs.

> Given that you're an ARM Ltd employee, I'm sure you can find 32-bit
> systems to test - or have ARM Ltd got rid of everything that isn't
> 64-bit? ;)
> 

well...worst case there's always Amazon anyway ... :D

Cheers

Cristian

Re: [RFC PATCH v2 01/12] smp: add generic SMP-stop support to common code

[Cristian Marussi]

On 13/09/2019 19:19, Cristian Marussi wrote:
> There was a lot of code duplication across architectures regarding the
> SMP stop procedures' logic; moreover some of this duplicated code logic
> happened to be similarly faulty across a few architectures: while fixing
> such logic, move such generic logic as much as possible inside common
> code.
> 
> Collect all the common logic related to SMP stop operations into the
> common SMP code; any architecture willing to use such centralized logic
> can select CONFIG_ARCH_USE_COMMON_STOP=y and provide the related
> arch-specific helpers: in such a scenario, those architectures will
> transparently start using the common code provided by smp_send_stop()
> common function.
> 
> On the other side, Architectures not willing to use common code SMP stop
> logic will simply leave CONFIG_ARCH_USE_COMMON_STOP undefined and carry
> on executing their local arch-specific smp_send_stop() as before.
> 
> Suggested-by: Dave Martin <Dave.Martin@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v1 --> v2
> - moved related Kconfig to common code inside arch/Kconfig
> - introduced additional CONFIG_USE_COMMON_STOP selected by
>   CONFIG_ARCH_USE_COMMON_STOP
> - introduced helpers to let architectures optionally alter
>   the default common code behaviour while waiting for CPUs:
>   change timeout or wait for ever. (will be needed by x86)
> ---
>  arch/Kconfig        |  7 +++++
>  include/linux/smp.h | 55 +++++++++++++++++++++++++++++++++++++
>  kernel/smp.c        | 67 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index a7b57dd42c26..89766e6c0ac8 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -166,6 +166,13 @@ config ARCH_USE_BUILTIN_BSWAP
>  	 instructions should set this. And it shouldn't hurt to set it
>  	 on architectures that don't have such instructions.
>  
> +config ARCH_USE_COMMON_SMP_STOP
> +       def_bool n
> +
> +config USE_COMMON_SMP_STOP
> +       depends on SMP && ARCH_USE_COMMON_SMP_STOP
> +       def_bool y
> +
>  config KRETPROBES
>  	def_bool y
>  	depends on KPROBES && HAVE_KRETPROBES
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 6fc856c9eda5..381a14bfcd96 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -77,6 +77,61 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd);
>   */
>  extern void smp_send_stop(void);
>  
> +#ifdef CONFIG_USE_COMMON_SMP_STOP
> +static atomic_t wait_forever;
> +static atomic_t wait_timeout = ATOMIC_INIT(USEC_PER_SEC);
> +
> +/*
> + * An Architecture can optionally decide to use this helper to change the
> + * waiting behaviour of common STOP logic, forcing to wait forever for
> + * all CPUs to be stopped.
> + */
> +static inline void smp_stop_set_wait_forever(int wait)
> +{
> +	atomic_set(&wait_forever, wait);
> +	/* ensure wait atomic-op is visible */
> +	smp_mb__after_atomic();
> +}
> +

These new helpers I added in V2 to let x86 configure wait/timeout SMP common stop behavior
are in fact deadly broken as of now since based on underlying static header-globals.
I'll fix in V3.

Cheers

Cristian


> +/*
> + * An Architecture can optionally decide to use this helper to change the
> + * waiting timeout of common STOP logic. A ZERO timeout means no timeout
> + * at all as long as wait_forever was not previously set.
> + *
> + * Note that wait_forever and timeout must remain individually selectable:
> + * so you can temporarily request wait_forever while keeping the same timeout
> + * settings.
> + */
> +static inline void smp_stop_set_wait_timeout_us(unsigned long timeout)
> +{
> +	atomic_set(&wait_timeout, timeout);
> +	/* ensure timeout atomic-op is visible */
> +	smp_mb__after_atomic();
> +}
> +
> +/* Retrieve the current wait settings. */
> +static inline bool smp_stop_get_wait_timeout_us(unsigned long *timeout)
> +{
> +	if (timeout)
> +		*timeout = atomic_read(&wait_timeout);
> +	return atomic_read(&wait_forever);
> +}
> +
> +/*
> + * Any Architecture willing to use STOP common logic implementation
> + * MUST at least provide the arch_smp_stop_call() helper which is in
> + * charge of its own arch-specific CPU-stop mechanism.
> + */
> +extern void arch_smp_stop_call(cpumask_t *cpus);
> +
> +/*
> + * An Architecture CAN also provide the arch_smp_cpus_stop_complete()
> + * dedicated helper, to perform any final arch-specific operation on
> + * the local CPU once the other CPUs have been successfully stopped.
> + */
> +void arch_smp_cpus_stop_complete(void);
> +#endif
> +
>  /*
>   * sends a 'reschedule' event to another CPU:
>   */
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 7dbcb402c2fc..72f99bf13fd0 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -20,6 +20,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/idle.h>
>  #include <linux/hypervisor.h>
> +#include <linux/delay.h>
>  
>  #include "smpboot.h"
>  
> @@ -817,3 +818,69 @@ int smp_call_on_cpu(unsigned int cpu, int (*func)(void *), void *par, bool phys)
>  	return sscs.ret;
>  }
>  EXPORT_SYMBOL_GPL(smp_call_on_cpu);
> +
> +#ifdef CONFIG_USE_COMMON_SMP_STOP
> +void __weak arch_smp_cpus_stop_complete(void) { }
> +
> +static inline bool any_other_cpus_online(cpumask_t *mask,
> +					 unsigned int this_cpu_id)
> +{
> +	cpumask_copy(mask, cpu_online_mask);
> +	cpumask_clear_cpu(this_cpu_id, mask);
> +
> +	return !cpumask_empty(mask);
> +}
> +
> +/*
> + * This centralizes the common logic to:
> + *
> + *  - evaluate which CPUs are online and needs to be notified for stop,
> + *    while considering properly the status of the calling CPU
> + *
> + *  - call the arch-specific helpers to request the effective stop
> + *
> + *  - wait for the stop operation to be completed across all involved CPUs
> + *    monitoring the cpu_online_mask
> + */
> +void smp_send_stop(void)
> +{
> +	unsigned int this_cpu_id;
> +	cpumask_t mask;
> +
> +	this_cpu_id = smp_processor_id();
> +	if (any_other_cpus_online(&mask, this_cpu_id)) {
> +		bool wait;
> +		unsigned long timeout;
> +		unsigned int this_cpu_online = cpu_online(this_cpu_id);
> +
> +		if (system_state <= SYSTEM_RUNNING)
> +			pr_crit("stopping secondary CPUs\n");
> +		arch_smp_stop_call(&mask);
> +
> +		/*
> +		 * Defaults to wait up to one second for other CPUs to stop;
> +		 * architectures can modify the default timeout or request
> +		 * to wait forever.
> +		 *
> +		 * Here we rely simply on cpu_online_mask to sync with
> +		 * arch-specific stop code without bloating the code with an
> +		 * additional atomic_t ad-hoc counter.
> +		 *
> +		 * As a consequence we'll need proper explicit memory barriers
> +		 * in case the other CPUs running the arch-specific stop-code
> +		 * would need to commit to memory some data (like saved_regs).
> +		 */
> +		wait = smp_stop_get_wait_timeout_us(&timeout);
> +		while (num_online_cpus() > this_cpu_online &&
> +		       (wait || timeout--))
> +			udelay(1);
> +		/* ensure any stopping-CPUs memory access is made visible */
> +		smp_rmb();
> +		if (num_online_cpus() > this_cpu_online)
> +			pr_warn("failed to stop secondary CPUs %*pbl\n",
> +				cpumask_pr_args(cpu_online_mask));
> +	}
> +	/* Perform final (possibly arch-specific) work on this CPU */
> +	arch_smp_cpus_stop_complete();
> +}
> +#endif
>