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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/05/2024 1:19 pm, Jan Beulich wrote:
> 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.

Oh yes - that's a consequence of these hunks having between written 3
years apart.

It's important for the first one (logical comparison against a bitmap)
that it's split apart.

>
>> +    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?

Yes I do...  That was a victim of a last minute refactor.

It also shows that even the cross-check with hardware isn't terribly
effective.  More on that in the other thread.

>  Then:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> I'm also inclined to suggest making this the initializer of s.

Hmm, good point.  Will change.

Thanks,

~Andrew