[PATCH] xen/x86: Record xsave features in c->x86_capabilities

[PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Andrew Cooper]

Convert existing cpu_has_x??? to being functions of boot_cpu_data (matching
the prevailing style), and mask out unsupported features.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Shuai Ruan <shuai.ruan@linux.intel.com>

This is another patch from my feature levelling series which is being posted
early because of interactions with other code.

Shuai: This patch will interact with your xsaves series, although I hope it
will make your series easier.

Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
uncertainty with how this information is exposed in libxl.  This patch
introduces no change with how the information is represented in userspace.
---
 xen/arch/x86/cpu/common.c        |    2 +-
 xen/arch/x86/traps.c             |    5 +----
 xen/arch/x86/xstate.c            |   31 +++++++++++--------------------
 xen/include/asm-x86/cpufeature.h |   13 ++++++++++++-
 xen/include/asm-x86/xstate.h     |    8 ++------
 5 files changed, 27 insertions(+), 32 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 35ef21b..0be0656 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -311,7 +311,7 @@ void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 		clear_bit(X86_FEATURE_XSAVE, boot_cpu_data.x86_capability);
 
 	if ( cpu_has_xsave )
-		xstate_init(c == &boot_cpu_data);
+		xstate_init(c);
 
 	/*
 	 * The vendor-specific functions might have changed features.  Now
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9f5a6c6..07feb6d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -935,10 +935,7 @@ void pv_cpuid(struct cpu_user_regs *regs)
             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);
+            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
             if ( !cpu_has_xsaves )
                 b = c = d = 0;
         }
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index d5f5e3b..3e107a2 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -14,11 +14,6 @@
 #include <asm/xstate.h>
 #include <asm/asm_defns.h>
 
-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
  * the supported and enabled features on the processor, including the
@@ -281,8 +276,9 @@ unsigned int xstate_ctxt_size(u64 xcr0)
 }
 
 /* Collect the information of processor's extended state */
-void xstate_init(bool_t bsp)
+void xstate_init(struct cpuinfo_x86 *c)
 {
+    bool_t bsp = c == &boot_cpu_data;
     u32 eax, ebx, ecx, edx;
     u64 feature_mask;
 
@@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
 
     /* 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)); */
-    }
+
+    /* Mask out features not currently understood by Xen. */
+    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
+            cpufeat_mask(X86_FEATURE_XSAVEC));
+
+    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
+
+    if ( !bsp )
+        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
 }
 
 static bool_t valid_xcr0(u64 xcr0)
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 9a01563..aa228db 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -9,7 +9,7 @@
 #define __ASM_I386_CPUFEATURE_H
 #endif
 
-#define NCAPINTS	8	/* N 32-bit words worth of info */
+#define NCAPINTS	9	/* N 32-bit words worth of info */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (edx), word 0 */
 #define X86_FEATURE_FPU		(0*32+ 0) /* Onboard FPU */
@@ -154,6 +154,12 @@
 #define X86_FEATURE_ADX		(7*32+19) /* ADCX, ADOX instructions */
 #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
 
+/* Intel-defined CPU features, CPUID level 0x0000000D:1 (eax), word 8 */
+#define X86_FEATURE_XSAVEOPT	(8*32+ 0)
+#define X86_FEATURE_XSAVEC	(8*32+ 1)
+#define X86_FEATURE_XGETBV1	(8*32+ 2)
+#define X86_FEATURE_XSAVES	(8*32+ 3)
+
 #if !defined(__ASSEMBLY__) && !defined(X86_FEATURES_ONLY)
 #include <xen/bitops.h>
 
@@ -219,6 +225,11 @@
 
 #define cpu_has_cx16            boot_cpu_has(X86_FEATURE_CX16)
 
+#define cpu_has_xsaveopt	boot_cpu_has(X86_FEATURE_XSAVEOPT)
+#define cpu_has_xsavec		boot_cpu_has(X86_FEATURE_XSAVEC)
+#define cpu_has_xgetbv1		boot_cpu_has(X86_FEATURE_XGETBV1)
+#define cpu_has_xsaves		boot_cpu_has(X86_FEATURE_XSAVES)
+
 enum _cache_type {
     CACHE_TYPE_NULL = 0,
     CACHE_TYPE_DATA = 1,
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 4c690db..f0d8f0b 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -9,16 +9,13 @@
 #define __ASM_XSTATE_H
 
 #include <xen/types.h>
+#include <asm/cpufeature.h>
 
 #define FCW_DEFAULT               0x037f
 #define FCW_RESET                 0x0040
 #define MXCSR_DEFAULT             0x1f80
 
 #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 */
 
@@ -43,7 +40,6 @@
 #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
@@ -91,7 +87,7 @@ struct __packed __attribute__((aligned (64))) xsave_struct
 /* extended state init and cleanup functions */
 void xstate_free_save_area(struct vcpu *v);
 int xstate_alloc_save_area(struct vcpu *v);
-void xstate_init(bool_t bsp);
+void xstate_init(struct cpuinfo_x86 *c);
 unsigned int xstate_ctxt_size(u64 xcr0);
 
 #endif /* __ASM_XSTATE_H */
-- 
1.7.10.4

Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Jan Beulich]

>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
> uncertainty with how this information is exposed in libxl.  This patch
> introduces no change with how the information is represented in userspace.

Mind explaining this "uncertainty"? I'd like to avoid extending the array
for no real reason...

> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
>  
>      /* 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)); */
> -    }
> +
> +    /* Mask out features not currently understood by Xen. */
> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> +            cpufeat_mask(X86_FEATURE_XSAVEC));
> +
> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
> +
> +    if ( !bsp )
> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>  }

The !bsp conditional seems pretty pointless. And with the revised
model it looks like it could be relaxed (BUG only when bits the BSP
has set aren't set on the AP).

> --- a/xen/include/asm-x86/cpufeature.h
> +++ b/xen/include/asm-x86/cpufeature.h
> @@ -9,7 +9,7 @@
>  #define __ASM_I386_CPUFEATURE_H
>  #endif
>  
> -#define NCAPINTS	8	/* N 32-bit words worth of info */
> +#define NCAPINTS	9	/* N 32-bit words worth of info */
>  
>  /* Intel-defined CPU features, CPUID level 0x00000001 (edx), word 0 */
>  #define X86_FEATURE_FPU		(0*32+ 0) /* Onboard FPU */
> @@ -154,6 +154,12 @@
>  #define X86_FEATURE_ADX		(7*32+19) /* ADCX, ADOX instructions */
>  #define X86_FEATURE_SMAP	(7*32+20) /* Supervisor Mode Access Prevention */
>  
> +/* Intel-defined CPU features, CPUID level 0x0000000D:1 (eax), word 8 */
> +#define X86_FEATURE_XSAVEOPT	(8*32+ 0)
> +#define X86_FEATURE_XSAVEC	(8*32+ 1)
> +#define X86_FEATURE_XGETBV1	(8*32+ 2)
> +#define X86_FEATURE_XSAVES	(8*32+ 3)
> +
>  #if !defined(__ASSEMBLY__) && !defined(X86_FEATURES_ONLY)
>  #include <xen/bitops.h>
>  
> @@ -219,6 +225,11 @@
>  
>  #define cpu_has_cx16            boot_cpu_has(X86_FEATURE_CX16)
>  
> +#define cpu_has_xsaveopt	boot_cpu_has(X86_FEATURE_XSAVEOPT)
> +#define cpu_has_xsavec		boot_cpu_has(X86_FEATURE_XSAVEC)
> +#define cpu_has_xgetbv1		boot_cpu_has(X86_FEATURE_XGETBV1)
> +#define cpu_has_xsaves		boot_cpu_has(X86_FEATURE_XSAVES)
> +
>  enum _cache_type {
>      CACHE_TYPE_NULL = 0,
>      CACHE_TYPE_DATA = 1,
> diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
> index 4c690db..f0d8f0b 100644
> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -9,16 +9,13 @@
>  #define __ASM_XSTATE_H
>  
>  #include <xen/types.h>
> +#include <asm/cpufeature.h>
>  
>  #define FCW_DEFAULT               0x037f
>  #define FCW_RESET                 0x0040
>  #define MXCSR_DEFAULT             0x1f80
>  
>  #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 */
>  
> @@ -43,7 +40,6 @@
>  #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
> @@ -91,7 +87,7 @@ struct __packed __attribute__((aligned (64))) xsave_struct
>  /* extended state init and cleanup functions */
>  void xstate_free_save_area(struct vcpu *v);
>  int xstate_alloc_save_area(struct vcpu *v);
> -void xstate_init(bool_t bsp);
> +void xstate_init(struct cpuinfo_x86 *c);
>  unsigned int xstate_ctxt_size(u64 xcr0);
>  
>  #endif /* __ASM_XSTATE_H */
> -- 
> 1.7.10.4

Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Andrew Cooper]

On 21/09/15 14:04, Jan Beulich wrote:
>>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
>> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
>> uncertainty with how this information is exposed in libxl.  This patch
>> introduces no change with how the information is represented in userspace.
> Mind explaining this "uncertainty"? I'd like to avoid extending the array
> for no real reason...

libxl exports "hw_caps" as uint32_t caps[8] in its API.

I am uncertain what reusing word 2, or extending the length of the array 
means WRT to the API/ABI guarantees of libxl.

For hw_caps itself, the data is essentially useless as there is no 
defined layout,  Furthermore, some of the leaves are 
arbitrary/synthetic.  One option might be to just drop it from libxl 
entirely, but this will need to be decided by the toolstack maintainers.

For my cpuid levelling series, there is a new set exported which has an 
ABI guarantee of which words map to which cpuid feature leaves, and a 
strict rule of no synthetic values.

>
>> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
>>   
>>       /* 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)); */
>> -    }
>> +
>> +    /* Mask out features not currently understood by Xen. */
>> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>> +            cpufeat_mask(X86_FEATURE_XSAVEC));
>> +
>> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>> +
>> +    if ( !bsp )
>> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>   }
> The !bsp conditional seems pretty pointless. And with the revised
> model it looks like it could be relaxed (BUG only when bits the BSP
> has set aren't set on the AP).

I would be very wary about allowing a situation where certain amounts of 
heterogeneity would be permitted.  Even moreso with the xsaves 
extensions, any non-homogeneity in the system will result in data 
corruption.

I think it is better to keep this as strictly that the BSP must match 
all APs.  As soon as we encounter a system where this is not the case, 
far more areas will also need modifying.

~Andrew

Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Jan Beulich]

>>> On 21.09.15 at 15:51, <andrew.cooper3@citrix.com> wrote:
> On 21/09/15 14:04, Jan Beulich wrote:
>>>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
>>> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
>>> uncertainty with how this information is exposed in libxl.  This patch
>>> introduces no change with how the information is represented in userspace.
>> Mind explaining this "uncertainty"? I'd like to avoid extending the array
>> for no real reason...
> 
> libxl exports "hw_caps" as uint32_t caps[8] in its API.
> 
> I am uncertain what reusing word 2, or extending the length of the array 
> means WRT to the API/ABI guarantees of libxl.
> 
> For hw_caps itself, the data is essentially useless as there is no 
> defined layout,  Furthermore, some of the leaves are 
> arbitrary/synthetic.  One option might be to just drop it from libxl 
> entirely, but this will need to be decided by the toolstack maintainers.

Even more so a reason to re-use word 2.

>>> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
>>>   
>>>       /* 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)); */
>>> -    }
>>> +
>>> +    /* Mask out features not currently understood by Xen. */
>>> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>>> +            cpufeat_mask(X86_FEATURE_XSAVEC));
>>> +
>>> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>>> +
>>> +    if ( !bsp )
>>> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>>   }
>> The !bsp conditional seems pretty pointless. And with the revised
>> model it looks like it could be relaxed (BUG only when bits the BSP
>> has set aren't set on the AP).
> 
> I would be very wary about allowing a situation where certain amounts of 
> heterogeneity would be permitted.  Even moreso with the xsaves 
> extensions, any non-homogeneity in the system will result in data 
> corruption.
> 
> I think it is better to keep this as strictly that the BSP must match 
> all APs.  As soon as we encounter a system where this is not the case, 
> far more areas will also need modifying.

I guess you misunderstood - I didn't mean for both lines to be
dropped; I meant the if() surrounding the BUG_ON() to go away.

Jan

Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Andrew Cooper]

On 21/09/15 15:00, Jan Beulich wrote:
>>>> On 21.09.15 at 15:51, <andrew.cooper3@citrix.com> wrote:
>> On 21/09/15 14:04, Jan Beulich wrote:
>>>>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
>>>> Jan: I have opted for adding leaf 8 rather than reusing leaf 2, due to the
>>>> uncertainty with how this information is exposed in libxl.  This patch
>>>> introduces no change with how the information is represented in userspace.
>>> Mind explaining this "uncertainty"? I'd like to avoid extending the array
>>> for no real reason...
>> libxl exports "hw_caps" as uint32_t caps[8] in its API.
>>
>> I am uncertain what reusing word 2, or extending the length of the array
>> means WRT to the API/ABI guarantees of libxl.
>>
>> For hw_caps itself, the data is essentially useless as there is no
>> defined layout,  Furthermore, some of the leaves are
>> arbitrary/synthetic.  One option might be to just drop it from libxl
>> entirely, but this will need to be decided by the toolstack maintainers.
> Even more so a reason to re-use word 2.

This works for 0xd:1.eax, but the array has to be extended for 0x7:0.ebx 
and 0x7:0.ecx, both of which are also included in my levelling series.

Currently I have just left the libxl question alone.

>
>>>> @@ -325,20 +321,15 @@ void xstate_init(bool_t bsp)
>>>>    
>>>>        /* 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)); */
>>>> -    }
>>>> +
>>>> +    /* Mask out features not currently understood by Xen. */
>>>> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>>>> +            cpufeat_mask(X86_FEATURE_XSAVEC));
>>>> +
>>>> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>>>> +
>>>> +    if ( !bsp )
>>>> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>>>    }
>>> The !bsp conditional seems pretty pointless. And with the revised
>>> model it looks like it could be relaxed (BUG only when bits the BSP
>>> has set aren't set on the AP).
>> I would be very wary about allowing a situation where certain amounts of
>> heterogeneity would be permitted.  Even moreso with the xsaves
>> extensions, any non-homogeneity in the system will result in data
>> corruption.
>>
>> I think it is better to keep this as strictly that the BSP must match
>> all APs.  As soon as we encounter a system where this is not the case,
>> far more areas will also need modifying.
> I guess you misunderstood - I didn't mean for both lines to be
> dropped; I meant the if() surrounding the BUG_ON() to go away.

I don't mind dropping the if(), but I was querying your suggestion in 
brackets.

~Andrew

Re: [PATCH] xen/x86: Record xsave features in c->x86_capabilities

[Jan Beulich]

>>> On 21.09.15 at 16:09, <andrew.cooper3@citrix.com> wrote:
> On 21/09/15 15:00, Jan Beulich wrote:
>>>>> On 21.09.15 at 15:51, <andrew.cooper3@citrix.com> wrote:
>>> On 21/09/15 14:04, Jan Beulich wrote:
>>>>>>> On 17.09.15 at 13:40, <andrew.cooper3@citrix.com> wrote:
>>>>> +    /* Mask out features not currently understood by Xen. */
>>>>> +    eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
>>>>> +            cpufeat_mask(X86_FEATURE_XSAVEC));
>>>>> +
>>>>> +    c->x86_capability[X86_FEATURE_XSAVEOPT / 32] = eax;
>>>>> +
>>>>> +    if ( !bsp )
>>>>> +        BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
>>>>>    }
>>>> The !bsp conditional seems pretty pointless. And with the revised
>>>> model it looks like it could be relaxed (BUG only when bits the BSP
>>>> has set aren't set on the AP).
>>> I would be very wary about allowing a situation where certain amounts of
>>> heterogeneity would be permitted.  Even moreso with the xsaves
>>> extensions, any non-homogeneity in the system will result in data
>>> corruption.
>>>
>>> I think it is better to keep this as strictly that the BSP must match
>>> all APs.  As soon as we encounter a system where this is not the case,
>>> far more areas will also need modifying.
>> I guess you misunderstood - I didn't mean for both lines to be
>> dropped; I meant the if() surrounding the BUG_ON() to go away.
> 
> I don't mind dropping the if(), but I was querying your suggestion in 
> brackets.

Oh, I see. For that one, if all code consistently uses cpu_has_*,
then I can't see any issue (not even a theoretical one).

Jan