* Re: Patch "arm64: fpsimd: Drop unneeded 'busy' flag" has been added to the 6.6-stable tree
[not found] <20240530191110.24847-1-sashal@kernel.org>
@ 2024-05-31 6:16 ` Ard Biesheuvel
2024-06-06 13:10 ` Greg KH
0 siblings, 1 reply; 4+ messages in thread
From: Ard Biesheuvel @ 2024-05-31 6:16 UTC (permalink / raw)
To: stable; +Cc: stable-commits, Catalin Marinas, Will Deacon
On Thu, 30 May 2024 at 21:11, Sasha Levin <sashal@kernel.org> wrote:
>
> This is a note to let you know that I've just added the patch titled
>
> arm64: fpsimd: Drop unneeded 'busy' flag
>
> to the 6.6-stable tree
Why?
> which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
> arm64-fpsimd-drop-unneeded-busy-flag.patch
> and it can be found in the queue-6.6 subdirectory.
>
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
>
>
>
> commit 37f2773a1ef05374538d5e4ed26cbacebe363241
> Author: Ard Biesheuvel <ardb@kernel.org>
> Date: Fri Dec 8 12:32:20 2023 +0100
>
> arm64: fpsimd: Drop unneeded 'busy' flag
>
> [ Upstream commit 9b19700e623f96222c69ecb2adecb1a3e3664cc0 ]
>
> 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@kernel.org>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Link: https://lore.kernel.org/r/20231208113218.3001940-7-ardb@google.com
> [will: Folded in fix from CAMj1kXFhzbJRyWHELCivQW1yJaF=p07LLtbuyXYX3G1WtsdyQg@mail.gmail.com]
> Signed-off-by: Will Deacon <will@kernel.org>
> Stable-dep-of: b8995a184170 ("Revert "arm64: fpsimd: Implement lazy restore for kernel mode FPSIMD"")
> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
> diff --git a/arch/arm64/include/asm/simd.h b/arch/arm64/include/asm/simd.h
> index 6a75d7ecdcaa2..8e86c9e70e483 100644
> --- a/arch/arm64/include/asm/simd.h
> +++ b/arch/arm64/include/asm/simd.h
> @@ -12,8 +12,6 @@
> #include <linux/preempt.h>
> #include <linux/types.h>
>
> -DECLARE_PER_CPU(bool, fpsimd_context_busy);
> -
> #ifdef CONFIG_KERNEL_MODE_NEON
>
> /*
> @@ -28,17 +26,10 @@ static __must_check inline bool may_use_simd(void)
> /*
> * We must make sure that the SVE has been initialized properly
> * before using the SIMD in kernel.
> - * fpsimd_context_busy is only set while preemption is disabled,
> - * and is clear whenever preemption is enabled. Since
> - * this_cpu_read() is atomic w.r.t. preemption, fpsimd_context_busy
> - * cannot change under our feet -- if it's set we cannot be
> - * migrated, and if it's clear we cannot be migrated to a CPU
> - * where it is set.
> */
> return !WARN_ON(!system_capabilities_finalized()) &&
> system_supports_fpsimd() &&
> - !in_hardirq() && !irqs_disabled() && !in_nmi() &&
> - !this_cpu_read(fpsimd_context_busy);
> + !in_hardirq() && !irqs_disabled() && !in_nmi();
> }
>
> #else /* ! CONFIG_KERNEL_MODE_NEON */
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 5cdfcc9e3e54b..b805bdab284c4 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -85,13 +85,13 @@
> * softirq kicks in. Upon vcpu_put(), KVM will save the vcpu FP state and
> * flag the register state as invalid.
> *
> - * In order to allow softirq handlers to use FPSIMD, kernel_neon_begin() may
> - * save the task's FPSIMD context back to task_struct from softirq context.
> - * To prevent this from racing with the manipulation of the task's FPSIMD state
> - * from task context and thereby corrupting the state, it is necessary to
> - * protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE
> - * flag with {, __}get_cpu_fpsimd_context(). This will still allow softirqs to
> - * run but prevent them to use FPSIMD.
> + * In order to allow softirq handlers to use FPSIMD, kernel_neon_begin() may be
> + * called from softirq context, which will save the task's FPSIMD context back
> + * to task_struct. To prevent this from racing with the manipulation of the
> + * task's FPSIMD state from task context and thereby corrupting the state, it
> + * is necessary to protect any manipulation of a task's fpsimd_state or
> + * TIF_FOREIGN_FPSTATE flag with get_cpu_fpsimd_context(), which will suspend
> + * softirq servicing entirely until put_cpu_fpsimd_context() is called.
> *
> * For a certain task, the sequence may look something like this:
> * - the task gets scheduled in; if both the task's fpsimd_cpu field
> @@ -209,27 +209,14 @@ static inline void sme_free(struct task_struct *t) { }
>
> #endif
>
> -DEFINE_PER_CPU(bool, fpsimd_context_busy);
> -EXPORT_PER_CPU_SYMBOL(fpsimd_context_busy);
> -
> static void fpsimd_bind_task_to_cpu(void);
>
> -static void __get_cpu_fpsimd_context(void)
> -{
> - bool busy = __this_cpu_xchg(fpsimd_context_busy, true);
> -
> - WARN_ON(busy);
> -}
> -
> /*
> * Claim ownership of the CPU FPSIMD context for use by the calling context.
> *
> * The caller may freely manipulate the FPSIMD context metadata until
> * put_cpu_fpsimd_context() is called.
> *
> - * The double-underscore version must only be called if you know the task
> - * can't be preempted.
> - *
> * On RT kernels local_bh_disable() is not sufficient because it only
> * serializes soft interrupt related sections via a local lock, but stays
> * preemptible. Disabling preemption is the right choice here as bottom
> @@ -242,14 +229,6 @@ static void get_cpu_fpsimd_context(void)
> local_bh_disable();
> else
> preempt_disable();
> - __get_cpu_fpsimd_context();
> -}
> -
> -static void __put_cpu_fpsimd_context(void)
> -{
> - bool busy = __this_cpu_xchg(fpsimd_context_busy, false);
> -
> - WARN_ON(!busy); /* No matching get_cpu_fpsimd_context()? */
> }
>
> /*
> @@ -261,18 +240,12 @@ static void __put_cpu_fpsimd_context(void)
> */
> static void put_cpu_fpsimd_context(void)
> {
> - __put_cpu_fpsimd_context();
> if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> local_bh_enable();
> else
> preempt_enable();
> }
>
> -static bool have_cpu_fpsimd_context(void)
> -{
> - return !preemptible() && __this_cpu_read(fpsimd_context_busy);
> -}
> -
> unsigned int task_get_vl(const struct task_struct *task, enum vec_type type)
> {
> return task->thread.vl[type];
> @@ -383,7 +356,7 @@ static void task_fpsimd_load(void)
> bool restore_ffr;
>
> WARN_ON(!system_supports_fpsimd());
> - WARN_ON(!have_cpu_fpsimd_context());
> + WARN_ON(preemptible());
>
> if (system_supports_sve() || system_supports_sme()) {
> switch (current->thread.fp_type) {
> @@ -467,7 +440,7 @@ static void fpsimd_save(void)
> unsigned int vl;
>
> WARN_ON(!system_supports_fpsimd());
> - WARN_ON(!have_cpu_fpsimd_context());
> + WARN_ON(preemptible());
>
> if (test_thread_flag(TIF_FOREIGN_FPSTATE))
> return;
> @@ -1583,7 +1556,7 @@ void fpsimd_thread_switch(struct task_struct *next)
> if (!system_supports_fpsimd())
> return;
>
> - __get_cpu_fpsimd_context();
> + WARN_ON_ONCE(!irqs_disabled());
>
> /* Save unsaved fpsimd state, if any: */
> fpsimd_save();
> @@ -1599,8 +1572,6 @@ void fpsimd_thread_switch(struct task_struct *next)
>
> update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
> wrong_task || wrong_cpu);
> -
> - __put_cpu_fpsimd_context();
> }
>
> static void fpsimd_flush_thread_vl(enum vec_type type)
> @@ -1892,13 +1863,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();
> fpsimd_flush_cpu_state();
> - __put_cpu_fpsimd_context();
> + local_irq_restore(flags);
> }
>
> #ifdef CONFIG_KERNEL_MODE_NEON
^ permalink raw reply [flat|nested] 4+ messages in thread