| Message ID | 20190403164156.19645-25-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/27] x86/fpu: Remove fpu->initialized usage in __fpu__restore_sig() | expand |
On Wed, 3 Apr 2019, Sebastian Andrzej Siewior wrote: > The previous commits refactor the restoration of the FPU registers so > that they can be loaded from in-kernel memory. This overhead can be > avoided if the load can be performed without a pagefault. > > Attempt to restore FPU registers by invoking > copy_user_to_fpregs_zeroing(). If it fails try the slowpath which can handle > pagefaults. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > arch/x86/kernel/fpu/signal.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > index a5b086ec426a5..f20e1d1fffa29 100644 > --- a/arch/x86/kernel/fpu/signal.c > +++ b/arch/x86/kernel/fpu/signal.c > @@ -242,10 +242,10 @@ sanitize_restored_xstate(union fpregs_state *state, > /* > * Restore the extended state if present. Otherwise, restore the FP/SSE state. > */ > -static inline int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > +static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > { > if (use_xsave()) { > - if ((unsigned long)buf % 64 || fx_only) { > + if (fx_only) { This change is weird and not mentioned in the changelog.... > u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE; > copy_kernel_to_xregs(&init_fpstate.xsave, init_bv); > return copy_user_to_fxregs(buf); > @@ -327,7 +327,19 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) > if (ret) > goto err_out; > envp = &env; > + } else { > + fpregs_lock(); > + pagefault_disable(); > + ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only); > + pagefault_enable(); > + if (!ret) { > + fpregs_mark_activate(); > + fpregs_unlock(); > + return 0; > + } > + fpregs_unlock(); > } > + > if (use_xsave() && !fx_only) { > u64 init_bv = xfeatures_mask & ~xfeatures; > > -- > 2.20.1 > >
On 2019-04-08 19:05:56 [+0200], Thomas Gleixner wrote: > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > > index a5b086ec426a5..f20e1d1fffa29 100644 > > --- a/arch/x86/kernel/fpu/signal.c > > +++ b/arch/x86/kernel/fpu/signal.c > > @@ -242,10 +242,10 @@ sanitize_restored_xstate(union fpregs_state *state, > > /* > > * Restore the extended state if present. Otherwise, restore the FP/SSE state. > > */ > > -static inline int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > > +static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > > { > > if (use_xsave()) { > > - if ((unsigned long)buf % 64 || fx_only) { > > + if (fx_only) { > > This change is weird and not mentioned in the changelog.... if you scroll up there is this: | * to loaded again on return to userland (overriding last_cpu avoids the | * optimisation). | */ | set_thread_flag(TIF_NEED_FPU_LOAD); | __fpu_invalidate_fpregs_state(fpu); | | if ((unsigned long)buf_fx % 64) | fx_only = 1; … | ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only); | pagefault_enable(); so I just removed that part because it was already done earlier. Is it still weird and should be mentioned in the changelog? Sebastian
On Mon, 8 Apr 2019, Sebastian Andrzej Siewior wrote: > On 2019-04-08 19:05:56 [+0200], Thomas Gleixner wrote: > > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > > > index a5b086ec426a5..f20e1d1fffa29 100644 > > > --- a/arch/x86/kernel/fpu/signal.c > > > +++ b/arch/x86/kernel/fpu/signal.c > > > @@ -242,10 +242,10 @@ sanitize_restored_xstate(union fpregs_state *state, > > > /* > > > * Restore the extended state if present. Otherwise, restore the FP/SSE state. > > > */ > > > -static inline int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > > > +static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > > > { > > > if (use_xsave()) { > > > - if ((unsigned long)buf % 64 || fx_only) { > > > + if (fx_only) { > > > > This change is weird and not mentioned in the changelog.... > > if you scroll up there is this: > | * to loaded again on return to userland (overriding last_cpu avoids the > | * optimisation). > | */ > | set_thread_flag(TIF_NEED_FPU_LOAD); > | __fpu_invalidate_fpregs_state(fpu); > | > | if ((unsigned long)buf_fx % 64) > | fx_only = 1; > … > | ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only); > | pagefault_enable(); > > > so I just removed that part because it was already done earlier. > Is it still weird and should be mentioned in the changelog? Bah. Staring at this for too long. All good ...
On Wed, Apr 03, 2019 at 06:41:53PM +0200, Sebastian Andrzej Siewior wrote: > The previous commits refactor the restoration of the FPU registers so > that they can be loaded from in-kernel memory. This overhead can be > avoided if the load can be performed without a pagefault. > > Attempt to restore FPU registers by invoking > copy_user_to_fpregs_zeroing(). If it fails try the slowpath which can handle > pagefaults. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > arch/x86/kernel/fpu/signal.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > index a5b086ec426a5..f20e1d1fffa29 100644 > --- a/arch/x86/kernel/fpu/signal.c > +++ b/arch/x86/kernel/fpu/signal.c > @@ -242,10 +242,10 @@ sanitize_restored_xstate(union fpregs_state *state, > /* > * Restore the extended state if present. Otherwise, restore the FP/SSE state. > */ > -static inline int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > +static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) > { > if (use_xsave()) { > - if ((unsigned long)buf % 64 || fx_only) { > + if (fx_only) { > u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE; > copy_kernel_to_xregs(&init_fpstate.xsave, init_bv); > return copy_user_to_fxregs(buf); > @@ -327,7 +327,19 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) > if (ret) > goto err_out; > envp = &env; > + } else { I've added here: + /* + * Attempt to restore the FPU registers directly from user + * memory. For that to succeed, the user accesses cannot cause + * page faults. If they do, fall back to the slow path below, + * going through the kernel buffer. + */ so that it is clear what's happening. This function is doing gazillion things again ;-\
On 2019-04-12 19:17:59 [+0200], Borislav Petkov wrote: > > @@ -327,7 +327,19 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) > > if (ret) > > goto err_out; > > envp = &env; > > + } else { > > I've added here: I would suggest to add it in the last patch once we have the complete fast path. The other one extended (in case you want to add a comment there, too). > + /* /** > + * Attempt to restore the FPU registers directly from user > + * memory. For that to succeed, the user accesses cannot cause I would say "user access" because it is always a single instruction. > + * page faults. If they do, fall back to the slow path below, > + * going through the kernel buffer. "with the enabled page fault handler". > + */ > > so that it is clear what's happening. > > This function is doing gazillion things again ;-\ It is the old code with disabled page fault handler. But I understand. Sebastian
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index a5b086ec426a5..f20e1d1fffa29 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -242,10 +242,10 @@ sanitize_restored_xstate(union fpregs_state *state, /* * Restore the extended state if present. Otherwise, restore the FP/SSE state. */ -static inline int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) +static int copy_user_to_fpregs_zeroing(void __user *buf, u64 xbv, int fx_only) { if (use_xsave()) { - if ((unsigned long)buf % 64 || fx_only) { + if (fx_only) { u64 init_bv = xfeatures_mask & ~XFEATURE_MASK_FPSSE; copy_kernel_to_xregs(&init_fpstate.xsave, init_bv); return copy_user_to_fxregs(buf); @@ -327,7 +327,19 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) if (ret) goto err_out; envp = &env; + } else { + fpregs_lock(); + pagefault_disable(); + ret = copy_user_to_fpregs_zeroing(buf_fx, xfeatures, fx_only); + pagefault_enable(); + if (!ret) { + fpregs_mark_activate(); + fpregs_unlock(); + return 0; + } + fpregs_unlock(); } + if (use_xsave() && !fx_only) { u64 init_bv = xfeatures_mask & ~xfeatures;
The previous commits refactor the restoration of the FPU registers so that they can be loaded from in-kernel memory. This overhead can be avoided if the load can be performed without a pagefault. Attempt to restore FPU registers by invoking copy_user_to_fpregs_zeroing(). If it fails try the slowpath which can handle pagefaults. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/x86/kernel/fpu/signal.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)