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

[Don Koch <dkoch@xxxxxxxxxxx>]

On Tue, 21 Oct 2014 09:57:18 +0100
Jan Beulich <JBeulich@xxxxxxxx> wrote:

> >>> On 20.10.14 at 22:40, <dkoch@xxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1971,6 +1971,7 @@ static int hvm_load_cpu_xsave_states(struct domain 
> > *d, 
> > hvm_domain_context_t *h)
> >      struct vcpu *v;
> >      struct hvm_hw_cpu_xsave *ctxt;
> >      struct hvm_save_descriptor *desc;
> > +    int i, overflow_start;
> 
> unsigned int

Will change.

> > @@ -2020,6 +2021,7 @@ static int hvm_load_cpu_xsave_states(struct domain 
> > *d, 
> > hvm_domain_context_t *h)
> >          return -EOPNOTSUPP;
> >      }
> >      h->cur += sizeof (*desc);
> > +    overflow_start = h->cur;
> >  
> >      ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
> >      h->cur += desc->length;
> > @@ -2041,7 +2043,18 @@ static int hvm_load_cpu_xsave_states(struct domain 
> > *d, hvm_domain_context_t *h)
> >          printk(XENLOG_G_WARNING
> >                 "HVM%d.%d restore mismatch: xsave length %u > %u\n",
> >                 d->domain_id, vcpuid, desc->length, size);
> > -        return -EOPNOTSUPP;
> > +
> > +        /* Make sure missing bytes are all zero. */
> > +        for ( i = size; i < desc->length; i++ )
> > +        {
> > +            if ( h->data[overflow_start + i] )
> > +            {
> > +                printk(XENLOG_G_WARNING
> > +                       "HVM%d.%d restore mismatch: xsave has non-zero data 
> > starting at %d\n",
> > +                       d->domain_id, vcpuid, i);
> > +                return -EOPNOTSUPP;
> 
> You were asked to avoid issuing two messages in this case, and iirc
> you indicated you would adjust your patch to do so, yet nothing
> really changed here. Also please print offsets in hex (%#x) and
> don't further proliferate the wrong format specifier used for vcpuid
> into newly added messages: vcpuid is "unsigned int" and hence
> wants to be printed using %u.

Sorry, slipped my mind when reformatting. Didn't notice the
unsignedness of vcpuid and just copied what was in the other prints;
will change.

Actually, the request was to elide the first message if the zero check
passed, which I interpret as: if we find a non-zero value, issue both,
else issue neither. I can either print both or, if you wish, print a
combined message, something along the line of "non-zero data found in
oversized buffer..." (which I think I prefer).

> And finally iirc you also indicated you'd drop the full-size check
> (against HVM_CPU_XSAVE_SIZE(xfeature_mask)), which we
> identified to be wrong in case the origin machine had bigger
> xsave state than the receiving one.

I dropped MY full size check, the one based on the target machine's idea of
full sized.

Do we want to drop the original check, too? I'm assuming you mean the one that
starts out:
    size = HVM_CPU_XSAVE_SIZE(xfeature_mask);
    if ( desc->length > size )
at (or near) 2015. If so, will drop the size assignment and if block.

> Jan

-d


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