| Date | Mon, 21 Jun 2021 17:30:07 +0200 | From | Borislav Petkov <> | Subject | Re: [patch V3 22/66] x86/fpu: Move fpu__write_begin() to regset |
| |
On Fri, Jun 18, 2021 at 04:18:45PM +0200, Thomas Gleixner wrote: > The only usecase for fpu__write_begin is the set() callback of regset, so > the function is pointlessly global. > > Move it to the regset code and rename it to fpu_force_restore() which is > exactly decribing what the function does.
And makes the _set() callbacks a lot more readable, nice!
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > arch/x86/include/asm/fpu/internal.h | 1 - > arch/x86/kernel/fpu/core.c | 24 ------------------------ > arch/x86/kernel/fpu/regset.c | 25 ++++++++++++++++++++++--- > 3 files changed, 22 insertions(+), 28 deletions(-)
...
> --- a/arch/x86/kernel/fpu/regset.c > +++ b/arch/x86/kernel/fpu/regset.c > @@ -44,6 +44,25 @@ static void fpu_sync_fpstate(struct fpu > fpu__save(fpu); > } > > +/* > + * Invalidate cached FPU registers before modifying the stopped target > + * task's fpstate. > + * > + * This forces the target task on resume to restore the FPU registers from > + * modified fpstate. Otherwise the task might skip the restore and operate > + * with the cached FPU registers which discards the modifications. > + */ > +static void fpu_force_restore(struct fpu *fpu) > +{ > + /* > + * Only stopped child tasks can be used to modify the FPU > + * state in the fpstate buffer: > + */ > + WARN_ON_FPU(fpu == ¤t->thread.fpu); > + > + __fpu_invalidate_fpregs_state(fpu); > +} > + > int xfpregs_get(struct task_struct *target, const struct user_regset *regset, > struct membuf to) > { > @@ -89,7 +108,7 @@ int xfpregs_set(struct task_struct *targ > if (newstate.mxcsr & ~mxcsr_feature_mask) > ret = -EINVAL; ^^^^^^^^^^^^^^
Don't forget to fix that thing, btw.
> - fpu__prepare_write(fpu); > + fpu_force_restore(fpu); > > /* Copy the state */ > memcpy(&fpu->state.fxsave, &newstate, sizeof(newstate)); > @@ -147,7 +166,7 @@ int xstateregs_set(struct task_struct *t > } > } > > - fpu__prepare_write(fpu); > + fpu_force_restore(fpu); > ret = copy_kernel_to_xstate(&fpu->state.xsave, kbuf ?: tmpbuf);
Ok, this here:
fpu_force_restore() will unconditionally invalidate the fpregs but the following copy_kernel_to_xstate() call can return -EINVAL when the xstate header validation fails or mxcsr has reserved bits set so we will not have copied anything to the target thread.
Or do we say this is a slowpath so we don't care?
Thx.
-- Regards/Gruss, Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
|