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

[Marc Zyngier <maz@kernel.org>]

On 2020-09-15 09:35, Marek Szyprowski wrote:
> Hi Marc,
> 
> On 15.09.2020 10:07, Marc Zyngier wrote:
>> On 2020-09-15 07:48, Marek Szyprowski wrote:
>>>>> Both Exynos 4210 and 4412 use non-zero cpu-offset in GIC node in
>>>>> device-tree: arch/arm/boot/dts/exynos{4210,4412}.dtsi, so I assume
>>>>> that
>>>>> the GIC registers are not banked.
>>>> 
>>>> Annoyingly, it seems to work correctly in QEMU:
>> 
>> [...]
>> 
>>>> Do you happen to know whether the QEMU emulation is trustworthy?
>>> 
>>> I didn't play much with Exynos emulation on QEMU. All I know is that
>>> this patch simply doesn't work on the real hw.
>> 
>> I don't doubt it. The question was more whether we could trust QEMU
>> to be reliable, in which case the issue would be around a kernel
>> configuration problem. Could you stash your kernel config somewhere?
> 
> I just use the vanilla exynos_defconfig for my tests.

Tried that with QEMU, same result. It keeps working. Oh well.

> 
>>> If there is anything to check or test, let me know. I will try to 
>>> help
>>> as much as possible.
>> 
>> It would be interesting to see whether the CPUs are getting any IPI.
>> Can you try the following patch, and send the results back?

[...]

> [    0.145493] smp: Bringing up secondary CPUs ...
> [    0.152740] CPU0 send IPI0 base = f0800000
> [    0.152786] CPU1: Booted secondary processor
> [    0.155582] CPU0 send IPI0 base = f0800000
> [    0.163945] CPU1 IPI0 base = f0808000
> [    0.163956] CPU1 IPI1 base = f0808000
> [    0.163966] CPU1 IPI2 base = f0808000
> [    0.163976] CPU1 IPI3 base = f0808000
> [    0.163986] CPU1 IPI4 base = f0808000
> [    0.163995] CPU1 IPI5 base = f0808000
> [    0.164004] CPU1 IPI6 base = f0808000
> [    0.164014] CPU1 IPI7 base = f0808000
> [    0.164025] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
> [    0.164035] CPU1: Spectre v2: using BPIALL workaround
> [    0.203803] CPU1 send IPI2 base = f0808000
> [    0.207834] CPU1 IPI0 received
> [    0.207839] CPU0 IPI2 received
> [    0.214052] CPU0 send IPI2 base = f0800000
> [    0.217990] CPU1 IPI2 received
> [    0.222188] CPU1 send IPI2 base = f0808000
> [    2.754062] random: fast init done

So IPIs *do work* for some time, but CPU0 ends up not seeing IPI2.
I see a slightly different behaviour in QEMU:

[    0.555590] smp: Bringing up secondary CPUs ...
[    0.606032] CPU0 send IPI0 base = f0800000
[    0.609149] CPU0 send IPI0 base = f0800000
[    0.610329] CPU0 send IPI0 base = f0800000
[    0.611445] CPU0 send IPI0 base = f0800000
[    0.611588] CPU1: Booted secondary processor
[    0.613579] CPU0 send IPI0 base = f0800000
[    0.616180] CPU1 IPI0 base = f0808000
[    0.616470] CPU1 IPI1 base = f0808000
[    0.616634] CPU1 IPI2 base = f0808000
[    0.616781] CPU1 IPI3 base = f0808000
[    0.616931] CPU1 IPI4 base = f0808000
[    0.617074] CPU1 IPI5 base = f0808000
[    0.617220] CPU1 IPI6 base = f0808000
[    0.617366] CPU1 IPI7 base = f0808000
[    0.617824] CPU1: thread -1, cpu 1, socket 9, mpidr 80000901
[    0.618115] CPU1: Spectre v2: using BPIALL workaround
[    0.627969] CPU1 send IPI3 base = f0808000
[    0.631301] CPU0 IPI3 received
[    0.631389] CPU1 IPI0 received
[    0.639726] CPU0 send IPI2 base = f0800000
[    0.641632] CPU1 IPI2 received
[    0.664666] CPU1 send IPI2 base = f0808000
[    0.665987] CPU0 IPI2 received
[    0.670718] smp: Brought up 1 node, 2 CPUs
[    0.672175] SMP: Total of 2 processors activated (48.00 BogoMIPS).
[    0.674071] CPU: All CPU(s) started in SVC mode.

where the secondary starts by sending IPI3 (IPI_CALL_FUNC). Not sure it
matters.

The fact that CPU0 doesn't process the second IPI2 makes me wonder
if there is something flawed in the EOI logic.

Can you try applying this patch, which reverts that particular logic?
If that happens to work, we'll have to investigate what comes out
of the IAR register...

Otherwise, we'll keep reverting bits of the patch until we nail it...

Thanks,

         M.

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4be2b62f816f..6daf2de7233a 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -335,22 +335,31 @@ static void __exception_irq_entry 
gic_handle_irq(struct pt_regs *regs)
  		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
  		irqnr = irqstat & GICC_IAR_INT_ID_MASK;

-		if (unlikely(irqnr >= 1020))
-			break;
-
-		if (static_branch_likely(&supports_deactivate_key))
+		if (likely(irqnr > 15 && irqnr < 1020)) {
+			if (static_branch_likely(&supports_deactivate_key))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
+			isb();
+			handle_domain_irq(gic->domain, irqnr, regs);
+			continue;
+		}
+		if (irqnr < 16) {
  			writel_relaxed(irqstat, cpu_base + GIC_CPU_EOI);
-		isb();
-
-		/*
-		 * Ensure any shared data written by the CPU sending the IPI
-		 * is read after we've read the ACK register on the GIC.
-		 *
-		 * Pairs with the write barrier in gic_ipi_send_mask
-		 */
-		if (irqnr <= 15)
+			if (static_branch_likely(&supports_deactivate_key))
+				writel_relaxed(irqstat, cpu_base + GIC_CPU_DEACTIVATE);
+#ifdef CONFIG_SMP
+			/*
+			 * Ensure any shared data written by the CPU sending
+			 * the IPI is read after we've read the ACK register
+			 * on the GIC.
+			 *
+			 * Pairs with the write barrier in gic_raise_softirq
+			 */
  			smp_rmb();
-		handle_domain_irq(gic->domain, irqnr, regs);
+			handle_IPI(irqnr, regs);
+#endif
+			continue;
+		}
+		break;
  	} while (1);
  }


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