Re: [patch v3 1/2] x86, ptrace: regset extensions to support xstate

[Roland McGrath <roland@redhat.com>]

> +	/*
> +	 * First copy the fxsave bytes 0..463.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave, 0,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the 48bytes defined by software.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  xstate_fx_sw_bytes,
> +				  offsetof(struct user_xstateregs,
> +					   i387.xstate_fx_sw),
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Copy the rest of xstate memory layout.
> +	 */
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +				  &target->thread.xstate->xsave.xsave_hdr,
> +				  offsetof(struct user_xstateregs,
> +					   xsave_hdr), -1);
> +	return ret;

I don't know why this didn't occur to me before.  Is there a reason not to
just copy xstate_fx_sw_bytes into the thread.xstate buffer?  You could just
do it right here, or in the places that really touch thread.xstate contents.

Then the trivial single user_regset_copyout() call would be all you need
here, and that seems simpler.  IMHO it's enough simpler that even if you
pay the 6-words memcpy every time here, it's worth it.

If you don't want to do that, that's fine too.  
The code looks correct as it is.

> --- tip.orig/arch/x86/include/asm/user.h
> +++ tip/arch/x86/include/asm/user.h

I don't have any special opinions about the placement, naming, etc. of
these asm/user.h bits (just that there be some symbolic form of the ABI
somewhere).  But perhaps folks want to think about it a bit more before we
bake it into the source API.

These definitions duplicate the asm/processor.h ones used inside the kernel.
IMHO these should be consolidated so asm/processor.h refers to the public types.

I'm not sure whether 'struct user_*' names make most sense for these or not.
They are processor-defined layouts.

> +struct user_ymmh_regs {
> +	/* 16 * 16 bytes for each YMMH-reg */
> +	__u32 ymmh_space[64];
> +};

This is the high half of %ymmN registers, where %xmmN is the low half.
Right?  Though of course documented in the chip manuals, it is rather
nonobvious that %ymmN are split this way in the storage, so I think it
merits a comment here saying so explicitly.


On these cosmetic issues I leave it up to you and the x86 maintainers.
I don't have strong opinions.  But I wanted to air what had occurred to me.

Modulo any cosmetic changes you want to make, this patch has my ACK
contingent on Oleg's ACK.


Thanks,
Roland