Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625064 > unrolled thread

[PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave Juergen Gross <jgross@suse.com> - 2017-04-18 08:40 +0200
    Re: [Xen-devel] [PATCH v3 09/11] x86/xen: use capabilities instead of  fake cpuid values for xsave Andrew Cooper <andrew.cooper3@citrix.com> - 2017-04-18 12:10 +0200
      Re: [Xen-devel] [PATCH v3 09/11] x86/xen: use capabilities instead of  fake cpuid values for xsave Juergen Gross <jgross@suse.com> - 2017-04-18 14:00 +0200
    Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid  values for xsave Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-04-21 16:30 +0200
      Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid  values for xsave Juergen Gross <jgross@suse.com> - 2017-04-21 16:40 +0200
        Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid  values for xsave Andrew Cooper <andrew.cooper3@citrix.com> - 2017-04-21 20:30 +0200
          Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid  values for xsave Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-04-21 21:30 +0200
            Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid  values for xsave Juergen Gross <jgross@suse.com> - 2017-04-24 08:30 +0200

#1625064 — [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

When running as pv domain xen_cpuid() is being used instead of
native_cpuid(). In xen_cpuid() the xsave feature availability is
indicated by special casing the related cpuid leaf.

Instead of delivering fake cpuid values set or clear the cpu
capability bits for xsave instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_pv.c | 46 +++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 38dec28a8e6d..e6bf71d76e10 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -165,8 +165,6 @@ xen_running_on_version_or_later(unsigned int major, unsigned int minor)
 	return false;
 }
 
-static __read_mostly unsigned int cpuid_leaf1_ecx_mask = ~0;
-
 static __read_mostly unsigned int cpuid_leaf5_ecx_val;
 static __read_mostly unsigned int cpuid_leaf5_edx_val;
 
@@ -174,16 +172,12 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		      unsigned int *cx, unsigned int *dx)
 {
 	unsigned maskebx = ~0;
-	unsigned maskecx = ~0;
+
 	/*
 	 * Mask out inconvenient features, to try and disable as many
 	 * unsupported kernel subsystems as possible.
 	 */
 	switch (*ax) {
-	case 1:
-		maskecx = cpuid_leaf1_ecx_mask;
-		break;
-
 	case CPUID_MWAIT_LEAF:
 		/* Synthesize the values.. */
 		*ax = 0;
@@ -206,7 +200,6 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 		: "0" (*ax), "2" (*cx));
 
 	*bx &= maskebx;
-	*cx &= maskecx;
 }
 STACK_FRAME_NON_STANDARD(xen_cpuid); /* XEN_EMULATE_PREFIX */
 
@@ -281,22 +274,24 @@ static bool __init xen_check_mwait(void)
 	return false;
 #endif
 }
-static void __init xen_init_cpuid_mask(void)
+
+static bool __init xen_check_xsave(void)
 {
-	unsigned int ax, bx, cx, dx;
-	unsigned int xsave_mask;
+	unsigned int err, eax, edx;
 
-	ax = 1;
-	cx = 0;
-	cpuid(1, &ax, &bx, &cx, &dx);
+	/* Test OSXSAVE capability via xgetbv instruction. */
+	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
+		     "xor %[err], %[err]\n"
+		     "2:\n\t"
+		     ".pushsection .fixup,\"ax\"\n\t"
+		     "3: movl $1,%[err]\n\t"
+		     "jmp 2b\n\t"
+		     ".popsection\n\t"
+		     _ASM_EXTABLE(1b, 3b)
+		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
+		     : "c" (0));
 
-	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 */
+	return err == 0;
 }
 
 static void __init xen_init_capabilities(void)
@@ -316,6 +311,14 @@ static void __init xen_init_capabilities(void)
 		setup_force_cpu_cap(X86_FEATURE_MWAIT);
 	else
 		setup_clear_cpu_cap(X86_FEATURE_MWAIT);
+
+	if (xen_check_xsave()) {
+		setup_force_cpu_cap(X86_FEATURE_XSAVE);
+		setup_force_cpu_cap(X86_FEATURE_OSXSAVE);
+	} else {
+		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
+		setup_clear_cpu_cap(X86_FEATURE_OSXSAVE);
+	}
 }
 
 static void xen_set_debugreg(int reg, unsigned long val)
@@ -1308,7 +1311,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	xen_setup_gdt(0);
 
 	xen_init_irq_ops();
-	xen_init_cpuid_mask();
 	xen_init_capabilities();
 
 #ifdef CONFIG_X86_LOCAL_APIC
-- 
2.12.0

[toc] | [next] | [standalone]

#1625201 — Re: [Xen-devel] [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 18/04/17 07:31, Juergen Gross wrote:
> @@ -281,22 +274,24 @@ static bool __init xen_check_mwait(void)
>  	return false;
>  #endif
>  }
> -static void __init xen_init_cpuid_mask(void)
> +
> +static bool __init xen_check_xsave(void)
>  {
> -	unsigned int ax, bx, cx, dx;
> -	unsigned int xsave_mask;
> +	unsigned int err, eax, edx;
>  
> -	ax = 1;
> -	cx = 0;
> -	cpuid(1, &ax, &bx, &cx, &dx);
> +	/* Test OSXSAVE capability via xgetbv instruction. */

The code is fine, but this comment isn't going to be any help to people
reading this code in 6 months time.

How about this:

"Xen 4.0 and older accidentally leaked the host XSAVE flag into guest
view, despite not being able to support guests using the functionality. 
Probe for the actual availability of XSAVE by seeing whether xgetbv
executes successfully or raises #UD."

Everything else is fine, so Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

~Andrew

[toc] | [prev] | [next] | [standalone]

#1625281 — Re: [Xen-devel] [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 18/04/17 12:02, Andrew Cooper wrote:
> On 18/04/17 07:31, Juergen Gross wrote:
>> @@ -281,22 +274,24 @@ static bool __init xen_check_mwait(void)
>>  	return false;
>>  #endif
>>  }
>> -static void __init xen_init_cpuid_mask(void)
>> +
>> +static bool __init xen_check_xsave(void)
>>  {
>> -	unsigned int ax, bx, cx, dx;
>> -	unsigned int xsave_mask;
>> +	unsigned int err, eax, edx;
>>  
>> -	ax = 1;
>> -	cx = 0;
>> -	cpuid(1, &ax, &bx, &cx, &dx);
>> +	/* Test OSXSAVE capability via xgetbv instruction. */
> 
> The code is fine, but this comment isn't going to be any help to people
> reading this code in 6 months time.
> 
> How about this:
> 
> "Xen 4.0 and older accidentally leaked the host XSAVE flag into guest
> view, despite not being able to support guests using the functionality. 
> Probe for the actual availability of XSAVE by seeing whether xgetbv
> executes successfully or raises #UD."

I'll update the comment.

> Everything else is fine, so Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Thanks,

Juergen

[toc] | [prev] | [next] | [standalone]

#1628329 — Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

> +static bool __init xen_check_xsave(void)
>  {
> -	unsigned int ax, bx, cx, dx;
> -	unsigned int xsave_mask;
> +	unsigned int err, eax, edx;
>  
> -	ax = 1;
> -	cx = 0;
> -	cpuid(1, &ax, &bx, &cx, &dx);
> +	/* Test OSXSAVE capability via xgetbv instruction. */
> +	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
> +		     "xor %[err], %[err]\n"
> +		     "2:\n\t"
> +		     ".pushsection .fixup,\"ax\"\n\t"
> +		     "3: movl $1,%[err]\n\t"
> +		     "jmp 2b\n\t"
> +		     ".popsection\n\t"
> +		     _ASM_EXTABLE(1b, 3b)
> +		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
> +		     : "c" (0));

Have you tested this on processors where we actually trap on xgetbv?

I have an AMD box without XSAVE support and this is a fatal error. I
suspect it's too early to use exception fixup framework here.

-boris

[toc] | [prev] | [next] | [standalone]

#1628352 — Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 21/04/17 16:24, Boris Ostrovsky wrote:
> 
>> +static bool __init xen_check_xsave(void)
>>  {
>> -	unsigned int ax, bx, cx, dx;
>> -	unsigned int xsave_mask;
>> +	unsigned int err, eax, edx;
>>  
>> -	ax = 1;
>> -	cx = 0;
>> -	cpuid(1, &ax, &bx, &cx, &dx);
>> +	/* Test OSXSAVE capability via xgetbv instruction. */
>> +	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
>> +		     "xor %[err], %[err]\n"
>> +		     "2:\n\t"
>> +		     ".pushsection .fixup,\"ax\"\n\t"
>> +		     "3: movl $1,%[err]\n\t"
>> +		     "jmp 2b\n\t"
>> +		     ".popsection\n\t"
>> +		     _ASM_EXTABLE(1b, 3b)
>> +		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
>> +		     : "c" (0));
> 
> Have you tested this on processors where we actually trap on xgetbv?
> 
> I have an AMD box without XSAVE support and this is a fatal error. I
> suspect it's too early to use exception fixup framework here.

Uuh, too bad.

Then I fear we must use the other solution Andrew didn't like. :-(
Andrew, would you be okay with that?


Juergen

[toc] | [prev] | [next] | [standalone]

#1628435 — Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 21/04/17 15:38, Juergen Gross wrote:
> On 21/04/17 16:24, Boris Ostrovsky wrote:
>>> +static bool __init xen_check_xsave(void)
>>>  {
>>> -	unsigned int ax, bx, cx, dx;
>>> -	unsigned int xsave_mask;
>>> +	unsigned int err, eax, edx;
>>>  
>>> -	ax = 1;
>>> -	cx = 0;
>>> -	cpuid(1, &ax, &bx, &cx, &dx);
>>> +	/* Test OSXSAVE capability via xgetbv instruction. */
>>> +	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
>>> +		     "xor %[err], %[err]\n"
>>> +		     "2:\n\t"
>>> +		     ".pushsection .fixup,\"ax\"\n\t"
>>> +		     "3: movl $1,%[err]\n\t"
>>> +		     "jmp 2b\n\t"
>>> +		     ".popsection\n\t"
>>> +		     _ASM_EXTABLE(1b, 3b)
>>> +		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
>>> +		     : "c" (0));
>> Have you tested this on processors where we actually trap on xgetbv?
>>
>> I have an AMD box without XSAVE support and this is a fatal error. I
>> suspect it's too early to use exception fixup framework here.
> Uuh, too bad.
>
> Then I fear we must use the other solution Andrew didn't like. :-(
> Andrew, would you be okay with that?

Hmm fine.  The status quo is probably best then to unblock this series.

As an independent question, why are exceptions set up so late?  They
really should be the very first thing done.

~Andrew

[toc] | [prev] | [next] | [standalone]

#1628531 — Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 04/21/2017 10:45 AM, Andrew Cooper wrote:
> On 21/04/17 15:38, Juergen Gross wrote:
>> On 21/04/17 16:24, Boris Ostrovsky wrote:
>>>> +static bool __init xen_check_xsave(void)
>>>>  {
>>>> -	unsigned int ax, bx, cx, dx;
>>>> -	unsigned int xsave_mask;
>>>> +	unsigned int err, eax, edx;
>>>>  
>>>> -	ax = 1;
>>>> -	cx = 0;
>>>> -	cpuid(1, &ax, &bx, &cx, &dx);
>>>> +	/* Test OSXSAVE capability via xgetbv instruction. */
>>>> +	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
>>>> +		     "xor %[err], %[err]\n"
>>>> +		     "2:\n\t"
>>>> +		     ".pushsection .fixup,\"ax\"\n\t"
>>>> +		     "3: movl $1,%[err]\n\t"
>>>> +		     "jmp 2b\n\t"
>>>> +		     ".popsection\n\t"
>>>> +		     _ASM_EXTABLE(1b, 3b)
>>>> +		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
>>>> +		     : "c" (0));
>>> Have you tested this on processors where we actually trap on xgetbv?
>>>
>>> I have an AMD box without XSAVE support and this is a fatal error. I
>>> suspect it's too early to use exception fixup framework here.
>> Uuh, too bad.
>>
>> Then I fear we must use the other solution Andrew didn't like. :-(
>> Andrew, would you be okay with that?
> Hmm fine.  The status quo is probably best then to unblock this series.
>
> As an independent question, why are exceptions set up so late?  They
> really should be the very first thing done

It's exception fixup that is not set up yet --- we are executing here
before "main" kernel's entry point.

This is somewhat similar to what
arch/x86/kernel/head_64.S:early_idt_handler_common() does --- it has
special handling for early fixup --- early_fixup_exception().

I wonder though --- can this feature masking be deferred until a bit later?

-boris

[toc] | [prev] | [next] | [standalone]

#1629236 — Re: [PATCH v3 09/11] x86/xen: use capabilities instead of fake cpuid values for xsave

On 21/04/17 16:51, Boris Ostrovsky wrote:
> On 04/21/2017 10:45 AM, Andrew Cooper wrote:
>> On 21/04/17 15:38, Juergen Gross wrote:
>>> On 21/04/17 16:24, Boris Ostrovsky wrote:
>>>>> +static bool __init xen_check_xsave(void)
>>>>>  {
>>>>> -	unsigned int ax, bx, cx, dx;
>>>>> -	unsigned int xsave_mask;
>>>>> +	unsigned int err, eax, edx;
>>>>>  
>>>>> -	ax = 1;
>>>>> -	cx = 0;
>>>>> -	cpuid(1, &ax, &bx, &cx, &dx);
>>>>> +	/* Test OSXSAVE capability via xgetbv instruction. */
>>>>> +	asm volatile("1: .byte 0x0f,0x01,0xd0\n\t" /* xgetbv */
>>>>> +		     "xor %[err], %[err]\n"
>>>>> +		     "2:\n\t"
>>>>> +		     ".pushsection .fixup,\"ax\"\n\t"
>>>>> +		     "3: movl $1,%[err]\n\t"
>>>>> +		     "jmp 2b\n\t"
>>>>> +		     ".popsection\n\t"
>>>>> +		     _ASM_EXTABLE(1b, 3b)
>>>>> +		     : [err] "=r" (err), "=a" (eax), "=d" (edx)
>>>>> +		     : "c" (0));
>>>> Have you tested this on processors where we actually trap on xgetbv?
>>>>
>>>> I have an AMD box without XSAVE support and this is a fatal error. I
>>>> suspect it's too early to use exception fixup framework here.
>>> Uuh, too bad.
>>>
>>> Then I fear we must use the other solution Andrew didn't like. :-(
>>> Andrew, would you be okay with that?
>> Hmm fine.  The status quo is probably best then to unblock this series.
>>
>> As an independent question, why are exceptions set up so late?  They
>> really should be the very first thing done
> 
> It's exception fixup that is not set up yet --- we are executing here
> before "main" kernel's entry point.
> 
> This is somewhat similar to what
> arch/x86/kernel/head_64.S:early_idt_handler_common() does --- it has
> special handling for early fixup --- early_fixup_exception().
> 
> I wonder though --- can this feature masking be deferred until a bit later?

At least the xsave feature is tested rather early: it is needed in
early_cpu_init() being called way before trap_init().


Juergen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web