[PATCH v5 0/5] arm64: Add framework to turn an IPI as NMI

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>
---
 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 <sumit.garg@linaro.org>
+ */
+
+#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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Add support to handle SGIs as regular NMIs. As SGIs or IPIs defaults to a
special flow handler: handle_percpu_devid_fasteoi_ipi(), so skip NMI
handler update in case of SGIs.

Also, enable NMI support prior to gic_smp_init() as allocation of SGIs
as IRQs/NMIs happen as part of this routine.

Signed-off-by: Sumit Garg <redacted>
---
 drivers/irqchip/irq-gic-v3.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 16fecc0..5efc865 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c

@@ -477,6 +477,11 @@ static int gic_irq_nmi_setup(struct irq_data *d)
 	if (WARN_ON(gic_irq(d) >= 8192))
 		return -EINVAL;
 
+	if (get_intid_range(d) == SGI_RANGE) {
+		gic_irq_set_prio(d, GICD_INT_NMI_PRI);
+		return 0;
+	}
+
 	/* desc lock should already be held */
 	if (gic_irq_in_rdist(d)) {
 		u32 idx = gic_get_ppi_index(d);

@@ -514,6 +519,11 @@ static void gic_irq_nmi_teardown(struct irq_data *d)
 	if (WARN_ON(gic_irq(d) >= 8192))
 		return;
 
+	if (get_intid_range(d) == SGI_RANGE) {
+		gic_irq_set_prio(d, GICD_INT_DEF_PRI);
+		return;
+	}
+
 	/* desc lock should already be held */
 	if (gic_irq_in_rdist(d)) {
 		u32 idx = gic_get_ppi_index(d);

@@ -1708,6 +1718,7 @@ static int __init gic_init_bases(void __iomem *dist_base,
 
 	gic_dist_init();
 	gic_cpu_init();
+	gic_enable_nmi_support();
 	gic_smp_init();
 	gic_cpu_pm_init();
 

@@ -1719,8 +1730,6 @@ static int __init gic_init_bases(void __iomem *dist_base,
 			gicv2m_init(handle, gic_data.domain);
 	}
 
-	gic_enable_nmi_support();
-
 	return 0;
 
 out_free:

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 14, 2020 at 04:42:09PM +0530, Sumit Garg wrote:

Looks good to me. Please feel free to add:

        Reviewed-by: Masayoshi Mizuma [off-list ref]

Thanks!
Masa


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 14, 2020 at 04:42:11PM +0530, Sumit Garg wrote:

It works well. Please feel free to add:

        Tested-by: Masayoshi Mizuma [off-list ref]

Thanks!
Masa

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 14, 2020 at 04:42:08PM +0530, Sumit Garg wrote:

Looks good to me. Please feel free to add:

        Reviewed-by: Masayoshi Mizuma [off-list ref]

Thanks!
Masa

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, Oct 14, 2020 at 04:42:07PM +0530, Sumit Garg wrote:

Looks good to me. Please feel free to add:

	Reviewed-by: Masayoshi Mizuma [off-list ref]

Thanks!
Masa

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-14 12:12, Sumit Garg wrote:

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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 <redacted>
---
 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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-14 12:12, Sumit Garg wrote:

Add support to handle SGIs as regular NMIs. As SGIs or IPIs defaults to 
a

There is nothing "regular" about NMIs. Drop "or IPIs". 
s/defaults/default/

Please follow the existing control flow, or rework it to be organised by 
range.

Same here.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-14 12:12, Sumit Garg wrote:

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...).

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-14 12:12, Sumit Garg wrote:

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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 <sumit.garg@linaro.org>
+ */
+
+#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".

quoted

+}
+
+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()

quoted

+{
+     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.

quoted

+     }
+}
+
+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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:26, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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

[...]

quoted

+     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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:29, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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

quoted

Also, invoke corresponding NMI setup/teardown APIs.

Signed-off-by: Sumit Garg <redacted>
---
 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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:37, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

Add support to handle SGIs as regular NMIs. As SGIs or IPIs defaults to
a

There is nothing "regular" about NMIs.

Okay, will do s/regular/pseudo/.

Drop "or IPIs".
s/defaults/default/

Ack.

quoted

special flow handler: handle_percpu_devid_fasteoi_ipi(), so skip NMI
handler update in case of SGIs.

Also, enable NMI support prior to gic_smp_init() as allocation of SGIs
as IRQs/NMIs happen as part of this routine.

Signed-off-by: Sumit Garg <redacted>
---
 drivers/irqchip/irq-gic-v3.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c

b/drivers/irqchip/irq-gic-v3.c
index 16fecc0..5efc865 100644

--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c

@@ -477,6 +477,11 @@ static int gic_irq_nmi_setup(struct irq_data *d)
      if (WARN_ON(gic_irq(d) >= 8192))
              return -EINVAL;

+     if (get_intid_range(d) == SGI_RANGE) {
+             gic_irq_set_prio(d, GICD_INT_NMI_PRI);
+             return 0;
+     }
+

Please follow the existing control flow, or rework it to be organised by
range.

Okay.

quoted

      /* desc lock should already be held */
      if (gic_irq_in_rdist(d)) {
              u32 idx = gic_get_ppi_index(d);

@@ -514,6 +519,11 @@ static void gic_irq_nmi_teardown(struct irq_data

*d)
      if (WARN_ON(gic_irq(d) >= 8192))
              return;

+     if (get_intid_range(d) == SGI_RANGE) {
+             gic_irq_set_prio(d, GICD_INT_DEF_PRI);
+             return;
+     }

Same here.

Okay.

-Sumit

quoted

+
      /* desc lock should already be held */
      if (gic_irq_in_rdist(d)) {
              u32 idx = gic_get_ppi_index(d);

@@ -1708,6 +1718,7 @@ static int __init gic_init_bases(void __iomem

*dist_base,

      gic_dist_init();
      gic_cpu_init();
+     gic_enable_nmi_support();
      gic_smp_init();
      gic_cpu_pm_init();

@@ -1719,8 +1730,6 @@ static int __init gic_init_bases(void __iomem

*dist_base,
                      gicv2m_init(handle, gic_data.domain);
      }

-     gic_enable_nmi_support();
-
      return 0;

 out_free:

Thanks,

         M.
--
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:45, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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

quoted

      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.

quoted

+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

quoted

+}
+#endif

Thanks,

         M.
--
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, 19 Oct 2020 at 17:50, Marc Zyngier [off-list ref] wrote:

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-20 07:43, Sumit Garg wrote:

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

+{
+     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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, 20 Oct 2020 at 15:38, Marc Zyngier [off-list ref] wrote:

On 2020-10-20 07:43, Sumit Garg wrote:

quoted

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

quoted

+{
+     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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:

On Tue, 20 Oct 2020 at 15:38, Marc Zyngier [off-list ref] wrote:

quoted

On 2020-10-20 07:43, Sumit Garg wrote:

quoted

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

quoted

+{
+     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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-20 13:25, Daniel Thompson wrote:

On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:

[...]

quoted

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Tue, 20 Oct 2020 at 18:02, Marc Zyngier [off-list ref] wrote:

On 2020-10-20 13:25, Daniel Thompson wrote:

quoted

On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:

[...]

quoted

quoted

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-20 12:22, Sumit Garg wrote:

On Tue, 20 Oct 2020 at 15:38, Marc Zyngier [off-list ref] wrote:

quoted

On 2020-10-20 07:43, Sumit Garg wrote:

quoted

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

quoted

+{
+     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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On 2020-10-20 10:13, Sumit Garg wrote:

On Mon, 19 Oct 2020 at 17:50, Marc Zyngier [off-list ref] wrote:

quoted

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, 21 Oct 2020 at 16:02, Marc Zyngier [off-list ref] wrote:

On 2020-10-20 10:13, Sumit Garg wrote:

quoted

On Mon, 19 Oct 2020 at 17:50, Marc Zyngier [off-list ref] wrote:

quoted

On 2020-10-14 12:12, Sumit Garg wrote:

quoted

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 <redacted>
---
 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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Wed, 21 Oct 2020 at 15:57, Marc Zyngier [off-list ref] wrote:

On 2020-10-20 12:22, Sumit Garg wrote:

quoted

On Tue, 20 Oct 2020 at 15:38, Marc Zyngier [off-list ref] wrote:

quoted

On 2020-10-20 07:43, Sumit Garg wrote:

quoted

On Mon, 19 Oct 2020 at 17:07, Marc Zyngier [off-list ref] wrote:

[...]

quoted

quoted

quoted

+{
+     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?

quoted

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...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel