[PATCH 00/10] x86, xsave: some code cleanups and reworks

[PATCH 00/10] x86, xsave: some code cleanups and reworks

[Robert Richter]

This patch series contains some cleanups and reworks I made during
code review and feature implementation for upcoming cpus.

Most patches refactor the xsave initialization that is very dependent
on fpu initialization. This series starts to decouple this a little
bit as xsave not only supports fpu features. Also this is an attempt
to ease the xsave interface by making some of the functions and
variables static.

There is also one patch that removes boot_cpu_id variable, which is
not really related to xsave. Maybe this should be applied to another
branch.

The patches are relative to today's tip/x86/xsave branch.

(The patches are small for better review and rebasing.)

-Robert

[PATCH 01/10] x86, xsave: do not include asm/i387.h in asm/xsave.h

[Robert Richter]

There are no dependencies to asm/i387.h. Instead, if including only
xsave.h the following error occurs:

 .../arch/x86/include/asm/i387.h:110: error: ‘XSTATE_FP’ undeclared (first use in this function)
 .../arch/x86/include/asm/i387.h:110: error: (Each undeclared identifier is reported only once
 .../arch/x86/include/asm/i387.h:110: error: for each function it appears in.)

This patch fixes this.

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

diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
index ec86c5f..94d5f84 100644
--- a/arch/x86/include/asm/xsave.h
+++ b/arch/x86/include/asm/xsave.h
@@ -3,7 +3,6 @@
 
 #include <linux/types.h>
 #include <asm/processor.h>
-#include <asm/i387.h>
 
 #define XSTATE_FP	0x1
 #define XSTATE_SSE	0x2
-- 
1.7.1.1

Re: [PATCH 01/10] x86, xsave: do not include asm/i387.h in asm/xsave.h

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> There are no dependencies to asm/i387.h. Instead, if including only
> xsave.h the following error occurs:
> 
>  .../arch/x86/include/asm/i387.h:110: error: ‘XSTATE_FP’ undeclared (first use in this function)
>  .../arch/x86/include/asm/i387.h:110: error: (Each undeclared identifier is reported only once
>  .../arch/x86/include/asm/i387.h:110: error: for each function it appears in.)
> 
> This patch fixes this.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/include/asm/xsave.h |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xsave.h b/arch/x86/include/asm/xsave.h
> index ec86c5f..94d5f84 100644
> --- a/arch/x86/include/asm/xsave.h
> +++ b/arch/x86/include/asm/xsave.h
> @@ -3,7 +3,6 @@
>  
>  #include <linux/types.h>
>  #include <asm/processor.h>
> -#include <asm/i387.h>
>  
>  #define XSTATE_FP	0x1
>  #define XSTATE_SSE	0x2

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

[PATCH 02/10] x86, xsave: 32/64 bit boot cpu check unification in initialization

[Robert Richter]

Boot cpu id is always 0, thus simplifying and unifying boot cpu check.

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.

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

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3f715ef..26804b2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1273,7 +1273,7 @@ void __cpuinit cpu_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (smp_processor_id() == boot_cpu_id)
+	if (!smp_processor_id())
 		init_thread_xstate();
 
 	xsave_init();
-- 
1.7.1.1

Re: [PATCH 02/10] x86, xsave: 32/64 bit boot cpu check unification in initialization

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> Boot cpu id is always 0, thus simplifying and unifying boot cpu check.
> 
> 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.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/kernel/cpu/common.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 3f715ef..26804b2 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1273,7 +1273,7 @@ void __cpuinit cpu_init(void)
>  	/*
>  	 * Boot processor to setup the FP and extended state context info.
>  	 */
> -	if (smp_processor_id() == boot_cpu_id)
> +	if (!smp_processor_id())
>  		init_thread_xstate();
>  
>  	xsave_init();

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

[tip:x86/xsave] x86, xsave: 32/64 bit boot cpu check unification in initialization

[tip-bot for Robert Richter]

Commit-ID:  db10db48b2c530def21bfd76d576702c7df7f620
Gitweb:     http://git.kernel.org/tip/db10db48b2c530def21bfd76d576702c7df7f620
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Tue, 20 Jul 2010 20:50:49 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 20 Jul 2010 16:21:38 -0700

x86, xsave: 32/64 bit boot cpu check unification in initialization

Boot cpu id is always 0, thus simplifying and unifying boot cpu check.

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.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279651857-24639-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/cpu/common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3f715ef..26804b2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1273,7 +1273,7 @@ void __cpuinit cpu_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (smp_processor_id() == boot_cpu_id)
+	if (!smp_processor_id())
 		init_thread_xstate();
 
 	xsave_init();

[PATCH 03/10] 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 26804b2..433afed 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

[PATCH 04/10] x86, xsave: moving boot cpu initialization to xsave_init()

[Robert Richter]

This patch moves boot cpu initialization to xsave_init(). Now all cpus
are initialized in one single function.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/kernel/cpu/common.c |    6 ------
 arch/x86/kernel/i387.c       |    5 -----
 arch/x86/kernel/xsave.c      |   14 ++++++++++++--
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 433afed..1a053e0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1270,12 +1270,6 @@ void __cpuinit cpu_init(void)
 	clear_used_math();
 	mxcsr_feature_mask_init();
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		init_thread_xstate();
-
 	xsave_init();
 }
 #endif
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 6106af9..2f32ef0 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -93,11 +93,6 @@ void __cpuinit fpu_init(void)
 
 	write_cr0(oldcr0 & ~(X86_CR0_TS|X86_CR0_EM)); /* clear TS and EM */
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		init_thread_xstate();
 	xsave_init();
 
 	mxcsr_feature_mask_init();
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 368047c..ab9ad48 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,7 +360,7 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-void __cpuinit xsave_init(void)
+static void __cpuinit __xsave_init(void)
 {
 	if (!cpu_has_xsave)
 		return;
@@ -446,7 +446,7 @@ void __ref xsave_cntxt_init(void)
 	 * Support only the state known to OS.
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
-	xsave_init();
+	__xsave_init();
 
 	/*
 	 * Recompute the context size for enabled features
@@ -463,3 +463,13 @@ void __ref xsave_cntxt_init(void)
 	       "cntxt size 0x%x\n",
 	       pcntxt_mask, xstate_size);
 }
+
+void __cpuinit xsave_init(void)
+{
+	/*
+	 * Boot processor to setup the FP and extended state context info.
+	 */
+	if (!smp_processor_id())
+		init_thread_xstate();
+	__xsave_init();
+}
-- 
1.7.1.1

Re: [PATCH 04/10] x86, xsave: moving boot cpu initialization to xsave_init()

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> This patch moves boot cpu initialization to xsave_init(). Now all cpus
> are initialized in one single function.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/kernel/cpu/common.c |    6 ------
>  arch/x86/kernel/i387.c       |    5 -----
>  arch/x86/kernel/xsave.c      |   14 ++++++++++++--
>  3 files changed, 12 insertions(+), 13 deletions(-)

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

> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 433afed..1a053e0 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1270,12 +1270,6 @@ void __cpuinit cpu_init(void)
>  	clear_used_math();
>  	mxcsr_feature_mask_init();
>  
> -	/*
> -	 * Boot processor to setup the FP and extended state context info.
> -	 */
> -	if (!smp_processor_id())
> -		init_thread_xstate();
> -
>  	xsave_init();
>  }
>  #endif
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 6106af9..2f32ef0 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -93,11 +93,6 @@ void __cpuinit fpu_init(void)
>  
>  	write_cr0(oldcr0 & ~(X86_CR0_TS|X86_CR0_EM)); /* clear TS and EM */
>  
> -	/*
> -	 * Boot processor to setup the FP and extended state context info.
> -	 */
> -	if (!smp_processor_id())
> -		init_thread_xstate();
>  	xsave_init();
>  
>  	mxcsr_feature_mask_init();
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index 368047c..ab9ad48 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -360,7 +360,7 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
>  /*
>   * Enable the extended processor state save/restore feature
>   */
> -void __cpuinit xsave_init(void)
> +static void __cpuinit __xsave_init(void)
>  {
>  	if (!cpu_has_xsave)
>  		return;
> @@ -446,7 +446,7 @@ void __ref xsave_cntxt_init(void)
>  	 * Support only the state known to OS.
>  	 */
>  	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
> -	xsave_init();
> +	__xsave_init();
>  
>  	/*
>  	 * Recompute the context size for enabled features
> @@ -463,3 +463,13 @@ void __ref xsave_cntxt_init(void)
>  	       "cntxt size 0x%x\n",
>  	       pcntxt_mask, xstate_size);
>  }
> +
> +void __cpuinit xsave_init(void)
> +{
> +	/*
> +	 * Boot processor to setup the FP and extended state context info.
> +	 */
> +	if (!smp_processor_id())
> +		init_thread_xstate();
> +	__xsave_init();
> +}

[tip:x86/xsave] x86, xsave: Move boot cpu initialization to xsave_init()

[tip-bot for Robert Richter]

Commit-ID:  82d4150cec83b9775f84810b39a1c0b91585d429
Gitweb:     http://git.kernel.org/tip/82d4150cec83b9775f84810b39a1c0b91585d429
Author:     Robert Richter <robert.richter@amd.com>
AuthorDate: Tue, 20 Jul 2010 20:50:51 +0200
Committer:  H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Tue, 20 Jul 2010 16:21:44 -0700

x86, xsave: Move boot cpu initialization to xsave_init()

This patch moves boot cpu initialization to xsave_init(). Now all cpus
are initialized in one single function.

Signed-off-by: Robert Richter <robert.richter@amd.com>
LKML-Reference: <1279651857-24639-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/kernel/cpu/common.c |    6 ------
 arch/x86/kernel/i387.c       |    5 -----
 arch/x86/kernel/xsave.c      |   14 ++++++++++++--
 3 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 26804b2..4056108 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1270,12 +1270,6 @@ void __cpuinit cpu_init(void)
 	clear_used_math();
 	mxcsr_feature_mask_init();
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		init_thread_xstate();
-
 	xsave_init();
 }
 #endif
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 6106af9..2f32ef0 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -93,11 +93,6 @@ void __cpuinit fpu_init(void)
 
 	write_cr0(oldcr0 & ~(X86_CR0_TS|X86_CR0_EM)); /* clear TS and EM */
 
-	/*
-	 * Boot processor to setup the FP and extended state context info.
-	 */
-	if (!smp_processor_id())
-		init_thread_xstate();
 	xsave_init();
 
 	mxcsr_feature_mask_init();
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 368047c..ab9ad48 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -360,7 +360,7 @@ unsigned int sig_xstate_size = sizeof(struct _fpstate);
 /*
  * Enable the extended processor state save/restore feature
  */
-void __cpuinit xsave_init(void)
+static void __cpuinit __xsave_init(void)
 {
 	if (!cpu_has_xsave)
 		return;
@@ -446,7 +446,7 @@ void __ref xsave_cntxt_init(void)
 	 * Support only the state known to OS.
 	 */
 	pcntxt_mask = pcntxt_mask & XCNTXT_MASK;
-	xsave_init();
+	__xsave_init();
 
 	/*
 	 * Recompute the context size for enabled features
@@ -463,3 +463,13 @@ void __ref xsave_cntxt_init(void)
 	       "cntxt size 0x%x\n",
 	       pcntxt_mask, xstate_size);
 }
+
+void __cpuinit xsave_init(void)
+{
+	/*
+	 * Boot processor to setup the FP and extended state context info.
+	 */
+	if (!smp_processor_id())
+		init_thread_xstate();
+	__xsave_init();
+}

[PATCH 05/10] x86, xsave: make xsave_cntxt_init() static

[Robert Richter]

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

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

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/i387.c b/arch/x86/kernel/i387.c
index 2f32ef0..196b8c7 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -66,10 +66,9 @@ void __cpuinit init_thread_xstate(void)
 		return;
 	}
 
-	if (cpu_has_xsave) {
-		xsave_cntxt_init();
+	if (cpu_has_xsave)
+		/* xstate_size was already initialized */
 		return;
-	}
 
 	if (cpu_has_fxsr)
 		xstate_size = sizeof(struct i387_fxsave_struct);
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index ab9ad48..c46082d 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -429,10 +429,13 @@ 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;
 
+	if (!cpu_has_xsave)
+		return;
+
 	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
 	pcntxt_mask = eax + ((u64)edx << 32);
 
@@ -469,7 +472,9 @@ void __cpuinit xsave_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (!smp_processor_id())
+	if (!smp_processor_id()) {
+		xsave_cntxt_init();
 		init_thread_xstate();
+	}
 	__xsave_init();
 }
-- 
1.7.1.1

Re: [PATCH 05/10] x86, xsave: make xsave_cntxt_init() static

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> There is a lot of cross referencing between fpu and xsave code. This
> patch reduces this by making xsave_cntxt_init() a static function.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>
> ---
>  arch/x86/include/asm/xsave.h |    1 -
>  arch/x86/kernel/i387.c       |    5 ++---
>  arch/x86/kernel/xsave.c      |    9 +++++++--
>  3 files changed, 9 insertions(+), 6 deletions(-)

Perhaps we can fold this patch into 
[PATCH 06/10] x86, xsave: do not initialize xsave in fpu_init()

and probably do  fpu_init() first followed by xsave_init() so that
fpu_init() will set xstate_size based on fpu features and xsave_init()
can later overwrite xstate_size based on xstate features.

thanks.

> 
> 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/i387.c b/arch/x86/kernel/i387.c
> index 2f32ef0..196b8c7 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -66,10 +66,9 @@ void __cpuinit init_thread_xstate(void)
>  		return;
>  	}
>  
> -	if (cpu_has_xsave) {
> -		xsave_cntxt_init();
> +	if (cpu_has_xsave)
> +		/* xstate_size was already initialized */
>  		return;
> -	}
>  
>  	if (cpu_has_fxsr)
>  		xstate_size = sizeof(struct i387_fxsave_struct);
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index ab9ad48..c46082d 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -429,10 +429,13 @@ 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;
>  
> +	if (!cpu_has_xsave)
> +		return;
> +
>  	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
>  	pcntxt_mask = eax + ((u64)edx << 32);
>  
> @@ -469,7 +472,9 @@ void __cpuinit xsave_init(void)
>  	/*
>  	 * Boot processor to setup the FP and extended state context info.
>  	 */
> -	if (!smp_processor_id())
> +	if (!smp_processor_id()) {
> +		xsave_cntxt_init();
>  		init_thread_xstate();
> +	}
>  	__xsave_init();
>  }

Re: [PATCH 05/10] x86, xsave: make xsave_cntxt_init() static

[Robert Richter]

On 20.07.10 18:20:24, Suresh Siddha wrote:

> Perhaps we can fold this patch into 
> [PATCH 06/10] x86, xsave: do not initialize xsave in fpu_init()
> 
> and probably do  fpu_init() first followed by xsave_init() so that
> fpu_init() will set xstate_size based on fpu features and xsave_init()
> can later overwrite xstate_size based on xstate features.

Yes, I will do this instead.

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

-Robert

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

Re: [PATCH 05/10] x86, xsave: make xsave_cntxt_init() static

[H. Peter Anvin]

On 07/21/2010 06:48 AM, Robert Richter wrote:
> On 20.07.10 18:20:24, Suresh Siddha wrote:
> 
>> Perhaps we can fold this patch into 
>> [PATCH 06/10] x86, xsave: do not initialize xsave in fpu_init()
>>
>> and probably do  fpu_init() first followed by xsave_init() so that
>> fpu_init() will set xstate_size based on fpu features and xsave_init()
>> can later overwrite xstate_size based on xstate features.
> 
> Yes, I will do this instead.
> 
> Lookin at this, I found that xsave is broken for (!HAVE_HWFP). This
> should be the case if config MATH_EMULATION and 'no387' kernel
> parameter are set and xsave exists. xsave will not work because
> x86/math-emu and xsave share the same memory. But this case can be
> treated as corner case. Maybe we should simply disable xsave then.
> 

Yes, I think we should.  It is *extremely* unlikely that any such real
hardware will ever be created at this point, and if so we can burn that
bridge when we get to it.

	-hpa

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

[PATCH 06/10] x86, xsave: do not initialize xsave in fpu_init()

[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.

Signed-off-by: Robert Richter <robert.richter@amd.com>
---
 arch/x86/include/asm/i387.h  |    1 -
 arch/x86/kernel/cpu/common.c |    2 ++
 arch/x86/kernel/i387.c       |   17 ++++++++++++++---
 arch/x86/kernel/xsave.c      |    4 +---
 4 files changed, 17 insertions(+), 7 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/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 1a053e0..d8eba22 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1209,6 +1209,7 @@ void __cpuinit cpu_init(void)
 	clear_all_debug_regs();
 	dbg_restore_debug_regs();
 
+	xsave_init();
 	fpu_init();
 
 	raw_local_save_flags(kernel_eflags);
@@ -1271,5 +1272,6 @@ void __cpuinit cpu_init(void)
 	mxcsr_feature_mask_init();
 
 	xsave_init();
+	fpu_init();
 }
 #endif
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 196b8c7..122a764 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -59,7 +59,7 @@ void __cpuinit mxcsr_feature_mask_init(void)
 	stts();
 }
 
-void __cpuinit init_thread_xstate(void)
+static void __cpuinit init_thread_xstate(void)
 {
 	if (!HAVE_HWFP) {
 		xstate_size = sizeof(struct i387_soft_struct);
@@ -83,6 +83,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();
@@ -92,14 +93,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 c46082d..a594f49 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -472,9 +472,7 @@ void __cpuinit xsave_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (!smp_processor_id()) {
+	if (!smp_processor_id())
 		xsave_cntxt_init();
-		init_thread_xstate();
-	}
 	__xsave_init();
 }
-- 
1.7.1.1

Re: [PATCH 06/10] x86, xsave: do not initialize xsave in fpu_init()

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> As xsave also supports other than fpu features, it should be
> initialized independently of the fpu. This patch moves this out of fpu
> initialization.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>

As I mentioned in the previous patch, can we do xsave_init() after
fpu_init(). fpu_init() does some basic initialization like clearing TS
and EM bits etc. xsave_init() can be followed after this.

thanks.

> ---
>  arch/x86/include/asm/i387.h  |    1 -
>  arch/x86/kernel/cpu/common.c |    2 ++
>  arch/x86/kernel/i387.c       |   17 ++++++++++++++---
>  arch/x86/kernel/xsave.c      |    4 +---
>  4 files changed, 17 insertions(+), 7 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/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 1a053e0..d8eba22 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1209,6 +1209,7 @@ void __cpuinit cpu_init(void)
>  	clear_all_debug_regs();
>  	dbg_restore_debug_regs();
>  
> +	xsave_init();
>  	fpu_init();
>  
>  	raw_local_save_flags(kernel_eflags);
> @@ -1271,5 +1272,6 @@ void __cpuinit cpu_init(void)
>  	mxcsr_feature_mask_init();
>  
>  	xsave_init();
> +	fpu_init();
>  }
>  #endif
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 196b8c7..122a764 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -59,7 +59,7 @@ void __cpuinit mxcsr_feature_mask_init(void)
>  	stts();
>  }
>  
> -void __cpuinit init_thread_xstate(void)
> +static void __cpuinit init_thread_xstate(void)
>  {
>  	if (!HAVE_HWFP) {
>  		xstate_size = sizeof(struct i387_soft_struct);
> @@ -83,6 +83,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();
> @@ -92,14 +93,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 c46082d..a594f49 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -472,9 +472,7 @@ void __cpuinit xsave_init(void)
>  	/*
>  	 * Boot processor to setup the FP and extended state context info.
>  	 */
> -	if (!smp_processor_id()) {
> +	if (!smp_processor_id())
>  		xsave_cntxt_init();
> -		init_thread_xstate();
> -	}
>  	__xsave_init();
>  }

[PATCH 07/10] x86, xsave: reduce cpu_has_xsave checks

[Robert Richter]

This 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/kernel/xsave.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index a594f49..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);
 
 	/*
@@ -433,9 +430,6 @@ static void __cpuinit xsave_cntxt_init(void)
 {
 	unsigned int eax, ebx, ecx, edx;
 
-	if (!cpu_has_xsave)
-		return;
-
 	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
 	pcntxt_mask = eax + ((u64)edx << 32);
 
@@ -469,6 +463,9 @@ static void __cpuinit 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.
 	 */
-- 
1.7.1.1

Re: [PATCH 07/10] x86, xsave: reduce cpu_has_xsave checks

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> This 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>

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

> ---
>  arch/x86/kernel/xsave.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index a594f49..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);
>  
>  	/*
> @@ -433,9 +430,6 @@ static void __cpuinit xsave_cntxt_init(void)
>  {
>  	unsigned int eax, ebx, ecx, edx;
>  
> -	if (!cpu_has_xsave)
> -		return;
> -
>  	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
>  	pcntxt_mask = eax + ((u64)edx << 32);
>  
> @@ -469,6 +463,9 @@ static void __cpuinit 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.
>  	 */

[PATCH 08/10] 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 08/10] x86, xsave: introduce xstate enable functions

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, 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>

Acked-by: Suresh Siddha <suresh.b.siddha@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 09/10] x86, xsave: check cpuid level for XSTATE_CPUID (0x0d)

[Robert Richter]

The patch 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      |    8 +++++++-
 2 files changed, 9 insertions(+), 1 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..49540b7 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -423,9 +423,15 @@ static void __init setup_xstate_init(void)
  */
 static void __cpuinit xstate_enable_boot_cpu(void)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
 	unsigned int eax, ebx, ecx, edx;
 
-	cpuid_count(0xd, 0, &eax, &ebx, &ecx, &edx);
+	if (c->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) {
-- 
1.7.1.1

Re: [PATCH 09/10] x86, xsave: check cpuid level for XSTATE_CPUID (0x0d)

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> The patch adds a check that tests if XSTATE_CPUID exists.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>

While we are at this, can you please rename the other hardcoded 0xd's
with XSTATE_CPUID in xsave.c?

> + struct cpuinfo_x86 *c = &cpu_data(0);

Can we use boot_cpu_data instead of cpu_data(0).

thanks.

Re: [PATCH 09/10] x86, xsave: check cpuid level for XSTATE_CPUID (0x0d)

[H. Peter Anvin]

On 07/20/2010 03:26 PM, Suresh Siddha wrote:
> On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
>> The patch adds a check that tests if XSTATE_CPUID exists.
>>
>> Signed-off-by: Robert Richter <robert.richter@amd.com>
> 
> While we are at this, can you please rename the other hardcoded 0xd's
> with XSTATE_CPUID in xsave.c?
> 
>> + struct cpuinfo_x86 *c = &cpu_data(0);
> 
> Can we use boot_cpu_data instead of cpu_data(0).
> 

Not the same thing!

boot_cpu_data is the intersection of all the boot CPU features (and yes,
it's misnamed); cpu_data(0) is the CPU data for CPU 0.

	-hpa

Re: [PATCH 09/10] x86, xsave: check cpuid level for XSTATE_CPUID (0x0d)

[H. Peter Anvin]

On 07/20/2010 03:26 PM, Suresh Siddha wrote:
> On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
>> The patch adds a check that tests if XSTATE_CPUID exists.
>>
>> Signed-off-by: Robert Richter <robert.richter@amd.com>
> 
> While we are at this, can you please rename the other hardcoded 0xd's
> with XSTATE_CPUID in xsave.c?
> 
>> + struct cpuinfo_x86 *c = &cpu_data(0);
> 
> Can we use boot_cpu_data instead of cpu_data(0).
> 

I think boot_cpu_data is the right thing for the early stuff like this
(early_cpu_init() initializes boot_cpu_data, not cpu_data(0)) so just
use boot_cpu_data.cpuid_level instead of forming a pointer.

	-hpa

[PATCH 10/10] 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 49540b7..e7f3d39 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

Re: [PATCH 10/10] x86, xsave: make init_xstate_buf static

[Suresh Siddha]

On Tue, 2010-07-20 at 11:50 -0700, Robert Richter wrote:
> The pointer is only used in xsave.c. Making it static.
> 
> Signed-off-by: Robert Richter <robert.richter@amd.com>

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Tue, Jul 20, 2010 at 08:50:47PM +0200, Robert Richter wrote:
>
> This patch series contains some cleanups and reworks I made during
> code review and feature implementation for upcoming cpus.
> 
> Most patches refactor the xsave initialization that is very dependent
> on fpu initialization. This series starts to decouple this a little
> bit as xsave not only supports fpu features. Also this is an attempt
> to ease the xsave interface by making some of the functions and
> variables static.
> 
> There is also one patch that removes boot_cpu_id variable, which is
> not really related to xsave. Maybe this should be applied to another
> branch.
> 
> The patches are relative to today's tip/x86/xsave branch.
> 
> (The patches are small for better review and rebasing.)
> 
> -Robert
> 

Hi Robert, I recall there was a thread related to boot_cpu_id and
cpu = 0. Unfortunately I can't find it neither in my mbox nor somewhere
in net at moment. Ie technically speaking -- yes boot_cpu_id will be 0
but perhaps instead of magic !cpu and friends explicit boot_cpu_id might
be better for code reading. It might be is_boot_cpu() macro helper or
so as well.

Though I don't have strong opinion but for ones who will be
reading the code first time it might be confusing :) Agreed?

	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Robert Richter]

On 20.07.10 15:27:17, Cyrill Gorcunov wrote:
> On Tue, Jul 20, 2010 at 08:50:47PM +0200, Robert Richter wrote:
> >
> > This patch series contains some cleanups and reworks I made during
> > code review and feature implementation for upcoming cpus.
> > 
> > Most patches refactor the xsave initialization that is very dependent
> > on fpu initialization. This series starts to decouple this a little
> > bit as xsave not only supports fpu features. Also this is an attempt
> > to ease the xsave interface by making some of the functions and
> > variables static.
> > 
> > There is also one patch that removes boot_cpu_id variable, which is
> > not really related to xsave. Maybe this should be applied to another
> > branch.
> > 
> > The patches are relative to today's tip/x86/xsave branch.
> > 
> > (The patches are small for better review and rebasing.)
> > 
> > -Robert
> > 
> 
> Hi Robert, I recall there was a thread related to boot_cpu_id and
> cpu = 0. Unfortunately I can't find it neither in my mbox nor somewhere
> in net at moment.

I found this patch:

 b3572e3 x86/voyager: fix compile breakage caused by dc1e35c6e95e8923cf1d3510438b63c600fee1e2

indicating that boot cpu id could be different than 0.

But either this is broken again, or the issue is gone in a different
way.

> Ie technically speaking -- yes boot_cpu_id will be 0
> but perhaps instead of magic !cpu and friends explicit boot_cpu_id might
> be better for code reading. It might be is_boot_cpu() macro helper or
> so as well.
> 
> Though I don't have strong opinion but for ones who will be
> reading the code first time it might be confusing :) Agreed?

That's true, but once you know...

I could make a follow on patch with an is_boot_cpu() macro. Ingo, what
do you think?

But first question is, is it always !smp_processor_id()? At least
current implementation indicates this:

 void __cpuinit identify_secondary_cpu(struct cpuinfo_x86 *c)
 {
 	BUG_ON(c == &boot_cpu_data);
 	...

with:

 #define boot_cpu_data cpu_data[0]

... which is valid for 32 and 64 bit.

-Robert

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Suresh Siddha]

On Tue, 2010-07-20 at 12:46 -0700, Robert Richter wrote:
> On 20.07.10 15:27:17, Cyrill Gorcunov wrote:
> > On Tue, Jul 20, 2010 at 08:50:47PM +0200, Robert Richter wrote:
> > >
> > > This patch series contains some cleanups and reworks I made during
> > > code review and feature implementation for upcoming cpus.
> > > 
> > > Most patches refactor the xsave initialization that is very dependent
> > > on fpu initialization. This series starts to decouple this a little
> > > bit as xsave not only supports fpu features. Also this is an attempt
> > > to ease the xsave interface by making some of the functions and
> > > variables static.
> > > 
> > > There is also one patch that removes boot_cpu_id variable, which is
> > > not really related to xsave. Maybe this should be applied to another
> > > branch.
> > > 
> > > The patches are relative to today's tip/x86/xsave branch.
> > > 
> > > (The patches are small for better review and rebasing.)
> > > 
> > > -Robert
> > > 
> > 
> > Hi Robert, I recall there was a thread related to boot_cpu_id and
> > cpu = 0. Unfortunately I can't find it neither in my mbox nor somewhere
> > in net at moment.
> 
> I found this patch:
> 
>  b3572e3 x86/voyager: fix compile breakage caused by dc1e35c6e95e8923cf1d3510438b63c600fee1e2
> 
> indicating that boot cpu id could be different than 0.
> 
> But either this is broken again, or the issue is gone in a different
> way.

Voyager code was removed from the tree since then.

thanks
suresh

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Tue, Jul 20, 2010 at 09:46:06PM +0200, Robert Richter wrote:
> On 20.07.10 15:27:17, Cyrill Gorcunov wrote:
> > On Tue, Jul 20, 2010 at 08:50:47PM +0200, Robert Richter wrote:
> > >
> > > This patch series contains some cleanups and reworks I made during
> > > code review and feature implementation for upcoming cpus.
> > > 
> > > Most patches refactor the xsave initialization that is very dependent
> > > on fpu initialization. This series starts to decouple this a little
> > > bit as xsave not only supports fpu features. Also this is an attempt
> > > to ease the xsave interface by making some of the functions and
> > > variables static.
> > > 
> > > There is also one patch that removes boot_cpu_id variable, which is
> > > not really related to xsave. Maybe this should be applied to another
> > > branch.
> > > 
> > > The patches are relative to today's tip/x86/xsave branch.
> > > 
> > > (The patches are small for better review and rebasing.)
> > > 
> > > -Robert
> > > 
> > 
> > Hi Robert, I recall there was a thread related to boot_cpu_id and
> > cpu = 0. Unfortunately I can't find it neither in my mbox nor somewhere
> > in net at moment.
> 
> I found this patch:
> 
>  b3572e3 x86/voyager: fix compile breakage caused by dc1e35c6e95e8923cf1d3510438b63c600fee1e2
> 
> indicating that boot cpu id could be different than 0.
> 

yeah, I forgot about voyager indeed but seems this is quite specific
to voyager trick

> But either this is broken again, or the issue is gone in a different
> way.
> 
> > Ie technically speaking -- yes boot_cpu_id will be 0
> > but perhaps instead of magic !cpu and friends explicit boot_cpu_id might
> > be better for code reading. It might be is_boot_cpu() macro helper or
> > so as well.
> > 
> > Though I don't have strong opinion but for ones who will be
> > reading the code first time it might be confusing :) Agreed?
> 
> That's true, but once you know...
> 

yes, but before you know ;)

> I could make a follow on patch with an is_boot_cpu() macro. Ingo, what
> do you think?
> 
> But first question is, is it always !smp_processor_id()? At least
> current implementation indicates this:
> 

I guess so, since it's assigned from boot_cpu_id iirc

>  void __cpuinit identify_secondary_cpu(struct cpuinfo_x86 *c)
>  {
>  	BUG_ON(c == &boot_cpu_data);
>  	...
> 
> with:
> 
>  #define boot_cpu_data cpu_data[0]
> 
> ... which is valid for 32 and 64 bit.
> 

I suppose this is just self-protection for "what if something will go
wrong and this will be called on non-BP cpu".

> -Robert
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> 
	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 12:07:29AM +0400, Cyrill Gorcunov wrote:
...
> > 
> > But first question is, is it always !smp_processor_id()? At least
> > current implementation indicates this:
> > 
> 
> I guess so, since it's assigned from boot_cpu_id iirc
> 

well, not true, this id is being set in setup_per_cpu_areas()
note the snippet

		if (cpu == boot_cpu_id)
			switch_to_new_gdt(cpu);

but cycle of assignment is done over all possible cpus so
smp_processor_id will be = 0 for BP but definitely it's
confusing and better to check for BP via explicit cpu == boot_cpu_id
I think. Though I might be missing something.

> >  void __cpuinit identify_secondary_cpu(struct cpuinfo_x86 *c)
> >  {
> >  	BUG_ON(c == &boot_cpu_data);
> >  	...
> > 
> > with:
> > 
> >  #define boot_cpu_data cpu_data[0]
> > 
> > ... which is valid for 32 and 64 bit.
> >
 
	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Robert Richter]

On 20.07.10 16:17:40, Cyrill Gorcunov wrote:

> note the snippet
> 
> 		if (cpu == boot_cpu_id)
> 			switch_to_new_gdt(cpu);
> 
> but cycle of assignment is done over all possible cpus so
> smp_processor_id will be = 0 for BP but definitely it's
> confusing and better to check for BP via explicit cpu == boot_cpu_id
> I think. Though I might be missing something.

This in smpboot.c makes it clear:

void __cpuinit smp_store_cpu_info(int id)
{
        struct cpuinfo_x86 *c = &cpu_data(id);

        copy_cpuinfo_x86(c, &boot_cpu_data);
        c->cpu_index = id;
        if (id != 0)
                identify_secondary_cpu(c);
}

So boot cpu id is always 0.

Also note, as Hans Peter already pointed out, this for CONFIG_SMP:

 &cpu_data(0) != &boot_cpu_data

The data in boot_cpu_data is (partly) already available after
early_cpu_init(). It is later copied to the &cpu_data() structures. So
boot_cpu_data should be used for init code.

Also, to make the test obviously, instead of testing (cpu ==
boot_cpu_id) I rather tend to use an is_boot_cpu() macro as you
suggested in your earlier mail.

-Robert

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 06:16:54PM +0200, Robert Richter wrote:
> On 20.07.10 16:17:40, Cyrill Gorcunov wrote:
> 
> > note the snippet
> > 
> > 		if (cpu == boot_cpu_id)
> > 			switch_to_new_gdt(cpu);
> > 
> > but cycle of assignment is done over all possible cpus so
> > smp_processor_id will be = 0 for BP but definitely it's
> > confusing and better to check for BP via explicit cpu == boot_cpu_id
> > I think. Though I might be missing something.
> 
> This in smpboot.c makes it clear:
> 
> void __cpuinit smp_store_cpu_info(int id)
> {
>         struct cpuinfo_x86 *c = &cpu_data(id);
> 
>         copy_cpuinfo_x86(c, &boot_cpu_data);
>         c->cpu_index = id;
>         if (id != 0)
>                 identify_secondary_cpu(c);
> }
> 
> So boot cpu id is always 0.

yeah, thanks!

> 
> Also note, as Hans Peter already pointed out, this for CONFIG_SMP:
> 
>  &cpu_data(0) != &boot_cpu_data
> 
> The data in boot_cpu_data is (partly) already available after
> early_cpu_init(). It is later copied to the &cpu_data() structures. So
> boot_cpu_data should be used for init code.
> 
> Also, to make the test obviously, instead of testing (cpu ==
> boot_cpu_id) I rather tend to use an is_boot_cpu() macro as you
> suggested in your earlier mail.
> 
> -Robert
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> 

ok, thanks Robert!

	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[H. Peter Anvin]

On 07/20/2010 01:17 PM, Cyrill Gorcunov wrote:
> 
> well, not true, this id is being set in setup_per_cpu_areas()
> note the snippet
> 
> 		if (cpu == boot_cpu_id)
> 			switch_to_new_gdt(cpu);
> 
> but cycle of assignment is done over all possible cpus so
> smp_processor_id will be = 0 for BP but definitely it's
> confusing and better to check for BP via explicit cpu == boot_cpu_id
> I think. Though I might be missing something.
> 

I think the style (!smp_processor_id()) is already in use in other
places, but we should be consistent in style; if you want to introduce a
new style I certainly agree that (is_boot_cpu()) is pretty clear but it
should be introduced universally.

	-hpa

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 09:32:35AM -0700, H. Peter Anvin wrote:
> On 07/20/2010 01:17 PM, Cyrill Gorcunov wrote:
> > 
> > well, not true, this id is being set in setup_per_cpu_areas()
> > note the snippet
> > 
> > 		if (cpu == boot_cpu_id)
> > 			switch_to_new_gdt(cpu);
> > 
> > but cycle of assignment is done over all possible cpus so
> > smp_processor_id will be = 0 for BP but definitely it's
> > confusing and better to check for BP via explicit cpu == boot_cpu_id
> > I think. Though I might be missing something.
> > 
> 
> I think the style (!smp_processor_id()) is already in use in other
> places, but we should be consistent in style; if you want to introduce a
> new style I certainly agree that (is_boot_cpu()) is pretty clear but it
> should be introduced universally.
> 
> 	-hpa
>

yes, also I bet there will be places with patterns like

	cpu = smp_processor_id();
	if (!cpu)
	 or
	if (cpu == 0)

so every single smp_processor_id and "raw" version as well should be
checked. I'll take a look as only get ability. If Robert (or anyone)
will like to beat me on this -- I would be only glad ;)

	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 08:52:11PM +0400, Cyrill Gorcunov wrote:
> On Wed, Jul 21, 2010 at 09:32:35AM -0700, H. Peter Anvin wrote:
> > On 07/20/2010 01:17 PM, Cyrill Gorcunov wrote:
> > > 
> > > well, not true, this id is being set in setup_per_cpu_areas()
> > > note the snippet
> > > 
> > > 		if (cpu == boot_cpu_id)
> > > 			switch_to_new_gdt(cpu);
> > > 
> > > but cycle of assignment is done over all possible cpus so
> > > smp_processor_id will be = 0 for BP but definitely it's
> > > confusing and better to check for BP via explicit cpu == boot_cpu_id
> > > I think. Though I might be missing something.
> > > 
> > 
> > I think the style (!smp_processor_id()) is already in use in other
> > places, but we should be consistent in style; if you want to introduce a
> > new style I certainly agree that (is_boot_cpu()) is pretty clear but it
> > should be introduced universally.
> > 
> > 	-hpa
> >
> 

Peter, also I think such tuning must be done at merge window time only,
just to not break other's patch queues.

	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[H. Peter Anvin]

On 07/21/2010 10:01 AM, Cyrill Gorcunov wrote:
> 
> Peter, also I think such tuning must be done at merge window time only,
> just to not break other's patch queues.
> 

Realistically it should be done right after the merge window for the
*next* merge window.

	-hpa

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 10:11:56AM -0700, H. Peter Anvin wrote:
> On 07/21/2010 10:01 AM, Cyrill Gorcunov wrote:
> > 
> > Peter, also I think such tuning must be done at merge window time only,
> > just to not break other's patch queues.
> > 
> 
> Realistically it should be done right after the merge window for the
> *next* merge window.
> 
> 	-hpa
>

ok, which means Robert should use old conventional test at the moment
instead of introducing is_boot_cpu I suppose.

	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Robert Richter]

On 21.07.10 13:17:18, Cyrill Gorcunov wrote:
> On Wed, Jul 21, 2010 at 10:11:56AM -0700, H. Peter Anvin wrote:
> > On 07/21/2010 10:01 AM, Cyrill Gorcunov wrote:
> > > 
> > > Peter, also I think such tuning must be done at merge window time only,
> > > just to not break other's patch queues.
> > > 
> > 
> > Realistically it should be done right after the merge window for the
> > *next* merge window.
> > 
> > 	-hpa
> >
> 
> ok, which means Robert should use old conventional test at the moment
> instead of introducing is_boot_cpu I suppose.

Yes, I am just using:

if (!smp_processor_id())
	...

Cyrill, if you like, I will leave it up to you to introduce the
is_boot_cpu() macro.

I still have patch 7/7 in my new posting in that removes the
boot_cpu_id. If your patch comes soon this will be obsolete as all
places will have the macro then.

Thanks,

-Robert

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

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 07:24:28PM +0200, Robert Richter wrote:
> On 21.07.10 13:17:18, Cyrill Gorcunov wrote:
> > On Wed, Jul 21, 2010 at 10:11:56AM -0700, H. Peter Anvin wrote:
> > > On 07/21/2010 10:01 AM, Cyrill Gorcunov wrote:
> > > > 
> > > > Peter, also I think such tuning must be done at merge window time only,
> > > > just to not break other's patch queues.
> > > > 
> > > 
> > > Realistically it should be done right after the merge window for the
> > > *next* merge window.
> > > 
> > > 	-hpa
> > >
> > 
> > ok, which means Robert should use old conventional test at the moment
> > instead of introducing is_boot_cpu I suppose.
> 
> Yes, I am just using:
> 
> if (!smp_processor_id())
> 	...

ok, I see

> 
> Cyrill, if you like, I will leave it up to you to introduce the
> is_boot_cpu() macro.

ok Robert, I may handle it, notes below

> 
> I still have patch 7/7 in my new posting in that removes the
> boot_cpu_id. If your patch comes soon this will be obsolete as all
> places will have the macro then.
> 

I guess I can make it in a hour or so (since need to check all possible
places) but it might break other's queue I fear. That is why hpa noted
such things should go after merge window.

So I don't know Robert what would be preferred. If Peter pick up your
patch for now -- I may just keep it somewhere localy to not loose the traces of
boot_cpu_id and use this patch as helper. I just fear I can forget
about this promise to introduce is_boot_cpu helper later ;)

Another option could be -- introduce it now and send to trivial@ ML
so it'll be picked up there and pushed upstream after merge window.

> Thanks,
> 
> -Robert
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> 
	-- Cyrill

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[Cyrill Gorcunov]

On Wed, Jul 21, 2010 at 07:24:28PM +0200, Robert Richter wrote:
> On 21.07.10 13:17:18, Cyrill Gorcunov wrote:
> > On Wed, Jul 21, 2010 at 10:11:56AM -0700, H. Peter Anvin wrote:
> > > On 07/21/2010 10:01 AM, Cyrill Gorcunov wrote:
> > > > 
> > > > Peter, also I think such tuning must be done at merge window time only,
> > > > just to not break other's patch queues.
> > > > 
> > > 
> > > Realistically it should be done right after the merge window for the
> > > *next* merge window.
> > > 
> > > 	-hpa
> > >
> > 
> > ok, which means Robert should use old conventional test at the moment
> > instead of introducing is_boot_cpu I suppose.
> 
> Yes, I am just using:
> 
> if (!smp_processor_id())
> 	...
> 
> Cyrill, if you like, I will leave it up to you to introduce the
> is_boot_cpu() macro.
> 
> I still have patch 7/7 in my new posting in that removes the
> boot_cpu_id. If your patch comes soon this will be obsolete as all
> places will have the macro then.
> 
> Thanks,
> 
> -Robert
> 
> -- 
> Advanced Micro Devices, Inc.
> Operating System Research Center
> 

Robert, Peter, it might be something like the patch
below. I've compile-tested it only. On top of current
-tip with Robert's patch applied. If you find it convenient
we could queue it somewhere and give it a shot. Complains
are welcome. Initially I thought about kepping boot_cpu_id
as macro too (to be consistent with say IA-64) but not sure
so I drop this idea.

	-- Cyrill
---
x86: Introduce is_boot_cpu() helper

This allow us to make clear the code snippets where we
test for cpu being bootstrap one instead of open coded test
with zero.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Robert Richter <robert.richter@amd.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/include/asm/apb_timer.h |    2 ++
 arch/x86/include/asm/cpu.h       |    1 -
 arch/x86/include/asm/smp.h       |    2 ++
 arch/x86/kernel/apb_timer.c      |    2 +-
 arch/x86/kernel/apic/apic.c      |    6 +++---
 arch/x86/kernel/apic/nmi.c       |    2 +-
 arch/x86/kernel/apm_32.c         |    2 +-
 arch/x86/kernel/cpu/amd.c        |    2 +-
 arch/x86/kernel/cpu/common.c     |    2 +-
 arch/x86/kernel/cpu/intel.c      |    2 +-
 arch/x86/kernel/i387.c           |    2 +-
 arch/x86/kernel/setup_percpu.c   |    2 +-
 arch/x86/kernel/smpboot.c        |    4 ++--
 arch/x86/kernel/traps.c          |    2 +-
 arch/x86/xen/smp.c               |    4 ++--
 arch/x86/xen/suspend.c           |    2 +-
 16 files changed, 21 insertions(+), 18 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/apb_timer.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/apb_timer.h
+++ linux-2.6.git/arch/x86/include/asm/apb_timer.h
@@ -59,6 +59,8 @@ extern struct sfi_timer_table_entry *sfi
 extern void sfi_free_mtmr(struct sfi_timer_table_entry *mtmr);
 extern int sfi_mtimer_num;
 
+#define is_boot_cpu(cpu)	((cpu) == 0)
+
 #else /* CONFIG_APB_TIMER */
 
 static inline unsigned long apbt_quick_calibrate(void) {return 0; }
Index: linux-2.6.git/arch/x86/include/asm/cpu.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/cpu.h
+++ linux-2.6.git/arch/x86/include/asm/cpu.h
@@ -32,5 +32,4 @@ extern void arch_unregister_cpu(int);
 
 DECLARE_PER_CPU(int, cpu_state);
 
-
 #endif /* _ASM_X86_CPU_H */
Index: linux-2.6.git/arch/x86/include/asm/smp.h
=====================================================================
--- linux-2.6.git.orig/arch/x86/include/asm/smp.h
+++ linux-2.6.git/arch/x86/include/asm/smp.h
@@ -203,5 +203,7 @@ extern int hard_smp_processor_id(void);
 
 #endif /* CONFIG_X86_LOCAL_APIC */
 
+#define is_boot_cpu(cpu) ((cpu) == 0)
+
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_SMP_H */
Index: linux-2.6.git/arch/x86/kernel/apb_timer.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apb_timer.c
+++ linux-2.6.git/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)
+	if (is_boot_cpu(cpu))
 		return;
 	/*
 	 * We need to calculate the scaled math multiplication factor for
Index: linux-2.6.git/arch/x86/kernel/apic/apic.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/apic.c
+++ linux-2.6.git/arch/x86/kernel/apic/apic.c
@@ -1293,7 +1293,7 @@ void __cpuinit setup_local_APIC(void)
 	 * TODO: set up through-local-APIC from through-I/O-APIC? --macro
 	 */
 	value = apic_read(APIC_LVT0) & APIC_LVT_MASKED;
-	if (!smp_processor_id() && (pic_mode || !value)) {
+	if (is_boot_cpu(smp_processor_id()) && (pic_mode || !value)) {
 		value = APIC_DM_EXTINT;
 		apic_printk(APIC_VERBOSE, "enabled ExtINT on CPU#%d\n",
 				smp_processor_id());
@@ -1307,7 +1307,7 @@ void __cpuinit setup_local_APIC(void)
 	/*
 	 * only the BP should see the LINT1 NMI signal, obviously.
 	 */
-	if (!smp_processor_id())
+	if (is_boot_cpu(smp_processor_id()))
 		value = APIC_DM_NMI;
 	else
 		value = APIC_DM_NMI | APIC_LVT_MASKED;
@@ -1319,7 +1319,7 @@ void __cpuinit setup_local_APIC(void)
 
 #ifdef CONFIG_X86_MCE_INTEL
 	/* Recheck CMCI information after local APIC is up on CPU #0 */
-	if (smp_processor_id() == 0)
+	if (is_boot_cpu(smp_processor_id()))
 		cmci_recheck();
 #endif
 }
Index: linux-2.6.git/arch/x86/kernel/apic/nmi.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apic/nmi.c
+++ linux-2.6.git/arch/x86/kernel/apic/nmi.c
@@ -316,7 +316,7 @@ void setup_apic_nmi_watchdog(void *unuse
 
 	/* cheap hack to support suspend/resume */
 	/* if cpu0 is not active neither should the other cpus */
-	if (smp_processor_id() != 0 && atomic_read(&nmi_active) <= 0)
+	if (!is_boot_cpu(smp_processor_id()) && atomic_read(&nmi_active) <= 0)
 		return;
 
 	switch (nmi_watchdog) {
Index: linux-2.6.git/arch/x86/kernel/apm_32.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/apm_32.c
+++ linux-2.6.git/arch/x86/kernel/apm_32.c
@@ -1746,7 +1746,7 @@ static int apm(void *unused)
 	 * Method suggested by Ingo Molnar.
 	 */
 	set_cpus_allowed_ptr(current, cpumask_of(0));
-	BUG_ON(smp_processor_id() != 0);
+	BUG_ON(!is_boot_cpu(smp_processor_id());
 
 	if (apm_info.connection_version == 0) {
 		apm_info.connection_version = apm_info.bios.version;
Index: linux-2.6.git/arch/x86/kernel/cpu/amd.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/amd.c
+++ linux-2.6.git/arch/x86/kernel/cpu/amd.c
@@ -148,7 +148,7 @@ static void __cpuinit amd_k7_smp_check(s
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (!c->cpu_index)
+	if (is_boot_cpu(c->cpu_index))
 		return;
 
 	/*
Index: linux-2.6.git/arch/x86/kernel/cpu/common.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/common.c
+++ linux-2.6.git/arch/x86/kernel/cpu/common.c
@@ -1273,7 +1273,7 @@ void __cpuinit cpu_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (smp_processor_id() == boot_cpu_id)
+	if (is_boot_cpu(smp_processor_id())
 		init_thread_xstate();
 
 	xsave_init();
Index: linux-2.6.git/arch/x86/kernel/cpu/intel.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/cpu/intel.c
+++ linux-2.6.git/arch/x86/kernel/cpu/intel.c
@@ -169,7 +169,7 @@ static void __cpuinit intel_smp_check(st
 {
 #ifdef CONFIG_SMP
 	/* calling is from identify_secondary_cpu() ? */
-	if (!c->cpu_index)
+	if (is_boot_cpu(c->cpu_index))
 		return;
 
 	/*
Index: linux-2.6.git/arch/x86/kernel/i387.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/i387.c
+++ linux-2.6.git/arch/x86/kernel/i387.c
@@ -96,7 +96,7 @@ void __cpuinit fpu_init(void)
 	/*
 	 * Boot processor to setup the FP and extended state context info.
 	 */
-	if (!smp_processor_id())
+	if (is_boot_cpu(smp_processor_id()))
 		init_thread_xstate();
 	xsave_init();
 
Index: linux-2.6.git/arch/x86/kernel/setup_percpu.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/setup_percpu.c
+++ linux-2.6.git/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)
+		if (is_boot_cpu(cpu))
 			switch_to_new_gdt(cpu);
 	}
 
Index: linux-2.6.git/arch/x86/kernel/smpboot.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/smpboot.c
+++ linux-2.6.git/arch/x86/kernel/smpboot.c
@@ -369,7 +369,7 @@ void __cpuinit smp_store_cpu_info(int id
 
 	copy_cpuinfo_x86(c, &boot_cpu_data);
 	c->cpu_index = id;
-	if (id != 0)
+	if (!is_boot_cpu(id))
 		identify_secondary_cpu(c);
 }
 
@@ -1319,7 +1319,7 @@ int native_cpu_disable(void)
 	 * interrupts only being able to be serviced by the BSP.
 	 * Especially so if we're not using an IOAPIC	-zwane
 	 */
-	if (cpu == 0)
+	if (is_boot_cpu(cpu))
 		return -EBUSY;
 
 	if (nmi_watchdog == NMI_LOCAL_APIC)
Index: linux-2.6.git/arch/x86/kernel/traps.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/kernel/traps.c
+++ linux-2.6.git/arch/x86/kernel/traps.c
@@ -385,7 +385,7 @@ static notrace __kprobes void default_do
 	cpu = smp_processor_id();
 
 	/* Only the BSP gets external NMIs from the system. */
-	if (!cpu)
+	if (is_boot_cpu(cpu))
 		reason = get_nmi_reason();
 
 	if (!(reason & 0xc0)) {
Index: linux-2.6.git/arch/x86/xen/smp.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/xen/smp.c
+++ linux-2.6.git/arch/x86/xen/smp.c
@@ -167,7 +167,7 @@ static void __init xen_fill_possible_map
 
 static void __init xen_smp_prepare_boot_cpu(void)
 {
-	BUG_ON(smp_processor_id() != 0);
+	BUG_ON(!is_boot_cpu(smp_processor_id()));
 	native_smp_prepare_boot_cpu();
 
 	/* We've switched to the "real" per-cpu gdt, so make sure the
@@ -336,7 +336,7 @@ static void xen_smp_cpus_done(unsigned i
 static int xen_cpu_disable(void)
 {
 	unsigned int cpu = smp_processor_id();
-	if (cpu == 0)
+	if (is_boot_cpu(cpu))
 		return -EBUSY;
 
 	cpu_disable_common();
Index: linux-2.6.git/arch/x86/xen/suspend.c
=====================================================================
--- linux-2.6.git.orig/arch/x86/xen/suspend.c
+++ linux-2.6.git/arch/x86/xen/suspend.c
@@ -52,7 +52,7 @@ static void xen_vcpu_notify_restore(void
 	unsigned long reason = (unsigned long)data;
 
 	/* Boot processor notified via generic timekeeping_resume() */
-	if ( smp_processor_id() == 0)
+	if (is_boot_cpu(smp_processor_id()))
 		return;
 
 	clockevents_notify(reason, NULL);

Re: [PATCH 00/10] x86, xsave: some code cleanups and reworks

[H. Peter Anvin]

On 07/20/2010 11:50 AM, Robert Richter wrote:
> 
> There is also one patch that removes boot_cpu_id variable, which is
> not really related to xsave. Maybe this should be applied to another
> branch.
> 

Probably, yes.

	-hpa