| Message ID | 20181004140547.13014-11-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: load FPU registers on return to userland | expand |
On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote: > From: Rik van Riel <riel@surriel.com> > > If TIF_LOAD_FPU is set, then the registers are saved (not loaded). In that case > we skip the saving part. This sentence hurts my brain. "If TIF_LOAD_FPU is set the registers are ... not loaded" I think that means that something could use some better naming. Should TIF_LOAD_FPU be TIF_NEED_FPU_LOAD, perhaps? > Signed-off-by: Rik van Riel <riel@surriel.com> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > arch/x86/kernel/fpu/signal.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > index c8f5ff58578ed..979dcd1ed82e0 100644 > --- a/arch/x86/kernel/fpu/signal.c > +++ b/arch/x86/kernel/fpu/signal.c > @@ -155,13 +155,17 @@ 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; > > - /* Update the thread's fxstate to save the fsave header. */ > - if (ia32_fxstate) { > - copy_fxregs_to_kernel(fpu); > - } else { > - copy_fpregs_to_fpstate(fpu); > - fpregs_deactivate(fpu); > + __fpregs_changes_begin(); > + if (!test_thread_flag(TIF_LOAD_FPU)) { This needs commenting, please. If we do not need to load the FPU at return to userspace, it means the state is in the the registers, not the buffer. So, update the buffer to match the registers. > + /* Update the thread's fxstate to save the fsave header. */ > + if (ia32_fxstate) { > + copy_fxregs_to_kernel(fpu); > + } else { > + copy_fpregs_to_fpstate(fpu); > + fpregs_deactivate(fpu); > + } > } > + __fpregs_changes_end(); Do we really need the __fpregs_changes_*() abstraction for this single call site?
On Fri, Oct 12, 2018 at 12:40:19PM -0700, Dave Hansen wrote: > > + __fpregs_changes_end(); > > Do we really need the __fpregs_changes_*() abstraction for this single > call site? Yap, I'm staring at those in patch 2, there's no documentation there what they're supposed to do, only the commit message of patch 11 says: "The __fpregs_changes_{begin|end}() section ensures that the register remain unchanged. Otherwise a context switch or a BH could save the registers to its FPU context and processor's FPU register would remain random." So I'd say we should drop that abstraction, use preempt_* and put that text above the single usage site. Thx.
On 2018-10-15 17:24:31 [+0200], Borislav Petkov wrote: > On Fri, Oct 12, 2018 at 12:40:19PM -0700, Dave Hansen wrote: > > > + __fpregs_changes_end(); > > > > Do we really need the __fpregs_changes_*() abstraction for this single > > call site? > > Yap, I'm staring at those in patch 2, there's no documentation there what > they're supposed to do, only the commit message of patch 11 says: > > "The __fpregs_changes_{begin|end}() section ensures that the register > remain unchanged. Otherwise a context switch or a BH could save the > registers to its FPU context and processor's FPU register would remain > random." > > So I'd say we should drop that abstraction, use preempt_* and put that > text above the single usage site. There are more than one caller and this function disables preemption and BH in the end. Therefore I would like to keep it. But as suggested in the previous patch I will think about renaming it. > Thx. Sebastian
On 2018-10-12 12:40:19 [-0700], Dave Hansen wrote: > On 10/04/2018 07:05 AM, Sebastian Andrzej Siewior wrote: > > From: Rik van Riel <riel@surriel.com> > > > > If TIF_LOAD_FPU is set, then the registers are saved (not loaded). In that case > > we skip the saving part. > > This sentence hurts my brain. > > "If TIF_LOAD_FPU is set the registers are ... not loaded" > > I think that means that something could use some better naming. > > Should TIF_LOAD_FPU be TIF_NEED_FPU_LOAD, perhaps? ARM has TIF_FOREIGN_FPSTATE which basically means that the current FP stat does not belong to current(). Let me try to use TIF_NEED_FPU_LOAD and see how that works out. > > Signed-off-by: Rik van Riel <riel@surriel.com> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > --- > > arch/x86/kernel/fpu/signal.c | 16 ++++++++++------ > > 1 file changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > > index c8f5ff58578ed..979dcd1ed82e0 100644 > > --- a/arch/x86/kernel/fpu/signal.c > > +++ b/arch/x86/kernel/fpu/signal.c > > @@ -155,13 +155,17 @@ 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; > > > > - /* Update the thread's fxstate to save the fsave header. */ > > - if (ia32_fxstate) { > > - copy_fxregs_to_kernel(fpu); > > - } else { > > - copy_fpregs_to_fpstate(fpu); > > - fpregs_deactivate(fpu); > > + __fpregs_changes_begin(); > > + if (!test_thread_flag(TIF_LOAD_FPU)) { > > This needs commenting, please. > > If we do not need to load the FPU at return to userspace, it means the > state is in the the registers, not the buffer. So, update the buffer to > match the registers. okay. Let me add this then. Sebastian
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index c8f5ff58578ed..979dcd1ed82e0 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -155,13 +155,17 @@ 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; - /* Update the thread's fxstate to save the fsave header. */ - if (ia32_fxstate) { - copy_fxregs_to_kernel(fpu); - } else { - copy_fpregs_to_fpstate(fpu); - fpregs_deactivate(fpu); + __fpregs_changes_begin(); + if (!test_thread_flag(TIF_LOAD_FPU)) { + /* Update the thread's fxstate to save the fsave header. */ + if (ia32_fxstate) { + copy_fxregs_to_kernel(fpu); + } else { + copy_fpregs_to_fpstate(fpu); + fpregs_deactivate(fpu); + } } + __fpregs_changes_end(); if (using_compacted_format()) { copy_xstate_to_user(buf_fx, xsave, 0, size);