From: Stan Skowronek <[email protected]>
On Apple processors, the timer is wired through FIQ.
As such, add FIQ support to the kernel.
Signed-off-by: Stan Skowronek <[email protected]>
---
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
Hi Mohamed,
On 2021-01-20 11:36, Mohamed Mediouni wrote:
> From: Stan Skowronek <[email protected]>
>
> 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 <[email protected]>
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...
> On 20 Jan 2021, at 14:16, Marc Zyngier <[email protected]> wrote:
>
> Hi Mohamed,
>
> On 2021-01-20 11:36, Mohamed Mediouni wrote:
>> From: Stan Skowronek <[email protected]>
>> 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 <[email protected]>
>
> 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...