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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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