| Message ID | 1602673931-28782-2-git-send-email-sumit.garg@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Add framework to turn an IPI as NMI | expand |
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 <sumit.garg@linaro.org> > --- > 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; > +} > -- Looks good to me. Please feel free to add: Reviewed-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> 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 <sumit.garg@linaro.org> > --- > 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. > +} > + > +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.
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 <sumit.garg@linaro.org> > --- > 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.
On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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 <sumit.garg@linaro.org> > > --- > > 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". > > > +} > > + > > +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 <maz@kernel.org> 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 <sumit.garg@linaro.org> > > --- > > 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 2020-10-20 07:43, Sumit Garg wrote: > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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.
On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote: > > On 2020-10-20 07:43, Sumit Garg wrote: > > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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 Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote: > On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote: > > > > On 2020-10-20 07:43, Sumit Garg wrote: > > > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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.
On Tue, 20 Oct 2020 at 18:02, Marc Zyngier <maz@kernel.org> 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 12:22, Sumit Garg wrote: > On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote: >> >> On 2020-10-20 07:43, Sumit Garg wrote: >> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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.
On Wed, 21 Oct 2020 at 15:57, Marc Zyngier <maz@kernel.org> wrote: > > On 2020-10-20 12:22, Sumit Garg wrote: > > On Tue, 20 Oct 2020 at 15:38, Marc Zyngier <maz@kernel.org> wrote: > >> > >> On 2020-10-20 07:43, Sumit Garg wrote: > >> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier <maz@kernel.org> 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...
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; +}
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 <sumit.garg@linaro.org> --- 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