Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts

[Marc Zyngier]

On 2020-09-16 17:22, Marc Zyngier wrote:
> On 2020-09-16 16:58, Jon Hunter wrote:
> > On 16/09/2020 16:55, Marc Zyngier wrote:
> > > On 2020-09-16 16:46, Jon Hunter wrote:
> > > > On 16/09/2020 16:10, Marc Zyngier wrote:
> > > > > Hi Jon,
> > > > >
> > > > > +Linus, who is facing a similar issue.
> > > > >
> > > > > On 2020-09-16 15:16, Jon Hunter wrote:
> > > > > > Hi Marc,
> > > > > >
> > > > > > On 14/09/2020 14:06, Marek Szyprowski wrote:
> > > > > > > Hi Marc,
> > > > > > >
> > > > > > > On 01.09.2020 16:43, Marc Zyngier wrote:
> > > > > > > > Change the way we deal with GIC SGIs by turning them into proper
> > > > > > > > IRQs, and calling into the arch code to register the interrupt 
> > > > > > > > range
> > > > > > > > instead of a callback.
> > > > > > > >
> > > > > > > > Reviewed-by: Valentin Schneider <valentin.schneider@xxxxxxx>
> > > > > > > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > > > > > > This patch landed in linux next-20200914 as commit ac063232d4b0
> > > > > > > ("irqchip/gic: Configure SGIs as standard interrupts"). Sadly it
> > > > > > > breaks
> > > > > > > booting of all Samsung Exynos 4210/4412 based boards (dual/quad 
> > > > > > > ARM
> > > > > > > Cortex A9 based). Here are the last lines from the bootlog:
> > > > > >
> > > > > > I am observing the same thing on several Tegra boards (both arm 
> > > > > > and
> > > > > > arm64). Bisect is pointing to this commit. Reverting this alone 
> > > > > > does
> > > > > > not
> > > > > > appear to be enough to fix the issue.
> > > > >
> > > > > Right, I am just massively by the GICv3 spec, and failed to 
> > > > > remember
> > > > > that ye olde GIC exposes the source CPU in AIR *and* wants it back,
> > > > > while
> > > > > newer GICs deal with that transparently.
> > > > >
> > > > > Can you try the patch below and let me know?
> > > >
> > > > Yes will do.
> > > >
> > > > > @@ -365,14 +354,13 @@ static void __exception_irq_entry
> > > > > gic_handle_irq(struct pt_regs *regs)
> > > > >              smp_rmb();
> > > > >
> > > > >              /*
> > > > > -             * Samsung's funky GIC encodes the source CPU in
> > > > > -             * GICC_IAR, leading to the deactivation to fail if
> > > > > -             * not written back as is to GICC_EOI.  Stash the
> > > > > -             * INTID away for gic_eoi_irq() to write back.
> > > > > -             * This only works because we don't nest SGIs...
> > > > > +             * The GIC encodes the source CPU in GICC_IAR,
> > > > > +             * leading to the deactivation to fail if not
> > > > > +             * written back as is to GICC_EOI.  Stash the INTID
> > > > > +             * away for gic_eoi_irq() to write back.  This only
> > > > > +             * works because we don't nest SGIs...
> > > > >               */
> > > > > -            if (is_frankengic())
> > > > > -                set_sgi_intid(irqstat);
> > > > > +            this_cpu_write(sgi_intid, intid);
> > > >
> > > > I assume that it should be irqstat here and not intid?
> > >
> > > Indeed. As you can tell, I haven't even tried to compile it, sorry 
> > > about
> > > that.
> >
> > No worries, I got the gist. However, even with this change, it still
> > does not boot :-(
>
> Do you boot form EL2? If so, you'd also need this:
>
>  static void gic_eoimode1_eoi_irq(struct irq_data *d)
>  {
> +	u32 hwirq = gic_irq(d);
> +
>  	/* Do not deactivate an IRQ forwarded to a vcpu. */
>  	if (irqd_is_forwarded_to_vcpu(d))
>  		return;
>
> +	if (hwirq < 16)
> +		hwirq = this_cpu_read(sgi_intid);
> +
>  	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
>  }

Make it that instead:

 static void gic_eoimode1_eoi_irq(struct irq_data *d)
 {
+	u32 hwirq = gic_irq(d);
+
 	/* Do not deactivate an IRQ forwarded to a vcpu. */
 	if (irqd_is_forwarded_to_vcpu(d))
 		return;

-	writel_relaxed(gic_irq(d), gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
+	if (hwirq < 16)
+		hwirq = this_cpu_read(sgi_intid);
+
+	writel_relaxed(hwirq, gic_cpu_base(d) + GIC_CPU_DEACTIVATE);
 }


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