X86: fix typo PAT to X86_PAT

PATCH In review Yinghai Lu 4 participants 13 messages Jan 18, 2008 – Jan 22, 2008

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Yinghai Lu

author

2008-01-18 03:44 UTC #0 lore raw

PATCH 1/1 X86: fix typo PAT to X86_PAT

[PATCH] X86: fix typo PAT to X86_PAT

Signed-off-by Yinghai Lu <[email protected]>

arch/x86/Kconfig +1 -1

index 1b38c21..980f054 100644
@@ -953,7 +953,7 @@ config MATH_EMULATION
 
 config MTRR
 	bool "MTRR (Memory Type Range Register) support"
-	depends on !PAT
+	depends on !X86_PAT
 	---help---
 	  On Intel P6 family processors (Pentium Pro, Pentium II and later)
 	  the Memory Type Range Registers (MTRRs) may be used to control

Ingo Molnar

2008-01-18 08:10 UTC #1 lore raw

arch/x86/Kconfig config MATH_EMULATION · line 959

Quoted 7 lines · Yinghai Lu

config MTRR

arch/x86/Kconfig +1 -1

 	bool "MTRR (Memory Type Range Register) support"
-	depends on !PAT
+	depends on !X86_PAT
 	---help---
 	  On Intel P6 family processors (Pentium Pro, Pentium II and later)
 	  the Memory Type Range Registers (MTRRs) may be used to control

thanks. But, i think we should rather do the following: if X86_PAT is eanbled then /proc/mtrr should be read-only. There's no problem _looking_ at MTRR contents, as long as we do not try to modify them. Hm?

Ingo

Yinghai Lu

author

2008-01-18 09:00 UTC #2 lore raw

Quoted 14 lines · Ingo Molnar

arch/x86/Kconfig config MATH_EMULATION · line 959
Quoted 7 lines · Yinghai Lu
config MTRR
arch/x86/Kconfig +1 -1
 	bool "MTRR (Memory Type Range Register) support"
-	depends on !PAT
+	depends on !X86_PAT
 	---help---
 	  On Intel P6 family processors (Pentium Pro, Pentium II and later)
 	  the Memory Type Range Registers (MTRRs) may be used to control
thanks. But, i think we should rather do the following: if X86_PAT is eanbled then /proc/mtrr should be read-only. There's no problem _looking_ at MTRR contents, as long as we do not try to modify them. Hm?

anyway

depends on !PAT

need to be removed.

it seems when PAT is used, some code still touch MTRR.

Ingo Molnar

2008-01-18 12:32 UTC #3 lore raw

Quoted 5 lines · Yinghai Lu

Quoted 4 lines
thanks. But, i think we should rather do the following: if X86_PAT is eanbled then /proc/mtrr should be read-only. There's no problem _looking_ at MTRR contents, as long as we do not try to modify them. Hm?

Quoted 7 lines

anyway
depends on !PAT
need to be removed.
it seems when PAT is used, some code still touch MTRR.

you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)

Ingo

Dave Jones

2008-01-18 18:25 UTC #4 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 7 lines · Yinghai Lu
Quoted 4 lines
thanks. But, i think we should rather do the following: if X86_PAT is eanbled then /proc/mtrr should be read-only. There's no problem _looking_ at MTRR contents, as long as we do not try to modify them. Hm?
anyway

Quoted 8 lines

Quoted 5 lines
depends on !PAT
need to be removed.
it seems when PAT is used, some code still touch MTRR.
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)

This exclusion is going to be a real pain in the ass for distro kernels. It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.

Additionally, given people tend to update their kernels a lot more often than they update to a whole new version of X, it means until userspace has caught up, we can't ship a kernel with PAT supported, or else X gets a lot slower due to the missing mtrr support.

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Pallipadi, Venkatesh

2008-01-18 18:46 UTC #5 lore raw

Quoted 29 lines

-----Original Message-----
From: Dave Jones [mailto:[email protected]]
Sent: Friday, January 18, 2008 10:25 AM
To: Ingo Molnar
Cc: Yinghai Lu; Pallipadi, Venkatesh; LKML
Subject: Re: [PATCH] X86: fix typo PAT to X86_PAT
Quoted 3 lines · Ingo Molnar
Quoted 1 line · Yinghai Lu
Quoted 1 line
thanks. But, i think we should rather do the following:
if X86_PAT
Quoted 1 line
Quoted 1 line
Quoted 1 line
is eanbled then /proc/mtrr should be read-only. There's
no problem
Quoted 1 line
Quoted 1 line
Quoted 1 line
_looking_ at MTRR contents, as long as we do not try to
modify them.
Quoted 10 lines
Quoted 9 lines
Quoted 1 line
Hm?
anyway
depends on !PAT
need to be removed.
it seems when PAT is used, some code still touch MTRR.

Quoted 9 lines

Quoted 2 lines
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)
This exclusion is going to be a real pain in the ass for distro kernels.
It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.

Actually, this exclusion will not work at all with the current code. Infact it should be PAT selects MTRR, for the current code. As pat_init() is called during mtrr init as the rules for how to change PAT and how to change MTRR are same. Further, MTRR is always required on SMP, as we read the MTRR setting from boot CPU and set it on Aps at boot time. We should only remove the /proc/mtrr write permissions with CONFIG_PAT. We need to deprecate it for a while before that... Ingo, can you remove this PAT MTRR exclusion.

Thanks,
Venki

Dave Jones

2008-01-18 18:57 UTC #6 lore raw

Quoted 6 lines · Venki Pallipadi

Quoted 5 lines
This exclusion is going to be a real pain in the ass for distro kernels.
It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.

Quoted 8 lines

Actually, this exclusion will not work at all with the current code. Infact it should be PAT selects MTRR, for the current code. As pat_init() is called during mtrr init as the rules for how to change PAT and how to change MTRR are same. Further, MTRR is always required on SMP, as we read the MTRR setting from boot CPU and set it on Aps at boot time. We should only remove the /proc/mtrr write permissions with CONFIG_PAT. We need to deprecate it for a while before that... Ingo, can you remove this PAT MTRR exclusion.

The removal of write-permission also needs to be decided at runtime rather than compile time, or we screw over the "doesn't support PAT" CPUs in distro kernels.

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Ingo Molnar

2008-01-18 21:04 UTC #7 lore raw

In reply to Pallipadi, Venkatesh

Quoted 1 line · Pallipadi, Venkatesh

Ingo, can you remove this PAT MTRR exclusion.

yeah, already did that.

Quoted 7 lines

Actually, this exclusion will not work at all with the current code. Infact it should be PAT selects MTRR, for the current code. As pat_init() is called during mtrr init as the rules for how to change PAT and how to change MTRR are same. Further, MTRR is always required on SMP, as we read the MTRR setting from boot CPU and set it on Aps at boot time. We should only remove the /proc/mtrr write permissions with CONFIG_PAT. We need to deprecate it for a while before that...

ok.

Ingo

Ingo Molnar

2008-01-18 21:02 UTC #8 lore raw

In reply to Dave Jones

Quoted 8 lines · Dave Jones

Quoted 2 lines
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)
This exclusion is going to be a real pain in the ass for distro kernels. It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.

Quoted 4 lines

Additionally, given people tend to update their kernels a lot more often than they update to a whole new version of X, it means until userspace has caught up, we can't ship a kernel with PAT supported, or else X gets a lot slower due to the missing mtrr support.

there's no exclusion enforced right now, and if a CPU is PAT-incapable (or if the kernel is booted nopat) then the MTRR bits should be usable. But if we boot with PAT enabled, and Xorg gets /proc/mtrr wrong, we'll see nasty crashes. If it gets them right, it should all still work just fine. Is this ok? Then, in a year or two, distros can disable write support to /proc/mtrr. Hm?

Ingo

Dave Jones

2008-01-19 03:29 UTC #9 lore raw

Quoted 16 lines · Ingo Molnar

Quoted 8 lines · Dave Jones
Quoted 2 lines
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)
This exclusion is going to be a real pain in the ass for distro kernels. It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.
Quoted 4 lines
Additionally, given people tend to update their kernels a lot more often than they update to a whole new version of X, it means until userspace has caught up, we can't ship a kernel with PAT supported, or else X gets a lot slower due to the missing mtrr support.

Quoted 6 lines

there's no exclusion enforced right now, and if a CPU is PAT-incapable (or if the kernel is booted nopat) then the MTRR bits should be usable. But if we boot with PAT enabled, and Xorg gets /proc/mtrr wrong, we'll see nasty crashes. If it gets them right, it should all still work just fine. Is this ok? Then, in a year or two, distros can disable write support to /proc/mtrr. Hm?

A crazy idea just occured to me.. We could make /proc/mtrr an interface to set PAT on a range of memory. This would make it transparently work without any changes in X or anything else that sets them in userspace.

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Yinghai Lu

author

2008-01-19 07:50 UTC #10 lore raw

PATCH 1/1 X86: disable X86_PAT really

[PATCH] X86: disable X86_PAT really

when X86_PAT is not selected, we don't need to do anything in reserve_mattr and free_mattr

also need to bail out if cpu not support PAT.

Signed-off-by Yinghai Lu <[email protected]>

arch/x86/mm/pat.c +16 -4

index 1036134..b3cdee1 100644
@@ -57,12 +57,9 @@ static int pat_known_cpu(void)
 
 void pat_init(void)
 {
+#ifdef CONFIG_X86_PAT
 	u64 pat;
 
-#ifndef CONFIG_X86_PAT
-	nopat(NULL);
-#endif
-
 	if (!smp_processor_id() && !pat_known_cpu())
 		return;

@@ -90,6 +87,7 @@ void pat_init(void)
 	wrmsrl(MSR_IA32_CR_PAT, pat);
 	printk(KERN_INFO "x86 PAT enabled: cpu %d, old 0x%Lx, new 0x%Lx\n",
 		smp_processor_id(), boot_pat_state, pat);
+#endif
 }
 
 #undef PAT

@@ -135,9 +133,13 @@ static DEFINE_SPINLOCK(mattr_lock); 	/* protects memattr list */
 
 int reserve_mattr(u64 start, u64 end, unsigned long attr, unsigned long *fattr)
 {
+#ifdef CONFIG_X86_PAT
 	struct memattr *ma = NULL, *ml;
 	int err = 0;
 
+	if (!pat_wc_enabled)
+		return 0;
+
 	if (fattr)
 		*fattr = attr;

@@ -191,13 +193,20 @@ int reserve_mattr(u64 start, u64 end, unsigned long attr, unsigned long *fattr)
 
 	spin_unlock(&mattr_lock);
 	return err;
+#else
+	return 0;
+#endif
 }
 
 int free_mattr(u64 start, u64 end, unsigned long attr)
 {
+#ifdef CONFIG_X86_PAT
 	struct memattr *ml;
 	int err = attr ? -EBUSY : 0;
 
+	if (!pat_wc_enabled)
+		return 0;
+
 	if (is_memory_any_valid(start, end))
 		return 0;

@@ -221,6 +230,9 @@ int free_mattr(u64 start, u64 end, unsigned long attr)
 			current->comm, current->pid,
 			start, end, cattr_name(attr));
 	return err;
+#else
+	return 0;
+#endif
 }
 
 /* /dev/mem interface. Use the previous mapping */

Yinghai Lu

author

2008-01-19 07:52 UTC #11 lore raw

In reply to Dave Jones

Quoted 24 lines · Dave Jones

Quoted 16 lines · Ingo Molnar
Quoted 8 lines · Dave Jones
Quoted 2 lines
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)
This exclusion is going to be a real pain in the ass for distro kernels. It's impossible for example to build a kernel that will now support the MTRR-alike registers on the AMD K6/early Cyrix etc and also support PAT.
Quoted 4 lines
Additionally, given people tend to update their kernels a lot more often than they update to a whole new version of X, it means until userspace has caught up, we can't ship a kernel with PAT supported, or else X gets a lot slower due to the missing mtrr support.
Quoted 6 lines
there's no exclusion enforced right now, and if a CPU is PAT-incapable (or if the kernel is booted nopat) then the MTRR bits should be usable. But if we boot with PAT enabled, and Xorg gets /proc/mtrr wrong, we'll see nasty crashes. If it gets them right, it should all still work just fine. Is this ok? Then, in a year or two, distros can disable write support to /proc/mtrr. Hm?

Quoted 3 lines

A crazy idea just occured to me.. We could make /proc/mtrr an interface to set PAT on a range of memory. This would make it transparently work without any changes in X or anything else that sets them in userspace.

goog idea...

we need to make X86_PAT depend on MTRR in arch/x86/Kconfig

Pallipadi, Venkatesh

2008-01-22 18:25 UTC #12 lore raw

In reply to Dave Jones

Quoted 38 lines

-----Original Message-----
From: Dave Jones [mailto:[email protected]]
Sent: Friday, January 18, 2008 7:29 PM
To: Ingo Molnar
Cc: Yinghai Lu; Pallipadi, Venkatesh; LKML
Subject: Re: [PATCH] X86: fix typo PAT to X86_PAT
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Dave Jones
Quoted 2 lines
you mean modifies MTRRs? Which code is that? (besides the /proc/mtrr userspace API)
This exclusion is going to be a real pain in the ass for distro kernels. It's impossible for example to build a kernel
that will now
Quoted 1 line
Quoted 1 line
support the MTRR-alike registers on the AMD K6/early
Cyrix etc and
Quoted 3 lines
Quoted 3 lines
also support PAT.
Additionally, given people tend to update their kernels a
lot more
Quoted 1 line
Quoted 1 line
often than they update to a whole new version of X, it
means until
Quoted 1 line
Quoted 1 line
userspace has caught up, we can't ship a kernel with PAT
supported, or
Quoted 3 lines
Quoted 1 line
else X gets a lot slower due to the missing mtrr support.
there's no exclusion enforced right now, and if a CPU is
PAT-incapable
Quoted 1 line
(or if the kernel is booted nopat) then the MTRR bits
should be usable.
Quoted 1 line
But if we boot with PAT enabled, and Xorg gets /proc/mtrr
wrong, we'll
Quoted 1 line
see nasty crashes. If it gets them right, it should all

Quoted 9 lines

still work just
Quoted 2 lines
fine. Is this ok? Then, in a year or two, distros can disable write support to /proc/mtrr. Hm?
A crazy idea just occured to me.. We could make /proc/mtrr an interface
to set PAT on a range of memory. This would make it transparently work without any changes in X or anything else that sets them in userspace.

Yes. We actually used this earlier while we were testing PAT functionality internally :).

There are some issues though.
1) Current X does /dev/mem mapping of the region followed by MTRR setting for this region. For this to work with PAT based MTRR, either the order has to change (so that there wont be any conflict due to WB devmem mapping when we try to simulate mtrr) or we need a mechanism to go and change devmem mapping to reflect the later PAT attribute changes.
2) We will have to fail mtrr setting when there are hard conflicts with PAT requests.

We will look at this as a possible optimization for next round of PAT patches. But, to work with existing X, we will have to have mechanism to go and change existing mappings which is slightly more complicated than what we already have with current PAT changes.

Thanks,
Venki