Re: [PATCH 1/3] arm64/kernel: FIQ support

From: Mohamed Mediouni
Date: Wed Jan 20 2021 - 09:15:08 EST

Next message: Gerd Hoffmann: "Re: [PATCH v2] drm/virtio: Track total GPU memory for virtio driver"
Previous message: Adrian Ratiu: "Re: [PATCH v4 1/2] arm: lib: xor-neon: remove unnecessary GCC < 4.6 warning"
In reply to: Marc Zyngier: "Re: [PATCH 1/3] arm64/kernel: FIQ support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> On 20 Jan 2021, at 14:16, Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Hi Mohamed,
>
> On 2021-01-20 11:36, Mohamed Mediouni wrote:
>> From: Stan Skowronek <stan@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>
> 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...

Next message: Gerd Hoffmann: "Re: [PATCH v2] drm/virtio: Track total GPU memory for virtio driver"
Previous message: Adrian Ratiu: "Re: [PATCH v4 1/2] arm: lib: xor-neon: remove unnecessary GCC < 4.6 warning"
In reply to: Marc Zyngier: "Re: [PATCH 1/3] arm64/kernel: FIQ support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]