Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts

From: Valentin Schneider <valentin.schneider@arm.com>
To: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Russell King <linux@arm.linux.org.uk>,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>,
	Sumit Garg <sumit.garg@linaro.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Andrew Lunn <andrew@lunn.ch>,
	kernel-team@android.com
Subject: Re: [PATCH v2 06/17] irqchip/gic-v3: Configure SGIs as standard interrupts
Date: Thu, 02 Jul 2020 15:24:30 +0100	[thread overview]
Message-ID: <jhjlfk28069.mognet@arm.com> (raw)
In-Reply-To: <1f3ac757acd15f6804917e222194c9cf@kernel.org>


On 02/07/20 14:48, Marc Zyngier wrote:
> On 2020-07-02 14:23, Valentin Schneider wrote:
>> On 30/06/20 11:15, Marc Zyngier wrote:
>>> On 2020-06-25 19:25, Valentin Schneider wrote:
>>>> Also, while staring at this it dawned on me that IPI's don't need the
>>>> eoimode=0 isb(): due to how the IPI flow-handler is structured, we'll
>>>> get a
>>>> gic_eoi_irq() just before calling into the irqaction. Dunno how much
>>>> we
>>>> care about it.
>>>
>>> That's interesting. This ISB is a leftover from the loop we had before
>>> the pseudo-NMI code, where we had to make sure the write to EOIR was
>>> ordered with the read from IAR.
>>>
>>> Given that we have an exception return right after the interrupt
>>> handling, I *think* we could get rid of it (but that would need
>>> mode checking on broken systems such as TX1...).  I don't think
>>> this is specific to IPIs though.
>>>
>>
>> If I got this one right:
>>
>>   39a06b67c2c1 ("irqchip/gic: Ensure we have an ISB between ack and
>> ->handle_irq")
>>
>> you're describing case 2, which is indeed gone on gic-v3. However IIUC
>> we
>> also want an ISB between poking IAR and calling into the irqaction
>> (case 1)
>> - we get just that with IPIs due to the early gic_eoi_irq(), but we
>> don't
>> for the other flows.
>
> You just made me realise something amazing: I've started to forget about
> all this crap. Which is wonderful! ;-)
>

:)

> More seriously, you are absolutely right. If we wanted to address this,
> we'd probably have to give IPIs their own irqchip so that they get their
> own eoi callback. Not sure that's worth it.
>

I was initially thinking of something like

        if (static_branch_likely(&supports_deactivate_key))
                gic_write_eoir(irqnr);
        else if (__get_intid_range(irqnr) != SGI_RANGE)
                isb();

which is not particularly pretty, so maybe we should just slap this to the
isb():

                /* Superfluous for SGIs, but who cares */

>          M.