| Message ID | 20230314125743.4165575-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFT] ARM: vfp: Fix broken softirq handling with instrumentation enabled | expand |
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. >
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.
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"
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" >
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"
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(-)