Re: [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2024 20:28, Andrew Cooper wrote:
> @@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
>      return size;
>  }
>  
> -/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
> -unsigned int xstate_ctxt_size(u64 xcr0)
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
>  {
> +    unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
>      if ( xcr0 == xfeature_mask )
>          return xsave_cntxt_size;
>  
>      if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
>          return 0;
>  
> -    return hw_uncompressed_size(xcr0);
> +    if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )

This is open-coded XSTATE_FP_SSE, which I wouldn't mind if ...

> +        return size;
> +
> +    /*
> +     * For the non-legacy states, search all activate states and find the
> +     * maximum offset+size.  Some states (e.g. LWP, APX_F) are out-of-order
> +     * with respect their index.
> +     */
> +    xcr0 &= ~XSTATE_FP_SSE;

... you didn't use that macro here (and once further down). IOW please
be consistent, no matter which way round.

> +    for_each_set_bit ( i, &xcr0, 63 )
> +    {
> +        unsigned int s;
> +
> +        ASSERT(xstate_offsets[i] && xstate_sizes[i]);
> +
> +        s = xstate_offsets[i] && xstate_sizes[i];

You mean + here, don't you? Then:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I'm also inclined to suggest making this the initializer of s.

Jan