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

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

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?

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

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?

> +	}
> +
> +	/* 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.
In fact, what is the purpose?

Also general notes:

 * Please KNF
 * Don't add too many comments when the code is simple enough