tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

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

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 :
+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

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

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

Home | Main Index | Thread Index | Old Index