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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.15 at 15:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 21/09/15 14:04, Jan Beulich wrote:
>>>>> On 17.09.15 at 13:40, <andrew.cooper3@xxxxxxxxxx> 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.

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

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel