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

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

On 21/09/15 15:00, Jan Beulich wrote:
>>>> On 21.09.15 at 15:51, <andrew.cooper3@citrix.com> wrote:
>> 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.
> Even more so a reason to re-use word 2.

This works for 0xd:1.eax, but the array has to be extended for 0x7:0.ebx 
and 0x7:0.ecx, both of which are also included in my levelling series.

Currently I have just left the libxl question alone.

>
>>>> @@ -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.
> I guess you misunderstood - I didn't mean for both lines to be
> dropped; I meant the if() surrounding the BUG_ON() to go away.

I don't mind dropping the if(), but I was querying your suggestion in 
brackets.

~Andrew