Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI

[Sumit Garg <sumit.garg@linaro.org>]

Hi Marc,

On Wed, 29 Apr 2020 at 13:53, Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Sumit,
>
> On 2020-04-28 15:11, Sumit Garg wrote:
> > Hi Marc,
> >
> > Thanks for your comments and apologies for my delayed response as I
> > was exploring ideas that you have shared.
> >
> > On Sat, 25 Apr 2020 at 20:02, Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On 2020-04-25 11:29, Marc Zyngier wrote:
> >> > On Fri, 24 Apr 2020 16:39:12 +0530
> >> > Sumit Garg <sumit.garg@linaro.org> wrote:
> >> >
> >> > Hi Sumit,
> >> >
> >> >> With pseudo NMIs enabled, interrupt controller can be configured to
> >> >> deliver SGI as a pseudo NMI. So add corresponding handling for SGIs.
> >> >>
> >> >> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >> >> ---
> >> >>  drivers/irqchip/irq-gic-v3.c | 22 +++++++++++++++++-----
> >> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >> >>
> >> >> diff --git a/drivers/irqchip/irq-gic-v3.c
> >> >> b/drivers/irqchip/irq-gic-v3.c
> >> >> index d7006ef..be361bf 100644
> >> >> --- a/drivers/irqchip/irq-gic-v3.c
> >> >> +++ b/drivers/irqchip/irq-gic-v3.c
> >> >> @@ -609,17 +609,29 @@ static inline void gic_handle_nmi(u32 irqnr,
> >> >> struct pt_regs *regs)
> >> >>      if (irqs_enabled)
> >> >>              nmi_enter();
> >> >>
> >> >> -    if (static_branch_likely(&supports_deactivate_key))
> >> >> -            gic_write_eoir(irqnr);
> >> >>      /*
> >> >>       * Leave the PSR.I bit set to prevent other NMIs to be
> >> >>       * received while handling this one.
> >> >>       * PSR.I will be restored when we ERET to the
> >> >>       * interrupted context.
> >> >>       */
> >> >> -    err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> >> -    if (err)
> >> >> -            gic_deactivate_unhandled(irqnr);
> >> >> +    if (likely(irqnr > 15)) {
> >> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> >> +                    gic_write_eoir(irqnr);
> >> >> +
> >> >> +            err = handle_domain_nmi(gic_data.domain, irqnr, regs);
> >> >> +            if (err)
> >> >> +                    gic_deactivate_unhandled(irqnr);
> >> >> +    } else {
> >> >> +            gic_write_eoir(irqnr);
> >> >> +            if (static_branch_likely(&supports_deactivate_key))
> >> >> +                    gic_write_dir(irqnr);
> >> >> +#ifdef CONFIG_SMP
> >> >> +            handle_IPI(irqnr, regs);
> >> >> +#else
> >> >> +            WARN_ONCE(true, "Unexpected SGI received!\n");
> >> >> +#endif
> >> >> +    }
> >> >>
> >> >>      if (irqs_enabled)
> >> >>              nmi_exit();
> >> >
> >> > If there is one thing I would like to avoid, it is to add more ugly
> >> > hacks to the way we handle SGIs. There is very little reason why SGIs
> >> > should be handled differently from all other interrupts. They have the
> >> > same properties, and it is only because of the 32bit legacy that we
> >> > deal
> >> > with them in such a cumbersome way. Nothing that we cannot fix though.
> >> >
> >> > What I would really like to see is first a conversion of the SGIs to
> >> > normal, full fat interrupts. These interrupts can then be configured as
> >> > NMI using the normal API.
> >> >
> >> > I think Julien had something along these lines (or was that limited to
> >> > the PMU?). Otherwise, I'll happily help you with that.
> >>
> >> OK, to give you an idea of what I am after, here's a small series[1]
> >> that
> >> can be used as a base (it has been booted exactly *once* on a model,
> >> and
> >> is thus absolutely perfect ;-).
> >
> > Thanks for this series. I have re-based my patch-set on top of this
> > series [1] and just dropped this patch #2. It works fine for me.
>
> I just had a look.
>
> "irqchip/gic-v3: Enable arch specific IPI as pseudo NMI" is still done
> the wrong way, I'm afraid. You directly poke into the GIC configuration,
> which isn't acceptable, as you leave the rest of the kernel completely
> unaware that this is a NMI. You should make the interrupt a NMI as it
> is being configured in gic_smp_init(), calling request_nmi() at this
> stage.

Sure, I will try to follow your suggestion. But I think it's better to
first generalize the base IPI allocation scheme.

>
> >>
> >> There is still a bit of work to be able to actually request a SGI
> >> (they
> >> are hard-wired as chained interrupts so far, as this otherwise changes
> >> the output of /proc/interrupts, among other things), but you will
> >> hopefully see what I'm aiming for.
> >
> > I was exploring this idea: "request a SGI". I guess here you meant to
> > request a new SGI as a normal NMI/IRQ via common APIs such as
> > request_percpu_nmi() or request_percpu_irq() rather than statically
> > adding a new IPI as per this patch [2], correct? If yes, then I have
> > following follow up queries:
> >
> > 1. Do you envision any drivers to use SGIs in a similar manner as they
> > use SPIs or PPIs?
>
> No. SGIs are already pretty much all allocated for the kernel's internal
> needs and once we allocate an additional one for this KGDB feature,
> we're out of non-secure SGIs. We could start a multiplexing scheme to
> overcome this, but the kernel already has plenty of other mechanisms
> for internal communication. After all, why would you need anything more
> than smp_call_function()?
>
> The single use case I can imagine is that you'd want to signal a CPU
> that isn't running Linux. This would require a lot more work than
> just an interrupt, and is out of scope for the time being.

Thanks for the clarification.

>
> > 2. How do you envision allocation of SGIs as currently they are
> > hardcoded in an arch specific file (like arch/arm64/kernel/smp.c
> > +794)?
>
> What I would like is for the arch code to request these interrupts as
> normal interrupts, for which there is one problem to solve: how do you
> find out about the Linux IRQ number for a given IPI. Or rather, how
> do you get rid of the requirement to have IPI numbers at all and just
> say "give me a per-cpu interrupt that I can use as an IPI, and by the
> way here's the handler you can call".

I think what you are looking for here is something that could be
sufficed via enabling "CONFIG_GENERIC_IRQ_IPI" framework for arm64/arm
architectures. It's currently used for mips architecture. Looking at
its implementation, I think it should be possible to hook up SGIs
under new IPI irq_domain for GICv2/v3.

So with this framework we should be able to dynamically allocate IPIs
and use common APIs such as request_irq()/request_nmi() to tell IPI
specific handlers.

If this approach looks sane to you then I can start working on a PoC
implementation for arm64.

>
> And I insist: this is only for the arch code. Nothing else.
>

Makes sense.

> > 3. AFAIK, the major difference among SGIs and SPIs or PPIs is the
> > trigger method where SGIs are software triggered and SPIs or PPIs are
> > hardware triggered. And I couldn't find a generalized method across
> > architectures to invoke SGIs. So how do you envision drivers to invoke
> > SGIs in an architecture agnostic manner?
>
> Well, SGIs are not architecture agnostic. They are fundamentally part of
> the GIC architecture, which only exists for the two ARM architectures.
>
> SGIs are not a general purpose mechanism. IPIs are, and we have services
> on top of IPIs, such as invoking a function on a remote CPU. What else
> do you need?

Yeah that was mine understanding as well. But I was just clarifying if
you have any further use-cases in mind for IPIs.

-Sunit

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