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

[Don Koch <dkoch@xxxxxxxxxxx>]

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

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

OK, will spin V6 shortly.

> Jan

-d

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