Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 18 Sep 2020 11:21:59 +0100
Message-ID: <87h7rvqta0.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     James Morse <james.morse@arm.com>
Cc:     jonathanh@nvidia.com, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, Sumit Garg <sumit.garg@linaro.org>,
        kernel-team@android.com, Florian Fainelli <f.fainelli@gmail.com>,
        Russell King <linux@arm.linux.org.uk>,
        Jason Cooper <jason@lakedaemon.net>,
        Saravana Kannan <saravanak@google.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Will Deacon <will@kernel.org>,
        Valentin Schneider <valentin.schneider@arm.com>
Subject: Re: [PATCH v3 08/16] irqchip/gic: Configure SGIs as standard interrupts
In-Reply-To: <f3af8930-b61d-4945-475c-b49e326cd24f@arm.com>
References: <20200901144324.1071694-1-maz@kernel.org>
        <20200901144324.1071694-9-maz@kernel.org>
        <f3af8930-b61d-4945-475c-b49e326cd24f@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.