Xen devel - Sep 2012 - [PATCH/RFC] Fix xsave bug on older Xen hypervisors

When writing unsupported flags into CR4 (for some time the
xen_write_cr4 function would refuse to do anything at all)
older Xen hypervisors (and patch can potentially be improved
by finding out what older means in version numbers) would
crash the guest.

Since Amazon EC2 would at least in the past be affected by that,
Fedora and Ubuntu were carrying a hack that would filter out
X86_CR4_OSXSAVE before writing to CR4. This would affect any
PV guest, even those running on a newer HV.

And this recently caused trouble because some user-space was
only partially checking (or maybe only looking at the cpuid
bits) and then trying to use xsave even though the OS support
was not set.

So I came up with a patch that would
- limit the work-around to certain Xen versions
- prevent the write to CR4 by unsetting xsave and osxsave in
  the cpuid bits

Doing things that way may actually allow this to be acceptable
upstream, so I am sending it around, now.
It probably could be improved when knowing the exact version
to test for but otherwise should allow to work around the guest
crash while not preventing xsave on Xen 4.x and newer hosts.

-Stefan


From dff8885934d4e1274a69c4cedd28a4d18a1255e8 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Fri, 15 Jun 2012 11:54:59 +0200
Subject: [PATCH] xen: Mask xsave cpu capability on Xen host < 4

Older Xen hypervisors (like RHEL5 versions found to be used
on Amazon''s EC2) did have a bug which would crash the domain
when trying to write unsupported CR4 values.
Newer versions of the Xen hypervisor do handle this correctly.
But when a 2.6.28 or later kernel (those seem to have
xen_write_cr4 and xsave support) is booted as a PV guest on EC2,
it potentially crashes when hitting the right CPU and the wrong
hypervisor.

We were using a patch (taken from Fedora) that did always filter
the OSXSAVE off the values written to CR4 when running as Xen PV
guest. While not completely wrong this creates an inconsistency
between the cpuid bits a guest sees and the CR4 settings.
But it prevents any use of xsave even on recent Xen hypervisors.
And this did recently cause problems because user-space was not
testing all bits when deciding to use certain features.

This patch will actually mask off the cpuid bits for XSAVE and
OSXSAVE, so generic code will not even try to set CR4. It is
limited to PV guests and (since we do not actually know the
exact version) Xen hypervisors before version 4.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 arch/x86/xen/enlighten.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 9642d4a..4241055 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -210,6 +210,18 @@ void xen_vcpu_restore(void)
 	}
 }
 
+/*
+ * Older (with no clear statement about what old means) Xen hypervisors
+ * will crash a PV guest that tries to store OSXSAVE into CR4.
+ * To prevent this, we force the feature bits related to this off in the
+ * xen cpuid call. This inline function serves as a centralized test
+ * on whether the quirk should be done.
+ */
+static inline needs_xsave_quirk(unsigned version)
+{
+	return (xen_pv_domain() && ((version >> 16) < 4)) ? 1 : 0;
+}
+
 static void __init xen_banner(void)
 {
 	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
@@ -221,6 +233,8 @@ static void __init xen_banner(void)
 	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
 	       version >> 16, version & 0xffff, extra.extraversion,
 	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? "
(preserve-AD)" : "");
+	if (needs_xsave_quirk(version))
+		printk(KERN_INFO "Forcing xsave off due to Xen version.\n");
 }
 
 #define CPUID_THERM_POWER_LEAF 6
@@ -351,6 +365,7 @@ static bool __init xen_check_mwait(void)
 }
 static void __init xen_init_cpuid_mask(void)
 {
+	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
 	unsigned int ax, bx, cx, dx;
 	unsigned int xsave_mask;
 
@@ -371,7 +386,7 @@ static void __init xen_init_cpuid_mask(void)
 		(1 << (X86_FEATURE_OSXSAVE % 32));
 
 	/* Xen will set CR4.OSXSAVE if supported and not disabled by force */
-	if ((cx & xsave_mask) != xsave_mask)
+	if (((cx & xsave_mask) != xsave_mask) || needs_xsave_quirk(version))
 		cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE */
 	if (xen_check_mwait())
 		cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32));
-- 
1.7.9.5

>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
> When writing unsupported flags into CR4 (for some time the
> xen_write_cr4 function would refuse to do anything at all)
> older Xen hypervisors (and patch can potentially be improved
> by finding out what older means in version numbers) would
> crash the guest.
> 
> Since Amazon EC2 would at least in the past be affected by that,
> Fedora and Ubuntu were carrying a hack that would filter out
> X86_CR4_OSXSAVE before writing to CR4. This would affect any
> PV guest, even those running on a newer HV.
> 
> And this recently caused trouble because some user-space was
> only partially checking (or maybe only looking at the cpuid
> bits) and then trying to use xsave even though the OS support
> was not set.
> 
> So I came up with a patch that would
> - limit the work-around to certain Xen versions
> - prevent the write to CR4 by unsetting xsave and osxsave in
>   the cpuid bits
> 
> Doing things that way may actually allow this to be acceptable
> upstream, so I am sending it around, now.
> It probably could be improved when knowing the exact version
> to test for but otherwise should allow to work around the guest
> crash while not preventing xsave on Xen 4.x and newer hosts.
Before considering a hack like this, I''d really like to see evidence
of the described behavior with an upstream kernel (i.e. not one
with that known broken hack patched in, which has never been
upstream afaict).

Jan
> From dff8885934d4e1274a69c4cedd28a4d18a1255e8 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Fri, 15 Jun 2012 11:54:59 +0200
> Subject: [PATCH] xen: Mask xsave cpu capability on Xen host < 4
> 
> Older Xen hypervisors (like RHEL5 versions found to be used
> on Amazon''s EC2) did have a bug which would crash the domain
> when trying to write unsupported CR4 values.
> Newer versions of the Xen hypervisor do handle this correctly.
> But when a 2.6.28 or later kernel (those seem to have
> xen_write_cr4 and xsave support) is booted as a PV guest on EC2,
> it potentially crashes when hitting the right CPU and the wrong
> hypervisor.
> 
> We were using a patch (taken from Fedora) that did always filter
> the OSXSAVE off the values written to CR4 when running as Xen PV
> guest. While not completely wrong this creates an inconsistency
> between the cpuid bits a guest sees and the CR4 settings.
> But it prevents any use of xsave even on recent Xen hypervisors.
> And this did recently cause problems because user-space was not
> testing all bits when deciding to use certain features.
> 
> This patch will actually mask off the cpuid bits for XSAVE and
> OSXSAVE, so generic code will not even try to set CR4. It is
> limited to PV guests and (since we do not actually know the
> exact version) Xen hypervisors before version 4.
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/xen/enlighten.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 9642d4a..4241055 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -210,6 +210,18 @@ void xen_vcpu_restore(void)
>  	}
>  }
>  
> +/*
> + * Older (with no clear statement about what old means) Xen hypervisors
> + * will crash a PV guest that tries to store OSXSAVE into CR4.
> + * To prevent this, we force the feature bits related to this off in the
> + * xen cpuid call. This inline function serves as a centralized test
> + * on whether the quirk should be done.
> + */
> +static inline needs_xsave_quirk(unsigned version)
> +{
> +	return (xen_pv_domain() && ((version >> 16) < 4)) ? 1 :
0;
> +}
> +
>  static void __init xen_banner(void)
>  {
>  	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
> @@ -221,6 +233,8 @@ static void __init xen_banner(void)
>  	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
>  	       version >> 16, version & 0xffff, extra.extraversion,
>  	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? "
(preserve-AD)" :
> "");
> +	if (needs_xsave_quirk(version))
> +		printk(KERN_INFO "Forcing xsave off due to Xen version.\n");
>  }
>  
>  #define CPUID_THERM_POWER_LEAF 6
> @@ -351,6 +365,7 @@ static bool __init xen_check_mwait(void)
>  }
>  static void __init xen_init_cpuid_mask(void)
>  {
> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>  	unsigned int ax, bx, cx, dx;
>  	unsigned int xsave_mask;
>  
> @@ -371,7 +386,7 @@ static void __init xen_init_cpuid_mask(void)
>  		(1 << (X86_FEATURE_OSXSAVE % 32));
>  
>  	/* Xen will set CR4.OSXSAVE if supported and not disabled by force */
> -	if ((cx & xsave_mask) != xsave_mask)
> +	if (((cx & xsave_mask) != xsave_mask) || needs_xsave_quirk(version))
>  		cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE
*/
>  	if (xen_check_mwait())
>  		cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32));
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

On 07.09.2012 14:33, Jan Beulich wrote:
>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
>> When writing unsupported flags into CR4 (for some time the
>> xen_write_cr4 function would refuse to do anything at all)
>> older Xen hypervisors (and patch can potentially be improved
>> by finding out what older means in version numbers) would
>> crash the guest.
>>
>> Since Amazon EC2 would at least in the past be affected by that,
>> Fedora and Ubuntu were carrying a hack that would filter out
>> X86_CR4_OSXSAVE before writing to CR4. This would affect any
>> PV guest, even those running on a newer HV.
>>
>> And this recently caused trouble because some user-space was
>> only partially checking (or maybe only looking at the cpuid
>> bits) and then trying to use xsave even though the OS support
>> was not set.
>>
>> So I came up with a patch that would
>> - limit the work-around to certain Xen versions
>> - prevent the write to CR4 by unsetting xsave and osxsave in
>>   the cpuid bits
>>
>> Doing things that way may actually allow this to be acceptable
>> upstream, so I am sending it around, now.
>> It probably could be improved when knowing the exact version
>> to test for but otherwise should allow to work around the guest
>> crash while not preventing xsave on Xen 4.x and newer hosts.
> 
> Before considering a hack like this, I''d really like to see
evidence
> of the described behavior with an upstream kernel (i.e. not one
> with that known broken hack patched in, which has never been
> upstream afaict).
This is the reason I wrote that Fedora and Ubuntu were carrying it. It never has
been send upstream (the other version) because it would filter the CR4 write for
any PV guest regardless of host version.

Gathering the evidence is a bit of a problem: First you need a host which has
the xsave support, then that has to run the affected Xen version (which I do not
know) and lastly run a PV guest kernel trying to write OSXSAVE into CR4
(probably 2.6.28 onwards, we were facing those random early crashes with 2.6.32
on EC2 in some cases).

-Stefan
> 
> Jan
> 
>> From dff8885934d4e1274a69c4cedd28a4d18a1255e8 Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> Date: Fri, 15 Jun 2012 11:54:59 +0200
>> Subject: [PATCH] xen: Mask xsave cpu capability on Xen host < 4
>>
>> Older Xen hypervisors (like RHEL5 versions found to be used
>> on Amazon''s EC2) did have a bug which would crash the domain
>> when trying to write unsupported CR4 values.
>> Newer versions of the Xen hypervisor do handle this correctly.
>> But when a 2.6.28 or later kernel (those seem to have
>> xen_write_cr4 and xsave support) is booted as a PV guest on EC2,
>> it potentially crashes when hitting the right CPU and the wrong
>> hypervisor.
>>
>> We were using a patch (taken from Fedora) that did always filter
>> the OSXSAVE off the values written to CR4 when running as Xen PV
>> guest. While not completely wrong this creates an inconsistency
>> between the cpuid bits a guest sees and the CR4 settings.
>> But it prevents any use of xsave even on recent Xen hypervisors.
>> And this did recently cause problems because user-space was not
>> testing all bits when deciding to use certain features.
>>
>> This patch will actually mask off the cpuid bits for XSAVE and
>> OSXSAVE, so generic code will not even try to set CR4. It is
>> limited to PV guests and (since we do not actually know the
>> exact version) Xen hypervisors before version 4.
>>
>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>> ---
>>  arch/x86/xen/enlighten.c |   17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index 9642d4a..4241055 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -210,6 +210,18 @@ void xen_vcpu_restore(void)
>>  	}
>>  }
>>  
>> +/*
>> + * Older (with no clear statement about what old means) Xen
hypervisors
>> + * will crash a PV guest that tries to store OSXSAVE into CR4.
>> + * To prevent this, we force the feature bits related to this off in
the
>> + * xen cpuid call. This inline function serves as a centralized test
>> + * on whether the quirk should be done.
>> + */
>> +static inline needs_xsave_quirk(unsigned version)
>> +{
>> +	return (xen_pv_domain() && ((version >> 16) < 4)) ?
1 : 0;
>> +}
>> +
>>  static void __init xen_banner(void)
>>  {
>>  	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>> @@ -221,6 +233,8 @@ static void __init xen_banner(void)
>>  	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
>>  	       version >> 16, version & 0xffff, extra.extraversion,
>>  	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? "
(preserve-AD)" :
>> "");
>> +	if (needs_xsave_quirk(version))
>> +		printk(KERN_INFO "Forcing xsave off due to Xen
version.\n");
>>  }
>>  
>>  #define CPUID_THERM_POWER_LEAF 6
>> @@ -351,6 +365,7 @@ static bool __init xen_check_mwait(void)
>>  }
>>  static void __init xen_init_cpuid_mask(void)
>>  {
>> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>>  	unsigned int ax, bx, cx, dx;
>>  	unsigned int xsave_mask;
>>  
>> @@ -371,7 +386,7 @@ static void __init xen_init_cpuid_mask(void)
>>  		(1 << (X86_FEATURE_OSXSAVE % 32));
>>  
>>  	/* Xen will set CR4.OSXSAVE if supported and not disabled by force */
>> -	if ((cx & xsave_mask) != xsave_mask)
>> +	if (((cx & xsave_mask) != xsave_mask) ||
needs_xsave_quirk(version))
>>  		cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE &
OSXSAVE */
>>  	if (xen_check_mwait())
>>  		cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32));
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 



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

On Fri, Sep 07, 2012 at 01:40:43PM +0200, Stefan Bader
wrote:
> When writing unsupported flags into CR4 (for some time the
> xen_write_cr4 function would refuse to do anything at all)
> older Xen hypervisors (and patch can potentially be improved
> by finding out what older means in version numbers) would
> crash the guest.
> 
> Since Amazon EC2 would at least in the past be affected by that,
> Fedora and Ubuntu were carrying a hack that would filter out
> X86_CR4_OSXSAVE before writing to CR4. This would affect any
> PV guest, even those running on a newer HV.
> 
> And this recently caused trouble because some user-space was
> only partially checking (or maybe only looking at the cpuid
> bits) and then trying to use xsave even though the OS support
> was not set.
> 
> So I came up with a patch that would
> - limit the work-around to certain Xen versions
> - prevent the write to CR4 by unsetting xsave and osxsave in
>   the cpuid bits
> 
> Doing things that way may actually allow this to be acceptable
> upstream, so I am sending it around, now.
> It probably could be improved when knowing the exact version
> to test for but otherwise should allow to work around the guest
> crash while not preventing xsave on Xen 4.x and newer hosts.
Perhaps Matt can give us some hints here.. but otherwise this
"quirk" should fix this. It should also allow one to run a virgin
kernel on Amazon EC2 - and we can ask the distros to ditch their
existing work-arounds for this..

> 
> -Stefan
> 
> 
> >From dff8885934d4e1274a69c4cedd28a4d18a1255e8 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Fri, 15 Jun 2012 11:54:59 +0200
> Subject: [PATCH] xen: Mask xsave cpu capability on Xen host < 4
> 
> Older Xen hypervisors (like RHEL5 versions found to be used
> on Amazon''s EC2) did have a bug which would crash the domain
> when trying to write unsupported CR4 values.
> Newer versions of the Xen hypervisor do handle this correctly.
> But when a 2.6.28 or later kernel (those seem to have
> xen_write_cr4 and xsave support) is booted as a PV guest on EC2,
> it potentially crashes when hitting the right CPU and the wrong
> hypervisor.
> 
> We were using a patch (taken from Fedora) that did always filter
> the OSXSAVE off the values written to CR4 when running as Xen PV
> guest. While not completely wrong this creates an inconsistency
> between the cpuid bits a guest sees and the CR4 settings.
> But it prevents any use of xsave even on recent Xen hypervisors.
> And this did recently cause problems because user-space was not
> testing all bits when deciding to use certain features.
> 
> This patch will actually mask off the cpuid bits for XSAVE and
> OSXSAVE, so generic code will not even try to set CR4. It is
> limited to PV guests and (since we do not actually know the
> exact version) Xen hypervisors before version 4.
> 
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/xen/enlighten.c |   17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 9642d4a..4241055 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -210,6 +210,18 @@ void xen_vcpu_restore(void)
>  	}
>  }
>  
> +/*
> + * Older (with no clear statement about what old means) Xen hypervisors
> + * will crash a PV guest that tries to store OSXSAVE into CR4.
> + * To prevent this, we force the feature bits related to this off in the
> + * xen cpuid call. This inline function serves as a centralized test
> + * on whether the quirk should be done.
> + */
> +static inline needs_xsave_quirk(unsigned version)
> +{
> +	return (xen_pv_domain() && ((version >> 16) < 4)) ? 1 :
0;
> +}
> +
>  static void __init xen_banner(void)
>  {
>  	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
> @@ -221,6 +233,8 @@ static void __init xen_banner(void)
>  	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
>  	       version >> 16, version & 0xffff, extra.extraversion,
>  	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? "
(preserve-AD)" : "");
> +	if (needs_xsave_quirk(version))
> +		printk(KERN_INFO "Forcing xsave off due to Xen version.\n");
>  }
>  
>  #define CPUID_THERM_POWER_LEAF 6
> @@ -351,6 +365,7 @@ static bool __init xen_check_mwait(void)
>  }
>  static void __init xen_init_cpuid_mask(void)
>  {
> +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
>  	unsigned int ax, bx, cx, dx;
>  	unsigned int xsave_mask;
>  
> @@ -371,7 +386,7 @@ static void __init xen_init_cpuid_mask(void)
>  		(1 << (X86_FEATURE_OSXSAVE % 32));
>  
>  	/* Xen will set CR4.OSXSAVE if supported and not disabled by force */
> -	if ((cx & xsave_mask) != xsave_mask)
> +	if (((cx & xsave_mask) != xsave_mask) || needs_xsave_quirk(version))
>  		cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE
*/
>  	if (xen_check_mwait())
>  		cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32));
> -- 
> 1.7.9.5

>>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
> On 07.09.2012 14:33, Jan Beulich wrote:
>>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
>>> When writing unsupported flags into CR4 (for some time the
>>> xen_write_cr4 function would refuse to do anything at all)
>>> older Xen hypervisors (and patch can potentially be improved
>>> by finding out what older means in version numbers) would
>>> crash the guest.
>>>
>>> Since Amazon EC2 would at least in the past be affected by that,
>>> Fedora and Ubuntu were carrying a hack that would filter out
>>> X86_CR4_OSXSAVE before writing to CR4. This would affect any
>>> PV guest, even those running on a newer HV.
>>>
>>> And this recently caused trouble because some user-space was
>>> only partially checking (or maybe only looking at the cpuid
>>> bits) and then trying to use xsave even though the OS support
>>> was not set.
>>>
>>> So I came up with a patch that would
>>> - limit the work-around to certain Xen versions
>>> - prevent the write to CR4 by unsetting xsave and osxsave in
>>>   the cpuid bits
>>>
>>> Doing things that way may actually allow this to be acceptable
>>> upstream, so I am sending it around, now.
>>> It probably could be improved when knowing the exact version
>>> to test for but otherwise should allow to work around the guest
>>> crash while not preventing xsave on Xen 4.x and newer hosts.
>> 
>> Before considering a hack like this, I''d really like to see
evidence
>> of the described behavior with an upstream kernel (i.e. not one
>> with that known broken hack patched in, which has never been
>> upstream afaict).
> 
> This is the reason I wrote that Fedora and Ubuntu were carrying it. It
never
> has
> been send upstream (the other version) because it would filter the CR4
write
> for
> any PV guest regardless of host version.
But iirc that bad patch is a Linux side one (i.e. you''re trying to fix
something upstream that isn''t upstream)?

Jan

On Fri, Sep 07, 2012 at 03:02:29PM +0100, Jan Beulich
wrote:
> >>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
> > On 07.09.2012 14:33, Jan Beulich wrote:
> >>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
> >>> When writing unsupported flags into CR4 (for some time the
> >>> xen_write_cr4 function would refuse to do anything at all)
> >>> older Xen hypervisors (and patch can potentially be improved
> >>> by finding out what older means in version numbers) would
> >>> crash the guest.
> >>>
> >>> Since Amazon EC2 would at least in the past be affected by
that,
> >>> Fedora and Ubuntu were carrying a hack that would filter out
> >>> X86_CR4_OSXSAVE before writing to CR4. This would affect any
> >>> PV guest, even those running on a newer HV.
> >>>
> >>> And this recently caused trouble because some user-space was
> >>> only partially checking (or maybe only looking at the cpuid
> >>> bits) and then trying to use xsave even though the OS support
> >>> was not set.
> >>>
> >>> So I came up with a patch that would
> >>> - limit the work-around to certain Xen versions
> >>> - prevent the write to CR4 by unsetting xsave and osxsave in
> >>>   the cpuid bits
> >>>
> >>> Doing things that way may actually allow this to be acceptable
> >>> upstream, so I am sending it around, now.
> >>> It probably could be improved when knowing the exact version
> >>> to test for but otherwise should allow to work around the
guest
> >>> crash while not preventing xsave on Xen 4.x and newer hosts.
> >> 
> >> Before considering a hack like this, I''d really like to
see evidence
> >> of the described behavior with an upstream kernel (i.e. not one
> >> with that known broken hack patched in, which has never been
> >> upstream afaict).
> > 
> > This is the reason I wrote that Fedora and Ubuntu were carrying it. It
never
> > has
> > been send upstream (the other version) because it would filter the CR4
write
> > for
> > any PV guest regardless of host version.
> 
> But iirc that bad patch is a Linux side one (i.e. you''re trying to
fix
> something upstream that isn''t upstream)?
> 
Right, so the patch that this improves upon, and that Fedora and Ubuntu are
currently carrying is not upstream because:

a) It''s crap, it cripples upstream xen users, but doesn''t
impact RHEL xen
users because xsave was never supported there.

b) The hypervisor was patched to make it unnecessary quite some time ago,
and we hoped EC2 would eventually pick up that correct patch and we could
drop the crap kernel patch.

Unfortunately this has not happened. We are at a point where EC2 really is
a quirk that has to be worked around. Distros do not want to maintain
a separate EC2 build of the kernel, so the easiest way is to cripple
current upstream xen users.  This quirk is unfortunately the best possible
solution.  Having it upstream also makes it possible for any user to build
an upstream kernel that will run on EC2 without having to dig a random
patch out of a vendor kernel.

Justin

> > But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
> > something upstream that isn''t upstream)?
> > 
> Right, so the patch that this improves upon, and that Fedora and Ubuntu are
> currently carrying is not upstream because:
> 
> a) It''s crap, it cripples upstream xen users, but doesn''t
impact RHEL xen
> users because xsave was never supported there.
> 
> b) The hypervisor was patched to make it unnecessary quite some time ago,
> and we hoped EC2 would eventually pick up that correct patch and we could
> drop the crap kernel patch.
> 
> Unfortunately this has not happened. We are at a point where EC2 really is
> a quirk that has to be worked around. Distros do not want to maintain
> a separate EC2 build of the kernel, so the easiest way is to cripple
> current upstream xen users.  This quirk is unfortunately the best possible
> solution.  Having it upstream also makes it possible for any user to build
> an upstream kernel that will run on EC2 without having to dig a random
> patch out of a vendor kernel.
Sure. Jan is asking though for actual confirmation that the upstream kernel
does indeed go belly up without a workaround.
And whether this patch (which I would did since Canonical is carrying it) does
fix the issue.

I am still a newbie on the Amazon EC2 upload your kernel thing (hint, would
appreciate somebody taking this patch and trying it out).

On 07.09.2012 16:54, Konrad Rzeszutek Wilk wrote:
>>> But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
>>> something upstream that isn''t upstream)?
>>>
>> Right, so the patch that this improves upon, and that Fedora and Ubuntu
are
>> currently carrying is not upstream because:
>>
>> a) It''s crap, it cripples upstream xen users, but
doesn''t impact RHEL xen
>> users because xsave was never supported there.
>>
>> b) The hypervisor was patched to make it unnecessary quite some time
ago,
>> and we hoped EC2 would eventually pick up that correct patch and we
could
>> drop the crap kernel patch.
>>
>> Unfortunately this has not happened. We are at a point where EC2 really
is
>> a quirk that has to be worked around. Distros do not want to maintain
>> a separate EC2 build of the kernel, so the easiest way is to cripple
>> current upstream xen users.  This quirk is unfortunately the best
possible
>> solution.  Having it upstream also makes it possible for any user to
build
>> an upstream kernel that will run on EC2 without having to dig a random
>> patch out of a vendor kernel.
> 
> Sure. Jan is asking though for actual confirmation that the upstream kernel
> does indeed go belly up without a workaround.
> And whether this patch (which I would did since Canonical is carrying it)
does
> fix the issue.
It is really hard to tell. It might even be that all of the hosts on EC2 are now
upgraded. There might be different version around. Even back when the problem
was found it would not always happen.

So it is, still a bit hackish, an attempt to play safe. All that is known is
that there happened to be hosts running Xen 3.something which would do that.
Historical evidence might be:

commit 389a3c02496dd1b399bb0efd005e9fa2be24e9ee
Author: Jeremy Fitzhardinge <jeremy@goop.org>
Date:   Tue Sep 18 22:46:33 2007 -0700

    xen: don''t bother trying to set cr4

    Xen ignores all updates to cr4, and some versions will kill the domain if
    you try to change its value.  Just ignore all changes.

Of course this is ancient v2.6.23 and it was later relaxed to just filter out
some flags:

commit 2956a3511c8c5dccb1d4739ead17c7c3c23a24b7
Author: Jeremy Fitzhardinge <jeremy@goop.org>
Date:   Mon May 26 23:31:04 2008 +0100

    xen: allow some cr4 updates

    The guest can legitimately change things like cr4.OSFXSR and
    OSXMMEXCPT, so let it.

But whether there are still hosts out there running a bad version of the HV is
hard to say. EC2 may or may not be the only case (or not at all anymore). For
upstream Linux it would be a valid decision either way. Not adding a quirk
because the use-case is so small. Or adding it because it avoids special patches
in distros and forcing those bits off on Xen 3 hosts is an acceptable drawback.

At least posting it here would allow those who carry the older more intrusive
hack to replace that. So we do not run again into that mess like when xsave was
half-disabled by guests with that patch and hosts supporting it.
> 
> I am still a newbie on the Amazon EC2 upload your kernel thing (hint, would
> appreciate somebody taking this patch and trying it out).
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 



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

>>> On 07.09.12 at 16:22, "Justin M. Forbes"
<jmforbes@linuxtx.org> wrote:
> On Fri, Sep 07, 2012 at 03:02:29PM +0100, Jan Beulich wrote:
>> >>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
>> > On 07.09.2012 14:33, Jan Beulich wrote:
>> >>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
>> >>> When writing unsupported flags into CR4 (for some time the
>> >>> xen_write_cr4 function would refuse to do anything at all)
>> >>> older Xen hypervisors (and patch can potentially be
improved
>> >>> by finding out what older means in version numbers) would
>> >>> crash the guest.
>> >>>
>> >>> Since Amazon EC2 would at least in the past be affected by
that,
>> >>> Fedora and Ubuntu were carrying a hack that would filter
out
>> >>> X86_CR4_OSXSAVE before writing to CR4. This would affect
any
>> >>> PV guest, even those running on a newer HV.
>> >>>
>> >>> And this recently caused trouble because some user-space
was
>> >>> only partially checking (or maybe only looking at the
cpuid
>> >>> bits) and then trying to use xsave even though the OS
support
>> >>> was not set.
>> >>>
>> >>> So I came up with a patch that would
>> >>> - limit the work-around to certain Xen versions
>> >>> - prevent the write to CR4 by unsetting xsave and osxsave
in
>> >>>   the cpuid bits
>> >>>
>> >>> Doing things that way may actually allow this to be
acceptable
>> >>> upstream, so I am sending it around, now.
>> >>> It probably could be improved when knowing the exact
version
>> >>> to test for but otherwise should allow to work around the
guest
>> >>> crash while not preventing xsave on Xen 4.x and newer
hosts.
>> >> 
>> >> Before considering a hack like this, I''d really like
to see evidence
>> >> of the described behavior with an upstream kernel (i.e. not
one
>> >> with that known broken hack patched in, which has never been
>> >> upstream afaict).
>> > 
>> > This is the reason I wrote that Fedora and Ubuntu were carrying
it. It
> never 
>> > has
>> > been send upstream (the other version) because it would filter the
CR4
> write 
>> > for
>> > any PV guest regardless of host version.
>> 
>> But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
>> something upstream that isn''t upstream)?
>> 
> Right, so the patch that this improves upon, and that Fedora and Ubuntu are
> currently carrying is not upstream because:
> 
> a) It''s crap, it cripples upstream xen users, but doesn''t
impact RHEL xen
> users because xsave was never supported there.
> 
> b) The hypervisor was patched to make it unnecessary quite some time ago,
> and we hoped EC2 would eventually pick up that correct patch and we could
> drop the crap kernel patch.
> 
> Unfortunately this has not happened. We are at a point where EC2 really is
> a quirk that has to be worked around. Distros do not want to maintain
> a separate EC2 build of the kernel, so the easiest way is to cripple
> current upstream xen users.  This quirk is unfortunately the best possible
> solution.  Having it upstream also makes it possible for any user to build
> an upstream kernel that will run on EC2 without having to dig a random
> patch out of a vendor kernel.
All of this still doesn''t provide evidence that a plain upstream
kernel is actually having any problems in the first place. Further,
if you say EC2 has a crippled hypervisor patch - is that patch
available for looking at somewhere?

Jan

On 07.09.2012 17:44, Jan Beulich wrote:
>>>> On 07.09.12 at 16:22, "Justin M. Forbes"
<jmforbes@linuxtx.org> wrote:
>> On Fri, Sep 07, 2012 at 03:02:29PM +0100, Jan Beulich wrote:
>>>>>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
>>>> On 07.09.2012 14:33, Jan Beulich wrote:
>>>>>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
>>>>>> When writing unsupported flags into CR4 (for some time
the
>>>>>> xen_write_cr4 function would refuse to do anything at
all)
>>>>>> older Xen hypervisors (and patch can potentially be
improved
>>>>>> by finding out what older means in version numbers)
would
>>>>>> crash the guest.
>>>>>>
>>>>>> Since Amazon EC2 would at least in the past be affected
by that,
>>>>>> Fedora and Ubuntu were carrying a hack that would
filter out
>>>>>> X86_CR4_OSXSAVE before writing to CR4. This would
affect any
>>>>>> PV guest, even those running on a newer HV.
>>>>>>
>>>>>> And this recently caused trouble because some
user-space was
>>>>>> only partially checking (or maybe only looking at the
cpuid
>>>>>> bits) and then trying to use xsave even though the OS
support
>>>>>> was not set.
>>>>>>
>>>>>> So I came up with a patch that would
>>>>>> - limit the work-around to certain Xen versions
>>>>>> - prevent the write to CR4 by unsetting xsave and
osxsave in
>>>>>>   the cpuid bits
>>>>>>
>>>>>> Doing things that way may actually allow this to be
acceptable
>>>>>> upstream, so I am sending it around, now.
>>>>>> It probably could be improved when knowing the exact
version
>>>>>> to test for but otherwise should allow to work around
the guest
>>>>>> crash while not preventing xsave on Xen 4.x and newer
hosts.
>>>>>
>>>>> Before considering a hack like this, I''d really
like to see evidence
>>>>> of the described behavior with an upstream kernel (i.e. not
one
>>>>> with that known broken hack patched in, which has never
been
>>>>> upstream afaict).
>>>>
>>>> This is the reason I wrote that Fedora and Ubuntu were carrying
it. It
>> never 
>>>> has
>>>> been send upstream (the other version) because it would filter
the CR4
>> write 
>>>> for
>>>> any PV guest regardless of host version.
>>>
>>> But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
>>> something upstream that isn''t upstream)?
>>>
>> Right, so the patch that this improves upon, and that Fedora and Ubuntu
are
>> currently carrying is not upstream because:
>>
>> a) It''s crap, it cripples upstream xen users, but
doesn''t impact RHEL xen
>> users because xsave was never supported there.
>>
>> b) The hypervisor was patched to make it unnecessary quite some time
ago,
>> and we hoped EC2 would eventually pick up that correct patch and we
could
>> drop the crap kernel patch.
>>
>> Unfortunately this has not happened. We are at a point where EC2 really
is
>> a quirk that has to be worked around. Distros do not want to maintain
>> a separate EC2 build of the kernel, so the easiest way is to cripple
>> current upstream xen users.  This quirk is unfortunately the best
possible
>> solution.  Having it upstream also makes it possible for any user to
build
>> an upstream kernel that will run on EC2 without having to dig a random
>> patch out of a vendor kernel.
> 
> All of this still doesn''t provide evidence that a plain upstream
> kernel is actually having any problems in the first place. Further,
> if you say EC2 has a crippled hypervisor patch - is that patch
> available for looking at somewhere?
It was not a hypervisor patch. It was one for the guest. This was the hack:

From 57bb316c938a9ad65a8093f0584fd22eda88521f Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Tue, 27 Jul 2010 06:06:07 -0700
Subject: [PATCH] UBUNTU: SAUCE: fix pv-ops for legacy Xen

Import fix_xen_guest_on_old_EC2.patch from fedora 14

Legacy hypervisors (RHEL 5.0 and RHEL 5.1) do not handle guest writes to
cr4 gracefully. If a guest attempts to write a bit of cr4 that is
unsupported, then the HV is so offended it crashes the domain. While
later guest kernels (such as RHEL6) don''t assume the HV supports all
features, they do expect nicer responses. That assumption introduced
code that probes whether or not xsave is supported early in the boot. So
now when attempting to boot a RHEL6 guest on RHEL5.0 or RHEL5.1 an early
crash will occur.

This patch is quite obviously an undesirable hack. The real fix for this
problem should be in the HV, and is, in later HVs. However, to support
running on old HVs, RHEL6 can take this small change. No impact will
occur for running on any RHEL HV (not even RHEL 5.5 supports xsave).
There is only potential for guest performance loss on upstream Xen.

All this by way of explanation for why is this patch not going upstream.

Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com>
---
 arch/x86/xen/enlighten.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 1f92865..9043464 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -806,6 +806,7 @@ static void xen_write_cr4(unsigned long cr4)
 {
 	cr4 &= ~X86_CR4_PGE;
 	cr4 &= ~X86_CR4_PSE;
+	cr4 &= ~X86_CR4_OSXSAVE;

 	native_write_cr4(cr4);
 }
-- 
1.7.9.5
> 
> Jan
> 



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

On Fri, Sep 07, 2012 at 04:52:59PM +0100, Jan Beulich
wrote:
> >>> On 07.09.12 at 17:47, Stefan Bader
<stefan.bader@canonical.com> wrote:
> > On 07.09.2012 17:44, Jan Beulich wrote:
> >> All of this still doesn''t provide evidence that a plain
upstream
> >> kernel is actually having any problems in the first place.
Further,
> >> if you say EC2 has a crippled hypervisor patch - is that patch
> >> available for looking at somewhere?
> > 
> > It was not a hypervisor patch. It was one for the guest. This was the
hack:
> 
> So then why do you want to patch the upstream kernel? It won''t
> make that hack go away, nor will it help any existing kernels.
It will make both distro ditch that patch - and instead they can use this.
[As we can ask them to ditch their crippled patch and they can rest
safely knowing that the upstream kernel has a quirk workaround for what
they had been hitting for ages]

Also with this patch any upstream kernel that runs on Amazon EC2 will not
run in-to the issue that Fedora and Canonical ran with an virgin kernel
when they were deploying it first time. The Amazon EC2 guidelines have it
spelled out somewhere that one can''t depend on certain things  - even
if
they are detected. This was one of them, and MWAIT I believe was the other.

It won''t fix existing kernels - that is true but that is not what the
purpose of this patch is.
> 
> Jan

>>> On 07.09.12 at 17:47, Stefan Bader
<stefan.bader@canonical.com> wrote:
> On 07.09.2012 17:44, Jan Beulich wrote:
>> All of this still doesn''t provide evidence that a plain
upstream
>> kernel is actually having any problems in the first place. Further,
>> if you say EC2 has a crippled hypervisor patch - is that patch
>> available for looking at somewhere?
> 
> It was not a hypervisor patch. It was one for the guest. This was the hack:
So then why do you want to patch the upstream kernel? It won''t
make that hack go away, nor will it help any existing kernels.

Jan

On Fri, 2012-09-07 at 16:47 +0100, Stefan Bader wrote:
> On 07.09.2012 17:44, Jan Beulich wrote:
> >>>> On 07.09.12 at 16:22, "Justin M. Forbes"
<jmforbes@linuxtx.org> wrote:
> >> On Fri, Sep 07, 2012 at 03:02:29PM +0100, Jan Beulich wrote:
> >>>>>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
> >>>> On 07.09.2012 14:33, Jan Beulich wrote:
> >>>>>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
> >>>>>> When writing unsupported flags into CR4 (for some
time the
> >>>>>> xen_write_cr4 function would refuse to do anything
at all)
> >>>>>> older Xen hypervisors (and patch can potentially
be improved
> >>>>>> by finding out what older means in version
numbers) would
> >>>>>> crash the guest.
> >>>>>>
> >>>>>> Since Amazon EC2 would at least in the past be
affected by that,
> >>>>>> Fedora and Ubuntu were carrying a hack that would
filter out
> >>>>>> X86_CR4_OSXSAVE before writing to CR4. This would
affect any
> >>>>>> PV guest, even those running on a newer HV.
> >>>>>>
> >>>>>> And this recently caused trouble because some
user-space was
> >>>>>> only partially checking (or maybe only looking at
the cpuid
> >>>>>> bits) and then trying to use xsave even though the
OS support
> >>>>>> was not set.
> >>>>>>
> >>>>>> So I came up with a patch that would
> >>>>>> - limit the work-around to certain Xen versions
> >>>>>> - prevent the write to CR4 by unsetting xsave and
osxsave in
> >>>>>>   the cpuid bits
> >>>>>>
> >>>>>> Doing things that way may actually allow this to
be acceptable
> >>>>>> upstream, so I am sending it around, now.
> >>>>>> It probably could be improved when knowing the
exact version
> >>>>>> to test for but otherwise should allow to work
around the guest
> >>>>>> crash while not preventing xsave on Xen 4.x and
newer hosts.
> >>>>>
> >>>>> Before considering a hack like this, I''d
really like to see evidence
> >>>>> of the described behavior with an upstream kernel
(i.e. not one
> >>>>> with that known broken hack patched in, which has
never been
> >>>>> upstream afaict).
> >>>>
> >>>> This is the reason I wrote that Fedora and Ubuntu were
carrying it. It
> >> never 
> >>>> has
> >>>> been send upstream (the other version) because it would
filter the CR4
> >> write 
> >>>> for
> >>>> any PV guest regardless of host version.
> >>>
> >>> But iirc that bad patch is a Linux side one (i.e.
you''re trying to fix
> >>> something upstream that isn''t upstream)?
> >>>
> >> Right, so the patch that this improves upon, and that Fedora and
Ubuntu are
> >> currently carrying is not upstream because:
> >>
> >> a) It''s crap, it cripples upstream xen users, but
doesn''t impact RHEL xen
> >> users because xsave was never supported there.
> >>
> >> b) The hypervisor was patched to make it unnecessary quite some
time ago,
> >> and we hoped EC2 would eventually pick up that correct patch and
we could
> >> drop the crap kernel patch.
> >>
> >> Unfortunately this has not happened. We are at a point where EC2
really is
> >> a quirk that has to be worked around. Distros do not want to
maintain
> >> a separate EC2 build of the kernel, so the easiest way is to
cripple
> >> current upstream xen users.  This quirk is unfortunately the best
possible
> >> solution.  Having it upstream also makes it possible for any user
to build
> >> an upstream kernel that will run on EC2 without having to dig a
random
> >> patch out of a vendor kernel.
> > 
> > All of this still doesn''t provide evidence that a plain
upstream
> > kernel is actually having any problems in the first place. Further,
> > if you say EC2 has a crippled hypervisor patch - is that patch
> > available for looking at somewhere?
> 
> It was not a hypervisor patch. It was one for the guest. This was the hack:
> 
> From 57bb316c938a9ad65a8093f0584fd22eda88521f Mon Sep 17 00:00:00 2001
> From: John Johansen <john.johansen@canonical.com>
> Date: Tue, 27 Jul 2010 06:06:07 -0700
> Subject: [PATCH] UBUNTU: SAUCE: fix pv-ops for legacy Xen
> 
> Import fix_xen_guest_on_old_EC2.patch from fedora 14
> 
> Legacy hypervisors (RHEL 5.0 and RHEL 5.1) do not handle guest writes to
> cr4 gracefully. If a guest attempts to write a bit of cr4 that is
> unsupported, then the HV is so offended it crashes the domain.
For completeness, was the hypervisor fix this one:

        changeset:   19288:9ed53e602119
        user:        Keir Fraser <keir.fraser@citrix.com>
        date:        Mon Mar 09 08:54:19 2009 +0000
        files:       [...]
        description:
        x86: Mask X86_FEATURE_XSAVE in cpuid leaf 1, ecx, as we don''t
allow
        guests to use it (by setting cr4.OSXSAVE).
        
        This prevents crashes in pvops kernels, as new versions of Linux
        try to use this feature.
        
        Signed-off-by: Jeremy Fitzhardinge
<jeremy.fitzhardinge@citrix.com>
        Signed-off-by: Keir Fraser <keir.fraser@citrix.com>

??

On Fri, 2012-09-07 at 16:44 +0100, Jan Beulich wrote:
> >>> On 07.09.12 at 16:22, "Justin M. Forbes"
<jmforbes@linuxtx.org> wrote:
> > On Fri, Sep 07, 2012 at 03:02:29PM +0100, Jan Beulich wrote:
> >> >>> On 07.09.12 at 15:21, Stefan Bader
<stefan.bader@canonical.com> wrote:
> >> > On 07.09.2012 14:33, Jan Beulich wrote:
> >> >>>>> On 07.09.12 at 13:40, Stefan Bader
<stefan.bader@canonical.com> wrote:
> >> >>> When writing unsupported flags into CR4 (for some
time the
> >> >>> xen_write_cr4 function would refuse to do anything at
all)
> >> >>> older Xen hypervisors (and patch can potentially be
improved
> >> >>> by finding out what older means in version numbers)
would
> >> >>> crash the guest.
> >> >>>
> >> >>> Since Amazon EC2 would at least in the past be
affected by that,
> >> >>> Fedora and Ubuntu were carrying a hack that would
filter out
> >> >>> X86_CR4_OSXSAVE before writing to CR4. This would
affect any
> >> >>> PV guest, even those running on a newer HV.
> >> >>>
> >> >>> And this recently caused trouble because some
user-space was
> >> >>> only partially checking (or maybe only looking at the
cpuid
> >> >>> bits) and then trying to use xsave even though the OS
support
> >> >>> was not set.
> >> >>>
> >> >>> So I came up with a patch that would
> >> >>> - limit the work-around to certain Xen versions
> >> >>> - prevent the write to CR4 by unsetting xsave and
osxsave in
> >> >>>   the cpuid bits
> >> >>>
> >> >>> Doing things that way may actually allow this to be
acceptable
> >> >>> upstream, so I am sending it around, now.
> >> >>> It probably could be improved when knowing the exact
version
> >> >>> to test for but otherwise should allow to work around
the guest
> >> >>> crash while not preventing xsave on Xen 4.x and newer
hosts.
> >> >> 
> >> >> Before considering a hack like this, I''d really
like to see evidence
> >> >> of the described behavior with an upstream kernel (i.e.
not one
> >> >> with that known broken hack patched in, which has never
been
> >> >> upstream afaict).
> >> > 
> >> > This is the reason I wrote that Fedora and Ubuntu were
carrying it. It
> > never 
> >> > has
> >> > been send upstream (the other version) because it would
filter the CR4
> > write 
> >> > for
> >> > any PV guest regardless of host version.
> >> 
> >> But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
> >> something upstream that isn''t upstream)?
> >> 
> > Right, so the patch that this improves upon, and that Fedora and
Ubuntu are
> > currently carrying is not upstream because:
> > 
> > a) It''s crap, it cripples upstream xen users, but
doesn''t impact RHEL xen
> > users because xsave was never supported there.
> > 
> > b) The hypervisor was patched to make it unnecessary quite some time
ago,
> > and we hoped EC2 would eventually pick up that correct patch and we
could
> > drop the crap kernel patch.
> > 
> > Unfortunately this has not happened. We are at a point where EC2
really is
> > a quirk that has to be worked around. Distros do not want to maintain
> > a separate EC2 build of the kernel, so the easiest way is to cripple
> > current upstream xen users.  This quirk is unfortunately the best
possible
> > solution.  Having it upstream also makes it possible for any user to
build
> > an upstream kernel that will run on EC2 without having to dig a random
> > patch out of a vendor kernel.
> 
> All of this still doesn''t provide evidence that a plain upstream
> kernel is actually having any problems in the first place. Further,
> if you say EC2 has a crippled hypervisor patch - is that patch
> available for looking at somewhere?
Yes, I can verify that a plain upstream kernel has problems in the first
place, which is why we are carrying a patch to simply disable xsave all
together in the pv guest.
EC2 is not carrying a patch to cripple the hypervisor, there was an old
xen bug that makes all this fail.  The correct fix for that bug is to
patch the hypervisor, but they have not done so. Upstream xen has had
the fix for quite some time, but that doesn''t change the fact that a
lot
of xen guest usage these days is on EC2.  This is no different than
putting in a quirk to work around a firmware bug in common use.

Justin

On 07.09.2012 17:52, Jan Beulich wrote:
>>>> On 07.09.12 at 17:47, Stefan Bader
<stefan.bader@canonical.com> wrote:
>> On 07.09.2012 17:44, Jan Beulich wrote:
>>> All of this still doesn''t provide evidence that a plain
upstream
>>> kernel is actually having any problems in the first place. Further,
>>> if you say EC2 has a crippled hypervisor patch - is that patch
>>> available for looking at somewhere?
>>
>> It was not a hypervisor patch. It was one for the guest. This was the
hack:
> 
> So then why do you want to patch the upstream kernel? It won''t
> make that hack go away, nor will it help any existing kernels.
> 
> Jan
> 
It would not make it go away automatically, but whoever uses it could drop it.
It was unpatched upstream kernels that would have the problem. However, while
reading again through all the changelogs I noticed

commit 61f4237d5b005767a76f4f3694e68e6f78f392d9
Author: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Date:   Sat Sep 18 22:25:30 2010 -0700

    xen: just completely disable XSAVE

    Some (old) versions of Xen just kill the domain if it tries to set any
    unknown bits in CR4, so we can''t reliably probe for OSXSAVE in
    CR4.

    Since Xen doesn''t support XSAVE for guests at the moment, and no
such
    support is being worked on, there''s no downside in just
unconditionally
    masking XSAVE support.

    Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

So until then the write to CR4 was deliberately done to probe the feature. This
was changed by

commit 947ccf9c3c30307b774af3666ee74fcd9f47f646
Author: Shan Haitao <haitao.shan@intel.com>
Date:   Tue Nov 9 11:43:36 2010 -0800

    xen: Allow PV-OPS kernel to detect whether XSAVE is supported

    Xen fails to mask XSAVE from the cpuid feature, despite not historically
    supporting guest use of XSAVE.  However, now that XSAVE support has been
    added to Xen, we need to reliably detect its presence.

    The most reliable way to do this is to look at the OSXSAVE feature in
    cpuid which is set iff the OS (Xen, in this case), has set
    CR4.OSXSAVE.

    [ Cleaned up conditional a bit. - Jeremy ]

    Signed-off-by: Shan Haitao <haitao.shan@intel.com>
    Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

This would make it save again _if_ the HV failing to handle the writes to CR4
(which iirc the kernel code still does when the cpuid bit is set) does have at
least the patch to mask off the cpuid bits (the one Ian mentioned)

Probably a lot of hand wavy iffs... :/



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

Il 07/09/2012 16:54, Konrad Rzeszutek Wilk ha scritto:
>>> But iirc that bad patch is a Linux side one (i.e. you''re
trying to fix
>>> something upstream that isn''t upstream)?
>>>
>> Right, so the patch that this improves upon, and that Fedora and Ubuntu
are
>> currently carrying is not upstream because:
>>
>> a) It''s crap, it cripples upstream xen users, but
doesn''t impact RHEL xen
>> users because xsave was never supported there.
>>
>> b) The hypervisor was patched to make it unnecessary quite some time
ago,
>> and we hoped EC2 would eventually pick up that correct patch and we
could
>> drop the crap kernel patch.
>>
>> Unfortunately this has not happened. We are at a point where EC2 really
is
>> a quirk that has to be worked around. Distros do not want to maintain
>> a separate EC2 build of the kernel, so the easiest way is to cripple
>> current upstream xen users.  This quirk is unfortunately the best
possible
>> solution.  Having it upstream also makes it possible for any user to
build
>> an upstream kernel that will run on EC2 without having to dig a random
>> patch out of a vendor kernel.
> 
> Sure. Jan is asking though for actual confirmation that the upstream kernel
> does indeed go belly up without a workaround.
> And whether this patch (which I would did since Canonical is carrying it)
does
> fix the issue.
> 
> I am still a newbie on the Amazon EC2 upload your kernel thing (hint, would
> appreciate somebody taking this patch and trying it out).
You can just pick an old version of the CentOS kernel package, for
example 2.6.18-128.el5 or 2.6.18-164.el5 (respectively CentOS/RHEL 5.3
and 5.4).

Paolo

Il 07/09/2012 17:47, Stefan Bader ha scritto:
> 
> Legacy hypervisors (RHEL 5.0 and RHEL 5.1) do not handle guest writes to
> cr4 gracefully. If a guest attempts to write a bit of cr4 that is
> unsupported, then the HV is so offended it crashes the domain. While
> later guest kernels (such as RHEL6) don''t assume the HV supports
all
> features, they do expect nicer responses. That assumption introduced
> code that probes whether or not xsave is supported early in the boot. So
> now when attempting to boot a RHEL6 guest on RHEL5.0 or RHEL5.1 an early
> crash will occur.
> 
> This patch is quite obviously an undesirable hack. The real fix for this
> problem should be in the HV, and is, in later HVs. However, to support
> running on old HVs, RHEL6 can take this small change. No impact will
> occur for running on any RHEL HV (not even RHEL 5.5 supports xsave).
> There is only potential for guest performance loss on upstream Xen.
> 
> All this by way of explanation for why is this patch not going upstream.
If it is just 5.0 and 5.1, you can restrict the patch to Xen 3.0.

5.2 switched to Xen 3.1, which has been subsequently patched to death
without rebasing.

Paolo

Il 07/09/2012 18:13, Stefan Bader ha scritto:
> This would make it save again _if_ the HV failing to handle the writes to
CR4
> (which iirc the kernel code still does when the cpuid bit is set) does have
at
> least the patch to mask off the cpuid bits (the one Ian mentioned)
Given how old it is, that''s very unlikely.  You can download CentOS 5.0
from http://mirror.stanford.edu/yum/pub/centos/5/isos/x86_64/ or get the
source RPM for the RHEL5.0 kernel (including the Xen hypervisor) at
ftp://ftp.redhat.com/redhat/linux/enterprise/5Client/en/os/SRPMS/kernel-2.6.18-8.el5.src.rpm

Paolo

On Fri, Sep 07, 2012 at 10:54:33AM -0400, Konrad Rzeszutek Wilk
wrote:
> 
> Sure. Jan is asking though for actual confirmation that the upstream kernel
> does indeed go belly up without a workaround.
> And whether this patch (which I would did since Canonical is carrying it)
does
> fix the issue.
> 
> I am still a newbie on the Amazon EC2 upload your kernel thing (hint, would
> appreciate somebody taking this patch and trying it out).
For what it''s worth, that''s super easy these days. All modern
Linux
AMIs on EC2 should be using PV-GRUB. Just adjust the GRUB 0.97
configuration file in /boot/grub/menu.lst to point at your new kernel.

I have seen some OEL AMIs that, for some reason, have a partition
table and /boot on a separate partition. This means that the
configuration file is in /boot/boot/grub/menu.lst.

Matt

On Fri, Sep 07, 2012 at 09:47:01AM -0400, Konrad Rzeszutek Wilk
wrote:
> On Fri, Sep 07, 2012 at 01:40:43PM +0200, Stefan Bader wrote:
> > When writing unsupported flags into CR4 (for some time the
> > xen_write_cr4 function would refuse to do anything at all)
> > older Xen hypervisors (and patch can potentially be improved
> > by finding out what older means in version numbers) would
> > crash the guest.
> > 
> > Since Amazon EC2 would at least in the past be affected by that,
> > Fedora and Ubuntu were carrying a hack that would filter out
> > X86_CR4_OSXSAVE before writing to CR4. This would affect any
> > PV guest, even those running on a newer HV.
> > 
> > And this recently caused trouble because some user-space was
> > only partially checking (or maybe only looking at the cpuid
> > bits) and then trying to use xsave even though the OS support
> > was not set.
> > 
> > So I came up with a patch that would
> > - limit the work-around to certain Xen versions
> > - prevent the write to CR4 by unsetting xsave and osxsave in
> >   the cpuid bits
> > 
> > Doing things that way may actually allow this to be acceptable
> > upstream, so I am sending it around, now.
> > It probably could be improved when knowing the exact version
> > to test for but otherwise should allow to work around the guest
> > crash while not preventing xsave on Xen 4.x and newer hosts.
> 
> Perhaps Matt can give us some hints here.. but otherwise this
> "quirk" should fix this. It should also allow one to run a virgin
> kernel on Amazon EC2 - and we can ask the distros to ditch their
> existing work-arounds for this..
Xen 3.4.0 rc1 contained changeset 19288:9ed53e602119, so checking for
major version < 4 is a bit restrictive. That being said, I don''t
think
that full PV support of xsave landed until 4.0.2.

Anyway, given that Xen advertises support for XSAVE via setting
OSXSAVE, I''m not happy with a version number check.
> > >From dff8885934d4e1274a69c4cedd28a4d18a1255e8 Mon Sep 17 00:00:00
2001
> > From: Stefan Bader <stefan.bader@canonical.com>
> > Date: Fri, 15 Jun 2012 11:54:59 +0200
> > Subject: [PATCH] xen: Mask xsave cpu capability on Xen host < 4
> >
> > Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
[...]
> > +/*
> > + * Older (with no clear statement about what old means) Xen
hypervisors
> > + * will crash a PV guest that tries to store OSXSAVE into CR4.
> > + * To prevent this, we force the feature bits related to this off in
the
> > + * xen cpuid call. This inline function serves as a centralized test
> > + * on whether the quirk should be done.
> > + */
> > +static inline needs_xsave_quirk(unsigned version)
> > +{
> > +	return (xen_pv_domain() && ((version >> 16) < 4)) ?
1 : 0;
What has me really a bit confused is this: it seems that Xen should be
setting OSXSAVE for the guest if XSAVE is supported. Going back to the
changeset that Konrad point out:

commit 947ccf9c3c30307b774af3666ee74fcd9f47f646
Author: Shan Haitao <haitao.shan@intel.com>
Date:   Tue Nov 9 11:43:36 2010 -0800

    xen: Allow PV-OPS kernel to detect whether XSAVE is supported
    
    Xen fails to mask XSAVE from the cpuid feature, despite not historically
    supporting guest use of XSAVE.  However, now that XSAVE support has been
    added to Xen, we need to reliably detect its presence.
    
    The most reliable way to do this is to look at the OSXSAVE feature in
    cpuid which is set iff the OS (Xen, in this case), has set
    CR4.OSXSAVE.
[...]
+       xsave_mask +               (1 << (X86_FEATURE_XSAVE % 32)) |
+               (1 << (X86_FEATURE_OSXSAVE % 32));
+
+       /* Xen will set CR4.OSXSAVE if supported and not disabled by force */
+       if ((cx & xsave_mask) != xsave_mask)
+               cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE &
OSXSAVE */

Since this proposed change is basically in the same codepath as the
existing check to see if OSXSAVE is set, I doubt that it will prevent
xsave_init() from writing the killer bits into cr4.

Let me do a bit of testing.

Matt
> > +}
> > +
> >  static void __init xen_banner(void)
> >  {
> >  	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
> > @@ -221,6 +233,8 @@ static void __init xen_banner(void)
> >  	printk(KERN_INFO "Xen version: %d.%d%s%s\n",
> >  	       version >> 16, version & 0xffff,
extra.extraversion,
> >  	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? "
(preserve-AD)" : "");
> > +	if (needs_xsave_quirk(version))
> > +		printk(KERN_INFO "Forcing xsave off due to Xen
version.\n");
> >  }
> >  
> >  #define CPUID_THERM_POWER_LEAF 6
> > @@ -351,6 +365,7 @@ static bool __init xen_check_mwait(void)
> >  }
> >  static void __init xen_init_cpuid_mask(void)
> >  {
> > +	unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
> >  	unsigned int ax, bx, cx, dx;
> >  	unsigned int xsave_mask;
> >  
> > @@ -371,7 +386,7 @@ static void __init xen_init_cpuid_mask(void)
> >  		(1 << (X86_FEATURE_OSXSAVE % 32));
> >  
> >  	/* Xen will set CR4.OSXSAVE if supported and not disabled by force
*/
> > -	if ((cx & xsave_mask) != xsave_mask)
> > +	if (((cx & xsave_mask) != xsave_mask) ||
needs_xsave_quirk(version))
> >  		cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE &
OSXSAVE */
> >  	if (xen_check_mwait())
> >  		cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32));

On Fri, Sep 07, 2012 at 11:00:22AM -0500, Justin M. Forbes
wrote:
> On Fri, 2012-09-07 at 16:44 +0100, Jan Beulich wrote:
> > 
> > All of this still doesn''t provide evidence that a plain
upstream
> > kernel is actually having any problems in the first place. Further,
> > if you say EC2 has a crippled hypervisor patch - is that patch
> > available for looking at somewhere?
> 
> Yes, I can verify that a plain upstream kernel has problems in the first
> place, which is why we are carrying a patch to simply disable xsave all
> together in the pv guest.
> EC2 is not carrying a patch to cripple the hypervisor, there was an old
> xen bug that makes all this fail.  The correct fix for that bug is to
> patch the hypervisor, but they have not done so. Upstream xen has had
> the fix for quite some time, but that doesn''t change the fact that
a lot
> of xen guest usage these days is on EC2.  This is no different than
> putting in a quirk to work around a firmware bug in common use.
I''ve done some testing and have results that indicate otherwise. The
out-of-tree xen_write_cr4() patch is not needed as of 2.6.39. I tested
3.2.21 on a machine that has XSAVE capabilities:

[ec2-user@ip-10-160-18-80 ~]$ cpuid -1 -i | grep -i xsave/xstor
      XSAVE/XSTOR states                      = true
      OS-enabled XSAVE/XSTOR                  = false

on an older hypervisor build:

[ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/major
3
[ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/minor
0

and it boots without a problem. This patch correctly detects that the
hypervisor supports XSAVE by testing for OSXSAVE:

commit 947ccf9c3c30307b774af3666ee74fcd9f47f646
Author:     Shan Haitao <haitao.shan@intel.com>
AuthorDate: Tue Nov 9 11:43:36 2010 -0800
Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CommitDate: Wed Apr 6 08:31:13 2011 -0400

    xen: Allow PV-OPS kernel to detect whether XSAVE is supported
    
    Xen fails to mask XSAVE from the cpuid feature, despite not historically
    supporting guest use of XSAVE.  However, now that XSAVE support has been
    added to Xen, we need to reliably detect its presence.
    
    The most reliable way to do this is to look at the OSXSAVE feature in
    cpuid which is set iff the OS (Xen, in this case), has set
    CR4.OSXSAVE.

Matt

On Mon, Sep 10, 2012 at 07:40:47PM -0700, Matt Wilson
wrote:
> On Fri, Sep 07, 2012 at 11:00:22AM -0500, Justin M. Forbes wrote:
> > On Fri, 2012-09-07 at 16:44 +0100, Jan Beulich wrote:
> > > 
> > > All of this still doesn''t provide evidence that a plain
upstream
> > > kernel is actually having any problems in the first place.
Further,
> > > if you say EC2 has a crippled hypervisor patch - is that patch
> > > available for looking at somewhere?
> > 
> > Yes, I can verify that a plain upstream kernel has problems in the
first
> > place, which is why we are carrying a patch to simply disable xsave
all
> > together in the pv guest.
> > EC2 is not carrying a patch to cripple the hypervisor, there was an
old
> > xen bug that makes all this fail.  The correct fix for that bug is to
> > patch the hypervisor, but they have not done so. Upstream xen has had
> > the fix for quite some time, but that doesn''t change the fact
that a lot
> > of xen guest usage these days is on EC2.  This is no different than
> > putting in a quirk to work around a firmware bug in common use.
> 
> I''ve done some testing and have results that indicate otherwise.
The
> out-of-tree xen_write_cr4() patch is not needed as of 2.6.39. I tested
> 3.2.21 on a machine that has XSAVE capabilities:
> 
> [ec2-user@ip-10-160-18-80 ~]$ cpuid -1 -i | grep -i xsave/xstor
>       XSAVE/XSTOR states                      = true
>       OS-enabled XSAVE/XSTOR                  = false
> 
> on an older hypervisor build:
> 
> [ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/major
> 3
> [ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/minor
> 0
> 
> and it boots without a problem. This patch correctly detects that the
> hypervisor supports XSAVE by testing for OSXSAVE:
> 
> commit 947ccf9c3c30307b774af3666ee74fcd9f47f646
> Author:     Shan Haitao <haitao.shan@intel.com>
> AuthorDate: Tue Nov 9 11:43:36 2010 -0800
> Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CommitDate: Wed Apr 6 08:31:13 2011 -0400
> 
>     xen: Allow PV-OPS kernel to detect whether XSAVE is supported
>     
>     Xen fails to mask XSAVE from the cpuid feature, despite not
historically
>     supporting guest use of XSAVE.  However, now that XSAVE support has
been
>     added to Xen, we need to reliably detect its presence.
>     
>     The most reliable way to do this is to look at the OSXSAVE feature in
>     cpuid which is set iff the OS (Xen, in this case), has set
>     CR4.OSXSAVE.
> 
> Matt
Hey Matt,

Thank you for testing. CC-ing some of the Fedora folks so they are aware that
they
can ditch the: fix_xen_guest_on_old_EC2.patch

On Tue, Sep 11, 2012 at 7:37 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Sep 10, 2012 at 07:40:47PM -0700, Matt Wilson wrote:
>> > Yes, I can verify that a plain upstream kernel has problems in the
first
>> > place, which is why we are carrying a patch to simply disable
xsave all
>> > together in the pv guest.
>> > EC2 is not carrying a patch to cripple the hypervisor, there was
an old
>> > xen bug that makes all this fail.  The correct fix for that bug is
to
>> > patch the hypervisor, but they have not done so. Upstream xen has
had
>> > the fix for quite some time, but that doesn''t change the
fact that a lot
>> > of xen guest usage these days is on EC2.  This is no different
than
>> > putting in a quirk to work around a firmware bug in common use.
>>
>> I''ve done some testing and have results that indicate
otherwise. The
>> out-of-tree xen_write_cr4() patch is not needed as of 2.6.39. I tested
>> 3.2.21 on a machine that has XSAVE capabilities:
>>
>> [ec2-user@ip-10-160-18-80 ~]$ cpuid -1 -i | grep -i xsave/xstor
>>       XSAVE/XSTOR states                      = true
>>       OS-enabled XSAVE/XSTOR                  = false
>>
>> on an older hypervisor build:
>>
>> [ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/major
>> 3
>> [ec2-user@ip-10-160-18-80 ~]$ cat /sys/hypervisor/version/minor
>> 0
>>
>> and it boots without a problem. This patch correctly detects that the
>> hypervisor supports XSAVE by testing for OSXSAVE:
>>
>> commit 947ccf9c3c30307b774af3666ee74fcd9f47f646
>> Author:     Shan Haitao <haitao.shan@intel.com>
>> AuthorDate: Tue Nov 9 11:43:36 2010 -0800
>> Commit:     Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CommitDate: Wed Apr 6 08:31:13 2011 -0400
>>
>>     xen: Allow PV-OPS kernel to detect whether XSAVE is supported
>>
>>     Xen fails to mask XSAVE from the cpuid feature, despite not
historically
>>     supporting guest use of XSAVE.  However, now that XSAVE support has
been
>>     added to Xen, we need to reliably detect its presence.
>>
>>     The most reliable way to do this is to look at the OSXSAVE feature
in
>>     cpuid which is set iff the OS (Xen, in this case), has set
>>     CR4.OSXSAVE.
>>
>> Matt
>
> Hey Matt,
>
> Thank you for testing. CC-ing some of the Fedora folks so they are aware
that they
> can ditch the: fix_xen_guest_on_old_EC2.patch
Ditched.  Thanks Matt and Konrad.

(BTW, kernel-team@fedoraproject.org is the easiest way to get things to
the fedora kernel team.)

josh