Re: [patch v2 2/4] x86, ptrace: regset extensions to support xstate

Roland McGrath
2010-02-10T01:30:51Z

> Add the xstate regset support which helps extend the kernel ptrace and the
> core-dump interfaces to support AVX state etc.
>
> This regset interface is designed to support all the future state that gets
> supported using xsave/xrstor infrastructure.

Looks good modulo cosmetic nits below.

> And hence the xsave memory layout available through this regset
> interface uses SW usable bytes [464..511] to convey what state is represented
> in the memory layout.
>
> First 8 bytes of the sw_usable_bytes[464..467] will be set to OS enabled xstate
> mask(which is same as the 64bit mask returned by the xgetbv's xCR0).

I'd like to see some macros or struct or something in some user-visible
header (ptrace-abi.h I guess?) that instruct userland where to find this
software-defined word. The rest of the layout and meaning is specified by
the chip manuals and it's only a question of convenience if we want to help
userland out with those bits. But the use of the reserved-for-software
area to store a bitmask (or whatever else Linux might put there in the
future) is entirely a Linux invention that needs to be a clear and explicit
part of this userland ABI format.

> +/*
> + * The xfpregs_active() routine is same as the fpregs_active() routine,

s/xfpregs_active/xstateregs_active/
s/is same/is the same/

> @@ -204,8 +209,6 @@ int xfpregs_set(struct task_struct *targ
> if (ret)
> return ret;
>
> - set_stopped_child_used_math(target);
> -

You didn't mention this change in a comment or log entry.
It looks like it's superfluous after init_fpu. So this change
is right, but AFAICT it belongs in an entirely separate patch
and has nothing to do with xsave support.

> +int xstateregs_get(struct task_struct *target, const struct user_regset *regset,
> + unsigned int pos, unsigned int count,
> + void *kbuf, void __user *ubuf)
> +{
> + int ret;
> + int size = regset->n * regset->size;
[...]
> + /*
> + * Copy the rest of xstate memory layout
> + */
> + ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> + xsave_hdr,
> + offsetof(struct xsave_struct,
> + xsave_hdr), size);

This calculation is unnecessary (though it doesn't hurt); you can just use
-1 here. The arch user_regset.get and user_regset.set functions are not
required to worry about bogus arguments. Their callers are required to
pass values that don't violate the .n, .size, and .align constraints in the
user_regset.

> +int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
> + unsigned int pos, unsigned int count,
> + const void *kbuf, const void __user *ubuf)
> +{
> + int ret;
> + int size = regset->n * regset->size;
[...]
> + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> + &target->thread.xstate->xsave, 0, size);

Same here.

> + [REGSET_XSTATE] = {
> + .core_note_type = NT_X86_XSTATE,
> + .size = sizeof(long), .align = sizeof(long),
[...]
> + [REGSET_XSTATE] = {
> + .core_note_type = NT_X86_XSTATE,
> + .size = sizeof(u32), .align = sizeof(u32),

Isn't the xsave format is really the same for 32-bit and 64-bit?
All its components are naturally 64 bits or larger, right?

If that's correct, I think it would be better to make the 32 and 64 version
of the regset uniform with .size and .align being sizeof(u64).

> +u64 xstate_fx_sw_bytes[6];
> +void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
> +{
> +#ifdef CONFIG_X86_64
> + x86_64_regsets[REGSET_XSTATE].n = size / sizeof(long);
> +#endif
> +#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> + x86_32_regsets[REGSET_XSTATE].n = size / sizeof(u32);
> +#endif

Just change these to / sizeof(u64) accordingly.

Thanks,
Roland