On Sun, 2019-06-09 at 09:08 +0200, Maxime Villard wrote:
> Le 07/06/2019 à 20:33, Michał Górny a écrit :
> > [...]
> > +int
> > +process_machdep_doxstate(struct lwp *curl, struct lwp *l, struct uio *uio)
> > + /* curl: tracer */
> > + /* l: traced */
> > +{
> > + int error;
> > + struct xstate r;
> > + char *kv;
> > + ssize_t kl;
> > +
> > + kl = MIN(uio->uio_iov->iov_len, sizeof(r));
> > + kv = (char *) &r;
> > +
> > + kv += uio->uio_offset;
> > + kl -= uio->uio_offset;
> > + if (kl > uio->uio_resid)
> > + kl = uio->uio_resid;
> > +
> > + if (kl < 0)
> > + error = EINVAL;
> > + else
> > + error = process_machdep_read_xstate(l, &r);
> > + if (error == 0)
> > + error = uiomove(kv, kl, uio);
> > + if (error == 0 && uio->uio_rw == UIO_WRITE) {
> > + if (l->l_proc->p_stat != SSTOP)
> > + error = EBUSY;
>
> Isn't it supposed to always be the case?
To be honest, I've followed suit with all other getter-setters there.
I can't say if it's fail-safe that's never supposed to fire or if there
could be some mistaken use possible that would trigger this.
>
> > + /* Copy MXCSR if either SSE or AVX state is requested */
> > + if (xstate->xs_xstate_bv & (XCR0_SSE|XCR0_YMM_Hi128)) {
> > + memcpy(&fpu_save->sv_xmm.fx_mxcsr, &xstate->xs_fxsave.fx_mxcsr, 8);
> > +
> > + /*
> > + * Invalid bits in mxcsr or mxcsr_mask will cause faults.
> > + */
> > + fpu_save->sv_xmm.fx_mxcsr_mask &= x86_fpu_mxcsr_mask;
> > + fpu_save->sv_xmm.fx_mxcsr &= fpu_save->sv_xmm.fx_mxcsr_mask;
>
> Please use a simple assignment instead of memcpy, and also filter out 'xstate'
> and not 'fpu_save'.
Will do. Actually, simple assignment means I can filter it out while
assigning.
> Also, it would be nice to clarify the use case here. On x86, mxcsr gets reloaded
> *regardless* of whether xstate_bv contains SSE|AVX. Should the ptrace api also
> reload mxcsr regardless of whether the user requested SSE|AVX in xstate_bv?
I was following the Intel programmer's manual. XRSTOR uses mxcsr either
if SSE or AVX is requested via EAX.
> > + }
> > +
> > + /* Copy SSE state if requested. */
> > + if (xstate->xs_xstate_bv & XCR0_SSE) {
> > + if (x86_fpu_save >= FPU_SAVE_XSAVE) {
> > + KASSERT(fpu_save->sv_xsave_hdr.xsh_xstate_bv & XCR0_SSE);
>
> Mmh, maybe it is possible that this KASSERT fires, if the LWP hasn't used SSE.
I don't think that can actually happen with XSAVE. I wouldn't be sure
with XSAVEOPT but we're not using it.
> In fact, what is the purpose?
The exact purpose is to verify that a request to write unsupported
component didn't get through process_verify_xstate(). It's an assert,
so it means to check for something-we-really-don't-expect-to-happen.
> Also general notes:
>
> * Please KNF
> * Don't add too many comments when the code is simple enough
--
Best regards,
Michał Górny
Attachment:
signature.asc
Description: This is a digitally signed message part