2.6.26, PAT and AMD family 6

2.6.26, PAT and AMD family 6

[Rene Herman]

Good day.

On 2.6.25 and below, my /proc/cpuinfo looks like:

processor       : 0
vendor_id       : AuthenticAMD
cpu family      : 6
model           : 7
model name      : AMD Duron(tm) Processor
stepping        : 1
cpu MHz         : 1312.969
cache size      : 64 KB
fdiv_bug        : no
hlt_bug         : no
f00f_bug        : no
coma_bug        : no
fpu             : yes
fpu_exception   : yes
cpuid level     : 1
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov 
pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
bogomips        : 2628.68
clflush size    : 32

while on current mainline PAT and TS (Temperature Sensor) drop from the 
feature flags:

flags		: fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov pse36 mmx 
fxsr sse syscall mmxext 3dnowext 3dnow

With respect to PAT, I guess it's 9307cacad0dfe3749f00303125c6f7f0523e5616, 
"x86: pat cpu feature bit setting for known cpus" but what's this about?

Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The 
commit message for that change is completely and totally unhelpful.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

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

On Tue, May 6, 2008 at 6:48 PM, Rene Herman <rene.herman@keyaccess.nl> wrote:
> Good day.
>
>  On 2.6.25 and below, my /proc/cpuinfo looks like:
>
>  processor       : 0
>  vendor_id       : AuthenticAMD
>  cpu family      : 6
>  model           : 7
>  model name      : AMD Duron(tm) Processor
>  stepping        : 1
>  cpu MHz         : 1312.969
>  cache size      : 64 KB
>  fdiv_bug        : no
>  hlt_bug         : no
>  f00f_bug        : no
>  coma_bug        : no
>  fpu             : yes
>  fpu_exception   : yes
>  cpuid level     : 1
>  wp              : yes
>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
>  bogomips        : 2628.68
>  clflush size    : 32
>
>  while on current mainline PAT and TS (Temperature Sensor) drop from the
> feature flags:
>
>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
>
>  With respect to PAT, I guess it's 9307cacad0dfe3749f00303125c6f7f0523e5616,
> "x86: pat cpu feature bit setting for known cpus" but what's this about?
>
>  Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> commit message for that change is completely and totally unhelpful.

others like to to whitebox methods, ..., please try attach patch to
see if duron support PAT.

YH

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: duron_pat.patch --]
[-- Type: text/x-patch; name=duron_pat.patch, Size: 526 bytes --]

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a428ffc..81483ec 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
 			set_cpu_cap(c, X86_FEATURE_PAT);
+		if (c->x86 == 6 && c->x86_modes == 7)
+			set_cpu_cap(c, X86_FEATURE_PAT);
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))

Undocumented and duplicated code

[Adrian Bunk]

On Tue, May 06, 2008 at 07:39:44PM -0700, Yinghai Lu wrote:
> On Tue, May 6, 2008 at 6:48 PM, Rene Herman <rene.herman@keyaccess.nl> wrote:
> > Good day.
> >
> >  On 2.6.25 and below, my /proc/cpuinfo looks like:
> >
> >  processor       : 0
> >  vendor_id       : AuthenticAMD
> >  cpu family      : 6
> >  model           : 7
> >  model name      : AMD Duron(tm) Processor
> >  stepping        : 1
> >  cpu MHz         : 1312.969
> >  cache size      : 64 KB
> >  fdiv_bug        : no
> >  hlt_bug         : no
> >  f00f_bug        : no
> >  coma_bug        : no
> >  fpu             : yes
> >  fpu_exception   : yes
> >  cpuid level     : 1
> >  wp              : yes
> >  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> > pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
> >  bogomips        : 2628.68
> >  clflush size    : 32
> >
> >  while on current mainline PAT and TS (Temperature Sensor) drop from the
> > feature flags:
> >
> >  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
> > pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
> >
> >  With respect to PAT, I guess it's 9307cacad0dfe3749f00303125c6f7f0523e5616,
> > "x86: pat cpu feature bit setting for known cpus" but what's this about?
> >
> >  Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> > commit message for that change is completely and totally unhelpful.
> 
> others like to to whitebox methods, ..., please try attach patch to
> see if duron support PAT.


There surely is documentation available covering this?

And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
manual setting of X86_FEATURE_PAT at all?

There's no indication in the code, and as Rene already says there's even
no description at all in commit 9307cacad0dfe3749f00303125c6f7f0523e5616

Such code really needs a comment explaining why we have to do this at all.

There must be some CPUs with the "pat" flag set but not being usable?
Which?

According to the linux-kernel discussions there might not be any broken 
CPU at all - but in this case the whitelist will not fill itself, and 
expecting people to note that their flags changed and complaining is not 
really a good approach.

And that your commit added the same clear/set code in three different 
places doesn't look good - this really deserved from the beginning being 
factored out into an own function to avoid future problems when CPUs get 
added (like what happens with your patch here - it touches only one 
place, and since the same context is present in two places in the same 
file "patch" might even choose freely where it gets applied...).

Pavel even made a similar comment on linux-kernel before the patch 
got merged into Linus' tree. [1]

Guys, even if it compiles in all randconfig configurations and works on 
all test machines this is exactly the kind of stuff that causes 
headaches in the future.

And this patch (by the author of the code himself) is the first time 
where it breaks.


> YH

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index a428ffc..81483ec 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
>  	case X86_VENDOR_AMD:
>  		if (c->x86 >= 0xf && c->x86 <= 0x11)
>  			set_cpu_cap(c, X86_FEATURE_PAT);
> +		if (c->x86 == 6 && c->x86_modes == 7)
> +			set_cpu_cap(c, X86_FEATURE_PAT);
>  		break;
>  	case X86_VENDOR_INTEL:
>  		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))


cu
Adrian

[1] http://lkml.org/lkml/2008/3/28/184

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

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

On 07-05-08 04:39, Yinghai Lu wrote:

> On Tue, May 6, 2008 at 6:48 PM, Rene Herman <rene.herman@keyaccess.nl> wrote:

>>  On 2.6.25 and below, my /proc/cpuinfo looks like:
>>
>>  processor       : 0
>>  vendor_id       : AuthenticAMD
>>  cpu family      : 6
>>  model           : 7
>>  model name      : AMD Duron(tm) Processor

[ ... ]

>>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
>> pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts

>>  while on current mainline PAT and TS (Temperature Sensor) drop from the
>> feature flags:
>>
>>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca cmov
>> pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
>>
>>  With respect to PAT, I guess it's 9307cacad0dfe3749f00303125c6f7f0523e5616,
>> "x86: pat cpu feature bit setting for known cpus" but what's this about?
>>
>> Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
>> commit message for that change is completely and totally unhelpful.
> 
> others like to to whitebox methods, ..., please try attach patch to
> see if duron support PAT.

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index a428ffc..81483ec 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
>  	case X86_VENDOR_AMD:
>  		if (c->x86 >= 0xf && c->x86 <= 0x11)
>  			set_cpu_cap(c, X86_FEATURE_PAT);
> +		if (c->x86 == 6 && c->x86_modes == 7)
> +			set_cpu_cap(c, X86_FEATURE_PAT);
>  		break;
>  	case X86_VENDOR_INTEL:
>  		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))

s/modes/model/ but, as far as I'm aware, works fine other than that. When I boot
with CONFIG_X86_PAT after applying that, I see:

	x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106

and PAT is retained in the feature flags. However, this I do not consider very
surprising. Why is this code doing what it is doing in the first place?

These feature flags are read from hardware in the CPUID instruction. Why is this code
then going "ah, this CPU may _claim_ PAT but we won't actually believe it unless it's
model foo, bar or baz". Is that feature flag buggy?

That is, why is the attached not the better fix? Works fine for me as well and will
retain the PAT feature flag also for for example the AMD family 6, model 1 and model
4 that both also advertise PAT in their feature flags, but wil have lost them now as
well in the same way.

If for some or other reason this explicit PAT white listing is in fact necessary, in
addition to the above, it seems you also need/want the same in generic_identify() in
that same file.

And what it definitely wanted was a changelog...

Rene.

[-- Attachment #2: pat.diff --]
[-- Type: text/plain, Size: 1114 bytes --]

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 35b4f6a..45d8738 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -307,20 +307,6 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
 		}
 
 	}
-
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
-	switch (c->x86_vendor) {
-	case X86_VENDOR_AMD:
-		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	case X86_VENDOR_INTEL:
-		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	}
-
 }
 
 /*
@@ -408,19 +394,6 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 
 		init_scattered_cpuid_features(c);
 	}
-
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
-	switch (c->x86_vendor) {
-	case X86_VENDOR_AMD:
-		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	case X86_VENDOR_INTEL:
-		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	}
 }
 
 static void __cpuinit squash_the_stupid_serial_number(struct cpuinfo_x86 *c)

Re: Undocumented and duplicated code

[Rene Herman]

On 07-05-08 14:46, Adrian Bunk wrote:

>> others like to to whitebox methods, ..., please try attach patch to
>> see if duron support PAT.
> 
> 
> There surely is documentation available covering this?
> 
> And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
> manual setting of X86_FEATURE_PAT at all?
> 
> There's no indication in the code, and as Rene already says there's even
> no description at all in commit 9307cacad0dfe3749f00303125c6f7f0523e5616
> 
> Such code really needs a comment explaining why we have to do this at all.
> 
> There must be some CPUs with the "pat" flag set but not being usable?
> Which?
> 
> According to the linux-kernel discussions there might not be any broken 
> CPU at all - but in this case the whitelist will not fill itself, and 
> expecting people to note that their flags changed and complaining is not 
> really a good approach.

I'd say it's an insane approach unless there's actually some indication of
buggy CPUs. Which there might be, I don't know, but this then definitely
wants a comment and changelog.

Just now sent out another reply as well saying much the same:

http://marc.info/?l=linux-kernel&m=121016531804511&w=2

but I see you tracked the people involved and added them to CC on this
thread-leaf...

> And that your commit added the same clear/set code in three different 
> places doesn't look good - this really deserved from the beginning being 
> factored out into an own function to avoid future problems when CPUs get 
> added (like what happens with your patch here - it touches only one 
> place, and since the same context is present in two places in the same 
> file "patch" might even choose freely where it gets applied...).
> 
> Pavel even made a similar comment on linux-kernel before the patch 
> got merged into Linus' tree. [1]
> 
> Guys, even if it compiles in all randconfig configurations and works on 
> all test machines this is exactly the kind of stuff that causes 
> headaches in the future.
> 
> And this patch (by the author of the code himself) is the first time 
> where it breaks.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Arjan van de Ven]

On Wed, 07 May 2008 15:00:18 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 07-05-08 04:39, Yinghai Lu wrote:
> 
> > On Tue, May 6, 2008 at 6:48 PM, Rene Herman
> > <rene.herman@keyaccess.nl> wrote:
> 
> >>  On 2.6.25 and below, my /proc/cpuinfo looks like:
> >>
> >>  processor       : 0
> >>  vendor_id       : AuthenticAMD
> >>  cpu family      : 6
> >>  model           : 7
> >>  model name      : AMD Duron(tm) Processor
> 
> [ ... ]
> 
> >>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge
> >> mca cmov pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
> 
> >>  while on current mainline PAT and TS (Temperature Sensor) drop
> >> from the feature flags:
> >>
> >>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge
> >> mca cmov pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
> >>
> >>  With respect to PAT, I guess it's
> >> 9307cacad0dfe3749f00303125c6f7f0523e5616, "x86: pat cpu feature
> >> bit setting for known cpus" but what's this about?
> >>
> >> Did my cpuinfo lie upto this point or shouldn't the flag be
> >> cleared? The commit message for that change is completely and
> >> totally unhelpful.
> > 
> > others like to to whitebox methods, ..., please try attach patch to
> > see if duron support PAT.
> 
> > diff --git a/arch/x86/kernel/cpu/common.c
> > b/arch/x86/kernel/cpu/common.c index a428ffc..81483ec 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct
> > cpuinfo_x86 *c) case X86_VENDOR_AMD:
> >  		if (c->x86 >= 0xf && c->x86 <= 0x11)
> >  			set_cpu_cap(c, X86_FEATURE_PAT);
> > +		if (c->x86 == 6 && c->x86_modes == 7)
> > +			set_cpu_cap(c, X86_FEATURE_PAT);
> >  		break;
> >  	case X86_VENDOR_INTEL:
> >  		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model
> > >= 15))
> 
> s/modes/model/ but, as far as I'm aware, works fine other than that.
> When I boot with CONFIG_X86_PAT after applying that, I see:
> 
> 	x86 PAT enabled: cpu 0, old 0x7040600070406, new
> 0x7010600070106
> 
> and PAT is retained in the feature flags. However, this I do not
> consider very surprising. Why is this code doing what it is doing in
> the first place?
> 
> These feature flags are read from hardware in the CPUID instruction.
> Why is this code then going "ah, this CPU may _claim_ PAT but we
> won't actually believe it unless it's model foo, bar or baz". Is that
> feature flag buggy?
> 

older cpus had various issues with PAT, some blatently obvious, some
more subtle. Since for old systems the mtrrs clearly work fine... the
idea was to not take the risk (since there's no reward) and just leave
them as is, in a working state.

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 07-05-08 15:42, Arjan van de Ven wrote:
> On Wed, 07 May 2008 15:00:18 +0200
> Rene Herman <rene.herman@keyaccess.nl> wrote:
> 
>> On 07-05-08 04:39, Yinghai Lu wrote:
>>
>>> On Tue, May 6, 2008 at 6:48 PM, Rene Herman
>>> <rene.herman@keyaccess.nl> wrote:
>>>>  On 2.6.25 and below, my /proc/cpuinfo looks like:
>>>>
>>>>  processor       : 0
>>>>  vendor_id       : AuthenticAMD
>>>>  cpu family      : 6
>>>>  model           : 7
>>>>  model name      : AMD Duron(tm) Processor
>> [ ... ]
>>
>>>>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge
>>>> mca cmov pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
>>>>  while on current mainline PAT and TS (Temperature Sensor) drop
>>>> from the feature flags:
>>>>
>>>>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge
>>>> mca cmov pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
>>>>
>>>>  With respect to PAT, I guess it's
>>>> 9307cacad0dfe3749f00303125c6f7f0523e5616, "x86: pat cpu feature
>>>> bit setting for known cpus" but what's this about?
>>>>
>>>> Did my cpuinfo lie upto this point or shouldn't the flag be
>>>> cleared? The commit message for that change is completely and
>>>> totally unhelpful.
>>> others like to to whitebox methods, ..., please try attach patch to
>>> see if duron support PAT.
>>> diff --git a/arch/x86/kernel/cpu/common.c
>>> b/arch/x86/kernel/cpu/common.c index a428ffc..81483ec 100644
>>> --- a/arch/x86/kernel/cpu/common.c
>>> +++ b/arch/x86/kernel/cpu/common.c
>>> @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct
>>> cpuinfo_x86 *c) case X86_VENDOR_AMD:
>>>  		if (c->x86 >= 0xf && c->x86 <= 0x11)
>>>  			set_cpu_cap(c, X86_FEATURE_PAT);
>>> +		if (c->x86 == 6 && c->x86_modes == 7)
>>> +			set_cpu_cap(c, X86_FEATURE_PAT);
>>>  		break;
>>>  	case X86_VENDOR_INTEL:
>>>  		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model
>>>> = 15))
>> s/modes/model/ but, as far as I'm aware, works fine other than that.
>> When I boot with CONFIG_X86_PAT after applying that, I see:
>>
>> 	x86 PAT enabled: cpu 0, old 0x7040600070406, new
>> 0x7010600070106
>>
>> and PAT is retained in the feature flags. However, this I do not
>> consider very surprising. Why is this code doing what it is doing in
>> the first place?
>>
>> These feature flags are read from hardware in the CPUID instruction.
>> Why is this code then going "ah, this CPU may _claim_ PAT but we
>> won't actually believe it unless it's model foo, bar or baz". Is that
>> feature flag buggy?
>>
> 
> older cpus had various issues with PAT, some blatently obvious, some
> more subtle.

And I suppose you have a list of these older CPUs or is this going to be
one of these things where in 5 years time I say to yet another person "ah
yes, I remember someone once telling me that old CPUs apparently had some
issues, some blatantly obvious, some more subtle" and the saga continues
on from there again?

> Since for old systems the mtrrs clearly work fine... the idea was to
> not take the risk (since there's no reward) and just leave them as
> is, in a working state.

With CONFIG_X86_PAT, you now see "CPU and/or kernel does not support PAT."
at the top of your dmesg which is going to make people wonder. I did a
cat /proc/cpuinfo, saw no PAT flag and was just suspicious enough that I
didn't trust it.

A blacklist would be a better idea I feel, but in ANY case I think it's
really bad form to clear the feature flag. They are provided by hardware;
if arch/x86/mm/pat.c won't risk running except on a select few tested
models, whatever, but my /proc/cpuinfo shouldn't be lying to me.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Arjan van de Ven]

On Wed, 07 May 2008 16:09:05 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:


[as for which cpus]

The errata docs have these errata, at least for Intel I know they're
there. We can add back older cpus over time, but again.. think of it as
a risk/reward thing ;( Right now, breaking all kinds of well working
older systems without benefits on the upside..... that's not very
popular on lkml. Once PAT is rock solid I hope we can relax this check
greatly...

> A blacklist would be a better idea I feel, but in ANY case I think
> it's really bad form to clear the feature flag. They are provided by
> hardware; if arch/x86/mm/pat.c won't risk running except on a select
> few tested models, whatever, but my /proc/cpuinfo shouldn't be lying
> to me.

we filter those for all kinds of things already, sorry.
What good is showing "pat" if "pat" isn't deemed usable (yet)????
Now *that* is deception :)
> 
> Rene.

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 07-05-08 16:24, Arjan van de Ven wrote:

> On Wed, 07 May 2008 16:09:05 +0200
> Rene Herman <rene.herman@keyaccess.nl> wrote:

> [as for which cpus]
> 
> The errata docs have these errata, at least for Intel I know they're
> there. We can add back older cpus over time, but again.. think of it as
> a risk/reward thing ;( Right now, breaking all kinds of well working
> older systems without benefits on the upside.....

What/who do you break, when they are not selecting CONFIG_X86_PAT? The
help text can be suitably alarming. I feel this is no argument at all.

> that's not very popular on lkml. Once PAT is rock solid I hope we can
> relax this check greatly...

A blacklist would seem to be really much preferred. All of AMD family 6
has been excluded in one fell swoop now for example, while I sort of
expect not any of them should be.

>> A blacklist would be a better idea I feel, but in ANY case I think
>> it's really bad form to clear the feature flag. They are provided by
>> hardware; if arch/x86/mm/pat.c won't risk running except on a select
>> few tested models, whatever, but my /proc/cpuinfo shouldn't be lying
>> to me.
> 
> we filter those for all kinds of things already, sorry.
> What good is showing "pat" if "pat" isn't deemed usable (yet)????
> Now *that* is deception :)

The trouble is -- if you hide that the CPU _should_ have PAT, how many
people do you expect are going to look further and test? I knew that I
should have PAT so I distrusted my CPU feature flag display but you've
now limited your testers to people who've read the CPU datasheet.
That's really no good.

If Linux messes around with those flags already, it's doing things
wrong already. /proc/cpuinfo is not a display of software features
but of hardware features -- anything else is outright wrong. Being
wrong for PAT also doesn't improve things.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Daniel Hazelton]

On Wednesday 07 May 2008 09:00:18 Rene Herman wrote:
> On 07-05-08 04:39, Yinghai Lu wrote:
> > On Tue, May 6, 2008 at 6:48 PM, Rene Herman <rene.herman@keyaccess.nl> 
wrote:
> >>  On 2.6.25 and below, my /proc/cpuinfo looks like:
> >>
> >>  processor       : 0
> >>  vendor_id       : AuthenticAMD
> >>  cpu family      : 6
> >>  model           : 7
> >>  model name      : AMD Duron(tm) Processor
>
> [ ... ]
>
> >>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca
> >> cmov pat pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow ts
> >>
> >>  while on current mainline PAT and TS (Temperature Sensor) drop from the
> >> feature flags:
> >>
> >>  flags           : fpu vme de pse tsc msr pae mce cx8 sep mtrr pge mca
> >> cmov pse36 mmx fxsr sse syscall mmxext 3dnowext 3dnow
> >>
> >>  With respect to PAT, I guess it's
> >> 9307cacad0dfe3749f00303125c6f7f0523e5616, "x86: pat cpu feature bit
> >> setting for known cpus" but what's this about?
> >>
> >> Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> >> commit message for that change is completely and totally unhelpful.
> >
> > others like to to whitebox methods, ..., please try attach patch to
> > see if duron support PAT.
> >
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index a428ffc..81483ec 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct
> > cpuinfo_x86 *c) case X86_VENDOR_AMD:
> >  		if (c->x86 >= 0xf && c->x86 <= 0x11)
> >  			set_cpu_cap(c, X86_FEATURE_PAT);
> > +		if (c->x86 == 6 && c->x86_modes == 7)
> > +			set_cpu_cap(c, X86_FEATURE_PAT);
> >  		break;
> >  	case X86_VENDOR_INTEL:
> >  		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
>
> s/modes/model/ but, as far as I'm aware, works fine other than that. When I
> boot with CONFIG_X86_PAT after applying that, I see:
>
> 	x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106
>
> and PAT is retained in the feature flags. However, this I do not consider
> very surprising. Why is this code doing what it is doing in the first
> place?
>
> These feature flags are read from hardware in the CPUID instruction. Why is
> this code then going "ah, this CPU may _claim_ PAT but we won't actually
> believe it unless it's model foo, bar or baz". Is that feature flag buggy?

HPA asked about why they used a whitelist instead of a blacklist in [1]. The 
answer (in [2]) was that those are the CPU's that are guaranteed to properly 
support PAT (no known or potential errata). However in [3] Dean Gaudet 
complained about the AMD detection code having a limit that the Intel 
detection code did not. Perhaps changing the 'c->x86 <= 0x11' test in the 
X86_VENDOR_AMD block to not exist?

> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index a428ffc..81483ec 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 
*c)
>       case X86_VENDOR_AMD:
>               if (c->x86 >= 0xf && c->x86 <= 0x11)
>                       set_cpu_cap(c, X86_FEATURE_PAT);
> +             if (c->x86 == 6 && c->x86_modes == 7)
> +                     set_cpu_cap(c, X86_FEATURE_PAT);
                     ^^^^^---- Here in Rene's patch...
Wouldn't this be better if written the same as the Intel side, ie:
if (c->x86 >= 0xF && (c->x86 == 6 && c->x86_model == 7))
(or even with c->x86_model >= 7  ?)

>               break;
>       case X86_VENDOR_INTEL:
>               if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))

DRH

[1] http://lkml.org/lkml/2008/3/25/118
[2] http://lkml.org/lkml/2008/3/25/292
[3] http://lkml.org/lkml/2008/3/30/37

-- 
Dialup is like pissing through a pipette. Slow and excruciatingly painful.

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 07-05-08 21:39, Daniel Hazelton wrote:

> HPA asked about why they used a whitelist instead of a blacklist in [1]. The 
> answer (in [2]) was that those are the CPU's that are guaranteed to properly 
> support PAT (no known or potential errata). However in [3] Dean Gaudet 
> complained about the AMD detection code having a limit that the Intel 
> detection code did not.

And in that thread both HPA and Ingo Molnar -- two of the three x86 arch
maintainers -- agreed that a whitelist is the wrong approach, with HPA
commenting that it lead to vendor lockin. And here I am talkng to an
Intel employee about why my entire AMD CPU family was excluded.

So why is this thing now in mainline with Ingo's sign-off and not a line
of changelog to explain it?

>                      ^^^^^---- Here in Rene's patch...

Yinghai's.

> Wouldn't this be better if written the same as the Intel side, ie:
> if (c->x86 >= 0xF && (c->x86 == 6 && c->x86_model == 7))
> (or even with c->x86_model >= 7  ?)

I doubt it, given that that condition would optimize to 0 but assuming
s/&&/||/ that's still excluding my previous Duron model 4 which, as far
as I'm aware, had functional PAT as well. Nor am I myself aware of any
model 1 trouble. Really, this whitelist seems a pretty bad idea.

> [1] http://lkml.org/lkml/2008/3/25/118
> [2] http://lkml.org/lkml/2008/3/25/292
> [3] http://lkml.org/lkml/2008/3/30/37

Questions...

-- Why is this thing in with the whitelist over the objection of arch
   maintainers?
-- why is this thing in without a single line of changelog?
-- Why does this thing hide the fact that my CPU does have PAT from
   me (even though it might elect to not trust it)?

Rene.

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 1:06 PM, Rene Herman <rene.herman@keyaccess.nl> wrote:
> On 07-05-08 21:39, Daniel Hazelton wrote:
>
>
> > HPA asked about why they used a whitelist instead of a blacklist in [1].
> The answer (in [2]) was that those are the CPU's that are guaranteed to
> properly support PAT (no known or potential errata). However in [3] Dean
> Gaudet complained about the AMD detection code having a limit that the Intel
> detection code did not.
> >
>
>  And in that thread both HPA and Ingo Molnar -- two of the three x86 arch
>  maintainers -- agreed that a whitelist is the wrong approach, with HPA
>  commenting that it lead to vendor lockin. And here I am talkng to an
>  Intel employee about why my entire AMD CPU family was excluded.
>
>  So why is this thing now in mainline with Ingo's sign-off and not a line
>  of changelog to explain it?
>
>
>
> >                     ^^^^^---- Here in Rene's patch...
> >
>
>  Yinghai's.
>
>
>
> > Wouldn't this be better if written the same as the Intel side, ie:
> > if (c->x86 >= 0xF && (c->x86 == 6 && c->x86_model == 7))
> > (or even with c->x86_model >= 7  ?)

i only can access opteron Rev E, Rev F, and Quad core. So i enabled that.

now, enable other one by one...

YH

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 1:16 PM, Yinghai Lu <yhlu.kernel@gmail.com> wrote:
> On Wed, May 7, 2008 at 1:06 PM, Rene Herman <rene.herman@keyaccess.nl> wrote:
>  > On 07-05-08 21:39, Daniel Hazelton wrote:
>  >
>  >
>  > > HPA asked about why they used a whitelist instead of a blacklist in [1].
>  > The answer (in [2]) was that those are the CPU's that are guaranteed to
>  > properly support PAT (no known or potential errata). However in [3] Dean
>  > Gaudet complained about the AMD detection code having a limit that the Intel
>  > detection code did not.
>  > >
>  >
>  >  And in that thread both HPA and Ingo Molnar -- two of the three x86 arch
>  >  maintainers -- agreed that a whitelist is the wrong approach, with HPA
>  >  commenting that it lead to vendor lockin. And here I am talkng to an
>  >  Intel employee about why my entire AMD CPU family was excluded.
>  >
>  >  So why is this thing now in mainline with Ingo's sign-off and not a line
>  >  of changelog to explain it?
>  >
>  >
>  >
>  > >                     ^^^^^---- Here in Rene's patch...
>  > >
>  >
>  >  Yinghai's.
>  >
>  >
>  >
>  > > Wouldn't this be better if written the same as the Intel side, ie:
>  > > if (c->x86 >= 0xF && (c->x86 == 6 && c->x86_model == 7))
>  > > (or even with c->x86_model >= 7  ?)
>
>  i only can access opteron Rev E, Rev F, and Quad core. So i enabled that.
>
>  now, enable other one by one...

may add enable_pat command line, so other guys could test if their
cpus are ok with PAT...

YH

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Wed, 7 May 2008, Adrian Bunk wrote:

Can you please stop changing the subject lines? There is no need that
you grandstand your important findings.

> > >  Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> > > commit message for that change is completely and totally unhelpful.
> > 
> > others like to to whitebox methods, ..., please try attach patch to
> > see if duron support PAT.
> 
> 
> There surely is documentation available covering this?

There is, but there is no known validation of working CPUs, erratas
...

We have a well identified set of CPUs which can handle PAT and we dont
want to open up a can of worms by unconditionally enabling it on any
piece of silicon which claims to support PAT. This was made clear from
the first submission of PAT.

Also there is no downside on these older systems. They are fine with
MTRRs and have been for years. We can revisit that once PAT support
has stabilized.

> And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
> manual setting of X86_FEATURE_PAT at all?

The reason is to make cpu_has_pat a useful check and to avoid checking
cpu vendors, families and models inside of the PAT code. That's a good
thing actually, because the PAT code only cares about that cpu_has_pat
flag.

Clearing it in the cpuinfo is just a cosmetic side effect which does
no harm at all.

> There's no indication in the code, and as Rene already says there's even
> no description at all in commit 9307cacad0dfe3749f00303125c6f7f0523e5616
>
> Such code really needs a comment explaining why we have to do this at all.

Yes, that's a valid complaint. The changelog is pretty useless.
 
> There must be some CPUs with the "pat" flag set but not being usable?
> Which?

See above.

> According to the linux-kernel discussions there might not be any broken 
> CPU at all - but in this case the whitelist will not fill itself, and 
> expecting people to note that their flags changed and complaining is not 
> really a good approach.

But this is a much better approach than enabling it on everything and
getting flooded with hard to debug problems. PAT can fire back in
various ways and restricting the stabilization of new software to a
set of working hardware is just basic good engineering practice.

> And that your commit added the same clear/set code in three different 
> places doesn't look good - this really deserved from the beginning being 
> factored out into an own function to avoid future problems when CPUs get 
> added (like what happens with your patch here - it touches only one 
> place, and since the same context is present in two places in the same 
> file "patch" might even choose freely where it gets applied...).

Did you even look into the code which does all this feature trickery
based on CPU vendors, families and models ? This is something which
can not be changed easily. It's on our todo list and you wouldnt have
noticed if 32 and 64 bit would still live in different trees.

This feature and detection code is hard to clean up and definitely out
of the scope of this patch.

> Guys, even if it compiles in all randconfig configurations and works on 
> all test machines this is exactly the kind of stuff that causes 
> headaches in the future.

Dude, we are working hard to get x86 in shape, but we can not change
it over in no time except if we stop development on x86 completely for
two kernel versions. No, we have to clean this up gradually and accept
that there is still duplicated code moving in.

> And this patch (by the author of the code himself) is the first time 
> where it breaks.

Very interesting analysis. What broke ? This CPU was never in the set
of supported ones at all.

Anyway, you are welcome to review x86 code - it can definitely use
more eyeballs, but please try to inform yourself about the topic or
ask polite questions before yelling at people who contribute in a
very valuable way.

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

Hi!

> > And this patch (by the author of the code himself) is the first time 
> > where it breaks.
> 
> Very interesting analysis. What broke ? This CPU was never in the set
> of supported ones at all.
> 
> Anyway, you are welcome to review x86 code - it can definitely use
> more eyeballs, but please try to inform yourself about the topic or
> ask polite questions before yelling at people who contribute in a
> very valuable way.

Can we get rid of that copy&paste programming? They should have been
fixed _before_ merging the patch, but better late than never...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 07-05-08 22:52, Thomas Gleixner wrote:

>> And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
>> manual setting of X86_FEATURE_PAT at all?
> 
> The reason is to make cpu_has_pat a useful check and to avoid checking
> cpu vendors, families and models inside of the PAT code. That's a good
> thing actually, because the PAT code only cares about that cpu_has_pat
> flag.
> 
> Clearing it in the cpuinfo is just a cosmetic side effect which does
> no harm at all.

Oh yes, it does. It makes people unaware that their CPUs _should_ be
supporting PAT. The thing's not called /proc/kernelinfo for a reason.

>> And this patch (by the author of the code himself) is the first time 
>> where it breaks.
> 
> Very interesting analysis. What broke ? This CPU was never in the set
> of supported ones at all.

You misunderstood. Yinghai's patch only changed one of the code sites
and not the others, which (if I understood right) is the breakage
Adrian was reffering to.

> Anyway, you are welcome to review x86 code - it can definitely use 
> more eyeballs, but please try to inform yourself about the topic or 
> ask polite questions before yelling at people who contribute in a 
> very valuable way.

And would yelling at people how shuffle in code without (publicly at
least) addressing one of your fellow arch maintainers objections and
Pavel's review comments about code duplication without a single line
of explanation/changelog do?

Rene.

Re: 2.6.26, PAT and AMD family 6

[Adrian Bunk]

On Wed, May 07, 2008 at 10:52:36PM +0200, Thomas Gleixner wrote:
> On Wed, 7 May 2008, Adrian Bunk wrote:
>...
> > > >  Did my cpuinfo lie upto this point or shouldn't the flag be cleared? The
> > > > commit message for that change is completely and totally unhelpful.
> > > 
> > > others like to to whitebox methods, ..., please try attach patch to
> > > see if duron support PAT.
> > 
> > 
> > There surely is documentation available covering this?
> 
> There is, but there is no known validation of working CPUs, erratas
> ...
> 
> We have a well identified set of CPUs which can handle PAT and we dont
> want to open up a can of worms by unconditionally enabling it on any
> piece of silicon which claims to support PAT. This was made clear from
> the first submission of PAT.

But the commit description is empty.

> Also there is no downside on these older systems. They are fine with
> MTRRs and have been for years. We can revisit that once PAT support
> has stabilized.

My comment was about:
"please try attach patch to see if duron support PAT."

That information is for sure available.

>...
> > And that your commit added the same clear/set code in three different 
> > places doesn't look good - this really deserved from the beginning being 
> > factored out into an own function to avoid future problems when CPUs get 
> > added (like what happens with your patch here - it touches only one 
> > place, and since the same context is present in two places in the same 
> > file "patch" might even choose freely where it gets applied...).
> 
> Did you even look into the code which does all this feature trickery
> based on CPU vendors, families and models ? This is something which
> can not be changed easily. It's on our todo list and you wouldnt have
> noticed if 32 and 64 bit would still live in different trees.
> 
> This feature and detection code is hard to clean up and definitely out
> of the scope of this patch.

Did you even look at the commit we are discussing?

It ***adds*** exactly the same code at three different places.

>...
> > And this patch (by the author of the code himself) is the first time 
> > where it breaks.
> 
> Very interesting analysis. What broke ? This CPU was never in the set
> of supported ones at all.

The patch in the email I answered to was broken since the author did 
fall into his own trap by patching only one copy of his duplicated code.

> Anyway, you are welcome to review x86 code - it can definitely use
> more eyeballs, but please try to inform yourself about the topic or
> ask polite questions before yelling at people who contribute in a
> very valuable way.

I already try this (as far as my limited knowledge allows it).

But if you want "more eyeballs" and then valid critic like Pavel's in 
this case gets ignored that's pointless.

And the many commits with no adequate description or no description at 
all that came through the x86 tree are a real PITA for everyone trying 
to follow the commits.

> Thanks,
> 
> 	tglx

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Wed, 7 May 2008, Rene Herman wrote:
> On 07-05-08 22:52, Thomas Gleixner wrote:
> 
> > > And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then manual
> > > setting of X86_FEATURE_PAT at all?
> > 
> > The reason is to make cpu_has_pat a useful check and to avoid checking
> > cpu vendors, families and models inside of the PAT code. That's a good
> > thing actually, because the PAT code only cares about that cpu_has_pat
> > flag.
> > 
> > Clearing it in the cpuinfo is just a cosmetic side effect which does
> > no harm at all.
> 
> Oh yes, it does. It makes people unaware that their CPUs _should_ be
> supporting PAT. The thing's not called /proc/kernelinfo for a reason.

it's named /proc/cpuinfo and this unawareness of the until now not
utilized PAT feature is the least of our worries vs. PAT

> > > And this patch (by the author of the code himself) is the first time where
> > > it breaks.
> > 
> > Very interesting analysis. What broke ? This CPU was never in the set
> > of supported ones at all.
> 
> You misunderstood. Yinghai's patch only changed one of the code sites
> and not the others, which (if I understood right) is the breakage
> Adrian was reffering to.

I know exactly what he was referring to. So what's the problem ?

Yinghai missed to add it to the other place and he is hardly to blame
for that. This code is messy and thats not his fault. It will be
cleaned up, but that's not an simple taks to do. If you think you can
do it without breaking tons of systems, you're welcome.
 
> > Anyway, you are welcome to review x86 code - it can definitely use more
> > eyeballs, but please try to inform yourself about the topic or ask polite
> > questions before yelling at people who contribute in a very valuable way.
> 
> And would yelling at people how shuffle in code without (publicly at
> least) addressing one of your fellow arch maintainers objections

1) hpa asked a question http://lkml.org/lkml/2008/3/25/118
2) his question was answered http://lkml.org/lkml/2008/3/25/292
3) hpa did not object (no lkml ref, because there is none)

So what's your point ? Throwing factoids into a discussion is
not really helpful.

> and Pavel's review comments about code duplication without a single
> line of explanation/changelog do?

As I said before. The changelog is useless and Adrians point about
that is completely correct. 

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[Adrian Bunk]

On Wed, May 07, 2008 at 11:41:04PM +0200, Thomas Gleixner wrote:
> On Wed, 7 May 2008, Rene Herman wrote:
> > On 07-05-08 22:52, Thomas Gleixner wrote:
>...
> > > > And this patch (by the author of the code himself) is the first time where
> > > > it breaks.
> > > 
> > > Very interesting analysis. What broke ? This CPU was never in the set
> > > of supported ones at all.
> > 
> > You misunderstood. Yinghai's patch only changed one of the code sites
> > and not the others, which (if I understood right) is the breakage
> > Adrian was reffering to.
> 
> I know exactly what he was referring to. So what's the problem ?
> 
> Yinghai missed to add it to the other place and he is hardly to blame
> for that. This code is messy and thats not his fault. It will be
> cleaned up, but that's not an simple taks to do. If you think you can
> do it without breaking tons of systems, you're welcome.

It actually is completely his fault.

His commit ***added*** the three copies of the same code.

And Pavel did complain about it when it was sent to linux-kernel.

>...
> Thanks,
> 
> 	tglx

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Adrian Bunk wrote:
> On Wed, May 07, 2008 at 10:52:36PM +0200, Thomas Gleixner wrote:
> > We have a well identified set of CPUs which can handle PAT and we dont
> > want to open up a can of worms by unconditionally enabling it on any
> > piece of silicon which claims to support PAT. This was made clear from
> > the first submission of PAT.
> 
> But the commit description is empty.

Care to read, what I wrote futher down ?

"Yes, that's a valid complaint. The changelog is pretty useless."
 
> > Also there is no downside on these older systems. They are fine with
> > MTRRs and have been for years. We can revisit that once PAT support
> > has stabilized.
> 
> My comment was about:
> "please try attach patch to see if duron support PAT."

I know.
 
> That information is for sure available.

So you know where it is and you can confirm that it works fine ?

Pointers please. 

> > This feature and detection code is hard to clean up and definitely out
> > of the scope of this patch.
> 
> Did you even look at the commit we are discussing?
> 
> It ***adds*** exactly the same code at three different places.

Yes, I did. And it adds it for a fscking good reason. 

1) two times in common.c due to the existing detection logic mess
2) once in the 64 bit version

Again, this code is inherited and fragile mess, which can not be
changed at will.

> >...
> > > And this patch (by the author of the code himself) is the first time 
> > > where it breaks.
> > 
> > Very interesting analysis. What broke ? This CPU was never in the set
> > of supported ones at all.
> 
> The patch in the email I answered to was broken since the author did 

"the author" has a name.

> fall into his own trap by patching only one copy of his duplicated code.

That's not a real good reason to yell at him.

Yinghai  wanted to help out and check with the reporter whether this CPU
works with PAT or not.

Yinghai made a mistake.

You come along and ride a crusade against him for that. Very helpful.

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 07-05-08 23:41, Thomas Gleixner wrote:

>>> Clearing it in the cpuinfo is just a cosmetic side effect which does
>>> no harm at all.
>> Oh yes, it does. It makes people unaware that their CPUs _should_ be
>> supporting PAT. The thing's not called /proc/kernelinfo for a reason.
> 
> it's named /proc/cpuinfo

Exactly.

> and this unawareness of the until now not utilized PAT feature is the
> least of our worries vs. PAT

It's _my_ worry. I-the-user don't want my cpuinfo lying to me just
because if fits your-the-developer's codeflow better. Sheer insanity.

Getting philosophical tends to be a bad idea here but like always and
in this case; if you hide the world from people you disallow them from
improving it. It's that I _knew_ that my CPU should be supporting PAT
so that I could complain about it, but I'm your average LKML dwelling
datasheet collector.

Your feature may be very important and groovy to you, but to me it's
much more important that my cpuinfo doesn't lie to me. Please make it
not lie to me.

>>>> And this patch (by the author of the code himself) is the first time where
>>>> it breaks.
>>> Very interesting analysis. What broke ? This CPU was never in the set
>>> of supported ones at all.
>> You misunderstood. Yinghai's patch only changed one of the code sites
>> and not the others, which (if I understood right) is the breakage
>> Adrian was reffering to.
> 
> I know exactly what he was referring to. So what's the problem ?
> 
> Yinghai missed to add it to the other place and he is hardly to blame
> for that. This code is messy and thats not his fault.

What on earth? Yes, it is. He added that code. The only thing that this complaint
is about is that his fix-patch added the family 6, model 7 to only _one_ instance
of that code that he himself added and not all three.

>> And would yelling at people how shuffle in code without (publicly at
>> least) addressing one of your fellow arch maintainers objections
> 
> 1) hpa asked a question http://lkml.org/lkml/2008/3/25/118
> 2) his question was answered http://lkml.org/lkml/2008/3/25/292
> 3) hpa did not object (no lkml ref, because there is none)
> 
> So what's your point ? Throwing factoids into a discussion is
> not really helpful.

And HPA (and Molnar) then said in response to that answer:

http://lkml.org/lkml/2008/3/25/322

It's right there in response to your own link. It seems you originally missed
it, but how did you manage this time again?
 
>> and Pavel's review comments about code duplication without a single
>> line of explanation/changelog do?
> 
> As I said before. The changelog is useless and Adrians point about
> that is completely correct. 

Pavel was full of it? 

Rene.

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Adrian Bunk wrote:
> > 
> > Yinghai missed to add it to the other place and he is hardly to blame
> > for that. This code is messy and thats not his fault. It will be
> > cleaned up, but that's not an simple taks to do. If you think you can
> > do it without breaking tons of systems, you're welcome.
> 
> It actually is completely his fault.

No it is not. 

1) this code is messy to begin with

2) we accepted the change as is
 
> His commit ***added*** the three copies of the same code.

And you are not able to ******look******* over the brim of your tea
cup and accept that one of them is 64 bit and the others are a tribute
to the not yet distangled shit. There would have been a cleaner and
better way to solve this, but there is always a better and cleaner way
to do things.

Why dont you send a cleanup patch which fixes the three copies problem
instead of spending your time with writing up ivory tower pieces of
wisdom ?

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[Adrian Bunk]

On Wed, May 07, 2008 at 11:54:19PM +0200, Thomas Gleixner wrote:
> On Thu, 8 May 2008, Adrian Bunk wrote:
> > On Wed, May 07, 2008 at 10:52:36PM +0200, Thomas Gleixner wrote:
>...
> > > Also there is no downside on these older systems. They are fine with
> > > MTRRs and have been for years. We can revisit that once PAT support
> > > has stabilized.
> > 
> > My comment was about:
> > "please try attach patch to see if duron support PAT."
> 
> I know.
>  
> > That information is for sure available.
> 
> So you know where it is and you can confirm that it works fine ?
> 
> Pointers please. 

Rene testing the patch Yinghai Lu sent would can reveal reliably whether 
it works fine for all Duron steppings?

> > > This feature and detection code is hard to clean up and definitely out
> > > of the scope of this patch.
> > 
> > Did you even look at the commit we are discussing?
> > 
> > It ***adds*** exactly the same code at three different places.
> 
> Yes, I did. And it adds it for a fscking good reason. 
> 
> 1) two times in common.c due to the existing detection logic mess
> 2) once in the 64 bit version
> 
> Again, this code is inherited and fragile mess, which can not be
> changed at will.

Please explain why having this code twice in arch/x86/kernel/cpu/common.c
should be safer than doing the logical thing of putting it into an own 
function:

void clear_set_pat(struct cpuinfo_x86 *c)
{
	clear_cpu_cap(c, X86_FEATURE_PAT);

	switch (c->x86_vendor) {
	case X86_VENDOR_AMD:
		if (c->x86 >= 0xf && c->x86 <= 0x11)
			set_cpu_cap(c, X86_FEATURE_PAT);
		break;
	case X86_VENDOR_INTEL:
	if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
			set_cpu_cap(c, X86_FEATURE_PAT);
		break;
	}
}

> > >...
> > > > And this patch (by the author of the code himself) is the first time 
> > > > where it breaks.
> > > 
> > > Very interesting analysis. What broke ? This CPU was never in the set
> > > of supported ones at all.
> > 
> > The patch in the email I answered to was broken since the author did 
> 
> "the author" has a name.
> 
> > fall into his own trap by patching only one copy of his duplicated code.
> 
> That's not a real good reason to yell at him.
> 
> Yinghai  wanted to help out and check with the reporter whether this CPU
> works with PAT or not.
> 
> Yinghai made a mistake.
> 
> You come along and ride a crusade against him for that. Very helpful.

I'm actually on a crusade against:
- the many empty or unusable commit descriptions that recently came
  through the x86 tree
- the fact that Yinghai Lu ignored Pavel's valid commit to not do
  copy&paste programming

> Thanks,
> 
> 	tglx

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

Hi!

> > > This feature and detection code is hard to clean up and definitely out
> > > of the scope of this patch.
> > 
> > Did you even look at the commit we are discussing?
> > 
> > It ***adds*** exactly the same code at three different places.
> 
> Yes, I did. And it adds it for a fscking good reason. 
> 
> 1) two times in common.c due to the existing detection logic mess
> 2) once in the 64 bit version

WTF? The code can happily live in a function. No need to add two
copies to single file. If you need to share function between 32 and 64
bit, just put it to separate .c file.

> > fall into his own trap by patching only one copy of his duplicated code.
> 
> That's not a real good reason to yell at him.

Actually no, that's not a good reason to yell at _him_. But it is
_perfectly valid_ reason to yell at whoever commited that to x86 tree,
given that:

1) it has empty changelog (come on, "review" a patch and not notice
that changelog is empty?!) 

2) HPA said it was bad idea

3) copy&paste code remained in the patch 
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Arjan van de Ven]

On Wed, 07 May 2008 21:08:30 +0200
> > 
> > we filter those for all kinds of things already, sorry.
> > What good is showing "pat" if "pat" isn't deemed usable (yet)????
> > Now *that* is deception :)
> 
> The trouble is -- if you hide that the CPU _should_ have PAT, how many
> people do you expect are going to look further and test? I knew that I
> should have PAT so I distrusted my CPU feature flag display but you've
> now limited your testers to people who've read the CPU datasheet.
> That's really no good.

and... why would we care? there's no upside even if you use pat.
pat is nice to avoid problems on newer systems that run out of mtrrs.
For older systems, right now, there isn't any.

 
> If Linux messes around with those flags already, it's doing things
> wrong already. /proc/cpuinfo is not a display of software features
> but of hardware features -- anything else is outright wrong. Being
> wrong for PAT also doesn't improve things.

that is your interpretation of what that file means.
It's... not so easy.

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
>  3) copy&paste code remained in the patch

i thought to keep the stub so could add more other stuff in the switch
like 64 bit

        clear_cpu_cap(c, X86_FEATURE_PAT);

        switch (c->x86_vendor) {
        case X86_VENDOR_AMD:
                early_init_amd(c);
                if (c->x86 >= 0xf && c->x86 <= 0x11)
                        set_cpu_cap(c, X86_FEATURE_PAT);
                break;
        case X86_VENDOR_INTEL:
                early_init_intel(c);
                if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
                        set_cpu_cap(c, X86_FEATURE_PAT);
                break;
        case X86_VENDOR_CENTAUR:
                early_init_centaur(c);
                break;
        }

YH

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 00:04, Rene Herman wrote:

> On 07-05-08 23:41, Thomas Gleixner wrote:
> 
>>>> Clearing it in the cpuinfo is just a cosmetic side effect which does
>>>> no harm at all.
>>> Oh yes, it does. It makes people unaware that their CPUs _should_ be
>>> supporting PAT. The thing's not called /proc/kernelinfo for a reason.
>>
>> it's named /proc/cpuinfo
> 
> Exactly.

(yes, missed a "not" in the above).

Rene.

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

>  2) HPA said it was bad idea

hpa said white list is not a good idea.

YH

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

>  1) it has empty changelog (come on, "review" a patch and not notice
>  that changelog is empty?!)

the title is good enough

YH

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

> > His commit ***added*** the three copies of the same code.
> 
> And you are not able to ******look******* over the brim of your tea
> cup and accept that one of them is 64 bit and the others are a tribute
> to the not yet distangled shit. There would have been a cleaner and
> better way to solve this, but there is always a better and cleaner way
> to do things.

Existing code being a mess is not an excuse for messing it up a bit
more. Which is exactly what Yinghai did.

> Why dont you send a cleanup patch which fixes the three copies problem
> instead of spending your time with writing up ivory tower pieces of
> wisdom ?

Because no ammount of cleanup patches will solve "Ingo did not even
notice missing changelog, and 2 NAKs"?

commit 9307cacad0dfe3749f00303125c6f7f0523e5616 
tree bc28bf08dc18dacbb2654a9a368f4e09c580dfab 
parent a7c7d0e91daebd7c5e51f9416d612b6a15e7e79a 
author Yinghai Lu <yhlu.kernel.send@gmail.com> Mon, 24 Mar 2008 23:24:34 -0700 
committer Ingo Molnar <mingo@elte.hu> Thu, 17 Apr 2008 17:41:20 +0200 

    x86: pat cpu feature bit setting for known cpus
    
    Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

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

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 00:26, Yinghai Lu wrote:

>>  1) it has empty changelog (come on, "review" a patch and not notice
>>  that changelog is empty?!)
> 
> the title is good enough

And this thread proves it. No, the title wasn't good enough.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 2:41 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 7 May 2008, Rene Herman wrote:
>  > On 07-05-08 22:52, Thomas Gleixner wrote:
>  > You misunderstood. Yinghai's patch only changed one of the code sites
>  > and not the others, which (if I understood right) is the breakage
>  > Adrian was reffering to.
>
>  I know exactly what he was referring to. So what's the problem ?
>
>  Yinghai missed to add it to the other place and he is hardly to blame
>  for that. This code is messy and thats not his fault. It will be
>  cleaned up, but that's not an simple taks to do. If you think you can
>  do it without breaking tons of systems, you're welcome.

yes, i should modify two places in common.c.
but don't need to change the one in setup_64.c because duron is 32 bit only.

YH

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

On Wed 2008-05-07 15:22:37, Yinghai Lu wrote:
> On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
> >  3) copy&paste code remained in the patch
> 
> i thought to keep the stub so could add more other stuff in the switch
> like 64 bit

This is _not_ good enough reason to copy&paste. Just do it like this:

>         switch (c->x86_vendor) {
>         case X86_VENDOR_AMD:
>                 early_init_amd(c);
>                 break;
>         case X86_VENDOR_INTEL:
>                 early_init_intel(c);
>                 break;
>         case X86_VENDOR_CENTAUR:
>                 early_init_centaur(c);
>                 break;
>         }

#         clear_cpu_cap(c, X86_FEATURE_PAT);
#
#         switch (c->x86_vendor) {
#         case X86_VENDOR_AMD:
#                if (c->x86 >= 0xf && c->x86 <= 0x11)
#                         set_cpu_cap(c, X86_FEATURE_PAT);
#                 break;
#         case X86_VENDOR_INTEL:
#                 if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
#                         set_cpu_cap(c, X86_FEATURE_PAT);
#                 break;
#         }

And then, factor out code marked # into separate function, and call it
from all three places.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

On Wed 2008-05-07 15:23:46, Yinghai Lu wrote:
> >  2) HPA said it was bad idea
> 
> hpa said white list is not a good idea.

And guess what your patch is doing?

+       switch (c->x86_vendor) {
+       case X86_VENDOR_AMD:
+               if (c->x86 >= 0xf && c->x86 <= 0x11)
+                       set_cpu_cap(c, X86_FEATURE_PAT);
+               break;
+       case X86_VENDOR_INTEL:
+               if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >=
15))
+                       set_cpu_cap(c, X86_FEATURE_PAT);
+               break;
+       }

Seems like a whitelist to me. Will break with future CPUs.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 3:37 PM, Pavel Machek <pavel@suse.cz> wrote:
> On Wed 2008-05-07 15:22:37, Yinghai Lu wrote:
>  > On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
>  > >  3) copy&paste code remained in the patch
>  >
>  > i thought to keep the stub so could add more other stuff in the switch
>  > like 64 bit
>
>  This is _not_ good enough reason to copy&paste. Just do it like this:
>
>
>  >         switch (c->x86_vendor) {
>  >         case X86_VENDOR_AMD:
>  >                 early_init_amd(c);
>
> >                 break;
>  >         case X86_VENDOR_INTEL:
>  >                 early_init_intel(c);
>
> >                 break;
>  >         case X86_VENDOR_CENTAUR:
>  >                 early_init_centaur(c);
>  >                 break;
>  >         }
>
>  #         clear_cpu_cap(c, X86_FEATURE_PAT);
>
> #
>  #         switch (c->x86_vendor) {
>  #         case X86_VENDOR_AMD:
>  #                if (c->x86 >= 0xf && c->x86 <= 0x11)
>
> #                         set_cpu_cap(c, X86_FEATURE_PAT);
>  #                 break;
>  #         case X86_VENDOR_INTEL:
>  #                 if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
>
> #                         set_cpu_cap(c, X86_FEATURE_PAT);
>  #                 break;
>  #         }
>
>  And then, factor out code marked # into separate function, and call it
>  from all three places.

still need two copies or ifdef, otherwise you will check some 32bit
only cpu fam/model in 64bit node.

YH

Re: 2.6.26, PAT and AMD family 6

[Yinghai Lu]

On Wed, May 7, 2008 at 3:39 PM, Pavel Machek <pavel@suse.cz> wrote:
> On Wed 2008-05-07 15:23:46, Yinghai Lu wrote:
>  > >  2) HPA said it was bad idea
>  >
>  > hpa said white list is not a good idea.
>
>  And guess what your patch is doing?
>
>
>  +       switch (c->x86_vendor) {
>  +       case X86_VENDOR_AMD:
>  +               if (c->x86 >= 0xf && c->x86 <= 0x11)
>  +                       set_cpu_cap(c, X86_FEATURE_PAT);
>  +               break;
>  +       case X86_VENDOR_INTEL:
>  +               if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >=
>  15))
>  +                       set_cpu_cap(c, X86_FEATURE_PAT);
>  +               break;
>  +       }
>
>  Seems like a whitelist to me. Will break with future CPUs.

again, i only increased the whitelist a little bit to support AMD
opteron Rev E, Rev F and Fam10h that I could access.

the code handle future cpus should reach the kernel list before SW
developer could get system....

YH

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Rene Herman wrote:
> On 07-05-08 22:52, Thomas Gleixner wrote:
> 
>>> And why do we need this clear_cpu_cap(c, X86_FEATURE_PAT) and then 
>>> manual setting of X86_FEATURE_PAT at all?
>>
>> The reason is to make cpu_has_pat a useful check and to avoid checking
>> cpu vendors, families and models inside of the PAT code. That's a good
>> thing actually, because the PAT code only cares about that cpu_has_pat
>> flag.
>>
>> Clearing it in the cpuinfo is just a cosmetic side effect which does
>> no harm at all.
> 
> Oh yes, it does. It makes people unaware that their CPUs _should_ be
> supporting PAT. The thing's not called /proc/kernelinfo for a reason.
> 

Okay, that is utter nonsense.  /proc/cpuinfo has always been, and will 
always be, the CPU *AS THE KERNEL SEES IT*.  If you want something else, 
use x86info(1).

> And would yelling at people how shuffle in code without (publicly at
> least) addressing one of your fellow arch maintainers objections and
> Pavel's review comments about code duplication without a single line
> of explanation/changelog do?

We did discuss this (over IRC, I'm afraid), and came to the conclusion 
that it's too risky to do the proper thing (blacklist) straight out the 
gate.  Consider it a staged implementation.  The reason for this is that 
some of the earlier chips have downright frightening errata w.r.t. PAT. 
  *At this point*, we'd have no reasonable way to filter those bug 
reports from the issues with the software itself.

So, one step at a time.  PAT is massively overdue in Linux, so it's no 
wonder you're anxious about it, but we need a modicum of caution here.

	-hpa

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Pavel Machek wrote:
>
> Actually no, that's not a good reason to yell at _him_.

I'm glad, that we can agree on this.

> But it is _perfectly valid_ reason to yell at whoever commited that
> to x86 tree, given that:
>
> 1) it has empty changelog (come on, "review" a patch and not notice
> that changelog is empty?!) 

Granted as I said before.

> 2) HPA said it was bad idea

Interpretation.

> 3) copy&paste code remained in the patch 

Yes, it should have been done better.

So we made mistakes, but is this a justification for magnifying that
to be the major contribution to the end of the universe ?

Definitely not.

I have no problem to admit a mistake, but I vehemently fight the way
how such incidents are artificially exxagerated in the recent past.

I personally have broad enough shoulders to cope with the fact that
bashing on x86 maintainers and trying to play them out against each
other is currently en vogue.

But I have no understanding at all for the snotty way how valuable
contributors are treated even when the responsibility of the actual
merging of their imperfect patch is out of their scope.

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Yinghai Lu wrote:
>>  2) HPA said it was bad idea
> 
> hpa said white list is not a good idea.

I did, but I conceded that it is a reasonable intermediate step.

If the whitelist is there a year from now it's a problem.

	-hpa

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Pavel Machek wrote:
> On Wed 2008-05-07 15:22:37, Yinghai Lu wrote:
> > On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
> > >  3) copy&paste code remained in the patch
> > 
> > i thought to keep the stub so could add more other stuff in the switch
> > like 64 bit
> 
> This is _not_ good enough reason to copy&paste. Just do it like this:
> 
> >         switch (c->x86_vendor) {
> >         case X86_VENDOR_AMD:
> >                 early_init_amd(c);
> >                 break;
> >         case X86_VENDOR_INTEL:
> >                 early_init_intel(c);
> >                 break;
> >         case X86_VENDOR_CENTAUR:
> >                 early_init_centaur(c);
> >                 break;
> >         }
> 
> #         clear_cpu_cap(c, X86_FEATURE_PAT);
> #
> #         switch (c->x86_vendor) {
> #         case X86_VENDOR_AMD:
> #                if (c->x86 >= 0xf && c->x86 <= 0x11)
> #                         set_cpu_cap(c, X86_FEATURE_PAT);
> #                 break;
> #         case X86_VENDOR_INTEL:
> #                 if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
> #                         set_cpu_cap(c, X86_FEATURE_PAT);
> #                 break;
> #         }
> 
> And then, factor out code marked # into separate function, and call it
> from all three places.

And while you are at it, why don't you send a patch which makes this
all go away instead of wasting time producing pseudo code?

Thanks,

	tglx

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

On Wed 2008-05-07 15:40:38, Yinghai Lu wrote:
> On Wed, May 7, 2008 at 3:37 PM, Pavel Machek <pavel@suse.cz> wrote:
> > On Wed 2008-05-07 15:22:37, Yinghai Lu wrote:
> >  > On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
> >  > >  3) copy&paste code remained in the patch
> >  >
> >  > i thought to keep the stub so could add more other stuff in the switch
> >  > like 64 bit
> >
> >  This is _not_ good enough reason to copy&paste. Just do it like this:
> >
> >
> >  >         switch (c->x86_vendor) {
> >  >         case X86_VENDOR_AMD:
> >  >                 early_init_amd(c);
> >
> > >                 break;
> >  >         case X86_VENDOR_INTEL:
> >  >                 early_init_intel(c);
> >
> > >                 break;
> >  >         case X86_VENDOR_CENTAUR:
> >  >                 early_init_centaur(c);
> >  >                 break;
> >  >         }
> >
> >  #         clear_cpu_cap(c, X86_FEATURE_PAT);
> >
> > #
> >  #         switch (c->x86_vendor) {
> >  #         case X86_VENDOR_AMD:
> >  #                if (c->x86 >= 0xf && c->x86 <= 0x11)
> >
> > #                         set_cpu_cap(c, X86_FEATURE_PAT);
> >  #                 break;
> >  #         case X86_VENDOR_INTEL:
> >  #                 if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
> >
> > #                         set_cpu_cap(c, X86_FEATURE_PAT);
> >  #                 break;
> >  #         }
> >
> >  And then, factor out code marked # into separate function, and call it
> >  from all three places.
> 
> still need two copies or ifdef, otherwise you will check some 32bit
> only cpu fam/model in 64bit node.

So we'll check 32-bit only cpu in 64bit node. What is the problem?
Being microsecond slower during startup?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

On Wed 2008-05-07 15:45:18, Yinghai Lu wrote:
> On Wed, May 7, 2008 at 3:39 PM, Pavel Machek <pavel@suse.cz> wrote:
> > On Wed 2008-05-07 15:23:46, Yinghai Lu wrote:
> >  > >  2) HPA said it was bad idea
> >  >
> >  > hpa said white list is not a good idea.
> >
> >  And guess what your patch is doing?
> >
> >
> >  +       switch (c->x86_vendor) {
> >  +       case X86_VENDOR_AMD:
> >  +               if (c->x86 >= 0xf && c->x86 <= 0x11)
> >  +                       set_cpu_cap(c, X86_FEATURE_PAT);
> >  +               break;
> >  +       case X86_VENDOR_INTEL:
> >  +               if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >=
> >  15))
> >  +                       set_cpu_cap(c, X86_FEATURE_PAT);
> >  +               break;
> >  +       }
> >
> >  Seems like a whitelist to me. Will break with future CPUs.
> 
> again, i only increased the whitelist a little bit to support AMD
> opteron Rev E, Rev F and Fam10h that I could access.

(Which is what your commit log should have said, btw).

> the code handle future cpus should reach the kernel list before SW
> developer could get system....

There's no reason why 2.6.26 should not work reasonably on cpus
released in 2015.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Pavel Machek]

On Thu 2008-05-08 01:02:52, Thomas Gleixner wrote:
> On Thu, 8 May 2008, Pavel Machek wrote:
> > On Wed 2008-05-07 15:22:37, Yinghai Lu wrote:
> > > On Wed, May 7, 2008 at 3:14 PM, Pavel Machek <pavel@suse.cz> wrote:
> > > >  3) copy&paste code remained in the patch
> > > 
> > > i thought to keep the stub so could add more other stuff in the switch
> > > like 64 bit
> > 
> > This is _not_ good enough reason to copy&paste. Just do it like this:
> > 
> > >         switch (c->x86_vendor) {
> > >         case X86_VENDOR_AMD:
> > >                 early_init_amd(c);
> > >                 break;
> > >         case X86_VENDOR_INTEL:
> > >                 early_init_intel(c);
> > >                 break;
> > >         case X86_VENDOR_CENTAUR:
> > >                 early_init_centaur(c);
> > >                 break;
> > >         }
> > 
> > #         clear_cpu_cap(c, X86_FEATURE_PAT);
> > #
> > #         switch (c->x86_vendor) {
> > #         case X86_VENDOR_AMD:
> > #                if (c->x86 >= 0xf && c->x86 <= 0x11)
> > #                         set_cpu_cap(c, X86_FEATURE_PAT);
> > #                 break;
> > #         case X86_VENDOR_INTEL:
> > #                 if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
> > #                         set_cpu_cap(c, X86_FEATURE_PAT);
> > #                 break;
> > #         }
> > 
> > And then, factor out code marked # into separate function, and call it
> > from all three places.
> 
> And while you are at it, why don't you send a patch which makes this
> all go away instead of wasting time producing pseudo code?

Because I expect Ingo & Yinghai to do the work, and then test it, and
then add changelog, and then commit it. I expect Yinghai to learn how
to use functions in the process.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Pavel Machek wrote:
> On Thu 2008-05-08 01:02:52, Thomas Gleixner wrote:
> > On Thu, 8 May 2008, Pavel Machek wrote:
> > > And then, factor out code marked # into separate function, and call it
> > > from all three places.
> > 
> > And while you are at it, why don't you send a patch which makes this
> > all go away instead of wasting time producing pseudo code?
> 
> Because I expect Ingo & Yinghai to do the work, and then test it, and
> then add changelog, and then commit it. I expect Yinghai to learn how
> to use functions in the process.

Oh yes. He is the perfect kernel newbie who needs to be tought how to
use functions.

No, he is not. He provided a lot of valuable patches and he always was
cooperative when his patches were reviewed.

You are completely missing the point. We, the x86 maintainers accepted
and committed that functional correct but stylistically imperfect
patch. It's mainline now. We can spend tons of time to discuss how it
could have done better, but isn't it one of the virtues of Open Source
that we can actually prove that it can be done better ? From such a
patch others can learn at least as much as from ivory tower post
factum analysis.

We can all sit down and resort to "I expect that XYZ do the work". Is
this making things any better?

Thanks,

	tglx

P.S: we all wasted at least 10 times of the time to write that patch. :(

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 00:57, H. Peter Anvin wrote:

>>> Clearing it in the cpuinfo is just a cosmetic side effect which does
>>> no harm at all.
>>
>> Oh yes, it does. It makes people unaware that their CPUs _should_ be
>> supporting PAT. The thing's not called /proc/kernelinfo for a reason.
>>
> 
> Okay, that is utter nonsense.  /proc/cpuinfo has always been, and will 
> always be, the CPU *AS THE KERNEL SEES IT*.  If you want something else, 
> use x86info(1).

What a lovely way of syncing reality with your definitions. The kernel
_does_ see that my CPU features PAT, it just refuses to use it because
it doesn't trust it enough. Vital difference. Maybe not to the kernel,
but definitely to me, the user. /proc/cpuinfo is a user interface.

PAT is a CPU, hardware, flag documented in the arch manuals. If the
kernel wants to keep software flags as well why not simply add those?

>> And would yelling at people how shuffle in code without (publicly at
>> least) addressing one of your fellow arch maintainers objections and
>> Pavel's review comments about code duplication without a single line
>> of explanation/changelog do?
> 
> We did discuss this (over IRC, I'm afraid), and came to the conclusion 
> that it's too risky to do the proper thing (blacklist) straight out the 
> gate.  Consider it a staged implementation.  The reason for this is that 
> some of the earlier chips have downright frightening errata w.r.t. PAT. 
>  *At this point*, we'd have no reasonable way to filter those bug 
> reports from the issues with the software itself.
> 
> So, one step at a time.  PAT is massively overdue in Linux, so it's no 
> wonder you're anxious about it, but we need a modicum of caution here.

Really, I'm not so much anxious about PAT as I got anxious about seeing
my entire CPU famility dropped without a single explanation. Had the one
you provide above be in the commit message this thread had not happened
(yes, although redefining X86_FEATURE_PAT is still crap).

As it's merged, nothing said that it was specifically not using and/or
disabling PAT just that it wasn't supported. Even if it had done _that_
much this thread would've probably not happened.

Rene.

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Rene Herman wrote:
>>
>> Okay, that is utter nonsense.  /proc/cpuinfo has always been, and will 
>> always be, the CPU *AS THE KERNEL SEES IT*.  If you want something 
>> else, use x86info(1).
> 
> What a lovely way of syncing reality with your definitions. The kernel
> _does_ see that my CPU features PAT, it just refuses to use it because
> it doesn't trust it enough. Vital difference. Maybe not to the kernel,
> but definitely to me, the user. /proc/cpuinfo is a user interface.
> 

No, /proc/cpuinfo is informing the user about the kernel's view of the 
CPU.  It has always been the "cooked" view of CPUID, whether or not you 
like it.

	-hpa

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 02:03, H. Peter Anvin wrote:
> Rene Herman wrote:
>>>
>>> Okay, that is utter nonsense.  /proc/cpuinfo has always been, and 
>>> will always be, the CPU *AS THE KERNEL SEES IT*.  If you want 
>>> something else, use x86info(1).
>>
>> What a lovely way of syncing reality with your definitions. The kernel
>> _does_ see that my CPU features PAT, it just refuses to use it because
>> it doesn't trust it enough. Vital difference. Maybe not to the kernel,
>> but definitely to me, the user. /proc/cpuinfo is a user interface.
>>
> 
> No, /proc/cpuinfo is informing the user about the kernel's view of the 
> CPU.

And again, the kernel DOES see that my CPU features PAT, it just refuses
to use it.

Rene.

Re: 2.6.26, PAT and AMD family 6

[Linus Torvalds]

On Wed, 7 May 2008, H. Peter Anvin wrote:
> > 
> > What a lovely way of syncing reality with your definitions. The kernel
> > _does_ see that my CPU features PAT, it just refuses to use it because
> > it doesn't trust it enough. Vital difference. Maybe not to the kernel,
> > but definitely to me, the user. /proc/cpuinfo is a user interface.
> > 
> 
> No, /proc/cpuinfo is informing the user about the kernel's view of the CPU.
> It has always been the "cooked" view of CPUID, whether or not you like it.

Indeed. If people want to know what the CPU itself reports, they should 
just use the "cpuid" instruction directly. The /proc/cpuinfo file has 
been a window into the kernels notion of what is going on, and disabling 
kernel features have disabled the bits in that file as appropriate (eg 
"clearcpuid=xyzzy" and "mem=nopentium" etc).

Of course, the *common* case is that the two will match 100% in features. 
But the kernel view has some of its very own set of CPU features too.

And it is possible that the "PAT as the kernel sees it" should be a 
*separate* bit from "PAT as the cpu itself reports to support it". We do 
that for quite a lot of the "synthesized" features, eg there is the "TSC" 
feature as the CPU reports it, and then we have the "CONSTANT_TSC" feature 
that is the kernel version of it that says that we have a TSC _and_ it 
runs at a constant rate too (or the "UP" bit, or the "Mfence synchronizes 
RDTSC" bit etc etc).

So it's possible that we should do the same thing for PAT - and allow 
people to see the CPU view _and_ the kernel view as two separate issues. 
In many ways that would be the logical thing to do. Hmm?

		Linus

Re: 2.6.26, PAT and AMD family 6

[Linus Torvalds]

On Thu, 8 May 2008, Rene Herman wrote:
> 
> And again, the kernel DOES see that my CPU features PAT, it just refuses
> to use it.

Umm, and this is different how from clearing the PSE bit when we decide 
not to use large-page support? 

See arch/x86/kernel/setup_32.c: parse_mem() for an example.

Rene - you have valid points (ie that patches were NAK'ed etc and you 
might have wanted better changelog entries), but trust me, this one is not 
one of them. /proc/cpuinfo and the kernel clearing feature bits is 
_normal_. It's how it has always been.

			Linus

Re: 2.6.26, PAT and AMD family 6

[Thomas Gleixner]

On Thu, 8 May 2008, Rene Herman wrote:
> On 08-05-08 02:03, H. Peter Anvin wrote:
> > Rene Herman wrote:
> > > > 
> > > > Okay, that is utter nonsense.  /proc/cpuinfo has always been, and will
> > > > always be, the CPU *AS THE KERNEL SEES IT*.  If you want something else,
> > > > use x86info(1).
> > > 
> > > What a lovely way of syncing reality with your definitions. The kernel
> > > _does_ see that my CPU features PAT, it just refuses to use it because
> > > it doesn't trust it enough. Vital difference. Maybe not to the kernel,
> > > but definitely to me, the user. /proc/cpuinfo is a user interface.
> > > 
> > 
> > No, /proc/cpuinfo is informing the user about the kernel's view of the CPU.
> 
> And again, the kernel DOES see that my CPU features PAT, it just refuses
> to use it.

Yuck, because your CPU has the PAT feature the kernel has an obligation
to support it ?

Nice try.

	tglx

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 02:19, Linus Torvalds wrote:
> 
> On Thu, 8 May 2008, Rene Herman wrote:
>> And again, the kernel DOES see that my CPU features PAT, it just refuses
>> to use it.
> 
> Umm, and this is different how from clearing the PSE bit when we decide 
> not to use large-page support? 

It's not. That one's bad also. It's just that this one is the first one I
notice due to them blasted people screwing over my lowly Duron.

I completely and fully agree with your new flag suggestion. Was just in a
git bisect and am actually on the lowly thing in question so I'm not fast
but was looking already. X86_FEATURE_PAT_GOOD. Is it safe to reuse bit 14
now?

Rene.

Re: 2.6.26, PAT and AMD family 6

[Rene Herman]

On 08-05-08 02:21, Thomas Gleixner wrote:

> On Thu, 8 May 2008, Rene Herman wrote:

>> And again, the kernel DOES see that my CPU features PAT, it just refuses
>> to use it.
> 
> Yuck, because your CPU has the PAT feature the kernel has an obligation
> to support it ?
> 
> Nice try.

No, damnit, it does not need to use it, it needs to NOT HIDE IT FROM ME.

Rene.

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Wed, 7 May 2008, H. Peter Anvin wrote:
>>> What a lovely way of syncing reality with your definitions. The kernel
>>> _does_ see that my CPU features PAT, it just refuses to use it because
>>> it doesn't trust it enough. Vital difference. Maybe not to the kernel,
>>> but definitely to me, the user. /proc/cpuinfo is a user interface.
>>>
>> No, /proc/cpuinfo is informing the user about the kernel's view of the CPU.
>> It has always been the "cooked" view of CPUID, whether or not you like it.
> 
> Indeed. If people want to know what the CPU itself reports, they should 
> just use the "cpuid" instruction directly. The /proc/cpuinfo file has 
> been a window into the kernels notion of what is going on, and disabling 
> kernel features have disabled the bits in that file as appropriate (eg 
> "clearcpuid=xyzzy" and "mem=nopentium" etc).
> 
> Of course, the *common* case is that the two will match 100% in features. 
> But the kernel view has some of its very own set of CPU features too.
> 
> And it is possible that the "PAT as the kernel sees it" should be a 
> *separate* bit from "PAT as the cpu itself reports to support it". We do 
> that for quite a lot of the "synthesized" features, eg there is the "TSC" 
> feature as the CPU reports it, and then we have the "CONSTANT_TSC" feature 
> that is the kernel version of it that says that we have a TSC _and_ it 
> runs at a constant rate too (or the "UP" bit, or the "Mfence synchronizes 
> RDTSC" bit etc etc).
> 
> So it's possible that we should do the same thing for PAT - and allow 
> people to see the CPU view _and_ the kernel view as two separate issues. 
> In many ways that would be the logical thing to do. Hmm?
> 

Well, there *is* support for that - all the raw information is there in 
/dev/cpu/*/cpuid.  There are other reasons why /proc/cpuinfo is the 
wrong interface to try to get the "real" CPUID information - we only 
report CPU features that the kernel knows about; bits we don't, if we 
can decode them at all, we just don't show.

	-hpa

[PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Rene Herman]

On 08-05-08 02:28, Rene Herman wrote:

> I completely and fully agree with your new flag suggestion. Was just
> in a git bisect and am actually on the lowly thing in question so I'm
> not fast but was looking already. X86_FEATURE_PAT_GOOD. Is it safe to
> reuse bit 14 now?

Okay, so how's this? Seem to work well for me and makes me happy. Only
tested on UP.

(hope the inline thing works)

 arch/x86/kernel/cpu/common.c        |   12 ++++--------
 arch/x86/kernel/cpu/feature_names.c |    2 +-
 arch/x86/kernel/setup_64.c          |    7 ++-----
 arch/x86/mm/pat.c                   |    2 +-
 include/asm-x86/cpufeature.h        |    3 ++-
 5 files changed, 10 insertions(+), 16 deletions(-)

x86: introduce a new Linux defined feature flag for PAT support

Use a new Linux defined X86_FEATURE_PAT_GOOD feature flag to
indicate we're on a CPU we want to support PAT on, so as to
keep the CPU defined X86_FEATURE_PAT readily available for
indicating if the CPU even has the PAT feature.

Signed-off-by: Rene Herman <rene.herman@gmail.com>

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 35b4f6a..7bec5e0 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -308,16 +308,14 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
 
 	}
 
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
 	switch (c->x86_vendor) {
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	}
 
@@ -409,16 +407,14 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 		init_scattered_cpuid_features(c);
 	}
 
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
 	switch (c->x86_vendor) {
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	}
 }
diff --git a/arch/x86/kernel/cpu/feature_names.c b/arch/x86/kernel/cpu/feature_names.c
index e43ad4a..04f996d 100644
--- a/arch/x86/kernel/cpu/feature_names.c
+++ b/arch/x86/kernel/cpu/feature_names.c
@@ -38,7 +38,7 @@ const char * const x86_cap_flags[NCAPINTS*32] = {
 	"cxmmx", "k6_mtrr", "cyrix_arr", "centaur_mcr",
 	NULL, NULL, NULL, NULL,
 	"constant_tsc", "up", NULL, "arch_perfmon",
-	"pebs", "bts", NULL, NULL,
+	"pebs", "bts", "pat_good", NULL,
 	"rep_good", NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 	NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
 
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 22c14e2..460134e 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -1063,19 +1063,16 @@ static void __cpuinit early_identify_cpu(struct cpuinfo_x86 *c)
 	if (c->extended_cpuid_level >= 0x80000007)
 		c->x86_power = cpuid_edx(0x80000007);
 
-
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
 	switch (c->x86_vendor) {
 	case X86_VENDOR_AMD:
 		early_init_amd(c);
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_INTEL:
 		early_init_intel(c);
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_CENTAUR:
 		early_init_centaur(c);
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 277446c..6ee3efb 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -43,7 +43,7 @@ static int pat_known_cpu(void)
 	if (!pat_wc_enabled)
 		return 0;
 
-	if (cpu_has_pat)
+	if (cpu_has_pat && cpu_has_pat_good)
 		return 1;
 
 	pat_wc_enabled = 0;
diff --git a/include/asm-x86/cpufeature.h b/include/asm-x86/cpufeature.h
index 0d609c8..63c14e4 100644
--- a/include/asm-x86/cpufeature.h
+++ b/include/asm-x86/cpufeature.h
@@ -74,7 +74,7 @@
 #define X86_FEATURE_ARCH_PERFMON (3*32+11) /* Intel Architectural PerfMon */
 #define X86_FEATURE_PEBS	(3*32+12)  /* Precise-Event Based Sampling */
 #define X86_FEATURE_BTS		(3*32+13)  /* Branch Trace Store */
-/* 14 free */
+#define X86_FEATURE_PAT_GOOD	(3*32+14) /* PAT works correctly */
 /* 15 free */
 #define X86_FEATURE_REP_GOOD	(3*32+16) /* rep microcode works well on this CPU */
 #define X86_FEATURE_MFENCE_RDTSC (3*32+17) /* Mfence synchronizes RDTSC */
@@ -187,6 +187,7 @@ extern const char * const x86_power_flags[32];
 #define cpu_has_gbpages		boot_cpu_has(X86_FEATURE_GBPAGES)
 #define cpu_has_arch_perfmon	boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
 #define cpu_has_pat		boot_cpu_has(X86_FEATURE_PAT)
+#define cpu_has_pat_good	boot_cpu_has(X86_FEATURE_PAT_GOOD)
 
 #if defined(CONFIG_X86_INVLPG) || defined(CONFIG_X86_64)
 # define cpu_has_invlpg		1

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Linus Torvalds]

On Thu, 8 May 2008, Rene Herman wrote:
> 
> Okay, so how's this? Seem to work well for me and makes me happy. Only
> tested on UP.

Looks sane to me.

Not that I ever really saw the point about this whole argument in the 
first place. Clearing the flags wasn't really wrong to begin with.

		Linus

[PATCH] x86: enable PAT support on AMD Duron model 7

[Rene Herman]

On 08-05-08 03:57, Rene Herman wrote:

> Okay, so how's this? Seem to work well for me and makes me happy. Only
> tested on UP.

With this on top, I'm ecstatic:

  x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106


x86: enable PAT support on AMD Duron model 7

PAT support seems to work well on AMD Duron model 7. Enable it.

Signed-off-by: Rene Herman <rene.herman@gmail.com>
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 7bec5e0..39b308b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -312,6 +312,8 @@ static void __cpuinit early_get_cap(struct cpuinfo_x86 *c)
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
 			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
+		if (c->x86 == 6 && c->x86_model == 7)
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
@@ -411,6 +413,8 @@ static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
 			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
+		if (c->x86 == 6 && c->x86_model == 7)
+			set_cpu_cap(c, X86_FEATURE_PAT_GOOD);
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))

Re: [PATCH] x86: enable PAT support on AMD Duron model 7

[Arjan van de Ven]

On Thu, 08 May 2008 04:04:05 +0200
Rene Herman <rene.herman@keyaccess.nl> wrote:

> On 08-05-08 03:57, Rene Herman wrote:
> 
> > Okay, so how's this? Seem to work well for me and makes me happy.
> > Only tested on UP.
> 
> With this on top, I'm ecstatic:
> 
>   x86 PAT enabled: cpu 0, old 0x7040600070406, new 0x7010600070106
> 
> 
> x86: enable PAT support on AMD Duron model 7
> 
> PAT support seems to work well on AMD Duron model 7. Enable it.

have you read the errata sheets for this one?
have you tried various cachability/mtrr combinations (with a new
enough X)?

just booting doesn't say much if the feature won't just randomly
tripple fault later on

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Thu, 8 May 2008, Rene Herman wrote:
>> Okay, so how's this? Seem to work well for me and makes me happy. Only
>> tested on UP.
> 
> Looks sane to me.
> 
> Not that I ever really saw the point about this whole argument in the 
> first place. Clearing the flags wasn't really wrong to begin with.
> 

Indeed it wasn't, and at least I have no interest of maintaining what is 
in effect an in-kernel version of x86info(1).

*Certainly* I don't want anything like this crap:

> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 277446c..6ee3efb 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -43,7 +43,7 @@ static int pat_known_cpu(void)
>     if (!pat_wc_enabled)
>         return 0;
> 
> -    if (cpu_has_pat)
> +    if (cpu_has_pat && cpu_has_pat_good)
>         return 1;
> 
>     pat_wc_enabled = 0; 

	-hpa

Re: [PATCH] x86: enable PAT support on AMD Duron model 7

[Rene Herman]

On 08-05-08 04:08, Arjan van de Ven wrote:

>> x86: enable PAT support on AMD Duron model 7
>>
>> PAT support seems to work well on AMD Duron model 7. Enable it.
> 
> have you read the errata sheets for this one?
> have you tried various cachability/mtrr combinations (with a new
> enough X)?
> 
> just booting doesn't say much if the feature won't just randomly
> tripple fault later on

No, I haven't, but it's the same thing Yinghai sent to me earlier.
Shouldn't he have? Any indications that it might not work?

Rene.

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Rene Herman]

On 08-05-08 04:11, H. Peter Anvin wrote:

> Indeed it wasn't, and at least I have no interest of maintaining what is 
> in effect an in-kernel version of x86info(1).
> 
> *Certainly* I don't want anything like this crap:
> 
>> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
>> index 277446c..6ee3efb 100644
>> --- a/arch/x86/mm/pat.c
>> +++ b/arch/x86/mm/pat.c
>> @@ -43,7 +43,7 @@ static int pat_known_cpu(void)
>>     if (!pat_wc_enabled)
>>         return 0;
>>
>> -    if (cpu_has_pat)
>> +    if (cpu_has_pat && cpu_has_pat_good)
>>         return 1;

if (cpu_has_pat_good) would have been the exact same as now. Feel free
to drop the cpu_has_pat one but it's not crap. That whitelist thing
checks nothing -- and things like CONSTANT_TSC also don't, so I didn't
make the feature itself conditional. It has but one call site anyway.

Rene.

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Linus Torvalds]

On Wed, 7 May 2008, H. Peter Anvin wrote:
> 
> Indeed it wasn't, and at least I have no interest of maintaining what is in
> effect an in-kernel version of x86info(1).

Umm. We already do, since we effectively ignore what the actual hardware 
says, and replace it with our own version.

> *Certainly* I don't want anything like this crap:
> 
> > -    if (cpu_has_pat)
> > +    if (cpu_has_pat && cpu_has_pat_good)

This in fact is likely the best part of it.

Because that at least guarantees that we never say we have a good PAT when 
the hardware doesn't even report it.

As it is, we seem to just blindly override hardware. It may be correct for 
all the models we override, but still..

		Linus

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[H. Peter Anvin]

Linus Torvalds wrote:
> 
>> *Certainly* I don't want anything like this crap:
>>
>>> -    if (cpu_has_pat)
>>> +    if (cpu_has_pat && cpu_has_pat_good)
> 
> This in fact is likely the best part of it.
> 
> Because that at least guarantees that we never say we have a good PAT when 
> the hardware doesn't even report it.
> 
> As it is, we seem to just blindly override hardware. It may be correct for 
> all the models we override, but still..
> 

Yah, this is not good.  We should mask out the bit, but never, ever, set 
it if it was clear to begin with (unless we have it on really, *really*, 
good authority.)

I'm embarrassed to have let that slink by.

	-hpa

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Yinghai Lu wrote:
> 
> may add enable_pat command line, so other guys could test if their
> cpus are ok with PAT...
> 

That would definitely be good.

Also, we should *not* turn on the PAT bit if it was disabled to begin with.

	-hpa

Re: 2.6.26, PAT and AMD family 6

[Andi Kleen]

"H. Peter Anvin" <hpa@zytor.com> writes:
>
> Well, there *is* support for that - all the raw information is there
> in /dev/cpu/*/cpuid.  There are other reasons why /proc/cpuinfo is the
> wrong interface to try to get the "real" CPUID information - we only
> report CPU features that the kernel knows about; bits we don't, if we
> can decode them at all, we just don't show.

They are reported as numbers (assuming they are in the existing leaves)

-Andi

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Andi Kleen]

Rene Herman <rene.herman@keyaccess.nl> writes:
>
> Use a new Linux defined X86_FEATURE_PAT_GOOD feature flag to

Better would be PAT_TESTED or PAT_RANDOMLY_APPROVED.
Most of these CPUs without PAT_GOOD have actually perfectly good PAT, as 
Windows proves every day.

The main flaw in all of this of course is that there is no procedure to test
CPUs which do not have the flag set yet.

-Andi

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Rene Herman]

On 08-05-08 12:19, Andi Kleen wrote:

>> Use a new Linux defined X86_FEATURE_PAT_GOOD feature flag to
> 
> Better would be PAT_TESTED or PAT_RANDOMLY_APPROVED. Most of these
> CPUs without PAT_GOOD have actually perfectly good PAT, as Windows
> proves every day.
> 
> The main flaw in all of this of course is that there is no procedure
> to test CPUs which do not have the flag set yet.

Quite. And hiding the fact that the CPU _should_ have perfectly good
PAT doesn't help any at all. The discussion turned into a mini-flame
war enough that now noone would even consider backing down, but this
current PAT setup just sucks plain and simple.

Rene.

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Thomas Gleixner]

On Wed, 7 May 2008, H. Peter Anvin wrote:
> Linus Torvalds wrote:
> > 
> > > *Certainly* I don't want anything like this crap:
> > > 
> > > > -    if (cpu_has_pat)
> > > > +    if (cpu_has_pat && cpu_has_pat_good)
> > 
> > This in fact is likely the best part of it.
> > 
> > Because that at least guarantees that we never say we have a good PAT when
> > the hardware doesn't even report it.
> > 
> > As it is, we seem to just blindly override hardware. It may be correct for
> > all the models we override, but still..
> > 
> 
> Yah, this is not good.  We should mask out the bit, but never, ever, set it if
> it was clear to begin with (unless we have it on really, *really*, good
> authority.)
> 
> I'm embarrassed to have let that slink by.

/me too.

Fix below.

Thanks,
	tglx

---------->
Subject: x86: cleanup PAT cpu validation
From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 08 May 2008 09:18:43 +0200

Move the scattered checks for PAT support to a single function. Its
moved to addon_cpuid_features.c as this file is shared between 32 and
64 bit.

Remove the manipulation of the PAT feature bit and just disable PAT in
the PAT layer, based on the PAT bit provided by the CPU and the
current CPU version/model white list.

Change the boot CPU check so it works on Voyager somewhere in the
future as well :) Also panic, when a secondary has PAT disabled but
the primary one has alrady switched to PAT. We have no way to undo
that.

The white list is kept for now to ensure that we can rely on known to
work CPU types and concentrate on the software induced problems
instead of fighthing CPU erratas and subtle wreckage caused by not yet
verified CPUs. Once the PAT code has stabilized enough, we can remove
the white list and open the can of worms.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/addon_cpuid_features.c |   21 ++++++++++++
 arch/x86/kernel/cpu/common.c               |   27 +--------------
 arch/x86/kernel/setup_64.c                 |    9 +----
 arch/x86/mm/pat.c                          |   50 ++++++++++++-----------------
 include/asm-x86/pat.h                      |    8 ++++
 5 files changed, 55 insertions(+), 60 deletions(-)

Index: linux-2.6/arch/x86/kernel/cpu/addon_cpuid_features.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/addon_cpuid_features.c
+++ linux-2.6/arch/x86/kernel/cpu/addon_cpuid_features.c
@@ -6,6 +6,7 @@
 
 #include <linux/cpu.h>
 
+#include <asm/pat.h>
 #include <asm/processor.h>
 
 struct cpuid_bit {
@@ -48,3 +49,23 @@ void __cpuinit init_scattered_cpuid_feat
 			set_cpu_cap(c, cb->feature);
 	}
 }
+
+#ifdef CONFIG_X86_PAT
+void __cpuinit validate_pat_support(struct cpuinfo_x86 *c)
+{
+	switch (c->x86_vendor) {
+	case X86_VENDOR_AMD:
+		if (c->x86 >= 0xf && c->x86 <= 0x11)
+			return;
+		break;
+	case X86_VENDOR_INTEL:
+		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
+			return;
+		break;
+	}
+
+	pat_disable(cpu_has_pat ?
+		    "PAT disabled. Not yet verified on this CPU type." :
+		    "PAT not supported by CPU.");
+}
+#endif
Index: linux-2.6/arch/x86/kernel/cpu/common.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/common.c
+++ linux-2.6/arch/x86/kernel/cpu/common.c
@@ -12,6 +12,7 @@
 #include <asm/mmu_context.h>
 #include <asm/mtrr.h>
 #include <asm/mce.h>
+#include <asm/pat.h>
 #ifdef CONFIG_X86_LOCAL_APIC
 #include <asm/mpspec.h>
 #include <asm/apic.h>
@@ -308,19 +309,6 @@ static void __cpuinit early_get_cap(stru
 
 	}
 
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
-	switch (c->x86_vendor) {
-	case X86_VENDOR_AMD:
-		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	case X86_VENDOR_INTEL:
-		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	}
-
 }
 
 /*
@@ -409,18 +397,6 @@ static void __cpuinit generic_identify(s
 		init_scattered_cpuid_features(c);
 	}
 
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
-	switch (c->x86_vendor) {
-	case X86_VENDOR_AMD:
-		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	case X86_VENDOR_INTEL:
-		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
-		break;
-	}
 }
 
 static void __cpuinit squash_the_stupid_serial_number(struct cpuinfo_x86 *c)
@@ -651,6 +627,7 @@ void __init early_cpu_init(void)
 		cpu_devs[cvdev->vendor] = cvdev->cpu_dev;
 
 	early_cpu_detect();
+	validate_pat_support(&boot_cpu_data);
 }
 
 /* Make sure %fs is initialized properly in idle threads */
Index: linux-2.6/arch/x86/kernel/setup_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup_64.c
+++ linux-2.6/arch/x86/kernel/setup_64.c
@@ -70,6 +70,7 @@
 #include <asm/ds.h>
 #include <asm/topology.h>
 #include <asm/trampoline.h>
+#include <asm/pat.h>
 
 #include <mach_apic.h>
 #ifdef CONFIG_PARAVIRT
@@ -1063,25 +1064,19 @@ static void __cpuinit early_identify_cpu
 	if (c->extended_cpuid_level >= 0x80000007)
 		c->x86_power = cpuid_edx(0x80000007);
 
-
-	clear_cpu_cap(c, X86_FEATURE_PAT);
-
 	switch (c->x86_vendor) {
 	case X86_VENDOR_AMD:
 		early_init_amd(c);
-		if (c->x86 >= 0xf && c->x86 <= 0x11)
-			set_cpu_cap(c, X86_FEATURE_PAT);
 		break;
 	case X86_VENDOR_INTEL:
 		early_init_intel(c);
-		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
-			set_cpu_cap(c, X86_FEATURE_PAT);
 		break;
 	case X86_VENDOR_CENTAUR:
 		early_init_centaur(c);
 		break;
 	}
 
+	validate_pat_support(c);
 }
 
 /*
Index: linux-2.6/arch/x86/mm/pat.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/pat.c
+++ linux-2.6/arch/x86/mm/pat.c
@@ -25,31 +25,24 @@
 #include <asm/mtrr.h>
 #include <asm/io.h>
 
-int pat_wc_enabled = 1;
+#ifdef CONFIG_X86_PAT
+int __read_mostly pat_wc_enabled = 1;
 
-static u64 __read_mostly boot_pat_state;
-
-static int nopat(char *str)
+void __init pat_disable(char *reason)
 {
 	pat_wc_enabled = 0;
-	printk(KERN_INFO "x86: PAT support disabled.\n");
-
-	return 0;
+	printk(KERN_INFO "%s\n", reason);
 }
-early_param("nopat", nopat);
 
-static int pat_known_cpu(void)
+static int nopat(char *str)
 {
-	if (!pat_wc_enabled)
-		return 0;
-
-	if (cpu_has_pat)
-		return 1;
-
-	pat_wc_enabled = 0;
-	printk(KERN_INFO "CPU and/or kernel does not support PAT.\n");
+	pat_disable("PAT support disabled.");
 	return 0;
 }
+early_param("nopat", nopat);
+#endif
+
+static u64 __read_mostly boot_pat_state;
 
 enum {
 	PAT_UC = 0,		/* uncached */
@@ -66,17 +59,19 @@ void pat_init(void)
 {
 	u64 pat;
 
-#ifndef CONFIG_X86_PAT
-	nopat(NULL);
-#endif
-
-	/* Boot CPU enables PAT based on CPU feature */
-	if (!smp_processor_id() && !pat_known_cpu())
+	if (!pat_wc_enabled)
 		return;
 
-	/* APs enable PAT iff boot CPU has enabled it before */
-	if (smp_processor_id() && !pat_wc_enabled)
-		return;
+	/* Paranoia check. */
+	if (!cpu_has_pat) {
+		printk(KERN_ERR "PAT enabled, but CPU feature cleared\n");
+		/*
+		 * Panic if this happens on the secondary CPU, and we
+		 * switched to PAT on the boot CPU. We have no way to
+		 * undo PAT.
+		*/
+		BUG_ON(boot_pat_state);
+	}
 
 	/* Set PWT to Write-Combining. All other bits stay the same */
 	/*
@@ -95,9 +90,8 @@ void pat_init(void)
 	      PAT(4,WB) | PAT(5,WC) | PAT(6,UC_MINUS) | PAT(7,UC);
 
 	/* Boot CPU check */
-	if (!smp_processor_id()) {
+	if (!boot_pat_state)
 		rdmsrl(MSR_IA32_CR_PAT, boot_pat_state);
-	}
 
 	wrmsrl(MSR_IA32_CR_PAT, pat);
 	printk(KERN_INFO "x86 PAT enabled: cpu %d, old 0x%Lx, new 0x%Lx\n",
Index: linux-2.6/include/asm-x86/pat.h
===================================================================
--- linux-2.6.orig/include/asm-x86/pat.h
+++ linux-2.6/include/asm-x86/pat.h
@@ -4,7 +4,13 @@
 
 #include <linux/types.h>
 
+#ifdef CONFIG_X86_PAT
 extern int pat_wc_enabled;
+extern void validate_pat_support(struct cpuinfo_x86 *c);
+#else
+# define pat_wc_enabled		(0)
+static inline void validate_pat_support(struct cpuinfo_x86 *c) { }
+#endif
 
 extern void pat_init(void);
 
@@ -12,5 +18,7 @@ extern int reserve_memtype(u64 start, u6
 		unsigned long req_type, unsigned long *ret_type);
 extern int free_memtype(u64 start, u64 end);
 
+extern void pat_disable(char *reason);
+
 #endif
 

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Subject: x86: cleanup PAT cpu validation
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 08 May 2008 09:18:43 +0200
> 
> Move the scattered checks for PAT support to a single function. Its 
> moved to addon_cpuid_features.c as this file is shared between 32 and 
> 64 bit.

thanks Thomas, i have applied your cleanup patch - looks good here too 
and it booted fine on a few boxes.

(i did a small cleanup: changed the define to static const int)

Peter, what are your plans for a more general cleanup and 32-bit/64-bit 
unification for the x86 CPU detection code - do you have any specific 
ideas already? I suspect we want it all to end up unified under 
arch/x86/kernel/cpu/, by moving the relevant bits of 
arch/x86/kernel/setup_64.c there, right? It is no small task for sure.

	Ingo

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Adrian Bunk]

On Thu, May 08, 2008 at 02:49:19PM +0200, Thomas Gleixner wrote:
> On Wed, 7 May 2008, H. Peter Anvin wrote:
> > Linus Torvalds wrote:
> > > 
> > > > *Certainly* I don't want anything like this crap:
> > > > 
> > > > > -    if (cpu_has_pat)
> > > > > +    if (cpu_has_pat && cpu_has_pat_good)
> > > 
> > > This in fact is likely the best part of it.
> > > 
> > > Because that at least guarantees that we never say we have a good PAT when
> > > the hardware doesn't even report it.
> > > 
> > > As it is, we seem to just blindly override hardware. It may be correct for
> > > all the models we override, but still..
> > > 
> > 
> > Yah, this is not good.  We should mask out the bit, but never, ever, set it if
> > it was clear to begin with (unless we have it on really, *really*, good
> > authority.)
> > 
> > I'm embarrassed to have let that slink by.
> 
> /me too.
> 
> Fix below.
> 
> Thanks,
> 	tglx
>...
> The white list is kept for now to ensure that we can rely on known to
> work CPU types and concentrate on the software induced problems
> instead of fighthing CPU erratas and subtle wreckage caused by not yet
> verified CPUs. Once the PAT code has stabilized enough, we can remove
> the white list and open the can of worms.
>...
> +#ifdef CONFIG_X86_PAT
> +void __cpuinit validate_pat_support(struct cpuinfo_x86 *c)
> +{
> +	switch (c->x86_vendor) {
> +	case X86_VENDOR_AMD:
> +		if (c->x86 >= 0xf && c->x86 <= 0x11)
> +			return;
> +		break;
> +	case X86_VENDOR_INTEL:
> +		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
> +			return;
> +		break;
> +	}
>...

This still has the inconsistent handling of future CPUs between AMD
and Intel.

IMHO we should also assume for future AMD CPUs that PAT will work until 
proven otherwise.

Besides this, I didn't spot any problem in the patch. 

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Thomas Gleixner]

On Thu, 8 May 2008, Adrian Bunk wrote:
> 
> This still has the inconsistent handling of future CPUs between AMD
> and Intel.
> 
> IMHO we should also assume for future AMD CPUs that PAT will work until 
> proven otherwise.

Right, I did not change the logic of that. This code is an interim
protection anyway, so we should be able to remove it _before_ new AMDs
hit the wild.

Thanks,

	tglx

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Andi Kleen]

Rene Herman wrote:
> On 08-05-08 12:19, Andi Kleen wrote:
> 
>>> Use a new Linux defined X86_FEATURE_PAT_GOOD feature flag to
>>
>> Better would be PAT_TESTED or PAT_RANDOMLY_APPROVED. Most of these
>> CPUs without PAT_GOOD have actually perfectly good PAT, as Windows
>> proves every day.
>>
>> The main flaw in all of this of course is that there is no procedure
>> to test CPUs which do not have the flag set yet.
> 
> Quite. And hiding the fact that the CPU _should_ have perfectly good
> PAT doesn't help any at all. The discussion turned into a mini-flame
> war enough that now noone would even consider backing down, but this
> current PAT setup just sucks plain and simple.

For old CPUs it is actually ok (after all they worked for years without
PAT), I just don't like it for new CPUs. It's a bad idea there and
in the x86 world it is a reasonable expectation that CPU features
generally work.

That said I am not aware of that many PAT erratas even on old CPUs.
There are a few, but they are known and well understood and could
well be black listed.

-Andi

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Rene Herman]

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

On 08-05-08 14:49, Thomas Gleixner wrote:

> Subject: x86: cleanup PAT cpu validation
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 08 May 2008 09:18:43 +0200

Thanks much. Definite ACK, very much including on the much better disable
message:

> +	pat_disable(cpu_has_pat ?
> +		    "PAT disabled. Not yet verified on this CPU type." :
> +		    "PAT not supported by CPU.");
> +}

However, I'm not sure, but:

> +	/* Paranoia check. */
> +	if (!cpu_has_pat) {
> +		printk(KERN_ERR "PAT enabled, but CPU feature cleared\n");
> +		/*
> +		 * Panic if this happens on the secondary CPU, and we
> +		 * switched to PAT on the boot CPU. We have no way to
> +		 * undo PAT.
> +		*/
> +		BUG_ON(boot_pat_state);
> +	}

The 'if this happens on the secondary CPU' sounds a bit like this is
directly checking the secondary CPU flag but cpu_has_pat translates into
boot_cpu_has(X86_FEATURE_PAT), refers always to the boot cpu. Yes, we
continued on on the boot CPU after the error message so this triggers,
but thought I'd make sure it was also intended this way. If so, never
mind...

I'm privately again placing this on top. If anyone has any explicit
testing suggestion, I'm all ears.

Rene.

[-- Attachment #2: pat_duron7.diff --]
[-- Type: text/plain, Size: 530 bytes --]

diff --git a/arch/x86/kernel/cpu/addon_cpuid_features.c b/arch/x86/kernel/cpu/addon_cpuid_features.c
index c2e1ce3..cfa1b6b 100644
--- a/arch/x86/kernel/cpu/addon_cpuid_features.c
+++ b/arch/x86/kernel/cpu/addon_cpuid_features.c
@@ -57,6 +57,8 @@ void __cpuinit validate_pat_support(struct cpuinfo_x86 *c)
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0xf && c->x86 <= 0x11)
 			return;
+		if (c->x86 == 6 && c->x86_model == 7)
+			return;
 		break;
 	case X86_VENDOR_INTEL:
 		if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Thomas Gleixner]

On Thu, 8 May 2008, Rene Herman wrote:
> However, I'm not sure, but:
> 
> > +	/* Paranoia check. */
> > +	if (!cpu_has_pat) {
> > +		printk(KERN_ERR "PAT enabled, but CPU feature cleared\n");
> > +		/*
> > +		 * Panic if this happens on the secondary CPU, and we
> > +		 * switched to PAT on the boot CPU. We have no way to
> > +		 * undo PAT.
> > +		*/
> > +		BUG_ON(boot_pat_state);
> > +	}
> 
> The 'if this happens on the secondary CPU' sounds a bit like this is
> directly checking the secondary CPU flag but cpu_has_pat translates into
> boot_cpu_has(X86_FEATURE_PAT), refers always to the boot cpu.

Right and thats fine because of:

        /*
         * On SMP, boot_cpu_data holds the common feature set between
         * all CPUs; so make sure that we indicate which features are
         * common between the CPUs.  The first time this routine gets
         * executed, c == &boot_cpu_data.
         */
        if (c != &boot_cpu_data) {
                /* AND the already accumulated flags with these */
                for (i = 0 ; i < NCAPINTS ; i++)
                        boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
        }

Thanks,
	tglx

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Alan Cox]

> For old CPUs it is actually ok (after all they worked for years without
> PAT), I just don't like it for new CPUs. It's a bad idea there and
> in the x86 world it is a reasonable expectation that CPU features
> generally work.

Agreed 100%. We should default to assuming newer processors work. That
will be true in almost if not all cases anyway, and since it'll bite
anyone at Intel/AMD/.. testing new CPU steppings when it is on by default
any problem cases won't be leaving the labs.

Alan

Re: 2.6.26, PAT and AMD family 6

[H. Peter Anvin]

Andi Kleen wrote:
> "H. Peter Anvin" <hpa@zytor.com> writes:
>> Well, there *is* support for that - all the raw information is there
>> in /dev/cpu/*/cpuid.  There are other reasons why /proc/cpuinfo is the
>> wrong interface to try to get the "real" CPUID information - we only
>> report CPU features that the kernel knows about; bits we don't, if we
>> can decode them at all, we just don't show.
> 
> They are reported as numbers (assuming they are in the existing leaves)

No, they're not.

	-hpa

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[H. Peter Anvin]

Ingo Molnar wrote:
> * Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> Subject: x86: cleanup PAT cpu validation
>> From: Thomas Gleixner <tglx@linutronix.de>
>> Date: Thu, 08 May 2008 09:18:43 +0200
>>
>> Move the scattered checks for PAT support to a single function. Its 
>> moved to addon_cpuid_features.c as this file is shared between 32 and 
>> 64 bit.
> 
> thanks Thomas, i have applied your cleanup patch - looks good here too 
> and it booted fine on a few boxes.
> 
> (i did a small cleanup: changed the define to static const int)
> 
> Peter, what are your plans for a more general cleanup and 32-bit/64-bit 
> unification for the x86 CPU detection code - do you have any specific 
> ideas already? I suspect we want it all to end up unified under 
> arch/x86/kernel/cpu/, by moving the relevant bits of 
> arch/x86/kernel/setup_64.c there, right? It is no small task for sure.
> 

Yes, that's the plan, and no, it's not a small task.

	-hpa

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[H. Peter Anvin]

Thomas Gleixner wrote:
>> The 'if this happens on the secondary CPU' sounds a bit like this is
>> directly checking the secondary CPU flag but cpu_has_pat translates into
>> boot_cpu_has(X86_FEATURE_PAT), refers always to the boot cpu.

No, it doesn't.  Although it is badly named, it refers to the common 
capability set of all online CPUs.

> Right and thats fine because of:
> 
>         /*
>          * On SMP, boot_cpu_data holds the common feature set between
>          * all CPUs; so make sure that we indicate which features are
>          * common between the CPUs.  The first time this routine gets
>          * executed, c == &boot_cpu_data.
>          */
>         if (c != &boot_cpu_data) {
>                 /* AND the already accumulated flags with these */
>                 for (i = 0 ; i < NCAPINTS ; i++)
>                         boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
>         }

... because of this code. :)

	-hpa

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[H. Peter Anvin]

Alan Cox wrote:
>> For old CPUs it is actually ok (after all they worked for years without
>> PAT), I just don't like it for new CPUs. It's a bad idea there and
>> in the x86 world it is a reasonable expectation that CPU features
>> generally work.
> 
> Agreed 100%. We should default to assuming newer processors work. That
> will be true in almost if not all cases anyway, and since it'll bite
> anyone at Intel/AMD/.. testing new CPU steppings when it is on by default
> any problem cases won't be leaving the labs.

Yes, capping the upper end is an actively bad thing, because it can 
actually *make* bugs appear (by artifically limiting testing by CPU houses.)

	-hpa

Re: [PATCH] x86: introduce a new Linux defined feature flag for PAT support

[Rene Herman]

On 08-05-08 18:48, H. Peter Anvin wrote:

> No, it doesn't.  Although it is badly named, it refers to the common 
> capability set of all online CPUs.

Yup, all fine. Will report ofcourse when anything bad happens with PAT
support here.

Rene.