Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/09/15 14:04, Jan Beulich wrote:
>>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
>> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
>> uncertainty with how this information is exposed in libxl.  This patch
>> introduces no change with how the information is represented in userspace.
> Mind explaining this "uncertainty"? I'd like to avoid extending the array
> for no real reason...

libxl exports "hw_caps" as uint32_t caps[8] in its API.

I am uncertain what reusing word 2, or extending the length of the array 
means WRT to the API/ABI guarantees of libxl.

For hw_caps itself, the data is essentially useless as there is no 
defined layout,  Furthermore, some of the leaves are 
arbitrary/synthetic.  One option might be to just drop it from libxl 
entirely, but this will need to be decided by the toolstack maintainers.

For my cpuid levelling series, there is a new set exported which has an 
ABI guarantee of which words map to which cpuid feature leaves, and a 
strict rule of no synthetic values.

>
>> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
>>   
>>       /* Check extended XSAVE features. */
>>       cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
>> -    if ( bsp )
>> -    {
>> -        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
>> -        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
>> -        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
>> -        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
>> -    }
>> -    else
>> -    {
>> -        BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
>> -        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
>> -        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
>> -        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
>> -    }
>> +
>> +    /* Mask out features not currently understood by Xen. */
>> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> +            cpufeat_mask(X86_FEATURE_XSAVEC));
>> +
>> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>> +
>> +    if ( !bsp )
>> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>   }
> The !bsp conditional seems pretty pointless. And with the revised
> model it looks like it could be relaxed (BUG only when bits the BSP
> has set aren't set on the AP).

I would be very wary about allowing a situation where certain amounts of 
heterogeneity would be permitted.  Even moreso with the xsaves 
extensions, any non-homogeneity in the system will result in data 
corruption.

I think it is better to keep this as strictly that the BSP must match 
all APs.  As soon as we encounter a system where this is not the case, 
far more areas will also need modifying.

~Andrew