Re: [PATCH 6/6] x86/xstate: Switch back to for_each_set_bit()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/06/2024 11:24 am, Jan Beulich wrote:
> On 25.06.2024 21:07, Andrew Cooper wrote:
>> In all 3 examples, we're iterating over a scaler.  No caller can pass the
>> COMPRESSED flag in, so the upper bound of 63, as opposed to 64, doesn't
>> matter.
> Not sure, maybe more a language question (for my education): Is "can"
> really appropriate here?

It's not the greatest choice, but it's not objectively wrong either.

>  In recalculate_xstate() we calculate the
> value ourselves, but in the two other cases the value is incoming to
> the functions. Architecturally those value should not have bit 63 set,
> but that's weaker than "can" according to my understanding. I'd be
> fine with "may", for example.

There's an ASSERT() in xstate_uncompressed_size() which covers the
property, but most if the justification comes from the fact that the
callers pass in values which are really loaded into hardware registers.

But it is certainly more accurate to say that callers don't pass the
flag in.

There isn't an ASSERT() in xstate_compressed_size(), but I suppose I
could fold this in:

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 88dbfbeafacd..f72f14626b7d 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -623,6 +623,8 @@ unsigned int xstate_compressed_size(uint64_t xstates)
 {
     unsigned int size = XSTATE_AREA_MIN_SIZE;
 
+    ASSERT((xstates & ~(X86_XCR0_STATES | X86_XSS_STATES)) == 0);
+
     if ( xstates == 0 )
         return 0;
 

which brings it more in line with xstate_uncompressed_size(), and has a
side effect of confirming the absence of the COMPRESSED bit.

Thoughts?

>> This alone produces:
>>
>>   add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-161 (-161)
>>   Function                                     old     new   delta
>>   compress_xsave_states                         66      58      -8
>>   xstate_uncompressed_size                     119      71     -48
>>   xstate_compressed_size                       124      76     -48
>>   recalculate_xstate                           347     290     -57
>>
>> where xstate_{un,}compressed_size() have practically halved in size despite
>> being small before.
>>
>> The change in compress_xsave_states() is unexpected.  The function is almost
>> entirely dead code, and within what remains there's a smaller stack frame.  I
>> suspect it's leftovers that the optimiser couldn't fully discard.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Other than the above:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

~Andrew