Re: [PATCH v5 4/5] arm64: kgdb: Round up cpus using IPI as NMI

[Sumit Garg]

On Mon, 19 Oct 2020 at 17:45, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On 2020-10-14 12:12, Sumit Garg wrote:
> > arm64 platforms with GICv3 or later supports pseudo NMIs which can be
> > leveraged to round up CPUs which are stuck in hard lockup state with
> > interrupts disabled that wouldn't be possible with a normal IPI.
> >
> > So instead switch to round up CPUs using IPI turned as NMI. And in
> > case a particular arm64 platform doesn't supports pseudo NMIs,
> > this IPI will act as a normal IPI which maintains existing kgdb
> > functionality.
> >
> > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> > ---
> >  arch/arm64/include/asm/kgdb.h |  8 ++++++++
> >  arch/arm64/kernel/ipi_nmi.c   |  5 ++++-
> >  arch/arm64/kernel/kgdb.c      | 21 +++++++++++++++++++++
> >  3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/kgdb.h
> > b/arch/arm64/include/asm/kgdb.h
> > index 21fc85e..6f3d3af 100644
> > --- a/arch/arm64/include/asm/kgdb.h
> > +++ b/arch/arm64/include/asm/kgdb.h
> > @@ -24,6 +24,14 @@ static inline void arch_kgdb_breakpoint(void)
> >  extern void kgdb_handle_bus_error(void);
> >  extern int kgdb_fault_expected;
> >
> > +#ifdef CONFIG_KGDB
> > +extern void ipi_kgdb_nmicallback(int cpu, void *regs);
> > +#else
> > +static inline void ipi_kgdb_nmicallback(int cpu, void *regs)
> > +{
> > +}
> > +#endif
> > +
> >  #endif /* !__ASSEMBLY__ */
> >
> >  /*
> > diff --git a/arch/arm64/kernel/ipi_nmi.c b/arch/arm64/kernel/ipi_nmi.c
> > index a959256..e0a9e03 100644
> > --- a/arch/arm64/kernel/ipi_nmi.c
> > +++ b/arch/arm64/kernel/ipi_nmi.c
> > @@ -8,6 +8,7 @@
> >
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> > +#include <linux/kgdb.h>
> >  #include <linux/smp.h>
> >
> >  #include <asm/nmi.h>
> > @@ -26,7 +27,9 @@ void arch_send_call_nmi_func_ipi_mask(cpumask_t
> > *mask)
> >
> >  static irqreturn_t ipi_nmi_handler(int irq, void *data)
> >  {
> > -     /* nop, NMI handlers for special features can be added here. */
> > +     unsigned int cpu = smp_processor_id();
> > +
> > +     ipi_kgdb_nmicallback(cpu, get_irq_regs());
>
> Please add a return value to ipi_kgdb_nmicallback(), and check it
> before returning IRQ_HANDLED.

Okay.

>
> Thinking a bit more about the whole thing, you should have a way to
> avoid requesting the NMI if there is no user for it (there is nothing
> worse than an enabled interrupt without handlers...).

I think IPIs or SGIs (for arm64) are different from normal interrupts
in this aspect as if there is no handler then we can be sure that
corresponding SGI won't be invoked as their invocation is controlled
via software.

This is the similar case with other IPIs as well whose handlers are
optionally enabled, see:
- IPI_CPU_CRASH_STOP
- IPI_TIMER
- IPI_IRQ_WORK
- IPI_WAKEUP

>
> >
> >       return IRQ_HANDLED;
> >  }
> > diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> > index 1a157ca3..0991275 100644
> > --- a/arch/arm64/kernel/kgdb.c
> > +++ b/arch/arm64/kernel/kgdb.c
> > @@ -17,6 +17,7 @@
> >
> >  #include <asm/debug-monitors.h>
> >  #include <asm/insn.h>
> > +#include <asm/nmi.h>
> >  #include <asm/traps.h>
> >
> >  struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
> > @@ -353,3 +354,23 @@ int kgdb_arch_remove_breakpoint(struct kgdb_bkpt
> > *bpt)
> >       return aarch64_insn_write((void *)bpt->bpt_addr,
> >                       *(u32 *)bpt->saved_instr);
> >  }
> > +
> > +void ipi_kgdb_nmicallback(int cpu, void *regs)
> > +{
> > +     if (atomic_read(&kgdb_active) != -1)
> > +             kgdb_nmicallback(cpu, regs);
> > +}
> > +
> > +#ifdef CONFIG_SMP
>
> There is no such thing as an arm64 UP kernel.
>

Okay, will remove this #ifdef.

> > +void kgdb_roundup_cpus(void)
> > +{
> > +     struct cpumask mask;
> > +
> > +     cpumask_copy(&mask, cpu_online_mask);
> > +     cpumask_clear_cpu(raw_smp_processor_id(), &mask);
> > +     if (cpumask_empty(&mask))
> > +             return;
> > +
> > +     arch_send_call_nmi_func_ipi_mask(&mask);
>
> Surely you can come up with a less convoluted name for this function.
> arm64_send_nmi() would be plenty in my opinion.

Okay, will use it instead.

-Sumit

>
> > +}
> > +#endif
>
> Thanks,
>
>          M.
> --
> Jazz is not dead. It just smells funny...