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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.10.14 at 15:27, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 20/10/14 11:21, Jan Beulich wrote:
>>>>> On 18.10.14 at 01:36, <andrew.cooper3@xxxxxxxxxx> 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.

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

Jan


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