Message-ID: <CAFA6WYPNNNZeX5zpadayfiZ7P_mHmiREpUd5LZ3Jp+TjGVqoEw@mail.gmail.com>
Date:   Tue, 28 Apr 2020 19:41:50 +0530
From:   Sumit Garg <sumit.garg@...aro.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Daniel Thompson <daniel.thompson@...aro.org>,
        Jason Cooper <jason@...edaemon.net>,
        Catalin Marinas <catalin.marinas@....com>,
        Douglas Anderson <dianders@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        Thomas Gleixner <tglx@...utronix.de>,
        Will Deacon <will@...nel.org>, julien.thierry.kdev@...il.com
Subject: Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as
 pseudo NMI

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@...nel.org> wrote:
>
> On 2020-04-25 11:29, Marc Zyngier wrote:
> > On Fri, 24 Apr 2020 16:39:12 +0530
> > Sumit Garg <sumit.garg@...aro.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@...aro.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.

>
> 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?
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)?
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?

[1] https://git.linaro.org/people/sumit.garg/linux.git/?h=kgdb-nmi
[2] https://git.linaro.org/people/sumit.garg/linux.git/commit/?h=kgdb-nmi&id=fc89e5f395f89966110554a15ab0fa0f0d471132

-Sumit

>
> Thanks,
>
>          M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/gic-sgi
> --
> Jazz is not dead. It just smells funny...

Powered by blists - more mailing lists