Re: [PATCH v4 1/4] arm64: fpsimd: Drop unneeded 'busy' flag

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 12 Dec 2023 at 13:34, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Will,
>
> On Tue, Dec 12, 2023 at 1:30 PM Will Deacon <will@xxxxxxxxxx> wrote:
> > On Tue, Dec 12, 2023 at 12:27:50PM +0100, Geert Uytterhoeven wrote:
> > > On Fri, Dec 8, 2023 at 12:34 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > > From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > Kernel mode NEON will preserve the user mode FPSIMD state by saving it
> > > > into the task struct before clobbering the registers. In order to avoid
> > > > the need for preserving kernel mode state too, we disallow nested use of
> > > > kernel mode NEON, i..e, use in softirq context while the interrupted
> > > > task context was using kernel mode NEON too.
> > > >
> > > > Originally, this policy was implemented using a per-CPU flag which was
> > > > exposed via may_use_simd(), requiring the users of the kernel mode NEON
> > > > to deal with the possibility that it might return false, and having NEON
> > > > and non-NEON code paths. This policy was changed by commit
> > > > 13150149aa6ded1 ("arm64: fpsimd: run kernel mode NEON with softirqs
> > > > disabled"), and now, softirq processing is disabled entirely instead,
> > > > and so may_use_simd() can never fail when called from task or softirq
> > > > context.
> > > >
> > > > This means we can drop the fpsimd_context_busy flag entirely, and
> > > > instead, ensure that we disable softirq processing in places where we
> > > > formerly relied on the flag for preventing races in the FPSIMD preserve
> > > > routines.
> > > >
> > > > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > Reviewed-by: Mark Brown <broonie@xxxxxxxxxx>
> > >
> > > Thanks for your patch, which is now commit e109130b0e5ec3fd
> > > ("arm64: fpsimd: Drop unneeded 'busy' flag") in arm64/for-next/core
> > > and next-20231212.
> > >
> > > I have bisected the following warning during boot (on Salvator-XS with
> > > R-Car H3 ES2.0 and on White-Hawk with R-Car V4H) followed by a lock-up
> > > (on Salvator-XS) to this commit:
> >
> > Please can you provide the output from the warning and, if possible, a
>
> Oops, how did that log disappear?
>
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at kernel/softirq.c:361 __local_bh_enable_ip+0x1a4/0x1c8
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 6.7.0-rc3-arm64-renesas-00001-ge109130b0e5e #2383
> Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : __local_bh_enable_ip+0x1a4/0x1c8
> lr : fpsimd_save_and_flush_cpu_state+0x4c/0x58
> sp : ffff800081703bb0
> x29: ffff800081703bb0 x28: ffff80008171c6b0 x27: ffff8000800167fc
> x26: 0000000000000000 x25: 0000000000000000 x24: ffff800081710a68
> x23: ffff8000813c4008 x22: ffff800081703ca4 x21: 0000000000000000
> x20: ffff8000800167c8 x19: 0000000000000200 x18: ffffffffffffffff
> x17: ffff0004c27f2400 x16: 0000000000000001 x15: 0000000000000003
> x14: 0000000000000000 x13: 0000000000000003 x12: 0000000000000000
> x11: 071c71c71c71c71c x10: ffff800082086b88 x9 : 0000000000000000
> x8 : ffff80008203ab50 x7 : 0000000000000bb0 x6 : ffff80008203b700
> x5 : ffff80067e2ee000 x4 : 0000000000000202 x3 : ffff80067e2ee000
> x2 : ffff80067e2ee000 x1 : ffff800081719fc0 x0 : 0000000100000202
> Call trace:
>  __local_bh_enable_ip+0x1a4/0x1c8
>  fpsimd_save_and_flush_cpu_state+0x4c/0x58
>  fpsimd_cpu_pm_notifier+0x1c/0x28
>  notifier_call_chain+0x9c/0x174
>  raw_notifier_call_chain_robust+0x40/0x98
>  cpu_pm_enter+0x3c/0x70
>  psci_enter_idle_state+0x28/0x78
>  cpuidle_enter_state+0xe4/0x568
>  cpuidle_enter+0x34/0x48
>  do_idle+0x214/0x290

Thanks for the report. I missed the fact that this is called from the
idle path. The old code just set and cleared the 'busy' flag there

Could you please check whether this fixes the issue? Thanks.

--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1835,13 +1835,15 @@ static void fpsimd_flush_cpu_state(void)
  */
 void fpsimd_save_and_flush_cpu_state(void)
 {
+       unsigned long flags;
+
        if (!system_supports_fpsimd())
                return;
        WARN_ON(preemptible());
-       get_cpu_fpsimd_context();
+       local_irq_save(flags);
        fpsimd_save_user_state();
        fpsimd_flush_cpu_state();
-       put_cpu_fpsimd_context();
+       local_irq_restore(flags);
 }

 #ifdef CONFIG_KERNEL_MODE_NEON





[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux