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 <[email protected]>
>
> I'm also inclined to suggest making this the initializer of s.
Hmm, good point. Will change.
Thanks,
~Andrew
