[17/22] x86/fpu: Prepare copy_fpstate_to_sigframe() for TIF_NEED_FPU_LOAD
diff mbox series

Message ID 20190109114744.10936-18-bigeasy@linutronix.de
State New
Headers show
Series
  • [v6] x86: load FPU registers on return to userland
Related show

Commit Message

Sebastian Andrzej Siewior Jan. 9, 2019, 11:47 a.m. UTC
From: Rik van Riel <riel@surriel.com>

The FPU registers need only to be saved if TIF_NEED_FPU_LOAD is not set.
Otherwise this has been already done and can be skipped.

Signed-off-by: Rik van Riel <riel@surriel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/kernel/fpu/signal.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Borislav Petkov Jan. 30, 2019, 11:56 a.m. UTC | #1
On Wed, Jan 09, 2019 at 12:47:39PM +0100, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@surriel.com>
> 
> The FPU registers need only to be saved if TIF_NEED_FPU_LOAD is not set.
> Otherwise this has been already done and can be skipped.
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/x86/kernel/fpu/signal.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> index bf4e6caad305e..a25be217f9a2c 100644
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -156,7 +156,16 @@ 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;
>  
> -	copy_fpregs_to_fpstate(fpu);
> +	__fpregs_changes_begin();
> +	/*
> +	 * If we do not need to load the FPU registers at return to userspace
> +	 * then the CPU has the current state and we need to save it. Otherwise
> +	 * it is already done and we can skip it.
> +	 */
> +	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> +		copy_fpregs_to_fpstate(fpu);

I wonder if this flag would make the code more easy to follow by calling
it

	TIF_FPU_REGS_VALID

instead, to denote that the FPU registers in the CPU have a valid
content.

Then the test becomes:

	if (test_thread_flag(TIF_FPU_REGS_VALID))
		copy_fpregs_to_fpstate(fpu);
Sebastian Andrzej Siewior Jan. 30, 2019, 12:28 p.m. UTC | #2
On 2019-01-30 12:56:14 [+0100], Borislav Petkov wrote:
> > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> > index bf4e6caad305e..a25be217f9a2c 100644
> > --- a/arch/x86/kernel/fpu/signal.c
> > +++ b/arch/x86/kernel/fpu/signal.c
> > @@ -156,7 +156,16 @@ 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;
> >  
> > -	copy_fpregs_to_fpstate(fpu);
> > +	__fpregs_changes_begin();
> > +	/*
> > +	 * If we do not need to load the FPU registers at return to userspace
> > +	 * then the CPU has the current state and we need to save it. Otherwise
> > +	 * it is already done and we can skip it.
> > +	 */
> > +	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> > +		copy_fpregs_to_fpstate(fpu);
> 
> I wonder if this flag would make the code more easy to follow by calling
> it
> 
> 	TIF_FPU_REGS_VALID
> 
> instead, to denote that the FPU registers in the CPU have a valid
> content.
> 
> Then the test becomes:
> 
> 	if (test_thread_flag(TIF_FPU_REGS_VALID))
> 		copy_fpregs_to_fpstate(fpu);

I've been asked to add comment above the sequence so it is understood. I
think the general approach is easy to follow once the concept is
understood. I don't mind renaming the TIF_ thingy once again (it
happend once or twice and I think the current one was suggested by Andy
unless I mixed things up).
The problem I have with the above is that

	if (test_thread_flag(TIF_NEED_FPU_LOAD))
		do_that()

becomes
	if (!test_thread_flag(TIF_FPU_REGS_VALID))
		do_that()

and you could argue again the other way around. So do we want NEED_LOAD
or NEED_SAVE flag which is another way of saying REGS_VALID?
More importantly the logic is changed when the bit is set and this
requires more thinking than just doing sed on the patch series.

Sebastian
Borislav Petkov Jan. 30, 2019, 12:53 p.m. UTC | #3
On Wed, Jan 30, 2019 at 01:28:20PM +0100, Sebastian Andrzej Siewior wrote:
> > > +	/*
> > > +	 * If we do not need to load the FPU registers at return to userspace
> > > +	 * then the CPU has the current state and we need to save it. Otherwise
> > > +	 * it is already done and we can skip it.
> > > +	 */
> > > +	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
> > > +		copy_fpregs_to_fpstate(fpu);
> > 
> > I wonder if this flag would make the code more easy to follow by calling
> > it
> > 
> > 	TIF_FPU_REGS_VALID
> > 
> > instead, to denote that the FPU registers in the CPU have a valid
> > content.
> > 
> > Then the test becomes:
> > 
> > 	if (test_thread_flag(TIF_FPU_REGS_VALID))
> > 		copy_fpregs_to_fpstate(fpu);
> 
> I've been asked to add comment above the sequence so it is understood. I
> think the general approach is easy to follow once the concept is
> understood. I don't mind renaming the TIF_ thingy once again (it
> happend once or twice and I think the current one was suggested by Andy
> unless I mixed things up).
> The problem I have with the above is that
> 
> 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> 		do_that()
> 
> becomes
> 	if (!test_thread_flag(TIF_FPU_REGS_VALID))
> 		do_that()

Err, above it becomes

	if (test_thread_flag(TIF_FPU_REGS_VALID))
		copy_fpregs_to_fpstate(fpu);

without the "!". I.e., CPU's FPU regs are valid and we need to save them.

Or am I misreading the comment above?

> and you could argue again the other way around. So do we want NEED_LOAD
> or NEED_SAVE flag which is another way of saying REGS_VALID?

All fine and dandy except NEED_FPU_LOAD is ambiguous to me: we need to
load them where? Into the CPU? Or into the FPU state save area?

TIF_FPU_REGS_VALID is clearer in the sense that the CPU's FPU registers
are currently valid for the current task. As there are no other FPU
registers except the CPU's.

> More importantly the logic is changed when the bit is set and this
> requires more thinking than just doing sed on the patch series.

Sure.

And I'll get accustomed to the logic whatever the name is - this is just
a "wouldn't it be better" thing.

Thx.
Sebastian Andrzej Siewior Feb. 7, 2019, 2:10 p.m. UTC | #4
On 2019-01-30 13:53:51 [+0100], Borislav Petkov wrote:
> > I've been asked to add comment above the sequence so it is understood. I
> > think the general approach is easy to follow once the concept is
> > understood. I don't mind renaming the TIF_ thingy once again (it
> > happend once or twice and I think the current one was suggested by Andy
> > unless I mixed things up).
> > The problem I have with the above is that
> > 
> > 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> > 		do_that()
> > 
> > becomes
> > 	if (!test_thread_flag(TIF_FPU_REGS_VALID))
> > 		do_that()
> 
> Err, above it becomes
> 
> 	if (test_thread_flag(TIF_FPU_REGS_VALID))
> 		copy_fpregs_to_fpstate(fpu);

The (your) above example yes. But the reverse state
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
 		do_that()

becomes
 	if (!test_thread_flag(TIF_FPU_REGS_VALID))
 		do_that()

> without the "!". I.e., CPU's FPU regs are valid and we need to save them.
> 
> Or am I misreading the comment above?

Your example is correct. But in the opposite case, when ! was not there
then we have to add it.

> > and you could argue again the other way around. So do we want NEED_LOAD
> > or NEED_SAVE flag which is another way of saying REGS_VALID?
> 
> All fine and dandy except NEED_FPU_LOAD is ambiguous to me: we need to
> load them where? Into the CPU? Or into the FPU state save area?

if you need to LOAD then task-saved-area into CPU-state. If you need to
save it then CPU-state into task-saved-area.

> TIF_FPU_REGS_VALID is clearer in the sense that the CPU's FPU registers
> are currently valid for the current task. As there are no other FPU
> registers except the CPU's.

hmmm. I think it is just taste / habit.

> > More importantly the logic is changed when the bit is set and this
> > requires more thinking than just doing sed on the patch series.
> 
> Sure.
> 
> And I'll get accustomed to the logic whatever the name is - this is just
> a "wouldn't it be better" thing.

If it would be just a name thing then I probably wouldn't mind. But
swapping the logic might break things so I try to avoid that.

> Thx.
> 

Sebastian

Patch
diff mbox series

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index bf4e6caad305e..a25be217f9a2c 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -156,7 +156,16 @@  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;
 
-	copy_fpregs_to_fpstate(fpu);
+	__fpregs_changes_begin();
+	/*
+	 * If we do not need to load the FPU registers at return to userspace
+	 * then the CPU has the current state and we need to save it. Otherwise
+	 * it is already done and we can skip it.
+	 */
+	if (!test_thread_flag(TIF_NEED_FPU_LOAD))
+		copy_fpregs_to_fpstate(fpu);
+
+	__fpregs_changes_end();
 
 	if (using_compacted_format()) {
 		copy_xstate_to_user(buf_fx, xsave, 0, size);