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

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

[Ard Biesheuvel]

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.

Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Peter Zijlstra <peterz@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@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"
-- 
2.39.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Frederic Weisbecker]

Le Tue, Mar 14, 2023 at 01:57:43PM +0100, Ard Biesheuvel a écrit :
> 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]
> +#endi

I suggest you avoid taking any risk and unconditionally call
__local_bh_disable_ip(). You never know what will be added to softirq
APIs in the future.

For example you're missing the CONFIG_DEBUG_PREEMPT part.

Thanks.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Tue, 14 Mar 2023 at 14:12, Frederic Weisbecker <frederic@kernel.org> wrote:
>
> Le Tue, Mar 14, 2023 at 01:57:43PM +0100, Ard Biesheuvel a écrit :
> > 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]
> > +#endi
>
> I suggest you avoid taking any risk and unconditionally call
> __local_bh_disable_ip(). You never know what will be added to softirq
> APIs in the future.
>

That is not possible - __local_bh_disable_ip() is only a callable
function under the #ifdef condition, and a static inline otherwise.

> For example you're missing the CONFIG_DEBUG_PREEMPT part.
>

Yeah, so I'd have to call preempt_count_add() directly from asm
instead - that should work, although it becomes a bit of a kludge now.
I'll try to find a solution that is a bit cleaner.

Thanks,
Ard.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [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@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Peter Zijlstra <peterz@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@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"


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Tue, 14 Mar 2023 at 17:21, Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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 for testing.

I'll have a v2 out shortly, so please wait for that if you're up for
doing some more testing.

Thanks,
Ard.


> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Peter Zijlstra <peterz@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@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"
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel