[PATCH] Ignore non-zero data in unused xsave area.

[PATCH] Ignore non-zero data in unused xsave area.

[Don Koch]

If we restore an xsave area from an older xen that has a larger
size than the xcr0 bit call for, it is possible to have non-zero
data in the unused area if an xsave has ever been done that used
that area (e.g. during a context switch). Since the vcpu's xsave
area is never zeroed after the initial allocation, that data is
still there. Since we are told that said area was not written to
during the save or migration, there is no need to restore it.

Signed-off-by: Don Koch <dkoch@verizon.com>
---
Turns out the assertion that the unused xsave area is zero
is wrong. Unfortunately, that leaves the following as the
only way I can think of to work around it (and is no worse
than xsave/xrestore during context switches). Alternate
suggestions welcome.

 xen/arch/x86/hvm/hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8f49b44..b2c0bc4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2044,7 +2044,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
                 printk(XENLOG_G_WARNING
                        "HVM%d.%u restore mismatch: xsave length %#x > %#x (non-zero data at %#x)\n",
                        d->domain_id, vcpuid, desc->length, size, i);
-                return -EOPNOTSUPP;
+                break;
             }
         }
         printk(XENLOG_G_WARNING
-- 
1.8.3.1

Re: [for-xen-4.5 PATCH] Ignore non-zero data in unused xsave area.

[Konrad Rzeszutek Wilk]

On Tue, Nov 18, 2014 at 10:26:31AM -0500, Don Koch wrote:
> If we restore an xsave area from an older xen that has a larger
> size than the xcr0 bit call for, it is possible to have non-zero
> data in the unused area if an xsave has ever been done that used
> that area (e.g. during a context switch). Since the vcpu's xsave
> area is never zeroed after the initial allocation, that data is
> still there. Since we are told that said area was not written to
> during the save or migration, there is no need to restore it.
> 
> Signed-off-by: Don Koch <dkoch@verizon.com>
> ---
> Turns out the assertion that the unused xsave area is zero
> is wrong. Unfortunately, that leaves the following as the
> only way I can think of to work around it (and is no worse
> than xsave/xrestore during context switches). Alternate
> suggestions welcome.

This is Xen 4.5 material I presume.

> 
>  xen/arch/x86/hvm/hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8f49b44..b2c0bc4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2044,7 +2044,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>                  printk(XENLOG_G_WARNING
>                         "HVM%d.%u restore mismatch: xsave length %#x > %#x (non-zero data at %#x)\n",
>                         d->domain_id, vcpuid, desc->length, size, i);
> -                return -EOPNOTSUPP;
> +                break;
>              }
>          }
>          printk(XENLOG_G_WARNING
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [for-xen-4.5 PATCH] Ignore non-zero data in unused xsave area.

[Andrew Cooper]

On 18/11/14 16:32, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 18, 2014 at 10:26:31AM -0500, Don Koch wrote:
>> If we restore an xsave area from an older xen that has a larger
>> size than the xcr0 bit call for, it is possible to have non-zero
>> data in the unused area if an xsave has ever been done that used

Do you mean "has never been done".

i.e. the non-zero data is actually Xen heap junk?

~Andrew

Re: [PATCH] Ignore non-zero data in unused xsave area.

[Jan Beulich]

>>> On 18.11.14 at 16:26, <dkoch@verizon.com> wrote:
> If we restore an xsave area from an older xen that has a larger
> size than the xcr0 bit call for, it is possible to have non-zero
> data in the unused area if an xsave has ever been done that used
> that area (e.g. during a context switch). Since the vcpu's xsave
> area is never zeroed after the initial allocation, that data is
> still there.

This needs more explanation: xcr0_accum is named this way
because bits never disappear from it. Hence afaics any piece of
the xsave area ever having got written with a non-zero value
would be part of the data actually used on migration.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2044,7 +2044,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>                  printk(XENLOG_G_WARNING
>                         "HVM%d.%u restore mismatch: xsave length %#x > %#x (non-zero data at %#x)\n",
>                         d->domain_id, vcpuid, desc->length, size, i);
> -                return -EOPNOTSUPP;
> +                break;
>              }
>          }
>          printk(XENLOG_G_WARNING

Even if we really have to go this route, you should recall the
discussion on the earlier patch of yours: The end result should
not be that two messages get logged for a single event.

Jan

Re: [PATCH] Ignore non-zero data in unused xsave area.

[Don Koch]

On Tue, 18 Nov 2014 16:35:42 +0000
Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 18.11.14 at 16:26, <dkoch@verizon.com> wrote:
> > If we restore an xsave area from an older xen that has a larger
> > size than the xcr0 bit call for, it is possible to have non-zero
> > data in the unused area if an xsave has ever been done that used
> > that area (e.g. during a context switch). Since the vcpu's xsave
> > area is never zeroed after the initial allocation, that data is
> > still there.
> 
> This needs more explanation: xcr0_accum is named this way
> because bits never disappear from it. Hence afaics any piece of
> the xsave area ever having got written with a non-zero value
> would be part of the data actually used on migration.

Let me investigate this further. Since the initial xsave area is cleared
when allocated, I see no other way to get anything in there.

> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2044,7 +2044,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
> >                  printk(XENLOG_G_WARNING
> >                         "HVM%d.%u restore mismatch: xsave length %#x > %#x (non-zero data at %#x)\n",
> >                         d->domain_id, vcpuid, desc->length, size, i);
> > -                return -EOPNOTSUPP;
> > +                break;
> >              }
> >          }
> >          printk(XENLOG_G_WARNING
> 
> Even if we really have to go this route, you should recall the
> discussion on the earlier patch of yours: The end result should
> not be that two messages get logged for a single event.

Ah, yes. Agreed. Will fix if my investigation reveals what occurred.

> Jan
> 

Konrad: Yes, this will be 4.5 fodder, pending the aforementioned
investigation.

Thanks,
-d

Re: [PATCH] Ignore non-zero data in unused xsave area.

[Don Koch]

This change can be ignored. Details below.

On Tue, 18 Nov 2014 10:26:31 -0500
Don Koch <dkoch@verizon.com> wrote:

> If we restore an xsave area from an older xen that has a larger
> size than the xcr0 bit call for, it is possible to have non-zero
> data in the unused area if an xsave has ever been done that used
> that area (e.g. during a context switch). Since the vcpu's xsave
> area is never zeroed after the initial allocation, that data is
> still there. Since we are told that said area was not written to
> during the save or migration, there is no need to restore it.

Found the issue. Don Slutz had reported this occurring a couple of times
and I witnessed the bad data in the unused area. Turns out he had
done a migration from 4.3 to 4.4, then another migration from 4.4 to
4.3 and when he tried another 4.3 to 4.4 migration, it failed.

Since 4.4 to 4.3 migration isn't supported, we can punt this fix.
Well, unless we want to support this (I assume not) in which case
it still needs a minor tweak.

Sorry for the noise.

-d

> Signed-off-by: Don Koch <dkoch@verizon.com>
> ---
> Turns out the assertion that the unused xsave area is zero
> is wrong. Unfortunately, that leaves the following as the
> only way I can think of to work around it (and is no worse
> than xsave/xrestore during context switches). Alternate
> suggestions welcome.
> 
>  xen/arch/x86/hvm/hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 8f49b44..b2c0bc4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2044,7 +2044,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>                  printk(XENLOG_G_WARNING
>                         "HVM%d.%u restore mismatch: xsave length %#x > %#x (non-zero data at %#x)\n",
>                         d->domain_id, vcpuid, desc->length, size, i);
> -                return -EOPNOTSUPP;
> +                break;
>              }
>          }
>          printk(XENLOG_G_WARNING
> -- 
> 1.8.3.1
> 

Re: [PATCH] Ignore non-zero data in unused xsave area.

[Jan Beulich]

>>> On 20.11.14 at 15:35, <dkoch@verizon.com> wrote:
> On Tue, 18 Nov 2014 10:26:31 -0500
> Don Koch <dkoch@verizon.com> wrote:
> 
>> If we restore an xsave area from an older xen that has a larger
>> size than the xcr0 bit call for, it is possible to have non-zero
>> data in the unused area if an xsave has ever been done that used
>> that area (e.g. during a context switch). Since the vcpu's xsave
>> area is never zeroed after the initial allocation, that data is
>> still there. Since we are told that said area was not written to
>> during the save or migration, there is no need to restore it.
> 
> Found the issue. Don Slutz had reported this occurring a couple of times
> and I witnessed the bad data in the unused area. Turns out he had
> done a migration from 4.3 to 4.4, then another migration from 4.4 to
> 4.3 and when he tried another 4.3 to 4.4 migration, it failed.

With the important part being that 4.3.0 (other than 4.3.3)
copied as much data as the receiving system's XSAVE
capabilities would require, i.e. potentially stuff that wasn't part
of the CPU_XSAVE save record at all (if the sending side either
had less capabilities or used an already fixed hypervisor).

Jan