[PATCH 0/3] x86: assorted pv_cpuid() adjustments

[PATCH 0/3] x86: assorted pv_cpuid() adjustments

[Jan Beulich]

1: don't expose XSAVES capability to PV guests
2: correctly check for sub-leaf zero of leaf 7 in pv_cpuid()
3: don't open-code cpuid_count() in pv_cpuid()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/3] x86: don't expose XSAVES capability to PV guests

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]

As done by the recent Linux commit b65d6e17fe ("kvm: x86: mask out
XSAVES") for KVM, we should also mask out XSAVES from what PV guests
get to see as long as we don't emulate accesses to MSR_IA32_XSS.

Actually, go beyond that: Just like for leaf 7, switch from
blacklisting to whitelisting, i.e. only allow XSAVEOPT and XSAVEC for
the time being. And do these overrides consistently for both Dom0 and
DomU-s.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -837,7 +837,7 @@ void pv_cpuid(struct cpu_user_regs *regs
 
         switch ( cpuid_leaf )
         {
-        case 0xd:
+        case XSTATE_CPUID:
         {
             unsigned int _eax, _ebx, _ecx, _edx;
             /* EBX value of main leaf 0 depends on enabled xsave features */
@@ -855,7 +855,7 @@ void pv_cpuid(struct cpu_user_regs *regs
                         b = _eax + _ebx;
                 }
             }
-        break;
+            goto xstate;
         }
         }
         goto out;
@@ -931,9 +931,19 @@ void pv_cpuid(struct cpu_user_regs *regs
         a = c = d = 0;
         break;
 
-    case 0x0000000d: /* XSAVE */
+    case XSTATE_CPUID:
+    xstate:
         if ( !cpu_has_xsave )
             goto unsupported;
+        if ( regs->_ecx == 1 )
+        {
+            a &= XSTATE_FEATURE_XSAVEOPT |
+                 XSTATE_FEATURE_XSAVEC |
+                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
+                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
+            if ( !cpu_has_xsaves )
+                b = c = d = 0;
+        }
         break;
 
     case 0x80000001:
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -14,7 +14,10 @@
 #include <asm/xstate.h>
 #include <asm/asm_defns.h>
 
-bool_t __read_mostly cpu_has_xsaveopt;
+static bool_t __read_mostly cpu_has_xsaveopt;
+static bool_t __read_mostly cpu_has_xsavec;
+bool_t __read_mostly cpu_has_xgetbv1;
+bool_t __read_mostly cpu_has_xsaves;
 
 /*
  * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
@@ -320,12 +323,22 @@ void xstate_init(bool_t bsp)
         BUG_ON(xsave_cntxt_size != _xstate_ctxt_size(feature_mask));
     }
 
-    /* Check XSAVEOPT feature. */
+    /* Check extended XSAVE features. */
     cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
     if ( bsp )
+    {
         cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
+        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
+        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
+        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
+    }
     else
+    {
         BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
+        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
+        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
+        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
+    }
 }
 
 static bool_t valid_xcr0(u64 xcr0)
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -16,6 +16,9 @@
 
 #define XSTATE_CPUID              0x0000000d
 #define XSTATE_FEATURE_XSAVEOPT   (1 << 0)    /* sub-leaf 1, eax[bit 0] */
+#define XSTATE_FEATURE_XSAVEC     (1 << 1)    /* sub-leaf 1, eax[bit 1] */
+#define XSTATE_FEATURE_XGETBV1    (1 << 2)    /* sub-leaf 1, eax[bit 2] */
+#define XSTATE_FEATURE_XSAVES     (1 << 3)    /* sub-leaf 1, eax[bit 3] */
 
 #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
 
@@ -40,6 +43,7 @@
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 
 extern u64 xfeature_mask;
+extern bool_t cpu_has_xsaves, cpu_has_xgetbv1;
 
 /* extended state save area */
 struct __packed __attribute__((aligned (64))) xsave_struct




[-- Attachment #2: x86-pv-suppress-xsaves.patch --]
[-- Type: text/plain, Size: 3896 bytes --]

x86: don't expose XSAVES capability to PV guests

As done by the recent Linux commit b65d6e17fe ("kvm: x86: mask out
XSAVES") for KVM, we should also mask out XSAVES from what PV guests
get to see as long as we don't emulate accesses to MSR_IA32_XSS.

Actually, go beyond that: Just like for leaf 7, switch from
blacklisting to whitelisting, i.e. only allow XSAVEOPT and XSAVEC for
the time being. And do these overrides consistently for both Dom0 and
DomU-s.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -837,7 +837,7 @@ void pv_cpuid(struct cpu_user_regs *regs
 
         switch ( cpuid_leaf )
         {
-        case 0xd:
+        case XSTATE_CPUID:
         {
             unsigned int _eax, _ebx, _ecx, _edx;
             /* EBX value of main leaf 0 depends on enabled xsave features */
@@ -855,7 +855,7 @@ void pv_cpuid(struct cpu_user_regs *regs
                         b = _eax + _ebx;
                 }
             }
-        break;
+            goto xstate;
         }
         }
         goto out;
@@ -931,9 +931,19 @@ void pv_cpuid(struct cpu_user_regs *regs
         a = c = d = 0;
         break;
 
-    case 0x0000000d: /* XSAVE */
+    case XSTATE_CPUID:
+    xstate:
         if ( !cpu_has_xsave )
             goto unsupported;
+        if ( regs->_ecx == 1 )
+        {
+            a &= XSTATE_FEATURE_XSAVEOPT |
+                 XSTATE_FEATURE_XSAVEC |
+                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
+                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
+            if ( !cpu_has_xsaves )
+                b = c = d = 0;
+        }
         break;
 
     case 0x80000001:
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -14,7 +14,10 @@
 #include <asm/xstate.h>
 #include <asm/asm_defns.h>
 
-bool_t __read_mostly cpu_has_xsaveopt;
+static bool_t __read_mostly cpu_has_xsaveopt;
+static bool_t __read_mostly cpu_has_xsavec;
+bool_t __read_mostly cpu_has_xgetbv1;
+bool_t __read_mostly cpu_has_xsaves;
 
 /*
  * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
@@ -320,12 +323,22 @@ void xstate_init(bool_t bsp)
         BUG_ON(xsave_cntxt_size != _xstate_ctxt_size(feature_mask));
     }
 
-    /* Check XSAVEOPT feature. */
+    /* Check extended XSAVE features. */
     cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
     if ( bsp )
+    {
         cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
+        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
+        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
+        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
+    }
     else
+    {
         BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
+        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
+        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
+        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
+    }
 }
 
 static bool_t valid_xcr0(u64 xcr0)
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -16,6 +16,9 @@
 
 #define XSTATE_CPUID              0x0000000d
 #define XSTATE_FEATURE_XSAVEOPT   (1 << 0)    /* sub-leaf 1, eax[bit 0] */
+#define XSTATE_FEATURE_XSAVEC     (1 << 1)    /* sub-leaf 1, eax[bit 1] */
+#define XSTATE_FEATURE_XGETBV1    (1 << 2)    /* sub-leaf 1, eax[bit 2] */
+#define XSTATE_FEATURE_XSAVES     (1 << 3)    /* sub-leaf 1, eax[bit 3] */
 
 #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
 
@@ -40,6 +43,7 @@
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 
 extern u64 xfeature_mask;
+extern bool_t cpu_has_xsaves, cpu_has_xgetbv1;
 
 /* extended state save area */
 struct __packed __attribute__((aligned (64))) xsave_struct

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 2/3] x86: correctly check for sub‑leaf zero of leaf 7 in pv_cpuid()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

Only the low 32 bits are relevant.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -916,7 +916,7 @@ void pv_cpuid(struct cpu_user_regs *regs
         break;
 
     case 0x00000007:
-        if ( regs->ecx == 0 )
+        if ( regs->_ecx == 0 )
             b &= (cpufeat_mask(X86_FEATURE_BMI1) |
                   cpufeat_mask(X86_FEATURE_HLE)  |
                   cpufeat_mask(X86_FEATURE_AVX2) |




[-- Attachment #2: x86-pv-cpuid-leaf7.patch --]
[-- Type: text/plain, Size: 539 bytes --]

x86: correctly check for sub-leaf zero of leaf 7 in pv_cpuid()

Only the low 32 bits are relevant.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -916,7 +916,7 @@ void pv_cpuid(struct cpu_user_regs *regs
         break;
 
     case 0x00000007:
-        if ( regs->ecx == 0 )
+        if ( regs->_ecx == 0 )
             b &= (cpufeat_mask(X86_FEATURE_BMI1) |
                   cpufeat_mask(X86_FEATURE_HLE)  |
                   cpufeat_mask(X86_FEATURE_AVX2) |

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

[PATCH 3/3] x86: don't open-code cpuid_count() in pv_cpuid()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -861,10 +861,7 @@ void pv_cpuid(struct cpu_user_regs *regs
         goto out;
     }
 
-    asm ( 
-        "cpuid"
-        : "=a" (a), "=b" (b), "=c" (c), "=d" (d)
-        : "0" (a), "1" (b), "2" (c), "3" (d) );
+    cpuid_count(a, c, &a, &b, &c, &d);
 
     if ( (regs->eax & 0x7fffffff) == 0x00000001 )
     {




[-- Attachment #2: x86-pv-cpuid-use-helper.patch --]
[-- Type: text/plain, Size: 488 bytes --]

x86: don't open-code cpuid_count() in pv_cpuid()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -861,10 +861,7 @@ void pv_cpuid(struct cpu_user_regs *regs
         goto out;
     }
 
-    asm ( 
-        "cpuid"
-        : "=a" (a), "=b" (b), "=c" (c), "=d" (d)
-        : "0" (a), "1" (b), "2" (c), "3" (d) );
+    cpuid_count(a, c, &a, &b, &c, &d);
 
     if ( (regs->eax & 0x7fffffff) == 0x00000001 )
     {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH 1/3] x86: don't expose XSAVES capability to PV guests

[Andrew Cooper]

On 19/01/15 15:27, Jan Beulich wrote:
> As done by the recent Linux commit b65d6e17fe ("kvm: x86: mask out
> XSAVES") for KVM, we should also mask out XSAVES from what PV guests
> get to see as long as we don't emulate accesses to MSR_IA32_XSS.
>
> Actually, go beyond that: Just like for leaf 7, switch from
> blacklisting to whitelisting, i.e. only allow XSAVEOPT and XSAVEC for
> the time being. And do these overrides consistently for both Dom0 and
> DomU-s.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -837,7 +837,7 @@ void pv_cpuid(struct cpu_user_regs *regs
>  
>          switch ( cpuid_leaf )
>          {
> -        case 0xd:
> +        case XSTATE_CPUID:
>          {
>              unsigned int _eax, _ebx, _ecx, _edx;
>              /* EBX value of main leaf 0 depends on enabled xsave features */
> @@ -855,7 +855,7 @@ void pv_cpuid(struct cpu_user_regs *regs
>                          b = _eax + _ebx;
>                  }
>              }
> -        break;
> +            goto xstate;
>          }
>          }
>          goto out;
> @@ -931,9 +931,19 @@ void pv_cpuid(struct cpu_user_regs *regs
>          a = c = d = 0;
>          break;
>  
> -    case 0x0000000d: /* XSAVE */
> +    case XSTATE_CPUID:
> +    xstate:
>          if ( !cpu_has_xsave )
>              goto unsupported;
> +        if ( regs->_ecx == 1 )
> +        {
> +            a &= XSTATE_FEATURE_XSAVEOPT |
> +                 XSTATE_FEATURE_XSAVEC |
> +                 (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
> +                 (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
> +            if ( !cpu_has_xsaves )
> +                b = c = d = 0;
> +        }
>          break;
>  
>      case 0x80000001:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -14,7 +14,10 @@
>  #include <asm/xstate.h>
>  #include <asm/asm_defns.h>
>  
> -bool_t __read_mostly cpu_has_xsaveopt;
> +static bool_t __read_mostly cpu_has_xsaveopt;
> +static bool_t __read_mostly cpu_has_xsavec;
> +bool_t __read_mostly cpu_has_xgetbv1;
> +bool_t __read_mostly cpu_has_xsaves;
>  
>  /*
>   * Maximum size (in byte) of the XSAVE/XRSTOR save area required by all
> @@ -320,12 +323,22 @@ void xstate_init(bool_t bsp)
>          BUG_ON(xsave_cntxt_size != _xstate_ctxt_size(feature_mask));
>      }
>  
> -    /* Check XSAVEOPT feature. */
> +    /* Check extended XSAVE features. */
>      cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
>      if ( bsp )
> +    {
>          cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
> +        cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
> +        /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
> +        /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
> +    }
>      else
> +    {
>          BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
> +        BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
> +        /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1)); */
> +        /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
> +    }
>  }
>  
>  static bool_t valid_xcr0(u64 xcr0)
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -16,6 +16,9 @@
>  
>  #define XSTATE_CPUID              0x0000000d
>  #define XSTATE_FEATURE_XSAVEOPT   (1 << 0)    /* sub-leaf 1, eax[bit 0] */
> +#define XSTATE_FEATURE_XSAVEC     (1 << 1)    /* sub-leaf 1, eax[bit 1] */
> +#define XSTATE_FEATURE_XGETBV1    (1 << 2)    /* sub-leaf 1, eax[bit 2] */
> +#define XSTATE_FEATURE_XSAVES     (1 << 3)    /* sub-leaf 1, eax[bit 3] */
>  
>  #define XCR_XFEATURE_ENABLED_MASK 0x00000000  /* index of XCR0 */
>  
> @@ -40,6 +43,7 @@
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
>  
>  extern u64 xfeature_mask;
> +extern bool_t cpu_has_xsaves, cpu_has_xgetbv1;
>  
>  /* extended state save area */
>  struct __packed __attribute__((aligned (64))) xsave_struct
>
>
>

Re: [PATCH 2/3] x86: correctly check for sub‑leaf zero of leaf 7 in pv_cpuid()

[Andrew Cooper]

On 19/01/15 15:27, Jan Beulich wrote:
> Only the low 32 bits are relevant.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

There are two examples of "regs->eax" which could turn to regs->_eax
under the same argument; the if statement immediately after cpuid, and
the switch statement.

Either way,

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -916,7 +916,7 @@ void pv_cpuid(struct cpu_user_regs *regs
>          break;
>  
>      case 0x00000007:
> -        if ( regs->ecx == 0 )
> +        if ( regs->_ecx == 0 )
>              b &= (cpufeat_mask(X86_FEATURE_BMI1) |
>                    cpufeat_mask(X86_FEATURE_HLE)  |
>                    cpufeat_mask(X86_FEATURE_AVX2) |
>
>
>

Re: [PATCH 3/3] x86: don't open-code cpuid_count() in pv_cpuid()

[Andrew Cooper]

On 19/01/15 15:28, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -861,10 +861,7 @@ void pv_cpuid(struct cpu_user_regs *regs
>          goto out;
>      }
>  
> -    asm ( 
> -        "cpuid"
> -        : "=a" (a), "=b" (b), "=c" (c), "=d" (d)
> -        : "0" (a), "1" (b), "2" (c), "3" (d) );
> +    cpuid_count(a, c, &a, &b, &c, &d);
>  
>      if ( (regs->eax & 0x7fffffff) == 0x00000001 )
>      {
>
>
>

Re: [PATCH 2/3] x86: correctly check for sub‑leaf zero of leaf 7 in pv_cpuid()

[Jan Beulich]

>>> On 20.01.15 at 11:46, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:27, Jan Beulich wrote:
>> Only the low 32 bits are relevant.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> There are two examples of "regs->eax" which could turn to regs->_eax
> under the same argument; the if statement immediately after cpuid,

In

    if ( (regs->eax & 0x7fffffff) == 0x00000001 )

it really doesn't matter, as the high 33 bits get chopped off anyway.

> and the switch statement.

Yes, this could be cleaned up. But still its not under the same argument
as

    switch ( (uint32_t)regs->eax )

and

    switch ( regs->_eax )

are functionally identical, whereas the sole original change actually
fixed a functionality problem.

Jan