Re: [V3 PATCH 6/9] x86/hvm: pkeys, add xstate support for pkeys

[George Dunlap <george.dunlap@citrix.com>]

On 07/12/15 09:16, Huaitong Han wrote:
> This patch adds xstate support for pkeys.

Hey Huaitong,

Hope you don't mind me giving you a little feedback on the way you've
broken down your patches here.  The purpose for breaking a change down
into separate patches like this is to make it easier for people
reviewing (and for people later who come and look at the commits) to
figure out what's going on.  But if you break the patch series down to
much, you go back to making things harder again.

Take this patch, for instance.  You're making get_xsave_addr() global
(non-static), and also making it handle the case where the xsave area
was compressed (which it didn't before).

But as a reviewer, I don't know at this point who is calling
get_xsave_addr() or why; I don't know if this change is necessary, or if
it is correct for all the callers (or if the change wrt compressed xsave
areas is even necessary).

So one problem with this patch is that you don't include that
information in the description.  But part of it is that the change
doesn't really make much sense by itself.

I think I'd squash patches 4-7 into a single patch.

More comments about the approach in patch 7.

 -George


> 
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
> ---
>  xen/arch/x86/xstate.c        | 7 +++++--
>  xen/include/asm-x86/xstate.h | 4 +++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index b65da38..db978c4 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -146,12 +146,15 @@ static void __init setup_xstate_comp(void)
>      }
>  }
>  
> -static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
> +void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
>  {
>      if ( !((1ul << xfeature_idx) & xfeature_mask) )
>          return NULL;
>  
> -    return xsave + xstate_comp_offsets[xfeature_idx];
> +    if ( xsave_area_compressed(xsave) )
> +        return xsave + xstate_comp_offsets[xfeature_idx];
> +    else
> +        return xsave + xstate_offsets[xfeature_idx];
>  }
>  
>  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
> index 12d939b..6536813 100644
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -34,13 +34,14 @@
>  #define XSTATE_OPMASK  (1ULL << 5)
>  #define XSTATE_ZMM     (1ULL << 6)
>  #define XSTATE_HI_ZMM  (1ULL << 7)
> +#define XSTATE_PKRU    (1ULL << 9)
>  #define XSTATE_LWP     (1ULL << 62) /* AMD lightweight profiling */
>  #define XSTATE_FP_SSE  (XSTATE_FP | XSTATE_SSE)
>  #define XCNTXT_MASK    (XSTATE_FP | XSTATE_SSE | XSTATE_YMM | XSTATE_OPMASK | \
>                          XSTATE_ZMM | XSTATE_HI_ZMM | XSTATE_NONLAZY)
>  
>  #define XSTATE_ALL     (~(1ULL << 63))
> -#define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR)
> +#define XSTATE_NONLAZY (XSTATE_LWP | XSTATE_BNDREGS | XSTATE_BNDCSR | XSTATE_PKRU)
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
>  #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
>  
> @@ -90,6 +91,7 @@ uint64_t get_msr_xss(void);
>  void xsave(struct vcpu *v, uint64_t mask);
>  void xrstor(struct vcpu *v, uint64_t mask);
>  bool_t xsave_enabled(const struct vcpu *v);
> +void *get_xsave_addr(void *xsave, unsigned int xfeature_idx);
>  int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum, u64 xstate_bv);
>  int __must_check handle_xsetbv(u32 index, u64 new_bv);
>  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
>