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
