[RFT PATCH] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Ard Biesheuvel]

On Tue, 14 Mar 2023 at 17:21, Guenter Roeck <linux at 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 at kernel.org>
> > Cc: Guenter Roeck <linux at roeck-us.net>
> > Cc: Peter Zijlstra <peterz at 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 at 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"
>