Re: [PATCH 10/11] x86/fpu: prepare copy_fpstate_to_sigframe for TIF_LOAD_FPU

[Dave Hansen]

On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@xxxxxxxxxxx>
> 
> If TIF_LOAD_FPU is set, then the registers are saved (not loaded). In that case
> we skip the saving part.

This sentence hurts my brain.

	"If TIF_LOAD_FPU is set the registers are ... not loaded"

I think that means that something could use some better naming.

Should TIF_LOAD_FPU be TIF_NEED_FPU_LOAD, perhaps?

> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/fpu/signal.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index c8f5ff58578ed..979dcd1ed82e0 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -155,13 +155,17 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  			sizeof(struct user_i387_ia32_struct), NULL,
>  			(struct _fpstate_32 __user *) buf) ? -1 : 1;
>  
> -	/* Update the thread's fxstate to save the fsave header. */
> -	if (ia32_fxstate) {
> -		copy_fxregs_to_kernel(fpu);
> -	} else {
> -		copy_fpregs_to_fpstate(fpu);
> -		fpregs_deactivate(fpu);
> +	__fpregs_changes_begin();
> +	if (!test_thread_flag(TIF_LOAD_FPU)) {

This needs commenting, please.

If we do not need to load the FPU at return to userspace, it means the
state is in the the registers, not the buffer.  So, update the buffer to
match the registers.

> +		/* Update the thread's fxstate to save the fsave header. */
> +		if (ia32_fxstate) {
> +			copy_fxregs_to_kernel(fpu);
> +		} else {
> +			copy_fpregs_to_fpstate(fpu);
> +			fpregs_deactivate(fpu);
> +		}
>  	}
> +	__fpregs_changes_end();

Do we really need the __fpregs_changes_*() abstraction for this single
call site?