lkml.org 
[lkml]   [2020]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v5 1/5] arm64: Add framework to turn IPI as NMI
    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...

    \
     
     \ /
      Last update: 2020-10-20 08:45    [W:2.555 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site