* [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled
@ 2023-03-14 12:57 Ard Biesheuvel
2023-03-14 13:12 ` Frederic Weisbecker
2023-03-14 16:21 ` Guenter Roeck
0 siblings, 2 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2023-03-14 12:57 UTC (permalink / raw)
To: linux-arm-kernel, linux
Cc: Ard Biesheuvel, Frederic Weisbecker, Guenter Roeck,
Peter Zijlstra
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled
2023-03-14 12:57 [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled Ard Biesheuvel
@ 2023-03-14 13:12 ` Frederic Weisbecker
2023-03-14 13:58 ` Ard Biesheuvel
2023-03-14 16:21 ` Guenter Roeck
1 sibling, 1 reply; 5+ messages in thread
From: Frederic Weisbecker @ 2023-03-14 13:12 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-arm-kernel, linux, Guenter Roeck, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled
2023-03-14 13:12 ` Frederic Weisbecker
@ 2023-03-14 13:58 ` Ard Biesheuvel
0 siblings, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2023-03-14 13:58 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: linux-arm-kernel, linux, Guenter Roeck, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled
2023-03-14 12:57 [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled Ard Biesheuvel
2023-03-14 13:12 ` Frederic Weisbecker
@ 2023-03-14 16:21 ` Guenter Roeck
2023-03-14 17:36 ` Ard Biesheuvel
1 sibling, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2023-03-14 16:21 UTC (permalink / raw)
To: Ard Biesheuvel, linux-arm-kernel, linux
Cc: Frederic Weisbecker, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled
2023-03-14 16:21 ` Guenter Roeck
@ 2023-03-14 17:36 ` Ard Biesheuvel
0 siblings, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2023-03-14 17:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-arm-kernel, linux, Frederic Weisbecker, Peter Zijlstra
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-14 17:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-14 12:57 [RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled Ard Biesheuvel
2023-03-14 13:12 ` Frederic Weisbecker
2023-03-14 13:58 ` Ard Biesheuvel
2023-03-14 16:21 ` Guenter Roeck
2023-03-14 17:36 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox