[PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Boris Ostrovsky]

Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
attribute issue") makes AMD processors set SS to __KERNEL_DS in
__switch_to() to deal with cases when SS is NULL.

This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.

Since the problem that the commit is trying to address would have to be
fixed in the hypervisor (if it in fact exists under Xen) there is no
reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.

This can be easily achieved by adding x86_hyper_xen_hvm.set_cpu_features
op which will clear this flag. (And since this structure is no longer
HVM-specific we should do some renaming).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
---
 arch/x86/include/asm/hypervisor.h |  2 +-
 arch/x86/kernel/cpu/hypervisor.c  |  4 ++--
 arch/x86/xen/enlighten.c          | 27 +++++++++++++++++----------
 3 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index e42f758..055ea99 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -50,7 +50,7 @@ extern const struct hypervisor_x86 *x86_hyper;
 /* Recognized hypervisors */
 extern const struct hypervisor_x86 x86_hyper_vmware;
 extern const struct hypervisor_x86 x86_hyper_ms_hyperv;
-extern const struct hypervisor_x86 x86_hyper_xen_hvm;
+extern const struct hypervisor_x86 x86_hyper_xen;
 extern const struct hypervisor_x86 x86_hyper_kvm;
 
 extern void init_hypervisor(struct cpuinfo_x86 *c);
diff --git a/arch/x86/kernel/cpu/hypervisor.c b/arch/x86/kernel/cpu/hypervisor.c
index 36ce402..d820d8e 100644
--- a/arch/x86/kernel/cpu/hypervisor.c
+++ b/arch/x86/kernel/cpu/hypervisor.c
@@ -27,8 +27,8 @@
 
 static const __initconst struct hypervisor_x86 * const hypervisors[] =
 {
-#ifdef CONFIG_XEN_PVHVM
-	&x86_hyper_xen_hvm,
+#ifdef CONFIG_XEN
+	&x86_hyper_xen,
 #endif
 	&x86_hyper_vmware,
 	&x86_hyper_ms_hyperv,
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 94578ef..6c7bfcb 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1758,8 +1758,11 @@ static struct notifier_block xen_hvm_cpu_notifier = {
 	.notifier_call	= xen_hvm_cpu_notify,
 };
 
-static void __init xen_hvm_guest_init(void)
+static void __init xen_guest_init(void)
 {
+	if (xen_pv_domain())
+		return;
+
 	init_hvm_pv_info();
 
 	xen_hvm_init_shared_info();
@@ -1784,14 +1787,11 @@ static __init int xen_parse_nopv(char *arg)
 }
 early_param("xen_nopv", xen_parse_nopv);
 
-static uint32_t __init xen_hvm_platform(void)
+static uint32_t __init xen_platform(void)
 {
 	if (xen_nopv)
 		return 0;
 
-	if (xen_pv_domain())
-		return 0;
-
 	return xen_cpuid_base();
 }
 
@@ -1809,11 +1809,18 @@ bool xen_hvm_need_lapic(void)
 }
 EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
 
-const struct hypervisor_x86 x86_hyper_xen_hvm __refconst = {
-	.name			= "Xen HVM",
-	.detect			= xen_hvm_platform,
-	.init_platform		= xen_hvm_guest_init,
+static void xen_set_cpu_features(struct cpuinfo_x86 *c)
+{
+	if (xen_pv_domain())
+		clear_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
+}
+
+const struct hypervisor_x86 x86_hyper_xen = {
+	.name			= "Xen",
+	.detect			= xen_platform,
+	.init_platform		= xen_guest_init,
 	.x2apic_available	= xen_x2apic_para_available,
+	.set_cpu_features       = xen_set_cpu_features,
 };
-EXPORT_SYMBOL(x86_hyper_xen_hvm);
+EXPORT_SYMBOL(x86_hyper_xen);
 #endif
-- 
1.9.3

Re: [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Andy Lutomirski]

On Thu, Apr 30, 2015 at 12:08 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
> attribute issue") makes AMD processors set SS to __KERNEL_DS in
> __switch_to() to deal with cases when SS is NULL.
>
> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
>
> Since the problem that the commit is trying to address would have to be
> fixed in the hypervisor (if it in fact exists under Xen) there is no
> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
>

Seems reasonable.

Have you run the test case on a Xen PV guest on AMD?  It's possible
that Xen is affected, since the old accidental workaround that we
deleted was in the vdso and probably would have worked on Xen.

--Andy

Re: [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Boris Ostrovsky]

On 04/30/2015 03:17 PM, Andy Lutomirski wrote:
> On Thu, Apr 30, 2015 at 12:08 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
>> attribute issue") makes AMD processors set SS to __KERNEL_DS in
>> __switch_to() to deal with cases when SS is NULL.
>>
>> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
>>
>> Since the problem that the commit is trying to address would have to be
>> fixed in the hypervisor (if it in fact exists under Xen) there is no
>> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
>>
> Seems reasonable.
>
> Have you run the test case on a Xen PV guest on AMD?  It's possible
> that Xen is affected, since the old accidental workaround that we
> deleted was in the vdso and probably would have worked on Xen.

Is there a specific test that would trigger this bug? I 
booted/suspended/resumed a bunch of various guest types on both AMD and 
Intel but not much more than that. What is the commit that you deleted?

I doubt that a change in the guest could suddenly start triggering this 
issue but I will take a look at hypervisor code.


-boris

Re: [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Andy Lutomirski]

On Thu, Apr 30, 2015 at 12:30 PM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 04/30/2015 03:17 PM, Andy Lutomirski wrote:
>>
>> On Thu, Apr 30, 2015 at 12:08 PM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>>
>>> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
>>> attribute issue") makes AMD processors set SS to __KERNEL_DS in
>>> __switch_to() to deal with cases when SS is NULL.
>>>
>>> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
>>>
>>> Since the problem that the commit is trying to address would have to be
>>> fixed in the hypervisor (if it in fact exists under Xen) there is no
>>> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
>>>
>> Seems reasonable.
>>
>> Have you run the test case on a Xen PV guest on AMD?  It's possible
>> that Xen is affected, since the old accidental workaround that we
>> deleted was in the vdso and probably would have worked on Xen.
>
>
> Is there a specific test that would trigger this bug? I
> booted/suspended/resumed a bunch of various guest types on both AMD and
> Intel but not much more than that. What is the commit that you deleted?
>
> I doubt that a change in the guest could suddenly start triggering this
> issue but I will take a look at hypervisor code.

http://article.gmane.org/gmane.linux.kernel/1937899

The change was that we stopped reloading ss after 32-bit sysret in the
vdso trampoline.  Before we always reloaded ss in userspace before
doing any stack operations.

I have no idea whether this was deliberate.  It had done that (with
the attendant performance hit) since the very beginning, and there was
no comment explaining why.

--Andy

Re: [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Boris Ostrovsky]

On 04/30/2015 03:35 PM, Andy Lutomirski wrote:
> On Thu, Apr 30, 2015 at 12:30 PM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> On 04/30/2015 03:17 PM, Andy Lutomirski wrote:
>>> On Thu, Apr 30, 2015 at 12:08 PM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
>>>> attribute issue") makes AMD processors set SS to __KERNEL_DS in
>>>> __switch_to() to deal with cases when SS is NULL.
>>>>
>>>> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
>>>>
>>>> Since the problem that the commit is trying to address would have to be
>>>> fixed in the hypervisor (if it in fact exists under Xen) there is no
>>>> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
>>>>
>>> Seems reasonable.
>>>
>>> Have you run the test case on a Xen PV guest on AMD?  It's possible
>>> that Xen is affected, since the old accidental workaround that we
>>> deleted was in the vdso and probably would have worked on Xen.
>>
>> Is there a specific test that would trigger this bug? I
>> booted/suspended/resumed a bunch of various guest types on both AMD and
>> Intel but not much more than that. What is the commit that you deleted?
>>
>> I doubt that a change in the guest could suddenly start triggering this
>> issue but I will take a look at hypervisor code.
> http://article.gmane.org/gmane.linux.kernel/1937899
>
> The change was that we stopped reloading ss after 32-bit sysret in the
> vdso trampoline.  Before we always reloaded ss in userspace before
> doing any stack operations.
>
> I have no idea whether this was deliberate.  It had done that (with
> the attendant performance hit) since the very beginning, and there was
> no comment explaining why.

The test passed. Which I guess is not surprising given that interrupt 
handling is done by the hypervisor.


-boris

Re: [Xen-devel] [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[David Vrabel]

On 30/04/15 20:08, Boris Ostrovsky wrote:
> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
> attribute issue") makes AMD processors set SS to __KERNEL_DS in
> __switch_to() to deal with cases when SS is NULL.
> 
> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
> 
> Since the problem that the commit is trying to address would have to be
> fixed in the hypervisor (if it in fact exists under Xen) there is no
> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
> 
> This can be easily achieved by adding x86_hyper_xen_hvm.set_cpu_features
> op which will clear this flag. (And since this structure is no longer
> HVM-specific we should do some renaming).

Applied to for-linus-4.1b, thanks.

David

Re: [Xen-devel] [PATCH] hypervisor/x86/xen: Unset X86_BUG_SYSRET_SS_ATTRS on Xen PV guests

[Sander Eikelenboom]

Friday, May 1, 2015, 12:37:54 PM, you wrote:

> On 30/04/15 20:08, Boris Ostrovsky wrote:
>> Commit 61f01dd941ba ("x86_64, asm: Work around AMD SYSRET SS descriptor
>> attribute issue") makes AMD processors set SS to __KERNEL_DS in
>> __switch_to() to deal with cases when SS is NULL.
>> 
>> This breaks Xen PV guests who do not want to load SS with__KERNEL_DS.
>> 
>> Since the problem that the commit is trying to address would have to be
>> fixed in the hypervisor (if it in fact exists under Xen) there is no
>> reason to set X86_BUG_SYSRET_SS_ATTRS flag for PV VPCUs here.
>> 
>> This can be easily achieved by adding x86_hyper_xen_hvm.set_cpu_features
>> op which will clear this flag. (And since this structure is no longer
>> HVM-specific we should do some renaming).

> Applied to for-linus-4.1b, thanks.

> David
                    
Also works fine here.
Thanks Boris !

--
Sander