LinuxLists.cc - [PATCH 00/10] x86, xsave: some code cleanups and reworks

[permalink] [raw]

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

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

Signed-off-by: Robert Richter <[email protected]>
---
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

2010-07-20 18:58:33

[permalink] [raw]

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

The patch adds a check that tests if XSTATE_CPUID exists.

Signed-off-by: Robert Richter <[email protected]>
---
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

2010-07-20 18:58:17

[permalink] [raw]

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

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

Signed-off-by: Robert Richter <[email protected]>
---
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

2010-07-20 18:58:54

[permalink] [raw]

Subject: [PATCH 03/10] x86: removing 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 at all.

Signed-off-by: Robert Richter <[email protected]>
---
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

2010-07-20 18:59:00

[permalink] [raw]

Subject: [PATCH 08/10] 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 <[email protected]>
---
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

2010-07-20 18:59:33

[permalink] [raw]

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

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 <[email protected]>
---
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

2010-07-20 18:59:42

[permalink] [raw]

Subject: [PATCH 04/10] x86, xsave: moving 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 <[email protected]>
---
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

2010-07-20 18:59:49

[permalink] [raw]

Subject: [PATCH 02/10] 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 <[email protected]>
---
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

2010-07-20 18:59:28

[permalink] [raw]

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

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 <[email protected]>
---
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

2010-07-20 19:00:32

[permalink] [raw]

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

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 <[email protected]>
---
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

2010-07-20 19:27:25

[permalink] [raw]

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

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

2010-07-20 19:46:21

[permalink] [raw]

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

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

2010-07-20 20:05:50

[permalink] [raw]

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

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

2010-07-20 20:07:36

[permalink] [raw]

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

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

2010-07-20 20:17:46

[permalink] [raw]

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

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

2010-07-20 20:22:36

[permalink] [raw]

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

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

2010-07-20 21:46:46

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

2010-07-20 21:48:16

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

2010-07-20 21:49:19

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

>
> 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();
> +}

2010-07-20 22:21:09

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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();
> }

2010-07-20 22:22:48

[permalink] [raw]

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

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 <[email protected]>

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();
> }

2010-07-20 22:23:16

[permalink] [raw]

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

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 <[email protected]>

Acked-by: Suresh Siddha <[email protected]>

> ---
> 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.
> */

2010-07-20 22:24:31

[permalink] [raw]

Subject: Re: [PATCH 08/10] x86, xsave: introduce xstate enable functions

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 <[email protected]>

Acked-by: Suresh Siddha <[email protected]>

> ---
> 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);
> }

2010-07-20 22:26:40

[permalink] [raw]

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

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 <[email protected]>

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.

2010-07-20 22:27:10

[permalink] [raw]

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

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 <[email protected]>

Acked-by: Suresh Siddha <[email protected]>

2010-07-20 22:45:34

[permalink] [raw]

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

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 <[email protected]>
>
> 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

2010-07-20 22:51:21

[permalink] [raw]

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

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 <[email protected]>
>
> 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

2010-07-21 00:40:26

[permalink] [raw]

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

Commit-ID: db10db48b2c530def21bfd76d576702c7df7f620
Gitweb: http://git.kernel.org/tip/db10db48b2c530def21bfd76d576702c7df7f620
Author: Robert Richter <[email protected]>
AuthorDate: Tue, 20 Jul 2010 20:50:49 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
LKML-Reference: <[email protected]>
Acked-by: Suresh Siddha <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
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();

2010-07-21 00:41:42

[permalink] [raw]

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

Commit-ID: 82d4150cec83b9775f84810b39a1c0b91585d429
Gitweb: http://git.kernel.org/tip/82d4150cec83b9775f84810b39a1c0b91585d429
Author: Robert Richter <[email protected]>
AuthorDate: Tue, 20 Jul 2010 20:50:51 +0200
Committer: H. Peter Anvin <[email protected]>
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 <[email protected]>
LKML-Reference: <[email protected]>
Acked-by: Suresh Siddha <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
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();
+}

2010-07-21 13:48:21

[permalink] [raw]

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

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

2010-07-21 16:16:26

[permalink] [raw]

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

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.

2010-07-21 16:17:12

[permalink] [raw]

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

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

2010-07-21 16:30:05

[permalink] [raw]

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

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

2010-07-21 16:32:52

[permalink] [raw]

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

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.

2010-07-21 16:52:22

[permalink] [raw]

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

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

2010-07-21 17:01:30

[permalink] [raw]

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

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

2010-07-21 17:12:14

[permalink] [raw]

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

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.

2010-07-21 17:17:27

[permalink] [raw]

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

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

2010-07-21 17:24:40

[permalink] [raw]

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

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

2010-07-21 17:37:30

[permalink] [raw]

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

2010-07-21 19:14:45