Skip to contentSkip to conversation

Clean up x86 CPU feature setup

RFC PATCH In review Chuck Ebbert 4 participants 8 messages Apr 20, 2007 – Apr 21, 2007

lore
Share & tools
lore
#0 lore raw

cover Clean up x86 CPU feature setup

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.

#1 lore raw

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

x86: use defined names for all CPU feature flags

Don't use hard coded values for CPU flags.

---
 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(-)
arch/x86_64/kernel/setup.c +1 -1
@@ -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);
include/asm-i386/cpufeature.h +3 -1
@@ -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) */
arch/i386/kernel/cpu/cyrix.c +3 -3
@@ -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);
 	}
 
include/asm-x86_64/cpufeature.h +1 -0
@@ -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! */
arch/i386/kernel/cpu/amd.c +1 -1
@@ -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);
 
arch/i386/kernel/cpu/centaur.c +1 -1
@@ -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) {
 
#5 lore raw

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

In reply to Chuck Ebbert

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

---
 include/asm-i386/cpufeature.h |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
include/asm-i386/cpufeature.h +17 -0
@@ -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)
#6 lore raw

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

In reply to Chuck Ebbert

x86: use the x86 CPU feature API

Just a small demo for now.

---
 arch/i386/kernel/cpu/amd.c    |    4 ++--
 arch/i386/kernel/cpu/common.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
arch/i386/kernel/cpu/amd.c +2 -2
@@ -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;
 			}
arch/i386/kernel/cpu/common.c +1 -1
@@ -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 */
#2 lore raw

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

In reply to Chuck Ebbert

arch/x86_64/kernel/setup.c static void __cpuinit init_amd(struct cp · line 579

Quoted 8 lines · Chuck Ebbert
arch/x86_64/kernel/setup.c +1 -1
@@ -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

Signature · 1 line
http://www.codemonkey.org.uk
#7 lore raw
In reply to Chuck Ebbert
Quoted 4 lines · 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.

Quoted 3 lines

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

#3 lore raw

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

In reply to Chuck Ebbert
Quoted 5 lines · 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

#4 lore raw

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

In reply to Chuck Ebbert

arch/x86_64/kernel/setup.c static void __cpuinit init_amd(struct cp · line 579

Quoted 8 lines
arch/x86_64/kernel/setup.c +1 -1
@@ -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 ?

arch/x86_64/kernel/setup.c static void __cpuinit init_amd(struct cp · line 580

Quoted 1 line
arch/x86_64/kernel/setup.c +1 -0
+#define X86_FEATURE_PBE		(0*32+31) /* PBE */

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

Alan