Re: [PATCH] x86/cpuid: fix dom0 crash on skylake machine

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

On 01/06/16 05:58, Luwei Kang wrote:
> CPUID.0XD.0X0.EAX is from machine value for dom0, and dom0 kernel will xsetbv
> with xfeatures_mask that is from CPUID.0XD.0X0.EAX, but handle_xsetbv has
> ingored XSTATE_PKRU with hardware protection fault emulation, so dom0 kernel
> will crash on skylake machine with PKRU support.
>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
>  xen/arch/x86/traps.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 1ef8401..5e72e44 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1100,6 +1100,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
>               */
>              if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
>                  cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp);
> +
> +            /* PV is not supported by XSTATE_PKRU. */
> +            a &= ~XSTATE_PKRU;
>              break;
>          }
>  

While this does work, it undoes some of the work I started with my cpuid
improvements in 4.7

Does the attached patch also resolve your issue?

~Andrew

[-- Attachment #2: 0001-xen-x86-Clip-guests-view-of-xfeature_mask-at-the-per.patch --]
[-- Type: text/x-diff, Size: 4491 bytes --]

>From 3cbb2a63fe368ac185e483b9ef5a8504340a3702 Mon Sep 17 00:00:00 2001
From: Andrew Cooper <andrew.cooper3@citrix.com>
Date: Wed, 1 Jun 2016 10:00:12 +0100
Subject: [PATCH] xen/x86: Clip guests view of xfeature_mask at the per-domain
 maximum

---
 xen/arch/x86/cpuid.c        | 33 ++++++++++++++++++++++++++++++---
 xen/arch/x86/hvm/hvm.c      |  3 +++
 xen/arch/x86/traps.c        |  3 +++
 xen/include/asm-x86/cpuid.h |  2 ++
 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e1e0e44..0a75d4a 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -4,6 +4,7 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/vmx/vmcs.h>
 #include <asm/processor.h>
+#include <asm/xstate.h>
 
 const uint32_t known_features[] = INIT_KNOWN_FEATURES;
 const uint32_t special_features[] = INIT_SPECIAL_FEATURES;
@@ -17,11 +18,14 @@ uint32_t __read_mostly raw_featureset[FSCAPINTS];
 uint32_t __read_mostly pv_featureset[FSCAPINTS];
 uint32_t __read_mostly hvm_featureset[FSCAPINTS];
 
-static void __init sanitise_featureset(uint32_t *fs)
+uint64_t __read_mostly pv_xfeature_mask, __read_mostly hvm_xfeature_mask;
+
+static void __init sanitise_featureset(uint32_t *fs, uint64_t *xfeature_mask)
 {
     /* for_each_set_bit() uses unsigned longs.  Extend with zeroes. */
     uint32_t disabled_features[
         ROUNDUP(FSCAPINTS, sizeof(unsigned long)/sizeof(uint32_t))] = {};
+    uint64_t mask = 0;
     unsigned int i;
 
     for ( i = 0; i < FSCAPINTS; ++i )
@@ -62,6 +66,29 @@ static void __init sanitise_featureset(uint32_t *fs)
      */
     fs[FEATURESET_e1d] = ((fs[FEATURESET_1d]  &  CPUID_COMMON_1D_FEATURES) |
                           (fs[FEATURESET_e1d] & ~CPUID_COMMON_1D_FEATURES));
+
+    /*
+     * Calculate the maximum applicable xfeature mask, based on the
+     * featureset.
+     */
+    if ( test_bit(X86_FEATURE_XSAVE, fs) )
+    {
+        mask = XSTATE_SSE | XSTATE_FP;
+
+        if ( test_bit(X86_FEATURE_AVX, fs) )
+            mask |= XSTATE_YMM;
+
+        if ( test_bit(X86_FEATURE_MPX, fs) )
+            mask |= XSTATE_BNDREGS | XSTATE_BNDCSR;
+
+        if ( test_bit(X86_FEATURE_PKU, fs) )
+            mask |= XSTATE_PKRU;
+
+        if ( test_bit(X86_FEATURE_LWP, fs) )
+            mask |= XSTATE_LWP;
+    }
+
+    *xfeature_mask = mask;
 }
 
 static void __init calculate_raw_featureset(void)
@@ -119,7 +146,7 @@ static void __init calculate_pv_featureset(void)
     __set_bit(X86_FEATURE_X2APIC, pv_featureset);
     __set_bit(X86_FEATURE_CMP_LEGACY, pv_featureset);
 
-    sanitise_featureset(pv_featureset);
+    sanitise_featureset(pv_featureset, &pv_xfeature_mask);
 }
 
 static void __init calculate_hvm_featureset(void)
@@ -179,7 +206,7 @@ static void __init calculate_hvm_featureset(void)
             __clear_bit(X86_FEATURE_PCOMMIT, hvm_featureset);
     }
 
-    sanitise_featureset(hvm_featureset);
+    sanitise_featureset(hvm_featureset, & hvm_xfeature_mask);
 }
 
 void __init calculate_featuresets(void)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5040a5c..8678da9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3448,6 +3448,9 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                 if ( (_eax + _ebx) > *ebx )
                     *ebx = _eax + _ebx;
             }
+
+            *eax &= (uint32_t)hvm_xfeature_mask;
+            *edx &= (uint32_t)(hvm_xfeature_mask >> 32);
         }
 
         if ( count == 1 )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1ef8401..81935b8 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1101,6 +1101,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
             if ( !is_control_domain(currd) && !is_hardware_domain(currd) )
                 cpuid_count(leaf, subleaf, &tmp, &b, &tmp, &tmp);
             break;
+
+            a &= (uint32_t)pv_xfeature_mask;
+            d &= (uint32_t)(pv_xfeature_mask >> 32);
         }
 
         case 1:
diff --git a/xen/include/asm-x86/cpuid.h b/xen/include/asm-x86/cpuid.h
index 9a21c25..231c6d4 100644
--- a/xen/include/asm-x86/cpuid.h
+++ b/xen/include/asm-x86/cpuid.h
@@ -29,6 +29,8 @@ extern uint32_t raw_featureset[FSCAPINTS];
 extern uint32_t pv_featureset[FSCAPINTS];
 extern uint32_t hvm_featureset[FSCAPINTS];
 
+extern uint64_t pv_xfeature_mask, hvm_xfeature_mask;
+
 void calculate_featuresets(void);
 
 const uint32_t *lookup_deep_deps(uint32_t feature);
-- 
2.1.4


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

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