Re: [PATCH 10/18] arm64: Introduce FIQ support

[Marc Zyngier <maz@kernel.org>]

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


-- 
Without deviation from the norm, progress is not possible.