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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2024 20:09, Andrew Cooper wrote:
> 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?

Definitely fine with me.

Jan