Re: [PATCH 5/5] x86/cpuid: Fix handling of xsave dynamic leaves

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/05/2021 13:56, Jan Beulich wrote:
> On 03.05.2021 17:39, Andrew Cooper wrote:
>> If max leaf is greater than 0xd but xsave not available to the guest, then 
>> the
>> current XSAVE size should not be filled in.  This is a latent bug for now as
>> the guest max leaf is 0xd, but will become problematic in the future.
>>
>> The comment concerning XSS state is wrong.  VT-x doesn't manage host/guest
>> state automatically, but there is provision for "host only" bits to be set, 
>> so
>> the implications are still accurate.
>>
>> Introduce {xstate,hw}_compressed_size() helpers to mirror the uncompressed
>> ones.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> albeit with a remark:
>
>> +unsigned int xstate_compressed_size(uint64_t xstates)
>> +{
>> +    unsigned int i, size = XSTATE_AREA_MIN_SIZE;
>> +
>> +    xstates &= ~XSTATE_FP_SSE;
>> +    for_each_set_bit ( i, &xstates, 63 )
>> +    {
>> +        if ( test_bit(i, &xstate_align) )
>> +            size = ROUNDUP(size, 64);
>> +
>> +        size += xstate_sizes[i];
>> +    }
>> +
>> +    /* In debug builds, cross-check our calculation with hardware. */
>> +    if ( IS_ENABLED(CONFIG_DEBUG) )
>> +    {
>> +        unsigned int hwsize;
>> +
>> +        xstates |= XSTATE_FP_SSE;
>> +        hwsize = hw_compressed_size(xstates);
>> +
>> +        if ( size != hwsize )
>> +            printk_once(XENLOG_ERR "%s(%#"PRIx64") size %#x != hwsize 
>> %#x\n",
>> +                        __func__, xstates, size, hwsize);
>> +        size = hwsize;
> To be honest, already on the earlier patch I was wondering whether
> it does any good to override size here: That'll lead to different
> behavior on debug vs release builds. If the log message is not
> paid attention to, we'd then end up with longer term breakage.

Well - our options are pass hardware size, or BUG(), because getting
this wrong will cause memory corruption.

The BUG() option is a total pain when developing new support - the first
version of this patch did use BUG(), but after hitting it 4 times in a
row (caused by issues with constants elsewhere), I decided against it.

If we had something which was a mix between WARN_ONCE() and a proper
printk() explaining what was going on, then I would have used that. 
Maybe it's time to introduce one...

~Andrew