Re: [Xen-devel] [PATCH 1/3] x86/xsaves: caculate the xstate_comp_offsets base on xcomp_bv

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.02.16 at 06:35, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> Previous patch using all available features caculate xstate_comp_offsets.
> This is wrong.This patch fix this bug by caculating the xstate_comp_offset

In the title and above: calculate (and alike).

> based on xcomp_bv of current guest.
> Also, the xstate_comp_offset should take alignment into consideration.

Reported-by: Jan Beulich <jbeulich@xxxxxxxx>

> Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
[...]
> @@ -106,34 +107,44 @@ static int setup_xstate_features(bool_t bsp)
>          xstate_sizes = xzalloc_array(unsigned int, xstate_features);
>          if ( !xstate_sizes )
>              return -ENOMEM;
> +
> +        xstate_align = xzalloc_array(unsigned int, xstate_features);
> +        if ( !xstate_align )
> +            return -ENOMEM;
>      }
>  
>      for ( leaf = 2; leaf < xstate_features; leaf++ )
>      {
>          if ( bsp )
> +     {
>              cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
> -                        &xstate_offsets[leaf], &tmp, &tmp);
> +                        &xstate_offsets[leaf], &ecx, &edx);
> +            xstate_align[leaf] = ecx & XSTATE_ALIGN64;
> +     }

Bogus hard tabs.

> -static void __init setup_xstate_comp(void)
> +static void setup_xstate_comp(const struct xsave_struct *xsave)
>  {
>      unsigned int i;
> +    u64 xcomp_bv =  xsave->xsave_hdr.xcomp_bv;

It looks like it would suffice if the caller passed xcomp_bv into
this function.

>      /*
>       * The FP xstates and SSE xstates are legacy states. They are always
>       * in the fixed offsets in the xsave area in either compacted form
>       * or standard form.
>       */
> +    memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets));
>      xstate_comp_offsets[0] = 0;

The addition makes this line redundant. However, you act on
static data here, and I don't think there's any serialization
between the various callers.

> @@ -141,8 +152,10 @@ static void __init setup_xstate_comp(void)
>  
>      for ( i = 3; i < xstate_features; i++ )
>      {
> -        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
> -                                 (((1ul << i) & xfeature_mask)
> +        xstate_comp_offsets[i] = (xstate_align[i] ?
> +                                  ROUNDUP(xstate_comp_offsets[i-1], 64) :

Coding style.

> @@ -172,6 +185,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>      }
>  
>      ASSERT(xsave_area_compressed(xsave));
> +    setup_xstate_comp(xsave);
>      /*

Please add a blank line before the comment.

> @@ -223,6 +237,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>      xsave->xsave_hdr.xstate_bv = xstate_bv;
>      xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | 
> XSTATE_COMPACTION_ENABLED;
>  
> +    setup_xstate_comp(xsave);
>      /*

Please retain a blank line before the comment.

> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -44,6 +44,7 @@
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
>  #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
>  
> +#define XSTATE_ALIGN64 (1ULL << 1)
>  extern u64 xfeature_mask;

Missing blank line again.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel