[RFC PATCH 0/3] Clean up x86 CPU feature setup

[RFC PATCH 0/3] Clean up x86 CPU feature setup

[Chuck Ebbert]

x86 CPU feature flag setup has become impossible to debug.
Every user just does set_bit()/clear_bit() or writes the
entire set to change the flags, so there's no way to trace
how they're being set.

This patchset creates an API and debug messages for tracking
how the flags get set. It's not nearly done, but I want to
know whether or not to continue.

[RFC PATCH 1/3] x86: use defined names for all CPU feature flags

[Chuck Ebbert]

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



[-- Attachment #2: x86_name_that_flag.patch --]
[-- Type: text/plain, Size: 4395 bytes --]

x86: use defined names for all CPU feature flags

Don't use hard coded values for CPU flags.

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
---
 arch/i386/kernel/cpu/amd.c      |    2 +-
 arch/i386/kernel/cpu/centaur.c  |    2 +-
 arch/i386/kernel/cpu/cyrix.c    |    6 +++---
 arch/x86_64/kernel/setup.c      |    2 +-
 include/asm-i386/cpufeature.h   |    4 +++-
 include/asm-x86_64/cpufeature.h |    1 +
 6 files changed, 10 insertions(+), 7 deletions(-)

--- 2.6.21-rc7-d390.orig/arch/x86_64/kernel/setup.c
+++ 2.6.21-rc7-d390/arch/x86_64/kernel/setup.c
@@ -576,7 +576,7 @@ static void __cpuinit init_amd(struct cp
 
 	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
 	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
-	clear_bit(0*32+31, &c->x86_capability);
+	clear_bit(X86_FEATURE_PBE, &c->x86_capability);
 	
 	/* On C+ stepping K8 rep microcode works well for copy/memset */
 	level = cpuid_eax(1);
--- 2.6.21-rc7-d390.orig/include/asm-i386/cpufeature.h
+++ 2.6.21-rc7-d390/include/asm-i386/cpufeature.h
@@ -42,6 +42,7 @@
 #define X86_FEATURE_HT		(0*32+28) /* Hyper-Threading */
 #define X86_FEATURE_ACC		(0*32+29) /* Automatic clock control */
 #define X86_FEATURE_IA64	(0*32+30) /* IA-64 processor */
+#define X86_FEATURE_PBE		(0*32+31) /* PBE */
 
 /* AMD-defined CPU features, CPUID level 0x80000001, word 1 */
 /* Don't duplicate feature flags which are redundant with Intel! */
@@ -49,6 +50,7 @@
 #define X86_FEATURE_MP		(1*32+19) /* MP Capable. */
 #define X86_FEATURE_NX		(1*32+20) /* Execute Disable */
 #define X86_FEATURE_MMXEXT	(1*32+22) /* AMD MMX extensions */
+#define X86_FEATURE_CXMMXORIG	(1*32+24) /* Cyrix MMX, initial location */
 #define X86_FEATURE_LM		(1*32+29) /* Long Mode (x86-64) */
 #define X86_FEATURE_3DNOWEXT	(1*32+30) /* AMD 3DNow! extensions */
 #define X86_FEATURE_3DNOW	(1*32+31) /* 3DNow! */
@@ -60,7 +62,7 @@
 
 /* Other features, Linux-defined mapping, word 3 */
 /* This range is used for feature bits which conflict or are synthesized */
-#define X86_FEATURE_CXMMX	(3*32+ 0) /* Cyrix MMX extensions */
+#define X86_FEATURE_CXMMX	(3*32+ 0) /* Cyrix MMX extensions, final location */
 #define X86_FEATURE_K6_MTRR	(3*32+ 1) /* AMD K6 nonstandard MTRRs */
 #define X86_FEATURE_CYRIX_ARR	(3*32+ 2) /* Cyrix ARRs (= MTRRs) */
 #define X86_FEATURE_CENTAUR_MCR	(3*32+ 3) /* Centaur MCRs (= MTRRs) */
--- 2.6.21-rc7-d390.orig/arch/i386/kernel/cpu/cyrix.c
+++ 2.6.21-rc7-d390/arch/i386/kernel/cpu/cyrix.c
@@ -192,11 +192,11 @@ static void __cpuinit init_cyrix(struct 
 
 	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
 	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
-	clear_bit(0*32+31, c->x86_capability);
+	clear_bit(X86_FEATURE_PBE, c->x86_capability);
 
 	/* Cyrix used bit 24 in extended (AMD) CPUID for Cyrix MMX extensions */
-	if ( test_bit(1*32+24, c->x86_capability) ) {
-		clear_bit(1*32+24, c->x86_capability);
+	if ( test_bit(X86_FEATURE_CXMMXORIG, c->x86_capability) ) {
+		clear_bit(X86_FEATURE_CXMMXORIG, c->x86_capability);
 		set_bit(X86_FEATURE_CXMMX, c->x86_capability);
 	}
 
--- 2.6.21-rc7-d390.orig/include/asm-x86_64/cpufeature.h
+++ 2.6.21-rc7-d390/include/asm-x86_64/cpufeature.h
@@ -40,6 +40,7 @@
 #define X86_FEATURE_HT		(0*32+28) /* Hyper-Threading */
 #define X86_FEATURE_ACC		(0*32+29) /* Automatic clock control */
 #define X86_FEATURE_IA64	(0*32+30) /* IA-64 processor */
+#define X86_FEATURE_PBE		(0*32+31) /* PBE */
 
 /* AMD-defined CPU features, CPUID level 0x80000001, word 1 */
 /* Don't duplicate feature flags which are redundant with Intel! */
--- 2.6.21-rc7-d390.orig/arch/i386/kernel/cpu/amd.c
+++ 2.6.21-rc7-d390/arch/i386/kernel/cpu/amd.c
@@ -83,7 +83,7 @@ static void __cpuinit init_amd(struct cp
 
 	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
 	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
-	clear_bit(0*32+31, c->x86_capability);
+	clear_bit(X86_FEATURE_PBE, c->x86_capability);
 	
 	r = get_model_name(c);
 
--- 2.6.21-rc7-d390.orig/arch/i386/kernel/cpu/centaur.c
+++ 2.6.21-rc7-d390/arch/i386/kernel/cpu/centaur.c
@@ -334,7 +334,7 @@ static void __cpuinit init_centaur(struc
 
 	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
 	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
-	clear_bit(0*32+31, c->x86_capability);
+	clear_bit(X86_FEATURE_PBE, c->x86_capability);
 
 	switch (c->x86) {
 

Re: [RFC PATCH 1/3] x86: use defined names for all CPU feature flags

[Dave Jones]

On Fri, Apr 20, 2007 at 06:52:52PM -0400, Chuck Ebbert wrote:

 > --- 2.6.21-rc7-d390.orig/arch/x86_64/kernel/setup.c
 > +++ 2.6.21-rc7-d390/arch/x86_64/kernel/setup.c
 > @@ -576,7 +576,7 @@ static void __cpuinit init_amd(struct cp
 >  
 >  	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
 >  	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
 > -	clear_bit(0*32+31, &c->x86_capability);
 > +	clear_bit(X86_FEATURE_PBE, &c->x86_capability);

The comment suggests this should be X86_FEATURE_3DNOW
Same for the other instances.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [RFC PATCH 1/3] x86: use defined names for all CPU feature flags

[Andi Kleen]

On Saturday 21 April 2007 00:52:52 Chuck Ebbert wrote:
>         /* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
>            3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
> -       clear_bit(0*32+31, &c->x86_capability);
> +       clear_bit(X86_FEATURE_PBE, &c->x86_capability);
>         


That doesn't match the comment.

-Andi

Re: [RFC PATCH 1/3] x86: use defined names for all CPU feature flags

[Alan Cox]

> --- 2.6.21-rc7-d390.orig/arch/x86_64/kernel/setup.c
> +++ 2.6.21-rc7-d390/arch/x86_64/kernel/setup.c
> @@ -576,7 +576,7 @@ static void __cpuinit init_amd(struct cp
>  
>  	/* Bit 31 in normal CPUID used for nonstandard 3DNow ID;
>  	   3DNow is IDd by bit 31 in extended CPUID (1*32+31) anyway */
> -	clear_bit(0*32+31, &c->x86_capability);
> +	clear_bit(X86_FEATURE_PBE, &c->x86_capability);

And this is more clear why ?

> +#define X86_FEATURE_PBE		(0*32+31) /* PBE */

For these platforms it isn't "PBE" its 3DNow

Alan

[RFC PATCH 2/3] x86: new API for modifying CPU feature flags

[Chuck Ebbert]

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



[-- Attachment #2: x86_cpufeature_api.patch --]
[-- Type: text/plain, Size: 1467 bytes --]

x86: new API for modifying CPU feature flags

Use an API for setting/clearing CPU features, so the
process can be debugged.

Adds:
set_cpu_feature()
clear_cpu_feature()
clear_all_cpu_features()

Todo:
mask_boot_cpu_features()
set_cpu_feature_word()
more?

(Hardcoded printk for now, should be dprintk.)

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
---
 include/asm-i386/cpufeature.h |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

--- 2.6.21-rc7-d390.orig/include/asm-i386/cpufeature.h
+++ 2.6.21-rc7-d390/include/asm-i386/cpufeature.h
@@ -108,6 +108,23 @@
 #define cpu_has(c, bit)		test_bit(bit, (c)->x86_capability)
 #define boot_cpu_has(bit)	test_bit(bit, boot_cpu_data.x86_capability)
 
+#define alter_cpu_feature(fn, feat, c) \
+		do {    typeof(c) __c = (c); \
+			printk("CPU: %s: %s feature %s for CPU %p", \
+				__func__, #fn, #feat, __c); \
+			fn ## _bit(X86_FEATURE_ ## feat, __c->x86_capability); \
+		} while (0)
+
+#define set_cpu_feature(f, c)	alter_cpu_feature(set, f, c)
+#define clear_cpu_feature(f, c)	alter_cpu_feature(clear, f, c)
+
+#define clear_all_cpu_features(c) \
+		do {    typeof(c) __c = (c); \
+			printk("CPU: %s: clearing all capabilities for CPU %p", \
+				__func__, __c); \
+			memset(&__c->x86_capability, 0, sizeof __c->x86_capability); \
+		} while (0)
+
 #define cpu_has_fpu		boot_cpu_has(X86_FEATURE_FPU)
 #define cpu_has_vme		boot_cpu_has(X86_FEATURE_VME)
 #define cpu_has_de		boot_cpu_has(X86_FEATURE_DE)

[RFC PATCH 3/3] x86: use the x86 CPU feature API

[Chuck Ebbert]

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



[-- Attachment #2: x86_use_cpu_api.patch --]
[-- Type: text/plain, Size: 1122 bytes --]

x86: use the x86 CPU feature API

Just a small demo for now.

Signed-off-by: Chuck Ebbert <cebbert@redhat.com>
---
 arch/i386/kernel/cpu/amd.c    |    4 ++--
 arch/i386/kernel/cpu/common.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

--- 2.6.21-rc7-d390.orig/arch/i386/kernel/cpu/amd.c
+++ 2.6.21-rc7-d390/arch/i386/kernel/cpu/amd.c
@@ -109,8 +109,8 @@ static void __cpuinit init_amd(struct cp
 			{
 				/* Based on AMD doc 20734R - June 2000 */
 				if ( c->x86_model == 0 ) {
-					clear_bit(X86_FEATURE_APIC, c->x86_capability);
-					set_bit(X86_FEATURE_PGE, c->x86_capability);
+					clear_cpu_feature(APIC, c);
+					set_cpu_feature(PGE, c);
 				}
 				break;
 			}
--- 2.6.21-rc7-d390.orig/arch/i386/kernel/cpu/common.c
+++ 2.6.21-rc7-d390/arch/i386/kernel/cpu/common.c
@@ -381,7 +381,7 @@ void __cpuinit identify_cpu(struct cpuin
 	c->x86_model_id[0] = '\0';  /* Unset */
 	c->x86_max_cores = 1;
 	c->x86_clflush_size = 32;
-	memset(&c->x86_capability, 0, sizeof c->x86_capability);
+	clear_all_cpu_features(c);
 
 	if (!have_cpuid_p()) {
 		/* First of all, decide if this is a 486 or higher */

Re: [RFC PATCH 0/3] Clean up x86 CPU feature setup

[Andi Kleen]

On Saturday 21 April 2007 00:49:48 Chuck Ebbert wrote:
> x86 CPU feature flag setup has become impossible to debug.
> Every user just does set_bit()/clear_bit() or writes the
> entire set to change the flags, so there's no way to trace
> how they're being set.

Just use grep or printk? It is not *that* complicated. 

> This patchset creates an API and debug messages for tracking
> how the flags get set. It's not nearly done, but I want to
> know whether or not to continue.

I don't see any particular value. In theory we could add a "debug API"
for nearly everything, but in practice it is usually not needed.
Like here.

-Andi