Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Wed, 21 Oct 2020 at 15:57, Marc Zyngier wrote:
>
> On 2020-10-20 12:22, Sumit Garg wrote:
> > On Tue, 20 Oct 2020 at 15:38, Marc Zyngier wrote:
> >>
> >> On 2020-10-20 07:43, Sumit Garg wrote:
> >> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On 2020-10-20 12:22, Sumit Garg wrote:
On Tue, 20 Oct 2020 at 15:38, Marc Zyngier wrote:
On 2020-10-20 07:43, Sumit Garg wrote:
> On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Tue, 20 Oct 2020 at 18:02, Marc Zyngier 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Tue, Oct 20, 2020 at 04:52:43PM +0530, Sumit Garg wrote:
> On Tue, 20 Oct 2020 at 15:38, Marc Zyngier wrote:
> >
> > On 2020-10-20 07:43, Sumit Garg wrote:
> > > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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.
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Tue, 20 Oct 2020 at 15:38, Marc Zyngier wrote:
>
> On 2020-10-20 07:43, Sumit Garg wrote:
> > On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On 2020-10-20 07:43, Sumit Garg wrote:
On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Mon, 19 Oct 2020 at 17:26, Marc Zyngier 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 > > --- > > 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
On Mon, 19 Oct 2020 at 17:07, Marc Zyngier 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
> > ---
> > 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 000..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
> > +
> > +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 000..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +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 {
> > +
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
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 --- 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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
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
---
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 000..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
+
+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 000..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
+ */
+
+#include
+#include
+#include
+
+#include
+
+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...
Re: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
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
> ---
> 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 000..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
> +
> +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 000..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
> + */
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +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
Thanks!
Masa
