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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.05.2024 13:16, Andrew Cooper wrote:
> @@ -611,6 +587,40 @@ static bool valid_xcr0(uint64_t xcr0)
>      return true;
>  }
>  
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
> +{
> +    unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
> +    ASSERT((xcr0 & ~X86_XCR0_STATES) == 0);

I'm puzzled by the combination of this assertion and ...

> +    if ( xcr0 == xfeature_mask )
> +        return xsave_cntxt_size;

... this conditional return. Yes, right now we don't support/use any XSS
components, but without any comment the assertion looks overly restrictive
to me.

> @@ -818,14 +834,14 @@ void xstate_init(struct cpuinfo_x86 *c)
>           * xsave_cntxt_size is the max size required by enabled features.
>           * We know FP/SSE and YMM about eax, and nothing about edx at 
> present.
>           */
> -        xsave_cntxt_size = hw_uncompressed_size(feature_mask);
> +        xsave_cntxt_size = cpuid_count_ebx(0xd, 0);
>          printk("xstate: size: %#x and states: %#"PRIx64"\n",
>                 xsave_cntxt_size, xfeature_mask);
>      }
>      else
>      {
>          BUG_ON(xfeature_mask != feature_mask);
> -        BUG_ON(xsave_cntxt_size != hw_uncompressed_size(feature_mask));
> +        BUG_ON(xsave_cntxt_size != cpuid_count_ebx(0xd, 0));
>      }

Hmm, this may make re-basing of said earlier patch touching this code yet
more interesting. Or maybe it actually simplifies things, will need to see
... The overall comment remains though: Patches pending for so long should
really take priority over creating yet more new ones. But what do I do - I
can't enforce this, unless I was now going to block your work the same way.
Which I don't mean to do.

Jan