Skip to contentSkip to conversation

x86: make clflush a required feature on x86_64

PATCH In review Kyle McMartin 5 participants 11 messages Jan 18, 2008 – Jan 23, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 x86: make clflush a required feature on x86_64

Hopefully nobody will be stupid enough to implement a cpu without it. Frankly, it seems safe enough given we already require SSE2.

This means the compiler can optimise away "if (!cpu_has_clflush)" blocks.

---
 include/asm-x86/cpufeature_64.h     |    3 +++
 include/asm-x86/required-features.h |    4 +++-
 2 files changed, 6 insertions(+), 1 deletions(-)
include/asm-x86/cpufeature_64.h +3 -0
index e18496b..ef11d27 100644
@@ -18,6 +18,9 @@
 #undef  cpu_has_mp
 #define cpu_has_mp             1 /* XXX */
 
+#undef	cpu_has_clflush
+#define	cpu_has_clflush        1
+
 #undef  cpu_has_k6_mtrr
 #define cpu_has_k6_mtrr        0
 
include/asm-x86/required-features.h +3 -1
index 7400d3a..cc27664 100644
@@ -45,6 +45,7 @@
 #define NEED_XMM	(1<<(X86_FEATURE_XMM & 31))
 #define NEED_XMM2	(1<<(X86_FEATURE_XMM2 & 31))
 #define NEED_LM		(1<<(X86_FEATURE_LM & 31))
+#define NEED_CLFLSH	(1<<(X86_FEATURE_CLFLSH & 31))
 #else
 #define NEED_PSE	0
 #define NEED_MSR	0
@@ -53,11 +54,12 @@
 #define NEED_XMM	0
 #define NEED_XMM2	0
 #define NEED_LM		0
+#define NEED_CLFLSH	0
 #endif
 
 #define REQUIRED_MASK0	(NEED_FPU|NEED_PSE|NEED_MSR|NEED_PAE|\
 			 NEED_CX8|NEED_PGE|NEED_FXSR|NEED_CMOV|\
-			 NEED_XMM|NEED_XMM2)
+			 NEED_XMM|NEED_XMM2|NEED_CLFLSH)
 #define SSE_MASK	(NEED_XMM|NEED_XMM2)
 
 #define REQUIRED_MASK1	(NEED_LM|NEED_3DNOW)
Signature · 1 line
1.5.3.7
#1 lore raw
Quoted 17 lines · Kyle McMartin

Hopefully nobody will be stupid enough to implement a cpu without it. Frankly, it seems safe enough given we already require SSE2.

This means the compiler can optimise away "if (!cpu_has_clflush)" blocks.

---
 include/asm-x86/cpufeature_64.h     |    3 +++
 include/asm-x86/required-features.h |    4 +++-
 2 files changed, 6 insertions(+), 1 deletions(-)
include/asm-x86/cpufeature_64.h +0 -0
index e18496b..ef11d27 100644

include/asm-x86/cpufeature_64.h line 25

Quoted 8 lines
include/asm-x86/cpufeature_64.h +3 -0
@@ -18,6 +18,9 @@
 #undef  cpu_has_mp
 #define cpu_has_mp             1 /* XXX */
 
+#undef	cpu_has_clflush
+#define	cpu_has_clflush        1
+
 #undef  cpu_has_k6_mtrr
 #define cpu_has_k6_mtrr        0

Unnecessary. These overrides are only needed for the anticases (known to be zero) or for some special hacks.

Stuff that have proper CPUID bits get these defined as constants via the
REQUIRED_MASK macros.

-hpa

#2 lore raw

PATCH 1/1 x86_64: remove redundant cpu_has_ definitions

Quoted 3 lines · H. Peter Anvin

Unnecessary. These overrides are only needed for the anticases (known to be zero) or for some special hacks.

Cool, guess that means a bunch of them can go...

Quoted 3 lines

Stuff that have proper CPUID bits get these defined as constants via the REQUIRED_MASK macros.

PSE, PGE, XMM, XMM2, and FXSR are defined as required features, and will be optimized to a constant at compile time. Remove their redundant definitions.

include/asm-x86/cpufeature.h +0 -15
@@ -195,21 +195,6 @@
 #undef  cpu_has_centaur_mcr
 #define cpu_has_centaur_mcr	0
 
-#undef  cpu_has_pse
-#define cpu_has_pse		1
-
-#undef  cpu_has_pge
-#define cpu_has_pge		1
-
-#undef  cpu_has_xmm
-#define cpu_has_xmm		1
-
-#undef  cpu_has_xmm2
-#define cpu_has_xmm2		1
-
-#undef  cpu_has_fxsr
-#define cpu_has_fxsr		1
-
 #endif /* CONFIG_X86_64 */
 
 #endif /* _ASM_X86_CPUFEATURE_H */
Applied
#3 lore raw

Re: [PATCH] x86_64: remove redundant cpu_has_ definitions

Quoted 11 lines · Kyle McMartin
include/asm-x86/cpufeature.h +0 -5
@@ -195,21 +195,6 @@
 #undef  cpu_has_centaur_mcr
 #define cpu_has_centaur_mcr	0
 
-#undef  cpu_has_pse
-#define cpu_has_pse		1
-
-#undef  cpu_has_pge
-#define cpu_has_pge		1

include/asm-x86/cpufeature.h line 212

Quoted 10 lines
include/asm-x86/cpufeature.h +0 -10
-
-#undef  cpu_has_xmm
-#define cpu_has_xmm		1
-
-#undef  cpu_has_xmm2
-#define cpu_has_xmm2		1
-
-#undef  cpu_has_fxsr
-#define cpu_has_fxsr		1
-

thanks, applied.

Ingo

#4 lore raw
In reply to Kyle McMartin
Quoted 5 lines · Kyle McMartin

Hopefully nobody will be stupid enough to implement a cpu without it. Frankly, it seems safe enough given we already require SSE2.

This means the compiler can optimise away "if (!cpu_has_clflush)" blocks.

The original required CPUID bit set for x86-64 came out of very detailed discussions and thinking from AMD. I think they had good reasons to chose the current set. I would only recommend to depart from it for very good reasons and frankly you haven't given any. You're talking about removing two instructions.

One problem that we had in the past is that some simulators only implement the absolutely minimum feature set and you might have well broken one of these with this.

-Andi

#5 lore raw
Quoted 3 lines · Andi Kleen

One problem that we had in the past is that some simulators only implement the absolutely minimum feature set and you might have well broken one of these with this.

Yeah, true. Please ignore the patch folks.

cheers, Kyle

#6 lore raw
Quoted 8 lines · Kyle McMartin
Quoted 3 lines · Andi Kleen

One problem that we had in the past is that some simulators only implement the absolutely minimum feature set and you might have well broken one of these with this.

Yeah, true. Please ignore the patch folks.

cheers, Kyle

Simulators can be fixed, even if it takes time; for something like clflush this is easier since clflush can be implemented as a simple noop for most of them.

I just verified that Bochs 2.3.0 lacks this CPUID bit whereas the current version, 2.3.6, enables CLFLUSH iff SSE2 is enabled. Qemu 0.9.0 has CLFLUSH. Andi, do you happen to know of any specific simulators which are problematic? I would assume any recent version of SimNow is up to date.

-hpa

#7 lore raw
Quoted 1 line

Simulators can be fixed,

They could, but why? I don't know of a good reason to require CLFLUSH.

Quoted 5 lines

I just verified that Bochs 2.3.0 lacks this CPUID bit whereas the current version, 2.3.6, enables CLFLUSH iff SSE2 is enabled. Qemu 0.9.0 has CLFLUSH. Andi, do you happen to know of any specific simulators which are problematic? I would assume any recent version of SimNow is up to date.

I don't know of any specific ones that lack CLFLUSH, although Bochs definitely had similar problems in the past.

-Andi

#8 lore raw
Quoted 3 lines · Andi Kleen
Quoted 1 line

Simulators can be fixed,

They could, but why? I don't know of a good reason to require CLFLUSH.

Well, simulators are generally expected to follow the architecture, not vice versa. I would tend to agree with the coupling that recent versions of Bochs appeared to have made here -- I think we're unlikely to see any processors with sse2 sans clflush, so keeping code branches in which will never be executed seems like a bad idea in the long term.
I'm much more worried about the possibility of embedded 64-bit CPUs
who try to skimp on SSE2 than CLFLUSH.

Now, that being said, this being encapsulated in the required set (and unified, which means the branches will be executed on 32-bit hardware) it seems the impact of leaving them in is small for now. We can re-evaluate that as appropriate. Either way, we should *not* have #ifdef CONFIG_X86_64 around usage sites, circumventing the master switch. It either goes in the required masks or it doesn't.

Quoted 4 lines
Quoted 1 line

I just verified that Bochs 2.3.0 lacks this CPUID bit whereas the current version, 2.3.6, enables CLFLUSH iff SSE2 is enabled. Qemu 0.9.0 has CLFLUSH. Andi, do you happen to know of any specific simulators which are problematic? I would assume any recent version of SimNow is up to date.

I don't know of any specific ones that lack CLFLUSH, although Bochs definitely had similar problems in the past.

OK, so we're talking about outdated versions of Bochs, then?

-hpa

#9 lore raw
Quoted 10 lines · H. Peter Anvin
Quoted 3 lines · Andi Kleen
Quoted 1 line

Simulators can be fixed,

They could, but why? I don't know of a good reason to require CLFLUSH.

Well, simulators are generally expected to follow the architecture, not vice versa. I would tend to agree with the coupling that recent versions of Bochs appeared to have made here -- I think we're unlikely to see any processors with sse2 sans clflush, so keeping code branches in which will never be executed seems like a bad idea in the long term.

Here's another argument: Ingo just asked me to add a noclflush option to the code. Guess what check that option will need?

Besides compared to the cost of a flushing clflush the branches are absolutely in the noise.

-Andi

#10 lore raw
In reply to H. Peter Anvin
Quoted 17 lines · H. Peter Anvin
Quoted 11 lines · Kyle McMartin
Quoted 5 lines · Andi Kleen

One problem that we had in the past is that some simulators
only implement the absolutely minimum feature set and you
might have well broken one of these with this.

Yeah, true. Please ignore the patch folks.

cheers, Kyle

Simulators can be fixed, even if it takes time; for something like clflush this is easier since clflush can be implemented as a simple noop for most of them.

Quoted 6 lines

I just verified that Bochs 2.3.0 lacks this CPUID bit whereas the current version, 2.3.6, enables CLFLUSH iff SSE2 is enabled. Qemu 0.9.0 has CLFLUSH. Andi, do you happen to know of any specific simulators which are problematic? I would assume any recent version of SimNow is up to date.

simics? It was very useful long time ago...
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html