Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Ingo Molnar]

* Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:

> early_init_intel is introduced by
> 
> commit 2b16a2353814a513cdb5c5c739b76a19d7ea39ce
> Author: Andi Kleen <ak@suse.de>
> Date:   Wed Jan 30 13:32:40 2008 +0100
> 
>     x86: move X86_FEATURE_CONSTANT_TSC into early cpu feature detection
> 
> set CONSTANT_TSC for intel cpus
> 
> but it already set in init_intel
> 
> don't need to set that two times in early_init_intel() and init_intel. this patch remove one.
> 
> also fix error in early_init_intel and reference about x86_capality, because it
> is array already.., prevent possible data corruption...
> 
> this should be applied for 2.6.25

thanks Yinghai, applied.

	Ingo

[PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Yinghai Lu]

early_init_intel is introduced by

commit 2b16a2353814a513cdb5c5c739b76a19d7ea39ce
Author: Andi Kleen <ak@suse.de>
Date:   Wed Jan 30 13:32:40 2008 +0100

    x86: move X86_FEATURE_CONSTANT_TSC into early cpu feature detection

set CONSTANT_TSC for intel cpus

but it already set in init_intel

don't need to set that two times in early_init_intel() and init_intel. this patch remove one.

also fix error in early_init_intel and reference about x86_capality, because it
is array already.., prevent possible data corruption...

this should be applied for 2.6.25

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com> 

diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 62d3f14..210134c 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -1027,7 +1027,7 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
 {
 	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
 	    (c->x86 == 0x6 && c->x86_model >= 0x0e))
-		set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability);
+		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 }
 
 static void __cpuinit init_intel(struct cpuinfo_x86 *c)
@@ -1071,9 +1071,6 @@ static void __cpuinit init_intel(struct cpuinfo_x86 *c)
 
 	if (c->x86 == 15)
 		c->x86_cache_alignment = c->x86_clflush_size * 2;
-	if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
-	    (c->x86 == 0x6 && c->x86_model >= 0x0e))
-		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 	if (c->x86 == 6)
 		set_cpu_cap(c, X86_FEATURE_REP_GOOD);
 	set_cpu_cap(c, X86_FEATURE_LFENCE_RDTSC);

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Ingo Molnar]

* Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:

> also fix error in early_init_intel and reference about x86_capality, 
> because it is array already.., prevent possible data corruption...

hm, why should there be data corruption:

> -		set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability);
> +		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);

cpu_cpu_cap() is currently defined as:

 #define set_cpu_cap(c, bit)     set_bit(bit, (unsigned long *)((c)->x86_capability)

which is the same. set_cpu_cap() is indeed the cleaner form to do this 
so your patch is correct as a cleanup.

	Ingo

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Yinghai Lu]

On Mon, Feb 25, 2008 at 11:20 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  * Yinghai Lu <Yinghai.Lu@Sun.COM> wrote:
>
>
> > also fix error in early_init_intel and reference about x86_capality,
>  > because it is array already.., prevent possible data corruption...
>
>  hm, why should there be data corruption:
>
>
>  > -             set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability);
>  > +             set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
>
>  cpu_cpu_cap() is currently defined as:
>
>   #define set_cpu_cap(c, bit)     set_bit(bit, (unsigned long *)((c)->x86_capability)
>
>  which is the same. set_cpu_cap() is indeed the cleaner form to do this
>  so your patch is correct as a cleanup.
set_cpu_cap is right
==
set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
should be
set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);

x86_capability is a array ...

so this could prevent some data corruption.

YH

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[H. Peter Anvin]

Yinghai Lu wrote:
>>  which is the same. set_cpu_cap() is indeed the cleaner form to do this
>>  so your patch is correct as a cleanup.
> set_cpu_cap is right
> ==
> set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
> should be
> set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> 
> x86_capability is a array ...
> 

For an array, the & is optional and has no effect.

So they mean the same thing.

	-hpa

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Ingo Molnar]

* Yinghai Lu <yhlu.kernel@gmail.com> wrote:

> >   #define set_cpu_cap(c, bit)     set_bit(bit, (unsigned long *)((c)->x86_capability)
> >
> >  which is the same. set_cpu_cap() is indeed the cleaner form to do this
> >  so your patch is correct as a cleanup.
> set_cpu_cap is right
> ==
> set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
> should be
> set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> 
> x86_capability is a array ...
> 
> so this could prevent some data corruption.

ah, right you are! The commit was done in a sloppy, incomplete way, 
leaving around lots of direct c->x86_capability references and giving 
room for this bug ...

Btw., there's one other place that has the same bug. I'll fix that up 
too.

	Ingo

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > set_cpu_cap is right
> > ==
> > set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
> > should be
> > set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
> > 
> > x86_capability is a array ...
> > 
> > so this could prevent some data corruption.
> 
> ah, right you are! [...]

actually, not: &c->x86_capability and c->x86_capability result in the 
same address (it's an array, not a pointer), so there's no "data 
corruption". If x86_capability were a pointer then you would be right - 
so this is all worth cleaning up.

	Ingo

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Yinghai Lu wrote:
>>>  which is the same. set_cpu_cap() is indeed the cleaner form to do this
>>>  so your patch is correct as a cleanup.
>> set_cpu_cap is right
>> ==
>> set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
>> should be
>> set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
>>
>> x86_capability is a array ...
>
> For an array, the & is optional and has no effect.
>
> So they mean the same thing.

yeah. It's unnecessary entropy nevertheless and i've cleaned it all up 
in x86.git.

	Ingo

Re: [PATCH] x86_64: remove wrong setting about CONSTANT_TSC for intel cpu

[Yinghai Lu]

On Mon, Feb 25, 2008 at 11:33 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> Yinghai Lu wrote:
>  >>  which is the same. set_cpu_cap() is indeed the cleaner form to do this
>  >>  so your patch is correct as a cleanup.
>  > set_cpu_cap is right
>  > ==
>  > set_bit(X86_FEATURE_CONSTANT_TSC, &c->x86_capability); ===> is wrong
>  > should be
>  > set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability);
>  >
>  > x86_capability is a array ...
>  >
>
>  For an array, the & is optional and has no effect.

good to know.

YH