Messages in this thread | | | Date | Tue, 22 Aug 2023 13:10:57 -0700 | Subject | Re: [PATCH] x86: Use __fpu_invalidate_fpregs_state() in exec | From | Sohil Mehta <> |
| |
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 = ¤t->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>
| |