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 11:21:49 +0100
Jan Beulich <JBeulich@xxxxxxxx> 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.

It was (sort of) checked in an earlier test:
    size = HVM_CPU_XSAVE_SIZE(xfeature_mask);
    if ( desc->length > size )

I suppose we could skip the check as the only other possibility is that
the block sent is larger than what the current architecture can handle
and, if it's all zero, it won't matter (at least in the xsave area).

> >> +        if ( desc->length !=
> >> +             ecx + offsetof(struct hvm_hw_cpu_xsave, save_area) ) {
> >> +            printk(XENLOG_G_WARNING
> >> +                   "HVM%d.%d restore mismatch: xsave length %u != %u\n",
> >> +                   d->domain_id, vcpuid, desc->length, ecx +
> >> +                   (u32)offsetof(struct hvm_hw_cpu_xsave, save_area));
> >> +            return -EOPNOTSUPP;
> >> +        }
> >> +
> >> +        /* Make sure unused bytes are all zero. */
> > 
> > ...which is fine, as we literally only care whether the overflow data is 
> > all zeros of not.
> > 
> > It is is all empty, we really don't care how large it was supposed to be 
> > before.  If it is not empty, then something is certainly corrupt in this 
> > record.
> 
> Not sure - as said above, if we can determine the precise size, then
> I don't see why we shouldn't be making use of this value. 
> 
> >> +        {
> >> +            if ( *(overflow_start + i) )
> >> +            {
> >> +                printk(XENLOG_G_WARNING
> >> +                       "HVM%d.%d restore mismatch: xsave[%d] has non-zero 
> >> data: %x\n",
> >> +                       d->domain_id, vcpuid, i, *(overflow_start +i));
> > 
> > I don't think it is useful to print the value of the first non-zero 
> > byte.  I would reduce it just to "junk found in overflow space".
> 
> I think "non-zero" is better than "junk", but I agree that printing a
> single byte's value here isn't very helpful. Printing the offset, otoh,
> may well provide a clue at what went wrong.

Agreed on both. Will change to something like "has non-zero data at <i>".

> Jan
> 

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