Re: [PATCH v2] Sanity check xsave area when migrating or restoring from older Xen verions

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

On 20/10/14 15:21, Jan Beulich wrote:
>>>> On 20.10.14 at 15:27, <andrew.cooper3@citrix.com> wrote:
>> On 20/10/14 11:21, Jan Beulich wrote:
>>>>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> wrote:
>>>> On 17/10/2014 18:11, Don Koch wrote:
>>>>> +
>>>>> +        /* Check to see if the xsave_area is the maximum size.
>>>>> +           If so, it is likely the save is from an older xen. */
>>>>> +        cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
>>>> This check is bogus for heterogeneous hardware.  We have no way of 
>>>> calculating what the maximum xsave area size was on the sending side 
>>>> should have been...
>>> Actually we have a way, using the xfeature_mask field that you
>>> made being ignored a while back. And I think applying sanity
>>> checks where they can be applied is a good thing. But of course
>>> we can't blindly compare against the full size found on the receiving
>>> host. We could get the size from xstate_ctxt_size() unless the
>>> sending host had features we don't have, in which case we'd need
>>> to resort to manually calculating the value.
>> I am not in favour of reinstating that check.
>>
>> Whether the state was valid for the sending side, is not something the
>> receiving side should care about.
> I can see your point, and mostly agree. Nevertheless, at least for the
> record, two related comments further down:
>
>> All the receiving side should care about is whether the state received
>> is valid.  In this case, reinstating the check still doesn't allow us to
>> correctly calculate the size, and manually doing so is fragile and very
>> prone to error.
> I don't think there's much room for errors here - all the offsets and
> sizes are well defined, and hence just require being put in e.g. a
> static table.

They are different between Intel and AMD.  For better or for worse, we
do support cross-vendor migration (for a sufficiently feature-stripped
VM), and have no way of distinguishing sending-vendor given the
migration stream.

>
>> If the record is overly long, but the trailing space is all zeroes, the
>> state is valid whether or not it is the correct length for the sending side.
> The problem is - this is true only as long as the default values for
> that state are zero. Considering that the base state already
> violates this, I don't see there being a guarantee for this to be true
> for all future extensions.

Hmm.  This is indeed a concern I had overlooked.

Furthermore, there is now a compact XSAVE area defined by Intel. 
Support for this is defined in the XSAVE header, not xcr0, so
calculating the expected size based on xcr0 is going to get more
complicated in the future, if we decide to make use of this feature.

In the case that we receive an overly long record, we are still
calculating length based on xcr0_accum.  This means that even if the
extra xsave area defaults are non0, the guest has not seen them yet.  In
this case, the defaults from the current hardware are just as good as
the defaults from the old hardware, in the case that they differ.

All of this is quite a mess.

~Andrew