Re: [PATCH v9 3/7] arm64: Add framework for a debug IPI

[Mark Rutland]

On Thu, Jun 01, 2023 at 02:31:47PM -0700, Douglas Anderson wrote:
> From: Sumit Garg <sumit.garg@xxxxxxxxxx>
> 
> Introduce a framework for an IPI that will be used for debug
> purposes. The primary use case of this IPI will be to generate stack
> crawls on other CPUs, but it will also be used to round up CPUs for
> kgdb.
> 
> When possible, we try to allocate this debug IPI as an NMI (or a
> pseudo NMI). If that fails (due to CONFIG, an incompatible interrupt
> controller, a quirk, missing the "irqchip.gicv3_pseudo_nmi=1" kernel
> parameter, etc) we fall back to a normal IPI.
> 
> NOTE: hooking this up for CPU backtrace / kgdb will happen in a future
> patch, this just adds the framework.
> 
> Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>

I think that we shouldn't add a framework in a separate file for this:

* This is very similar to our existing IPI management in smp.c, so it feels
  like duplication, or at least another thing we'd like to keep in-sync.

* We're going to want an NMI backtrace regardless of KGDB

* We're going to want the IPI_CPU_STOP and IPI_CRASH_CPU_STOP IPIs to be NMIs
  too.

I reckon it'd be better to extend the existing IPI logic in smp.c to allow IPIs
to be requested as NMIs, e.g.

----
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index edd63894d61e8..48e6aa62c473e 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -33,6 +33,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/kexec.h>
 #include <linux/kvm_host.h>
+#include <linux/nmi.h>
 
 #include <asm/alternative.h>
 #include <asm/atomic.h>
@@ -926,6 +927,21 @@ static void smp_cross_call(const struct cpumask *target, unsigned int ipinr)
 	__ipi_send_mask(ipi_desc[ipinr], target);
 }
 
+static bool ipi_should_be_nmi(enum ipi_msg_type ipi)
+{
+	if (!system_uses_irq_prio_masking())
+		return false;
+
+	switch (ipi) {
+	/*
+	 * TODO: select NMI IPIs here
+	 */
+		return true;
+	default:
+		return false;
+	}
+}
+
 static void ipi_setup(int cpu)
 {
 	int i;
@@ -933,8 +949,14 @@ static void ipi_setup(int cpu)
 	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
-	for (i = 0; i < nr_ipi; i++)
-		enable_percpu_irq(ipi_irq_base + i, 0);
+	for (i = 0; i < nr_ipi; i++) {
+		if (ipi_should_be_nmi(i)) {
+			prepare_percpu_nmi(ipi_irq_base + i);
+			enable_percpu_nmi(ipi_irq_base + i, 0);
+		} else {
+			enable_percpu_irq(ipi_irq_base + i, 0);
+		}
+	}
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -945,8 +967,14 @@ static void ipi_teardown(int cpu)
 	if (WARN_ON_ONCE(!ipi_irq_base))
 		return;
 
-	for (i = 0; i < nr_ipi; i++)
-		disable_percpu_irq(ipi_irq_base + i);
+	for (i = 0; i < nr_ipi; i++) {
+		if (ipi_should_be_nmi(i)) {
+			disable_percpu_nmi(ipi_irq_base + i);
+			teardown_percpu_nmi(ipi_irq_base + i);
+		} else {
+			disable_percpu_irq(ipi_irq_base + i);
+		}
+	}
 }
 #endif
 
@@ -958,11 +986,19 @@ void __init set_smp_ipi_range(int ipi_base, int n)
 	nr_ipi = min(n, NR_IPI);
 
 	for (i = 0; i < nr_ipi; i++) {
-		int err;
-
-		err = request_percpu_irq(ipi_base + i, ipi_handler,
-					 "IPI", &cpu_number);
-		WARN_ON(err);
+		int err = -EINVAL;
+
+		if (ipi_should_be_nmi(i)) {
+			err = request_percpu_nmi(ipi_base + i, ipi_handler,
+						 "IPI", &cpu_number);
+			WARN(err, "Could not request IPI %d as NMI, err=%d\n",
+			     i, err);
+		} else {
+			err = request_percpu_irq(ipi_base + i, ipi_handler,
+						 "IPI", &cpu_number);
+			WARN(err, "Could not request IPI %d as IRQ, err=%d\n",
+			     i, err);
+		}
 
 		ipi_desc[i] = irq_to_desc(ipi_base + i);
 		irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
----

... and then if we need an IPI for KGDB, we can add that to the existing list
of IPIs, and have it requested/enabled/disabled as usual.

Thanks,
Mark.

> ---
> I didn't get any feedback from v8 patch #10 [1], but I went ahead and
> folded it in here anyway since it really simplfies things. If people
> don't like the fallback to regular IPI, I can also undo it.
> 
> [1] https://lore.kernel.org/r/20230419155341.v8.10.Ic3659997d6243139d0522fc3afcdfd88d7a5f030@changeid/
> 
> Changes in v9:
> - Fold in v8 patch #10 ("Fallback to a regular IPI if NMI isn't enabled")
> - Moved header file out of "include" since it didn't need to be there.
> - Remove arm64_supports_nmi()
> - Renamed "NMI IPI" to "debug IPI" since it might not be backed by NMI.
> 
> Changes in v8:
> - debug_ipi_setup() and debug_ipi_teardown() no longer take cpu param
> 
>  arch/arm64/kernel/Makefile    |  2 +-
>  arch/arm64/kernel/ipi_debug.c | 76 +++++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/ipi_debug.h | 13 ++++++
>  3 files changed, 90 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kernel/ipi_debug.c
>  create mode 100644 arch/arm64/kernel/ipi_debug.h
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index cc22011ab66a..737838f803b7 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -34,7 +34,7 @@ obj-y			:= debug-monitors.o entry.o irq.o fpsimd.o		\
>  			   cpufeature.o alternative.o cacheinfo.o		\
>  			   smp.o smp_spin_table.o topology.o smccc-call.o	\
>  			   syscall.o proton-pack.o idreg-override.o idle.o	\
> -			   patching.o
> +			   patching.o ipi_debug.o
>  
>  obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
>  					   sys_compat.o
> diff --git a/arch/arm64/kernel/ipi_debug.c b/arch/arm64/kernel/ipi_debug.c
> new file mode 100644
> index 000000000000..b57833e31eaf
> --- /dev/null
> +++ b/arch/arm64/kernel/ipi_debug.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Debug IPI support
> + *
> + * Copyright (C) 2020 Linaro Limited
> + * Author: Sumit Garg <sumit.garg@xxxxxxxxxx>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/smp.h>
> +
> +#include "ipi_debug.h"
> +
> +static struct irq_desc *ipi_debug_desc __read_mostly;
> +static int ipi_debug_id __read_mostly;
> +static bool is_nmi;
> +
> +void arm64_debug_ipi(cpumask_t *mask)
> +{
> +	if (WARN_ON_ONCE(!ipi_debug_desc))
> +		return;
> +
> +	__ipi_send_mask(ipi_debug_desc, mask);
> +}
> +
> +static irqreturn_t ipi_debug_handler(int irq, void *data)
> +{
> +	/* nop, NMI handlers for special features can be added here. */
> +
> +	return IRQ_NONE;
> +}
> +
> +void debug_ipi_setup(void)
> +{
> +	if (!ipi_debug_desc)
> +		return;
> +
> +	if (is_nmi) {
> +		if (!prepare_percpu_nmi(ipi_debug_id))
> +			enable_percpu_nmi(ipi_debug_id, IRQ_TYPE_NONE);
> +	} else {
> +		enable_percpu_irq(ipi_debug_id, IRQ_TYPE_NONE);
> +	}
> +}
> +
> +void debug_ipi_teardown(void)
> +{
> +	if (!ipi_debug_desc)
> +		return;
> +
> +	if (is_nmi) {
> +		disable_percpu_nmi(ipi_debug_id);
> +		teardown_percpu_nmi(ipi_debug_id);
> +	} else {
> +		disable_percpu_irq(ipi_debug_id);
> +	}
> +}
> +
> +void __init set_smp_debug_ipi(int ipi)
> +{
> +	int err;
> +
> +	if (!request_percpu_nmi(ipi, ipi_debug_handler, "IPI", &cpu_number)) {
> +		is_nmi = true;
> +	} else {
> +		err = request_percpu_irq(ipi, ipi_debug_handler, "IPI", &cpu_number);
> +		if (WARN_ON(err))
> +			return;
> +
> +		irq_set_status_flags(ipi, IRQ_HIDDEN);
> +	}
> +
> +	ipi_debug_desc = irq_to_desc(ipi);
> +	ipi_debug_id = ipi;
> +}
> diff --git a/arch/arm64/kernel/ipi_debug.h b/arch/arm64/kernel/ipi_debug.h
> new file mode 100644
> index 000000000000..f6011a09282f
> --- /dev/null
> +++ b/arch/arm64/kernel/ipi_debug.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_NMI_H
> +#define __ASM_NMI_H
> +
> +#include <linux/cpumask.h>
> +
> +void arm64_debug_ipi(cpumask_t *mask);
> +
> +void set_smp_debug_ipi(int ipi);
> +void debug_ipi_setup(void);
> +void debug_ipi_teardown(void);
> +
> +#endif
> -- 
> 2.41.0.rc2.161.g9c6817b8e7-goog
>