From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/xsave: initialization improvements Date: Thu, 29 Aug 2013 11:17:19 +0100 Message-ID: <521F1FAF.4040301@citrix.com> References: <521F32E402000078000EF56B@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4570089808788384999==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VEzKT-0006aP-TS for xen-devel@lists.xenproject.org; Thu, 29 Aug 2013 10:19:55 +0000 In-Reply-To: <521F32E402000078000EF56B@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser , Donald D Dugger , Jun Nakajima List-Id: xen-devel@lists.xenproject.org --===============4570089808788384999== Content-Type: multipart/alternative; boundary="------------020105030407090307080703" --------------020105030407090307080703 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 29/08/13 10:39, Jan Beulich wrote: > - properly validate available feature set on APs > - also validate xsaveopt availability on APs > - properly indicate whether the initialization is on the BSP (we > shouldn't be using "cpu == 0" checks for this) > > Signed-off-by: Jan Beulich Why is "cpu == 0" insufficient? The pcpu with id 0 is necessarily the BSP because of set_processor_id(0) early in __start_xen(). ~Andrew > > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -304,7 +304,7 @@ void __cpuinit identify_cpu(struct cpuin > clear_bit(X86_FEATURE_XSAVE, boot_cpu_data.x86_capability); > > if ( cpu_has_xsave ) > - xstate_init(); > + xstate_init(c == &boot_cpu_data); > > /* > * The vendor-specific functions might have changed features. Now > --- a/xen/arch/x86/xstate.c > +++ b/xen/arch/x86/xstate.c > @@ -251,11 +251,10 @@ void xstate_free_save_area(struct vcpu * > } > > /* Collect the information of processor's extended state */ > -void xstate_init(void) > +void xstate_init(bool_t bsp) > { > - u32 eax, ebx, ecx, edx; > - int cpu = smp_processor_id(); > - u32 min_size; > + u32 eax, ebx, ecx, edx, min_size; > + u64 feature_mask; > > if ( boot_cpu_data.cpuid_level < XSTATE_CPUID ) > return; > @@ -264,6 +263,7 @@ void xstate_init(void) > > BUG_ON((eax & XSTATE_FP_SSE) != XSTATE_FP_SSE); > BUG_ON((eax & XSTATE_YMM) && !(eax & XSTATE_SSE)); > + feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK; > > /* FP/SSE, XSAVE.HEADER, YMM */ > min_size = XSTATE_AREA_MIN_SIZE; > @@ -275,31 +275,33 @@ void xstate_init(void) > * Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size. > */ > set_in_cr4(X86_CR4_OSXSAVE); > - if ( !set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK) ) > + if ( !set_xcr0(feature_mask) ) > BUG(); > cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx); > > - if ( cpu == 0 ) > + if ( bsp ) > { > + xfeature_mask = feature_mask; > /* > * xsave_cntxt_size is the max size required by enabled features. > * We know FP/SSE and YMM about eax, and nothing about edx at present. > */ > xsave_cntxt_size = ebx; > - xfeature_mask = eax + ((u64)edx << 32); > - xfeature_mask &= XCNTXT_MASK; > printk("%s: using cntxt_size: %#x and states: %#"PRIx64"\n", > __func__, xsave_cntxt_size, xfeature_mask); > - > - /* Check XSAVEOPT feature. */ > - cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx); > - cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT); > } > else > { > + BUG_ON(xfeature_mask != feature_mask); > BUG_ON(xsave_cntxt_size != ebx); > - BUG_ON(xfeature_mask != (xfeature_mask & XCNTXT_MASK)); > } > + > + /* Check XSAVEOPT feature. */ > + cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx); > + if ( bsp ) > + cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT); > + else > + BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT)); > } > > int handle_xsetbv(u32 index, u64 new_bv) > --- a/xen/include/asm-x86/xstate.h > +++ b/xen/include/asm-x86/xstate.h > @@ -82,6 +82,6 @@ int __must_check handle_xsetbv(u32 index > /* 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(void); > +void xstate_init(bool_t bsp); > > #endif /* __ASM_XSTATE_H */ > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------020105030407090307080703 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 29/08/13 10:39, Jan Beulich wrote:
- properly validate available feature set on APs
- also validate xsaveopt availability on APs
- properly indicate whether the initialization is on the BSP (we
  shouldn't be using "cpu == 0" checks for this)

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

Why is "cpu == 0" insufficient?

The pcpu with id 0 is necessarily the BSP because of set_processor_id(0) early in __start_xen().

~Andrew


--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -304,7 +304,7 @@ void __cpuinit identify_cpu(struct cpuin
 		clear_bit(X86_FEATURE_XSAVE, boot_cpu_data.x86_capability);
 
 	if ( cpu_has_xsave )
-		xstate_init();
+		xstate_init(c == &boot_cpu_data);
 
 	/*
 	 * The vendor-specific functions might have changed features.  Now
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -251,11 +251,10 @@ void xstate_free_save_area(struct vcpu *
 }
 
 /* Collect the information of processor's extended state */
-void xstate_init(void)
+void xstate_init(bool_t bsp)
 {
-    u32 eax, ebx, ecx, edx;
-    int cpu = smp_processor_id();
-    u32 min_size;
+    u32 eax, ebx, ecx, edx, min_size;
+    u64 feature_mask;
 
     if ( boot_cpu_data.cpuid_level < XSTATE_CPUID )
         return;
@@ -264,6 +263,7 @@ void xstate_init(void)
 
     BUG_ON((eax & XSTATE_FP_SSE) != XSTATE_FP_SSE);
     BUG_ON((eax & XSTATE_YMM) && !(eax & XSTATE_SSE));
+    feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK;
 
     /* FP/SSE, XSAVE.HEADER, YMM */
     min_size =  XSTATE_AREA_MIN_SIZE;
@@ -275,31 +275,33 @@ void xstate_init(void)
      * Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
      */
     set_in_cr4(X86_CR4_OSXSAVE);
-    if ( !set_xcr0((((u64)edx << 32) | eax) & XCNTXT_MASK) )
+    if ( !set_xcr0(feature_mask) )
         BUG();
     cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 
-    if ( cpu == 0 )
+    if ( bsp )
     {
+        xfeature_mask = feature_mask;
         /*
          * xsave_cntxt_size is the max size required by enabled features.
          * We know FP/SSE and YMM about eax, and nothing about edx at present.
          */
         xsave_cntxt_size = ebx;
-        xfeature_mask = eax + ((u64)edx << 32);
-        xfeature_mask &= XCNTXT_MASK;
         printk("%s: using cntxt_size: %#x and states: %#"PRIx64"\n",
             __func__, xsave_cntxt_size, xfeature_mask);
-
-        /* Check XSAVEOPT feature. */
-        cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
-        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
     }
     else
     {
+        BUG_ON(xfeature_mask != feature_mask);
         BUG_ON(xsave_cntxt_size != ebx);
-        BUG_ON(xfeature_mask != (xfeature_mask & XCNTXT_MASK));
     }
+
+    /* Check XSAVEOPT feature. */
+    cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
+    if ( bsp )
+        cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
+    else
+        BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
 }
 
 int handle_xsetbv(u32 index, u64 new_bv)
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -82,6 +82,6 @@ int __must_check handle_xsetbv(u32 index
 /* 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(void);
+void xstate_init(bool_t bsp);
 
 #endif /* __ASM_XSTATE_H */






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

--------------020105030407090307080703-- --===============4570089808788384999== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4570089808788384999==--