Re: [V2 PATCH 8/9] x86/hvm: pkeys, add xstate support for pkeys

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/11/15 09:52, Huaitong Han wrote:
> This patch adds xstate support for pkeys.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
> ---
>  xen/arch/x86/xstate.c        | 18 ++++++++++++++++++
>  xen/include/asm-x86/xstate.h |  5 ++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 827e0e1..00bddb0 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -294,6 +294,24 @@ unsigned int xstate_ctxt_size(u64 xcr0)
>      return _xstate_ctxt_size(xcr0);
>  }
>  
> +/*
> + * Given the xsave area and a state inside, this function returns the
> + * address of the state.
> + *
> + * This is the API that is called to get xstate address in standard format.
> + * Just because XSAVE function does not use compacted format of xsave
> + * area.
> + */
> +void *get_xsave_addr(struct xsave_struct *xsave, u32 xfeature)
> +{
> +    u32 xstate_offsets, xstate_sizes, ecx, edx;
> +    u32 xstate_nr = fls64(xfeature) - 1;
> +
> +    cpuid_count(XSTATE_CPUID, xstate_nr, &xstate_sizes, &xstate_offsets, &ecx, &edx);
> +
> +    return (void *)xsave + xstate_offsets;
> +}
> +

Does this even compile?  There is already

static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)

higher in the same file.

That function should be augmented to take a struct xsave_struct *xsave,
look at whether the representation is compressed or not, and use the
appropriate offset array.

>  /* Collect the information of processor's extended state */
>  void xstate_init(struct cpuinfo_x86 *c)
>  {
> diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
> index b95a5b5..e9abe71 100644
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -34,13 +34,15 @@
>  #define XSTATE_OPMASK  (1ULL << 5)
>  #define XSTATE_ZMM     (1ULL << 6)
>  #define XSTATE_HI_ZMM  (1ULL << 7)
> +#define XSTATE_UNUSED  (1ULL << 8)

No need for this.

~Andrew