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

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

[Don Koch]

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 size is
checked against the maximum size and the part of the xsave area
between the actual and maximum used size is checked for zero data. If
either the max size check or any part of the overflow area is
non-zero, we return with an error.

Signed-off-by: Don Koch <dkoch@verizon.com>
---
 xen/arch/x86/hvm/hvm.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f0e1edc..bdebc67 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1971,6 +1971,8 @@ 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;
+    u32 eax, ebx, ecx, edx;
+    uint8_t *overflow_start;
 
     /* Which vcpu is this? */
     vcpuid = hvm_load_instance(h);
@@ -2041,8 +2043,32 @@ 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;
+
+        /* 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);
+        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. */
+        overflow_start = (uint8_t *)&ctxt->save_area -
+                           offsetof(struct hvm_hw_cpu_xsave, save_area);
+        for (int i = size; i < desc->length; i++)
+        {
+            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));
+                return -EOPNOTSUPP;
+            }
+        }
     }
     /* Checking finished */
 
-- 
1.8.3.1

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

[Andrew Cooper]

On 17/10/2014 18:11, 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 size is
> checked against the maximum size and the part of the xsave area
> between the actual and maximum used size is checked for zero data. If
> either the max size check or any part of the overflow area is
> non-zero, we return with an error.
>
> Signed-off-by: Don Koch <dkoch@verizon.com>
> ---
>   xen/arch/x86/hvm/hvm.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index f0e1edc..bdebc67 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1971,6 +1971,8 @@ 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;
> +    u32 eax, ebx, ecx, edx;
> +    uint8_t *overflow_start;
>   
>       /* Which vcpu is this? */
>       vcpuid = hvm_load_instance(h);
> @@ -2041,8 +2043,32 @@ 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);

This warning should be elided if we pass the zero-check.

> -        return -EOPNOTSUPP;
> +
> +        /* 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...

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

> +        overflow_start = (uint8_t *)&ctxt->save_area -
> +                           offsetof(struct hvm_hw_cpu_xsave, save_area);

I am having a hard time working out whether this expression is correct.  
As ctxt is superimposed on top of h->data, would it not make sense to 
check h->data between the expected end, and the real length?  h->data is 
even the correct type for doing this with.

> +        for (int i = size; i < desc->length; i++)

Style

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

~Andrew

> +                return -EOPNOTSUPP;
> +            }
> +        }
>       }
>       /* Checking finished */
>   

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

[Jan Beulich]

>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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

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

[Don Koch]

On Mon, 20 Oct 2014 11:21:49 +0100
Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.

It was (sort of) checked in an earlier test:
    size = HVM_CPU_XSAVE_SIZE(xfeature_mask);
    if ( desc->length > size )

I suppose we could skip the check as the only other possibility is that
the block sent is larger than what the current architecture can handle
and, if it's all zero, it won't matter (at least in the xsave area).

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

Agreed on both. Will change to something like "has non-zero data at <i>".

> Jan
> 

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

[Jan Beulich]

>>> On 20.10.14 at 14:54, <dkoch@verizon.com> wrote:
> On Mon, 20 Oct 2014 11:21:49 +0100
> Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.
> 
> It was (sort of) checked in an earlier test:
>     size = HVM_CPU_XSAVE_SIZE(xfeature_mask);
>     if ( desc->length > size )

No, that's not checking incoming state's consistency, but whether
it fits in what the receiving side can handle. An eventual
consistency check would need to use the incoming record's
xfeature_mask field.

> I suppose we could skip the check as the only other possibility is that
> the block sent is larger than what the current architecture can handle
> and, if it's all zero, it won't matter (at least in the xsave area).

With the model change that you're proposing we actually _need_ to
have a way to also bypass this check, as now what is arriving may
be legitimately larger than what our own possible biggest save area
might be.

Jan

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

[Andrew Cooper]

On 20/10/14 11:21, Jan Beulich wrote:
>>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.

I am not in favour of reinstating that check.

Whether the state was valid for the sending side, is not something the
receiving side should care about.

All the receiving side should care about is whether the state received
is valid.  In this case, reinstating the check still doesn't allow us to
correctly calculate the size, and manually doing so is fragile and very
prone to error.

If the record is overly long, but the trailing space is all zeroes, the
state is valid whether or not it is the correct length for the sending side.

~Andrew

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

[Jan Beulich]

>>> On 20.10.14 at 15:27, <andrew.cooper3@citrix.com> wrote:
> On 20/10/14 11:21, Jan Beulich wrote:
>>>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.
> 
> I am not in favour of reinstating that check.
> 
> Whether the state was valid for the sending side, is not something the
> receiving side should care about.

I can see your point, and mostly agree. Nevertheless, at least for the
record, two related comments further down:

> All the receiving side should care about is whether the state received
> is valid.  In this case, reinstating the check still doesn't allow us to
> correctly calculate the size, and manually doing so is fragile and very
> prone to error.

I don't think there's much room for errors here - all the offsets and
sizes are well defined, and hence just require being put in e.g. a
static table.

> If the record is overly long, but the trailing space is all zeroes, the
> state is valid whether or not it is the correct length for the sending side.

The problem is - this is true only as long as the default values for
that state are zero. Considering that the base state already
violates this, I don't see there being a guarantee for this to be true
for all future extensions.

Jan

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

[Don Koch]

On Mon, 20 Oct 2014 15:21:19 +0100
Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 20.10.14 at 15:27, <andrew.cooper3@citrix.com> wrote:
> > On 20/10/14 11:21, Jan Beulich wrote:
> >>>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.
> > 
> > I am not in favour of reinstating that check.
> > 
> > Whether the state was valid for the sending side, is not something the
> > receiving side should care about.
> 
> I can see your point, and mostly agree. Nevertheless, at least for the
> record, two related comments further down:
> 
> > All the receiving side should care about is whether the state received
> > is valid.  In this case, reinstating the check still doesn't allow us to
> > correctly calculate the size, and manually doing so is fragile and very
> > prone to error.
> 
> I don't think there's much room for errors here - all the offsets and
> sizes are well defined, and hence just require being put in e.g. a
> static table.
> 
> > If the record is overly long, but the trailing space is all zeroes, the
> > state is valid whether or not it is the correct length for the sending side.
> 
> The problem is - this is true only as long as the default values for
> that state are zero. Considering that the base state already
> violates this, I don't see there being a guarantee for this to be true
> for all future extensions.

This shouldn't be a problem. The original block was zalloced, ergo zeroed,
and it wouldn't have been touched by the xsave if the bit(s) for said
area(s) weren't set. We also don't copy these zeros back in, we're just
sanity checking them.

If we believe the sender, and I don't see any reason not to, then the
size it sent is the size it really wanted. That would mean that the
size check shouldn't be needed, either.

> Jan

-d

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

[Andrew Cooper]

On 20/10/14 15:21, Jan Beulich wrote:
>>>> On 20.10.14 at 15:27, <andrew.cooper3@citrix.com> wrote:
>> On 20/10/14 11:21, Jan Beulich wrote:
>>>>>> On 18.10.14 at 01:36, <andrew.cooper3@citrix.com> 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.
>> I am not in favour of reinstating that check.
>>
>> Whether the state was valid for the sending side, is not something the
>> receiving side should care about.
> I can see your point, and mostly agree. Nevertheless, at least for the
> record, two related comments further down:
>
>> All the receiving side should care about is whether the state received
>> is valid.  In this case, reinstating the check still doesn't allow us to
>> correctly calculate the size, and manually doing so is fragile and very
>> prone to error.
> I don't think there's much room for errors here - all the offsets and
> sizes are well defined, and hence just require being put in e.g. a
> static table.

They are different between Intel and AMD.  For better or for worse, we
do support cross-vendor migration (for a sufficiently feature-stripped
VM), and have no way of distinguishing sending-vendor given the
migration stream.

>
>> If the record is overly long, but the trailing space is all zeroes, the
>> state is valid whether or not it is the correct length for the sending side.
> The problem is - this is true only as long as the default values for
> that state are zero. Considering that the base state already
> violates this, I don't see there being a guarantee for this to be true
> for all future extensions.

Hmm.  This is indeed a concern I had overlooked.

Furthermore, there is now a compact XSAVE area defined by Intel. 
Support for this is defined in the XSAVE header, not xcr0, so
calculating the expected size based on xcr0 is going to get more
complicated in the future, if we decide to make use of this feature.

In the case that we receive an overly long record, we are still
calculating length based on xcr0_accum.  This means that even if the
extra xsave area defaults are non0, the guest has not seen them yet.  In
this case, the defaults from the current hardware are just as good as
the defaults from the old hardware, in the case that they differ.

All of this is quite a mess.

~Andrew

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

[Don Koch]

On Sat, 18 Oct 2014 00:36:28 +0100
Andrew Cooper <andrew.cooper3@citrix.com> wrote:

> On 17/10/2014 18:11, 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 size is
> > checked against the maximum size and the part of the xsave area
> > between the actual and maximum used size is checked for zero data. If
> > either the max size check or any part of the overflow area is
> > non-zero, we return with an error.
> >
> > Signed-off-by: Don Koch <dkoch@verizon.com>
> > ---
> >   xen/arch/x86/hvm/hvm.c | 29 ++++++++++++++++++++++++++++-
> >   1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index f0e1edc..bdebc67 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1971,6 +1971,8 @@ 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;
> > +    u32 eax, ebx, ecx, edx;
> > +    uint8_t *overflow_start;
> >   
> >       /* Which vcpu is this? */
> >       vcpuid = hvm_load_instance(h);
> > @@ -2041,8 +2043,32 @@ 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);
> 
> This warning should be elided if we pass the zero-check.

OK. I'll combine it with the zero check message.

> > -        return -EOPNOTSUPP;
> > +
> > +        /* 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...

I didn't think migration or save/restore was supported between heterogeneous
architectures. I'll drop this check.

> > +        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.
> 
> > +        overflow_start = (uint8_t *)&ctxt->save_area -
> > +                           offsetof(struct hvm_hw_cpu_xsave, save_area);
> 
> I am having a hard time working out whether this expression is correct.  
> As ctxt is superimposed on top of h->data, would it not make sense to 
> check h->data between the expected end, and the real length?  h->data is 
> even the correct type for doing this with.

That's OK. I had a hard time working it out in the first place. I'll try
a spin using h->data. I'll have to cache the correct offset as h->cur has
been mucked with by this time.

> > +        for (int i = size; i < desc->length; i++)
> 
> Style

Is not really defined for "for". I checked the CODING_STYLE file and it only
defines style for "if" and "while". I found more examples of "for" statements
with no spaces inside the parens; so, I went with that. Will fix.

Do you want me to update the CODING_STYLES file, too? (In a separate patch,
of course.)

> > +        {
> > +            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".
> 
> ~Andrew
> 
> > +                return -EOPNOTSUPP;
> > +            }
> > +        }
> >       }
> >       /* Checking finished */
> >   
> 

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

[Jan Beulich]

>>> On 20.10.14 at 14:54, <dkoch@verizon.com> wrote:
> On Sat, 18 Oct 2014 00:36:28 +0100
> Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 17/10/2014 18:11, Don Koch wrote:
>> > +        for (int i = size; i < desc->length; i++)
>> 
>> Style
> 
> Is not really defined for "for". I checked the CODING_STYLE file and it only
> defines style for "if" and "while".

Did you skip the "such as" in the section dealing with them?

> I found more examples of "for" statements
> with no spaces inside the parens; so, I went with that. Will fix.

It's not just about the missing spaces. You also shouldn't declare
variables inside the for() statement.

> Do you want me to update the CODING_STYLES file, too? (In a separate patch,
> of course.)

As hinted at -  the document is sufficient as is afaict.

Jan

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

[Don Koch]

On Mon, 20 Oct 2014 16:28:47 +0100
Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 20.10.14 at 14:54, <dkoch@verizon.com> wrote:
> > On Sat, 18 Oct 2014 00:36:28 +0100
> > Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> On 17/10/2014 18:11, Don Koch wrote:
> >> > +        for (int i = size; i < desc->length; i++)
> >> 
> >> Style
> > 
> > Is not really defined for "for". I checked the CODING_STYLE file and it only
> > defines style for "if" and "while".
> 
> Did you skip the "such as" in the section dealing with them?

Still not explicitly mentioned. I would have assumed they needed
spaces if it weren't for all the counterexamples.

> > I found more examples of "for" statements
> > with no spaces inside the parens; so, I went with that. Will fix.
> 
> It's not just about the missing spaces. You also shouldn't declare
> variables inside the for() statement.

This, too, wasn't mentioned at all in CODING_STYLES, but, of course, will
fix in my code (I had wondered about that, too).

> > Do you want me to update the CODING_STYLES file, too? (In a separate patch,
> > of course.)
> 
> As hinted at -  the document is sufficient as is afaict.
> 
> Jan

As hinted at - the document is lacking; too much is impliedn (at least, IMO).

-d

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

[Jan Beulich]

>>> On 20.10.14 at 17:40, <dkoch@verizon.com> wrote:
> On Mon, 20 Oct 2014 16:28:47 +0100
> Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 20.10.14 at 14:54, <dkoch@verizon.com> wrote:
>> > On Sat, 18 Oct 2014 00:36:28 +0100
>> > Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> >> On 17/10/2014 18:11, Don Koch wrote:
>> >> > +        for (int i = size; i < desc->length; i++)
>> >> 
>> >> Style
>> > 
>> > Is not really defined for "for". I checked the CODING_STYLE file and it 
> only
>> > defines style for "if" and "while".
>> 
>> Did you skip the "such as" in the section dealing with them?
> 
> Still not explicitly mentioned.

Why are you insisting on everything being explicitly mentioned. I
agree there are areas in the document that could do with
improvement, but adding "for" alongside "if" and "while" isn't one
of them imo.

> I would have assumed they needed
> spaces if it weren't for all the counterexamples.

Yes, there are counterexamples, but largely due to the mixture of
Xen and Linux style files (and sometimes within a file). This is
unfortunate, but there are sufficiently many good examples in
case you can't derive the general rules from the examples given
in ./CODING_STYLE.

>> > I found more examples of "for" statements
>> > with no spaces inside the parens; so, I went with that. Will fix.
>> 
>> It's not just about the missing spaces. You also shouldn't declare
>> variables inside the for() statement.
> 
> This, too, wasn't mentioned at all in CODING_STYLES, but, of course, will
> fix in my code (I had wondered about that, too).

But I don't think you'll find many bad examples throughout xen/ for
this one.

>> > Do you want me to update the CODING_STYLES file, too? (In a separate patch,
>> > of course.)
>> 
>> As hinted at -  the document is sufficient as is afaict.
> 
> As hinted at - the document is lacking; too much is impliedn (at least, IMO).

As said above - useful improvements to the document are welcome,
but adding stuff that is intentional to be implied doesn't fall in this
category afaic. The document should remain reasonably short, or
else we risk people not reading enough of it.

Jan