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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.09.15 at 13:40, <andrew.cooper3@xxxxxxxxxx> 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



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