On Sun, 2019-06-09 at 13:17 +0200, Maxime Villard wrote:
> Le 09/06/2019 à 09:46, Michał Górny a écrit :
> > 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.
>
> I don't see other places that do that. If it was needed there would be a
> race, because it doesn't seem to me we lock the proc, so what if the proc
> goes != SSTOP after the check.
I've copied it from process_machdep_doxmmregs()
in sys/arch/i386/i386/process_machdep.c. I can remove it. Should I
also remove the original occurrence?
>
> > > > + /* 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.
>
> Yes, but EAX is the RFBM, not XSTATE_BV.
I agree I could use a better term here. The idea is that it is a field
stating which of the components in other fields are present.
>
> > > > + }
> > > > +
> > > > + /* 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.
>
> The KASSERT is on 'fpu_save', not 'xstate', so process_verify_xstate() doesn't
> play any role here. My point was: if the traced proc never used SSE, then
> hardware.xstate_bv[SSE]=0 (which means, the "init" state). And the KASSERT
> fires.
Hmm, you are correct. I wrongly assumed the 0 value is specific to
XRSTOR but now I see that it can happen with XSAVE too. In that case,
I suppose I should be forcing it to 1.
> A few other things:
>
> * In process_machdep_doxstate(), need to add a memset(&r, 0, sizeof(r));
I don't think this is strictly necessary. The function unconditionally
calls process_machdep_read_xstate() which will overwrite at least
the header field. The data that is not present according to header can
be unset, I think.
>
> * In process_write_xstate(), need to OR the xstate_bv in 'fpu_save' with
> the xstate_bv in 'xstate', otherwise the next XRSTOR of the traced proc
> may not install the registers correctly
Will do.
>
> * Is there any urgency in adding support for XSTATE in ptrace? Because I am
> currently rewriting the FPU code entirely
Depends on how you define 'urgency'. I can keep the patches local for
now but I'd prefer to have them committed so I could start depending
on the API in LLDB. I'd also like to avoid having to rebase them long-
term.
> * I think it would be easier if you hosted your patches online, because the
> repeated threads make it hard to follow what's going on
I'm pushing my updates to 'xstate' branch on my GitHub src fork:
https://github.com/mgorny/netbsd-src/commits/xstate
--
Best regards,
Michał Górny
Attachment:
signature.asc
Description: This is a digitally signed message part