* [PATCH] x86/xsave: initialization improvements
@ 2013-08-29 9:39 Jan Beulich
2013-08-29 10:13 ` Keir Fraser
2013-08-29 10:17 ` Andrew Cooper
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2013-08-29 9:39 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser, Jun Nakajima, Donald D Dugger
[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]
- 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 */
[-- Attachment #2: x86-xsave-init.patch --]
[-- Type: text/plain, Size: 3341 bytes --]
x86/xsave: initialization improvements
- 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 */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/xsave: initialization improvements
2013-08-29 9:39 [PATCH] x86/xsave: initialization improvements Jan Beulich
@ 2013-08-29 10:13 ` Keir Fraser
2013-08-29 10:17 ` Andrew Cooper
1 sibling, 0 replies; 6+ messages in thread
From: Keir Fraser @ 2013-08-29 10:13 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Jun Nakajima, Donald D Dugger
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 */
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/xsave: initialization improvements
2013-08-29 9:39 [PATCH] x86/xsave: initialization improvements Jan Beulich
2013-08-29 10:13 ` Keir Fraser
@ 2013-08-29 10:17 ` Andrew Cooper
2013-08-29 10:49 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2013-08-29 10:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Donald D Dugger, Jun Nakajima
[-- Attachment #1.1: Type: text/plain, Size: 3717 bytes --]
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
[-- Attachment #1.2: Type: text/html, Size: 4523 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/xsave: initialization improvements
2013-08-29 10:17 ` Andrew Cooper
@ 2013-08-29 10:49 ` Jan Beulich
2013-08-29 11:25 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2013-08-29 10:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Jun Nakajima, Donald D Dugger
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/xsave: initialization improvements
2013-08-29 10:49 ` Jan Beulich
@ 2013-08-29 11:25 ` Andrew Cooper
2013-08-29 12:26 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2013-08-29 11:25 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Jun Nakajima, Donald D Dugger
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/xsave: initialization improvements
2013-08-29 11:25 ` Andrew Cooper
@ 2013-08-29 12:26 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2013-08-29 12:26 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, KeirFraser, Jun Nakajima, Donald D Dugger
>>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-29 12:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-29 9:39 [PATCH] x86/xsave: initialization improvements Jan Beulich
2013-08-29 10:13 ` Keir Fraser
2013-08-29 10:17 ` Andrew Cooper
2013-08-29 10:49 ` Jan Beulich
2013-08-29 11:25 ` Andrew Cooper
2013-08-29 12:26 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.