Messages in this thread | | | From | Sumit Garg <> | Date | Thu, 30 Apr 2020 12:50:28 +0530 | Subject | Re: [RFC Patch v1 2/4] irqchip/gic-v3: Add support to handle SGI as pseudo NMI |
| |
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...
| |