Re: [PATCH v3 2/2] Implement PT_GETXSTATE and PT_SETXSTATE

[Maxime Villard <max%m00nbsd.net@localhost>]

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.

> > > +	/* 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.

> > > +	}
> > > +
> > > +	/* 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.

A few other things:

 * In process_machdep_doxstate(), need to add a memset(&r, 0, sizeof(r));

 * 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

 * Is there any urgency in adding support for XSTATE in ptrace? Because I am
   currently rewriting the FPU code entirely

 * 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