| Message ID | 20210215121713.57687-9-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
Hi Hector, On Mon, Feb 15, 2021 at 09:16:56PM +0900, Hector Martin wrote: > Apple SoCs (A11 and newer) have some interrupt sources hardwired to the > FIQ line. We implement support for this by simply treating IRQs and FIQs > the same way in the interrupt vectors. > > To support these systems, the FIQ mask bit needs to be kept in sync with > the IRQ mask bit, so both kinds of exceptions are masked together. No > other platforms should be delivering FIQ exceptions right now, and we > already unmask FIQ in normal process context, so this should not have an > effect on other systems - if spurious FIQs were arriving, they would > already panic the kernel. Keeping these in sync sounds fine to me, FWIW. > Root irqchip drivers can discriminate between IRQs and FIQs by checking > the ISR_EL1 system register. I think we can remove this note for now. If we go with seperate handlers this won't be necessary, and if not this would be better placed on a commit adding the FIQ handling capability. > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > arch/arm64/include/asm/assembler.h | 6 +++--- > arch/arm64/include/asm/daifflags.h | 4 ++-- > arch/arm64/include/asm/irqflags.h | 19 +++++++++++-------- > arch/arm64/kernel/entry.S | 6 +++--- > 4 files changed, 19 insertions(+), 16 deletions(-) Judging by `git grep -Wi daif -- arch/arm64`, with this patch applied, we'll also need fixups in: * gic_arch_enable_irqs() in arch/arm64/include/asm/arch_gicv3.h * save_and_disable_irq() in arch/arm64/include/asm/assembler.h (noted below) * local_daif_save_flags() in arch/arm64/include/asm/daifflags.h (the fake DAIF should have F set too) * __cpu_do_idle_irqprio() in arch/arm64/kernel/process.c > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index bf125c591116..ac4c823bf2b6 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -40,9 +40,9 @@ > msr daif, \flags > .endm > > - /* IRQ is the lowest priority flag, unconditionally unmask the rest. */ > - .macro enable_da_f > - msr daifclr, #(8 | 4 | 1) > + /* IRQ/FIQ are the lowest priority flags, unconditionally unmask the rest. */ > + .macro enable_da > + msr daifclr, #(8 | 4) > .endm I think save_and_diable_irq below needs to be updated too, since it only sets DAIF.I and leaves DAIF.F as-is. [...] > diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h > index ff328e5bbb75..125201dced5f 100644 > --- a/arch/arm64/include/asm/irqflags.h > +++ b/arch/arm64/include/asm/irqflags.h > @@ -12,15 +12,18 @@ > > /* > * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and > - * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai' > + * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'daif' > * order: > * Masking debug exceptions causes all other exceptions to be masked too/ > - * Masking SError masks irq, but not debug exceptions. Masking irqs has no > - * side effects for other flags. Keeping to this order makes it easier for > - * entry.S to know which exceptions should be unmasked. > + * Masking SError masks IRQ/FIQ, but not debug exceptions. IRQ and FIQ are > + * always masked and unmasked together, and have no side effects for other > + * flags. Keeping to this order makes it easier for entry.S to know which > + * exceptions should be unmasked. > * This sounds good. > - * 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 keep it synchronized > + * with the IRQ mask status. On platforms that do not expect FIQ, that vector > + * triggers a kernel panic. On platforms that do, the FIQ vector is unified > + * with the IRQ vector. > */ Can we please delete this bit, though? Now that we say IRQ and FIQ are masked/unmasked together, I don't think the rest is necessary to understand the masking logic, and it's one less thing to keep in sync with changes to the entry code. Otherwise this looks good to me. Thanks, Mark.
On 17/02/2021 21.22, Mark Rutland wrote: >> Root irqchip drivers can discriminate between IRQs and FIQs by checking >> the ISR_EL1 system register. > > I think we can remove this note for now. If we go with seperate handlers > this won't be necessary, and if not this would be better placed on a > commit adding the FIQ handling capability. Indeed, this doesn't make sense any more. Changed for v3. > Judging by `git grep -Wi daif -- arch/arm64`, with this patch applied, > we'll also need fixups in: > > * gic_arch_enable_irqs() in arch/arm64/include/asm/arch_gicv3.h > * save_and_disable_irq() in arch/arm64/include/asm/assembler.h (noted below) > * local_daif_save_flags() in arch/arm64/include/asm/daifflags.h > (the fake DAIF should have F set too) > * __cpu_do_idle_irqprio() in arch/arm64/kernel/process.c Good catches. A few of those are irrelevant for M1 but need to be done now that we're making this change globally, others I just missed from the beginning. There's also an incorrect comment in entry.S: /* * DA_F were cleared at start of handling. If anything is set in * DAIF, we come back from an NMI, so skip preemption */ mrs x0, daif orr x24, x24, x0 Now only DA__ are cleared. This actually pairs with gic_arch_enable_irqs() and begs the question: in priority masking systems, do we unmask both IRQ and FIQ (the gic_arch_enable_irqs change), or do we leave FIQ masked (which instead would need an AND in that part of entry.S so as to not consider FIQ masked as meaning we're coming back from an NMI)? And a minor related one: should init_gic_priority_masking() WARN if FIQ is masked too? This probably goes with the above. Either way, this was nontrivial to make sense of, so I'll make that entry.S comment clearer while I'm touching it. > I think save_and_diable_irq below needs to be updated too, since it > only sets DAIF.I and leaves DAIF.F as-is. Totally missed this one! Fixed for v3. >> - * 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 keep it synchronized >> + * with the IRQ mask status. On platforms that do not expect FIQ, that vector >> + * triggers a kernel panic. On platforms that do, the FIQ vector is unified >> + * with the IRQ vector. >> */ > > Can we please delete this bit, though? Now that we say IRQ and FIQ are > masked/unmasked together, I don't think the rest is necessary to > understand the masking logic, and it's one less thing to keep in sync > with changes to the entry code. Gone :)
On Thu, Feb 18, 2021 at 09:51:40PM +0900, Hector Martin wrote: > On 17/02/2021 21.22, Mark Rutland wrote: > > > Root irqchip drivers can discriminate between IRQs and FIQs by checking > > > the ISR_EL1 system register. > > > > I think we can remove this note for now. If we go with seperate handlers > > this won't be necessary, and if not this would be better placed on a > > commit adding the FIQ handling capability. > > Indeed, this doesn't make sense any more. Changed for v3. > > > Judging by `git grep -Wi daif -- arch/arm64`, with this patch applied, > > we'll also need fixups in: > > > > * gic_arch_enable_irqs() in arch/arm64/include/asm/arch_gicv3.h > > * save_and_disable_irq() in arch/arm64/include/asm/assembler.h (noted below) > > * local_daif_save_flags() in arch/arm64/include/asm/daifflags.h > > (the fake DAIF should have F set too) > > * __cpu_do_idle_irqprio() in arch/arm64/kernel/process.c > > Good catches. A few of those are irrelevant for M1 but need to be done now > that we're making this change globally, others I just missed from the > beginning. Sure; my general view is that we should aim for consistency, and should ensure that DAIF.F==DAIF.I at all times on all platforms unless we have a strong reason to violate that rule. That generally makes it easier to reason about the code and avoid accidentally breaking M1/non-M1 if/when we refactor masking logic. > There's also an incorrect comment in entry.S: > > /* > * DA_F were cleared at start of handling. If anything is set in > * DAIF, we come back from an NMI, so skip preemption > */ > mrs x0, daif > orr x24, x24, x0 > > Now only DA__ are cleared. This actually pairs with gic_arch_enable_irqs() > and begs the question: in priority masking systems, do we unmask both IRQ > and FIQ (the gic_arch_enable_irqs change), or do we leave FIQ masked (which > instead would need an AND in that part of entry.S so as to not consider FIQ > masked as meaning we're coming back from an NMI)? I think that for consistency we always want to keep IRQ and FIQ in-sync, even when using GIC priorities. So when handling a pseudo-NMI we should unmask DAIF.DA and leave DAIF.IF masked. > And a minor related one: should init_gic_priority_masking() WARN if FIQ is > masked too? This probably goes with the above. I think it should, yes. > Either way, this was nontrivial to make sense of, so I'll make that entry.S > comment clearer while I'm touching it. Sounds good; thanks! > > I think save_and_diable_irq below needs to be updated too, since it > > only sets DAIF.I and leaves DAIF.F as-is. > > Totally missed this one! Fixed for v3. > > > > - * 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 keep it synchronized > > > + * with the IRQ mask status. On platforms that do not expect FIQ, that vector > > > + * triggers a kernel panic. On platforms that do, the FIQ vector is unified > > > + * with the IRQ vector. > > > */ > > > > Can we please delete this bit, though? Now that we say IRQ and FIQ are > > masked/unmasked together, I don't think the rest is necessary to > > understand the masking logic, and it's one less thing to keep in sync > > with changes to the entry code. > > Gone :) Thanks, Mark.
On 18/02/2021 23.22, Mark Rutland wrote: > I think that for consistency we always want to keep IRQ and FIQ in-sync, > even when using GIC priorities. So when handling a pseudo-NMI we should > unmask DAIF.DA and leave DAIF.IF masked. In that case there's one more, in daifflags.h:local_daif_restore(): /* * If interrupts are disabled but we can take * asynchronous errors, we can take NMIs */ flags &= PSR_I_BIT; pmr = GIC_PRIO_IRQOFF; >> And a minor related one: should init_gic_priority_masking() WARN if FIQ is >> masked too? This probably goes with the above. > > I think it should, yes. Done for v3 then. Thanks!
On Thu, Feb 18, 2021 at 11:42:01PM +0900, Hector Martin wrote: > On 18/02/2021 23.22, Mark Rutland wrote: > > I think that for consistency we always want to keep IRQ and FIQ in-sync, > > even when using GIC priorities. So when handling a pseudo-NMI we should > > unmask DAIF.DA and leave DAIF.IF masked. > > In that case there's one more, in daifflags.h:local_daif_restore(): > > /* > * If interrupts are disabled but we can take > * asynchronous errors, we can take NMIs > */ > flags &= PSR_I_BIT; > pmr = GIC_PRIO_IRQOFF; Good spot, yes! I did a quick scan with `git grep 'PSR_[IF]_BIT' -- arch/arm64`, and AFAICT that's the last one. > > > And a minor related one: should init_gic_priority_masking() WARN if FIQ is > > > masked too? This probably goes with the above. > > > > I think it should, yes. > > Done for v3 then. Thanks! Cool! Mark.
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index bf125c591116..ac4c823bf2b6 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -40,9 +40,9 @@ msr daif, \flags .endm - /* IRQ is the lowest priority flag, unconditionally unmask the rest. */ - .macro enable_da_f - msr daifclr, #(8 | 4 | 1) + /* IRQ/FIQ are the lowest priority flags, unconditionally unmask the rest. */ + .macro enable_da + msr daifclr, #(8 | 4) .endm /* diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index 1c26d7baa67f..9d1d4ab98585 100644 --- a/arch/arm64/include/asm/daifflags.h +++ b/arch/arm64/include/asm/daifflags.h @@ -13,8 +13,8 @@ #include <asm/ptrace.h> #define DAIF_PROCCTX 0 -#define DAIF_PROCCTX_NOIRQ PSR_I_BIT -#define DAIF_ERRCTX (PSR_I_BIT | PSR_A_BIT) +#define DAIF_PROCCTX_NOIRQ (PSR_I_BIT | PSR_F_BIT) +#define DAIF_ERRCTX (PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) #define DAIF_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index ff328e5bbb75..125201dced5f 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -12,15 +12,18 @@ /* * Aarch64 has flags for masking: Debug, Asynchronous (serror), Interrupts and - * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'dai' + * FIQ exceptions, in the 'daif' register. We mask and unmask them in 'daif' * order: * Masking debug exceptions causes all other exceptions to be masked too/ - * Masking SError masks irq, but not debug exceptions. Masking irqs has no - * side effects for other flags. Keeping to this order makes it easier for - * entry.S to know which exceptions should be unmasked. + * Masking SError masks IRQ/FIQ, but not debug exceptions. IRQ and FIQ are + * always masked and unmasked together, and have no 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 keep it synchronized + * with the IRQ mask status. On platforms that do not expect FIQ, that vector + * triggers a kernel panic. On platforms that do, the FIQ vector is unified + * with the IRQ vector. */ /* @@ -35,7 +38,7 @@ static inline void arch_local_irq_enable(void) } asm volatile(ALTERNATIVE( - "msr daifclr, #2 // arch_local_irq_enable", + "msr daifclr, #3 // arch_local_irq_enable", __msr_s(SYS_ICC_PMR_EL1, "%0"), ARM64_HAS_IRQ_PRIO_MASKING) : @@ -54,7 +57,7 @@ static inline void arch_local_irq_disable(void) } asm volatile(ALTERNATIVE( - "msr daifset, #2 // arch_local_irq_disable", + "msr daifset, #3 // arch_local_irq_disable", __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..ba5f9aa379ce 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -661,7 +661,7 @@ SYM_CODE_END(el1_sync) SYM_CODE_START_LOCAL_NOALIGN(el1_irq) kernel_entry 1 gic_prio_irq_setup pmr=x20, tmp=x1 - enable_da_f + enable_da mov x0, sp bl enter_el1_irq_or_nmi @@ -727,7 +727,7 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq) el0_irq_naked: gic_prio_irq_setup pmr=x20, tmp=x0 user_exit_irqoff - enable_da_f + enable_da tbz x22, #55, 1f bl do_el0_irq_bp_hardening @@ -757,7 +757,7 @@ el0_error_naked: mov x0, sp mov x1, x25 bl do_serror - enable_da_f + enable_da b ret_to_user SYM_CODE_END(el0_error)
Apple SoCs (A11 and newer) have some interrupt sources hardwired to the FIQ line. We implement support for this by simply treating IRQs and FIQs the same way in the interrupt vectors. To support these systems, the FIQ mask bit needs to be kept in sync with the IRQ mask bit, so both kinds of exceptions are masked together. No other platforms should be delivering FIQ exceptions right now, and we already unmask FIQ in normal process context, so this should not have an effect on other systems - if spurious FIQs were arriving, they would already panic the kernel. 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 | 6 +++--- arch/arm64/include/asm/daifflags.h | 4 ++-- arch/arm64/include/asm/irqflags.h | 19 +++++++++++-------- arch/arm64/kernel/entry.S | 6 +++--- 4 files changed, 19 insertions(+), 16 deletions(-)