arm64/fpsimd: Avoid erroneous elide of user state reload
PATCH 1/1 arm64/fpsimd: Avoid erroneous elide of user state reload
From: Ard Biesheuvel <[email protected]>
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.
--- arch/arm64/kernel/fpsimd.c | 44 +++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
arch/arm64/kernel/fpsimd.c +22 -22
@@ -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.
Signature · 7 lines
2.45.1.288.g0e0cd299f1-goog _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hej,
Quoted 34 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
TIF_FOREIGN_FPSTATEis 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_FPSTATEis 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, theTIF_FOREIGN_FPSTATEflag 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_FPSTATEflag 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_FPSTATEwhen scheduling in a task withTIF_KERNEL_FPSTATEset, the underlying state should be updated so thatTIF_FOREIGN_FPSTATEwill 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/[email protected]Reported-by Johannes Nixdorf <[email protected]>Fixes aefbab8e77eb ("arm64: fpsimd: Preserve/restore kernel mode NEON
Quoted 8 lines
at context switch")
Cc Mark Brown <[email protected]>Cc Dave Martin <[email protected]>Cc Janne Grunau <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>--- 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: [email protected] and feel free to add
thanks
Janne
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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).
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 8 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
TIF_FOREIGN_FPSTATEis 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).
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 8 lines · Ard Biesheuvel
TIF_FOREIGN_FPSTATEis 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,
Signature · 10 lines
Will https://fixes.arm64.dev https://next.arm64.dev https://will.arm64.dev _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel