Messages in this thread | | | Date | Fri, 18 Sep 2020 11:21:59 +0100 | From | Marc Zyngier <> | Subject | 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 <james.morse@arm.com> wrote: > > Hi Marc, > > (CC: +Jon) > > On 01/09/2020 15: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. > > 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:) > > > 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.
| |