| Message ID | 20190109114744.10936-18-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] x86: load FPU registers on return to userland | expand |
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);
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
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.
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
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);