* [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions @ 2014-10-21 18:40 Don Koch 2014-10-21 19:00 ` Andrew Cooper 0 siblings, 1 reply; 11+ messages in thread From: Don Koch @ 2014-10-21 18:40 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Keir Fraser, Don Koch, Jan Beulich Xen 4.3 and older transferred a maximum sized xsave area (as if all the available XCR0 bits were set); the new version only transfers based on the actual XCR0 bits. This may result in a smaller area if the last sections were missing (e.g., the LWP area from an AMD machine). If the size doesn't match the XCR0 derived size, the part of the xsave area between the XCR0 specified and transferred size is checked for zero data. If any part of the overflow area is non-zero, we return with an error. Signed-off-by: Don Koch <dkoch@verizon.com> --- Changes in V4: - Removed check of size base on xfeature_mask. - Unsign some ints. - Change %d to %u for unsigned ints. - Move printk to only print if non-zero data found. Changes in V3: - use h->data for zero check - remove max size check (use size that was sent) - fix error message (drop first byte value) - fix "for" issues Changes in V2: - Add check for size. - Add check for non-zero data in unused part of block. xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index f0e1edc..c2780d2 100644 --- 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; + unsigned int i, overflow_start; /* Which vcpu is this? */ vcpuid = hvm_load_instance(h); @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) save_area) + XSTATE_AREA_MIN_SIZE); return -EINVAL; } - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); - if ( desc->length > size ) - { - printk(XENLOG_G_WARNING - "HVM%d.%d restore mismatch: xsave length %u > %u\n", - d->domain_id, vcpuid, desc->length, size); - return -EOPNOTSUPP; - } h->cur += sizeof (*desc); + overflow_start = h->cur; ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; h->cur += desc->length; @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); if ( desc->length > size ) { - 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%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); + return -EOPNOTSUPP; + } + } } /* Checking finished */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-21 18:40 [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions Don Koch @ 2014-10-21 19:00 ` Andrew Cooper 2014-10-21 19:25 ` Don Koch 0 siblings, 1 reply; 11+ messages in thread From: Andrew Cooper @ 2014-10-21 19:00 UTC (permalink / raw) To: Don Koch, xen-devel; +Cc: Keir Fraser, Jan Beulich On 21/10/14 19:40, Don Koch wrote: > Xen 4.3 and older transferred a maximum sized xsave area (as if all > the available XCR0 bits were set); the new version only transfers > based on the actual XCR0 bits. This may result in a smaller area if > the last sections were missing (e.g., the LWP area from an AMD > machine). If the size doesn't match the XCR0 derived size, the part of > the xsave area between the XCR0 specified and transferred size is > checked for zero data. If any part of the overflow area is non-zero, > we return with an error. > > Signed-off-by: Don Koch <dkoch@verizon.com> > --- > Changes in V4: > - Removed check of size base on xfeature_mask. > - Unsign some ints. > - Change %d to %u for unsigned ints. > - Move printk to only print if non-zero data found. > > Changes in V3: > - use h->data for zero check > - remove max size check (use size that was sent) > - fix error message (drop first byte value) > - fix "for" issues > > Changes in V2: > - Add check for size. > - Add check for non-zero data in unused part of block. > > xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index f0e1edc..c2780d2 100644 > --- 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; > + unsigned int i, overflow_start; > > /* Which vcpu is this? */ > vcpuid = hvm_load_instance(h); > @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) > save_area) + XSTATE_AREA_MIN_SIZE); > return -EINVAL; > } > - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); > - if ( desc->length > size ) > - { > - printk(XENLOG_G_WARNING > - "HVM%d.%d restore mismatch: xsave length %u > %u\n", > - d->domain_id, vcpuid, desc->length, size); > - return -EOPNOTSUPP; > - } > h->cur += sizeof (*desc); > + overflow_start = h->cur; > > ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; > h->cur += desc->length; > @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) > size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); > if ( desc->length > size ) > { > - 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. */ Please make a reference to the bug in this comment, so the reasons for the strange check is a little more obvious given a glance at the code. Perhaps /* * Xen-4.3 and older used to send longer-than-needed xsave regions. Permit loading the record if the extra data is all zero */ (suitably wrapped, given its natural indentation) > + for ( i = size; i < desc->length; i++ ) > + { > + if ( h->data[overflow_start + i] ) > + { > + 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. Perhaps "HVM%d.%u restore: xsave length %#x > %#x with non-zero data at %#x\n" It is quite unhelpful to report 3 related numbers, two in one base with one in a different base. I feel hex is more useful here, when comparing the offsets against the manuals. ~Andrew > + return -EOPNOTSUPP; > + } > + } > } > /* Checking finished */ > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-21 19:00 ` Andrew Cooper @ 2014-10-21 19:25 ` Don Koch 2014-10-22 10:00 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Don Koch @ 2014-10-21 19:25 UTC (permalink / raw) To: Andrew Cooper; +Cc: Keir Fraser, Jan Beulich, xen-devel On Tue, 21 Oct 2014 20:00:53 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 21/10/14 19:40, Don Koch wrote: > > Xen 4.3 and older transferred a maximum sized xsave area (as if all > > the available XCR0 bits were set); the new version only transfers > > based on the actual XCR0 bits. This may result in a smaller area if > > the last sections were missing (e.g., the LWP area from an AMD > > machine). If the size doesn't match the XCR0 derived size, the part of > > the xsave area between the XCR0 specified and transferred size is > > checked for zero data. If any part of the overflow area is non-zero, > > we return with an error. > > > > Signed-off-by: Don Koch <dkoch@verizon.com> > > --- > > Changes in V4: > > - Removed check of size base on xfeature_mask. > > - Unsign some ints. > > - Change %d to %u for unsigned ints. > > - Move printk to only print if non-zero data found. > > > > Changes in V3: > > - use h->data for zero check > > - remove max size check (use size that was sent) > > - fix error message (drop first byte value) > > - fix "for" issues > > > > Changes in V2: > > - Add check for size. > > - Add check for non-zero data in unused part of block. > > > > xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > > index f0e1edc..c2780d2 100644 > > --- 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; > > + unsigned int i, overflow_start; > > > > /* Which vcpu is this? */ > > vcpuid = hvm_load_instance(h); > > @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) > > save_area) + XSTATE_AREA_MIN_SIZE); > > return -EINVAL; > > } > > - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); > > - if ( desc->length > size ) > > - { > > - printk(XENLOG_G_WARNING > > - "HVM%d.%d restore mismatch: xsave length %u > %u\n", > > - d->domain_id, vcpuid, desc->length, size); > > - return -EOPNOTSUPP; > > - } > > h->cur += sizeof (*desc); > > + overflow_start = h->cur; > > > > ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; > > h->cur += desc->length; > > @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h) > > size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); > > if ( desc->length > size ) > > { > > - 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. */ > > Please make a reference to the bug in this comment, so the reasons for > the strange check is a little more obvious given a glance at the code. > > Perhaps > > /* > * Xen-4.3 and older used to send longer-than-needed xsave regions. > Permit loading the record if the extra data is all zero > */ > > (suitably wrapped, given its natural indentation) OK, will do. > > + for ( i = size; i < desc->length; i++ ) > > + { > > + if ( h->data[overflow_start + i] ) > > + { > > + 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. I'll change it to one combined message (after letting Jan get his say). Also, after dredging through the includes (and I easily may have taken a wrong turn) it looks like domain_id in this structure is declared as a domid_t which is typedefed as an uint16_t. But, I'll change it back to %d if you insist. > Perhaps > > "HVM%d.%u restore: xsave length %#x > %#x with non-zero data at %#x\n" > > It is quite unhelpful to report 3 related numbers, two in one base with > one in a different base. I feel hex is more useful here, when comparing > the offsets against the manuals. Sounds good to me. > ~Andrew Thanks, -d > > + return -EOPNOTSUPP; > > + } > > + } > > } > > /* Checking finished */ > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-21 19:25 ` Don Koch @ 2014-10-22 10:00 ` Jan Beulich 2014-10-22 13:19 ` Don Koch 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2014-10-22 10:00 UTC (permalink / raw) To: Andrew Cooper, Don Koch; +Cc: Keir Fraser, xen-devel >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: > On Tue, 21 Oct 2014 20:00:53 +0100 > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 21/10/14 19:40, Don Koch wrote: >> > Xen 4.3 and older transferred a maximum sized xsave area (as if all >> > the available XCR0 bits were set); the new version only transfers >> > based on the actual XCR0 bits. This may result in a smaller area if >> > the last sections were missing (e.g., the LWP area from an AMD >> > machine). If the size doesn't match the XCR0 derived size, the part of >> > the xsave area between the XCR0 specified and transferred size is >> > checked for zero data. If any part of the overflow area is non-zero, >> > we return with an error. >> > >> > Signed-off-by: Don Koch <dkoch@verizon.com> >> > --- >> > Changes in V4: >> > - Removed check of size base on xfeature_mask. >> > - Unsign some ints. >> > - Change %d to %u for unsigned ints. >> > - Move printk to only print if non-zero data found. >> > >> > Changes in V3: >> > - use h->data for zero check >> > - remove max size check (use size that was sent) >> > - fix error message (drop first byte value) >> > - fix "for" issues >> > >> > Changes in V2: >> > - Add check for size. >> > - Add check for non-zero data in unused part of block. >> > >> > xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ >> > 1 file changed, 16 insertions(+), 12 deletions(-) >> > >> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> > index f0e1edc..c2780d2 100644 >> > --- 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; >> > + unsigned int i, overflow_start; >> > >> > /* Which vcpu is this? */ >> > vcpuid = hvm_load_instance(h); >> > @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain > *d, hvm_domain_context_t *h) >> > save_area) + XSTATE_AREA_MIN_SIZE); >> > return -EINVAL; >> > } >> > - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); >> > - if ( desc->length > size ) >> > - { >> > - printk(XENLOG_G_WARNING >> > - "HVM%d.%d restore mismatch: xsave length %u > %u\n", >> > - d->domain_id, vcpuid, desc->length, size); >> > - return -EOPNOTSUPP; >> > - } >> > h->cur += sizeof (*desc); >> > + overflow_start = h->cur; >> > >> > ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; >> > h->cur += desc->length; >> > @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain > *d, hvm_domain_context_t *h) >> > size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); >> > if ( desc->length > size ) >> > { >> > - 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. */ >> >> Please make a reference to the bug in this comment, so the reasons for >> the strange check is a little more obvious given a glance at the code. >> >> Perhaps >> >> /* >> * Xen-4.3 and older used to send longer-than-needed xsave regions. >> Permit loading the record if the extra data is all zero >> */ >> >> (suitably wrapped, given its natural indentation) > > OK, will do. > >> > + for ( i = size; i < desc->length; i++ ) >> > + { >> > + if ( h->data[overflow_start + i] ) >> > + { >> > + 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. Jan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 10:00 ` Jan Beulich @ 2014-10-22 13:19 ` Don Koch 2014-10-22 13:21 ` Andrew Cooper 2014-10-22 14:03 ` Jan Beulich 0 siblings, 2 replies; 11+ messages in thread From: Don Koch @ 2014-10-22 13:19 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel On Wed, 22 Oct 2014 11:00:52 +0100 Jan Beulich <JBeulich@suse.com> wrote: > >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: > > On Tue, 21 Oct 2014 20:00:53 +0100 > > Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > >> On 21/10/14 19:40, Don Koch wrote: > >> > Xen 4.3 and older transferred a maximum sized xsave area (as if all > >> > the available XCR0 bits were set); the new version only transfers > >> > based on the actual XCR0 bits. This may result in a smaller area if > >> > the last sections were missing (e.g., the LWP area from an AMD > >> > machine). If the size doesn't match the XCR0 derived size, the part of > >> > the xsave area between the XCR0 specified and transferred size is > >> > checked for zero data. If any part of the overflow area is non-zero, > >> > we return with an error. > >> > > >> > Signed-off-by: Don Koch <dkoch@verizon.com> > >> > --- > >> > Changes in V4: > >> > - Removed check of size base on xfeature_mask. > >> > - Unsign some ints. > >> > - Change %d to %u for unsigned ints. > >> > - Move printk to only print if non-zero data found. > >> > > >> > Changes in V3: > >> > - use h->data for zero check > >> > - remove max size check (use size that was sent) > >> > - fix error message (drop first byte value) > >> > - fix "for" issues > >> > > >> > Changes in V2: > >> > - Add check for size. > >> > - Add check for non-zero data in unused part of block. > >> > > >> > xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ > >> > 1 file changed, 16 insertions(+), 12 deletions(-) > >> > > >> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > >> > index f0e1edc..c2780d2 100644 > >> > --- 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; > >> > + unsigned int i, overflow_start; > >> > > >> > /* Which vcpu is this? */ > >> > vcpuid = hvm_load_instance(h); > >> > @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain > > *d, hvm_domain_context_t *h) > >> > save_area) + XSTATE_AREA_MIN_SIZE); > >> > return -EINVAL; > >> > } > >> > - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); > >> > - if ( desc->length > size ) > >> > - { > >> > - printk(XENLOG_G_WARNING > >> > - "HVM%d.%d restore mismatch: xsave length %u > %u\n", > >> > - d->domain_id, vcpuid, desc->length, size); > >> > - return -EOPNOTSUPP; > >> > - } > >> > h->cur += sizeof (*desc); > >> > + overflow_start = h->cur; > >> > > >> > ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; > >> > h->cur += desc->length; > >> > @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain > > *d, hvm_domain_context_t *h) > >> > size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); > >> > if ( desc->length > size ) > >> > { > >> > - 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. */ > >> > >> Please make a reference to the bug in this comment, so the reasons for > >> the strange check is a little more obvious given a glance at the code. > >> > >> Perhaps > >> > >> /* > >> * Xen-4.3 and older used to send longer-than-needed xsave regions. > >> Permit loading the record if the extra data is all zero > >> */ > >> > >> (suitably wrapped, given its natural indentation) > > > > OK, will do. > > > >> > + for ( i = size; i < desc->length; i++ ) > >> > + { > >> > + if ( h->data[overflow_start + i] ) > >> > + { > >> > + 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? > Jan Confusedly yours, -d ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 13:19 ` Don Koch @ 2014-10-22 13:21 ` Andrew Cooper 2014-10-22 13:37 ` Don Koch 2014-10-22 14:03 ` Jan Beulich 1 sibling, 1 reply; 11+ messages in thread From: Andrew Cooper @ 2014-10-22 13:21 UTC (permalink / raw) To: Don Koch, Jan Beulich; +Cc: Keir Fraser, xen-devel On 22/10/14 14:19, Don Koch wrote: > On Wed, 22 Oct 2014 11:00:52 +0100 > Jan Beulich <JBeulich@suse.com> wrote: > >>>>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: >>> On Tue, 21 Oct 2014 20:00:53 +0100 >>> Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>>> On 21/10/14 19:40, Don Koch wrote: >>>>> Xen 4.3 and older transferred a maximum sized xsave area (as if all >>>>> the available XCR0 bits were set); the new version only transfers >>>>> based on the actual XCR0 bits. This may result in a smaller area if >>>>> the last sections were missing (e.g., the LWP area from an AMD >>>>> machine). If the size doesn't match the XCR0 derived size, the part of >>>>> the xsave area between the XCR0 specified and transferred size is >>>>> checked for zero data. If any part of the overflow area is non-zero, >>>>> we return with an error. >>>>> >>>>> Signed-off-by: Don Koch <dkoch@verizon.com> >>>>> --- >>>>> Changes in V4: >>>>> - Removed check of size base on xfeature_mask. >>>>> - Unsign some ints. >>>>> - Change %d to %u for unsigned ints. >>>>> - Move printk to only print if non-zero data found. >>>>> >>>>> Changes in V3: >>>>> - use h->data for zero check >>>>> - remove max size check (use size that was sent) >>>>> - fix error message (drop first byte value) >>>>> - fix "for" issues >>>>> >>>>> Changes in V2: >>>>> - Add check for size. >>>>> - Add check for non-zero data in unused part of block. >>>>> >>>>> xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ >>>>> 1 file changed, 16 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >>>>> index f0e1edc..c2780d2 100644 >>>>> --- 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; >>>>> + unsigned int i, overflow_start; >>>>> >>>>> /* Which vcpu is this? */ >>>>> vcpuid = hvm_load_instance(h); >>>>> @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain >>> *d, hvm_domain_context_t *h) >>>>> save_area) + XSTATE_AREA_MIN_SIZE); >>>>> return -EINVAL; >>>>> } >>>>> - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); >>>>> - if ( desc->length > size ) >>>>> - { >>>>> - printk(XENLOG_G_WARNING >>>>> - "HVM%d.%d restore mismatch: xsave length %u > %u\n", >>>>> - d->domain_id, vcpuid, desc->length, size); >>>>> - return -EOPNOTSUPP; >>>>> - } >>>>> h->cur += sizeof (*desc); >>>>> + overflow_start = h->cur; >>>>> >>>>> ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; >>>>> h->cur += desc->length; >>>>> @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain >>> *d, hvm_domain_context_t *h) >>>>> size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); >>>>> if ( desc->length > size ) >>>>> { >>>>> - 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. */ >>>> Please make a reference to the bug in this comment, so the reasons for >>>> the strange check is a little more obvious given a glance at the code. >>>> >>>> Perhaps >>>> >>>> /* >>>> * Xen-4.3 and older used to send longer-than-needed xsave regions. >>>> Permit loading the record if the extra data is all zero >>>> */ >>>> >>>> (suitably wrapped, given its natural indentation) >>> OK, will do. >>> >>>>> + for ( i = size; i < desc->length; i++ ) >>>>> + { >>>>> + if ( h->data[overflow_start + i] ) >>>>> + { >>>>> + 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? On consideration, having the unconditional overly-long error will be useful for debugging purposes, so best to keep it, independently of the "and non-zero data" error. ~Andrew ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 13:21 ` Andrew Cooper @ 2014-10-22 13:37 ` Don Koch 0 siblings, 0 replies; 11+ messages in thread From: Don Koch @ 2014-10-22 13:37 UTC (permalink / raw) To: Andrew Cooper; +Cc: Keir Fraser, Jan Beulich, xen-devel On Wed, 22 Oct 2014 14:21:40 +0100 Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 22/10/14 14:19, Don Koch wrote: > > On Wed, 22 Oct 2014 11:00:52 +0100 > > Jan Beulich <JBeulich@suse.com> wrote: > > > >>>>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: > >>> On Tue, 21 Oct 2014 20:00:53 +0100 > >>> Andrew Cooper <andrew.cooper3@citrix.com> 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? > > On consideration, having the unconditional overly-long error will be > useful for debugging purposes, so best to keep it, independently of the > "and non-zero data" error. OK, back to two, one before the loop and the second inside. V5 will be coming out after doing a sanity check. > ~Andrew Thanks, all. -d ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 13:19 ` Don Koch 2014-10-22 13:21 ` Andrew Cooper @ 2014-10-22 14:03 ` Jan Beulich 2014-10-22 14:11 ` Don Koch 1 sibling, 1 reply; 11+ messages in thread From: Jan Beulich @ 2014-10-22 14:03 UTC (permalink / raw) To: Don Koch; +Cc: Andrew Cooper, Keir Fraser, xen-devel >>> On 22.10.14 at 15:19, <dkoch@verizon.com> wrote: > On Wed, 22 Oct 2014 11:00:52 +0100 > Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: >> > On Tue, 21 Oct 2014 20:00:53 +0100 >> > Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> > >> >> On 21/10/14 19:40, Don Koch wrote: >> >> > Xen 4.3 and older transferred a maximum sized xsave area (as if all >> >> > the available XCR0 bits were set); the new version only transfers >> >> > based on the actual XCR0 bits. This may result in a smaller area if >> >> > the last sections were missing (e.g., the LWP area from an AMD >> >> > machine). If the size doesn't match the XCR0 derived size, the part of >> >> > the xsave area between the XCR0 specified and transferred size is >> >> > checked for zero data. If any part of the overflow area is non-zero, >> >> > we return with an error. >> >> > >> >> > Signed-off-by: Don Koch <dkoch@verizon.com> >> >> > --- >> >> > Changes in V4: >> >> > - Removed check of size base on xfeature_mask. >> >> > - Unsign some ints. >> >> > - Change %d to %u for unsigned ints. >> >> > - Move printk to only print if non-zero data found. >> >> > >> >> > Changes in V3: >> >> > - use h->data for zero check >> >> > - remove max size check (use size that was sent) >> >> > - fix error message (drop first byte value) >> >> > - fix "for" issues >> >> > >> >> > Changes in V2: >> >> > - Add check for size. >> >> > - Add check for non-zero data in unused part of block. >> >> > >> >> > xen/arch/x86/hvm/hvm.c | 28 ++++++++++++++++------------ >> >> > 1 file changed, 16 insertions(+), 12 deletions(-) >> >> > >> >> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> >> > index f0e1edc..c2780d2 100644 >> >> > --- 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; >> >> > + unsigned int i, overflow_start; >> >> > >> >> > /* Which vcpu is this? */ >> >> > vcpuid = hvm_load_instance(h); >> >> > @@ -2011,15 +2012,8 @@ static int hvm_load_cpu_xsave_states(struct domain >> > *d, hvm_domain_context_t *h) >> >> > save_area) + XSTATE_AREA_MIN_SIZE); >> >> > return -EINVAL; >> >> > } >> >> > - size = HVM_CPU_XSAVE_SIZE(xfeature_mask); >> >> > - if ( desc->length > size ) >> >> > - { >> >> > - printk(XENLOG_G_WARNING >> >> > - "HVM%d.%d restore mismatch: xsave length %u > %u\n", >> >> > - d->domain_id, vcpuid, desc->length, size); >> >> > - return -EOPNOTSUPP; >> >> > - } >> >> > h->cur += sizeof (*desc); >> >> > + overflow_start = h->cur; >> >> > >> >> > ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur]; >> >> > h->cur += desc->length; >> >> > @@ -2038,10 +2032,20 @@ static int hvm_load_cpu_xsave_states(struct domain >> > *d, hvm_domain_context_t *h) >> >> > size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum); >> >> > if ( desc->length > size ) >> >> > { >> >> > - 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. */ >> >> >> >> Please make a reference to the bug in this comment, so the reasons for >> >> the strange check is a little more obvious given a glance at the code. >> >> >> >> Perhaps >> >> >> >> /* >> >> * Xen-4.3 and older used to send longer-than-needed xsave regions. >> >> Permit loading the record if the extra data is all zero >> >> */ >> >> >> >> (suitably wrapped, given its natural indentation) >> > >> > OK, will do. >> > >> >> > + for ( i = size; i < desc->length; i++ ) >> >> > + { >> >> > + if ( h->data[overflow_start + i] ) >> >> > + { >> >> > + 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 ... Jan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 14:03 ` Jan Beulich @ 2014-10-22 14:11 ` Don Koch 2014-10-22 14:21 ` Jan Beulich 0 siblings, 1 reply; 11+ messages in thread From: Don Koch @ 2014-10-22 14:11 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel On Wed, 22 Oct 2014 15:03:23 +0100 Jan Beulich <JBeulich@suse.com> wrote: > >>> On 22.10.14 at 15:19, <dkoch@verizon.com> wrote: > > On Wed, 22 Oct 2014 11:00:52 +0100 > > Jan Beulich <JBeulich@suse.com> wrote: > > > >> >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: > >> > On Tue, 21 Oct 2014 20:00:53 +0100 > >> > Andrew Cooper <andrew.cooper3@citrix.com> 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?" > > Jan -d ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 14:11 ` Don Koch @ 2014-10-22 14:21 ` Jan Beulich 2014-10-22 14:28 ` Don Koch 0 siblings, 1 reply; 11+ messages in thread From: Jan Beulich @ 2014-10-22 14:21 UTC (permalink / raw) To: Don Koch; +Cc: Andrew Cooper, Keir Fraser, xen-devel >>> On 22.10.14 at 16:11, <dkoch@verizon.com> wrote: > On Wed, 22 Oct 2014 15:03:23 +0100 > Jan Beulich <JBeulich@suse.com> wrote: > >> >>> On 22.10.14 at 15:19, <dkoch@verizon.com> wrote: >> > On Wed, 22 Oct 2014 11:00:52 +0100 >> > Jan Beulich <JBeulich@suse.com> wrote: >> > >> >> >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: >> >> > On Tue, 21 Oct 2014 20:00:53 +0100 >> >> > Andrew Cooper <andrew.cooper3@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions 2014-10-22 14:21 ` Jan Beulich @ 2014-10-22 14:28 ` Don Koch 0 siblings, 0 replies; 11+ messages in thread From: Don Koch @ 2014-10-22 14:28 UTC (permalink / raw) To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel On Wed, 22 Oct 2014 15:21:19 +0100 Jan Beulich <JBeulich@suse.com> wrote: > >>> On 22.10.14 at 16:11, <dkoch@verizon.com> wrote: > > On Wed, 22 Oct 2014 15:03:23 +0100 > > Jan Beulich <JBeulich@suse.com> wrote: > > > >> >>> On 22.10.14 at 15:19, <dkoch@verizon.com> wrote: > >> > On Wed, 22 Oct 2014 11:00:52 +0100 > >> > Jan Beulich <JBeulich@suse.com> wrote: > >> > > >> >> >>> On 21.10.14 at 21:25, <dkoch@verizon.com> wrote: > >> >> > On Tue, 21 Oct 2014 20:00:53 +0100 > >> >> > Andrew Cooper <andrew.cooper3@citrix.com> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-10-22 14:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-21 18:40 [PATCH v4] Sanity check xsave area when migrating or restoring from older Xen verions Don Koch 2014-10-21 19:00 ` Andrew Cooper 2014-10-21 19:25 ` Don Koch 2014-10-22 10:00 ` Jan Beulich 2014-10-22 13:19 ` Don Koch 2014-10-22 13:21 ` Andrew Cooper 2014-10-22 13:37 ` Don Koch 2014-10-22 14:03 ` Jan Beulich 2014-10-22 14:11 ` Don Koch 2014-10-22 14:21 ` Jan Beulich 2014-10-22 14:28 ` Don Koch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.