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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.15 at 16:09, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 21/09/15 15:00, Jan Beulich wrote:
>>>>> 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:
>>>>> +    /* 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.

Oh, I see. For that one, if all code consistently uses cpu_has_*,
then I can't see any issue (not even a theoretical one).

Jan


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