2.6.26, PAT and AMD family 6

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.

Quoted 31 lines · 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

Quoted 5 lines

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

arch/x86/kernel/cpu/common.c +0 -0

index a428ffc..81483ec 100644

Quoted 7 lines · Rene Herman

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

Quoted 6 lines

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:

Quoted 8 lines

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.

arch/x86/kernel/cpu/common.c +0 -0

index a428ffc..81483ec 100644

Quoted 2 lines

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.

Quoted 10 lines · Yinghai Lu

Quoted 7 lines · Rene Herman

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

[ ... ]

Quoted 33 lines

Quoted 9 lines

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

Quoted 7 lines

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.

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/common.c index a428ffc..81483ec 100644

arch/x86/kernel/cpu/common.c +0 -0

@@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct

cpuinfo_x86 *c) case X86_VENDOR_AMD:

arch/x86/kernel/cpu/common.c +2 -0

 		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

Quoted 1 line

Quoted 1 line

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

Quoted 54 lines · On Wed, 07 May 2008 15:00:18 +0200 Rene Herman <[email protected]> wrote:

Quoted 9 lines · Yinghai Lu

Quoted 7 lines · Rene Herman

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

[ ... ]

Quoted 30 lines

Quoted 8 lines

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

Quoted 7 lines

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.

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/common.c index a428ffc..81483ec 100644

arch/x86/kernel/cpu/common.c +0 -0

@@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct

cpuinfo_x86 *c) case X86_VENDOR_AMD:

arch/x86/kernel/cpu/common.c +2 -0

 		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

Quoted 1 line

Quoted 1 line

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

Quoted 5 lines

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.

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.

On Wed, 07 May 2008 16:09:05 +0200
Rene Herman <[email protected]> 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...

Quoted 5 lines

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

Quoted 1 line · Yinghai Lu

On Tue, May 6, 2008 at 6:48 PM, Rene Herman <[email protected]>

Quoted 7 lines

Quoted 7 lines

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

[ ... ]

Quoted 25 lines

Quoted 9 lines

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

Quoted 6 lines

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.

arch/x86/kernel/cpu/common.c +0 -0

index a428ffc..81483ec 100644
@@ -314,6 +314,8 @@ static void __cpuinit early_get_cap(struct

cpuinfo_x86 *c) case X86_VENDOR_AMD:

arch/x86/kernel/cpu/common.c

Quoted 7 lines

arch/x86/kernel/cpu/common.c +2 -0

 		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.

                     ^^^^^---- 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 ?)

[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

Quoted 1 line · Daniel Hazelton

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.

Quoted 1 line

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?

Quoted 2 lines

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

Yinghai's.

Quoted 3 lines

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

Quoted 27 lines · Rene Herman

Quoted 1 line · Daniel Hazelton

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.

Quoted 1 line

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?

Quoted 2 lines

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

Yinghai's.

Quoted 3 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 2 lines

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

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 1 line

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.

On Wed, 7 May 2008, Adrian Bunk wrote:
...

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

...

Quoted 6 lines

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.

...

Quoted 2 lines

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

Quoted 8 lines · Thomas Gleixner

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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?

Quoted 1 line · Rene Herman

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

...

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

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.

...
Thanks,

tglx

Quoted 4 lines · Thomas Gleixner

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

...

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 1 line · Adrian Bunk

On Wed, May 07, 2008 at 10:52:36PM +0200, Thomas Gleixner wrote:

...

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

That information is for sure available.

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

Pointers please.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 6 lines

...

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 6 lines

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 4 lines

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.

3) copy&paste code remained in the patch

Quoted 6 lines · Thomas Gleixner

Quoted 4 lines

Quoted 2 lines

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.

2) HPA said it was bad idea

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

Quoted 2 lines

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 ?

Quoted 2 lines

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

the title is good enough

Quoted 4 lines · Rene Herman

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.

Quoted 1 line · Pavel Machek

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

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

Quoted 1 line

2) HPA said it was bad idea

hpa said white list is not a good idea.

Quoted 5 lines · Yinghai Lu

Quoted 1 line · Pavel Machek

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:

Quoted 5 lines

        switch (c->x86_vendor) {
        case X86_VENDOR_AMD:
                early_init_amd(c);

                break;

Quoted 8 lines

        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.

Quoted 3 lines · Yinghai Lu

Quoted 1 line

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.

Quoted 7 lines · Thomas Gleixner

Quoted 1 line

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.

Quoted 2 lines

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

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

Quoted 1 line

2) HPA said it was bad idea

hpa said white list is not a good idea.

Quoted 40 lines · Pavel Machek

Quoted 5 lines · Yinghai Lu

Quoted 1 line · Pavel Machek

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:

Quoted 5 lines

        switch (c->x86_vendor) {
        case X86_VENDOR_AMD:
                early_init_amd(c);

                break;

Quoted 8 lines

        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;
# }

Quoted 2 lines

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.

Quoted 5 lines · Yinghai Lu

Quoted 1 line · Pavel Machek

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:

Quoted 11 lines

        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.

Quoted 20 lines · Pavel Machek

Quoted 3 lines · Yinghai Lu

Quoted 1 line

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;
+ }

Quoted 1 line

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

Quoted 34 lines · Pavel Machek

Quoted 5 lines · Yinghai Lu

Quoted 1 line · Pavel Machek

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:

Quoted 11 lines

        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;
# }

Quoted 2 lines

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?

Quoted 6 lines · Thomas Gleixner

Quoted 2 lines · Pavel Machek

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 8 lines · Rene Herman

Quoted 2 lines

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.

Quoted 6 lines

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.

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

Quoted 12 lines · H. Peter Anvin

Quoted 10 lines · Rene Herman

Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 2 lines · Rene Herman

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?

Quoted 2 lines · Rene Herman

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.

Quoted 7 lines · H. Peter Anvin

Quoted 5 lines

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?

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.

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

Quoted 2 lines · Rene Herman

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.

Quoted 3 lines

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

Quoted 2 lines · Rene Herman

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:

Quoted 8 lines

arch/x86/mm/pat.c +0 -0

index 277446c..6ee3efb 100644
@@ -43,7 +43,7 @@ static int pat_known_cpu(void)
    if (!pat_wc_enabled)
        return 0;
 

arch/x86/mm/pat.c static int pat_known_cpu(void) · line 47

Quoted 3 lines

arch/x86/mm/pat.c +1 -1

-    if (cpu_has_pat)
+    if (cpu_has_pat && cpu_has_pat_good)
        return 1;

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:

Quoted 2 lines

(quoted diff) +1 -1

-    if (cpu_has_pat)
+    if (cpu_has_pat && cpu_has_pat_good)

Quoted 5 lines

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

Quoted 2 lines

(quoted diff) +1 -1

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

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

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.

Use a new Linux defined X86_FEATURE_PAT_GOOD feature flag to

Quoted 1 line

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.

Quoted 6 lines · Linus Torvalds

Quoted 5 lines

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

Quoted 2 lines

(quoted diff) +1 -1

-    if (cpu_has_pat)
+    if (cpu_has_pat && cpu_has_pat_good)

Quoted 8 lines

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.

Subject: x86: cleanup PAT cpu validation
From: Thomas Gleixner <[email protected]>
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.

Quoted 16 lines · H. Peter Anvin

Quoted 6 lines · Linus Torvalds

Quoted 5 lines

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

Quoted 2 lines

(quoted diff) +1 -1

-    if (cpu_has_pat)
+    if (cpu_has_pat && cpu_has_pat_good)

Quoted 8 lines

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

Quoted 5 lines

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

arch/x86/kernel/cpu/addon_cpuid_features.c +4 -0

+#ifdef CONFIG_X86_PAT
+void __cpuinit validate_pat_support(struct cpuinfo_x86 *c)
+{
+	switch (c->x86_vendor) {

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.

Quoted 6 lines · Andi Kleen

Quoted 1 line

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.

Quoted 2 lines

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.

Subject: x86: cleanup PAT cpu validation
From: Thomas Gleixner <[email protected]>
Date: Thu, 08 May 2008 09:18:43 +0200

However, I'm not sure, but:

Review

Quoted 10 lines

(quoted diff) +10 -0

+	/* 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.

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.

Quoted 4 lines · Thomas Gleixner

Subject: x86: cleanup PAT cpu validation
From: Thomas Gleixner <[email protected]>
Date: Thu, 08 May 2008 09:18:43 +0200

Quoted 1 line

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.

Quoted 3 lines

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];
        }

Quoted 4 lines

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.

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

Quoted 5 lines · H. Peter Anvin

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)