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

[Don Koch <dkoch@xxxxxxxxxxx>]

On Mon, 20 Oct 2014 15:21:19 +0100
Jan Beulich <JBeulich@xxxxxxxx> wrote:

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

This shouldn't be a problem. The original block was zalloced, ergo zeroed,
and it wouldn't have been touched by the xsave if the bit(s) for said
area(s) weren't set. We also don't copy these zeros back in, we're just
sanity checking them.

If we believe the sender, and I don't see any reason not to, then the
size it sent is the size it really wanted. That would mean that the
size check shouldn't be needed, either.

> Jan

-d

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