- 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>
--- 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
On 29/08/2013 10:39, "Jan Beulich" <JBeulich@suse.com> 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>Acked-by: Keir Fraser <keir@xen.org>> --- 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 */ > > >
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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 29.08.13 at 12:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > 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().But eventually we will want/need to be able to hot-remove CPU 0, and then some other CPU may later come back up getting set its ID to zero. That shouldn''t trigger one-time init code paths. I know this is not the only place, but as I come across them and touch respective code anyway, I''m trying to fix those instances. Jan
On 29/08/13 11:49, Jan Beulich wrote:>>>> On 29.08.13 at 12:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> 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(). > But eventually we will want/need to be able to hot-remove > CPU 0, and then some other CPU may later come back up > getting set its ID to zero. That shouldn''t trigger one-time init > code paths. > > I know this is not the only place, but as I come across them and > touch respective code anyway, I''m trying to fix those instances. > > Jan >Are we expecting that to actually work on real hardware? One example is the BSP NMI routing which will only go to the CPU which the BIOS booted as the BSP. ~Andrew
>>> On 29.08.13 at 13:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 29/08/13 11:49, Jan Beulich wrote: >> But eventually we will want/need to be able to hot-remove >> CPU 0, and then some other CPU may later come back up >> getting set its ID to zero. That shouldn''t trigger one-time init >> code paths. >> >> I know this is not the only place, but as I come across them and >> touch respective code anyway, I''m trying to fix those instances. > > Are we expecting that to actually work on real hardware? > > One example is the BSP NMI routing which will only go to the CPU which > the BIOS booted as the BSP.If you look at recent Linux, they at least give the impression that they are able to do this. Jan