Re: [PATCH] x86: make clflush a required feature on x86_64

Re: [PATCH] x86: make clflush a required feature on x86_64

[Pavel Machek]

On Fri 2008-01-18 01:27:06, H. Peter Anvin wrote:
> Kyle McMartin wrote:
> >On Fri, Jan 18, 2008 at 06:53:53AM +0100, Andi Kleen 
> >wrote:
> >>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.

simics? It was very useful long time ago...
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] x86: make clflush a required feature on x86_64

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

Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>
---
 include/asm-x86/cpufeature_64.h     |    3 +++
 include/asm-x86/required-features.h |    4 +++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/asm-x86/cpufeature_64.h b/include/asm-x86/cpufeature_64.h
index e18496b..ef11d27 100644
--- a/include/asm-x86/cpufeature_64.h
+++ b/include/asm-x86/cpufeature_64.h
@@ -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
 
diff --git a/include/asm-x86/required-features.h b/include/asm-x86/required-features.h
index 7400d3a..cc27664 100644
--- a/include/asm-x86/required-features.h
+++ b/include/asm-x86/required-features.h
@@ -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)
-- 
1.5.3.7

Re: [PATCH] x86: make clflush a required feature on x86_64

[H. Peter Anvin]

Kyle McMartin wrote:
> 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.
> 
> Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>
> ---
>  include/asm-x86/cpufeature_64.h     |    3 +++
>  include/asm-x86/required-features.h |    4 +++-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/include/asm-x86/cpufeature_64.h b/include/asm-x86/cpufeature_64.h
> index e18496b..ef11d27 100644
> --- a/include/asm-x86/cpufeature_64.h
> +++ b/include/asm-x86/cpufeature_64.h
> @@ -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

[PATCH] x86_64: remove redundant cpu_has_ definitions

[Kyle McMartin]

On Fri, Jan 18, 2008 at 12:04:54AM -0500, H. Peter Anvin wrote:
> 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...

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

Signed-off-by: Kyle McMartin <kyle@mcmartin.ca>

--- a/include/asm-x86/cpufeature.h
+++ b/include/asm-x86/cpufeature.h
@@ -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 */

Re: [PATCH] x86: make clflush a required feature on x86_64

[Andi Kleen]

Kyle McMartin <kyle@mcmartin.ca> writes:

> 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

Re: [PATCH] x86: make clflush a required feature on x86_64

[Kyle McMartin]

On Fri, Jan 18, 2008 at 06:53:53AM +0100, Andi Kleen wrote:
> 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

Re: [PATCH] x86: make clflush a required feature on x86_64

[H. Peter Anvin]

Kyle McMartin wrote:
> On Fri, Jan 18, 2008 at 06:53:53AM +0100, Andi Kleen wrote:
>> 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

Re: [PATCH] x86: make clflush a required feature on x86_64

[Andi Kleen]

> Simulators can be fixed, 

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

> 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

Re: [PATCH] x86_64: remove redundant cpu_has_ definitions

[Ingo Molnar]

* Kyle McMartin <kyle@mcmartin.ca> wrote:

> --- a/include/asm-x86/cpufeature.h
> +++ b/include/asm-x86/cpufeature.h
> @@ -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
> -

thanks, applied.

	Ingo

Re: [PATCH] x86: make clflush a required feature on x86_64

[H. Peter Anvin]

Andi Kleen wrote:
>> 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.

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

Re: [PATCH] x86: make clflush a required feature on x86_64

[Andi Kleen]

On Fri, Jan 18, 2008 at 08:56:43AM -0500, H. Peter Anvin wrote:
> Andi Kleen wrote:
> >>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