Re: [PATCH] x86: Use __fpu_invalidate_fpregs_state() in exec

[Sohil Mehta <sohil.mehta@intel.com>]

Hi Rick,

The comment below doesn't change anything meaningfully, but mainly helps
clarifies the original comment.

Maybe something like this:
>   * If the FPU register state is valid, the kernel can skip restoring the
>   * FPU state from memory.
>   *
> - * Any code that clobbers the FPU registers or updates the in-memory
> - * FPU state for a task MUST let the rest of the kernel know that the
> - * FPU registers are no longer valid for this task.
> + * Any code that clobbers the FPU registers or updates the in-memory FPU state
> + * for a task MUST let the rest of the kernel know that whether the FPU
> + * registers are no longer valid anywhere for this task or if the FPU state on
> + * a particular CPU is invalid.
>   *
> - * Either one of these invalidation functions is enough. Invalidate
> - * a resource you control: CPU if using the CPU for something else
> - * (with preemption disabled), FPU for the current task, or a task that
> - * is prevented from running by the current task.
> + * Invalidate a resource that you truly control:
> + * - Use __cpu_invalidate_fpregs_state() or equivalent when using the CPU for
> + *   something else (only with preemption disabled)
> + * - Use __fpu_invalidate_fpregs_state() or equivalent when modifying the FPU
> + *   for the current task, or a task that is prevented from running by the
> + *   current task.
>   */

Please disregard my suggestion if you think the existing one is
sufficient for now.

On 8/18/2023 10:03 AM, Rick Edgecombe wrote:
> diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
> index e03b6b107b20..a86d37052a64 100644
> --- a/arch/x86/kernel/fpu/core.c
> +++ b/arch/x86/kernel/fpu/core.c
> @@ -713,7 +713,7 @@ static void fpu_reset_fpregs(void)
>  	struct fpu *fpu = &current->thread.fpu;
>  
>  	fpregs_lock();
> -	fpu__drop(fpu);
> +	__fpu_invalidate_fpregs_state(fpu);
>  	/*
>  	 * This does not change the actual hardware registers. It just
>  	 * resets the memory image and sets TIF_NEED_FPU_LOAD so a

The code fix looks fine to me.

Even if you don't update the comment, please feel free to add:
Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>