Re: [PATCH 3/7] x86/fpu: Extract restore_from_ia32_fxstate() and clean up fpu__restore_sig()

["Chang S. Bae" <chang.seok.bae@intel.com>]

On 9/7/2026 9:34 PM, Andrei Vagin wrote:
> Improve readability of the signal frame restoration code. Previously,

Note the tip- changelog style:
   https://docs.kernel.org/process/maintainer-tip.htm
   ... A good structure is to explain the context, the problem and the
   solution in separate paragraphs and this order. ...

This first sentence doesn't appear to be the context/problem itself.

> most of __fpu_restore_sig() was dedicated to handling the 32-bit compat
> fpstate, while the native direct path lived in restore_fpregs_from_user().
> Having the compat handling intermixed with the main flow made it tricky
> to quickly see what code was doing what.
> 
> Extract the 32-bit legacy/compat FPU restore handling into a separate
> helper function, restore_from_ia32_fxstate(), and inline the remainder
> of __fpu_restore_sig() directly into fpu__restore_sig().
> 
> The legacy 32-bit FP frame duplicates the FP state portion of the
> FX/XSAVE frame. For backward compatibility, the legacy FP frame is
> treated as the source of truth, and its state is folded into the
> FX/XSAVE state before restoring the registers.
> 
> Reviewed-by: Alexander Mikhalitsyn <alexander@mihalicyn.com>
> Signed-off-by: Andrei Vagin <avagin@google.com>

...


>   static inline unsigned int xstate_sigframe_size(struct fpstate *fpstate)
>   {
> @@ -450,10 +449,11 @@ static inline unsigned int xstate_sigframe_size(struct fpstate *fpstate)
>   bool fpu__restore_sig(void __user *buf, int ia32_frame)
>   {
>   	struct fpu *fpu = x86_task_fpu(current);
> -	void __user *buf_fx = buf;
> +	bool success = false, fx_only = false;
>   	bool ia32_fxstate = false;
> -	bool success = false;
> +	void __user *buf_fx = buf;
>   	unsigned int size;
> +	u64 xrestore_mask;
>   
>   	if (unlikely(!buf)) {
>   		fpu__clear_user_states(fpu);
> @@ -482,10 +482,29 @@ bool fpu__restore_sig(void __user *buf, int ia32_frame)
>   		success = !fpregs_soft_set(current, NULL, 0,
>   					   sizeof(struct user_i387_ia32_struct),
>   					   NULL, buf);
> +		goto out;
> +	}
> +
> +	if (use_xsave()) {
> +		struct _fpx_sw_bytes fx_sw_user;
> +
> +		if (!check_xstate_in_sigframe(buf_fx, &fx_sw_user))
> +			goto out;
> +
> +		fx_only = !fx_sw_user.magic1;
> +		xrestore_mask = fx_sw_user.xfeatures;
>   	} else {
> -		success = __fpu_restore_sig(buf, buf_fx, ia32_fxstate);
> +		xrestore_mask = XFEATURE_MASK_FPSSE;
> +	}
> +
> +	if (ia32_fxstate) {
> +		success = restore_from_ia32_fxstate(buf, buf_fx,
> +						    xrestore_mask, fx_only);
> +		goto out;
>   	}
>   
> +	/* Restore the FPU registers directly from user memory. */
> +	success = restore_fpregs_from_user(buf_fx, xrestore_mask, fx_only);

Nit: Just stylistic - maybe this could also be kept a bit simpler:

   if (ia32_fxstate)
       success = restore_from_ia32_fxstate();
   else
       success = restore_fpregs_from_user();

Overall, I think this refactoring makes the flow easier to follow with 
smaller pieces:

   Reviewed-by: Chang S. Bae <chang.seok.bae@intel.com>

Thanks,
Chang