Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-06-25 19:25, Valentin Schneider wrote:
> On 24/06/20 20:58, Marc Zyngier wrote:
> > Change the way we deal with GICv3 SGIs by turning them into proper
> > IRQs, and calling into the arch code to register the interrupt range
> > instead of a callback.
> >
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  drivers/irqchip/irq-gic-v3.c | 81 
> > +++++++++++++++++++-----------------
> >  1 file changed, 43 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3.c 
> > b/drivers/irqchip/irq-gic-v3.c
> > index 19b294ed48ba..d275e9b9533d 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -36,6 +36,8 @@
> >  #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996	(1ULL << 0)
> >  #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539	(1ULL << 1)
> >
> > +#define GIC_IRQ_TYPE_PARTITION	(GIC_IRQ_TYPE_LPI + 1)
> > +
>
> Nit: this piqued my interest but ended up being just a define shuffle; 
> As a
> member of the git speleologists' guild, I'd be overjoyed with having a
> small notion of that in the changelog.

Fair enough.

> >  struct redist_region {
> >       void __iomem		*redist_base;
> >       phys_addr_t		phys_base;
> > @@ -657,38 +659,14 @@ static asmlinkage void __exception_irq_entry 
> > gic_handle_irq(struct pt_regs *regs
> >       if ((irqnr >= 1020 && irqnr <= 1023))
> >               return;
> >
> > -	/* Treat anything but SGIs in a uniform way */
> > -	if (likely(irqnr > 15)) {
> > -		int err;
> > -
> > -		if (static_branch_likely(&supports_deactivate_key))
> > -			gic_write_eoir(irqnr);
> > -		else
> > -			isb();
> > -
> > -		err = handle_domain_irq(gic_data.domain, irqnr, regs);
> > -		if (err) {
> > -			WARN_ONCE(true, "Unexpected interrupt received!\n");
> > -			gic_deactivate_unhandled(irqnr);
> > -		}
> > -		return;
> > -	}
> > -	if (irqnr < 16) {
> > +	if (static_branch_likely(&supports_deactivate_key))
> >               gic_write_eoir(irqnr);
> > -		if (static_branch_likely(&supports_deactivate_key))
> > -			gic_write_dir(irqnr);
> > -#ifdef CONFIG_SMP
> > -		/*
> > -		 * Unlike GICv2, we don't need an smp_rmb() here.
> > -		 * The control dependency from gic_read_iar to
> > -		 * the ISB in gic_write_eoir is enough to ensure
> > -		 * that any shared data read by handle_IPI will
> > -		 * be read after the ACK.
> > -		 */
>
> Isn't that still relevant?

It is. It is just that there is no really good place to put it.
I may end-up just leaving it where it is.

> Also, while staring at this it dawned on me that IPI's don't need the
> eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll 
> get a
> gic_eoi_irq() just before calling into the irqaction. Dunno how much we
> care about it.

That's interesting. This ISB is a leftover from the loop we had before
the pseudo-NMI code, where we had to make sure the write to EOIR was
ordered with the read from IAR.

Given that we have an exception return right after the interrupt
handling, I *think* we could get rid of it (but that would need
mode checking on broken systems such as TX1...).  I don't think
this is specific to IPIs though.

Thanks,

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