[PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[Robert Richter]

Hans Peter, Suresh, and Cyrill, thanks for your feedback.

This is version 2 of the patch series.

Patches 2, 4, 7 are unchanged compared to -v1.

Patch 1 folds the initialization code into one patch. fpu_init() is
now called before xsave_init(). xsave_init() can later overwrite
xstate_size based on xstate features.

Patch 3 replaces all hardcoded XSTATE_CPUID values know. cpuid_level
is now taken from boot_cpu_data.

Patches 5 and 6 are new:
* add __init attribute to setup_xstate_features()
* disable xsave in i387 emulation mode

-Robert

[PATCH 1/7] x86, xsave: separate fpu and xsave initialization

[Robert Richter]

As xsave also supports other than fpu features, it should be
initialized independently of the fpu. This patch moves this out of fpu
initialization.

There is also a lot of cross referencing between fpu and xsave
code. This patch reduces this by making xsave_cntxt_init() and
init_thread_xstate() static functions.

The patch moves the cpu_has_xsave check at the beginning of
xsave_init(). All other checks may removed then.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/i387.h  |    1 -
 arch/x86/include/asm/xsave.h |    1 -
 arch/x86/kernel/cpu/common.c |    2 ++
 arch/x86/kernel/i387.c       |   27 +++++++++++++++++++--------
 arch/x86/kernel/xsave.c      |   10 +++++-----
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index 59bd93a..509ddab 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -31,7 +31,6 @@ extern void mxcsr_feature_mask_init(void);
 extern int init_fpu(struct task_struct *child);
 extern asmlinkage void math_state_restore(void);
 extern void __math_state_restore(void);
-extern void init_thread_xstate(void);
 extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
 
 extern user_regset_active_fn fpregs_active, xfpregs_active;
diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 94d5f84..4d3b5d1 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -28,7 +28,6 @@ extern u64 pcntxt_mask;
 extern struct xsave_struct *init_xstate_buf;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
-extern void xsave_cntxt_init(void);
 extern void xsave_init(void);
 extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 extern int init_fpu(struct task_struct *child);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4056108..94c36c7 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1210,6 +1210,7 @@ void __cpuinit cpu_init(void)
 	dbg_restore_debug_regs();
 
 	fpu_init();
+	xsave_init();
 
 	raw_local_save_flags(kernel_eflags);
 
@@ -1270,6 +1271,7 @@ void __cpuinit cpu_init(void)
 	clear_used_math();
 	mxcsr_feature_mask_init();
 
+	fpu_init();
 	xsave_init();
 }
 #endif
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 2f32ef0..e73c54e 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -59,18 +59,18 @@ void __cpuinit mxcsr_feature_mask_init(void)
 	stts();
 }
 
-void __cpuinit init_thread_xstate(void)
+static void __cpuinit init_thread_xstate(void)
 {
+	/*
+	 * Note that xstate_size might be overwriten later during
+	 * xsave_init().
+	 */
+
 	if (!HAVE_HWFP) {
 		xstate_size = sizeof(struct i387_soft_struct);
 		return;
 	}
 
-	if (cpu_has_xsave) {
-		xsave_cntxt_init();
-		return;
-	}
-
 	if (cpu_has_fxsr)
 		xstate_size = sizeof(struct i387_fxsave_struct);
 #ifdef CONFIG_X86_32
@@ -84,6 +84,7 @@ void __cpuinit init_thread_xstate(void)
  * Called at bootup to set up the initial FPU state that is later cloned
  * into all processes.
  */
+
 void __cpuinit fpu_init(void)
 {
 	unsigned long oldcr0 = read_cr0();
@@ -93,14 +94,24 @@ void __cpuinit fpu_init(void)
 
 	write_cr0(oldcr0 & ~(X86_CR0_TS|X86_CR0_EM)); /* clear TS and EM */
 
-	xsave_init();
+	if (!smp_processor_id())
+		init_thread_xstate();
 
 	mxcsr_feature_mask_init();
 	/* clean state in init */
 	current_thread_info()->status = 0;
 	clear_used_math();
 }
-#endif	/* CONFIG_X86_64 */
+
+#else	/* CONFIG_X86_64 */
+
+void __cpuinit fpu_init(void)
+{
+	if (!smp_processor_id())
+		init_thread_xstate();
+}
+
+#endif	/* CONFIG_X86_32 */
 
 static void fpu_finit(struct fpu *fpu)
 {
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index ab9ad48..550bf45 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -362,9 +362,6 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
  */
 static void __cpuinit __xsave_init(void)
 {
-	if (!cpu_has_xsave)
-		return;
-
 	set_in_cr4(X86_CR4_OSXSAVE);
 
 	/*
@@ -429,7 +426,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-void __ref xsave_cntxt_init(void)
+static void __cpuinit xsave_cntxt_init(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -466,10 +463,13 @@ void __ref xsave_cntxt_init(void)
 
 void __cpuinit xsave_init(void)
 {
+	if (!cpu_has_xsave)
+		return;
+
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
 	if (!smp_processor_id())
-		init_thread_xstate();
+		xsave_cntxt_init();
 	__xsave_init();
 }
-- 
1.7.1.1

[tip:x86/xsave] x86, xsave: Separate fpu and xsave initialization

[tip-bot for Robert Richter]

Commit-ID:  0e49bf66d2ca649b167428adddbbbe9d9bd4894c
Gitweb:     http://git.kernel.org/tip/0e49bf66d2ca649b167428adddbbbe9d9bd4894c
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:52 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 14:06:04 -0700

x86, xsave: Separate fpu and xsave initialization

As xsave also supports other than fpu features, it should be
initialized independently of the fpu. This patch moves this out of fpu
initialization.

There is also a lot of cross referencing between fpu and xsave
code. This patch reduces this by making xsave_cntxt_init() and
init_thread_xstate() static functions.

The patch moves the cpu_has_xsave check at the beginning of
xsave_init(). All other checks may removed then.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-2-git-send-email-robert.richter@amd.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/i387.h  |    1 -
 arch/x86/include/asm/xsave.h |    1 -
 arch/x86/kernel/cpu/common.c |    2 ++
 arch/x86/kernel/i387.c       |   27 +++++++++++++++++++--------
 arch/x86/kernel/xsave.c      |   10 +++++-----
 5 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index 59bd93a..509ddab 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -31,7 +31,6 @@ extern void mxcsr_feature_mask_init(void);
 extern int init_fpu(struct task_struct *child);
 extern asmlinkage void math_state_restore(void);
 extern void __math_state_restore(void);
-extern void init_thread_xstate(void);
 extern int dump_fpu(struct pt_regs *, struct user_i387_struct *);
 
 extern user_regset_active_fn fpregs_active, xfpregs_active;
diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 94d5f84..4d3b5d1 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -28,7 +28,6 @@ extern u64 pcntxt_mask;
 extern struct xsave_struct *init_xstate_buf;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
-extern void xsave_cntxt_init(void);
 extern void xsave_init(void);
 extern void update_regset_xstate_info(unsigned int size, u64 xstate_mask);
 extern int init_fpu(struct task_struct *child);
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4056108..94c36c7 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1210,6 +1210,7 @@ void __cpuinit cpu_init(void)
 	dbg_restore_debug_regs();
 
 	fpu_init();
+	xsave_init();
 
 	raw_local_save_flags(kernel_eflags);
 
@@ -1270,6 +1271,7 @@ void __cpuinit cpu_init(void)
 	clear_used_math();
 	mxcsr_feature_mask_init();
 
+	fpu_init();
 	xsave_init();
 }
 #endif
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 2f32ef0..e73c54e 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -59,18 +59,18 @@ void __cpuinit mxcsr_feature_mask_init(void)
 	stts();
 }
 
-void __cpuinit init_thread_xstate(void)
+static void __cpuinit init_thread_xstate(void)
 {
+	/*
+	 * Note that xstate_size might be overwriten later during
+	 * xsave_init().
+	 */
+
 	if (!HAVE_HWFP) {
 		xstate_size = sizeof(struct i387_soft_struct);
 		return;
 	}
 
-	if (cpu_has_xsave) {
-		xsave_cntxt_init();
-		return;
-	}
-
 	if (cpu_has_fxsr)
 		xstate_size = sizeof(struct i387_fxsave_struct);
 #ifdef CONFIG_X86_32
@@ -84,6 +84,7 @@ void __cpuinit init_thread_xstate(void)
  * Called at bootup to set up the initial FPU state that is later cloned
  * into all processes.
  */
+
 void __cpuinit fpu_init(void)
 {
 	unsigned long oldcr0 = read_cr0();
@@ -93,14 +94,24 @@ void __cpuinit fpu_init(void)
 
 	write_cr0(oldcr0 & ~(X86_CR0_TS|X86_CR0_EM)); /* clear TS and EM */
 
-	xsave_init();
+	if (!smp_processor_id())
+		init_thread_xstate();
 
 	mxcsr_feature_mask_init();
 	/* clean state in init */
 	current_thread_info()->status = 0;
 	clear_used_math();
 }
-#endif	/* CONFIG_X86_64 */
+
+#else	/* CONFIG_X86_64 */
+
+void __cpuinit fpu_init(void)
+{
+	if (!smp_processor_id())
+		init_thread_xstate();
+}
+
+#endif	/* CONFIG_X86_32 */
 
 static void fpu_finit(struct fpu *fpu)
 {
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index ab9ad48..550bf45 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -362,9 +362,6 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
  */
 static void __cpuinit __xsave_init(void)
 {
-	if (!cpu_has_xsave)
-		return;
-
 	set_in_cr4(X86_CR4_OSXSAVE);
 
 	/*
@@ -429,7 +426,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-void __ref xsave_cntxt_init(void)
+static void __cpuinit xsave_cntxt_init(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -466,10 +463,13 @@ void __ref xsave_cntxt_init(void)
 
 void __cpuinit xsave_init(void)
 {
+	if (!cpu_has_xsave)
+		return;
+
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
 	if (!smp_processor_id())
-		init_thread_xstate();
+		xsave_cntxt_init();
 	__xsave_init();
 }

[PATCH 2/7] x86, xsave: introduce xstate enable functions

[Robert Richter]

The patch renames xsave_cntxt_init() and __xsave_init() into
xstate_enable_boot_cpu() and xstate_enable() as this names are more
meaningful.

It also removes the duplicate xcr setup for the boot cpu.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/xsave.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 550bf45..2322f58 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,15 +360,10 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-static void __cpuinit __xsave_init(void)
+static inline void xstate_enable(u64 mask)
 {
 	set_in_cr4(X86_CR4_OSXSAVE);
-
-	/*
-	 * Enable all the features that the HW is capable of
-	 * and the Linux kernel is aware of.
-	 */
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, pcntxt_mask);
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, mask);
 }
 
 /*
@@ -426,7 +421,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-static void __cpuinit xsave_cntxt_init(void)
+static void __cpuinit xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -443,7 +438,8 @@ static void __cpuinit xsave_cntxt_init(void)
 	 * Support only the state known to OS.
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
-	__xsave_init();
+
+	xstate_enable(pcntxt_mask);
 
 	/*
 	 * Recompute the context size for enabled features
@@ -470,6 +466,7 @@ void __cpuinit xsave_init(void)
 	 * Boot processor to setup the FP and extended state context info.
 	 */
 	if (!smp_processor_id())
-		xsave_cntxt_init();
-	__xsave_init();
+		xstate_enable_boot_cpu();
+	else
+		xstate_enable(pcntxt_mask);
 }
-- 
1.7.1.1

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[H. Peter Anvin]

On 07/21/2010 10:03 AM, Robert Richter wrote:
> The patch renames xsave_cntxt_init() and __xsave_init() into
> xstate_enable_boot_cpu() and xstate_enable() as this names are more
> meaningful.
> 
> It also removes the duplicate xcr setup for the boot cpu.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>

> -static void __cpuinit xsave_cntxt_init(void)
> +static void __cpuinit xstate_enable_boot_cpu(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  
> @@ -443,7 +438,8 @@ static void __cpuinit xsave_cntxt_init(void)
>  	 * Support only the state known to OS.
>  	 */
>  	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
> -	__xsave_init();
> +
> +	xstate_enable(pcntxt_mask);
>  
>  	/*
>  	 * Recompute the context size for enabled features

This one should be __init rather than __cpuinit, right?  As written, I get:


WARNING: vmlinux.o(.cpuinit.text+0x824): Section mismatch in reference
from the function xstate_enable_boot_cpu() to the function
.init.text:__alloc_bootmem()
The function __cpuinit xstate_enable_boot_cpu() references
a function __init __alloc_bootmem().
If __alloc_bootmem is only used by xstate_enable_boot_cpu then
annotate __alloc_bootmem with a matching annotation.

[No need to resend the patch, but if either Suresh or Robert could ACK
this change I'd appreciate it.]


	-hpa

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[Suresh Siddha]

On Wed, 2010-07-21 at 14:10 -0700, H. Peter Anvin wrote:
> On 07/21/2010 10:03 AM, Robert Richter wrote:
> > The patch renames xsave_cntxt_init() and __xsave_init() into
> > xstate_enable_boot_cpu() and xstate_enable() as this names are more
> > meaningful.
> > 
> > It also removes the duplicate xcr setup for the boot cpu.
> > 
> > Signed-off-by: Robert Richter <robert.richter@amd.com>
> 
> > -static void __cpuinit xsave_cntxt_init(void)
> > +static void __cpuinit xstate_enable_boot_cpu(void)
> >  {
> >  	unsigned int eax, ebx, ecx, edx;
> >  
> > @@ -443,7 +438,8 @@ static void __cpuinit xsave_cntxt_init(void)
> >  	 * Support only the state known to OS.
> >  	 */
> >  	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
> > -	__xsave_init();
> > +
> > +	xstate_enable(pcntxt_mask);
> >  
> >  	/*
> >  	 * Recompute the context size for enabled features
> 
> This one should be __init rather than __cpuinit, right?  As written, I get:
> 
> 
> WARNING: vmlinux.o(.cpuinit.text+0x824): Section mismatch in reference
> from the function xstate_enable_boot_cpu() to the function
> .init.text:__alloc_bootmem()
> The function __cpuinit xstate_enable_boot_cpu() references
> a function __init __alloc_bootmem().
> If __alloc_bootmem is only used by xstate_enable_boot_cpu then
> annotate __alloc_bootmem with a matching annotation.
> 
> [No need to resend the patch, but if either Suresh or Robert could ACK
> this change I'd appreciate it.]

Yes, it should be __init.

thanks,
suresh

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On 07/21/2010 02:20 PM, Suresh Siddha wrote:
> 
> Yes, it should be __init.
> 

OK, here is the proposed fix for that.  It's a bit uglier than I would
have liked.

It also fixes the assumption that "we are on the boot CPU so we are
early in the boot", which is true now but will not be true in the future
when we can offline (and later re-online) the boot CPU.

Comments appreciated...

	-hpa

[-- Attachment #2: 0001-x86-xsave-Make-xstate_enable_boot_cpu-__init-protect.patch --]
[-- Type: text/x-patch, Size: 2915 bytes --]

>From 55b936c7a359a14d72bcba6c3fceba4cfbe3fedf Mon Sep 17 00:00:00 2001
From: H. Peter Anvin <hpa@linux.intel.com>
Date: Wed, 21 Jul 2010 14:23:10 -0700
Subject: [PATCH] x86, xsave: Make xstate_enable_boot_cpu() __init, protect on CPU 0

xstate_enable_boot_cpu() is, as the name implies, only used on the
boot CPU; furthermore, it invokes alloc_bootmem(), which is __init;
hence it needs to be tagged __init rather than __cpuinit.

Furthermore, it is *not* safe in the long run to rely on CPU 0 only
coming online during the early boot -- at some point we're going to
support offlining (and re-onlining) the boot CPU, and at that point we
must not call xstate_enable_boot_cpu() again.

The code is a fair bit more obscure than one would like, because the
__ref overrides aren't quite powerful enough.

Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Robert Richter <robert.richter@amd.com>
LKML-Reference: <4C476236.1020302@zytor.com>
---
 arch/x86/kernel/xsave.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index cfc7901..b2549c3 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,10 +360,10 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-static inline void xstate_enable(u64 mask)
+static inline void xstate_enable(void)
 {
 	set_in_cr4(X86_CR4_OSXSAVE);
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, mask);
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, pcntxt_mask);
 }
 
 /*
@@ -421,7 +421,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-static void __cpuinit xstate_enable_boot_cpu(void)
+static void __init xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -444,7 +444,7 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
 
-	xstate_enable(pcntxt_mask);
+	xstate_enable();
 
 	/*
 	 * Recompute the context size for enabled features
@@ -462,16 +462,22 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	       pcntxt_mask, xstate_size);
 }
 
+/*
+ * For the very first instance, this calls xstate_enable_boot_cpu();
+ * for all subsequent instances, this calls xstate_enable().
+ *
+ * This is somewhat obfuscated due to the lack of powerful enough
+ * overrides for the section checks.
+ */
 void __cpuinit xsave_init(void)
 {
+	static __refdata void (*next_func)(void) = xstate_enable_boot_cpu;
+	void (*this_func)(void);
+
 	if (!cpu_has_xsave)
 		return;
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		xstate_enable_boot_cpu();
-	else
-		xstate_enable(pcntxt_mask);
+	this_func = next_func;
+	next_func = xstate_enable;
+	this_func();
 }
-- 
1.7.0.1

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[Suresh Siddha]

On Wed, 2010-07-21 at 14:53 -0700, H. Peter Anvin wrote:
> On 07/21/2010 02:20 PM, Suresh Siddha wrote:
> > 
> > Yes, it should be __init.
> > 
> 
> OK, here is the proposed fix for that.  It's a bit uglier than I would
> have liked.
> 
> It also fixes the assumption that "we are on the boot CPU so we are
> early in the boot", which is true now but will not be true in the future
> when we can offline (and later re-online) the boot CPU.
> 
> Comments appreciated...

I am ok with this fix. Thanks for taking a stab at this.

suresh

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[Robert Richter]

On 21.07.10 17:53:31, H. Peter Anvin wrote:

> From 55b936c7a359a14d72bcba6c3fceba4cfbe3fedf Mon Sep 17 00:00:00 2001
> From: H. Peter Anvin <hpa@linux.intel.com>
> Date: Wed, 21 Jul 2010 14:23:10 -0700
> Subject: [PATCH] x86, xsave: Make xstate_enable_boot_cpu() __init, protect on CPU 0
> 
> xstate_enable_boot_cpu() is, as the name implies, only used on the
> boot CPU; furthermore, it invokes alloc_bootmem(), which is __init;
> hence it needs to be tagged __init rather than __cpuinit.
> 
> Furthermore, it is *not* safe in the long run to rely on CPU 0 only
> coming online during the early boot -- at some point we're going to
> support offlining (and re-onlining) the boot CPU, and at that point we
> must not call xstate_enable_boot_cpu() again.
> 
> The code is a fair bit more obscure than one would like, because the
> __ref overrides aren't quite powerful enough.
> 
> Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
> Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Cc: Robert Richter <robert.richter@amd.com>
> LKML-Reference: <4C476236.1020302@zytor.com>

I am fine with your changes.

>  void __cpuinit xsave_init(void)
>  {
> +	static __refdata void (*next_func)(void) = xstate_enable_boot_cpu;
> +	void (*this_func)(void);
> +
>  	if (!cpu_has_xsave)
>  		return;
>  
> -	/*
> -	 * Boot processor to setup the FP and extended state context info.
> -	 */
> -	if (!smp_processor_id())
> -		xstate_enable_boot_cpu();
> -	else
> -		xstate_enable(pcntxt_mask);
> +	this_func = next_func;
> +	next_func = xstate_enable;
> +	this_func();
>  }

Just wondering why you are using this_func()? Instead, you could
simply do:

	next_func();
	next_func = xstate_enable;

Do you see races when bringing up multiple cpus in parallel?

Thanks.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[H. Peter Anvin]

On 07/22/2010 05:15 AM, Robert Richter wrote:
> 
> Just wondering why you are using this_func()? Instead, you could
> simply do:
> 
> 	next_func();
> 	next_func = xstate_enable;
> 
> Do you see races when bringing up multiple cpus in parallel?
> 

It allows the compiler to turn it into a tailcall if frame pointers are
disabled.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 2/7] x86, xsave: introduce xstate enable functions

[Robert Richter]

On 22.07.10 08:23:56, H. Peter Anvin wrote:
> On 07/22/2010 05:15 AM, Robert Richter wrote:
> > 
> > Just wondering why you are using this_func()? Instead, you could
> > simply do:
> > 
> > 	next_func();
> > 	next_func = xstate_enable;
> > 
> > Do you see races when bringing up multiple cpus in parallel?
> > 
> 
> It allows the compiler to turn it into a tailcall if frame pointers are
> disabled.

Yes, that makes sense.

Thanks.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[tip:x86/xsave] x86, xsave: Make xstate_enable_boot_cpu() __init, protect on CPU 0

[tip-bot for H. Peter Anvin]

Commit-ID:  1cff92d8fdb27684308864d9cdb324bee43b40ab
Gitweb:     http://git.kernel.org/tip/1cff92d8fdb27684308864d9cdb324bee43b40ab
Author:     H. Peter Anvin <hpa@linux.intel.com>
AuthorDate: Wed, 21 Jul 2010 14:23:10 -0700
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 15:33:54 -0700

x86, xsave: Make xstate_enable_boot_cpu() __init, protect on CPU 0

xstate_enable_boot_cpu() is, as the name implies, only used on the
boot CPU; furthermore, it invokes alloc_bootmem(), which is __init;
hence it needs to be tagged __init rather than __cpuinit.

Furthermore, it is *not* safe in the long run to rely on CPU 0 only
coming online during the early boot -- at some point we're going to
support offlining (and re-onlining) the boot CPU, and at that point we
must not call xstate_enable_boot_cpu() again.

The code is a fair bit more obscure than one would like, because the
__ref overrides aren't quite powerful enough.

Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Robert Richter <robert.richter@amd.com>
LKML-Reference: <4C476236.1020302@zytor.com>
---
 arch/x86/kernel/xsave.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index cfc7901..b2549c3 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,10 +360,10 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-static inline void xstate_enable(u64 mask)
+static inline void xstate_enable(void)
 {
 	set_in_cr4(X86_CR4_OSXSAVE);
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, mask);
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, pcntxt_mask);
 }
 
 /*
@@ -421,7 +421,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-static void __cpuinit xstate_enable_boot_cpu(void)
+static void __init xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -444,7 +444,7 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
 
-	xstate_enable(pcntxt_mask);
+	xstate_enable();
 
 	/*
 	 * Recompute the context size for enabled features
@@ -462,16 +462,22 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	       pcntxt_mask, xstate_size);
 }
 
+/*
+ * For the very first instance, this calls xstate_enable_boot_cpu();
+ * for all subsequent instances, this calls xstate_enable().
+ *
+ * This is somewhat obfuscated due to the lack of powerful enough
+ * overrides for the section checks.
+ */
 void __cpuinit xsave_init(void)
 {
+	static __refdata void (*next_func)(void) = xstate_enable_boot_cpu;
+	void (*this_func)(void);
+
 	if (!cpu_has_xsave)
 		return;
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		xstate_enable_boot_cpu();
-	else
-		xstate_enable(pcntxt_mask);
+	this_func = next_func;
+	next_func = xstate_enable;
+	this_func();
 }

[tip:x86/xsave] x86, xsave: Introduce xstate enable functions

[tip-bot for Robert Richter]

Commit-ID:  97e80a70db689fb1e876df9f12305cc72f85ca53
Gitweb:     http://git.kernel.org/tip/97e80a70db689fb1e876df9f12305cc72f85ca53
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:53 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 14:06:04 -0700

x86, xsave: Introduce xstate enable functions

The patch renames xsave_cntxt_init() and __xsave_init() into
xstate_enable_boot_cpu() and xstate_enable() as this names are more
meaningful.

It also removes the duplicate xcr setup for the boot cpu.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-3-git-send-email-robert.richter@amd.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/xsave.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 550bf45..2322f58 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,15 +360,10 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-static void __cpuinit __xsave_init(void)
+static inline void xstate_enable(u64 mask)
 {
 	set_in_cr4(X86_CR4_OSXSAVE);
-
-	/*
-	 * Enable all the features that the HW is capable of
-	 * and the Linux kernel is aware of.
-	 */
-	xsetbv(XCR_XFEATURE_ENABLED_MASK, pcntxt_mask);
+	xsetbv(XCR_XFEATURE_ENABLED_MASK, mask);
 }
 
 /*
@@ -426,7 +421,7 @@ static void __init setup_xstate_init(void)
 /*
  * Enable and initialize the xsave feature.
  */
-static void __cpuinit xsave_cntxt_init(void)
+static void __cpuinit xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
@@ -443,7 +438,8 @@ static void __cpuinit xsave_cntxt_init(void)
 	 * Support only the state known to OS.
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
-	__xsave_init();
+
+	xstate_enable(pcntxt_mask);
 
 	/*
 	 * Recompute the context size for enabled features
@@ -470,6 +466,7 @@ void __cpuinit xsave_init(void)
 	 * Boot processor to setup the FP and extended state context info.
 	 */
 	if (!smp_processor_id())
-		xsave_cntxt_init();
-	__xsave_init();
+		xstate_enable_boot_cpu();
+	else
+		xstate_enable(pcntxt_mask);
 }

[PATCH 3/7] x86, xsave: check cpuid level for XSTATE_CPUID (0x0d)

[Robert Richter]

The patch introduces the XSTATE_CPUID macro and adds a check that
tests if XSTATE_CPUID exists.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/xsave.h |    2 ++
 arch/x86/kernel/xsave.c      |   11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 4d3b5d1..d1b5f3a 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -4,6 +4,8 @@
 #include <linux/types.h>
 #include <asm/processor.h>
 
+#define XSTATE_CPUID		0x0000000d
+
 #define XSTATE_FP	0x1
 #define XSTATE_SSE	0x2
 #define XSTATE_YMM	0x4
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 2322f58..5adb7fb 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -379,7 +379,7 @@ static void setup_xstate_features(void)
 	xstate_sizes = alloc_bootmem(xstate_features * sizeof(int));
 
 	do {
-		cpuid_count(0xd, leaf, &eax, &ebx, &ecx, &edx);
+		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);
 
 		if (eax == 0)
 			break;
@@ -425,7 +425,12 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
-	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
+	if (boot_cpu_data.cpuid_level < XSTATE_CPUID) {
+		WARN(1, KERN_ERR "XSTATE_CPUID missing\n");
+		return;
+	}
+
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 	pcntxt_mask = eax + ((u64)edx << 32);
 
 	if ((pcntxt_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
@@ -444,7 +449,7 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	/*
 	 * Recompute the context size for enabled features
 	 */
-	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 	xstate_size = ebx;
 
 	update_regset_xstate_info(xstate_size, pcntxt_mask);
-- 
1.7.1.1

[tip:x86/xsave] x86, xsave: Check cpuid level for XSTATE_CPUID (0x0d)

[tip-bot for Robert Richter]

Commit-ID:  ee813d53a8e980a3a28318efb8935d45723f5211
Gitweb:     http://git.kernel.org/tip/ee813d53a8e980a3a28318efb8935d45723f5211
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:54 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 14:06:04 -0700

x86, xsave: Check cpuid level for XSTATE_CPUID (0x0d)

The patch introduces the XSTATE_CPUID macro and adds a check that
tests if XSTATE_CPUID exists.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-4-git-send-email-robert.richter@amd.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/xsave.h |    2 ++
 arch/x86/kernel/xsave.c      |   11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index 4d3b5d1..d1b5f3a 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -4,6 +4,8 @@
 #include <linux/types.h>
 #include <asm/processor.h>
 
+#define XSTATE_CPUID		0x0000000d
+
 #define XSTATE_FP	0x1
 #define XSTATE_SSE	0x2
 #define XSTATE_YMM	0x4
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 2322f58..5adb7fb 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -379,7 +379,7 @@ static void setup_xstate_features(void)
 	xstate_sizes = alloc_bootmem(xstate_features * sizeof(int));
 
 	do {
-		cpuid_count(0xd, leaf, &eax, &ebx, &ecx, &edx);
+		cpuid_count(XSTATE_CPUID, leaf, &eax, &ebx, &ecx, &edx);
 
 		if (eax == 0)
 			break;
@@ -425,7 +425,12 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
-	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
+	if (boot_cpu_data.cpuid_level < XSTATE_CPUID) {
+		WARN(1, KERN_ERR "XSTATE_CPUID missing\n");
+		return;
+	}
+
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 	pcntxt_mask = eax + ((u64)edx << 32);
 
 	if ((pcntxt_mask & XSTATE_FPSSE) != XSTATE_FPSSE) {
@@ -444,7 +449,7 @@ static void __cpuinit xstate_enable_boot_cpu(void)
 	/*
 	 * Recompute the context size for enabled features
 	 */
-	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
+	cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
 	xstate_size = ebx;
 
 	update_regset_xstate_info(xstate_size, pcntxt_mask);

[PATCH 4/7] x86, xsave: make init_xstate_buf static

[Robert Richter]

The pointer is only used in xsave.c. Making it static.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/xsave.h |    1 -
 arch/x86/kernel/xsave.c      |   10 +++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index d1b5f3a..0ae6b99 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -27,7 +27,6 @@
 
 extern unsigned int xstate_size;
 extern u64 pcntxt_mask;
-extern struct xsave_struct *init_xstate_buf;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
 extern void xsave_init(void);
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 5adb7fb..3b44a9b 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -16,6 +16,11 @@
  */
 u64 pcntxt_mask;
 
+/*
+ * Represents init state for the supported extended state.
+ */
+static struct xsave_struct *init_xstate_buf;
+
 struct _fpx_sw_bytes fx_sw_reserved;
 #ifdef CONFIG_IA32_EMULATION
 struct _fpx_sw_bytes fx_sw_reserved_ia32;
@@ -348,11 +353,6 @@ static void prepare_fx_sw_frame(void)
 #endif
 }
 
-/*
- * Represents init state for the supported extended state.
- */
-struct xsave_struct *init_xstate_buf;
-
 #ifdef CONFIG_X86_64
 unsigned int sig_xstate_size = sizeof(struct _fpstate);
 #endif
-- 
1.7.1.1

[tip:x86/xsave] x86, xsave: Make init_xstate_buf static

[tip-bot for Robert Richter]

Commit-ID:  45c2d7f46211a0b1f6b425c59575c53145afc4b4
Gitweb:     http://git.kernel.org/tip/45c2d7f46211a0b1f6b425c59575c53145afc4b4
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:55 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 14:06:05 -0700

x86, xsave: Make init_xstate_buf static

The pointer is only used in xsave.c. Making it static.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-5-git-send-email-robert.richter@amd.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/xsave.h |    1 -
 arch/x86/kernel/xsave.c      |   10 +++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index d1b5f3a..0ae6b99 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -27,7 +27,6 @@
 
 extern unsigned int xstate_size;
 extern u64 pcntxt_mask;
-extern struct xsave_struct *init_xstate_buf;
 extern u64 xstate_fx_sw_bytes[USER_XSTATE_FX_SW_WORDS];
 
 extern void xsave_init(void);
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 5adb7fb..3b44a9b 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -16,6 +16,11 @@
  */
 u64 pcntxt_mask;
 
+/*
+ * Represents init state for the supported extended state.
+ */
+static struct xsave_struct *init_xstate_buf;
+
 struct _fpx_sw_bytes fx_sw_reserved;
 #ifdef CONFIG_IA32_EMULATION
 struct _fpx_sw_bytes fx_sw_reserved_ia32;
@@ -348,11 +353,6 @@ static void prepare_fx_sw_frame(void)
 #endif
 }
 
-/*
- * Represents init state for the supported extended state.
- */
-struct xsave_struct *init_xstate_buf;
-
 #ifdef CONFIG_X86_64
 unsigned int sig_xstate_size = sizeof(struct _fpstate);
 #endif

[PATCH 5/7] x86, xsave: add __init attribute to setup_xstate_features()

[Robert Richter]

This is called only from initialization code.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/xsave.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 3b44a9b..cfc7901 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -370,7 +370,7 @@ static inline void xstate_enable(u64 mask)
  * Record the offsets and sizes of different state managed by the xsave
  * memory layout.
  */
-static void setup_xstate_features(void)
+static void __init setup_xstate_features(void)
 {
 	int eax, ebx, ecx, edx, leaf = 0x2;
 
-- 
1.7.1.1

[tip:x86/xsave] x86, xsave: Add __init attribute to setup_xstate_features()

[tip-bot for Robert Richter]

Commit-ID:  4995b9dba908436c1611454f9bd2cb3ddf6babee
Gitweb:     http://git.kernel.org/tip/4995b9dba908436c1611454f9bd2cb3ddf6babee
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:56 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed, 21 Jul 2010 14:06:05 -0700

x86, xsave: Add __init attribute to setup_xstate_features()

This is called only from initialization code.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-6-git-send-email-robert.richter@amd.com>
Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/xsave.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 3b44a9b..cfc7901 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -370,7 +370,7 @@ static inline void xstate_enable(u64 mask)
  * Record the offsets and sizes of different state managed by the xsave
  * memory layout.
  */
-static void setup_xstate_features(void)
+static void __init setup_xstate_features(void)
 {
 	int eax, ebx, ecx, edx, leaf = 0x2;
 

[PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Robert Richter]

xsave is broken for (!HAVE_HWFP). This is the case if config
MATH_EMULATION is enabled, 'no387' kernel parameter is set and xsave
exists. xsave will not work because x86/math-emu and xsave share the
same memory. As this case can be treated as corner case we simply
disable xsave then.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/i387.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index e73c54e..ff81143 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -67,6 +67,12 @@ static void __cpuinit init_thread_xstate(void)
 	 */
 
 	if (!HAVE_HWFP) {
+		/*
+		 * Disable xsave as we do not support it if i387
+		 * emulation is enabled.
+		 */
+		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
+		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
 		xstate_size = sizeof(struct i387_soft_struct);
 		return;
 	}
-- 
1.7.1.1

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Suresh Siddha]

On Wed, 2010-07-21 at 10:03 -0700, Robert Richter wrote:
> xsave is broken for (!HAVE_HWFP). This is the case if config
> MATH_EMULATION is enabled, 'no387' kernel parameter is set and xsave
> exists. xsave will not work because x86/math-emu and xsave share the
> same memory. As this case can be treated as corner case we simply
> disable xsave then.

I think it is cleaner to clear these cpu capabilities in the function
which handles no387 boot parameter.

Otherwise Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

thanks.

> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/kernel/i387.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index e73c54e..ff81143 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -67,6 +67,12 @@ static void __cpuinit init_thread_xstate(void)
>  	 */
>  
>  	if (!HAVE_HWFP) {
> +		/*
> +		 * Disable xsave as we do not support it if i387
> +		 * emulation is enabled.
> +		 */
> +		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
> +		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
>  		xstate_size = sizeof(struct i387_soft_struct);
>  		return;
>  	}

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Robert Richter]

On 21.07.10 14:16:02, Suresh Siddha wrote:

> I think it is cleaner to clear these cpu capabilities in the function
> which handles no387 boot parameter.

This does not cover all (of course weird but potentially existing)
cases. Disabling xsave in the no387 setup would only work if an fpu
exists. The implementation below disables it if the soft fpu is actual
used. An artificial condition that would break your approach would be
no fpu but xsave. There is no hardware like this but maybe virtual
machines configurations.

So I think it does not hurt to deactivate xsave directly when enabling
soft fpu. The only drawback here is if fpu and xsave initialization
order changes. Hmm...

-Robert

> 
> Otherwise Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> 
> thanks.
> 
> > 
> > Signed-off-by: Robert Richter <robert.richter@amd.com>
> > ---
> >  arch/x86/kernel/i387.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> > index e73c54e..ff81143 100644
> > --- a/arch/x86/kernel/i387.c
> > +++ b/arch/x86/kernel/i387.c
> > @@ -67,6 +67,12 @@ static void __cpuinit init_thread_xstate(void)
> >  	 */
> >  
> >  	if (!HAVE_HWFP) {
> > +		/*
> > +		 * Disable xsave as we do not support it if i387
> > +		 * emulation is enabled.
> > +		 */
> > +		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
> > +		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
> >  		xstate_size = sizeof(struct i387_soft_struct);
> >  		return;
> >  	}
> 
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Suresh Siddha]

On Thu, 2010-07-22 at 05:36 -0700, Robert Richter wrote:
> On 21.07.10 14:16:02, Suresh Siddha wrote:
> 
> > I think it is cleaner to clear these cpu capabilities in the function
> > which handles no387 boot parameter.
> 
> This does not cover all (of course weird but potentially existing)
> cases. Disabling xsave in the no387 setup would only work if an fpu
> exists. The implementation below disables it if the soft fpu is actual
> used. An artificial condition that would break your approach would be
> no fpu but xsave. There is no hardware like this but maybe virtual
> machines configurations.
> 
> So I think it does not hurt to deactivate xsave directly when enabling
> soft fpu. The only drawback here is if fpu and xsave initialization
> order changes. Hmm...

Then the more appropriate place for this is to check at the beginning of
the xsave init code.

thanks,
suresh

> 
> -Robert
> 
> > 
> > Otherwise Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > 
> > thanks.
> > 
> > > 
> > > Signed-off-by: Robert Richter <robert.richter@amd.com>
> > > ---
> > >  arch/x86/kernel/i387.c |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> > > index e73c54e..ff81143 100644
> > > --- a/arch/x86/kernel/i387.c
> > > +++ b/arch/x86/kernel/i387.c
> > > @@ -67,6 +67,12 @@ static void __cpuinit init_thread_xstate(void)
> > >  	 */
> > >  
> > >  	if (!HAVE_HWFP) {
> > > +		/*
> > > +		 * Disable xsave as we do not support it if i387
> > > +		 * emulation is enabled.
> > > +		 */
> > > +		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
> > > +		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
> > >  		xstate_size = sizeof(struct i387_soft_struct);
> > >  		return;
> > >  	}
> > 
> > 
> 

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Robert Richter]

On 23.07.10 13:50:05, Suresh Siddha wrote:

> Then the more appropriate place for this is to check at the beginning of
> the xsave init code.

No it isn't, HAVE_HWFP is an fpu implementation and the soft fpu check
would be duplicate in xsave.c. Also, the HAVE_HWFP macro is local in
i387.c and would have to be exported in a header file then, which
somehow taints the api.

So I rather tend to this patch than implementing fpu code in xsave for
a small or no benefit.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[H. Peter Anvin]

On 07/22/2010 05:36 AM, Robert Richter wrote:
> On 21.07.10 14:16:02, Suresh Siddha wrote:
> 
>> I think it is cleaner to clear these cpu capabilities in the function
>> which handles no387 boot parameter.
> 
> This does not cover all (of course weird but potentially existing)
> cases. Disabling xsave in the no387 setup would only work if an fpu
> exists.

If the fpu doesn't exist, then xsave can't exist, either.

	-hpa

Re: [PATCH 6/7] x86, xsave: disable xsave in i387 emulation mode

[Robert Richter]

On 26.07.10 14:26:36, H. Peter Anvin wrote:

> >> I think it is cleaner to clear these cpu capabilities in the function
> >> which handles no387 boot parameter.
> > 
> > This does not cover all (of course weird but potentially existing)
> > cases. Disabling xsave in the no387 setup would only work if an fpu
> > exists.
> 
> If the fpu doesn't exist, then xsave can't exist, either.

It does not convince me to move it either to no387 setup or
xsave_init(). There is no benefit with both of it. We should disable
it where we decide to use the soft fpu. Otherwise we will not have the
same underlying rules for it. In case of xsave_init() we need to
duplicate fpu code and export it to xsave code. I want to avoid this
as xsave and fpu code should be kept simple and only be shared where
necessary.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center

[tip:x86/fpu] x86, xsave: Disable xsave in i387 emulation mode

[tip-bot for Robert Richter]

Commit-ID:  1f999ab5a1360afc388868cc0ef9afe8edeef3be
Gitweb:     http://git.kernel.org/tip/1f999ab5a1360afc388868cc0ef9afe8edeef3be
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:57 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Thu, 12 Aug 2010 14:16:54 -0700

x86, xsave: Disable xsave in i387 emulation mode

xsave is broken for (!HAVE_HWFP). This is the case if config
MATH_EMULATION is enabled, 'no387' kernel parameter is set and xsave
exists. xsave will not work because x86/math-emu and xsave share the
same memory. As this case can be treated as corner case we simply
disable xsave then.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-7-git-send-email-robert.richter@amd.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/kernel/i387.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 1f11f5c..2605c50 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -67,6 +67,12 @@ static void __cpuinit init_thread_xstate(void)
 	 */
 
 	if (!HAVE_HWFP) {
+		/*
+		 * Disable xsave as we do not support it if i387
+		 * emulation is enabled.
+		 */
+		setup_clear_cpu_cap(X86_FEATURE_XSAVE);
+		setup_clear_cpu_cap(X86_FEATURE_XSAVEOPT);
 		xstate_size = sizeof(struct i387_soft_struct);
 		return;
 	}

[PATCH 7/7] x86: removing boot_cpu_id variable

[Robert Richter]

boot_cpu_id is there for historical reasons and was renamed to
boot_cpu_physical_apicid in patch:

 c70dcb7 x86: change boot_cpu_id to boot_cpu_physical_apicid

However, there are some remaining occurrences of boot_cpu_id that are
never touched in the kernel and thus its value is always 0.

This patch removes boot_cpu_id at all.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/apb_timer.h |    1 -
 arch/x86/include/asm/cpu.h       |    1 -
 arch/x86/kernel/apb_timer.c      |    2 +-
 arch/x86/kernel/apic/io_apic.c   |   12 ++++++------
 arch/x86/kernel/cpu/amd.c        |    2 +-
 arch/x86/kernel/cpu/common.c     |    2 +-
 arch/x86/kernel/cpu/intel.c      |    2 +-
 arch/x86/kernel/reboot.c         |    2 +-
 arch/x86/kernel/setup.c          |    1 -
 arch/x86/kernel/setup_percpu.c   |    4 ++--
 arch/x86/mm/k8topology_64.c      |    6 +++---
 11 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/apb_timer.h b/arch/x86/include/asm/apb_timer.h
index c74a2ee..d15d185 100644
--- a/arch/x86/include/asm/apb_timer.h
+++ b/arch/x86/include/asm/apb_timer.h
@@ -54,7 +54,6 @@ extern struct clock_event_device *global_clock_event;
 extern unsigned long apbt_quick_calibrate(void);
 extern int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu);
 extern void apbt_setup_secondary_clock(void);
-extern unsigned int boot_cpu_id;
 extern int disable_apbt_percpu;
 
 extern struct sfi_timer_table_entry *sfi_get_mtmr(int hint);
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index b185091..4fab24d 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -32,6 +32,5 @@ extern void arch_unregister_cpu(int);
 
 DECLARE_PER_CPU(int, cpu_state);
 
-extern unsigned int boot_cpu_id;
 
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/apb_timer.c b/arch/x86/kernel/apb_timer.c
index a353475..8e5b862 100644
--- a/arch/x86/kernel/apb_timer.c
+++ b/arch/x86/kernel/apb_timer.c
@@ -367,7 +367,7 @@ void apbt_setup_secondary_clock(void)
 
 	/* Don't register boot CPU clockevent */
 	cpu = smp_processor_id();
-	if (cpu == boot_cpu_id)
+	if (!cpu)
 		return;
 	/*
 	 * We need to calculate the scaled math multiplication factor for
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 33f3563..6b7c1df 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -162,7 +162,7 @@ int __init arch_early_irq_init(void)
 
 	cfg = irq_cfgx;
 	count = ARRAY_SIZE(irq_cfgx);
-	node= cpu_to_node(boot_cpu_id);
+	node = cpu_to_node(0);
 
 	for (i = 0; i < count; i++) {
 		desc = irq_to_desc(i);
@@ -1483,7 +1483,7 @@ static void __init setup_IO_APIC_irqs(void)
 	int notcon = 0;
 	struct irq_desc *desc;
 	struct irq_cfg *cfg;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 
 	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
 
@@ -1548,7 +1548,7 @@ static void __init setup_IO_APIC_irqs(void)
 void setup_IO_APIC_irq_extra(u32 gsi)
 {
 	int apic_id = 0, pin, idx, irq;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	struct irq_desc *desc;
 	struct irq_cfg *cfg;
 
@@ -2925,7 +2925,7 @@ static inline void __init check_timer(void)
 {
 	struct irq_desc *desc = irq_to_desc(0);
 	struct irq_cfg *cfg = desc->chip_data;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	int apic1, pin1, apic2, pin2;
 	unsigned long flags;
 	int no_pin1 = 0;
@@ -3279,7 +3279,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
 
 int create_irq(void)
 {
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	unsigned int irq_want;
 	int irq;
 
@@ -3901,7 +3901,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
 	if (dev)
 		node = dev_to_node(dev);
 	else
-		node = cpu_to_node(boot_cpu_id);
+		node = cpu_to_node(0);
 
 	desc = irq_to_desc_alloc_node(irq, node);
 	if (!desc) {
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 12b9cff..2eb3a89 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -148,7 +148,7 @@ static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (c->cpu_index == boot_cpu_id)
+	if (!c->cpu_index)
 		return;
 
 	/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 94c36c7..0b64950 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -664,7 +664,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		this_cpu->c_early_init(c);
 
 #ifdef CONFIG_SMP
-	c->cpu_index = boot_cpu_id;
+	c->cpu_index = 0;
 #endif
 	filter_cpuid_features(c, false);
 }
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 85f69cd..3a683ea 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -169,7 +169,7 @@ static void __cpuinit intel_smp_check(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (c->cpu_index == boot_cpu_id)
+	if (!c->cpu_index)
 		return;
 
 	/*
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 8e1aac8..31c8c36 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -84,7 +84,7 @@ static int __init reboot_setup(char *str)
 			}
 				/* we will leave sorting out the final value
 				   when we are ready to reboot, since we might not
-				   have set up boot_cpu_id or smp_num_cpu */
+				   have detected BSP APIC ID or smp_num_cpu */
 			break;
 #endif /* CONFIG_SMP */
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b4ae4ac..7b2a713 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -124,7 +124,6 @@ unsigned long max_pfn_mapped;
 RESERVE_BRK(dmi_alloc, 65536);
 #endif
 
-unsigned int boot_cpu_id __read_mostly;
 
 static __initdata unsigned long _brk_start = (unsigned long)__brk_base;
 unsigned long _brk_end = (unsigned long)__brk_base;
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index de3b63a..7260dc4 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -244,7 +244,7 @@ void __init setup_per_cpu_areas(void)
 		 * Up to this point, the boot CPU has been using .init.data
 		 * area.  Reload any changed state for the boot CPU.
 		 */
-		if (cpu == boot_cpu_id)
+		if (!cpu)
 			switch_to_new_gdt(cpu);
 	}
 
@@ -262,7 +262,7 @@ void __init setup_per_cpu_areas(void)
 	 * make sure boot cpu numa_node is right, when boot cpu is on the
 	 * node that doesn't have mem installed
 	 */
-	set_cpu_numa_node(boot_cpu_id, early_cpu_to_node(boot_cpu_id));
+	set_cpu_numa_node(0, early_cpu_to_node(0));
 #endif
 
 	/* Setup node to cpumask map */
diff --git a/arch/x86/mm/k8topology_64.c b/arch/x86/mm/k8topology_64.c
index 970ed57..240f864 100644
--- a/arch/x86/mm/k8topology_64.c
+++ b/arch/x86/mm/k8topology_64.c
@@ -54,8 +54,8 @@ static __init int find_northbridge(void)
 static __init void early_get_boot_cpu_id(void)
 {
 	/*
-	 * need to get boot_cpu_id so can use that to create apicid_to_node
-	 * in k8_scan_nodes()
+	 * need to get the APIC ID of the BSP so can use that to
+	 * create apicid_to_node in k8_scan_nodes()
 	 */
 #ifdef CONFIG_X86_MPPARSE
 	/*
@@ -212,7 +212,7 @@ int __init k8_scan_nodes(void)
 	bits = boot_cpu_data.x86_coreid_bits;
 	cores = (1<<bits);
 	apicid_base = 0;
-	/* need to get boot_cpu_id early for system with apicid lifting */
+	/* get the APIC ID of the BSP early for systems with apicid lifting */
 	early_get_boot_cpu_id();
 	if (boot_cpu_physical_apicid > 0) {
 		pr_info("BSP APIC ID: %02x\n", boot_cpu_physical_apicid);
-- 
1.7.1.1

[tip:x86/cleanups] x86, cleanup: Remove obsolete boot_cpu_id variable

[tip-bot for Robert Richter]

Commit-ID:  f6e9456c9272bb570df6e217cdbe007e270b1c4e
Gitweb:     http://git.kernel.org/tip/f6e9456c9272bb570df6e217cdbe007e270b1c4e
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Wed, 21 Jul 2010 19:03:58 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Thu, 12 Aug 2010 14:01:38 -0700

x86, cleanup: Remove obsolete boot_cpu_id variable

boot_cpu_id is there for historical reasons and was renamed to
boot_cpu_physical_apicid in patch:

 c70dcb7 x86: change boot_cpu_id to boot_cpu_physical_apicid

However, there are some remaining occurrences of boot_cpu_id that are
never touched in the kernel and thus its value is always 0.

This patch removes boot_cpu_id completely.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279731838-1522-8-git-send-email-robert.richter@amd.com>
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
 arch/x86/include/asm/apb_timer.h |    1 -
 arch/x86/include/asm/cpu.h       |    1 -
 arch/x86/kernel/apb_timer.c      |    2 +-
 arch/x86/kernel/apic/io_apic.c   |   12 ++++++------
 arch/x86/kernel/cpu/amd.c        |    2 +-
 arch/x86/kernel/cpu/common.c     |    2 +-
 arch/x86/kernel/cpu/intel.c      |    2 +-
 arch/x86/kernel/reboot.c         |    2 +-
 arch/x86/kernel/setup.c          |    1 -
 arch/x86/kernel/setup_percpu.c   |    2 +-
 arch/x86/mm/k8topology_64.c      |    6 +++---
 11 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/apb_timer.h b/arch/x86/include/asm/apb_timer.h
index a69b1ac..2fefa50 100644
--- a/arch/x86/include/asm/apb_timer.h
+++ b/arch/x86/include/asm/apb_timer.h
@@ -54,7 +54,6 @@ extern struct clock_event_device *global_clock_event;
 extern unsigned long apbt_quick_calibrate(void);
 extern int arch_setup_apbt_irqs(int irq, int trigger, int mask, int cpu);
 extern void apbt_setup_secondary_clock(void);
-extern unsigned int boot_cpu_id;
 
 extern struct sfi_timer_table_entry *sfi_get_mtmr(int hint);
 extern void sfi_free_mtmr(struct sfi_timer_table_entry *mtmr);
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index b185091..4fab24d 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -32,6 +32,5 @@ extern void arch_unregister_cpu(int);
 
 DECLARE_PER_CPU(int, cpu_state);
 
-extern unsigned int boot_cpu_id;
 
 #endif /* _ASM_X86_CPU_H */
diff --git a/arch/x86/kernel/apb_timer.c b/arch/x86/kernel/apb_timer.c
index 8dd7780..08f75fb 100644
--- a/arch/x86/kernel/apb_timer.c
+++ b/arch/x86/kernel/apb_timer.c
@@ -343,7 +343,7 @@ void apbt_setup_secondary_clock(void)
 
 	/* Don't register boot CPU clockevent */
 	cpu = smp_processor_id();
-	if (cpu == boot_cpu_id)
+	if (!cpu)
 		return;
 	/*
 	 * We need to calculate the scaled math multiplication factor for
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 4dc0084..8884928 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -162,7 +162,7 @@ int __init arch_early_irq_init(void)
 
 	cfg = irq_cfgx;
 	count = ARRAY_SIZE(irq_cfgx);
-	node= cpu_to_node(boot_cpu_id);
+	node = cpu_to_node(0);
 
 	for (i = 0; i < count; i++) {
 		desc = irq_to_desc(i);
@@ -1483,7 +1483,7 @@ static void __init setup_IO_APIC_irqs(void)
 	int notcon = 0;
 	struct irq_desc *desc;
 	struct irq_cfg *cfg;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 
 	apic_printk(APIC_VERBOSE, KERN_DEBUG "init IO_APIC IRQs\n");
 
@@ -1548,7 +1548,7 @@ static void __init setup_IO_APIC_irqs(void)
 void setup_IO_APIC_irq_extra(u32 gsi)
 {
 	int apic_id = 0, pin, idx, irq;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	struct irq_desc *desc;
 	struct irq_cfg *cfg;
 
@@ -2925,7 +2925,7 @@ static inline void __init check_timer(void)
 {
 	struct irq_desc *desc = irq_to_desc(0);
 	struct irq_cfg *cfg = desc->chip_data;
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	int apic1, pin1, apic2, pin2;
 	unsigned long flags;
 	int no_pin1 = 0;
@@ -3279,7 +3279,7 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
 
 int create_irq(void)
 {
-	int node = cpu_to_node(boot_cpu_id);
+	int node = cpu_to_node(0);
 	unsigned int irq_want;
 	int irq;
 
@@ -3901,7 +3901,7 @@ static int __io_apic_set_pci_routing(struct device *dev, int irq,
 	if (dev)
 		node = dev_to_node(dev);
 	else
-		node = cpu_to_node(boot_cpu_id);
+		node = cpu_to_node(0);
 
 	desc = irq_to_desc_alloc_node(irq, node);
 	if (!desc) {
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 60a57b1..3a7c852 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -148,7 +148,7 @@ static void __cpuinit amd_k7_smp_check(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (c->cpu_index == boot_cpu_id)
+	if (!c->cpu_index)
 		return;
 
 	/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 490dac6..787b3c7 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -665,7 +665,7 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 		this_cpu->c_early_init(c);
 
 #ifdef CONFIG_SMP
-	c->cpu_index = boot_cpu_id;
+	c->cpu_index = 0;
 #endif
 	filter_cpuid_features(c, false);
 }
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 85f69cd..3a683ea 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -169,7 +169,7 @@ static void __cpuinit intel_smp_check(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (c->cpu_index == boot_cpu_id)
+	if (!c->cpu_index)
 		return;
 
 	/*
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index e3af342..7a4cf14 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -84,7 +84,7 @@ static int __init reboot_setup(char *str)
 			}
 				/* we will leave sorting out the final value
 				   when we are ready to reboot, since we might not
-				   have set up boot_cpu_id or smp_num_cpu */
+				   have detected BSP APIC ID or smp_num_cpu */
 			break;
 #endif /* CONFIG_SMP */
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index b008e78..dede5c4 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -125,7 +125,6 @@ unsigned long max_pfn_mapped;
 RESERVE_BRK(dmi_alloc, 65536);
 #endif
 
-unsigned int boot_cpu_id __read_mostly;
 
 static __initdata unsigned long _brk_start = (unsigned long)__brk_base;
 unsigned long _brk_end = (unsigned long)__brk_base;
diff --git a/arch/x86/kernel/setup_percpu.c b/arch/x86/kernel/setup_percpu.c
index a60df9a..2335c15 100644
--- a/arch/x86/kernel/setup_percpu.c
+++ b/arch/x86/kernel/setup_percpu.c
@@ -253,7 +253,7 @@ void __init setup_per_cpu_areas(void)
 		 * Up to this point, the boot CPU has been using .init.data
 		 * area.  Reload any changed state for the boot CPU.
 		 */
-		if (cpu == boot_cpu_id)
+		if (!cpu)
 			switch_to_new_gdt(cpu);
 	}
 
diff --git a/arch/x86/mm/k8topology_64.c b/arch/x86/mm/k8topology_64.c
index 970ed57..240f864 100644
--- a/arch/x86/mm/k8topology_64.c
+++ b/arch/x86/mm/k8topology_64.c
@@ -54,8 +54,8 @@ static __init int find_northbridge(void)
 static __init void early_get_boot_cpu_id(void)
 {
 	/*
-	 * need to get boot_cpu_id so can use that to create apicid_to_node
-	 * in k8_scan_nodes()
+	 * need to get the APIC ID of the BSP so can use that to
+	 * create apicid_to_node in k8_scan_nodes()
 	 */
 #ifdef CONFIG_X86_MPPARSE
 	/*
@@ -212,7 +212,7 @@ int __init k8_scan_nodes(void)
 	bits = boot_cpu_data.x86_coreid_bits;
 	cores = (1<<bits);
 	apicid_base = 0;
-	/* need to get boot_cpu_id early for system with apicid lifting */
+	/* get the APIC ID of the BSP early for systems with apicid lifting */
 	early_get_boot_cpu_id();
 	if (boot_cpu_physical_apicid > 0) {
 		pr_info("BSP APIC ID: %02x\n", boot_cpu_physical_apicid);

Re: [PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[Suresh Siddha]

On Wed, 2010-07-21 at 10:03 -0700, Robert Richter wrote:
> Hans Peter, Suresh, and Cyrill, thanks for your feedback.
> 
> This is version 2 of the patch series.
> 
> Patches 2, 4, 7 are unchanged compared to -v1.
> 
> Patch 1 folds the initialization code into one patch. fpu_init() is
> now called before xsave_init(). xsave_init() can later overwrite
> xstate_size based on xstate features.
> 
> Patch 3 replaces all hardcoded XSTATE_CPUID values know. cpuid_level
> is now taken from boot_cpu_data.
> 
> Patches 5 and 6 are new:
> * add __init attribute to setup_xstate_features()
> * disable xsave in i387 emulation mode

Apart from the cleanup comments that I had for "[PATCH 6/7] x86, xsave:
disable xsave in i387 emulation mode", all other patches look good to
me.

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>

Thanks.

Re: [PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[H. Peter Anvin]

On 07/21/2010 10:03 AM, Robert Richter wrote:
> Hans Peter, Suresh, and Cyrill, thanks for your feedback.
> 
> This is version 2 of the patch series.
> 
> Patches 2, 4, 7 are unchanged compared to -v1.
> 
> Patch 1 folds the initialization code into one patch. fpu_init() is
> now called before xsave_init(). xsave_init() can later overwrite
> xstate_size based on xstate features.
> 
> Patch 3 replaces all hardcoded XSTATE_CPUID values know. cpuid_level
> is now taken from boot_cpu_data.
> 
> Patches 5 and 6 are new:
> * add __init attribute to setup_xstate_features()
> * disable xsave in i387 emulation mode
> 

Please note that I did integrate some of your patches already.  I might
need you to rebase; I'll know in a few minutes.

	-hpa

Re: [PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[H. Peter Anvin]

On 07/21/2010 01:55 PM, H. Peter Anvin wrote:
> 
> Please note that I did integrate some of your patches already.  I might
> need you to rebase; I'll know in a few minutes.
> 

Nevermind, you had already seen that.

I'm going to hold off on 6/7 to let you address Suresh' comments, and
will look for a sensible place to put 7/7.

	-hpa

Re: [osrc-patches] [PATCH 0/7] x86, xsave: some code cleanups and reworks, -v2

[Robert Richter]

On 21.07.10 19:03:51, Robert Richter wrote:
> This is version 2 of the patch series.

Hans Peter,

I still have patch 6 and 7 in my queue, any opinions?

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center