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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2021 16:17, Andrew Cooper wrote:
> On 04/05/2021 13:56, Jan Beulich wrote:
>> On 03.05.2021 17:39, Andrew Cooper wrote:
>>> +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.

I'm afraid I'm lost: Neither passing hardware size nor BUG() would
happen in a release build, so getting this wrong does mean memory
corruption there. And I'm of the clear opinion that debug builds
shouldn't differ in behavior in such regards.

If there wasn't an increasing number of possible combinations I
would be inclined to suggest that in all builds we check during
boot that calculation and hardware provided values match for all
possible (valid) combinations.

> 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.

I can fully understand this aspect. I'm not opposed to printk_once()
at all. My comment was purely towards the override.

> 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...

I don't think there's a need here - what you have in terms of info
put into the log is imo sufficient.

Jan