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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.10.14 at 16:11, <dkoch@xxxxxxxxxxx> wrote:
> On Wed, 22 Oct 2014 15:03:23 +0100
> Jan Beulich <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 22.10.14 at 15:19, <dkoch@xxxxxxxxxxx> wrote:
>> > On Wed, 22 Oct 2014 11:00:52 +0100
>> > Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> > 
>> >> >>> On 21.10.14 at 21:25, <dkoch@xxxxxxxxxxx> wrote:
>> >> > On Tue, 21 Oct 2014 20:00:53 +0100
>> >> > Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>> >> > 
>> >> >> On 21/10/14 19:40, Don Koch wrote:
> [...]
>> >> >> > +                printk(XENLOG_G_WARNING
>> >> >> > +                       "HVM%u.%u restore mismatch: xsave length %u 
>> >> >> > > 
>> > %u\n",
>> >> >> > +                       d->domain_id, vcpuid, desc->length, size);
>> >> >> > +                printk(XENLOG_G_WARNING
>> >> >> > +                       "HVM%u.%u restore mismatch: xsave has 
>> >> >> > non-zero 
> data 
>> > starting at %#x\n",
>> >> >> > +                       d->domain_id, vcpuid, i);
>> >> >> 
>> >> >> This should be one message.  Also note that, while a lot of code gets 
>> >> >> it
>> >> >> wrong, domain_id is signed while vcpuid is unsigned.
>> >> > 
>> >> > I had suggested one message. Jan said it should be two.
>> >> 
>> >> Right, and I still think it should be two. Just not the way you did it.
>> >> I specifically said in the reply to the previous version " just add your
>> >> new check ahead of the existing printk()". In case this was ambiguous
>> >> to you - I think the pre-existing printk() should continue to get
>> >> issued (even if not being on an error path anymore) so that we have
>> >> some kind of indication that the truncating path was taken. After all
>> >> this shouldn't happen frequently, considering that the most recent
>> >> stable releases of the older branches already don't do this anymore.
>> > 
>> > I thought that's where I had it. If the block size mismatch is detected,
>> > issue the first message then go into the loop to check for non-zero data
>> > and, if any is found, then issue the second and exit.
>> > 
>> > Andrew, IIUC, didn't want the first one issued unless the non-zero data
>> > case was found, i.e. issue no message unless both conditions were met.
>> > 
>> > So, which should I do?
>> 
>> I'm really getting tired of this; I don't think it's that difficult:
>> 
>> if size too large
>>      loop over extra data
>>              if non-zero
>>                      issue error message
>>                      return
>>      issue warning message
> 
> My only issue with this is seeing just the error:
>    HVM1.1 restore mismatch: xsave has non-zero data starting at 0x232
> would make one wonder, "What's wrong with non-zero data? If it's supposed
> to be zero, why is it being sent in the first place?"

Just make the message text meaningful enough for your liking then.
I personally don't think the text matters too much here - you'll want
to look at the source code anyway when you see this triggering.

Jan


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