Re: [PATCH v3 1/4] arm64: smp: Introduce a new IPI as IPI_CALL_NMI_FUNC

[Sumit Garg]

On Fri, 4 Sep 2020 at 13:30, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On 2020-09-04 06:30, Sumit Garg wrote:
> > On Thu, 3 Sep 2020 at 22:06, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >>
> >> On 2020-09-03 13:05, Sumit Garg wrote:
> >> > Introduce a new inter processor interrupt as IPI_CALL_NMI_FUNC that
> >> > can be invoked to run special handlers in NMI context. One such handler
> >> > example is kgdb_nmicallback() which is invoked in order to round up
> >> > CPUs
> >> > to enter kgdb context.
> >> >
> >> > As currently pseudo NMIs are supported on specific arm64 platforms
> >> > which
> >> > incorporates GICv3 or later version of interrupt controller. In case a
> >> > particular platform doesn't support pseudo NMIs, IPI_CALL_NMI_FUNC will
> >> > act as a normal IPI which can still be used to invoke special handlers.
> >> >
> >> > Signed-off-by: Sumit Garg <sumit.garg@xxxxxxxxxx>
> >> > ---
> >> >  arch/arm64/include/asm/smp.h |  1 +
> >> >  arch/arm64/kernel/smp.c      | 11 +++++++++++
> >> >  2 files changed, 12 insertions(+)
> >> >
> >> > diff --git a/arch/arm64/include/asm/smp.h
> >> > b/arch/arm64/include/asm/smp.h
> >> > index 2e7f529..e85f5d5 100644
> >> > --- a/arch/arm64/include/asm/smp.h
> >> > +++ b/arch/arm64/include/asm/smp.h
> >> > @@ -89,6 +89,7 @@ extern void secondary_entry(void);
> >> >
> >> >  extern void arch_send_call_function_single_ipi(int cpu);
> >> >  extern void arch_send_call_function_ipi_mask(const struct cpumask
> >> > *mask);
> >> > +extern void arch_send_call_nmi_func_ipi_mask(const struct cpumask
> >> > *mask);
> >> >
> >> >  #ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> >> >  extern void arch_send_wakeup_ipi_mask(const struct cpumask *mask);
> >> > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> >> > index b6bde26..1b4c07c 100644
> >> > --- a/arch/arm64/kernel/smp.c
> >> > +++ b/arch/arm64/kernel/smp.c
> >> > @@ -74,6 +74,7 @@ enum ipi_msg_type {
> >> >       IPI_TIMER,
> >> >       IPI_IRQ_WORK,
> >> >       IPI_WAKEUP,
> >> > +     IPI_CALL_NMI_FUNC,
> >> >       NR_IPI
> >> >  };
> >> >
> >> > @@ -793,6 +794,7 @@ static const char *ipi_types[NR_IPI]
> >> > __tracepoint_string = {
> >> >       S(IPI_TIMER, "Timer broadcast interrupts"),
> >> >       S(IPI_IRQ_WORK, "IRQ work interrupts"),
> >> >       S(IPI_WAKEUP, "CPU wake-up interrupts"),
> >> > +     S(IPI_CALL_NMI_FUNC, "NMI function call interrupts"),
> >> >  };
> >> >
> >> >  static void smp_cross_call(const struct cpumask *target, unsigned int
> >> > ipinr);
> >> > @@ -840,6 +842,11 @@ void arch_irq_work_raise(void)
> >> >  }
> >> >  #endif
> >> >
> >> > +void arch_send_call_nmi_func_ipi_mask(const struct cpumask *mask)
> >> > +{
> >> > +     smp_cross_call(mask, IPI_CALL_NMI_FUNC);
> >> > +}
> >> > +
> >> >  static void local_cpu_stop(void)
> >> >  {
> >> >       set_cpu_online(smp_processor_id(), false);
> >> > @@ -932,6 +939,10 @@ static void do_handle_IPI(int ipinr)
> >> >               break;
> >> >  #endif
> >> >
> >> > +     case IPI_CALL_NMI_FUNC:
> >> > +             /* nop, IPI handlers for special features can be added here. */
> >> > +             break;
> >> > +
> >> >       default:
> >> >               pr_crit("CPU%u: Unknown IPI message 0x%x\n", cpu, ipinr);
> >> >               break;
> >>
> >> I'm really not keen on adding more IPIs to the SMP code. One of the
> >> main reasons for using these SGIs as normal IRQs was to make them
> >> "requestable" from non-arch code as if they were standard percpu
> >> interrupts.
> >>
> >> What prevents you from moving that all the way to the kgdb code?
> >>
> >
> > Since we only have one SGI left (SGI7) which this patch reserves for
> > NMI purposes, I am not sure what value add do you see to make it
> > requestable from non-arch code.
>
> We have one *guaranteed* SGI left. Potentially another 8 which we
> could dynamically discover (reading the priority registers should
> be enough to weed out the secure SGIs). And I'd like to keep that
> last interrupt "just in case" we'd need it to workaround something.
>
> > Also, allocating SGI7 entirely to kgdb would not allow us to leverage
> > it for NMI backtrace support via magic sysrq. But with current
> > implementation we should be able to achieve that.
>
> I'd argue that there is no need for this interrupt to be a "well known
> number". Maybe putting this code in the kgdb code is one step too far,
> but keeping out of the arm64 SMP code is IMO the right thing to do.

IIUC, you would prefer to only allocate SGI7 (only one left) if there
is a corresponding user. And I agree that kgdb isn't commonly active
for most users. But I think magic sysrq is enabled for most users and
supporting NMI backtrace would be quite useful while debugging systems
in the field as well.

So if you like, I can create a separate file like
"arch/arm64/kernel/ipi_nmi.c" for this implementation.

> And if NMI backtracing is a thing, then we should probably implement
> that first.

Have a look at IPI_CPU_BACKTRACE implementation for arm [1]. Similar
implementation would be required for arm64 but now with IPI turned as
a pseudo NMI. So I will try to add corresponding support.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/smp.c#n72

>
> > Moreover, irq ids for normal interrupts are assigned via DT/ACPI, how
> > do you envision this for SGIs?
>
> Nothing could be further from the truth. How do you think MSIs work?
> We'd just bolt an allocator for non-arch IPIs.
>

Okay.

-Sumit

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