With pseudo NMIs support available its possible to configure SGIs to be
triggered as pseudo NMIs running in NMI context. And kernel features
such as:
- NMI backtrace can leverage IPI turned as NMI to get a backtrace of CPU
stuck in hard lockup using magic SYSRQ.
- kgdb relies on NMI support to round up CPUs which are stuck in hard
lockup state with interrupts disabled.
This patch-set adds framework to turn an IPI as NMI which can be triggered
as a pseudo NMI which in turn invokes registered NMI handlers.
After this patch-set we should be able to get a backtrace for a CPU
stuck in HARDLOCKUP. Have a look at an examples below from a hard lockup
testcase run on Developerbox:
$ echo HARDLOCKUP > /sys/kernel/debug/provoke-crash/DIRECT
NMI backtrace:
==============
# Issue Magic SysRq to dump backtrace
[ 376.894502] NMI backtrace for cpu 8
[ 376.894506] CPU: 8 PID: 555 Comm: bash Not tainted 5.9.0-rc3-00740-g06ff047-dirty #242
[ 376.894510] Hardware name: Socionext SynQuacer E-series DeveloperBox, BIOS build #73 Apr 6 2020
[ 376.894514] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--)
[ 376.894517] pc : lkdtm_HARDLOCKUP+0x8/0x18
[ 376.894520] lr : lkdtm_do_action+0x24/0x30
[ 376.894524] sp : ffff800012cebd20
[ 376.894527] pmr_save: 00000060
[ 376.894530] x29: ffff800012cebd20 x28: ffff000875ae8000
[ 376.894540] x27: 0000000000000000 x26: 0000000000000000
[ 376.894550] x25: 000000000000001a x24: ffff800012cebe40
[ 376.894560] x23: 000000000000000b x22: ffff800010fc5040
[ 376.894569] x21: ffff000878b61000 x20: ffff8000113b2870
[ 376.894579] x19: 000000000000001b x18: 0000000000000010
[ 376.894588] x17: 0000000000000000 x16: 0000000000000000
[ 376.894598] x15: ffff000875ae8470 x14: 00000000000002ad
[ 376.894613] x13: 0000000000000000 x12: 0000000000000000
[ 376.894622] x11: 0000000000000007 x10: 00000000000009c0
[ 376.894631] x9 : ffff800012ceba80 x8 : ffff000875ae8a20
[ 376.894641] x7 : ffff00087f6b3280 x6 : ffff00087f6b3200
[ 376.894651] x5 : 0000000000000000 x4 : ffff00087f6a91f8
[ 376.894660] x3 : ffff00087f6b0120 x2 : 1aa310cec69eb500
[ 376.894670] x1 : 0000000000000000 x0 : 0000000000000060
[ 376.894679] Call trace:
[ 376.894683] lkdtm_HARDLOCKUP+0x8/0x18
[ 376.894686] direct_entry+0x124/0x1c0
[ 376.894689] full_proxy_write+0x60/0xb0
[ 376.894693] vfs_write+0xf0/0x230
[ 376.894696] ksys_write+0x6c/0xf8
[ 376.894699] __arm64_sys_write+0x1c/0x28
[ 376.894703] el0_svc_common.constprop.0+0x74/0x1f0
[ 376.894707] do_el0_svc+0x24/0x90
[ 376.894710] el0_sync_handler+0x180/0x2f8
[ 376.894713] el0_sync+0x158/0x180
KGDB:
=====
# Enter kdb via Magic SysRq
[6]kdb> btc
btc: cpu status: Currently on cpu 6
Available cpus: 0-5(I), 6, 7(I), 8, 9-23(I)
<snip>
Stack traceback for pid 555
0xffff000875ae8000 555 554 1 8 R 0xffff000875ae89c0 bash
CPU: 8 PID: 555 Comm: bash Not tainted 5.9.0-rc3-00740-g06ff047-dirty #242
Hardware name: Socionext SynQuacer E-series DeveloperBox, BIOS build #73 Apr 6 2020
Call trace:
dump_backtrace+0x0/0x1a0
show_stack+0x18/0x28
dump_stack+0xc0/0x11c
kgdb_cpu_enter+0x648/0x660
kgdb_nmicallback+0xa0/0xa8
ipi_kgdb_nmicallback+0x24/0x30
ipi_nmi_handler+0x48/0x60
handle_percpu_devid_fasteoi_ipi+0x74/0x88
generic_handle_irq+0x30/0x48
handle_domain_nmi+0x48/0x80
gic_handle_irq+0x18c/0x34c
el1_irq+0xcc/0x180
lkdtm_HARDLOCKUP+0x8/0x18
direct_entry+0x124/0x1c0
full_proxy_write+0x60/0xb0
vfs_write+0xf0/0x230
ksys_write+0x6c/0xf8
__arm64_sys_write+0x1c/0x28
el0_svc_common.constprop.0+0x74/0x1f0
do_el0_svc+0x24/0x90
el0_sync_handler+0x180/0x2f8
el0_sync+0x158/0x180
<snip>
Changes in v5:
- Rebased to head of upstream master.
- Remove redundant invocation of ipi_nmi_setup().
- Addressed misc. comments.
Changes in v4:
- Move IPI NMI framework to a separate file.
- Get rid of hard-coded IPI_CALL_NMI_FUNC allocation.
- Add NMI backtrace support leveraged via magic SYSRQ.
Changes in v3:
- Rebased to Marc's latest IPIs patch-set [1].
[1] https://lkml.org/lkml/2020/9/1/603
Changes since RFC version [1]:
- Switch to use generic interrupt framework to turn an IPI as NMI.
- Dependent on Marc's patch-set [2] which turns IPIs into normal
interrupts.
- Addressed misc. comments from Doug on patch #4.
- Posted kgdb NMI printk() fixup separately which has evolved since
to be solved using different approach via changing kgdb interception
of printk() in common printk() code (see patch [3]).
[1] https://lkml.org/lkml/2020/4/24/328
[2] https://lkml.org/lkml/2020/5/19/710
[3] https://lkml.org/lkml/2020/5/20/418
Sumit Garg (5):
arm64: Add framework to turn IPI as NMI
irqchip/gic-v3: Enable support for SGIs to act as NMIs
arm64: smp: Allocate and setup IPI as NMI
arm64: kgdb: Round up cpus using IPI as NMI
arm64: ipi_nmi: Add support for NMI backtrace
arch/arm64/include/asm/irq.h | 6 +++
arch/arm64/include/asm/kgdb.h | 8 ++++
arch/arm64/include/asm/nmi.h | 16 ++++++++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/ipi_nmi.c | 90 +++++++++++++++++++++++++++++++++++++++++++
arch/arm64/kernel/kgdb.c | 21 ++++++++++
arch/arm64/kernel/smp.c | 8 ++++
drivers/irqchip/irq-gic-v3.c | 13 ++++++-
8 files changed, 161 insertions(+), 3 deletions(-)
create mode 100644 arch/arm64/include/asm/nmi.h
create mode 100644 arch/arm64/kernel/ipi_nmi.c
--
2.7.4
Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
particular platform doesn't support pseudo NMIs, then request IPI as a
regular IRQ.
The main motivation for this feature is to have an IPI that can be
leveraged to invoke NMI functions on other CPUs. And current prospective
users are NMI backtrace and KGDB CPUs round-up whose support is added
via future patches.
Signed-off-by: Sumit Garg <[email protected]>
---
arch/arm64/include/asm/nmi.h | 16 +++++++++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/ipi_nmi.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/include/asm/nmi.h
create mode 100644 arch/arm64/kernel/ipi_nmi.c
diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h
new file mode 100644
index 0000000..3433c55
--- /dev/null
+++ b/arch/arm64/include/asm/nmi.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_NMI_H
+#define __ASM_NMI_H
+
+#ifndef __ASSEMBLER__
+
+#include <linux/cpumask.h>
+
+extern void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask);
+
+void set_smp_ipi_nmi(int ipi);
+void ipi_nmi_setup(int cpu);
+void ipi_nmi_teardown(int cpu);
+
+#endif /* !__ASSEMBLER__ */
+#endif
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index bbaf0bc..525a1e0 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -17,7 +17,7 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
return_address.o cpuinfo.o cpu_errata.o \
cpufeature.o alternative.o cacheinfo.o \
smp.o smp_spin_table.o topology.o smccc-call.o \
- syscall.o proton-pack.o
+ syscall.o proton-pack.o ipi_nmi.o
targets += efi-entry.o
diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
new file mode 100644
index 0000000..a959256
--- /dev/null
+++ b/arch/arm64/kernel/ipi_nmi.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * NMI support for IPIs
+ *
+ * Copyright (C) 2020 Linaro Limited
+ * Author: Sumit Garg <[email protected]>
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/smp.h>
+
+#include <asm/nmi.h>
+
+static struct irq_desc *ipi_desc __read_mostly;
+static int ipi_id __read_mostly;
+static bool is_nmi __read_mostly;
+
+void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
+{
+ if (WARN_ON_ONCE(!ipi_desc))
+ return;
+
+ __ipi_send_mask(ipi_desc, mask);
+}
+
+static irqreturn_t ipi_nmi_handler(int irq, void *data)
+{
+ /* nop, NMI handlers for special features can be added here. */
+
+ return IRQ_HANDLED;
+}
+
+void ipi_nmi_setup(int cpu)
+{
+ if (!ipi_desc)
+ return;
+
+ if (is_nmi) {
+ if (!prepare_percpu_nmi(ipi_id))
+ enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
+ } else {
+ enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
+ }
+}
+
+void ipi_nmi_teardown(int cpu)
+{
+ if (!ipi_desc)
+ return;
+
+ if (is_nmi) {
+ disable_percpu_nmi(ipi_id);
+ teardown_percpu_nmi(ipi_id);
+ } else {
+ disable_percpu_irq(ipi_id);
+ }
+}
+
+void __init set_smp_ipi_nmi(int ipi)
+{
+ int err;
+
+ err = request_percpu_nmi(ipi, ipi_nmi_handler, "IPI", &cpu_number);
+ if (err) {
+ err = request_percpu_irq(ipi, ipi_nmi_handler, "IPI",
+ &cpu_number);
+ WARN_ON(err);
+ is_nmi = false;
+ } else {
+ is_nmi = true;
+ }
+
+ ipi_desc = irq_to_desc(ipi);
+ irq_set_status_flags(ipi, IRQ_HIDDEN);
+ ipi_id = ipi;
+}
--
2.7.4
Allocate an unused IPI that can be turned as NMI using ipi_nmi framework.
Also, invoke corresponding NMI setup/teardown APIs.
Signed-off-by: Sumit Garg <[email protected]>
---
arch/arm64/kernel/smp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 82e75fc..129ebfb 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -43,6 +43,7 @@
#include <asm/daifflags.h>
#include <asm/kvm_mmu.h>
#include <asm/mmu_context.h>
+#include <asm/nmi.h>
#include <asm/numa.h>
#include <asm/processor.h>
#include <asm/smp_plat.h>
@@ -962,6 +963,8 @@ static void ipi_setup(int cpu)
for (i = 0; i < nr_ipi; i++)
enable_percpu_irq(ipi_irq_base + i, 0);
+
+ ipi_nmi_setup(cpu);
}
#ifdef CONFIG_HOTPLUG_CPU
@@ -974,6 +977,8 @@ static void ipi_teardown(int cpu)
for (i = 0; i < nr_ipi; i++)
disable_percpu_irq(ipi_irq_base + i);
+
+ ipi_nmi_teardown(cpu);
}
#endif
@@ -995,6 +1000,9 @@ void __init set_smp_ipi_range(int ipi_base, int n)
irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
}
+ if (n > nr_ipi)
+ set_smp_ipi_nmi(ipi_base + nr_ipi);
+
ipi_irq_base = ipi_base;
/* Setup the boot CPU immediately */
--
2.7.4
arm64 platforms with GICv3 or later supports pseudo NMIs which can be
leveraged to round up CPUs which are stuck in hard lockup state with
interrupts disabled that wouldn't be possible with a normal IPI.
So instead switch to round up CPUs using IPI turned as NMI. And in
case a particular arm64 platform doesn't supports pseudo NMIs,
this IPI will act as a normal IPI which maintains existing kgdb
functionality.
Signed-off-by: Sumit Garg <[email protected]>
---
arch/arm64/include/asm/kgdb.h | 8 ++++++++
arch/arm64/kernel/ipi_nmi.c | 5 ++++-
arch/arm64/kernel/kgdb.c | 21 +++++++++++++++++++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/kgdb.h b/arch/arm64/include/asm/kgdb.h
index 21fc85e..6f3d3af 100644
--- a/arch/arm64/include/asm/kgdb.h
+++ b/arch/arm64/include/asm/kgdb.h
@@ -24,6 +24,14 @@ static inline void arch_kgdb_breakpoint(void)
extern void kgdb_handle_bus_error(void);
extern int kgdb_fault_expected;
+#ifdef CONFIG_KGDB
+extern void ipi_kgdb_nmicallback(int cpu, void *regs);
+#else
+static inline void ipi_kgdb_nmicallback(int cpu, void *regs)
+{
+}
+#endif
+
#endif /* !__ASSEMBLY__ */
/*
diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
index a959256..e0a9e03 100644
--- a/arch/arm64/kernel/ipi_nmi.c
+++ b/arch/arm64/kernel/ipi_nmi.c
@@ -8,6 +8,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
+#include <linux/kgdb.h>
#include <linux/smp.h>
#include <asm/nmi.h>
@@ -26,7 +27,9 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
static irqreturn_t ipi_nmi_handler(int irq, void *data)
{
- /* nop, NMI handlers for special features can be added here. */
+ unsigned int cpu = smp_processor_id();
+
+ ipi_kgdb_nmicallback(cpu, get_irq_regs());
return IRQ_HANDLED;
}
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 1a157ca3..0991275 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -17,6 +17,7 @@
#include <asm/debug-monitors.h>
#include <asm/insn.h>
+#include <asm/nmi.h>
#include <asm/traps.h>
struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
@@ -353,3 +354,23 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
return aarch64_insn_write((void *)bpt->bpt_addr,
*(u32 *)bpt->saved_instr);
}
+
+void ipi_kgdb_nmicallback(int cpu, void *regs)
+{
+ if (atomic_read(&kgdb_active) != -1)
+ kgdb_nmicallback(cpu, regs);
+}
+
+#ifdef CONFIG_SMP
+void kgdb_roundup_cpus(void)
+{
+ struct cpumask mask;
+
+ cpumask_copy(&mask, cpu_online_mask);
+ cpumask_clear_cpu(raw_smp_processor_id(), &mask);
+ if (cpumask_empty(&mask))
+ return;
+
+ arch_send_call_nmi_func_ipi_mask(&mask);
+}
+#endif
--
2.7.4
Enable NMI backtrace support on arm64 using IPI turned as an NMI
leveraging pseudo NMIs support. It is now possible for users to get a
backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
Signed-off-by: Sumit Garg <[email protected]>
---
arch/arm64/include/asm/irq.h | 6 ++++++
arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index b2b0c64..e840bf1 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -6,6 +6,12 @@
#include <asm-generic/irq.h>
+#ifdef CONFIG_SMP
+extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
+ bool exclude_self);
+#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
+#endif
+
struct pt_regs;
static inline int nr_legacy_irqs(void)
diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
index e0a9e03..e1dc19d 100644
--- a/arch/arm64/kernel/ipi_nmi.c
+++ b/arch/arm64/kernel/ipi_nmi.c
@@ -9,6 +9,7 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/kgdb.h>
+#include <linux/nmi.h>
#include <linux/smp.h>
#include <asm/nmi.h>
@@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
__ipi_send_mask(ipi_desc, mask);
}
+void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
+{
+ nmi_trigger_cpumask_backtrace(mask, exclude_self,
+ arch_send_call_nmi_func_ipi_mask);
+}
+
static irqreturn_t ipi_nmi_handler(int irq, void *data)
{
unsigned int cpu = smp_processor_id();
- ipi_kgdb_nmicallback(cpu, get_irq_regs());
+ if (nmi_cpu_backtrace(get_irq_regs()))
+ goto out;
+ ipi_kgdb_nmicallback(cpu, get_irq_regs());
+out:
return IRQ_HANDLED;
}
--
2.7.4
On Wed, Oct 14, 2020 at 04:42:09PM +0530, Sumit Garg wrote:
> Allocate an unused IPI that can be turned as NMI using ipi_nmi framework.
> Also, invoke corresponding NMI setup/teardown APIs.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 82e75fc..129ebfb 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -43,6 +43,7 @@
> #include <asm/daifflags.h>
> #include <asm/kvm_mmu.h>
> #include <asm/mmu_context.h>
> +#include <asm/nmi.h>
> #include <asm/numa.h>
> #include <asm/processor.h>
> #include <asm/smp_plat.h>
> @@ -962,6 +963,8 @@ static void ipi_setup(int cpu)
>
> for (i = 0; i < nr_ipi; i++)
> enable_percpu_irq(ipi_irq_base + i, 0);
> +
> + ipi_nmi_setup(cpu);
> }
>
> #ifdef CONFIG_HOTPLUG_CPU
> @@ -974,6 +977,8 @@ static void ipi_teardown(int cpu)
>
> for (i = 0; i < nr_ipi; i++)
> disable_percpu_irq(ipi_irq_base + i);
> +
> + ipi_nmi_teardown(cpu);
> }
> #endif
>
> @@ -995,6 +1000,9 @@ void __init set_smp_ipi_range(int ipi_base, int n)
> irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
> }
>
> + if (n > nr_ipi)
> + set_smp_ipi_nmi(ipi_base + nr_ipi);
> +
> ipi_irq_base = ipi_base;
>
> /* Setup the boot CPU immediately */
> --
Looks good to me. Please feel free to add:
Reviewed-by: Masayoshi Mizuma <[email protected]>
Thanks!
Masa
On Wed, Oct 14, 2020 at 04:42:11PM +0530, Sumit Garg wrote:
> Enable NMI backtrace support on arm64 using IPI turned as an NMI
> leveraging pseudo NMIs support. It is now possible for users to get a
> backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/irq.h | 6 ++++++
> arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index b2b0c64..e840bf1 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -6,6 +6,12 @@
>
> #include <asm-generic/irq.h>
>
> +#ifdef CONFIG_SMP
> +extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
> + bool exclude_self);
> +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> +#endif
> +
> struct pt_regs;
>
> static inline int nr_legacy_irqs(void)
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> index e0a9e03..e1dc19d 100644
> --- a/arch/arm64/kernel/ipi_nmi.c
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -9,6 +9,7 @@
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/kgdb.h>
> +#include <linux/nmi.h>
> #include <linux/smp.h>
>
> #include <asm/nmi.h>
> @@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
> __ipi_send_mask(ipi_desc, mask);
> }
>
> +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool exclude_self)
> +{
> + nmi_trigger_cpumask_backtrace(mask, exclude_self,
> + arch_send_call_nmi_func_ipi_mask);
> +}
> +
> static irqreturn_t ipi_nmi_handler(int irq, void *data)
> {
> unsigned int cpu = smp_processor_id();
>
> - ipi_kgdb_nmicallback(cpu, get_irq_regs());
> + if (nmi_cpu_backtrace(get_irq_regs()))
> + goto out;
>
> + ipi_kgdb_nmicallback(cpu, get_irq_regs());
> +out:
> return IRQ_HANDLED;
> }
>
> --
It works well. Please feel free to add:
Tested-by: Masayoshi Mizuma <[email protected]>
Thanks!
Masa
On Wed, Oct 14, 2020 at 04:42:07PM +0530, Sumit Garg wrote:
> Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
> particular platform doesn't support pseudo NMIs, then request IPI as a
> regular IRQ.
>
> The main motivation for this feature is to have an IPI that can be
> leveraged to invoke NMI functions on other CPUs. And current prospective
> users are NMI backtrace and KGDB CPUs round-up whose support is added
> via future patches.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/nmi.h | 16 +++++++++
> arch/arm64/kernel/Makefile | 2 +-
> arch/arm64/kernel/ipi_nmi.c | 77 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 94 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/nmi.h
> create mode 100644 arch/arm64/kernel/ipi_nmi.c
>
> diff --git a/arch/arm64/include/asm/nmi.h b/arch/arm64/include/asm/nmi.h
> new file mode 100644
> index 0000000..3433c55
> --- /dev/null
> +++ b/arch/arm64/include/asm/nmi.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_NMI_H
> +#define __ASM_NMI_H
> +
> +#ifndef __ASSEMBLER__
> +
> +#include <linux/cpumask.h>
> +
> +extern void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask);
> +
> +void set_smp_ipi_nmi(int ipi);
> +void ipi_nmi_setup(int cpu);
> +void ipi_nmi_teardown(int cpu);
> +
> +#endif /* !__ASSEMBLER__ */
> +#endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index bbaf0bc..525a1e0 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -17,7 +17,7 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
> return_address.o cpuinfo.o cpu_errata.o \
> cpufeature.o alternative.o cacheinfo.o \
> smp.o smp_spin_table.o topology.o smccc-call.o \
> - syscall.o proton-pack.o
> + syscall.o proton-pack.o ipi_nmi.o
>
> targets += efi-entry.o
>
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> new file mode 100644
> index 0000000..a959256
> --- /dev/null
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NMI support for IPIs
> + *
> + * Copyright (C) 2020 Linaro Limited
> + * Author: Sumit Garg <[email protected]>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/smp.h>
> +
> +#include <asm/nmi.h>
> +
> +static struct irq_desc *ipi_desc __read_mostly;
> +static int ipi_id __read_mostly;
> +static bool is_nmi __read_mostly;
> +
> +void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
> +{
> + if (WARN_ON_ONCE(!ipi_desc))
> + return;
> +
> + __ipi_send_mask(ipi_desc, mask);
> +}
> +
> +static irqreturn_t ipi_nmi_handler(int irq, void *data)
> +{
> + /* nop, NMI handlers for special features can be added here. */
> +
> + return IRQ_HANDLED;
> +}
> +
> +void ipi_nmi_setup(int cpu)
> +{
> + if (!ipi_desc)
> + return;
> +
> + if (is_nmi) {
> + if (!prepare_percpu_nmi(ipi_id))
> + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> + } else {
> + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
> + }
> +}
> +
> +void ipi_nmi_teardown(int cpu)
> +{
> + if (!ipi_desc)
> + return;
> +
> + if (is_nmi) {
> + disable_percpu_nmi(ipi_id);
> + teardown_percpu_nmi(ipi_id);
> + } else {
> + disable_percpu_irq(ipi_id);
> + }
> +}
> +
> +void __init set_smp_ipi_nmi(int ipi)
> +{
> + int err;
> +
> + err = request_percpu_nmi(ipi, ipi_nmi_handler, "IPI", &cpu_number);
> + if (err) {
> + err = request_percpu_irq(ipi, ipi_nmi_handler, "IPI",
> + &cpu_number);
> + WARN_ON(err);
> + is_nmi = false;
> + } else {
> + is_nmi = true;
> + }
> +
> + ipi_desc = irq_to_desc(ipi);
> + irq_set_status_flags(ipi, IRQ_HIDDEN);
> + ipi_id = ipi;
> +}
> --
Looks good to me. Please feel free to add:
Reviewed-by: Masayoshi Mizuma <[email protected]>
Thanks!
Masa
On 2020-10-14 12:12, Sumit Garg wrote:
> Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
> particular platform doesn't support pseudo NMIs, then request IPI as a
> regular IRQ.
>
> The main motivation for this feature is to have an IPI that can be
> leveraged to invoke NMI functions on other CPUs. And current
> prospective
> users are NMI backtrace and KGDB CPUs round-up whose support is added
> via future patches.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/nmi.h | 16 +++++++++
> arch/arm64/kernel/Makefile | 2 +-
> arch/arm64/kernel/ipi_nmi.c | 77
> ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 94 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/nmi.h
> create mode 100644 arch/arm64/kernel/ipi_nmi.c
>
> diff --git a/arch/arm64/include/asm/nmi.h
> b/arch/arm64/include/asm/nmi.h
> new file mode 100644
> index 0000000..3433c55
> --- /dev/null
> +++ b/arch/arm64/include/asm/nmi.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __ASM_NMI_H
> +#define __ASM_NMI_H
> +
> +#ifndef __ASSEMBLER__
> +
> +#include <linux/cpumask.h>
> +
> +extern void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask);
> +
> +void set_smp_ipi_nmi(int ipi);
> +void ipi_nmi_setup(int cpu);
> +void ipi_nmi_teardown(int cpu);
> +
> +#endif /* !__ASSEMBLER__ */
> +#endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index bbaf0bc..525a1e0 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -17,7 +17,7 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
> return_address.o cpuinfo.o cpu_errata.o \
> cpufeature.o alternative.o cacheinfo.o \
> smp.o smp_spin_table.o topology.o smccc-call.o \
> - syscall.o proton-pack.o
> + syscall.o proton-pack.o ipi_nmi.o
>
> targets += efi-entry.o
>
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> new file mode 100644
> index 0000000..a959256
> --- /dev/null
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * NMI support for IPIs
> + *
> + * Copyright (C) 2020 Linaro Limited
> + * Author: Sumit Garg <[email protected]>
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/smp.h>
> +
> +#include <asm/nmi.h>
> +
> +static struct irq_desc *ipi_desc __read_mostly;
> +static int ipi_id __read_mostly;
> +static bool is_nmi __read_mostly;
> +
> +void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
> +{
> + if (WARN_ON_ONCE(!ipi_desc))
> + return;
> +
> + __ipi_send_mask(ipi_desc, mask);
> +}
> +
> +static irqreturn_t ipi_nmi_handler(int irq, void *data)
> +{
> + /* nop, NMI handlers for special features can be added here. */
> +
> + return IRQ_HANDLED;
This definitely is the *wrong* default. If nothing is explicitly
handling the interrupt, it should be reported as such to the core
code to be disabled if this happens too often.
> +}
> +
> +void ipi_nmi_setup(int cpu)
The naming is awful. "ipi" is nowhere specific enough (we already have
another 7 of them), and this doesn't necessarily uses pseudo-NMIs, since
you are requesting IRQs.
> +{
> + if (!ipi_desc)
> + return;
> +
> + if (is_nmi) {
> + if (!prepare_percpu_nmi(ipi_id))
> + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> + } else {
> + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
I'm not keen on this. Normal IRQs can't reliably work, so why do you
even bother with this?
> + }
> +}
> +
> +void ipi_nmi_teardown(int cpu)
> +{
> + if (!ipi_desc)
> + return;
> +
> + if (is_nmi) {
> + disable_percpu_nmi(ipi_id);
> + teardown_percpu_nmi(ipi_id);
> + } else {
> + disable_percpu_irq(ipi_id);
> + }
> +}
> +
> +void __init set_smp_ipi_nmi(int ipi)
> +{
> + int err;
> +
> + err = request_percpu_nmi(ipi, ipi_nmi_handler, "IPI", &cpu_number);
> + if (err) {
> + err = request_percpu_irq(ipi, ipi_nmi_handler, "IPI",
> + &cpu_number);
> + WARN_ON(err);
> + is_nmi = false;
> + } else {
> + is_nmi = true;
> + }
> +
> + ipi_desc = irq_to_desc(ipi);
> + irq_set_status_flags(ipi, IRQ_HIDDEN);
> + ipi_id = ipi;
Updating global state without checking for errors doesn't seem
like a great move.
M.
--
Jazz is not dead. It just smells funny...
On 2020-10-14 12:12, Sumit Garg wrote:
> Allocate an unused IPI that can be turned as NMI using ipi_nmi
> framework.
This doesn't do any allocation, as far as I can see. It relies on
the initial grant from the interrupt controller to be larger than
what the kernel currently uses.
> Also, invoke corresponding NMI setup/teardown APIs.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/kernel/smp.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 82e75fc..129ebfb 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -43,6 +43,7 @@
> #include <asm/daifflags.h>
> #include <asm/kvm_mmu.h>
> #include <asm/mmu_context.h>
> +#include <asm/nmi.h>
> #include <asm/numa.h>
> #include <asm/processor.h>
> #include <asm/smp_plat.h>
> @@ -962,6 +963,8 @@ static void ipi_setup(int cpu)
>
> for (i = 0; i < nr_ipi; i++)
> enable_percpu_irq(ipi_irq_base + i, 0);
> +
> + ipi_nmi_setup(cpu);
> }
>
> #ifdef CONFIG_HOTPLUG_CPU
> @@ -974,6 +977,8 @@ static void ipi_teardown(int cpu)
>
> for (i = 0; i < nr_ipi; i++)
> disable_percpu_irq(ipi_irq_base + i);
> +
> + ipi_nmi_teardown(cpu);
> }
> #endif
>
> @@ -995,6 +1000,9 @@ void __init set_smp_ipi_range(int ipi_base, int n)
> irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
> }
>
> + if (n > nr_ipi)
> + set_smp_ipi_nmi(ipi_base + nr_ipi);
> +
> ipi_irq_base = ipi_base;
>
> /* Setup the boot CPU immediately */
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020-10-14 12:12, Sumit Garg wrote:
> arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> leveraged to round up CPUs which are stuck in hard lockup state with
> interrupts disabled that wouldn't be possible with a normal IPI.
>
> So instead switch to round up CPUs using IPI turned as NMI. And in
> case a particular arm64 platform doesn't supports pseudo NMIs,
> this IPI will act as a normal IPI which maintains existing kgdb
> functionality.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/kgdb.h | 8 ++++++++
> arch/arm64/kernel/ipi_nmi.c | 5 ++++-
> arch/arm64/kernel/kgdb.c | 21 +++++++++++++++++++++
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kgdb.h
> b/arch/arm64/include/asm/kgdb.h
> index 21fc85e..6f3d3af 100644
> --- a/arch/arm64/include/asm/kgdb.h
> +++ b/arch/arm64/include/asm/kgdb.h
> @@ -24,6 +24,14 @@ static inline void arch_kgdb_breakpoint(void)
> extern void kgdb_handle_bus_error(void);
> extern int kgdb_fault_expected;
>
> +#ifdef CONFIG_KGDB
> +extern void ipi_kgdb_nmicallback(int cpu, void *regs);
> +#else
> +static inline void ipi_kgdb_nmicallback(int cpu, void *regs)
> +{
> +}
> +#endif
> +
> #endif /* !__ASSEMBLY__ */
>
> /*
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> index a959256..e0a9e03 100644
> --- a/arch/arm64/kernel/ipi_nmi.c
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -8,6 +8,7 @@
>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> +#include <linux/kgdb.h>
> #include <linux/smp.h>
>
> #include <asm/nmi.h>
> @@ -26,7 +27,9 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> *mask)
>
> static irqreturn_t ipi_nmi_handler(int irq, void *data)
> {
> - /* nop, NMI handlers for special features can be added here. */
> + unsigned int cpu = smp_processor_id();
> +
> + ipi_kgdb_nmicallback(cpu, get_irq_regs());
Please add a return value to ipi_kgdb_nmicallback(), and check it
before returning IRQ_HANDLED.
Thinking a bit more about the whole thing, you should have a way to
avoid requesting the NMI if there is no user for it (there is nothing
worse than an enabled interrupt without handlers...).
>
> return IRQ_HANDLED;
> }
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 1a157ca3..0991275 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -17,6 +17,7 @@
>
> #include <asm/debug-monitors.h>
> #include <asm/insn.h>
> +#include <asm/nmi.h>
> #include <asm/traps.h>
>
> struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
> @@ -353,3 +354,23 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt
> *bpt)
> return aarch64_insn_write((void *)bpt->bpt_addr,
> *(u32 *)bpt->saved_instr);
> }
> +
> +void ipi_kgdb_nmicallback(int cpu, void *regs)
> +{
> + if (atomic_read(&kgdb_active) != -1)
> + kgdb_nmicallback(cpu, regs);
> +}
> +
> +#ifdef CONFIG_SMP
There is no such thing as an arm64 UP kernel.
> +void kgdb_roundup_cpus(void)
> +{
> + struct cpumask mask;
> +
> + cpumask_copy(&mask, cpu_online_mask);
> + cpumask_clear_cpu(raw_smp_processor_id(), &mask);
> + if (cpumask_empty(&mask))
> + return;
> +
> + arch_send_call_nmi_func_ipi_mask(&mask);
Surely you can come up with a less convoluted name for this function.
arm64_send_nmi() would be plenty in my opinion.
> +}
> +#endif
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2020-10-14 12:12, Sumit Garg wrote:
> Enable NMI backtrace support on arm64 using IPI turned as an NMI
> leveraging pseudo NMIs support. It is now possible for users to get a
> backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/irq.h | 6 ++++++
> arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
> 2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/irq.h
> b/arch/arm64/include/asm/irq.h
> index b2b0c64..e840bf1 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -6,6 +6,12 @@
>
> #include <asm-generic/irq.h>
>
> +#ifdef CONFIG_SMP
> +extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
> + bool exclude_self);
> +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> +#endif
> +
> struct pt_regs;
>
> static inline int nr_legacy_irqs(void)
> diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> index e0a9e03..e1dc19d 100644
> --- a/arch/arm64/kernel/ipi_nmi.c
> +++ b/arch/arm64/kernel/ipi_nmi.c
> @@ -9,6 +9,7 @@
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> #include <linux/kgdb.h>
> +#include <linux/nmi.h>
> #include <linux/smp.h>
>
> #include <asm/nmi.h>
> @@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> *mask)
> __ipi_send_mask(ipi_desc, mask);
> }
>
> +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool
> exclude_self)
> +{
> + nmi_trigger_cpumask_backtrace(mask, exclude_self,
> + arch_send_call_nmi_func_ipi_mask);
> +}
> +
> static irqreturn_t ipi_nmi_handler(int irq, void *data)
> {
> unsigned int cpu = smp_processor_id();
>
> - ipi_kgdb_nmicallback(cpu, get_irq_regs());
> + if (nmi_cpu_backtrace(get_irq_regs()))
> + goto out;
>
> + ipi_kgdb_nmicallback(cpu, get_irq_regs());
> +out:
> return IRQ_HANDLED;
> }
Can't you have *both* a request for a backtrace and a KGDB call?
It really shouldn't be either/or. It also outlines how well shared
interrupts work with edge triggered signalling...
M.
--
Jazz is not dead. It just smells funny...
On 2020-10-14 12:12, Sumit Garg wrote:
> Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
> particular platform doesn't support pseudo NMIs, then request IPI as a
> regular IRQ.
>
> The main motivation for this feature is to have an IPI that can be
> leveraged to invoke NMI functions on other CPUs. And current
> prospective
> users are NMI backtrace and KGDB CPUs round-up whose support is added
> via future patches.
>
> Signed-off-by: Sumit Garg <[email protected]>
> ---
> arch/arm64/include/asm/nmi.h | 16 +++++++++
> arch/arm64/kernel/Makefile | 2 +-
> arch/arm64/kernel/ipi_nmi.c | 77
> ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 94 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/include/asm/nmi.h
> create mode 100644 arch/arm64/kernel/ipi_nmi.c
[...]
> + irq_set_status_flags(ipi, IRQ_HIDDEN);
Another thing is this. Why are you hiding this from /proc/interrupts?
The only reason the other IPIs are hidden is that displaying them as
"normal" interrupts would be a change in userspace ABI.
In your case, this is something new that can perfectly appear as
a standard interrupt (and I don't see how you'd display the
statistics otherwise).
M.
--
Jazz is not dead. It just smells funny...
On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
> > particular platform doesn't support pseudo NMIs, then request IPI as a
> > regular IRQ.
> >
> > The main motivation for this feature is to have an IPI that can be
> > leveraged to invoke NMI functions on other CPUs. And current
> > prospective
> > users are NMI backtrace and KGDB CPUs round-up whose support is added
> > via future patches.
> >
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > arch/arm64/include/asm/nmi.h | 16 +++++++++
> > arch/arm64/kernel/Makefile | 2 +-
> > arch/arm64/kernel/ipi_nmi.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 94 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/include/asm/nmi.h
> > create mode 100644 arch/arm64/kernel/ipi_nmi.c
> >
> > diff --git a/arch/arm64/include/asm/nmi.h
> > b/arch/arm64/include/asm/nmi.h
> > new file mode 100644
> > index 0000000..3433c55
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/nmi.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_NMI_H
> > +#define __ASM_NMI_H
> > +
> > +#ifndef __ASSEMBLER__
> > +
> > +#include <linux/cpumask.h>
> > +
> > +extern void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask);
> > +
> > +void set_smp_ipi_nmi(int ipi);
> > +void ipi_nmi_setup(int cpu);
> > +void ipi_nmi_teardown(int cpu);
> > +
> > +#endif /* !__ASSEMBLER__ */
> > +#endif
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index bbaf0bc..525a1e0 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -17,7 +17,7 @@ obj-y := debug-monitors.o entry.o irq.o fpsimd.o \
> > return_address.o cpuinfo.o cpu_errata.o \
> > cpufeature.o alternative.o cacheinfo.o \
> > smp.o smp_spin_table.o topology.o smccc-call.o \
> > - syscall.o proton-pack.o
> > + syscall.o proton-pack.o ipi_nmi.o
> >
> > targets += efi-entry.o
> >
> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> > new file mode 100644
> > index 0000000..a959256
> > --- /dev/null
> > +++ b/arch/arm64/kernel/ipi_nmi.c
> > @@ -0,0 +1,77 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * NMI support for IPIs
> > + *
> > + * Copyright (C) 2020 Linaro Limited
> > + * Author: Sumit Garg <[email protected]>
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/smp.h>
> > +
> > +#include <asm/nmi.h>
> > +
> > +static struct irq_desc *ipi_desc __read_mostly;
> > +static int ipi_id __read_mostly;
> > +static bool is_nmi __read_mostly;
> > +
> > +void arch_send_call_nmi_func_ipi_mask(cpumask_t *mask)
> > +{
> > + if (WARN_ON_ONCE(!ipi_desc))
> > + return;
> > +
> > + __ipi_send_mask(ipi_desc, mask);
> > +}
> > +
> > +static irqreturn_t ipi_nmi_handler(int irq, void *data)
> > +{
> > + /* nop, NMI handlers for special features can be added here. */
> > +
> > + return IRQ_HANDLED;
>
> This definitely is the *wrong* default. If nothing is explicitly
> handling the interrupt, it should be reported as such to the core
> code to be disabled if this happens too often.
Okay will fix default as "IRQ_NONE".
>
> > +}
> > +
> > +void ipi_nmi_setup(int cpu)
>
> The naming is awful. "ipi" is nowhere specific enough (we already have
> another 7 of them), and this doesn't necessarily uses pseudo-NMIs, since
> you are requesting IRQs.
How about following naming conventions?
- dynamic_ipi_setup()
- dynamic_ipi_teardown()
- set_smp_dynamic_ipi()
>
> > +{
> > + if (!ipi_desc)
> > + return;
> > +
> > + if (is_nmi) {
> > + if (!prepare_percpu_nmi(ipi_id))
> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> > + } else {
> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
>
> I'm not keen on this. Normal IRQs can't reliably work, so why do you
> even bother with this?
Yeah I agree but we need to support existing functionality for kgdb
roundup and sysrq backtrace using normal IRQs as well.
>
> > + }
> > +}
> > +
> > +void ipi_nmi_teardown(int cpu)
> > +{
> > + if (!ipi_desc)
> > + return;
> > +
> > + if (is_nmi) {
> > + disable_percpu_nmi(ipi_id);
> > + teardown_percpu_nmi(ipi_id);
> > + } else {
> > + disable_percpu_irq(ipi_id);
> > + }
> > +}
> > +
> > +void __init set_smp_ipi_nmi(int ipi)
> > +{
> > + int err;
> > +
> > + err = request_percpu_nmi(ipi, ipi_nmi_handler, "IPI", &cpu_number);
> > + if (err) {
> > + err = request_percpu_irq(ipi, ipi_nmi_handler, "IPI",
> > + &cpu_number);
> > + WARN_ON(err);
> > + is_nmi = false;
> > + } else {
> > + is_nmi = true;
> > + }
> > +
> > + ipi_desc = irq_to_desc(ipi);
> > + irq_set_status_flags(ipi, IRQ_HIDDEN);
> > + ipi_id = ipi;
>
> Updating global state without checking for errors doesn't seem
> like a great move.
Are you worried about failure from request_percpu_irq()? If yes, there
is an explicit warning for that and if you like I can add an
additional check while updating the global state.
BTW, this is written on an existing pattern as for other 7 IPIs requests [1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/smp.c#n988
-Sumit
>
> M.
> --
> Jazz is not dead. It just smells funny...
On Mon, 19 Oct 2020 at 17:26, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > Introduce framework to turn an IPI as NMI using pseudo NMIs. In case a
> > particular platform doesn't support pseudo NMIs, then request IPI as a
> > regular IRQ.
> >
> > The main motivation for this feature is to have an IPI that can be
> > leveraged to invoke NMI functions on other CPUs. And current
> > prospective
> > users are NMI backtrace and KGDB CPUs round-up whose support is added
> > via future patches.
> >
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > arch/arm64/include/asm/nmi.h | 16 +++++++++
> > arch/arm64/kernel/Makefile | 2 +-
> > arch/arm64/kernel/ipi_nmi.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 94 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm64/include/asm/nmi.h
> > create mode 100644 arch/arm64/kernel/ipi_nmi.c
>
> [...]
>
> > + irq_set_status_flags(ipi, IRQ_HIDDEN);
>
> Another thing is this. Why are you hiding this from /proc/interrupts?
> The only reason the other IPIs are hidden is that displaying them as
> "normal" interrupts would be a change in userspace ABI.
>
> In your case, this is something new that can perfectly appear as
> a standard interrupt (and I don't see how you'd display the
> statistics otherwise).
Makes sense. I will remove this flag for this IPI so that it can be
displayed as a standard interrupt.
-Sumit
>
> M.
> --
> Jazz is not dead. It just smells funny...
On Mon, 19 Oct 2020 at 17:29, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > Allocate an unused IPI that can be turned as NMI using ipi_nmi
> > framework.
>
> This doesn't do any allocation, as far as I can see. It relies on
> the initial grant from the interrupt controller to be larger than
> what the kernel currently uses.
>
Okay, will update the commit message as s/Allocate/Assign/.
-Sumit
> > Also, invoke corresponding NMI setup/teardown APIs.
> >
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > arch/arm64/kernel/smp.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> > index 82e75fc..129ebfb 100644
> > --- a/arch/arm64/kernel/smp.c
> > +++ b/arch/arm64/kernel/smp.c
> > @@ -43,6 +43,7 @@
> > #include <asm/daifflags.h>
> > #include <asm/kvm_mmu.h>
> > #include <asm/mmu_context.h>
> > +#include <asm/nmi.h>
> > #include <asm/numa.h>
> > #include <asm/processor.h>
> > #include <asm/smp_plat.h>
> > @@ -962,6 +963,8 @@ static void ipi_setup(int cpu)
> >
> > for (i = 0; i < nr_ipi; i++)
> > enable_percpu_irq(ipi_irq_base + i, 0);
> > +
> > + ipi_nmi_setup(cpu);
> > }
> >
> > #ifdef CONFIG_HOTPLUG_CPU
> > @@ -974,6 +977,8 @@ static void ipi_teardown(int cpu)
> >
> > for (i = 0; i < nr_ipi; i++)
> > disable_percpu_irq(ipi_irq_base + i);
> > +
> > + ipi_nmi_teardown(cpu);
> > }
> > #endif
> >
> > @@ -995,6 +1000,9 @@ void __init set_smp_ipi_range(int ipi_base, int n)
> > irq_set_status_flags(ipi_base + i, IRQ_HIDDEN);
> > }
> >
> > + if (n > nr_ipi)
> > + set_smp_ipi_nmi(ipi_base + nr_ipi);
> > +
> > ipi_irq_base = ipi_base;
> >
> > /* Setup the boot CPU immediately */
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
On Mon, 19 Oct 2020 at 17:50, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > Enable NMI backtrace support on arm64 using IPI turned as an NMI
> > leveraging pseudo NMIs support. It is now possible for users to get a
> > backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
> >
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > arch/arm64/include/asm/irq.h | 6 ++++++
> > arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/irq.h
> > b/arch/arm64/include/asm/irq.h
> > index b2b0c64..e840bf1 100644
> > --- a/arch/arm64/include/asm/irq.h
> > +++ b/arch/arm64/include/asm/irq.h
> > @@ -6,6 +6,12 @@
> >
> > #include <asm-generic/irq.h>
> >
> > +#ifdef CONFIG_SMP
> > +extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
> > + bool exclude_self);
> > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> > +#endif
> > +
> > struct pt_regs;
> >
> > static inline int nr_legacy_irqs(void)
> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> > index e0a9e03..e1dc19d 100644
> > --- a/arch/arm64/kernel/ipi_nmi.c
> > +++ b/arch/arm64/kernel/ipi_nmi.c
> > @@ -9,6 +9,7 @@
> > #include <linux/interrupt.h>
> > #include <linux/irq.h>
> > #include <linux/kgdb.h>
> > +#include <linux/nmi.h>
> > #include <linux/smp.h>
> >
> > #include <asm/nmi.h>
> > @@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> > *mask)
> > __ipi_send_mask(ipi_desc, mask);
> > }
> >
> > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool
> > exclude_self)
> > +{
> > + nmi_trigger_cpumask_backtrace(mask, exclude_self,
> > + arch_send_call_nmi_func_ipi_mask);
> > +}
> > +
> > static irqreturn_t ipi_nmi_handler(int irq, void *data)
> > {
> > unsigned int cpu = smp_processor_id();
> >
> > - ipi_kgdb_nmicallback(cpu, get_irq_regs());
> > + if (nmi_cpu_backtrace(get_irq_regs()))
> > + goto out;
> >
> > + ipi_kgdb_nmicallback(cpu, get_irq_regs());
> > +out:
> > return IRQ_HANDLED;
> > }
>
> Can't you have *both* a request for a backtrace and a KGDB call?
> It really shouldn't be either/or. It also outlines how well shared
> interrupts work with edge triggered signalling...
Unfortunately, NMIs doesn't seem to support shared mode [1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/manage.c#n1480
-Sumit
>
> M.
> --
> Jazz is not dead. It just smells funny...
On 2020-10-20 07:43, Sumit Garg wrote:
> On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
[...]
>> > +{
>> > + if (!ipi_desc)
>> > + return;
>> > +
>> > + if (is_nmi) {
>> > + if (!prepare_percpu_nmi(ipi_id))
>> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
>> > + } else {
>> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
>>
>> I'm not keen on this. Normal IRQs can't reliably work, so why do you
>> even bother with this?
>
> Yeah I agree but we need to support existing functionality for kgdb
> roundup and sysrq backtrace using normal IRQs as well.
When has this become a requirement? I don't really see the point in
implementing something that is known not to work.
M.
--
Jazz is not dead. It just smells funny...
On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-20 07:43, Sumit Garg wrote:
> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
>
> [...]
>
> >> > +{
> >> > + if (!ipi_desc)
> >> > + return;
> >> > +
> >> > + if (is_nmi) {
> >> > + if (!prepare_percpu_nmi(ipi_id))
> >> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> >> > + } else {
> >> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
> >>
> >> I'm not keen on this. Normal IRQs can't reliably work, so why do you
> >> even bother with this?
> >
> > Yeah I agree but we need to support existing functionality for kgdb
> > roundup and sysrq backtrace using normal IRQs as well.
>
> When has this become a requirement? I don't really see the point in
> implementing something that is known not to work.
>
For kgdb:
Default implementation [1] uses smp_call_function_single_async() which
in turn will invoke IPI as a normal IRQ to roundup CPUs.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/debug/debug_core.c#n244
For sysrq backtrace:
Default implementation [2] fallbacks to smp_call_function() (IPI as a
normal IRQ) to print backtrace in case architecture doesn't provide
arch_trigger_cpumask_backtrace() hook.
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/sysrq.c#n250
So in general, IPI as a normal IRQ is still useful for debugging but
it can't debug a core which is stuck in deadlock with interrupts
disabled.
And since we choose override default implementations for pseudo NMI
support, we need to be backwards compatible for platforms which don't
possess pseudo NMI support.
-Sumit
> M.
> --
> Jazz is not dead. It just smells funny...
On Mon, 19 Oct 2020 at 17:45, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> > leveraged to round up CPUs which are stuck in hard lockup state with
> > interrupts disabled that wouldn't be possible with a normal IPI.
> >
> > So instead switch to round up CPUs using IPI turned as NMI. And in
> > case a particular arm64 platform doesn't supports pseudo NMIs,
> > this IPI will act as a normal IPI which maintains existing kgdb
> > functionality.
> >
> > Signed-off-by: Sumit Garg <[email protected]>
> > ---
> > arch/arm64/include/asm/kgdb.h | 8 ++++++++
> > arch/arm64/kernel/ipi_nmi.c | 5 ++++-
> > arch/arm64/kernel/kgdb.c | 21 +++++++++++++++++++++
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/kgdb.h
> > b/arch/arm64/include/asm/kgdb.h
> > index 21fc85e..6f3d3af 100644
> > --- a/arch/arm64/include/asm/kgdb.h
> > +++ b/arch/arm64/include/asm/kgdb.h
> > @@ -24,6 +24,14 @@ static inline void arch_kgdb_breakpoint(void)
> > extern void kgdb_handle_bus_error(void);
> > extern int kgdb_fault_expected;
> >
> > +#ifdef CONFIG_KGDB
> > +extern void ipi_kgdb_nmicallback(int cpu, void *regs);
> > +#else
> > +static inline void ipi_kgdb_nmicallback(int cpu, void *regs)
> > +{
> > +}
> > +#endif
> > +
> > #endif /* !__ASSEMBLY__ */
> >
> > /*
> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> > index a959256..e0a9e03 100644
> > --- a/arch/arm64/kernel/ipi_nmi.c
> > +++ b/arch/arm64/kernel/ipi_nmi.c
> > @@ -8,6 +8,7 @@
> >
> > #include <linux/interrupt.h>
> > #include <linux/irq.h>
> > +#include <linux/kgdb.h>
> > #include <linux/smp.h>
> >
> > #include <asm/nmi.h>
> > @@ -26,7 +27,9 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> > *mask)
> >
> > static irqreturn_t ipi_nmi_handler(int irq, void *data)
> > {
> > - /* nop, NMI handlers for special features can be added here. */
> > + unsigned int cpu = smp_processor_id();
> > +
> > + ipi_kgdb_nmicallback(cpu, get_irq_regs());
>
> Please add a return value to ipi_kgdb_nmicallback(), and check it
> before returning IRQ_HANDLED.
Okay.
>
> Thinking a bit more about the whole thing, you should have a way to
> avoid requesting the NMI if there is no user for it (there is nothing
> worse than an enabled interrupt without handlers...).
I think IPIs or SGIs (for arm64) are different from normal interrupts
in this aspect as if there is no handler then we can be sure that
corresponding SGI won't be invoked as their invocation is controlled
via software.
This is the similar case with other IPIs as well whose handlers are
optionally enabled, see:
- IPI_CPU_CRASH_STOP
- IPI_TIMER
- IPI_IRQ_WORK
- IPI_WAKEUP
>
> >
> > return IRQ_HANDLED;
> > }
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 1a157ca3..0991275 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -17,6 +17,7 @@
> >
> > #include <asm/debug-monitors.h>
> > #include <asm/insn.h>
> > +#include <asm/nmi.h>
> > #include <asm/traps.h>
> >
> > struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
> > @@ -353,3 +354,23 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt
> > *bpt)
> > return aarch64_insn_write((void *)bpt->bpt_addr,
> > *(u32 *)bpt->saved_instr);
> > }
> > +
> > +void ipi_kgdb_nmicallback(int cpu, void *regs)
> > +{
> > + if (atomic_read(&kgdb_active) != -1)
> > + kgdb_nmicallback(cpu, regs);
> > +}
> > +
> > +#ifdef CONFIG_SMP
>
> There is no such thing as an arm64 UP kernel.
>
Okay, will remove this #ifdef.
> > +void kgdb_roundup_cpus(void)
> > +{
> > + struct cpumask mask;
> > +
> > + cpumask_copy(&mask, cpu_online_mask);
> > + cpumask_clear_cpu(raw_smp_processor_id(), &mask);
> > + if (cpumask_empty(&mask))
> > + return;
> > +
> > + arch_send_call_nmi_func_ipi_mask(&mask);
>
> Surely you can come up with a less convoluted name for this function.
> arm64_send_nmi() would be plenty in my opinion.
Okay, will use it instead.
-Sumit
>
> > +}
> > +#endif
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:
> On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <[email protected]> wrote:
> >
> > On 2020-10-20 07:43, Sumit Garg wrote:
> > > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
> >
> > [...]
> >
> > >> > +{
> > >> > + if (!ipi_desc)
> > >> > + return;
> > >> > +
> > >> > + if (is_nmi) {
> > >> > + if (!prepare_percpu_nmi(ipi_id))
> > >> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> > >> > + } else {
> > >> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
> > >>
> > >> I'm not keen on this. Normal IRQs can't reliably work, so why do you
> > >> even bother with this?
> > >
> > > Yeah I agree but we need to support existing functionality for kgdb
> > > roundup and sysrq backtrace using normal IRQs as well.
> >
> > When has this become a requirement? I don't really see the point in
> > implementing something that is known not to work.
> >
>
> For kgdb:
>
> Default implementation [1] uses smp_call_function_single_async() which
> in turn will invoke IPI as a normal IRQ to roundup CPUs.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/debug/debug_core.c#n244
>
> For sysrq backtrace:
>
> Default implementation [2] fallbacks to smp_call_function() (IPI as a
> normal IRQ) to print backtrace in case architecture doesn't provide
> arch_trigger_cpumask_backtrace() hook.
>
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/sysrq.c#n250
>
> So in general, IPI as a normal IRQ is still useful for debugging but
> it can't debug a core which is stuck in deadlock with interrupts
> disabled.
>
> And since we choose override default implementations for pseudo NMI
> support, we need to be backwards compatible for platforms which don't
> possess pseudo NMI support.
Do the fallback implementations require a new IPI? The fallbacks
could rely on existing mechanisms such as the smp_call_function
family.
Daniel.
On 2020-10-20 13:25, Daniel Thompson wrote:
> On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:
[...]
>> So in general, IPI as a normal IRQ is still useful for debugging but
>> it can't debug a core which is stuck in deadlock with interrupts
>> disabled.
>>
>> And since we choose override default implementations for pseudo NMI
>> support, we need to be backwards compatible for platforms which don't
>> possess pseudo NMI support.
>
> Do the fallback implementations require a new IPI? The fallbacks
> could rely on existing mechanisms such as the smp_call_function
> family.
Indeed. I'd be worried of using that mechanism for NMIs, but normal
IPIs should stick to the normal cross-call stuff.
The jury is still out on why this is a good idea, given that it
doesn't work in the only interesting case (deadlocked CPUs).
M.
--
Jazz is not dead. It just smells funny...
On Tue, 20 Oct 2020 at 18:02, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-20 13:25, Daniel Thompson wrote:
> > On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:
>
> [...]
>
> >> So in general, IPI as a normal IRQ is still useful for debugging but
> >> it can't debug a core which is stuck in deadlock with interrupts
> >> disabled.
> >>
> >> And since we choose override default implementations for pseudo NMI
> >> support, we need to be backwards compatible for platforms which don't
> >> possess pseudo NMI support.
> >
> > Do the fallback implementations require a new IPI? The fallbacks
> > could rely on existing mechanisms such as the smp_call_function
> > family.
>
> Indeed. I'd be worried of using that mechanism for NMIs, but normal
> IPIs should stick to the normal cross-call stuff.
Yes, the fallback implementations could rely on existing cross-call
stuff but current framework only allows this fallback choice at
compile time and to make this choice at runtime, we need additional
framework changes like allowing arch_trigger_cpumask_backtrace() to
return a boolean in order to switch to fallback mode, similar would be
the case for kgdb.
I think this should be doable but I am still not sure regarding the
advantages of using existing IPI vs new IPI (which we will already use
in case of pseudo NMI support) as normal IRQ.
>
> The jury is still out on why this is a good idea, given that it
> doesn't work in the only interesting case (deadlocked CPUs).
I think CPU soft-lockups (interrupts enabled) is an interesting case
to debug as well.
-Sumit
>
> M.
> --
> Jazz is not dead. It just smells funny...
On 2020-10-20 10:13, Sumit Garg wrote:
> On Mon, 19 Oct 2020 at 17:50, Marc Zyngier <[email protected]> wrote:
>>
>> On 2020-10-14 12:12, Sumit Garg wrote:
>> > Enable NMI backtrace support on arm64 using IPI turned as an NMI
>> > leveraging pseudo NMIs support. It is now possible for users to get a
>> > backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
>> >
>> > Signed-off-by: Sumit Garg <[email protected]>
>> > ---
>> > arch/arm64/include/asm/irq.h | 6 ++++++
>> > arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
>> > 2 files changed, 17 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/include/asm/irq.h
>> > b/arch/arm64/include/asm/irq.h
>> > index b2b0c64..e840bf1 100644
>> > --- a/arch/arm64/include/asm/irq.h
>> > +++ b/arch/arm64/include/asm/irq.h
>> > @@ -6,6 +6,12 @@
>> >
>> > #include <asm-generic/irq.h>
>> >
>> > +#ifdef CONFIG_SMP
>> > +extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
>> > + bool exclude_self);
>> > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
>> > +#endif
>> > +
>> > struct pt_regs;
>> >
>> > static inline int nr_legacy_irqs(void)
>> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
>> > index e0a9e03..e1dc19d 100644
>> > --- a/arch/arm64/kernel/ipi_nmi.c
>> > +++ b/arch/arm64/kernel/ipi_nmi.c
>> > @@ -9,6 +9,7 @@
>> > #include <linux/interrupt.h>
>> > #include <linux/irq.h>
>> > #include <linux/kgdb.h>
>> > +#include <linux/nmi.h>
>> > #include <linux/smp.h>
>> >
>> > #include <asm/nmi.h>
>> > @@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
>> > *mask)
>> > __ipi_send_mask(ipi_desc, mask);
>> > }
>> >
>> > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool
>> > exclude_self)
>> > +{
>> > + nmi_trigger_cpumask_backtrace(mask, exclude_self,
>> > + arch_send_call_nmi_func_ipi_mask);
>> > +}
>> > +
>> > static irqreturn_t ipi_nmi_handler(int irq, void *data)
>> > {
>> > unsigned int cpu = smp_processor_id();
>> >
>> > - ipi_kgdb_nmicallback(cpu, get_irq_regs());
>> > + if (nmi_cpu_backtrace(get_irq_regs()))
>> > + goto out;
>> >
>> > + ipi_kgdb_nmicallback(cpu, get_irq_regs());
>> > +out:
>> > return IRQ_HANDLED;
>> > }
>>
>> Can't you have *both* a request for a backtrace and a KGDB call?
>> It really shouldn't be either/or. It also outlines how well shared
>> interrupts work with edge triggered signalling...
>
> Unfortunately, NMIs doesn't seem to support shared mode [1].
You are totally missing the point: shared interrupts *cannot* work
reliably with edge signalling. What I am saying here is that you need
implement the sharing yourself in this function.
M.
--
Jazz is not dead. It just smells funny...
On Wed, 21 Oct 2020 at 16:02, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-20 10:13, Sumit Garg wrote:
> > On Mon, 19 Oct 2020 at 17:50, Marc Zyngier <[email protected]> wrote:
> >>
> >> On 2020-10-14 12:12, Sumit Garg wrote:
> >> > Enable NMI backtrace support on arm64 using IPI turned as an NMI
> >> > leveraging pseudo NMIs support. It is now possible for users to get a
> >> > backtrace of a CPU stuck in hard-lockup using magic SYSRQ.
> >> >
> >> > Signed-off-by: Sumit Garg <[email protected]>
> >> > ---
> >> > arch/arm64/include/asm/irq.h | 6 ++++++
> >> > arch/arm64/kernel/ipi_nmi.c | 12 +++++++++++-
> >> > 2 files changed, 17 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/arch/arm64/include/asm/irq.h
> >> > b/arch/arm64/include/asm/irq.h
> >> > index b2b0c64..e840bf1 100644
> >> > --- a/arch/arm64/include/asm/irq.h
> >> > +++ b/arch/arm64/include/asm/irq.h
> >> > @@ -6,6 +6,12 @@
> >> >
> >> > #include <asm-generic/irq.h>
> >> >
> >> > +#ifdef CONFIG_SMP
> >> > +extern void arch_trigger_cpumask_backtrace(const cpumask_t *mask,
> >> > + bool exclude_self);
> >> > +#define arch_trigger_cpumask_backtrace arch_trigger_cpumask_backtrace
> >> > +#endif
> >> > +
> >> > struct pt_regs;
> >> >
> >> > static inline int nr_legacy_irqs(void)
> >> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> >> > index e0a9e03..e1dc19d 100644
> >> > --- a/arch/arm64/kernel/ipi_nmi.c
> >> > +++ b/arch/arm64/kernel/ipi_nmi.c
> >> > @@ -9,6 +9,7 @@
> >> > #include <linux/interrupt.h>
> >> > #include <linux/irq.h>
> >> > #include <linux/kgdb.h>
> >> > +#include <linux/nmi.h>
> >> > #include <linux/smp.h>
> >> >
> >> > #include <asm/nmi.h>
> >> > @@ -25,12 +26,21 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> >> > *mask)
> >> > __ipi_send_mask(ipi_desc, mask);
> >> > }
> >> >
> >> > +void arch_trigger_cpumask_backtrace(const cpumask_t *mask, bool
> >> > exclude_self)
> >> > +{
> >> > + nmi_trigger_cpumask_backtrace(mask, exclude_self,
> >> > + arch_send_call_nmi_func_ipi_mask);
> >> > +}
> >> > +
> >> > static irqreturn_t ipi_nmi_handler(int irq, void *data)
> >> > {
> >> > unsigned int cpu = smp_processor_id();
> >> >
> >> > - ipi_kgdb_nmicallback(cpu, get_irq_regs());
> >> > + if (nmi_cpu_backtrace(get_irq_regs()))
> >> > + goto out;
> >> >
> >> > + ipi_kgdb_nmicallback(cpu, get_irq_regs());
> >> > +out:
> >> > return IRQ_HANDLED;
> >> > }
> >>
> >> Can't you have *both* a request for a backtrace and a KGDB call?
> >> It really shouldn't be either/or. It also outlines how well shared
> >> interrupts work with edge triggered signalling...
> >
> > Unfortunately, NMIs doesn't seem to support shared mode [1].
>
> You are totally missing the point: shared interrupts *cannot* work
> reliably with edge signalling. What I am saying here is that you need
> implement the sharing yourself in this function.
Ah, I see your point now. Will instead share this IPI among both handlers.
-Sumit
>
> M.
> --
> Jazz is not dead. It just smells funny...
On 2020-10-20 12:22, Sumit Garg wrote:
> On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <[email protected]> wrote:
>>
>> On 2020-10-20 07:43, Sumit Garg wrote:
>> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
>>
>> [...]
>>
>> >> > +{
>> >> > + if (!ipi_desc)
>> >> > + return;
>> >> > +
>> >> > + if (is_nmi) {
>> >> > + if (!prepare_percpu_nmi(ipi_id))
>> >> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
>> >> > + } else {
>> >> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
>> >>
>> >> I'm not keen on this. Normal IRQs can't reliably work, so why do you
>> >> even bother with this?
>> >
>> > Yeah I agree but we need to support existing functionality for kgdb
>> > roundup and sysrq backtrace using normal IRQs as well.
>>
>> When has this become a requirement? I don't really see the point in
>> implementing something that is known not to work.
>>
>
> For kgdb:
>
> Default implementation [1] uses smp_call_function_single_async() which
> in turn will invoke IPI as a normal IRQ to roundup CPUs.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/debug/debug_core.c#n244
>
> For sysrq backtrace:
>
> Default implementation [2] fallbacks to smp_call_function() (IPI as a
> normal IRQ) to print backtrace in case architecture doesn't provide
> arch_trigger_cpumask_backtrace() hook.
>
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/sysrq.c#n250
>
> So in general, IPI as a normal IRQ is still useful for debugging but
> it can't debug a core which is stuck in deadlock with interrupts
> disabled.
And that's not something we implement today for good reasons:
it *cannot* work reliably. What changed that we all of a sudden need it?
> And since we choose override default implementations for pseudo NMI
> support, we need to be backwards compatible for platforms which don't
> possess pseudo NMI support.
No. There is nothing to be "backward compatible" with, because
- this isn't a userspace visible feature
- *it doesn't work*
So please drop this non-feature from this series.
M.
--
Jazz is not dead. It just smells funny...
On Wed, 21 Oct 2020 at 15:57, Marc Zyngier <[email protected]> wrote:
>
> On 2020-10-20 12:22, Sumit Garg wrote:
> > On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <[email protected]> wrote:
> >>
> >> On 2020-10-20 07:43, Sumit Garg wrote:
> >> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <[email protected]> wrote:
> >>
> >> [...]
> >>
> >> >> > +{
> >> >> > + if (!ipi_desc)
> >> >> > + return;
> >> >> > +
> >> >> > + if (is_nmi) {
> >> >> > + if (!prepare_percpu_nmi(ipi_id))
> >> >> > + enable_percpu_nmi(ipi_id, IRQ_TYPE_NONE);
> >> >> > + } else {
> >> >> > + enable_percpu_irq(ipi_id, IRQ_TYPE_NONE);
> >> >>
> >> >> I'm not keen on this. Normal IRQs can't reliably work, so why do you
> >> >> even bother with this?
> >> >
> >> > Yeah I agree but we need to support existing functionality for kgdb
> >> > roundup and sysrq backtrace using normal IRQs as well.
> >>
> >> When has this become a requirement? I don't really see the point in
> >> implementing something that is known not to work.
> >>
> >
> > For kgdb:
> >
> > Default implementation [1] uses smp_call_function_single_async() which
> > in turn will invoke IPI as a normal IRQ to roundup CPUs.
> >
> > [1]
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/debug/debug_core.c#n244
> >
> > For sysrq backtrace:
> >
> > Default implementation [2] fallbacks to smp_call_function() (IPI as a
> > normal IRQ) to print backtrace in case architecture doesn't provide
> > arch_trigger_cpumask_backtrace() hook.
> >
> > [2]
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/sysrq.c#n250
> >
> > So in general, IPI as a normal IRQ is still useful for debugging but
> > it can't debug a core which is stuck in deadlock with interrupts
> > disabled.
>
> And that's not something we implement today for good reasons:
> it *cannot* work reliably. What changed that we all of a sudden need it?
>
> > And since we choose override default implementations for pseudo NMI
> > support, we need to be backwards compatible for platforms which don't
> > possess pseudo NMI support.
>
> No. There is nothing to be "backward compatible" with, because
> - this isn't a userspace visible feature
> - *it doesn't work*
>
> So please drop this non-feature from this series.
>
Okay, fair enough. I will drop support for new IPI being normal IRQ
and instead update sysrq backtrace and kgdb roundup frameworks to use
existing cross-calls stuff in case a platform doesn't possess pseudo
NMI support.
-Sumit
> M.
> --
> Jazz is not dead. It just smells funny...