[RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Guenter Roeck]

On 3/14/23 05:57, Ard Biesheuvel wrote:
> Commit 62b95a7b44d1 (ARM: 9282/1: vfp: Manipulate task VFP state with
> softirqs disabled) replaced the en/disable preemption calls inside the
> VFP state handling code with en/disabling of soft IRQs, which is
> necessary to allow kernel use of the VFP/SIMD unit when handling a soft
> IRQ.
> 
> Unfortunately, when lockdep is enabled (or other instrumentation that
> enables TRACE_IRQFLAGS), the disable path implemented in asm fails to
> perform the lockdep and RCU related bookkeeping, resulting in spurious
> warnings and other badness.
> 
> So let's call the C versions when they are available, and only fall back
> to direct manipulation of the preempt_count when we disable soft IRQs
> with those instrumentations disabled.
> 

The problem is no longer seen with this patch applied on top of v6.3-rc2.
I only tested with CONFIG_TRACE_IRQFLAGS=y and CONFIG_PREEMPT=n. Should
I also test with CONFIG_PREEMPT=y and CONFIG_PREEMPT_RT=y ?

Thanks,
Guenter

> Cc: Frederic Weisbecker <frederic at kernel.org>
> Cc: Guenter Roeck <linux at roeck-us.net>
> Cc: Peter Zijlstra <peterz at infradead.org>
> Link: https://lore.kernel.org/all/ZBBYCSZUJOWBg1s8@localhost.localdomain/
> Fixes: 62b95a7b44d1 (ARM: 9282/1: vfp: Manipulate task VFP state with softirqs disabled)
> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> ---
>   arch/arm/include/asm/assembler.h | 15 ++++----------
>   arch/arm/vfp/entry.S             | 21 +++++++++++++++++---
>   arch/arm/vfp/vfphw.S             |  8 ++++----
>   3 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index 06b48ce23e1ca245..d9f7c546781a39eb 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -244,17 +244,10 @@ THUMB(	fpreg	.req	r7	)
>   	.endm
>   #endif
>   
> -	.macro	local_bh_disable, ti, tmp
> -	ldr	\tmp, [\ti, #TI_PREEMPT]
> -	add	\tmp, \tmp, #SOFTIRQ_DISABLE_OFFSET
> -	str	\tmp, [\ti, #TI_PREEMPT]
> -	.endm
> -
> -	.macro	local_bh_enable_ti, ti, tmp
> -	get_thread_info \ti
> -	ldr	\tmp, [\ti, #TI_PREEMPT]
> -	sub	\tmp, \tmp, #SOFTIRQ_DISABLE_OFFSET
> -	str	\tmp, [\ti, #TI_PREEMPT]
> +	.macro	local_bh_enable_and_ret
> +	adr	r0, .
> +	mov	r1, #SOFTIRQ_DISABLE_OFFSET
> +	b	__local_bh_enable_ip
>   	.endm
>   
>   #define USERL(l, x...)				\
> diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
> index 9a89264cdcc0b46e..9555c0a1c46fd47b 100644
> --- a/arch/arm/vfp/entry.S
> +++ b/arch/arm/vfp/entry.S
> @@ -22,7 +22,23 @@
>   @  IRQs enabled.
>   @
>   ENTRY(do_vfp)
> -	local_bh_disable r10, r4
> +#if defined(CONFIG_PREEMPT_RT) || defined(CONFIG_TRACE_IRQFLAGS)
> +	mov	r4, r0			@ stash r0, r2, lr
> +	mov	r5, r2
> +	mov	r6, lr
> +
> +	adr	r0, .
> +	mov	r1, #SOFTIRQ_DISABLE_OFFSET
> +	bl	__local_bh_disable_ip
> +
> +	mov	r0, r4			@ restore r0, r2, lr
> +	mov	r2, r5
> +	mov	lr, r6
> +#else
> +	ldr	r4, [r10, #TI_PREEMPT]
> +	add	r4, r4, #SOFTIRQ_DISABLE_OFFSET
> +	str	r4, [r10, #TI_PREEMPT]
> +#endif
>    	ldr	r4, .LCvfp
>   	ldr	r11, [r10, #TI_CPU]	@ CPU number
>   	add	r10, r10, #TI_VFPSTATE	@ r10 = workspace
> @@ -30,8 +46,7 @@ ENTRY(do_vfp)
>   ENDPROC(do_vfp)
>   
>   ENTRY(vfp_null_entry)
> -	local_bh_enable_ti r10, r4
> -	ret	lr
> +	local_bh_enable_and_ret		@ tail call
>   ENDPROC(vfp_null_entry)
>   
>   	.align	2
> diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
> index 26c4f61ecfa39638..84523de8bf059ce8 100644
> --- a/arch/arm/vfp/vfphw.S
> +++ b/arch/arm/vfp/vfphw.S
> @@ -175,8 +175,9 @@ vfp_hw_state_valid:
>   					@ else it's one 32-bit instruction, so
>   					@ always subtract 4 from the following
>   					@ instruction address.
> -	local_bh_enable_ti r10, r4
> -	ret	r9			@ we think we have handled things
> +
> +	mov	lr, r9			@ we think we have handled things
> +	local_bh_enable_and_ret		@ tail call
>   
>   
>   look_for_VFP_exceptions:
> @@ -200,8 +201,7 @@ skip:
>   	@ not recognised by VFP
>   
>   	DBGSTR	"not VFP"
> -	local_bh_enable_ti r10, r4
> -	ret	lr
> +	local_bh_enable_and_ret		@ tail call
>   
>   process_exception:
>   	DBGSTR	"bounce"