| Message ID | 20210204203951.52105-11-marcan@marcan.st (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
On Thu, 04 Feb 2021 20:39:43 +0000, Hector Martin <marcan@marcan.st> wrote: > > Apple SoCs (A11 and newer) have some interrupt sources hardwired to the > FIQ line. Implement support for this by simply treating IRQs and FIQs > the same way in the interrupt vectors. This is conditional on the > ARM64_NEEDS_FIQ CPU feature flag, and thus will not affect other > systems. > > Root irqchip drivers can discriminate between IRQs and FIQs by checking > the ISR_EL1 system register. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > arch/arm64/include/asm/assembler.h | 4 ++++ > arch/arm64/include/asm/daifflags.h | 7 +++++++ > arch/arm64/include/asm/irqflags.h | 17 +++++++++++++---- > arch/arm64/kernel/entry.S | 27 +++++++++++++++++++++++---- > 4 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index bf125c591116..6acfc372dc76 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -42,7 +42,11 @@ > > /* IRQ is the lowest priority flag, unconditionally unmask the rest. */ > .macro enable_da_f > +alternative_if ARM64_NEEDS_FIQ > + msr daifclr, #(8 | 4) > +alternative_else > msr daifclr, #(8 | 4 | 1) > +alternative_endif See my digression in patch 8. I really wonder what the benefit is to treat FIQ independently of IRQ, and we might as well generalise this. We could always panic on getting a FIQ on platforms that don't expect one. It'd be good to rope in the other interested parties (Mark for the early entry code, James for RAS and SError handling). > .endm > > /* > diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h > index 1c26d7baa67f..228a6039c701 100644 > --- a/arch/arm64/include/asm/daifflags.h > +++ b/arch/arm64/include/asm/daifflags.h > @@ -112,6 +112,13 @@ static inline void local_daif_restore(unsigned long flags) > * So we don't need additional synchronization here. > */ > gic_write_pmr(pmr); > + } else if (system_uses_fiqs()) { > + /* > + * On systems that use FIQs, disable FIQs if IRQs are disabled. > + * This can happen if the DAIF_* flags at the top of this file > + * are used to set DAIF directly. > + */ > + flags |= PSR_F_BIT; > } > > write_sysreg(flags, daif); > diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h > index ff328e5bbb75..689c573c4b47 100644 > --- a/arch/arm64/include/asm/irqflags.h > +++ b/arch/arm64/include/asm/irqflags.h > @@ -19,8 +19,9 @@ > * side effects for other flags. Keeping to this order makes it easier for > * entry.S to know which exceptions should be unmasked. > * > - * FIQ is never expected, but we mask it when we disable debug exceptions, and > - * unmask it at all other times. > + * FIQ is never expected on most platforms, but we mask it when we disable > + * debug exceptions, and unmask it at all other times. On platforms that > + * require FIQs, it tracks IRQ masking. > */ > > /* > @@ -34,8 +35,14 @@ static inline void arch_local_irq_enable(void) > WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); > } > > - asm volatile(ALTERNATIVE( > + /* > + * Yes, ALTERNATIVE() nests properly... only one of these should be > + * active on any given platform. > + */ > + asm volatile(ALTERNATIVE(ALTERNATIVE( > "msr daifclr, #2 // arch_local_irq_enable", > + "msr daifclr, #3 // arch_local_irq_enable", > + ARM64_NEEDS_FIQ), Err... no. Please. It may be a cool hack, but that's an unmaintainable one in the long run. If you *really* have to have a special case here, consider using a callback instead, and generate the right instruction directly. > __msr_s(SYS_ICC_PMR_EL1, "%0"), > ARM64_HAS_IRQ_PRIO_MASKING) > : > @@ -53,8 +60,10 @@ static inline void arch_local_irq_disable(void) > WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); > } > > - asm volatile(ALTERNATIVE( > + asm volatile(ALTERNATIVE(ALTERNATIVE( > "msr daifset, #2 // arch_local_irq_disable", > + "msr daifset, #3 // arch_local_irq_disable", > + ARM64_NEEDS_FIQ), > __msr_s(SYS_ICC_PMR_EL1, "%0"), > ARM64_HAS_IRQ_PRIO_MASKING) > : > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index c9bae73f2621..81ca04ebe37b 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -60,7 +60,7 @@ > #define BAD_FIQ 2 > #define BAD_ERROR 3 > > - .macro kernel_ventry, el, label, regsize = 64 > + .macro kernel_ventry, el, label, regsize = 64, altlabel = 0, alt = 0 > .align 7 > #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > .if \el == 0 > @@ -87,7 +87,15 @@ alternative_else_nop_endif > tbnz x0, #THREAD_SHIFT, 0f > sub x0, sp, x0 // x0'' = sp' - x0' = (sp + x0) - sp = x0 > sub sp, sp, x0 // sp'' = sp' - x0 = (sp + x0) - x0 = sp > + .if \altlabel != 0 > + alternative_if \alt > + b el\()\el\()_\altlabel > + alternative_else > b el\()\el\()_\label > + alternative_endif > + .else > + b el\()\el\()_\label > + .endif > > 0: > /* > @@ -119,7 +127,15 @@ alternative_else_nop_endif > sub sp, sp, x0 > mrs x0, tpidrro_el0 > #endif > + .if \altlabel != 0 > + alternative_if \alt > + b el\()\el\()_\altlabel > + alternative_else > b el\()\el\()_\label > + alternative_endif > + .else > + b el\()\el\()_\label > + .endif > .endm > > .macro tramp_alias, dst, sym > @@ -547,18 +563,21 @@ SYM_CODE_START(vectors) > > kernel_ventry 1, sync // Synchronous EL1h > kernel_ventry 1, irq // IRQ EL1h > - kernel_ventry 1, fiq_invalid // FIQ EL1h > + // FIQ EL1h > + kernel_ventry 1, fiq_invalid, 64, irq, ARM64_NEEDS_FIQ It could be better to create a set of first class FIQ handlers rather than this alternative target macro. I quickly hacked this instead, which I find more readable. Thanks, M. diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index a8c3e7aaca74..dc65b56626ab 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -547,18 +547,18 @@ SYM_CODE_START(vectors) kernel_ventry 1, sync // Synchronous EL1h kernel_ventry 1, irq // IRQ EL1h - kernel_ventry 1, fiq_invalid // FIQ EL1h + kernel_ventry 1, fiq // FIQ EL1h kernel_ventry 1, error // Error EL1h kernel_ventry 0, sync // Synchronous 64-bit EL0 kernel_ventry 0, irq // IRQ 64-bit EL0 - kernel_ventry 0, fiq_invalid // FIQ 64-bit EL0 + kernel_ventry 0, fiq // FIQ 64-bit EL0 kernel_ventry 0, error // Error 64-bit EL0 #ifdef CONFIG_COMPAT kernel_ventry 0, sync_compat, 32 // Synchronous 32-bit EL0 kernel_ventry 0, irq_compat, 32 // IRQ 32-bit EL0 - kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0 + kernel_ventry 0, fiq, 32 // FIQ 32-bit EL0 kernel_ventry 0, error_compat, 32 // Error 32-bit EL0 #else kernel_ventry 0, sync_invalid, 32 // Synchronous 32-bit EL0 @@ -658,6 +658,10 @@ SYM_CODE_START_LOCAL_NOALIGN(el1_sync) SYM_CODE_END(el1_sync) .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el1_fiq) +alternative_if_not ARM64_NEEDS_FIQ + b el1_fiq_invalid +alternative_else_nop_endif SYM_CODE_START_LOCAL_NOALIGN(el1_irq) kernel_entry 1 gic_prio_irq_setup pmr=x20, tmp=x1 @@ -688,6 +692,7 @@ alternative_else_nop_endif kernel_exit 1 SYM_CODE_END(el1_irq) +SYM_CODE_END(el1_fiq) /* * EL0 mode handlers. @@ -710,10 +715,15 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_sync_compat) SYM_CODE_END(el0_sync_compat) .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat) +alternative_if_not ARM64_NEEDS_FIQ + b el0_fiq_invalid +alternative_else_nop_endif SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat) kernel_entry 0, 32 b el0_irq_naked SYM_CODE_END(el0_irq_compat) +SYM_CODE_END(el0_fiq_compat) SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat) kernel_entry 0, 32 @@ -722,6 +732,10 @@ SYM_CODE_END(el0_error_compat) #endif .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq) +alternative_if_not ARM64_NEEDS_FIQ + b el0_fiq_invalid +alternative_else_nop_endif SYM_CODE_START_LOCAL_NOALIGN(el0_irq) kernel_entry 0 el0_irq_naked: @@ -736,6 +750,7 @@ el0_irq_naked: b ret_to_user SYM_CODE_END(el0_irq) +SYM_CODE_END(el0_fiq) SYM_CODE_START_LOCAL(el1_error) kernel_entry 1
On Sat, Feb 6, 2021 at 4:37 PM Marc Zyngier <maz@kernel.org> wrote: > On Thu, 04 Feb 2021 20:39:43 +0000, Hector Martin <marcan@marcan.st> wrote: > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > > index bf125c591116..6acfc372dc76 100644 > > --- a/arch/arm64/include/asm/assembler.h > > +++ b/arch/arm64/include/asm/assembler.h > > @@ -42,7 +42,11 @@ > > > > /* IRQ is the lowest priority flag, unconditionally unmask the rest. */ > > .macro enable_da_f > > +alternative_if ARM64_NEEDS_FIQ > > + msr daifclr, #(8 | 4) > > +alternative_else > > msr daifclr, #(8 | 4 | 1) > > +alternative_endif > > See my digression in patch 8. I really wonder what the benefit is to > treat FIQ independently of IRQ, and we might as well generalise > this. We could always panic on getting a FIQ on platforms that don't > expect one. > > It'd be good to rope in the other interested parties (Mark for the > early entry code, James for RAS and SError handling). There might be another slightly hacky but less intrusive option that could be done (almost) purely within the aic code: * 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. Arnd
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?) Neither sounds particularly clean to me... if we had FIQ status masking registers this would be more reasonable, but I'm not sure I'd want the AIC driver to mess with neither DAIF nor the timer registers. It's bad enough that it has to read the latter already (and I hope I can find a better way of doing that...). 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.
On 07/02/2021 00.37, Marc Zyngier wrote: > See my digression in patch 8. I really wonder what the benefit is to > treat FIQ independently of IRQ, and we might as well generalise > this. We could always panic on getting a FIQ on platforms that don't > expect one. > > It'd be good to rope in the other interested parties (Mark for the > early entry code, James for RAS and SError handling). CCing Mark and James: TL;DR what do you think about unconditionally keeping DAIF.I == DAIF.F, would this break other platforms with spurious FIQs or conversely mask FIQs when we don't want to in some cases? The FIQ vector would remain a panic except on platforms that require using it, via an alternatives patch. >> kernel_ventry 1, sync // Synchronous EL1h >> kernel_ventry 1, irq // IRQ EL1h >> - kernel_ventry 1, fiq_invalid // FIQ EL1h >> + // FIQ EL1h >> + kernel_ventry 1, fiq_invalid, 64, irq, ARM64_NEEDS_FIQ > > It could be better to create a set of first class FIQ handlers rather > than this alternative target macro. I quickly hacked this instead, > which I find more readable. I think I ended up with the macro change to keep it 1:1 with IRQ, vs a separate branch... but I didn't think of the fallthrough-with-nop trick, neat. It is definitely is more readable. Are you OK with me pulling this patch in for v2, with your name on it? > - kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0 > + kernel_ventry 0, fiq, 32 // FIQ 32-bit EL0 fiq_compat here, right?
On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' <marcan@marcan.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? > 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. Arnd
On 07/02/2021 21.25, Arnd Bergmann wrote: > On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' <marcan@marcan.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 :)
On Sun, Feb 7, 2021 at 4:40 PM Hector Martin 'marcan' <marcan@marcan.st> wrote: > On 07/02/2021 21.25, Arnd Bergmann wrote: > > On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' <marcan@marcan.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. Ok, I had not realized the timer was level triggered. In case of the timer, I suppose it could be either masked or acknowledged from the fiq top-half handler when deferring to irq, but I agree that it means a layering violation in either case. What might still work is an approach where FIQ is normally enabled, and local_irq_disable() leaves it on, while local_irq_enable() turns it on regardless of the current state. In this case, the fiq handler could run the timer function if interrupts are enabled but just turn off fiqs when they are turned off, waiting for the next local_irq_enable() to get us back in the state where the handler can run. Not sure if that would buy us anything though, or if that still requires platform specific conditionals in common code. > * 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. Right. Maybe you can even find a bit that switches between FIQ and IRQ mode for the timer, as that would solve the problem completely. I think it's not that rare for irqchips to be configurable to either route an interrupt one way or the other. > >> 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 :) The way I had imagined it was to have a parallel set_handle_irq() and set_handle_fiq() in the aic driver, which end up using the same logic in the entry code to call into the driver. The code leading up to that is all in assembler but isn't all that complex in the end, and is already abstracted with macros to a large degree. For existing machines that don't call set_handle_fiq() it could just end up in either panic() or in WARN_ONCE() if an FIQ does happen unexpectedly. The aic_handle_fiq() function itself would be straightforward, doing not much more than if (interrupts_enabled(ptregs)) /* safe to call timer interrupt here, as interrupts are on */ handle_domain_irq(aic->domain, AIC_TIMER_IRQ, regs); else /* need to defer until interrupts get re-enabled */ aic_send_ipi(smp_processor_id(), TIMER_SELF_IPI); Anyway, it's probably not worth pursuing this further if the timer interrupt is level-triggered, as you explained above. Arnd
On Sun, 07 Feb 2021 08:47:23 +0000, Hector Martin 'marcan' <marcan@marcan.st> wrote: > > On 07/02/2021 00.37, Marc Zyngier wrote: > > See my digression in patch 8. I really wonder what the benefit is to > > treat FIQ independently of IRQ, and we might as well generalise > > this. We could always panic on getting a FIQ on platforms that don't > > expect one. > > > > It'd be good to rope in the other interested parties (Mark for the > > early entry code, James for RAS and SError handling). > > CCing Mark and James: TL;DR what do you think about unconditionally > keeping DAIF.I == DAIF.F, would this break other platforms with > spurious FIQs or conversely mask FIQs when we don't want to in some > cases? The FIQ vector would remain a panic except on platforms that > require using it, via an alternatives patch. > > >> kernel_ventry 1, sync // Synchronous EL1h > >> kernel_ventry 1, irq // IRQ EL1h > >> - kernel_ventry 1, fiq_invalid // FIQ EL1h > >> + // FIQ EL1h > >> + kernel_ventry 1, fiq_invalid, 64, irq, ARM64_NEEDS_FIQ > > > > It could be better to create a set of first class FIQ handlers rather > > than this alternative target macro. I quickly hacked this instead, > > which I find more readable. > > I think I ended up with the macro change to keep it 1:1 with IRQ, vs a > separate branch... but I didn't think of the fallthrough-with-nop > trick, neat. It is definitely is more readable. Are you OK with me > pulling this patch in for v2, with your name on it? Up to you, I don't mind either way. This is just code! :D > > > - kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0 > > + kernel_ventry 0, fiq, 32 // FIQ 32-bit EL0 > > fiq_compat here, right? Of course. Thanks, M.
On 08/02/2021 03.49, Arnd Bergmann wrote: > Ok, I had not realized the timer was level triggered. In case of the > timer, I suppose it could be either masked or acknowledged from the > fiq top-half handler when deferring to irq, but I agree that it means a > layering violation in either case. > > What might still work is an approach where FIQ is normally enabled, > and local_irq_disable() leaves it on, while local_irq_enable() turns > it on regardless of the current state. > > In this case, the fiq handler could run the timer function if interrupts > are enabled but just turn off fiqs when they are turned off, waiting > for the next local_irq_enable() to get us back in the state where > the handler can run. Not sure if that would buy us anything though, > or if that still requires platform specific conditionals in common code. It looks like Marc is just leaning towards making the IRQ and FIQ masks track each other unconditionally on all platforms anyway, so I'm going to try that for v2 (which is certainly the simpler solution). If this ends up somehow breaking any other platform we can deal with it in the way that makes most sense, once we know how it breaks :) >> * 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. > > Right. Maybe you can even find a bit that switches between FIQ and > IRQ mode for the timer, as that would solve the problem completely. > I think it's not that rare for irqchips to be configurable to either route > an interrupt one way or the other. That seems increasingly unlikely here; I tried poking all the AIC config bits and nothing switched those to FIQ (which is the converse). It looks like Apple has done something like use FIQ for all core-internal interrupt sources, and IRQ for AIC, and this is all seemingly quite hardwired. In particular, a subtlety I discovered about how flipping TGE to 1 with a guest timer interrupt pending only takes effect properly (i.e. FIQ fires, and you get a FIQ storm if unhandled, no auto-masking) after subsequently issuing an isb, makes me think all this FIQ stuff is seriously deeply tied into the instruction pipeline. It's probably not an IRQ line any more...
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index bf125c591116..6acfc372dc76 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -42,7 +42,11 @@ /* IRQ is the lowest priority flag, unconditionally unmask the rest. */ .macro enable_da_f +alternative_if ARM64_NEEDS_FIQ + msr daifclr, #(8 | 4) +alternative_else msr daifclr, #(8 | 4 | 1) +alternative_endif .endm /* diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index 1c26d7baa67f..228a6039c701 100644 --- a/arch/arm64/include/asm/daifflags.h +++ b/arch/arm64/include/asm/daifflags.h @@ -112,6 +112,13 @@ static inline void local_daif_restore(unsigned long flags) * So we don't need additional synchronization here. */ gic_write_pmr(pmr); + } else if (system_uses_fiqs()) { + /* + * On systems that use FIQs, disable FIQs if IRQs are disabled. + * This can happen if the DAIF_* flags at the top of this file + * are used to set DAIF directly. + */ + flags |= PSR_F_BIT; } write_sysreg(flags, daif); diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index ff328e5bbb75..689c573c4b47 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -19,8 +19,9 @@ * side effects for other flags. Keeping to this order makes it easier for * entry.S to know which exceptions should be unmasked. * - * FIQ is never expected, but we mask it when we disable debug exceptions, and - * unmask it at all other times. + * FIQ is never expected on most platforms, but we mask it when we disable + * debug exceptions, and unmask it at all other times. On platforms that + * require FIQs, it tracks IRQ masking. */ /* @@ -34,8 +35,14 @@ static inline void arch_local_irq_enable(void) WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); } - asm volatile(ALTERNATIVE( + /* + * Yes, ALTERNATIVE() nests properly... only one of these should be + * active on any given platform. + */ + asm volatile(ALTERNATIVE(ALTERNATIVE( "msr daifclr, #2 // arch_local_irq_enable", + "msr daifclr, #3 // arch_local_irq_enable", + ARM64_NEEDS_FIQ), __msr_s(SYS_ICC_PMR_EL1, "%0"), ARM64_HAS_IRQ_PRIO_MASKING) : @@ -53,8 +60,10 @@ static inline void arch_local_irq_disable(void) WARN_ON_ONCE(pmr != GIC_PRIO_IRQON && pmr != GIC_PRIO_IRQOFF); } - asm volatile(ALTERNATIVE( + asm volatile(ALTERNATIVE(ALTERNATIVE( "msr daifset, #2 // arch_local_irq_disable", + "msr daifset, #3 // arch_local_irq_disable", + ARM64_NEEDS_FIQ), __msr_s(SYS_ICC_PMR_EL1, "%0"), ARM64_HAS_IRQ_PRIO_MASKING) : diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index c9bae73f2621..81ca04ebe37b 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -60,7 +60,7 @@ #define BAD_FIQ 2 #define BAD_ERROR 3 - .macro kernel_ventry, el, label, regsize = 64 + .macro kernel_ventry, el, label, regsize = 64, altlabel = 0, alt = 0 .align 7 #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 .if \el == 0 @@ -87,7 +87,15 @@ alternative_else_nop_endif tbnz x0, #THREAD_SHIFT, 0f sub x0, sp, x0 // x0'' = sp' - x0' = (sp + x0) - sp = x0 sub sp, sp, x0 // sp'' = sp' - x0 = (sp + x0) - x0 = sp + .if \altlabel != 0 + alternative_if \alt + b el\()\el\()_\altlabel + alternative_else b el\()\el\()_\label + alternative_endif + .else + b el\()\el\()_\label + .endif 0: /* @@ -119,7 +127,15 @@ alternative_else_nop_endif sub sp, sp, x0 mrs x0, tpidrro_el0 #endif + .if \altlabel != 0 + alternative_if \alt + b el\()\el\()_\altlabel + alternative_else b el\()\el\()_\label + alternative_endif + .else + b el\()\el\()_\label + .endif .endm .macro tramp_alias, dst, sym @@ -547,18 +563,21 @@ SYM_CODE_START(vectors) kernel_ventry 1, sync // Synchronous EL1h kernel_ventry 1, irq // IRQ EL1h - kernel_ventry 1, fiq_invalid // FIQ EL1h + // FIQ EL1h + kernel_ventry 1, fiq_invalid, 64, irq, ARM64_NEEDS_FIQ kernel_ventry 1, error // Error EL1h kernel_ventry 0, sync // Synchronous 64-bit EL0 kernel_ventry 0, irq // IRQ 64-bit EL0 - kernel_ventry 0, fiq_invalid // FIQ 64-bit EL0 + // FIQ 64-bit EL0 + kernel_ventry 0, fiq_invalid, 64, irq, ARM64_NEEDS_FIQ kernel_ventry 0, error // Error 64-bit EL0 #ifdef CONFIG_COMPAT kernel_ventry 0, sync_compat, 32 // Synchronous 32-bit EL0 kernel_ventry 0, irq_compat, 32 // IRQ 32-bit EL0 - kernel_ventry 0, fiq_invalid_compat, 32 // FIQ 32-bit EL0 + // FIQ 32-bit EL0 + kernel_ventry 0, fiq_invalid_compat, 32, irq_compat, ARM64_NEEDS_FIQ kernel_ventry 0, error_compat, 32 // Error 32-bit EL0 #else kernel_ventry 0, sync_invalid, 32 // Synchronous 32-bit EL0
Apple SoCs (A11 and newer) have some interrupt sources hardwired to the FIQ line. Implement support for this by simply treating IRQs and FIQs the same way in the interrupt vectors. This is conditional on the ARM64_NEEDS_FIQ CPU feature flag, and thus will not affect other systems. Root irqchip drivers can discriminate between IRQs and FIQs by checking the ISR_EL1 system register. Signed-off-by: Hector Martin <marcan@marcan.st> --- arch/arm64/include/asm/assembler.h | 4 ++++ arch/arm64/include/asm/daifflags.h | 7 +++++++ arch/arm64/include/asm/irqflags.h | 17 +++++++++++++---- arch/arm64/kernel/entry.S | 27 +++++++++++++++++++++++---- 4 files changed, 47 insertions(+), 8 deletions(-)