Messages in this thread | | | Date | Fri, 2 Nov 2018 15:42:28 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH 08/11] x86/fpu: Always store the registers in copy_fpstate_to_sigframe() |
| |
On 2018-10-12 11:15:51 [-0700], Dave Hansen wrote: > > @@ -172,27 +155,20 @@ 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; > > > > - if (fpu->initialized || using_compacted_format()) { > > - /* Save the live register state to the user directly. */ > > - if (copy_fpregs_to_sigframe(buf_fx)) > > - return -1; > > - /* Update the thread's fxstate to save the fsave header. */ > > - if (ia32_fxstate) > > - copy_fxregs_to_kernel(fpu); > > + /* Update the thread's fxstate to save the fsave header. */ > > + if (ia32_fxstate) { > > + copy_fxregs_to_kernel(fpu); > > } else { > > - /* > > - * It is a *bug* if kernel uses compacted-format for xsave > > - * area and we copy it out directly to a signal frame. It > > - * should have been handled above by saving the registers > > - * directly. > > - */ > > - if (boot_cpu_has(X86_FEATURE_XSAVES)) { > > - WARN_ONCE(1, "x86/fpu: saving compacted-format xsave area to a signal frame!\n"); > > - return -1; > > - } > > + copy_fpregs_to_fpstate(fpu); > > + fpregs_deactivate(fpu); > > + } > > Could you add a high-level comment for this if{}else{} block that says > something like: > > /* Save the registers to the fpstate. */ > > I also think it's worthwhile to explain the asymmetry between the > ia32_fxstate case and the other branch. Why don't we > fpregs_deactivate() in the ia32_fxstate path, for instance?
Since the ->initialized is gone, the whole hunk here looks differently and probably easier to understand.
> > + if (using_compacted_format()) { > > + copy_xstate_to_user(buf_fx, xsave, 0, size); > > + } else { > > fpstate_sanitize_xstate(fpu); > > - if (__copy_to_user(buf_fx, xsave, fpu_user_xstate_size)) > > + size = fpu_user_xstate_size; > > + if (__copy_to_user(buf_fx, xsave, size)) > > return -1; > > }
dropped this.
> This seems unnecessary. Why are you updating 'size' like this?
Sebastian
| |