Change in coreboot[master]: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[Raul Rangel (Code Review)]

Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

The AMD64 Architecture Programmer's Manual, Volume 2: Systems
Programming says the following about variable MTRRs:

Variable Range Size and Alignment.
The size and alignment of variable memory-ranges (MTRRs) and I/O ranges
(IORRs) are restricted as follows:
* The boundary on which a variable range is aligned must be equal to the
range size. For example, a memory range of 16 Mbytes must be aligned on a
16-Mbyte boundary.
* The range size must be a power of 2 (2 n , 52 > n > 11), with a minimum
allowable size of 4 Kbytes. For example, 4 Mbytes and 8 Mbytes are
allowable memory range sizes, but 6 Mbytes is not allowable.

Print out errors if these conditions are violated. I didn't assert since
`set_var_mtrr` can be used in boot block before the serial console is
enabled.

BUG=b:147042464
TEST=Boot trembyle and see MTRR errors:
MTRR Error: base 0xcc800000 must be aligned to size 0x1000000

Signed-off-by: Raul E Rangel 
Change-Id: I8b8c734c7599bd89cf9f212ed43c2dd5b2c8ba7b
---
M src/cpu/x86/mtrr/earlymtrr.c
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40762/1

diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c
index 4d14a8d..1fa0936 100644
--- a/src/cpu/x86/mtrr/earlymtrr.c
+++ b/src/cpu/x86/mtrr/earlymtrr.c
@@ -5,6 +5,8 @@
 #include <cpu/x86/cache.h>
 #include <cpu/x86/mtrr.h>
 #include <cpu/x86/msr.h>
+#include <console/console.h>
+#include <lib.h>

/* Get first available variable MTRR.
  * Returns var# if available, else returns -1.
@@ -35,6 +37,14 @@
    /* Bit Bit 32-35 of MTRRphysMask should be set to 1 */
    /* FIXME: It only support 4G less range */
    msr_t basem, maskm;
+
+	if (!is_power_of_two(size))
+		printk(BIOS_ERR, "MTRR Error: size %#x is not a power of two\n", size);
+	if (size < 4 * KiB)
+		printk(BIOS_ERR, "MTRR Error: size %#x smaller than 4KiB\n", size);
+	if (base % size != 0)
+		printk(BIOS_ERR, "MTRR Error: base %#x must be aligned to size %#x\n", base, size);
+
    basem.lo = base | type;
    basem.hi = 0;
    wrmsr(MTRR_PHYS_BASE(reg), basem);
@@ -42,3 +52,4 @@
    maskm.hi = (1 << (cpu_phys_address_size() - 32)) - 1;
    wrmsr(MTRR_PHYS_MASK(reg), maskm);
 }
+

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40762/1/src/cpu/x86/mtrr/earlymtrr.... 
File src/cpu/x86/mtrr/earlymtrr.c:

https://review.coreboot.org/c/coreboot/+/40762/1/src/cpu/x86/mtrr/earlymtrr.... 
PS1, Line 46: 		printk(BIOS_ERR, "MTRR Error: base %#x must be aligned to size %#x\n", base, size);
line over 96 characters

[Raul Rangel (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40762

to look at the new patch set (#2).

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

The AMD64 Architecture Programmer's Manual, Volume 2: Systems
Programming says the following about variable MTRRs:

Variable Range Size and Alignment.
The size and alignment of variable memory-ranges (MTRRs) and I/O ranges
(IORRs) are restricted as follows:
* The boundary on which a variable range is aligned must be equal to the
range size. For example, a memory range of 16 Mbytes must be aligned on a
16-Mbyte boundary.
* The range size must be a power of 2 (2 n , 52 > n > 11), with a minimum
allowable size of 4 Kbytes. For example, 4 Mbytes and 8 Mbytes are
allowable memory range sizes, but 6 Mbytes is not allowable.

Print out errors if these conditions are violated. I didn't assert since
`set_var_mtrr` can be used in boot block before the serial console is
enabled.

BUG=b:147042464
TEST=Boot trembyle and see MTRR errors:
MTRR Error: base 0xcc800000 must be aligned to size 0x1000000

Signed-off-by: Raul E Rangel 
Change-Id: I8b8c734c7599bd89cf9f212ed43c2dd5b2c8ba7b
---
M src/cpu/x86/mtrr/earlymtrr.c
1 file changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40762/2

[Raul Rangel (Code Review)]

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40762

to look at the new patch set (#3).

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

The AMD64 Architecture Programmer's Manual, Volume 2: Systems
Programming says the following about variable MTRRs:

Variable Range Size and Alignment.
The size and alignment of variable memory-ranges (MTRRs) and I/O ranges
(IORRs) are restricted as follows:
* The boundary on which a variable range is aligned must be equal to the
range size. For example, a memory range of 16 Mbytes must be aligned on a
16-Mbyte boundary.
* The range size must be a power of 2 (2 n , 52 > n > 11), with a minimum
allowable size of 4 Kbytes. For example, 4 Mbytes and 8 Mbytes are
allowable memory range sizes, but 6 Mbytes is not allowable.

Print out errors if these conditions are violated. I didn't assert since
`set_var_mtrr` can be used in boot block before the serial console is
enabled.

BUG=b:147042464
TEST=Boot trembyle and see MTRR errors:
MTRR Error: base 0xcc800000 must be aligned to size 0x1000000

Signed-off-by: Raul E Rangel 
Change-Id: I8b8c734c7599bd89cf9f212ed43c2dd5b2c8ba7b
---
M src/cpu/x86/mtrr/earlymtrr.c
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40762/3

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 3:

(2 comments)

Nice. Let’s see what is reported on current devices.

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@7 
PS3, Line 7: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
Please remove `src/` from the beginning.

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@18 
PS3, Line 18: 2 n
2^n

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 3: Code-Review+1

(3 comments)

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@10 
PS3, Line 10: Programming says the following about variable MTRRs:
FWIW, these constraints are x86 architectural.

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@17 
PS3, Line 17: 16-Mbyte boundary.
aka naturally aligned

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@24 
PS3, Line 24: enabled.
fyi, src/arch/x86/postcar_loader.c has code for setting up MTRRs based on regions passed in.

Change in coreboot[master]: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Marshall Dawson, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/40762

to look at the new patch set (#4).

Change subject: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

The AMD64 Architecture Programmer's Manual, Volume 2: Systems
Programming says the following about variable MTRRs:

Variable Range Size and Alignment.
The size and alignment of variable memory-ranges (MTRRs) and I/O ranges
(IORRs) are restricted as follows:
* The boundary on which a variable range is aligned must be equal to the
range size. For example, a memory range of 16 Mbytes must be aligned on a
16-Mbyte boundary (i.e., naturally aligned).
* The range size must be a power of 2 (2^n , 52 > n > 11), with a minimum
allowable size of 4 Kbytes. For example, 4 Mbytes and 8 Mbytes are
allowable memory range sizes, but 6 Mbytes is not allowable.

Print out errors if these conditions are violated. I didn't assert since
`set_var_mtrr` can be used in boot block before the serial console is
enabled.

BUG=b:147042464
TEST=Boot trembyle and see MTRR errors:
MTRR Error: base 0xcc800000 must be aligned to size 0x1000000

Signed-off-by: Raul E Rangel 
Change-Id: I8b8c734c7599bd89cf9f212ed43c2dd5b2c8ba7b
---
M src/cpu/x86/mtrr/earlymtrr.c
1 file changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40762/4

Change in coreboot[master]: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 4:

(5 comments)

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@7 
PS3, Line 7: src/cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
> Please remove `src/` from the beginning.

Done

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@10 
PS3, Line 10: Programming says the following about variable MTRRs:
> FWIW, these constraints are x86 architectural.

Right, I was just stating my source.

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@17 
PS3, Line 17: 16-Mbyte boundary.
> aka naturally aligned

Done

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@18 
PS3, Line 18: 2 n
> 2^n

Done

https://review.coreboot.org/c/coreboot/+/40762/3//COMMIT_MSG@24 
PS3, Line 24: enabled.
> fyi, src/arch/x86/postcar_loader.c has code for setting up MTRRs based on regions passed in.

Ack

Change in coreboot[master]: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[Aaron Durbin (Code Review)]

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

The AMD64 Architecture Programmer's Manual, Volume 2: Systems
Programming says the following about variable MTRRs:

Variable Range Size and Alignment.
The size and alignment of variable memory-ranges (MTRRs) and I/O ranges
(IORRs) are restricted as follows:
* The boundary on which a variable range is aligned must be equal to the
range size. For example, a memory range of 16 Mbytes must be aligned on a
16-Mbyte boundary (i.e., naturally aligned).
* The range size must be a power of 2 (2^n , 52 > n > 11), with a minimum
allowable size of 4 Kbytes. For example, 4 Mbytes and 8 Mbytes are
allowable memory range sizes, but 6 Mbytes is not allowable.

Print out errors if these conditions are violated. I didn't assert since
`set_var_mtrr` can be used in boot block before the serial console is
enabled.

BUG=b:147042464
TEST=Boot trembyle and see MTRR errors:
MTRR Error: base 0xcc800000 must be aligned to size 0x1000000

Signed-off-by: Raul E Rangel 
Change-Id: I8b8c734c7599bd89cf9f212ed43c2dd5b2c8ba7b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40762
Reviewed-by: Aaron Durbin 
Tested-by: build bot (Jenkins) 
---
M src/cpu/x86/mtrr/earlymtrr.c
1 file changed, 11 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c
index e8608fd..f96b050 100644
--- a/src/cpu/x86/mtrr/earlymtrr.c
+++ b/src/cpu/x86/mtrr/earlymtrr.c
@@ -4,6 +4,8 @@
 #include <cpu/cpu.h>
 #include <cpu/x86/mtrr.h>
 #include <cpu/x86/msr.h>
+#include <console/console.h>
+#include <commonlib/bsd/helpers.h>

/* Get first available variable MTRR.
  * Returns var# if available, else returns -1.
@@ -34,6 +36,15 @@
    /* Bit Bit 32-35 of MTRRphysMask should be set to 1 */
    /* FIXME: It only support 4G less range */
    msr_t basem, maskm;
+
+	if (!IS_POWER_OF_2(size))
+		printk(BIOS_ERR, "MTRR Error: size %#x is not a power of two\n", size);
+	if (size < 4 * KiB)
+		printk(BIOS_ERR, "MTRR Error: size %#x smaller than 4KiB\n", size);
+	if (base % size != 0)
+		printk(BIOS_ERR, "MTRR Error: base %#x must be aligned to size %#x\n", base,
+		       size);
+
    basem.lo = base | type;
    basem.hi = 0;
    wrmsr(MTRR_PHYS_BASE(reg), basem);

Change in coreboot[master]: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40762 )

Change subject: cpu/x86/mtrr/earlymtrr: Validate MTRR arguments
......................................................................

Patch Set 5:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3052
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3051
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3050
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3049

Please note: This test is under development and might not be accurate at all!