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

Hi James,

On Fri, 18 Sep 2020 10:58:45 +0100,
James Morse [off-list ref] wrote:

Hi Marc,

(CC: +Jon)

On 01/09/2020 15:43, Marc Zyngier wrote:

quoted

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.

Your comment "This only works because we don't nest SGIs..." on this
thread tripped some bad memories from adding the irq-stack. Softirq
causes us to nest irqs, but only once.


(I've messed with the below diff to remove the added stuff:)

quoted

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4ffd62af888f..4be2b62f816f 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c

@@ -335,31 +335,22 @@ 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 (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);
-			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_IPI(irqnr, regs);

If I read this right, previously we would EOI the interrupt before
calling handle_IPI().  Where as now with the version of this series
in your tree, we stuff the to-be-EOId value in a percpu variable,
which is only safe if these don't nest.

Hidden in irq_exit(), kernel/softirq.c::__irq_exit_rcu() has this:
|	preempt_count_sub(HARDIRQ_OFFSET);
|	if (!in_interrupt() && local_softirq_pending())
|		invoke_softirq();

The arch code doesn't raise the preempt counter by HARDIRQ, so once
__irq_exit_rcu() has dropped it, in_interrupt() returns false, and
we invoke_softirq().

invoke_softirq() -> __do_softirq() -> local_irq_enable()!

Fortunately, __do_softirq() raises the softirq count first using
__local_bh_disable_ip(), which in-interrupt() checks too, so this
can only happen once per IRQ.

Now the irq_exit() has moved from handle_IPI(), which ran after EOI,
into handle_domain_irq(), which runs before. I think its possible
SGIs nest, and the new percpu variable becomes corrupted.

I can't see how. The interrupt is active until we EOI/deactivate it,
and thus cannot be observed again by the CPU interface until this
happens.

Furthermore, irq_exit() in __handle_domain_irq() is *after* the EOI
anyway (generic_handle_irq_() directly calls the flow, which
immediately EOIs the interrupt). The only material change is that
irq_enter() happens before EOI. Is that what you are referring to?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel