diff mbox series

[08/11] x86/fpu: Always store the registers in copy_fpstate_to_sigframe()

Message ID 20181004140547.13014-9-bigeasy@linutronix.de (mailing list archive)
State New, archived
Headers show
Series x86: load FPU registers on return to userland | expand

Commit Message

Sebastian Andrzej Siewior Oct. 4, 2018, 2:05 p.m. UTC 
From: Rik van Riel <riel@surriel.com>

copy_fpstate_to_sigframe() has two callers and both invoke the function only if
fpu->initialized is set. So the check in the function for ->initialized makes
no sense. It might be a relict from the lazy-FPU time: If the FPU registers
were "loaded" then we could save them directly. Otherwise (the FPU
registers are not up to date) they are saved in the FPU struct and
it could be used for memcpy().

Since the registers always loaded at this point, save them and copy
later.
This code is extracted from an earlier version of the patchset while
there still was lazy-FPU on x86. This is a preparation for loading the
FPU registers on return to userland.

Signed-off-by: Rik van Riel <riel@surriel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/fpu/internal.h | 45 ----------------------------
 arch/x86/kernel/fpu/signal.c        | 46 +++++++----------------------
 2 files changed, 11 insertions(+), 80 deletions(-)

Comments

Christophe de Dinechin Oct. 11, 2018, 5:50 p.m. UTC | #1 
Sebastian Andrzej Siewior writes:

> From: Rik van Riel <riel@surriel.com>
>
> copy_fpstate_to_sigframe() has two callers and both invoke the function only if
> fpu->initialized is set.

One of the callers is in a different file.
Not sure if that warrants some kind of check that
fpu->initialized is set?

> So the check in the function for ->initialized makes
> no sense. It might be a relict from the lazy-FPU time: If the FPU
> registers

s/relict/relic/

> were "loaded" then we could save them directly. Otherwise (the FPU
> registers are not up to date) they are saved in the FPU struct and
> it could be used for memcpy().

>
> Since the registers always loaded at this point, save them and copy
> later.

register [are] always loaded

> This code is extracted from an earlier version of the patchset while
> there still was lazy-FPU on x86. This is a preparation for loading the
> FPU registers on return to userland.

maybe "only on return"?

>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/include/asm/fpu/internal.h | 45 ----------------------------
>  arch/x86/kernel/fpu/signal.c        | 46 +++++++----------------------
>  2 files changed, 11 insertions(+), 80 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 4ecaf4d22954e..df8816be3efdd 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -126,22 +126,6 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
>  		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
>  		     : output : input)
>
> -static inline int copy_fregs_to_user(struct fregs_state __user *fx)
> -{
> -	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
> -}
> -
> -static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
> -{
> -	if (IS_ENABLED(CONFIG_X86_32))
> -		return user_insn(fxsave %[fx], [fx] "=m" (*fx), "m" (*fx));
> -	else if (IS_ENABLED(CONFIG_AS_FXSAVEQ))
> -		return user_insn(fxsaveq %[fx], [fx] "=m" (*fx), "m" (*fx));
> -
> -	/* See comment in copy_fxregs_to_kernel() below. */
> -	return user_insn(rex64/fxsave (%[fx]), "=m" (*fx), [fx] "R" (fx));
> -}
> -
>  static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
>  {
>  	if (IS_ENABLED(CONFIG_X86_32)) {
> @@ -352,35 +336,6 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
>  	XSTATE_XRESTORE(xstate, lmask, hmask);
>  }
>
> -/*
> - * Save xstate to user space xsave area.
> - *
> - * We don't use modified optimization because xrstor/xrstors might track
> - * a different application.
> - *
> - * We don't use compacted format xsave area for
> - * backward compatibility for old applications which don't understand
> - * compacted format of xsave area.
> - */
> -static inline int copy_xregs_to_user(struct xregs_state __user *buf)
> -{
> -	int err;
> -
> -	/*
> -	 * Clear the xsave header first, so that reserved fields are
> -	 * initialized to zero.
> -	 */
> -	err = __clear_user(&buf->header, sizeof(buf->header));
> -	if (unlikely(err))
> -		return -EFAULT;
> -
> -	stac();
> -	XSTATE_OP(XSAVE, buf, -1, -1, err);
> -	clac();
> -
> -	return err;
> -}
> -
>  /*
>   * Restore xstate from user space xsave area.
>   */
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 23f1691670b66..c8f5ff58578ed 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -117,23 +117,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
>
>  	return err;
>  }
> -
> -static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> -{
> -	int err;
> -
> -	if (use_xsave())
> -		err = copy_xregs_to_user(buf);
> -	else if (use_fxsr())
> -		err = copy_fxregs_to_user((struct fxregs_state __user *) buf);
> -	else
> -		err = copy_fregs_to_user((struct fregs_state __user *) buf);
> -
> -	if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))
> -		err = -EFAULT;
> -	return err;
> -}
> -
>  /*
>   * Save the fpu, extended register state to the user signal frame.
>   *

I would modify the function comment to add the new expectation that
fpu->initialized must be set on entry.

> @@ -172,27 +155,20 @@ 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;
>
> -	if (fpu->initialized || using_compacted_format()) {
> -		/* Save the live register state to the user directly. */
> -		if (copy_fpregs_to_sigframe(buf_fx))
> -			return -1;
> -		/* Update the thread's fxstate to save the fsave header. */
> -		if (ia32_fxstate)
> -			copy_fxregs_to_kernel(fpu);
> +	/* Update the thread's fxstate to save the fsave header. */
> +	if (ia32_fxstate) {
> +		copy_fxregs_to_kernel(fpu);
>  	} else {
> -		/*
> -		 * It is a *bug* if kernel uses compacted-format for xsave
> -		 * area and we copy it out directly to a signal frame. It
> -		 * should have been handled above by saving the registers
> -		 * directly.
> -		 */
> -		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
> -			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
> -			return -1;
> -		}
> +		copy_fpregs_to_fpstate(fpu);
> +		fpregs_deactivate(fpu);
> +	}
>
> +	if (using_compacted_format()) {
> +		copy_xstate_to_user(buf_fx, xsave, 0, size);
> +	} else {
>  		fpstate_sanitize_xstate(fpu);
> -		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
> +		size = fpu_user_xstate_size;
> +		if (__copy_to_user(buf_fx, xsave, size))
>  			return -1;
>  	}


--
Cheers,
Christophe de Dinechin (IRC c3d)
Andy Lutomirski Oct. 11, 2018, 9:18 p.m. UTC | #2 
On Thu, Oct 11, 2018 at 10:50 AM Christophe de Dinechin
<christophe.de.dinechin@gmail.com> wrote:
>
>
> Sebastian Andrzej Siewior writes:
>
> > From: Rik van Riel <riel@surriel.com>
> >
> > copy_fpstate_to_sigframe() has two callers and both invoke the function only if
> > fpu->initialized is set.
>
> One of the callers is in a different file.
> Not sure if that warrants some kind of check that
> fpu->initialized is set?

I still kind of think that fpu->initialized should be removed entirely
as part of this series.  Keeping unnecessary complication around in
the FPU code increaseds the scope for interesting bugs and makes
understanding the code considerably harder.
Dave Hansen Oct. 12, 2018, 6:15 p.m. UTC | #3 
On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@surriel.com>
> 
> copy_fpstate_to_sigframe() has two callers and both invoke the function only if
> fpu->initialized is set. So the check in the function for ->initialized makes
> no sense. It might be a relict from the lazy-FPU time: If the FPU registers
> were "loaded" then we could save them directly. Otherwise (the FPU
> registers are not up to date) they are saved in the FPU struct and
> it could be used for memcpy().
> 
> Since the registers always loaded at this point, save them and copy
> later.
> This code is extracted from an earlier version of the patchset while
> there still was lazy-FPU on x86. This is a preparation for loading the
> FPU registers on return to userland.

Could you make a pass through all the changelogs at at least make sure
they look sane and consistent?  The text width and paragraph spacing
here looks rather cobbled together.

> ---
>  arch/x86/include/asm/fpu/internal.h | 45 ----------------------------
>  arch/x86/kernel/fpu/signal.c        | 46 +++++++----------------------
>  2 files changed, 11 insertions(+), 80 deletions(-)

The diffstat here is really nice.

> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 4ecaf4d22954e..df8816be3efdd 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -126,22 +126,6 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
>  		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
>  		     : output : input)
>  
> -static inline int copy_fregs_to_user(struct fregs_state __user *fx)
> -{
> -	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
> -}
> -
> -static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
> -{
> -	if (IS_ENABLED(CONFIG_X86_32))
> -		return user_insn(fxsave %[fx], [fx] "=m" (*fx), "m" (*fx));
> -	else if (IS_ENABLED(CONFIG_AS_FXSAVEQ))
> -		return user_insn(fxsaveq %[fx], [fx] "=m" (*fx), "m" (*fx));
> -
> -	/* See comment in copy_fxregs_to_kernel() below. */
> -	return user_insn(rex64/fxsave (%[fx]), "=m" (*fx), [fx] "R" (fx));
> -}
> -
>  static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
>  {
>  	if (IS_ENABLED(CONFIG_X86_32)) {
> @@ -352,35 +336,6 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
>  	XSTATE_XRESTORE(xstate, lmask, hmask);
>  }
>  
> -/*
> - * Save xstate to user space xsave area.
> - *
> - * We don't use modified optimization because xrstor/xrstors might track
> - * a different application.
> - *
> - * We don't use compacted format xsave area for
> - * backward compatibility for old applications which don't understand
> - * compacted format of xsave area.
> - */
> -static inline int copy_xregs_to_user(struct xregs_state __user *buf)
> -{
> -	int err;
> -
> -	/*
> -	 * Clear the xsave header first, so that reserved fields are
> -	 * initialized to zero.
> -	 */
> -	err = __clear_user(&buf->header, sizeof(buf->header));
> -	if (unlikely(err))
> -		return -EFAULT;
> -
> -	stac();
> -	XSTATE_OP(XSAVE, buf, -1, -1, err);
> -	clac();
> -
> -	return err;
> -}
> -
>  /*
>   * Restore xstate from user space xsave area.
>   */
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index 23f1691670b66..c8f5ff58578ed 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -117,23 +117,6 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
>  
>  	return err;
>  }
> -
> -static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
> -{
> -	int err;
> -
> -	if (use_xsave())
> -		err = copy_xregs_to_user(buf);
> -	else if (use_fxsr())
> -		err = copy_fxregs_to_user((struct fxregs_state __user *) buf);
> -	else
> -		err = copy_fregs_to_user((struct fregs_state __user *) buf);
> -
> -	if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))
> -		err = -EFAULT;
> -	return err;
> -}
> -
>  /*
>   * Save the fpu, extended register state to the user signal frame.
>   *
> @@ -172,27 +155,20 @@ 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;
>  
> -	if (fpu->initialized || using_compacted_format()) {
> -		/* Save the live register state to the user directly. */
> -		if (copy_fpregs_to_sigframe(buf_fx))
> -			return -1;
> -		/* Update the thread's fxstate to save the fsave header. */
> -		if (ia32_fxstate)
> -			copy_fxregs_to_kernel(fpu);
> +	/* Update the thread's fxstate to save the fsave header. */
> +	if (ia32_fxstate) {
> +		copy_fxregs_to_kernel(fpu);
>  	} else {
> -		/*
> -		 * It is a *bug* if kernel uses compacted-format for xsave
> -		 * area and we copy it out directly to a signal frame. It
> -		 * should have been handled above by saving the registers
> -		 * directly.
> -		 */
> -		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
> -			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
> -			return -1;
> -		}
> +		copy_fpregs_to_fpstate(fpu);
> +		fpregs_deactivate(fpu);
> +	}

Could you add a high-level comment for this if{}else{} block that says
something like:

	/* Save the registers to the fpstate. */

I also think it's worthwhile to explain the asymmetry between the
ia32_fxstate case and the other branch.  Why don't we
fpregs_deactivate() in the ia32_fxstate path, for instance?

> +	if (using_compacted_format()) {
> +		copy_xstate_to_user(buf_fx, xsave, 0, size);
> +	} else {
>  		fpstate_sanitize_xstate(fpu);
> -		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
> +		size = fpu_user_xstate_size;
> +		if (__copy_to_user(buf_fx, xsave, size))
>  			return -1;
>  	}

This seems unnecessary.  Why are you updating 'size' like this?
Sebastian Andrzej Siewior Nov. 2, 2018, 2:42 p.m. UTC | #4 
On 2018-10-12 11:15:51 [-0700], Dave Hansen wrote:
> > @@ -172,27 +155,20 @@ 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;
> >  
> > -	if (fpu->initialized || using_compacted_format()) {
> > -		/* Save the live register state to the user directly. */
> > -		if (copy_fpregs_to_sigframe(buf_fx))
> > -			return -1;
> > -		/* Update the thread's fxstate to save the fsave header. */
> > -		if (ia32_fxstate)
> > -			copy_fxregs_to_kernel(fpu);
> > +	/* Update the thread's fxstate to save the fsave header. */
> > +	if (ia32_fxstate) {
> > +		copy_fxregs_to_kernel(fpu);
> >  	} else {
> > -		/*
> > -		 * It is a *bug* if kernel uses compacted-format for xsave
> > -		 * area and we copy it out directly to a signal frame. It
> > -		 * should have been handled above by saving the registers
> > -		 * directly.
> > -		 */
> > -		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
> > -			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
> > -			return -1;
> > -		}
> > +		copy_fpregs_to_fpstate(fpu);
> > +		fpregs_deactivate(fpu);
> > +	}
> 
> Could you add a high-level comment for this if{}else{} block that says
> something like:
> 
> 	/* Save the registers to the fpstate. */
> 
> I also think it's worthwhile to explain the asymmetry between the
> ia32_fxstate case and the other branch.  Why don't we
> fpregs_deactivate() in the ia32_fxstate path, for instance?

Since the ->initialized is gone, the whole hunk here looks differently
and probably easier to understand.

> > +	if (using_compacted_format()) {
> > +		copy_xstate_to_user(buf_fx, xsave, 0, size);
> > +	} else {
> >  		fpstate_sanitize_xstate(fpu);
> > -		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
> > +		size = fpu_user_xstate_size;
> > +		if (__copy_to_user(buf_fx, xsave, size))
> >  			return -1;
> >  	}

dropped this.

> This seems unnecessary.  Why are you updating 'size' like this?

Sebastian
diff mbox series

Patch

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 4ecaf4d22954e..df8816be3efdd 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -126,22 +126,6 @@  extern void fpstate_sanitize_xstate(struct fpu *fpu);
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
 		     : output : input)
 
-static inline int copy_fregs_to_user(struct fregs_state __user *fx)
-{
-	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
-}
-
-static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
-{
-	if (IS_ENABLED(CONFIG_X86_32))
-		return user_insn(fxsave %[fx], [fx] "=m" (*fx), "m" (*fx));
-	else if (IS_ENABLED(CONFIG_AS_FXSAVEQ))
-		return user_insn(fxsaveq %[fx], [fx] "=m" (*fx), "m" (*fx));
-
-	/* See comment in copy_fxregs_to_kernel() below. */
-	return user_insn(rex64/fxsave (%[fx]), "=m" (*fx), [fx] "R" (fx));
-}
-
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
 	if (IS_ENABLED(CONFIG_X86_32)) {
@@ -352,35 +336,6 @@  static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
-/*
- * Save xstate to user space xsave area.
- *
- * We don't use modified optimization because xrstor/xrstors might track
- * a different application.
- *
- * We don't use compacted format xsave area for
- * backward compatibility for old applications which don't understand
- * compacted format of xsave area.
- */
-static inline int copy_xregs_to_user(struct xregs_state __user *buf)
-{
-	int err;
-
-	/*
-	 * Clear the xsave header first, so that reserved fields are
-	 * initialized to zero.
-	 */
-	err = __clear_user(&buf->header, sizeof(buf->header));
-	if (unlikely(err))
-		return -EFAULT;
-
-	stac();
-	XSTATE_OP(XSAVE, buf, -1, -1, err);
-	clac();
-
-	return err;
-}
-
 /*
  * Restore xstate from user space xsave area.
  */
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 23f1691670b66..c8f5ff58578ed 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -117,23 +117,6 @@  static inline int save_xstate_epilog(void __user *buf, int ia32_frame)
 
 	return err;
 }
-
-static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf)
-{
-	int err;
-
-	if (use_xsave())
-		err = copy_xregs_to_user(buf);
-	else if (use_fxsr())
-		err = copy_fxregs_to_user((struct fxregs_state __user *) buf);
-	else
-		err = copy_fregs_to_user((struct fregs_state __user *) buf);
-
-	if (unlikely(err) && __clear_user(buf, fpu_user_xstate_size))
-		err = -EFAULT;
-	return err;
-}
-
 /*
  * Save the fpu, extended register state to the user signal frame.
  *
@@ -172,27 +155,20 @@  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;
 
-	if (fpu->initialized || using_compacted_format()) {
-		/* Save the live register state to the user directly. */
-		if (copy_fpregs_to_sigframe(buf_fx))
-			return -1;
-		/* Update the thread's fxstate to save the fsave header. */
-		if (ia32_fxstate)
-			copy_fxregs_to_kernel(fpu);
+	/* Update the thread's fxstate to save the fsave header. */
+	if (ia32_fxstate) {
+		copy_fxregs_to_kernel(fpu);
 	} else {
-		/*
-		 * It is a *bug* if kernel uses compacted-format for xsave
-		 * area and we copy it out directly to a signal frame. It
-		 * should have been handled above by saving the registers
-		 * directly.
-		 */
-		if (boot_cpu_has(X86_FEATURE_XSAVES)) {
-			WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n");
-			return -1;
-		}
+		copy_fpregs_to_fpstate(fpu);
+		fpregs_deactivate(fpu);
+	}
 
+	if (using_compacted_format()) {
+		copy_xstate_to_user(buf_fx, xsave, 0, size);
+	} else {
 		fpstate_sanitize_xstate(fpu);
-		if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size))
+		size = fpu_user_xstate_size;
+		if (__copy_to_user(buf_fx, xsave, size))
 			return -1;
 	}