| Message ID | 20230314192756.217966-4-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: vfp: Switch to C API to en/disable softirqs | expand |
Hi Ard, On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> 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. Not good. > Set let's rework the VFP entry code a little bit so we can make the > local_bh_disable() call from C, with all the instrumentations that > happen to have been configured. Calling local_bh_enable() can be done > from asm, as it is always a callable function. Here it says local_bh_enable() is called (...) > +local_bh_enable_and_ret: > + adr r0, . > + mov r1, #SOFTIRQ_DISABLE_OFFSET > + b __local_bh_enable_ip @ tail call Please add a comment here that this is an open coded version of local_bh_enable() from the <linux/bottom_half.h> header file and we hope that inline code will not change. I.e this: static inline void local_bh_enable(void) { __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); } I almost want to make a warning comment into <linux/bottom_half.h> that the same function exists in an open coded version in arch/arm/vfp/vfphw.S so this static inline cannot be refactored without consequences. Either way: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> The kernel definitely looks better after this than before and it fixes a bug/bugs. Yours, Linus Walleij
On Tue, Mar 14, 2023 at 09:46:21PM +0100, Linus Walleij wrote: > Hi Ard, > > On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> 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. > > Not good. > > > Set let's rework the VFP entry code a little bit so we can make the > > local_bh_disable() call from C, with all the instrumentations that > > happen to have been configured. Calling local_bh_enable() can be done > > from asm, as it is always a callable function. > > Here it says local_bh_enable() is called > > (...) > > > +local_bh_enable_and_ret: > > + adr r0, . > > + mov r1, #SOFTIRQ_DISABLE_OFFSET > > + b __local_bh_enable_ip @ tail call > > Please add a comment here that this is an open coded version of > local_bh_enable() from the <linux/bottom_half.h> header file > and we hope that inline code will not change. > > I.e this: > > static inline void local_bh_enable(void) > { > __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > } > > I almost want to make a warning comment into <linux/bottom_half.h> > that the same function exists in an open coded version in arch/arm/vfp/vfphw.S > so this static inline cannot be refactored without consequences. So we could do something like the below to make local_bh_enable() both an inline and an actual symbol for those who need to call it from asm. It seems to compile on both GCC-12 and Clang-15. diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h index fc53e0ad56d9..85fcdf647f9f 100644 --- a/include/linux/bottom_half.h +++ b/include/linux/bottom_half.h @@ -28,7 +28,7 @@ static inline void local_bh_enable_ip(unsigned long ip) __local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET); } -static inline void local_bh_enable(void) +extern inline void local_bh_enable(void) { __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); } diff --git a/kernel/softirq.c b/kernel/softirq.c index c8a6913c067d..91d8677f890a 100644 --- a/kernel/softirq.c +++ b/kernel/softirq.c @@ -1010,3 +1010,8 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from) { return from; } + +void local_bh_enable(void) +{ + __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); +}
On Tue, 14 Mar 2023 at 22:46, Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Mar 14, 2023 at 09:46:21PM +0100, Linus Walleij wrote: > > Hi Ard, > > > > On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> 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. > > > > Not good. > > > > > Set let's rework the VFP entry code a little bit so we can make the > > > local_bh_disable() call from C, with all the instrumentations that > > > happen to have been configured. Calling local_bh_enable() can be done > > > from asm, as it is always a callable function. > > > > Here it says local_bh_enable() is called > > > > (...) > > > > > +local_bh_enable_and_ret: > > > + adr r0, . > > > + mov r1, #SOFTIRQ_DISABLE_OFFSET > > > + b __local_bh_enable_ip @ tail call > > > > Please add a comment here that this is an open coded version of > > local_bh_enable() from the <linux/bottom_half.h> header file > > and we hope that inline code will not change. > > > > I.e this: > > > > static inline void local_bh_enable(void) > > { > > __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > > } > > > > I almost want to make a warning comment into <linux/bottom_half.h> > > that the same function exists in an open coded version in arch/arm/vfp/vfphw.S > > so this static inline cannot be refactored without consequences. > > So we could do something like the below to make local_bh_enable() both > an inline and an actual symbol for those who need to call it from asm. > > It seems to compile on both GCC-12 and Clang-15. > > diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h > index fc53e0ad56d9..85fcdf647f9f 100644 > --- a/include/linux/bottom_half.h > +++ b/include/linux/bottom_half.h > @@ -28,7 +28,7 @@ static inline void local_bh_enable_ip(unsigned long ip) > __local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET); > } > > -static inline void local_bh_enable(void) > +extern inline void local_bh_enable(void) > { > __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > } > diff --git a/kernel/softirq.c b/kernel/softirq.c > index c8a6913c067d..91d8677f890a 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -1010,3 +1010,8 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from) > { > return from; > } > + > +void local_bh_enable(void) > +{ > + __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > +} That would be much better, yes. These fixes ultimately need to be backported to v6.2 so I'm not sure if we want to include this now, or keep it as a cleanup for the next cycle?
On Tue, 14 Mar 2023 at 23:25, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 14 Mar 2023 at 22:46, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Tue, Mar 14, 2023 at 09:46:21PM +0100, Linus Walleij wrote: > > > Hi Ard, > > > > > > On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> 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. > > > > > > Not good. > > > > > > > Set let's rework the VFP entry code a little bit so we can make the > > > > local_bh_disable() call from C, with all the instrumentations that > > > > happen to have been configured. Calling local_bh_enable() can be done > > > > from asm, as it is always a callable function. > > > > > > Here it says local_bh_enable() is called > > > > > > (...) > > > > > > > +local_bh_enable_and_ret: > > > > + adr r0, . > > > > + mov r1, #SOFTIRQ_DISABLE_OFFSET > > > > + b __local_bh_enable_ip @ tail call > > > > > > Please add a comment here that this is an open coded version of > > > local_bh_enable() from the <linux/bottom_half.h> header file > > > and we hope that inline code will not change. > > > > > > I.e this: > > > > > > static inline void local_bh_enable(void) > > > { > > > __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > > > } > > > > > > I almost want to make a warning comment into <linux/bottom_half.h> > > > that the same function exists in an open coded version in arch/arm/vfp/vfphw.S > > > so this static inline cannot be refactored without consequences. > > > > So we could do something like the below to make local_bh_enable() both > > an inline and an actual symbol for those who need to call it from asm. > > > > It seems to compile on both GCC-12 and Clang-15. > > > > diff --git a/include/linux/bottom_half.h b/include/linux/bottom_half.h > > index fc53e0ad56d9..85fcdf647f9f 100644 > > --- a/include/linux/bottom_half.h > > +++ b/include/linux/bottom_half.h > > @@ -28,7 +28,7 @@ static inline void local_bh_enable_ip(unsigned long ip) > > __local_bh_enable_ip(ip, SOFTIRQ_DISABLE_OFFSET); > > } > > > > -static inline void local_bh_enable(void) > > +extern inline void local_bh_enable(void) > > { > > __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > > } > > diff --git a/kernel/softirq.c b/kernel/softirq.c > > index c8a6913c067d..91d8677f890a 100644 > > --- a/kernel/softirq.c > > +++ b/kernel/softirq.c > > @@ -1010,3 +1010,8 @@ unsigned int __weak arch_dynirq_lower_bound(unsigned int from) > > { > > return from; > > } > > + > > +void local_bh_enable(void) > > +{ > > + __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET); > > +} > > That would be much better, yes. > > These fixes ultimately need to be backported to v6.2 so I'm not sure > if we want to include this now, or keep it as a cleanup for the next > cycle? Actually, I ended up going a bit further than this, and reimplemented vfp_support_entry() in C entirely. I will add that as a patch to the next revision, but that one should not be backported.
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h index 06b48ce23e1ca245..505a306e0271a9c4 100644 --- a/arch/arm/include/asm/assembler.h +++ b/arch/arm/include/asm/assembler.h @@ -244,19 +244,6 @@ 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] - .endm - #define USERL(l, x...) \ 9999: x; \ .pushsection __ex_table,"a"; \ diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S index 6dabb47617781a5f..7483ef8bccda394c 100644 --- a/arch/arm/vfp/entry.S +++ b/arch/arm/vfp/entry.S @@ -24,14 +24,5 @@ ENTRY(do_vfp) mov r1, r10 mov r3, r9 - ldr r4, .LCvfp - ldr pc, [r4] @ call VFP entry point + b vfp_entry ENDPROC(do_vfp) - -ENTRY(vfp_null_entry) - ret lr -ENDPROC(vfp_null_entry) - - .align 2 -.LCvfp: - .word vfp_vector diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S index 60acd42e05786e95..4d8478264d82b3d2 100644 --- a/arch/arm/vfp/vfphw.S +++ b/arch/arm/vfp/vfphw.S @@ -75,8 +75,6 @@ @ lr = unrecognised instruction return address @ IRQs enabled. ENTRY(vfp_support_entry) - local_bh_disable r1, r4 - ldr r11, [r1, #TI_CPU] @ CPU number add r10, r1, #TI_VFPSTATE @ r10 = workspace @@ -179,9 +177,12 @@ 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 r3 @ we think we have handled things + mov lr, r3 @ we think we have handled things +local_bh_enable_and_ret: + adr r0, . + mov r1, #SOFTIRQ_DISABLE_OFFSET + b __local_bh_enable_ip @ tail call look_for_VFP_exceptions: @ Check for synchronous or asynchronous exception @@ -204,8 +205,7 @@ skip: @ not recognised by VFP DBGSTR "not VFP" - local_bh_enable_ti r10, r4 - ret lr + b local_bh_enable_and_ret process_exception: DBGSTR "bounce" diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c index 01bc48d738478142..0f32c15d3c96b5ac 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -32,10 +32,28 @@ /* * Our undef handlers (in entry.S) */ -asmlinkage void vfp_support_entry(void); -asmlinkage void vfp_null_entry(void); +asmlinkage void vfp_support_entry(u32, void *, u32, u32); -asmlinkage void (*vfp_vector)(void) = vfp_null_entry; +static bool have_vfp __ro_after_init; + +/* + * Entered with: + * + * r0 = instruction opcode (32-bit ARM or two 16-bit Thumb) + * r1 = thread_info pointer + * r2 = PC value to resume execution after successful emulation + * r3 = normal "successful" return address + * lr = unrecognised instruction return address + */ +asmlinkage void vfp_entry(u32 opcode, struct thread_info *ti, u32 resume_pc, + u32 resume_return_address) +{ + if (unlikely(!have_vfp)) + return; + + local_bh_disable(); + vfp_support_entry(opcode, ti, resume_pc, resume_return_address); +} /* * Dual-use variable. @@ -798,7 +816,6 @@ static int __init vfp_init(void) vfpsid = fmrx(FPSID); barrier(); unregister_undef_hook(&vfp_detect_hook); - vfp_vector = vfp_null_entry; pr_info("VFP support v0.3: "); if (VFP_arch) { @@ -883,7 +900,7 @@ static int __init vfp_init(void) "arm/vfp:starting", vfp_starting_cpu, vfp_dying_cpu); - vfp_vector = vfp_support_entry; + have_vfp = true; thread_register_notifier(&vfp_notifier_block); vfp_pm_init();
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. Set let's rework the VFP entry code a little bit so we can make the local_bh_disable() call from C, with all the instrumentations that happen to have been configured. Calling local_bh_enable() can be done from asm, as it is always a callable function. 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 | 13 ---------- arch/arm/vfp/entry.S | 11 +------- arch/arm/vfp/vfphw.S | 12 ++++----- arch/arm/vfp/vfpmodule.c | 27 ++++++++++++++++---- 4 files changed, 29 insertions(+), 34 deletions(-)