| Message ID | 20240522091335.335346-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/fpsimd: Avoid erroneous elide of user state reload | expand |
Hej, On Wed, May 22, 2024, at 11:13, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > TIF_FOREIGN_FPSTATE is a 'convenience' flag that should reflect whether > the current CPU holds the most recent user mode FP/SIMD state of the > current task. It combines two conditions: > - whether the current CPU's FP/SIMD state belongs to the task; > - whether that state is the most recent associated with the task (as a > task may have executed on other CPUs as well). > > When a task is scheduled in and TIF_KERNEL_FPSTATE is set, it means the > task was in a kernel mode NEON section when it was scheduled out, and so > the kernel mode FP/SIMD state is restored. Since this implies that the > current CPU is *not* holding the most recent user mode FP/SIMD state of > the current task, the TIF_FOREIGN_FPSTATE flag is set too, so that the > user mode FP/SIMD state is reloaded from memory when returning to > userland. > > However, the task may be scheduled out after completing the kernel mode > NEON section, but before returning to userland. When this happens, the > TIF_FOREIGN_FPSTATE flag will not be preserved, but will be set as usual > the next time the task is scheduled in, and will be based on the above > conditions. > > This means that, rather than setting TIF_FOREIGN_FPSTATE when scheduling > in a task with TIF_KERNEL_FPSTATE set, the underlying state should be > updated so that TIF_FOREIGN_FPSTATE will assume the expected value as a > result. > > So instead, call fpsimd_flush_cpu_state(), which takes care of this. > > Closes: > https://lore.kernel.org/all/cb8822182231850108fa43e0446a4c7f@kernel.org > Reported-by: Johannes Nixdorf <mixi@shadowice.org> > Fixes: aefbab8e77eb ("arm64: fpsimd: Preserve/restore kernel mode NEON > at context switch") > Cc: Mark Brown <broonie@kernel.org> > Cc: Dave Martin <Dave.Martin@arm.com> > Cc: Janne Grunau <j@jannau.net> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm64/kernel/fpsimd.c | 44 +++++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 22 deletions(-) All previous errors no longer reproduce with this patch applied on top of v6.8. Over 20 repetitions of the fio reproducer without verification error, fp-stress mismatches and AV1 decoding errors. I'll continue to run the reproducer but I don't expect any failures. Previously at least one failure would occur for a single fio run. Please add Cc: stable@vger.kernel.org and feel free to add Tested-by: Janne Grunau <j@jannau.net> thanks Janne
This fixes the problem with my reproducer in 4/4 runs. Tested on top of
both master (29c73fc794c8) and the Asahi Linux kernel before the revert
(asahi-6.8.9-5).
Tested-by: Johannes Nixdorf <mixi@shadowice.org>
On Wed, May 22, 2024 at 11:13:36AM +0200, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > TIF_FOREIGN_FPSTATE is a 'convenience' flag that should reflect whether > the current CPU holds the most recent user mode FP/SIMD state of the > current task. It combines two conditions: > - whether the current CPU's FP/SIMD state belongs to the task; > - whether that state is the most recent associated with the task (as a > task may have executed on other CPUs as well). Reviewed-by: Mark Brown <broonie@kernel.org>
On Wed, 22 May 2024 11:13:36 +0200, Ard Biesheuvel wrote: > TIF_FOREIGN_FPSTATE is a 'convenience' flag that should reflect whether > the current CPU holds the most recent user mode FP/SIMD state of the > current task. It combines two conditions: > - whether the current CPU's FP/SIMD state belongs to the task; > - whether that state is the most recent associated with the task (as a > task may have executed on other CPUs as well). > > [...] Applied to arm64 (for-next/core), thanks! [1/1] arm64/fpsimd: Avoid erroneous elide of user state reload https://git.kernel.org/arm64/c/e92bee9f861b Cheers,
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index ebb0158997ca..82e8a6017382 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -1535,6 +1535,27 @@ static void fpsimd_save_kernel_state(struct task_struct *task) task->thread.kernel_fpsimd_cpu = smp_processor_id(); } +/* + * Invalidate any task's FPSIMD state that is present on this cpu. + * The FPSIMD context should be acquired with get_cpu_fpsimd_context() + * before calling this function. + */ +static void fpsimd_flush_cpu_state(void) +{ + WARN_ON(!system_supports_fpsimd()); + __this_cpu_write(fpsimd_last_state.st, NULL); + + /* + * Leaving streaming mode enabled will cause issues for any kernel + * NEON and leaving streaming mode or ZA enabled may increase power + * consumption. + */ + if (system_supports_sme()) + sme_smstop(); + + set_thread_flag(TIF_FOREIGN_FPSTATE); +} + void fpsimd_thread_switch(struct task_struct *next) { bool wrong_task, wrong_cpu; @@ -1552,7 +1573,7 @@ void fpsimd_thread_switch(struct task_struct *next) if (test_tsk_thread_flag(next, TIF_KERNEL_FPSTATE)) { fpsimd_load_kernel_state(next); - set_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE); + fpsimd_flush_cpu_state(); } else { /* * Fix up TIF_FOREIGN_FPSTATE to correctly describe next's @@ -1842,27 +1863,6 @@ void fpsimd_flush_task_state(struct task_struct *t) barrier(); } -/* - * Invalidate any task's FPSIMD state that is present on this cpu. - * The FPSIMD context should be acquired with get_cpu_fpsimd_context() - * before calling this function. - */ -static void fpsimd_flush_cpu_state(void) -{ - WARN_ON(!system_supports_fpsimd()); - __this_cpu_write(fpsimd_last_state.st, NULL); - - /* - * Leaving streaming mode enabled will cause issues for any kernel - * NEON and leaving streaming mode or ZA enabled may increase power - * consumption. - */ - if (system_supports_sme()) - sme_smstop(); - - set_thread_flag(TIF_FOREIGN_FPSTATE); -} - /* * Save the FPSIMD state to memory and invalidate cpu view. * This function must be called with preemption disabled.