Skip to contentSkip to conversation

x86: mtrr: Constrain WB MTRR to max phys mem prior to cleanup

RFC In review Peter Hurley 4 participants 12 messages Sep 7, 2012 – Sep 29, 2012 x86

lore
Share & tools
lore

Peter Hurley

author
#0 lore raw

RFC 1/1 x86: mtrr: Constrain WB MTRR to max phys mem prior to cleanup

If BIOS maps all or most of the phys address space as write-back, regardless of the amount of actual RAM, then mtrr_cleanup may create superfluous MTRR entries, or may be unable to create an optimal map (or may use all MTRRs attempting to do so, thus making cleanup pointless). Ideally, WB MTRRs should not be used for physical memory addresses where memory does not exist (Intel SDM, Vol 3A, 11.11.2.1).

For example, Dell Precision T5400 wkstn w/ rev. A04 BIOS constructs the following MTRR map for 10GB memory:

 kernel: [    0.000000] MTRR variable ranges enabled:
 kernel: [    0.000000]   0 base 0000000000 mask 2000000000 write-back
 kernel: [    0.000000]   1 base 00CFF00000 mask 3FFFF00000 uncachable
 kernel: [    0.000000]   2 base 00D0000000 mask 3FF0000000 uncachable
 kernel: [    0.000000]   3 base 00E0000000 mask 3FE0000000 uncachable
 kernel: [    0.000000]   4 base 1FF0000000 mask 3FF0000000 uncachable
 kernel: [    0.000000]   5 disabled
 kernel: [    0.000000]   6 disabled
 kernel: [    0.000000]   7 disabled
 kernel: [    0.000000] x86 PAT enabled: cpu 0 ......
 kernel: [    0.000000] original variable MTRRs
 kernel: [    0.000000] reg 0, base: 0GB, range: 128GB, type WB
 kernel: [    0.000000] reg 1, base: 3327MB, range: 1MB, type UC
 kernel: [    0.000000] reg 2, base: 3328MB, range: 256MB, type UC
 kernel: [    0.000000] reg 3, base: 3584MB, range: 512MB, type UC
 kernel: [    0.000000] reg 4, base: 130816MB, range: 256MB, type UC
 kernel: [    0.000000] total RAM covered: 130047M

mtrr_cleanup cannot construct an optimal map for this configuration. Additionally, if the mtrr_gran_size/mtrr_chunk_size is specified, mtrr_cleanup will use all entries attempting to map the full 128GB phys address range.

If a WB MTRR entry which maps phys addr range [0, > max_pfn) exists, constrain the entry to [0, roundup_pow_of_two(max_pfn) ) prior to generating potential solutions. The example MTRR map is optimized to:

 kernel: [    0.000000] New variable MTRRs
 kernel: [    0.000000] reg 0, base: 0GB, range: 2GB, type WB
 kernel: [    0.000000] reg 1, base: 2GB, range: 1GB, type WB
 kernel: [    0.000000] reg 2, base: 3GB, range: 256MB, type WB
 kernel: [    0.000000] reg 3, base: 3327MB, range: 1MB, type UC
 kernel: [    0.000000] reg 4, base: 4GB, range: 4GB, type WB
 kernel: [    0.000000] reg 5, base: 8GB, range: 8GB, type WB
Signed-off-by Peter Hurley <[email protected]>
---
 arch/x86/kernel/cpu/mtrr/cleanup.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
arch/x86/kernel/cpu/mtrr/cleanup.c +10 -0
index bdda2e6..ee399c3 100644
@@ -714,6 +714,16 @@ int __init mtrr_cleanup(unsigned address_bits)
 	if (mtrr_tom2)
 		x_remove_size = (mtrr_tom2 >> PAGE_SHIFT) - x_remove_base;
 
+	/* Constrain a WB entry that maps entire phys addr space to max mem */
+	for (i = 0; i < num_var_ranges; i++) {
+		if (range_state[i].type == MTRR_TYPE_WRBACK &&
+		    range_state[i].base_pfn == 0 &&
+		    range_state[i].size_pfn > max_pfn) {
+			range_state[i].size_pfn = roundup_pow_of_two(max_pfn);
+			break;
+		}
+	}
+
 	nr_range = x86_get_mtrr_mem_range(range, 0, x_remove_base, x_remove_size);
 	/*
 	 * [0, 1M) should always be covered by var mtrr with WB
Signature · 1 line
1.7.5.4
reviewer
#1 lore raw

On 09/07/2012 10:44 AM, Peter Hurley wrote:
\>

Quoted 8 lines
arch/x86/kernel/cpu/mtrr/cleanup.c +0 -0
index bdda2e6..ee399c3 100644
@@ -714,6 +714,16 @@ int __init mtrr_cleanup(unsigned address_bits)
  	if (mtrr_tom2)
  		x_remove_size = (mtrr_tom2 >> PAGE_SHIFT) - x_remove_base;

arch/x86/kernel/cpu/mtrr/cleanup.c int __init mtrr_cleanup(unsigned address_bits) · line 726

Quoted 10 lines
arch/x86/kernel/cpu/mtrr/cleanup.c +10 -0
+	/* Constrain a WB entry that maps entire phys addr space to max mem */
+	for (i = 0; i < num_var_ranges; i++) {
+		if (range_state[i].type == MTRR_TYPE_WRBACK &&
+		    range_state[i].base_pfn == 0 &&
+		    range_state[i].size_pfn > max_pfn) {
+			range_state[i].size_pfn = roundup_pow_of_two(max_pfn);
+			break;
+		}
+	}
+

I really don't like it as it introduces yet another user of max_pfn, which should be going away. Furthermore, the better question is what remaining needs there are for MTRR cleanup; historically the reason was that it prevented the display from being mapped WC via MTRR due to the MTRR conflict resolution rules favoring UC.

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

-hpa

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
#2 lore raw
Quoted 5 lines · H. Peter Anvin

On 09/07/2012 10:44 AM, Peter Hurley wrote:
\>

arch/x86/kernel/cpu/mtrr/cleanup.c

Quoted 1 line
arch/x86/kernel/cpu/mtrr/cleanup.c +0 -0

I really don't like it as it introduces yet another user of max_pfn, which should be going away. Furthermore, the better question is what remaining needs there are for MTRR cleanup; historically the reason was that it prevented the display from being mapped WC via MTRR due to the MTRR conflict resolution rules favoring UC.

For a large memory system, mtrr_cleanup offten fail in most case. Even if it succeed, it often occupy all of MTRR entrys. How was display mapped as WC in above case?
Why did bios give a lot of space then real mem, for hotplug?

Quoted 2 lines

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?
Regards
zduan

author
#3 lore raw
Quoted 12 lines · zhenzhong.duan
Quoted 10 lines · H. Peter Anvin

On 09/07/2012 10:44 AM, Peter Hurley wrote:
\>

arch/x86/kernel/cpu/mtrr/cleanup.c

Quoted 2 lines
arch/x86/kernel/cpu/mtrr/cleanup.c +0 -0

I really don't like it as it introduces yet another user of max_pfn, which should be going away. Furthermore, the better question is what remaining needs there are for MTRR cleanup; historically the reason was that it prevented the display from being mapped WC via MTRR due to the MTRR conflict resolution rules favoring UC.

Quoted 3 lines

For a large memory system, mtrr_cleanup offten fail in most case. Even if it succeed, it often occupy all of MTRR entrys.
How was display mapped as WC in above case?

Without this patch, mtrr_cleanup could not optimize. The original MTRR setup from BIOS remained, which left the display as UC (and a lot of log spew).

Quoted 1 line

Why did bios give a lot of space then real mem, for hotplug?

I assume the reason was for hotplug.

An interesting side note: more recent revisions of this BIOS (rev. A11) report one less variable MTRR (so, IA32_MTRRCAP is writable?)

Quoted 4 lines
Quoted 3 lines

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?

Well, since PAT entries marked WC override all MTRR settings, whatever the BIOS set the variable MTRRs to becomes irrelevant, so not disabled but rather ignored.

Regards,
Peter Hurley

���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m���� ������?�I�

#4 lore raw
Quoted 11 lines · Peter Hurley
Quoted 10 lines · zhenzhong.duan
Quoted 9 lines · H. Peter Anvin

arch/x86/kernel/cpu/mtrr/cleanup.c

Quoted 2 lines · Peter Hurley
arch/x86/kernel/cpu/mtrr/cleanup.c +0 -0

I really don't like it as it introduces yet another user of max_pfn, which should be going away. Furthermore, the better question is what remaining needs there are for MTRR cleanup; historically the reason was that it prevented the display from being mapped WC via MTRR due to the MTRR conflict resolution rules favoring UC.

Quoted 6 lines
Quoted 3 lines

For a large memory system, mtrr_cleanup offten fail in most case. Even if it succeed, it often occupy all of MTRR entrys.
How was display mapped as WC in above case?

Without this patch, mtrr_cleanup could not optimize. The original MTRR setup from BIOS remained, which left the display as UC (and a lot of log spew).

Hi,
I am confused here.
Does HPA means mtrr_cleanup's purpose is to occupy all mtrr entrys and prevent display setting a WC entry in it? As page level will do that in current code? If it is, then mtrr_cleanup could be removed now.

Quoted 6 lines
Quoted 2 lines

Why did bios give a lot of space then real mem, for hotplug?

I assume the reason was for hotplug.

An interesting side note: more recent revisions of this BIOS (rev. A11) report one less variable MTRR (so, IA32_MTRRCAP is writable?)

From manual, it's readonly, writing it will lead to #GP.

Quoted 8 lines
Quoted 5 lines
Quoted 4 lines

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?

Well, since PAT entries marked WC override all MTRR settings, whatever the BIOS set the variable MTRRs to becomes irrelevant, so not disabled but rather ignored.

Oh, I see, WC in page level take precedence. Is the fix already in upstream? thanks
zduan

reviewer
#5 lore raw
In reply to Peter Hurley
Quoted 4 lines · Peter Hurley

An interesting side note: more recent revisions of this BIOS (rev. A11) report one less variable MTRR (so, IA32_MTRRCAP is writable?)

Quoted 9 lines
Quoted 4 lines
Quoted 3 lines

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?

Well, since PAT entries marked WC override all MTRR settings, whatever the BIOS set the variable MTRRs to becomes irrelevant, so not disabled but rather ignored.

The whole point is that the display stuff should not use MTRR, but rather use PAT to provide WC. Then we don't need to "clean up" the BIOS-set MTRRs.

-hpa

#6 lore raw
Quoted 9 lines · H. Peter Anvin
Quoted 8 lines · Peter Hurley

An interesting side note: more recent revisions of this BIOS (rev. A11) report one less variable MTRR (so, IA32_MTRRCAP is writable?)

Quoted 4 lines
Quoted 3 lines

However, the right way to fix that is to use the PAT interfaces, which doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?

Quoted 8 lines
Quoted 4 lines

Well, since PAT entries marked WC override all MTRR settings, whatever the BIOS set the variable MTRRs to becomes irrelevant, so not disabled but rather ignored.

The whole point is that the display stuff should not use MTRR, but rather use PAT to provide WC. Then we don't need to "clean up" the BIOS-set MTRRs.

What about my trusty ThinkPad T43, and other ia32 boxes that either lack PAT support, or have it blacklisted?

These are exactly the boxes that will _never_ get a BIOS fix if their MTRRs are screwed up...

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
reviewer
#7 lore raw

PAT support are lacking only in the Pentium Pro and Pentium II. Sorry, if you're using crap that old, you don't get to screw up the kernel for everyone else.

Quoted 30 lines · Henrique de Moraes Holschuh
Quoted 2 lines · H. Peter Anvin
Quoted 1 line · Peter Hurley

An interesting side note: more recent revisions of this BIOS (rev.

A11)

Quoted 3 lines
Quoted 3 lines

report one less variable MTRR (so, IA32_MTRRCAP is writable?)

Quoted 1 line
Quoted 1 line

However, the right way to fix that is to use the PAT interfaces,

which

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines

doesn't have this drawback -- then MTRR cleanup becomes entirely superfluous and the problem goes away.

Do you mean disable MTRR totally here?

Well, since PAT entries marked WC override all MTRR settings,

whatever

Quoted 1 line
Quoted 1 line

the BIOS set the variable MTRRs to becomes irrelevant, so not

disabled

Quoted 6 lines
Quoted 2 lines

but rather ignored.

The whole point is that the display stuff should not use MTRR, but rather use PAT to provide WC. Then we don't need to "clean up" the BIOS-set MTRRs.

What about my trusty ThinkPad T43, and other ia32 boxes that either lack PAT
support, or have it blacklisted?

These are exactly the boxes that will _never_ get a BIOS fix if their MTRRs
are screwed up...

Signature · 1 line
Sent from my mobile phone. Please excuse brevity and lack of formatting.
#8 lore raw
Quoted 3 lines · H. Peter Anvin

PAT support are lacking only in the Pentium Pro and Pentium II. Sorry, if you're using crap that old, you don't get to screw up the kernel for everyone else.

PAT is blacklisted for x86_model < 15 on Intel, which covers a lot more boxes than p-pro and pII. There is a comment on kernel/cpu/intel.c:

/*
* 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 == 6 && c->x86_model < 15)
clear_cpu_cap(c, X86_FEATURE_PAT);

Intel doesn't make it easy to get all processor specification updates at once so that I could hunt down every processor which acknowledges the existence of that errata before replying, so I will assume for the moment that the comment is mostly correct.

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
reviewer
#9 lore raw
Quoted 21 lines · Henrique de Moraes Holschuh
Quoted 3 lines · H. Peter Anvin

PAT support are lacking only in the Pentium Pro and Pentium II. Sorry, if you're using crap that old, you don't get to screw up the kernel for everyone else.

PAT is blacklisted for x86_model < 15 on Intel, which covers a lot more boxes than p-pro and pII. There is a comment on kernel/cpu/intel.c:

/*
* 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 == 6 && c->x86_model < 15)
clear_cpu_cap(c, X86_FEATURE_PAT);

Quoted 5 lines

Intel doesn't make it easy to get all processor specification updates at once so that I could hunt down every processor which acknowledges the existence of that errata before replying, so I will assume for the moment that the comment is mostly correct.

Last I checked it was questionable if the erratum actually mattered enough to bother with. What is even more questionable is the number of machines which need the workaround *and* need the "MTRR cleanup" mess.

-hpa

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
reviewer
#10 lore raw
In reply to Henrique de Moraes Holschuh
Quoted 6 lines

Intel doesn't make it easy to get all processor specification updates at once so that I could hunt down every processor which acknowledges the existence of that errata before replying, so I will assume for the moment that the comment is mostly correct.

I should say: as far as I know the blacklist is there because noone has been willing to bother looking at if it actually matters, given how old the hardware is.

-hpa

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
#11 lore raw
Quoted 8 lines · H. Peter Anvin
Quoted 4 lines

Intel doesn't make it easy to get all processor specification updates at once so that I could hunt down every processor which acknowledges the existence of that errata before replying, so I will assume for the moment that the comment is mostly correct.

I should say: as far as I know the blacklist is there because noone has been willing to bother looking at if it actually matters, given how old the hardware is.

Hmm, well, I will probably have to look it up for a few processors that matter to me, apparently.

Can't people that work at Intel at least petition for meta-index pages of this kind of documentation?

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh