Message-ID: <df54df32-088a-c707-9ffd-e099878548bc@marcan.st>
Date:   Mon, 8 Feb 2021 00:38:44 +0900
From:   Hector Martin 'marcan' <marcan@...can.st>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Marc Zyngier <maz@...nel.org>, SoC Team <soc@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        Olof Johansson <olof@...om.net>
Subject: Re: [PATCH 10/18] arm64: Introduce FIQ support

On 07/02/2021 21.25, Arnd Bergmann wrote:
> On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' <marcan@...can.st> wrote:
>> On 07/02/2021 01.22, Arnd Bergmann wrote:
>>> * In the fiq handler code, check if normal interrupts were enabled
>>>     when the fiq hit. Normally they are enabled, so just proceed to
>>>     handle the timer and ipi directly
>>>
>>> * if irq was disabled, defer the handling by doing a self-ipi
>>>     through the aic's ipi method, and handle it from there
>>>     when dealing with the next interrupt once interrupts get
>>>     enabled.
>>>
>>> This would be similar to the soft-disable feature on powerpc, which
>>> never actually turns off interrupts from regular kernel code but
>>> just checks a flag in local_irq_enable that gets set when a
>>> hardirq happened.
>>
>> Case #2 seems messy. In AIC, we'd have to either:
>>
>> * Disable FIQs, and hope that doesn't confuse any save/restore code
>> going on, then set a flag and check it in *both* the IRQ and FIQ path
>> since either might trigger depending on what happens next, or
>> * Mask the relevant timer, which we'd then need to make sure does not
>> confuse the timer code (Unmask it again when we fire the interrupt? But
>> what if the timer code intended to mask it in the interim?)
> 
> I'm not quite following here. The IRQ should be disabled the entire time
> while handling that self-IPI and the timer top half code, so if we get
> another FIQ while handling the timer from the IRQ path, it will lead
> either yet another self-IPI or it will be ignored in case the previous timer
> event has not been Acked yet. I would expect that both cases are
> race-free here, the only time that the FIQ needs to be disabled is
> while actually handling the FIQ. Did I miss something?

FIQs are level-triggered, and there are only two* ways of masking them 
(that we know of): in the timer, or DAIF. That means that if we get a 
FIQ, we *must* do one of two things: either mask it in the timer 
register, or mask FIQs entirely. If we do neither of these, we get a FIQ 
storm.

So if a timer FIQ fires while IRQs are disabled, and we can't call into 
the timer code (because IRQs were disabled, so we need to defer handling 
via the IPI), the only other options are to either poke the timer mask 
bit directly, or to mask FIQs. Neither seems particularly correct.

* An exception seems to be non-HV timer interrupts firing while we have 
a VM guest running (HCR_EL2.TGE=0). This causes a single FIQ, and no 
more, which suggests there is a mask bit for guest timer FIQs somewhere 
that gets automatically set when the FIQ is delivered to the CPU core. 
I've yet to find where this bit lives, I'll be doing a brute force sweep 
of system register space soon to see if I can find it, and if there is 
anything else useful near it.

>> Plus I don't know if the vector entry code and other scaffolding between
>> the vector and the AIC driver would be happy with, effectively,
>> recursive interrupts. This could work with a carefully controlled path
>> to make sure it doesn't break things, but I'm not so sure about the
>> current "just point FIQ and IRQ to the same place" approach here.
> 
> If we do what I described above, the FIQ and IRQ entry would have
> to be separate and only arrive in the same code path when calling
> into drivers/clocksource/arm_arch_timer.c. It's not recursive there
> because that part is only called when IRQ is disabled, and no IRQ
> is being executed while the FIQ hits.

Right, that's what i'm saying; we can't re-use the IRQ handler like Marc 
proposed, because I don't think that expects to be called reentrantly; 
we'd have to have a separate FIQ entry, but since it can be called with 
IRQs enabled and handle the FIQ in-line, it also needs to be able to 
*conditionally* behave like a normal IRQ handler. This level of 
complexity seems somewhat dubious, just to not maintain the FIQ mask bit 
synced. That's not just AIC code any more, it needs a bespoke FIQ vector 
and logic to decide whether IRQs are masked (call AIC to self-IPI 
without doing the usual IRQ processing) or unmasked (go through regular 
IRQ accounting and behave like an IRQ).

Perhaps I'm misunderstanding what you're proposing here or how this 
would work :)

-- 
Hector Martin "marcan" (marcan@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists