Patchwork [06/22] x86/fpu: Don't save fxregs for ia32 frames in copy_fpstate_to_sigframe()

login
register
mail settings
Submitter Sebastian Andrzej Siewior
Date Jan. 9, 2019, 11:47 a.m.
Message ID <20190109114744.10936-7-bigeasy@linutronix.de>
Download mbox | patch
Permalink /patch/695725/
State New
Headers show

Comments

Sebastian Andrzej Siewior - Jan. 9, 2019, 11:47 a.m.
Why does copy_fpstate_to_sigframe() do copy_fxregs_to_kernel() in the
ia32_fxstate case? I don't know. It just does.
Maybe it was required at some point, maybe it was added by accident and
nobody noticed it because it makes no difference.

In copy_fpstate_to_sigframe() we stash the FPU state into the task's
stackframe. Then the CPU's FPU registers (and its fpu->state) are
cleared (handle_signal() does fpu__clear()). So it makes *no* difference
what happens to fpu->state after copy_fpregs_to_sigframe().

Remove copy_fxregs_to_kernel() since it does not matter what it does and
save a few cycles.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c | 3 ---
 1 file changed, 3 deletions(-)
Borislav Petkov - Jan. 24, 2019, 11:17 a.m.
On Wed, Jan 09, 2019 at 12:47:28PM +0100, Sebastian Andrzej Siewior wrote:
> Why does copy_fpstate_to_sigframe() do copy_fxregs_to_kernel() in the
> ia32_fxstate case? I don't know. It just does.
> Maybe it was required at some point, maybe it was added by accident and
> nobody noticed it because it makes no difference.

So

  72a671ced66d ("x86, fpu: Unify signal handling code paths for x86 and x86_64 kernels")

talks about some exclusion of legacy fsave state.

> In copy_fpstate_to_sigframe() we stash the FPU state into the task's
> stackframe. Then the CPU's FPU registers (and its fpu->state) are
> cleared (handle_signal() does fpu__clear()).

So that fpu__clear() name is not optimal. It should be
fpu__reinitialize() or so. The comment above it says so too:

/*
 * Clear the FPU state back to init state.

> So it makes *no* difference
> what happens to fpu->state after copy_fpregs_to_sigframe().
> 
> Remove copy_fxregs_to_kernel() since it does not matter what it does and
> save a few cycles.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/kernel/fpu/signal.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index c136a4327659d..047390a45e016 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -174,9 +174,6 @@ int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
>  	/* 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);

Need to get rid of that local "fpu" var too:

arch/x86/kernel/fpu/signal.c: In function ‘copy_fpstate_to_sigframe’:
arch/x86/kernel/fpu/signal.c:159:14: warning: unused variable ‘fpu’ [-Wunused-variable]
  struct fpu *fpu = &current->thread.fpu;
              ^~~

>  	/* Save the fsave header for the 32-bit frames. */
>  	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))
> -- 
> 2.20.1
>

Patch

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index c136a4327659d..047390a45e016 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -174,9 +174,6 @@  int copy_fpstate_to_sigframe(void __user *buf, void __user *buf_fx, int size)
 	/* 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);
 
 	/* Save the fsave header for the 32-bit frames. */
 	if ((ia32_fxstate || !use_fxsr()) && save_fsave_header(tsk, buf))