* [PATCH 1/3] arm64/kernel: FIQ support @ 2021-01-20 11:36 Mohamed Mediouni 2021-01-20 13:16 ` Marc Zyngier 0 siblings, 1 reply; 3+ messages in thread From: Mohamed Mediouni @ 2021-01-20 11:36 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: Catalin Marinas, Will Deacon, Stan Skowronek From: Stan Skowronek <stan@corellium.com> On Apple processors, the timer is wired through FIQ. As such, add FIQ support to the kernel. Signed-off-by: Stan Skowronek <stan@corellium.com> --- arch/arm64/include/asm/arch_gicv3.h | 2 +- arch/arm64/include/asm/assembler.h | 8 ++-- arch/arm64/include/asm/daifflags.h | 4 +- arch/arm64/include/asm/irq.h | 4 ++ arch/arm64/include/asm/irqflags.h | 6 +-- arch/arm64/kernel/entry.S | 74 ++++++++++++++++++++++++++--- arch/arm64/kernel/irq.c | 14 ++++++ arch/arm64/kernel/process.c | 2 +- 8 files changed, 97 insertions(+), 17 deletions(-) diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h index 880b9054d75c..934b9be582d2 100644 --- a/arch/arm64/include/asm/arch_gicv3.h +++ b/arch/arm64/include/asm/arch_gicv3.h @@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void) static inline void gic_arch_enable_irqs(void) { - asm volatile ("msr daifclr, #2" : : : "memory"); + asm volatile ("msr daifclr, #3" : : : "memory"); } #endif /* __ASSEMBLY__ */ diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index bf125c591116..6fe55713dfe0 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 is the lowest priority flag, unconditionally unmask the rest. */ + .macro enable_da + msr daifclr, #(8 | 4) .endm /* @@ -50,7 +50,7 @@ */ .macro save_and_disable_irq, flags mrs \flags, daif - msr daifset, #2 + msr daifset, #3 .endm .macro restore_irq, flags diff --git a/arch/arm64/include/asm/daifflags.h b/arch/arm64/include/asm/daifflags.h index 1c26d7baa67f..44de96c7fb1a 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_I_BIT | PSR_F_BIT | PSR_A_BIT) #define DAIF_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h index b2b0c6405eb0..2d1537d3a245 100644 --- a/arch/arm64/include/asm/irq.h +++ b/arch/arm64/include/asm/irq.h @@ -13,5 +13,9 @@ static inline int nr_legacy_irqs(void) return 0; } +int set_handle_fiq(void (*handle_fiq)(struct pt_regs *)); + +extern void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; + #endif /* !__ASSEMBLER__ */ #endif diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h index ff328e5bbb75..26d7f378113e 100644 --- a/arch/arm64/include/asm/irqflags.h +++ b/arch/arm64/include/asm/irqflags.h @@ -35,7 +35,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 +54,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) : @@ -85,7 +85,7 @@ static inline int arch_irqs_disabled_flags(unsigned long flags) int res; asm volatile(ALTERNATIVE( - "and %w0, %w1, #" __stringify(PSR_I_BIT), + "and %w0, %w1, #" __stringify(PSR_I_BIT | PSR_F_BIT), "eor %w0, %w1, #" __stringify(GIC_PRIO_IRQON), ARM64_HAS_IRQ_PRIO_MASKING) : "=&r" (res) diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index c9bae73f2621..abcca0db0736 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -499,6 +499,14 @@ tsk .req x28 // current thread_info irq_stack_exit .endm + .macro fiq_handler + ldr_l x1, handle_arch_fiq + mov x0, sp + irq_stack_entry + blr x1 + irq_stack_exit + .endm + #ifdef CONFIG_ARM64_PSEUDO_NMI /* * Set res to 0 if irqs were unmasked in interrupted context. @@ -547,18 +555,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_compat, 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 @@ -661,7 +669,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 @@ -689,6 +697,38 @@ alternative_else_nop_endif kernel_exit 1 SYM_CODE_END(el1_irq) + .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el1_fiq) + kernel_entry 1 + gic_prio_irq_setup pmr=x20, tmp=x1 + enable_da + + mov x0, sp + bl enter_el1_irq_or_nmi + + fiq_handler + +#ifdef CONFIG_PREEMPTION + ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count +alternative_if ARM64_HAS_IRQ_PRIO_MASKING + /* + * 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 +alternative_else_nop_endif + cbnz x24, 1f // preempt count != 0 || NMI return path + bl arm64_preempt_schedule_irq // irq en/disable is done inside +1: +#endif + + mov x0, sp + bl exit_el1_irq_or_nmi + + kernel_exit 1 +SYM_CODE_END(el1_fiq) + /* * EL0 mode handlers. */ @@ -715,6 +755,12 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat) b el0_irq_naked SYM_CODE_END(el0_irq_compat) + .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat) + kernel_entry 0, 32 + b el0_fiq_naked +SYM_CODE_END(el0_fiq_compat) + SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat) kernel_entry 0, 32 b el0_error_naked @@ -727,7 +773,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 @@ -737,6 +783,22 @@ el0_irq_naked: b ret_to_user SYM_CODE_END(el0_irq) + .align 6 +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq) + kernel_entry 0 +el0_fiq_naked: + gic_prio_irq_setup pmr=x20, tmp=x0 + user_exit_irqoff + enable_da + + tbz x22, #55, 1f + bl do_el0_irq_bp_hardening +1: + fiq_handler + + b ret_to_user +SYM_CODE_END(el0_fiq) + SYM_CODE_START_LOCAL(el1_error) kernel_entry 1 mrs x1, esr_el1 @@ -757,7 +819,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) diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c index dfb1feab867d..4d7a9fb41d93 100644 --- a/arch/arm64/kernel/irq.c +++ b/arch/arm64/kernel/irq.c @@ -88,3 +88,17 @@ void __init init_IRQ(void) local_daif_restore(DAIF_PROCCTX_NOIRQ); } } + +/* + * Analogous to the generic handle_arch_irq / set_handle_irq + */ +void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; + +int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *)) +{ + if (handle_arch_fiq) + return -EBUSY; + + handle_arch_fiq = handle_fiq; + return 0; +} diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 6616486a58fe..34ec400288d0 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void) unsigned long daif_bits; daif_bits = read_sysreg(daif); - write_sysreg(daif_bits | PSR_I_BIT, daif); + write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif); /* * Unmask PMR before going idle to make sure interrupts can -- 2.29.2 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] arm64/kernel: FIQ support 2021-01-20 11:36 [PATCH 1/3] arm64/kernel: FIQ support Mohamed Mediouni @ 2021-01-20 13:16 ` Marc Zyngier 2021-01-20 13:33 ` Mohamed Mediouni 0 siblings, 1 reply; 3+ messages in thread From: Marc Zyngier @ 2021-01-20 13:16 UTC (permalink / raw) To: Mohamed Mediouni Cc: Catalin Marinas, Will Deacon, linux-kernel, linux-arm-kernel, Stan Skowronek Hi Mohamed, On 2021-01-20 11:36, Mohamed Mediouni wrote: > From: Stan Skowronek <stan@corellium.com> > > On Apple processors, the timer is wired through FIQ. Which timer? There are at least 3, potentially 4 timers per CPU that can fire. > As such, add FIQ support to the kernel. > > Signed-off-by: Stan Skowronek <stan@corellium.com> Missing SoB from the sender. > --- > arch/arm64/include/asm/arch_gicv3.h | 2 +- > arch/arm64/include/asm/assembler.h | 8 ++-- > arch/arm64/include/asm/daifflags.h | 4 +- > arch/arm64/include/asm/irq.h | 4 ++ > arch/arm64/include/asm/irqflags.h | 6 +-- > arch/arm64/kernel/entry.S | 74 ++++++++++++++++++++++++++--- > arch/arm64/kernel/irq.c | 14 ++++++ > arch/arm64/kernel/process.c | 2 +- > 8 files changed, 97 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_gicv3.h > b/arch/arm64/include/asm/arch_gicv3.h > index 880b9054d75c..934b9be582d2 100644 > --- a/arch/arm64/include/asm/arch_gicv3.h > +++ b/arch/arm64/include/asm/arch_gicv3.h > @@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void) > > static inline void gic_arch_enable_irqs(void) > { > - asm volatile ("msr daifclr, #2" : : : "memory"); > + asm volatile ("msr daifclr, #3" : : : "memory"); If I trust the persistent rumour, this system doesn't have a GIC. Why this change? > } > > #endif /* __ASSEMBLY__ */ > diff --git a/arch/arm64/include/asm/assembler.h > b/arch/arm64/include/asm/assembler.h > index bf125c591116..6fe55713dfe0 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 is the lowest priority flag, unconditionally unmask the > rest. */ > + .macro enable_da > + msr daifclr, #(8 | 4) This cannot be unconditional. This potentially changes existing behaviours, and I'd feel a lot safer if FIQ was only messed with on that specific HW. I have the feeling that this should be detected on the boot CPU and patched before any interrupt can fire. > .endm > > /* > @@ -50,7 +50,7 @@ > */ > .macro save_and_disable_irq, flags > mrs \flags, daif > - msr daifset, #2 > + msr daifset, #3 > .endm > > .macro restore_irq, flags > diff --git a/arch/arm64/include/asm/daifflags.h > b/arch/arm64/include/asm/daifflags.h > index 1c26d7baa67f..44de96c7fb1a 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_I_BIT | PSR_F_BIT | PSR_A_BIT) > #define DAIF_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) > > > diff --git a/arch/arm64/include/asm/irq.h > b/arch/arm64/include/asm/irq.h > index b2b0c6405eb0..2d1537d3a245 100644 > --- a/arch/arm64/include/asm/irq.h > +++ b/arch/arm64/include/asm/irq.h > @@ -13,5 +13,9 @@ static inline int nr_legacy_irqs(void) > return 0; > } > > +int set_handle_fiq(void (*handle_fiq)(struct pt_regs *)); > + > +extern void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; I guess this is set from the root interrupt controller, which also will set handle_arch_irq? Why do we need two entry points? We have ISR_EL1 to find out what is pending. Isn't that enough? > + > #endif /* !__ASSEMBLER__ */ > #endif > diff --git a/arch/arm64/include/asm/irqflags.h > b/arch/arm64/include/asm/irqflags.h > index ff328e5bbb75..26d7f378113e 100644 > --- a/arch/arm64/include/asm/irqflags.h > +++ b/arch/arm64/include/asm/irqflags.h > @@ -35,7 +35,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 +54,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) > : > @@ -85,7 +85,7 @@ static inline int arch_irqs_disabled_flags(unsigned > long flags) > int res; > > asm volatile(ALTERNATIVE( > - "and %w0, %w1, #" __stringify(PSR_I_BIT), > + "and %w0, %w1, #" __stringify(PSR_I_BIT | PSR_F_BIT), > "eor %w0, %w1, #" __stringify(GIC_PRIO_IRQON), > ARM64_HAS_IRQ_PRIO_MASKING) > : "=&r" (res) > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index c9bae73f2621..abcca0db0736 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -499,6 +499,14 @@ tsk .req x28 // current thread_info > irq_stack_exit > .endm > > + .macro fiq_handler > + ldr_l x1, handle_arch_fiq > + mov x0, sp > + irq_stack_entry > + blr x1 > + irq_stack_exit > + .endm > + > #ifdef CONFIG_ARM64_PSEUDO_NMI > /* > * Set res to 0 if irqs were unmasked in interrupted context. > @@ -547,18 +555,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_compat, 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 > @@ -661,7 +669,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 > @@ -689,6 +697,38 @@ alternative_else_nop_endif > kernel_exit 1 > SYM_CODE_END(el1_irq) > > + .align 6 > +SYM_CODE_START_LOCAL_NOALIGN(el1_fiq) > + kernel_entry 1 > + gic_prio_irq_setup pmr=x20, tmp=x1 This doesn't make much sense. The HW doesn't have a GIC, and Linux doesn't use Group-0 interrupts, even in a guest. > + enable_da > + > + mov x0, sp > + bl enter_el1_irq_or_nmi > + > + fiq_handler > + > +#ifdef CONFIG_PREEMPTION > + ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count > +alternative_if ARM64_HAS_IRQ_PRIO_MASKING > + /* > + * 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 > +alternative_else_nop_endif > + cbnz x24, 1f // preempt count != 0 || NMI return path > + bl arm64_preempt_schedule_irq // irq en/disable is done inside > +1: > +#endif > + > + mov x0, sp > + bl exit_el1_irq_or_nmi > + > + kernel_exit 1 > +SYM_CODE_END(el1_fiq) Given that this is essentially a copy paste of el1_irq, and that a separate FIQ entry point seems superfluous, I don't think we need any of this, and it could be implemented just as the IRQ vector. > + > /* > * EL0 mode handlers. > */ > @@ -715,6 +755,12 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat) > b el0_irq_naked > SYM_CODE_END(el0_irq_compat) > > + .align 6 > +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat) > + kernel_entry 0, 32 > + b el0_fiq_naked > +SYM_CODE_END(el0_fiq_compat) > + > SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat) > kernel_entry 0, 32 > b el0_error_naked > @@ -727,7 +773,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 > @@ -737,6 +783,22 @@ el0_irq_naked: > b ret_to_user > SYM_CODE_END(el0_irq) > > + .align 6 > +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq) > + kernel_entry 0 > +el0_fiq_naked: > + gic_prio_irq_setup pmr=x20, tmp=x0 > + user_exit_irqoff > + enable_da > + > + tbz x22, #55, 1f > + bl do_el0_irq_bp_hardening > +1: > + fiq_handler > + > + b ret_to_user > +SYM_CODE_END(el0_fiq) Same thing. > + > SYM_CODE_START_LOCAL(el1_error) > kernel_entry 1 > mrs x1, esr_el1 > @@ -757,7 +819,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) > > diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c > index dfb1feab867d..4d7a9fb41d93 100644 > --- a/arch/arm64/kernel/irq.c > +++ b/arch/arm64/kernel/irq.c > @@ -88,3 +88,17 @@ void __init init_IRQ(void) > local_daif_restore(DAIF_PROCCTX_NOIRQ); > } > } > + > +/* > + * Analogous to the generic handle_arch_irq / set_handle_irq > + */ > +void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; > + > +int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *)) > +{ > + if (handle_arch_fiq) > + return -EBUSY; > + > + handle_arch_fiq = handle_fiq; > + return 0; > +} Also, this has no caller, so it is pretty hard to judge how useful this is. > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 6616486a58fe..34ec400288d0 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void) > unsigned long daif_bits; > > daif_bits = read_sysreg(daif); > - write_sysreg(daif_bits | PSR_I_BIT, daif); > + write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif); > > /* > * Unmask PMR before going idle to make sure interrupts can Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] arm64/kernel: FIQ support 2021-01-20 13:16 ` Marc Zyngier @ 2021-01-20 13:33 ` Mohamed Mediouni 0 siblings, 0 replies; 3+ messages in thread From: Mohamed Mediouni @ 2021-01-20 13:33 UTC (permalink / raw) To: Marc Zyngier Cc: Catalin Marinas, Will Deacon, linux-kernel, linux-arm-kernel, Stan Skowronek > On 20 Jan 2021, at 14:16, Marc Zyngier <maz@kernel.org> wrote: > > Hi Mohamed, > > On 2021-01-20 11:36, Mohamed Mediouni wrote: >> From: Stan Skowronek <stan@corellium.com> >> On Apple processors, the timer is wired through FIQ. > > Which timer? There are at least 3, potentially 4 timers per CPU > that can fire. This is about the Arm architectural timers. >> As such, add FIQ support to the kernel. >> Signed-off-by: Stan Skowronek <stan@corellium.com> > > Missing SoB from the sender. > Fixed in the RFC. >> --- >> arch/arm64/include/asm/arch_gicv3.h | 2 +- >> arch/arm64/include/asm/assembler.h | 8 ++-- >> arch/arm64/include/asm/daifflags.h | 4 +- >> arch/arm64/include/asm/irq.h | 4 ++ >> arch/arm64/include/asm/irqflags.h | 6 +-- >> arch/arm64/kernel/entry.S | 74 ++++++++++++++++++++++++++--- >> arch/arm64/kernel/irq.c | 14 ++++++ >> arch/arm64/kernel/process.c | 2 +- >> 8 files changed, 97 insertions(+), 17 deletions(-) >> diff --git a/arch/arm64/include/asm/arch_gicv3.h >> b/arch/arm64/include/asm/arch_gicv3.h >> index 880b9054d75c..934b9be582d2 100644 >> --- a/arch/arm64/include/asm/arch_gicv3.h >> +++ b/arch/arm64/include/asm/arch_gicv3.h >> @@ -173,7 +173,7 @@ static inline void gic_pmr_mask_irqs(void) >> static inline void gic_arch_enable_irqs(void) >> { >> - asm volatile ("msr daifclr, #2" : : : "memory"); >> + asm volatile ("msr daifclr, #3" : : : "memory"); > > If I trust the persistent rumour, this system doesn't have a GIC. > Why this change? > Will ask about why GIC functions were changed too… and yeah This exclusively has an Apple AIC interrupt controller. >> #endif /* __ASSEMBLY__ */ >> diff --git a/arch/arm64/include/asm/assembler.h >> b/arch/arm64/include/asm/assembler.h >> index bf125c591116..6fe55713dfe0 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 is the lowest priority flag, unconditionally unmask the rest. */ >> + .macro enable_da >> + msr daifclr, #(8 | 4) > > This cannot be unconditional. This potentially changes existing behaviours, > and I'd feel a lot safer if FIQ was only messed with on that specific HW. > > I have the feeling that this should be detected on the boot CPU and patched > before any interrupt can fire. > Could alternatives be the proper mechanism for this? >> .endm >> /* >> @@ -50,7 +50,7 @@ >> */ >> .macro save_and_disable_irq, flags >> mrs \flags, daif >> - msr daifset, #2 >> + msr daifset, #3 >> .endm >> .macro restore_irq, flags >> diff --git a/arch/arm64/include/asm/daifflags.h >> b/arch/arm64/include/asm/daifflags.h >> index 1c26d7baa67f..44de96c7fb1a 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_I_BIT | PSR_F_BIT | PSR_A_BIT) >> #define DAIF_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT) >> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h >> index b2b0c6405eb0..2d1537d3a245 100644 >> --- a/arch/arm64/include/asm/irq.h >> +++ b/arch/arm64/include/asm/irq.h >> @@ -13,5 +13,9 @@ static inline int nr_legacy_irqs(void) >> return 0; >> } >> +int set_handle_fiq(void (*handle_fiq)(struct pt_regs *)); >> + >> +extern void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; > > I guess this is set from the root interrupt controller, which also > will set handle_arch_irq? Why do we need two entry points? We have > ISR_EL1 to find out what is pending. Isn't that enough? > >> + >> #endif /* !__ASSEMBLER__ */ >> #endif >> diff --git a/arch/arm64/include/asm/irqflags.h >> b/arch/arm64/include/asm/irqflags.h >> index ff328e5bbb75..26d7f378113e 100644 >> --- a/arch/arm64/include/asm/irqflags.h >> +++ b/arch/arm64/include/asm/irqflags.h >> @@ -35,7 +35,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 +54,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) >> : >> @@ -85,7 +85,7 @@ static inline int arch_irqs_disabled_flags(unsigned >> long flags) >> int res; >> asm volatile(ALTERNATIVE( >> - "and %w0, %w1, #" __stringify(PSR_I_BIT), >> + "and %w0, %w1, #" __stringify(PSR_I_BIT | PSR_F_BIT), >> "eor %w0, %w1, #" __stringify(GIC_PRIO_IRQON), >> ARM64_HAS_IRQ_PRIO_MASKING) >> : "=&r" (res) >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S >> index c9bae73f2621..abcca0db0736 100644 >> --- a/arch/arm64/kernel/entry.S >> +++ b/arch/arm64/kernel/entry.S >> @@ -499,6 +499,14 @@ tsk .req x28 // current thread_info >> irq_stack_exit >> .endm >> + .macro fiq_handler >> + ldr_l x1, handle_arch_fiq >> + mov x0, sp >> + irq_stack_entry >> + blr x1 >> + irq_stack_exit >> + .endm >> + >> #ifdef CONFIG_ARM64_PSEUDO_NMI >> /* >> * Set res to 0 if irqs were unmasked in interrupted context. >> @@ -547,18 +555,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_compat, 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 >> @@ -661,7 +669,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 >> @@ -689,6 +697,38 @@ alternative_else_nop_endif >> kernel_exit 1 >> SYM_CODE_END(el1_irq) >> + .align 6 >> +SYM_CODE_START_LOCAL_NOALIGN(el1_fiq) >> + kernel_entry 1 >> + gic_prio_irq_setup pmr=x20, tmp=x1 > > This doesn't make much sense. The HW doesn't have a GIC, and Linux > doesn't use Group-0 interrupts, even in a guest. > >> + enable_da >> + >> + mov x0, sp >> + bl enter_el1_irq_or_nmi >> + >> + fiq_handler >> + >> +#ifdef CONFIG_PREEMPTION >> + ldr x24, [tsk, #TSK_TI_PREEMPT] // get preempt count >> +alternative_if ARM64_HAS_IRQ_PRIO_MASKING >> + /* >> + * 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 >> +alternative_else_nop_endif >> + cbnz x24, 1f // preempt count != 0 || NMI return path >> + bl arm64_preempt_schedule_irq // irq en/disable is done inside >> +1: >> +#endif >> + >> + mov x0, sp >> + bl exit_el1_irq_or_nmi >> + >> + kernel_exit 1 >> +SYM_CODE_END(el1_fiq) > > Given that this is essentially a copy paste of el1_irq, and that > a separate FIQ entry point seems superfluous, I don't think we > need any of this, and it could be implemented just as the IRQ > vector. > >> + >> /* >> * EL0 mode handlers. >> */ >> @@ -715,6 +755,12 @@ SYM_CODE_START_LOCAL_NOALIGN(el0_irq_compat) >> b el0_irq_naked >> SYM_CODE_END(el0_irq_compat) >> + .align 6 >> +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq_compat) >> + kernel_entry 0, 32 >> + b el0_fiq_naked >> +SYM_CODE_END(el0_fiq_compat) >> + >> SYM_CODE_START_LOCAL_NOALIGN(el0_error_compat) >> kernel_entry 0, 32 >> b el0_error_naked >> @@ -727,7 +773,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 >> @@ -737,6 +783,22 @@ el0_irq_naked: >> b ret_to_user >> SYM_CODE_END(el0_irq) >> + .align 6 >> +SYM_CODE_START_LOCAL_NOALIGN(el0_fiq) >> + kernel_entry 0 >> +el0_fiq_naked: >> + gic_prio_irq_setup pmr=x20, tmp=x0 >> + user_exit_irqoff >> + enable_da >> + >> + tbz x22, #55, 1f >> + bl do_el0_irq_bp_hardening >> +1: >> + fiq_handler >> + >> + b ret_to_user >> +SYM_CODE_END(el0_fiq) > > Same thing. > >> + >> SYM_CODE_START_LOCAL(el1_error) >> kernel_entry 1 >> mrs x1, esr_el1 >> @@ -757,7 +819,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) >> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >> index dfb1feab867d..4d7a9fb41d93 100644 >> --- a/arch/arm64/kernel/irq.c >> +++ b/arch/arm64/kernel/irq.c >> @@ -88,3 +88,17 @@ void __init init_IRQ(void) >> local_daif_restore(DAIF_PROCCTX_NOIRQ); >> } >> } >> + >> +/* >> + * Analogous to the generic handle_arch_irq / set_handle_irq >> + */ >> +void (*handle_arch_fiq)(struct pt_regs *) __ro_after_init; >> + >> +int __init set_handle_fiq(void (*handle_fiq)(struct pt_regs *)) >> +{ >> + if (handle_arch_fiq) >> + return -EBUSY; >> + >> + handle_arch_fiq = handle_fiq; >> + return 0; >> +} > > Also, this has no caller, so it is pretty hard to judge how useful > this is. Hello, in the RFC patch set that I just sent, it’s available now. >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 6616486a58fe..34ec400288d0 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -84,7 +84,7 @@ static void noinstr __cpu_do_idle_irqprio(void) >> unsigned long daif_bits; >> daif_bits = read_sysreg(daif); >> - write_sysreg(daif_bits | PSR_I_BIT, daif); >> + write_sysreg(daif_bits | PSR_I_BIT | PSR_F_BIT, daif); >> /* >> * Unmask PMR before going idle to make sure interrupts can > Thank you, > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-20 13:35 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 11:36 [PATCH 1/3] arm64/kernel: FIQ support Mohamed Mediouni 2021-01-20 13:16 ` Marc Zyngier 2021-01-20 13:33 ` Mohamed Mediouni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).