Re: [Xen-devel] [V2] x86/xsaves: calculate the xstate_comp_offsets base on xcomp_bv

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.03.16 at 10:46, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> Previous patch using all available features calculate xstate_comp_offsets.
> This is wrong.This patch fix this bug by calculating the xstate_comp_offset
> based on xcomp_bv of current guest.
> Also, the xstate_comp_offset should take alignment into consideration.
> 
> V2: Address comments from Jan:
>  1. code style fix
>  2. setup_xstate_comp take xcomp_bv as param.

This belongs after the first --- separator.

> @@ -106,26 +107,34 @@ 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;

Am I seeing it right that you're allocating an array of unsigned int
just to use a single bit in each element? This should be a bitmap
if so.

> @@ -134,16 +143,19 @@ static void __init setup_xstate_comp(void)
>       * in the fixed offsets in the xsave area in either compacted form
>       * or standard form.
>       */
> -    xstate_comp_offsets[0] = 0;
> +    memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets));

I'm sorry, but sending a new version without addressing _all_
comments on the previous version is a waste of already limited
review bandwidth. Using the static xstate_comp_offsets like
this is wrong.

>      xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
>  
>      xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
>  
>      for ( i = 3; i < xstate_features; i++ )
>      {
> -        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
> -                                 (((1ul << i) & xfeature_mask)
> -                                  ? xstate_sizes[i - 1] : 0);
> +        xstate_comp_offsets[i] = (xstate_align[i] ?
> +                                 ROUNDUP(xstate_comp_offsets[i-1], 64) :

The coding style issue here persists too.

> +                                 xstate_comp_offsets[i - 1]) +
> +                                 (((1ul << i) & xcomp_bv)
> +                                 ? xstate_sizes[i - 1] : 0);

And the - is this actually correct? Why would you enforce alignment
for a component not actually present in the save area? I.e. shouldn't
the assignment be made conditional upon the bit being set? In which
case things might end up being better readable by doing this in two
steps - first align if needed, then simply += sizes[i - 1].

> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -46,6 +46,8 @@
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
>  #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
>  
> +#define XSTATE_ALIGN64 (1ULL << 1)

Why 1ULL (instead of just 1U)?

Jan


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