2008-08-21 00:13:13

by Pallipadi, Venkatesh

[permalink] [raw]
Subject: [patch 3/4] x86: PAT Update validate_pat_support for intel CPUs

Pentium III and Core Solo/Duo CPUs have an erratum
" Page with PAT set to WC while associated MTRR is UC may consolidate to UC "
which can result in WC setting in PAT to be ineffective. We will disable
PAT on such CPUs, so that we can continue to use MTRR WC setting.

Signed-off-by: Venkatesh Pallipadi <[email protected]>

---
arch/x86/kernel/cpu/addon_cpuid_features.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

Index: tip/arch/x86/kernel/cpu/addon_cpuid_features.c
===================================================================
--- tip.orig/arch/x86/kernel/cpu/addon_cpuid_features.c 2008-08-20 14:25:18.000000000 -0700
+++ tip/arch/x86/kernel/cpu/addon_cpuid_features.c 2008-08-20 14:26:39.000000000 -0700
@@ -56,9 +56,22 @@ void __cpuinit validate_pat_support(stru

switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
- if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
+ /*
+ * There is a known erratum on Pentium III and Core Solo
+ * and Core Duo CPUs.
+ * " Page with PAT set to WC while associated MTRR is UC
+ * may consolidate to UC "
+ * Because of this erratum, it is better to stick with
+ * setting WC in MTRR rather than using PAT on these CPUs.
+ *
+ * Enable PAT WC only on P4, Core 2 or later CPUs.
+ */
+ if (c->x86 > 0x6 || (c->x86 == 6 && c->x86_model >= 15))
return;
- break;
+
+ pat_disable("PAT WC disabled due to known CPU erratum.");
+ return;
+
case X86_VENDOR_AMD:
case X86_VENDOR_CENTAUR:
case X86_VENDOR_TRANSMETA:

--


2008-08-26 07:06:00

by Dave Airlie

[permalink] [raw]
Subject: Re: [patch 3/4] x86: PAT Update validate_pat_support for intel CPUs

On Thu, Aug 21, 2008 at 9:45 AM, <[email protected]> wrote:
> Pentium III and Core Solo/Duo CPUs have an erratum
> " Page with PAT set to WC while associated MTRR is UC may consolidate to UC "
> which can result in WC setting in PAT to be ineffective. We will disable
> PAT on such CPUs, so that we can continue to use MTRR WC setting.
>

IMHO this seems like a bit hammer with which to squash this nut.

I really need PAT support for upcoming GPU stuff, esp where I have
pages of RAM allocated into a
GART and I want write-combined access to them. These pages will
physically me under a write-back MTRR, with a WC PAT entry
on the PTE mappings.

Can we not just be smarter and fix this when we know we have a UC MTRR
and a WC PAT mapping inside it?

Dave.

> Signed-off-by: Venkatesh Pallipadi <[email protected]>
>
> ---
> arch/x86/kernel/cpu/addon_cpuid_features.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> Index: tip/arch/x86/kernel/cpu/addon_cpuid_features.c
> ===================================================================
> --- tip.orig/arch/x86/kernel/cpu/addon_cpuid_features.c 2008-08-20 14:25:18.000000000 -0700
> +++ tip/arch/x86/kernel/cpu/addon_cpuid_features.c 2008-08-20 14:26:39.000000000 -0700
> @@ -56,9 +56,22 @@ void __cpuinit validate_pat_support(stru
>
> switch (c->x86_vendor) {
> case X86_VENDOR_INTEL:
> - if (c->x86 == 0xF || (c->x86 == 6 && c->x86_model >= 15))
> + /*
> + * There is a known erratum on Pentium III and Core Solo
> + * and Core Duo CPUs.
> + * " Page with PAT set to WC while associated MTRR is UC
> + * may consolidate to UC "
> + * Because of this erratum, it is better to stick with
> + * setting WC in MTRR rather than using PAT on these CPUs.
> + *
> + * Enable PAT WC only on P4, Core 2 or later CPUs.
> + */
> + if (c->x86 > 0x6 || (c->x86 == 6 && c->x86_model >= 15))
> return;
> - break;
> +
> + pat_disable("PAT WC disabled due to known CPU erratum.");
> + return;
> +
> case X86_VENDOR_AMD:
> case X86_VENDOR_CENTAUR:
> case X86_VENDOR_TRANSMETA:
>
> --
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2008-08-26 23:04:23

by Pallipadi, Venkatesh

[permalink] [raw]
Subject: RE: [patch 3/4] x86: PAT Update validate_pat_support for intel CPUs



The problem here is both with MTRR entry of UC and even with
default MTRR being UC.

Default MTRR being UC for all reserved regions and drivers
wanting WC is very common situation and performance will be
affected when that ends up being UC access.
With pat disabled, drivers can set MTRR with WC in this case
and get the expected performance.

Yes. We can still allow WC mappings only for the RAM
regions and disable PAT WC for all the reserved regions. I will
take a look at that option and see how cleanly we can
enable pat only for set_memory_wc and disable PAT for
ioremap_wc and pci regions.

Thanks,
Venki



>-----Original Message-----
>From: Dave Airlie [mailto:[email protected]]
>Sent: Tuesday, August 26, 2008 12:06 AM
>To: Pallipadi, Venkatesh
>Cc: [email protected]; [email protected]; [email protected];
>[email protected]; Siddha, Suresh B
>Subject: Re: [patch 3/4] x86: PAT Update validate_pat_support
>for intel CPUs
>
>On Thu, Aug 21, 2008 at 9:45 AM,
><[email protected]> wrote:
>> Pentium III and Core Solo/Duo CPUs have an erratum
>> " Page with PAT set to WC while associated MTRR is UC may
>consolidate to UC "
>> which can result in WC setting in PAT to be ineffective. We
>will disable
>> PAT on such CPUs, so that we can continue to use MTRR WC setting.
>>
>
>IMHO this seems like a bit hammer with which to squash this nut.
>
>I really need PAT support for upcoming GPU stuff, esp where I have
>pages of RAM allocated into a
>GART and I want write-combined access to them. These pages will
>physically me under a write-back MTRR, with a WC PAT entry
>on the PTE mappings.
>
>Can we not just be smarter and fix this when we know we have a UC MTRR
>and a WC PAT mapping inside it?
>
>Dave.
>
>> Signed-off-by: Venkatesh Pallipadi <[email protected]>
>>
>> ---
>> arch/x86/kernel/cpu/addon_cpuid_features.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> Index: tip/arch/x86/kernel/cpu/addon_cpuid_features.c
>> ===================================================================
>> --- tip.orig/arch/x86/kernel/cpu/addon_cpuid_features.c
>2008-08-20 14:25:18.000000000 -0700
>> +++ tip/arch/x86/kernel/cpu/addon_cpuid_features.c
>2008-08-20 14:26:39.000000000 -0700
>> @@ -56,9 +56,22 @@ void __cpuinit validate_pat_support(stru
>>
>> switch (c->x86_vendor) {
>> case X86_VENDOR_INTEL:
>> - if (c->x86 == 0xF || (c->x86 == 6 &&
>c->x86_model >= 15))
>> + /*
>> + * There is a known erratum on Pentium III
>and Core Solo
>> + * and Core Duo CPUs.
>> + * " Page with PAT set to WC while
>associated MTRR is UC
>> + * may consolidate to UC "
>> + * Because of this erratum, it is better to
>stick with
>> + * setting WC in MTRR rather than using PAT
>on these CPUs.
>> + *
>> + * Enable PAT WC only on P4, Core 2 or later CPUs.
>> + */
>> + if (c->x86 > 0x6 || (c->x86 == 6 &&
>c->x86_model >= 15))
>> return;
>> - break;
>> +
>> + pat_disable("PAT WC disabled due to known
>CPU erratum.");
>> + return;
>> +
>> case X86_VENDOR_AMD:
>> case X86_VENDOR_CENTAUR:
>> case X86_VENDOR_TRANSMETA:
>>
>> --
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>>
>

2008-08-27 05:31:39

by Brice Goglin

[permalink] [raw]
Subject: Re: [patch 3/4] x86: PAT Update validate_pat_support for intel CPUs

Pallipadi, Venkatesh wrote:
> Default MTRR being UC for all reserved regions and drivers
> wanting WC is very common situation and performance will be
> affected when that ends up being UC access.
> With pat disabled, drivers can set MTRR with WC in this case
> and get the expected performance.
>

Are you saying that drivers are supposed to check if PAT is disabled
before deciding whether they need to setup a MTRR WC? If so, how to do
we check that? Unless we get a way to know whether ioremap_wc() returned
WC or UC, I will always keep the MTRR WC in myri10ge.

Brice

2008-08-29 22:59:53

by Pallipadi, Venkatesh

[permalink] [raw]
Subject: RE: [patch 3/4] x86: PAT Update validate_pat_support for intel CPUs



>-----Original Message-----
>From: Brice Goglin [mailto:[email protected]]
>Sent: Tuesday, August 26, 2008 10:30 PM
>To: Pallipadi, Venkatesh
>Cc: Dave Airlie; [email protected]; [email protected];
>[email protected]; [email protected]; Siddha, Suresh B
>Subject: Re: [patch 3/4] x86: PAT Update validate_pat_support
>for intel CPUs
>
>Pallipadi, Venkatesh wrote:
>> Default MTRR being UC for all reserved regions and drivers
>> wanting WC is very common situation and performance will be
>> affected when that ends up being UC access.
>> With pat disabled, drivers can set MTRR with WC in this case
>> and get the expected performance.
>>
>
>Are you saying that drivers are supposed to check if PAT is disabled
>before deciding whether they need to setup a MTRR WC? If so, how to do
>we check that? Unless we get a way to know whether
>ioremap_wc() returned
>WC or UC, I will always keep the MTRR WC in myri10ge.
>

Agreed. There is no way for driver to cleanly know whether it got
WC mapping or not. I guess it is possibly simpler for kernel to
ignore MTRR writes when WC is working, rather than each driver
checking the return value. But, that will come later,
once we have PAT code stable.

Thanks,
Venki