Patchwork [PATCH/RFC] Fix xsave bug on older Xen hypervisors

login
register
mail settings
Submitter Stefan Bader
Date Sept. 7, 2012, 11:40 a.m.
Message ID <1347018043-21252-1-git-send-email-stefan.bader@canonical.com>
Download mbox | patch
Permalink /patch/1421751/
State New, archived
Headers show

Comments

Stefan Bader - Sept. 7, 2012, 11:40 a.m.
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(-)
Jan Beulich - Sept. 7, 2012, 12:33 p.m.
>>> 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 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Stefan Bader - Sept. 7, 2012, 1:21 p.m.
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
>
Konrad Rzeszutek Wilk - Sept. 7, 2012, 1:47 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jan Beulich - Sept. 7, 2012, 2:02 p.m.
>>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Justin M. Forbes - Sept. 7, 2012, 2:22 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Konrad Rzeszutek Wilk - Sept. 7, 2012, 2:54 p.m.
> > 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).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jan Beulich - Sept. 7, 2012, 3:44 p.m.
>>> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Justin M. Forbes - Sept. 7, 2012, 4 p.m.
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Paolo Bonzini - Sept. 8, 2012, 10:18 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Wilson - Sept. 10, 2012, 10:36 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Wilson - Sept. 11, 2012, 1:17 a.m.
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));
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Matt Wilson - Sept. 11, 2012, 2:40 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Konrad Rzeszutek Wilk - Sept. 11, 2012, 11:37 a.m.
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 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Josh Boyer - Sept. 11, 2012, 1:06 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

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));