| Message ID | 20230320131845.3138015-4-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: vfp: Switch to C API to en/disable softirqs | expand |
On 20.03.23 14:18, 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. > > 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 a simple wrapper around __local_bh_enable_ip(), which > 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> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Tested-by: Guenter Roeck <linux@roeck-us.net> > [...] It feels a lot like I'm missing something, but I can't figure out what, I thus have to ask: What happened to this patch and/or the series? From my very limited viewpoint it looks like there was no progress for nearly three week now. Reminder, 62b95a7b44d1 is a commit from 6.3 cycle and fixing a problem Guenter ran into. Hence it to me looks like it's something that better would be fixed this cycle. Or is this considered too dangerous for some reason and thus need to wait? But even then I wonder a little bit why it's not in next by now. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr If I did something stupid, please tell me, as explained on that page
On 4/9/23 07:29, Linux regression tracking (Thorsten Leemhuis) wrote: > On 20.03.23 14:18, 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. >> >> 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 a simple wrapper around __local_bh_enable_ip(), which >> 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> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Tested-by: Guenter Roeck <linux@roeck-us.net> >> [...] > > It feels a lot like I'm missing something, but I can't figure out what, > I thus have to ask: > > What happened to this patch and/or the series? From my very limited > viewpoint it looks like there was no progress for nearly three week now. > > Reminder, 62b95a7b44d1 is a commit from 6.3 cycle and fixing a problem > Guenter ran into. Hence it to me looks like it's something that better > would be fixed this cycle. Or is this considered too dangerous for some > reason and thus need to wait? But even then I wonder a little bit why > it's not in next by now. > Something for sure has changed. I can no longer reproduce the problem with the latest kernel (v6.3-rc6). I can still reproduce it with v6.3-rc2. Further testing shows that it is no longer reproducible for me with v6.3-rc3. That made me curious. I am currently bisecting, but that will take a while. Guenter
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..4c7473d2f89a040f 100644 --- a/arch/arm/vfp/vfpmodule.c +++ b/arch/arm/vfp/vfpmodule.c @@ -32,10 +32,9 @@ /* * 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; /* * Dual-use variable. @@ -669,6 +668,25 @@ static int vfp_kmode_exception(struct pt_regs *regs, unsigned int instr) return 1; } +/* + * 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 trigger, struct thread_info *ti, u32 resume_pc, + u32 resume_return_address) +{ + if (unlikely(!have_vfp)) + return; + + local_bh_disable(); + vfp_support_entry(trigger, ti, resume_pc, resume_return_address); +} + static struct undef_hook vfp_kmode_exception_hook[] = {{ .instr_mask = 0xfe000000, .instr_val = 0xf2000000, @@ -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 a simple wrapper around __local_bh_enable_ip(), which 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> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Tested-by: Guenter Roeck <linux@roeck-us.net> --- 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(-)