Re: [Patch v6 08/22] x86/fpu: Ensure TIF_NEED_FPU_LOAD is set after saving FPU state

[Dave Hansen]

On 2/11/26 11:39, Chang S. Bae wrote:
> But the changelog doesn't look clear enough to explain the reason to me.

Yeah, the changelog could use some improvement.

This also leaves the code rather fragile. Right now, all of the:

	save_fpregs_to_fpstate(fpu);
	set_thread_flag(TIF_NEED_FPU_LOAD);

pairs are open-coded. There's precisely nothing stopping someone from
coming in tomorrow and reversing the order at these or other call sites.
There's also zero comments left in the code to tell folks not to do this.

Are there enough of these to have a helper that does:

	save_fpregs_to_fpstate_before_invalidation(fpu);

which could do the save and set TIF_NEED_FPU_LOAD? (that name is awful
btw, please don't use it).

This:

>  	/* Swap fpstate */
>  	if (enter_guest) {
> -		fpu->__task_fpstate = cur_fps;
> +		WRITE_ONCE(fpu->__task_fpstate, cur_fps);
> +		barrier();
>  		fpu->fpstate = guest_fps;
>  		guest_fps->in_use = true;
>  	} else {
>  		guest_fps->in_use = false;
>  		fpu->fpstate = fpu->__task_fpstate;
> -		fpu->__task_fpstate = NULL;
> +		barrier();
> +		WRITE_ONCE(fpu->__task_fpstate, NULL);
>  	}

also urgently needs comments.

I also can't help but think that there might be a nicer way to do that
without the barrier(). I _think_ two correctly-ordered WRITE_ONCE()'s
would make the compiler do the same thing as the barrier().

But I'm not fully understanding what the barrier() is doing anyway, so
take that with a grain of salt.