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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan