[PATCH v2 0/3] ARM: vfp: Switch to C API to en/disable softirqs

[PATCH v2 0/3] ARM: vfp: Switch to C API to en/disable softirqs

[Ard Biesheuvel]

As it turns out, enabling or disabling softirqs from asm code is more
tricky than it looks. Simply bumping the associated bit in preempt_count
does the trick for uninstrumented kernels, but with lockdep or preempt
debug enabled, we really need to call the C versions, as replicating
their behavior in asm fully is intractible.

So let's rework the existing code a little bit so we can interpose a
little C helper 'vfp_entry()' that disables softirqs before calling the
existing vfpstate handling code in asm. Re-enabling softirqs from asm
code is more straight-forward, as we can simply perform a tail call via
a C routine that is guaranteed to be callable as a function.

Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/all/ZBBYCSZUJOWBg1s8@localhost.localdomain/

Ard Biesheuvel (3):
  ARM: vfp: Pass thread_info pointer to vfp_support_entry
  ARM: vfp: Pass successful return address via register R3
  ARM: vfp: Fix broken softirq handling with instrumentation enabled

 arch/arm/include/asm/assembler.h | 13 ---------
 arch/arm/vfp/entry.S             | 17 ++---------
 arch/arm/vfp/vfphw.S             | 30 +++++++++++---------
 arch/arm/vfp/vfpmodule.c         | 27 ++++++++++++++----
 4 files changed, 42 insertions(+), 45 deletions(-)

-- 
2.39.2


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

[PATCH v2 1/3] ARM: vfp: Pass thread_info pointer to vfp_support_entry

[Ard Biesheuvel]

Instead of dereferencing thread_info in do_vfp, pass the thread_info
pointer to vfp_support_entry via R1. That way, we only use a single
caller save register, which makes it easier to convert do_vfp to C code
in a subsequent patch.

Note that, unlike the CPU number, which can change due to preemption,
passing the thread_info pointer can safely be done with preemption
enabled.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/vfp/entry.S |  5 +----
 arch/arm/vfp/vfphw.S | 10 +++++++---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
index 9a89264cdcc0b46e..cfedc2a3dbd68f1c 100644
--- a/arch/arm/vfp/entry.S
+++ b/arch/arm/vfp/entry.S
@@ -22,15 +22,12 @@
 @  IRQs enabled.
 @
 ENTRY(do_vfp)
-	local_bh_disable r10, r4
+	mov	r1, r10
  	ldr	r4, .LCvfp
-	ldr	r11, [r10, #TI_CPU]	@ CPU number
-	add	r10, r10, #TI_VFPSTATE	@ r10 = workspace
 	ldr	pc, [r4]		@ call VFP entry point
 ENDPROC(do_vfp)
 
 ENTRY(vfp_null_entry)
-	local_bh_enable_ti r10, r4
 	ret	lr
 ENDPROC(vfp_null_entry)
 
diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
index 26c4f61ecfa39638..6d056d810e4868c2 100644
--- a/arch/arm/vfp/vfphw.S
+++ b/arch/arm/vfp/vfphw.S
@@ -6,9 +6,9 @@
  *  Written by Deep Blue Solutions Limited.
  *
  * This code is called from the kernel's undefined instruction trap.
+ * r1 holds the thread_info pointer
  * r9 holds the return address for successful handling.
  * lr holds the return address for unrecognised instructions.
- * r10 points at the start of the private FP workspace in the thread structure
  * sp points to a struct pt_regs (as defined in include/asm/proc/ptrace.h)
  */
 #include <linux/init.h>
@@ -69,13 +69,17 @@
 @ VFP hardware support entry point.
 @
 @  r0  = instruction opcode (32-bit ARM or two 16-bit Thumb)
+@  r1  = thread_info pointer
 @  r2  = PC value to resume execution after successful emulation
 @  r9  = normal "successful" return address
-@  r10 = vfp_state union
-@  r11 = CPU number
 @  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
+
 	DBGSTR3	"instr %08x pc %08x state %p", r0, r2, r10
 
 	.fpu	vfpv2
-- 
2.39.2


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

[PATCH v2 2/3] ARM: vfp: Pass successful return address via register R3

[Ard Biesheuvel]

In preparation for reimplementing the do_vfp()->vfp_support_entry()
handover in C code, switch to using R3 to pass the 'success' return
address, rather than R9, as it cannot be used for parameter passing.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/vfp/entry.S |  1 +
 arch/arm/vfp/vfphw.S | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/vfp/entry.S b/arch/arm/vfp/entry.S
index cfedc2a3dbd68f1c..6dabb47617781a5f 100644
--- a/arch/arm/vfp/entry.S
+++ b/arch/arm/vfp/entry.S
@@ -23,6 +23,7 @@
 @
 ENTRY(do_vfp)
 	mov	r1, r10
+	mov	r3, r9
  	ldr	r4, .LCvfp
 	ldr	pc, [r4]		@ call VFP entry point
 ENDPROC(do_vfp)
diff --git a/arch/arm/vfp/vfphw.S b/arch/arm/vfp/vfphw.S
index 6d056d810e4868c2..60acd42e05786e95 100644
--- a/arch/arm/vfp/vfphw.S
+++ b/arch/arm/vfp/vfphw.S
@@ -7,7 +7,7 @@
  *
  * This code is called from the kernel's undefined instruction trap.
  * r1 holds the thread_info pointer
- * r9 holds the return address for successful handling.
+ * r3 holds the return address for successful handling.
  * lr holds the return address for unrecognised instructions.
  * sp points to a struct pt_regs (as defined in include/asm/proc/ptrace.h)
  */
@@ -71,7 +71,7 @@
 @  r0  = instruction opcode (32-bit ARM or two 16-bit Thumb)
 @  r1  = thread_info pointer
 @  r2  = PC value to resume execution after successful emulation
-@  r9  = normal "successful" return address
+@  r3  = normal "successful" return address
 @  lr  = unrecognised instruction return address
 @  IRQs enabled.
 ENTRY(vfp_support_entry)
@@ -89,9 +89,9 @@ ENTRY(vfp_support_entry)
 	bne	look_for_VFP_exceptions	@ VFP is already enabled
 
 	DBGSTR1 "enable %x", r10
-	ldr	r3, vfp_current_hw_state_address
+	ldr	r9, vfp_current_hw_state_address
 	orr	r1, r1, #FPEXC_EN	@ user FPEXC has the enable bit set
-	ldr	r4, [r3, r11, lsl #2]	@ vfp_current_hw_state pointer
+	ldr	r4, [r9, r11, lsl #2]	@ vfp_current_hw_state pointer
 	bic	r5, r1, #FPEXC_EX	@ make sure exceptions are disabled
 	cmp	r4, r10			@ this thread owns the hw context?
 #ifndef CONFIG_SMP
@@ -150,7 +150,7 @@ vfp_reload_hw:
 #endif
 
 	DBGSTR1	"load state %p", r10
-	str	r10, [r3, r11, lsl #2]	@ update the vfp_current_hw_state pointer
+	str	r10, [r9, r11, lsl #2]	@ update the vfp_current_hw_state pointer
 					@ Load the saved state back into the VFP
 	VFPFLDMIA r10, r5		@ reload the working registers while
 					@ FPEXC is in a safe state
@@ -180,7 +180,7 @@ vfp_hw_state_valid:
 					@ always subtract 4 from the following
 					@ instruction address.
 	local_bh_enable_ti r10, r4
-	ret	r9			@ we think we have handled things
+	ret	r3			@ we think we have handled things
 
 
 look_for_VFP_exceptions:
@@ -210,7 +210,7 @@ skip:
 process_exception:
 	DBGSTR	"bounce"
 	mov	r2, sp			@ nothing stacked - regdump is at TOS
-	mov	lr, r9			@ setup for a return to the user code.
+	mov	lr, r3			@ setup for a return to the user code.
 
 	@ Now call the C code to package up the bounce to the support code
 	@   r0 holds the trigger instruction
-- 
2.39.2


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

[PATCH v2 3/3] 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.

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(-)

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();
-- 
2.39.2


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

Re: [PATCH v2 1/3] ARM: vfp: Pass thread_info pointer to vfp_support_entry

[Linus Walleij]

On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> Instead of dereferencing thread_info in do_vfp, pass the thread_info
> pointer to vfp_support_entry via R1. That way, we only use a single
> caller save register, which makes it easier to convert do_vfp to C code
> in a subsequent patch.
>
> Note that, unlike the CPU number, which can change due to preemption,
> passing the thread_info pointer can safely be done with preemption
> enabled.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Looks correct to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

Re: [PATCH v2 2/3] ARM: vfp: Pass successful return address via register R3

[Linus Walleij]

On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> In preparation for reimplementing the do_vfp()->vfp_support_entry()
> handover in C code, switch to using R3 to pass the 'success' return
> address, rather than R9, as it cannot be used for parameter passing.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

Re: [PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Linus Walleij]

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

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

Re: [PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Peter Zijlstra]

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);
+}

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

Re: [PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Ard Biesheuvel]

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?

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

Re: [PATCH v2 0/3] ARM: vfp: Switch to C API to en/disable softirqs

[Guenter Roeck]

On Tue, Mar 14, 2023 at 08:27:53PM +0100, Ard Biesheuvel wrote:
> As it turns out, enabling or disabling softirqs from asm code is more
> tricky than it looks. Simply bumping the associated bit in preempt_count
> does the trick for uninstrumented kernels, but with lockdep or preempt
> debug enabled, we really need to call the C versions, as replicating
> their behavior in asm fully is intractible.
> 
> So let's rework the existing code a little bit so we can interpose a
> little C helper 'vfp_entry()' that disables softirqs before calling the
> existing vfpstate handling code in asm. Re-enabling softirqs from asm
> code is more straight-forward, as we can simply perform a tail call via
> a C routine that is guaranteed to be callable as a function.
> 
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Link: https://lore.kernel.org/all/ZBBYCSZUJOWBg1s8@localhost.localdomain/
> 

For the series:

Tested-by: Guenter Roeck <linux@roeck-us.net>

Series applied on top of v6.3-rc2. Tested with 300+ boot tests on all
arm qemu machines in my test bed.

Cross-check: Without this series, a warning backtrace is observed
roughly once per ~20 boot attempts.

Thanks,
Guenter

> Ard Biesheuvel (3):
>   ARM: vfp: Pass thread_info pointer to vfp_support_entry
>   ARM: vfp: Pass successful return address via register R3
>   ARM: vfp: Fix broken softirq handling with instrumentation enabled
> 
>  arch/arm/include/asm/assembler.h | 13 ---------
>  arch/arm/vfp/entry.S             | 17 ++---------
>  arch/arm/vfp/vfphw.S             | 30 +++++++++++---------
>  arch/arm/vfp/vfpmodule.c         | 27 ++++++++++++++----
>  4 files changed, 42 insertions(+), 45 deletions(-)
> 
> -- 
> 2.39.2
> 

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

Re: [PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Ard Biesheuvel]

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.

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