Change in coreboot[master]: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr

[Raul Rangel (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr

Picasso does not define the state of variable MTRRs on boot. Add a
helper function to clear all MTRRs.

BUG=b:147042464
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: I21b887ce12849a95ddd8f1698028fb6bbfb4a7f6
---
M src/cpu/x86/mtrr/earlymtrr.c
M src/include/cpu/x86/mtrr.h
2 files changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/40764/1

diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c
index 4d14a8d..e400359 100644
--- a/src/cpu/x86/mtrr/earlymtrr.c
+++ b/src/cpu/x86/mtrr/earlymtrr.c
@@ -42,3 +42,17 @@
    maskm.hi = (1 << (cpu_phys_address_size() - 32)) - 1;
    wrmsr(MTRR_PHYS_MASK(reg), maskm);
 }
+
+void clear_all_var_mtrr(void)
+{
+	msr_t mtrr = {0, 0};
+	int vcnt;
+	int i;
+
+	vcnt = get_var_mtrr_count();
+
+	for (i = 0; i < vcnt; i++) {
+		wrmsr(MTRR_PHYS_MASK(i), mtrr);
+		wrmsr(MTRR_PHYS_BASE(i), mtrr);
+	}
+}
diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index 07db3cb..50148ff 100644
--- a/src/include/cpu/x86/mtrr.h
+++ b/src/include/cpu/x86/mtrr.h
@@ -107,6 +107,7 @@
 void set_var_mtrr(unsigned int reg, unsigned int base, unsigned int size,
    unsigned int type);
 int get_free_var_mtrr(void);
+void clear_all_var_mtrr(void);

asmlinkage void display_mtrrs(void);

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40764 )

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40764/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40764/1//COMMIT_MSG@9 
PS1, Line 9: variable MTRRs
What about fixed MTRRs?

[Paul Menzel (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/40764/1/src/cpu/x86/mtrr/earlymtrr.... 
PS1, Line 50: int
unsigned int

The compiler might then use shift when multiplying?

#define MTRR_PHYS_BASE(reg)             (0x200 + 2 * (reg))
    #define MTRR_PHYS_MASK(reg)             (MTRR_PHYS_BASE(reg) + 1)

[Raul Rangel (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/40764/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40764/1//COMMIT_MSG@9 
PS1, Line 9: variable MTRRs
> What about fixed MTRRs?

Those are undefined as well. But since I keep them disabled I don't bother clearing them.

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

https://review.coreboot.org/c/coreboot/+/40764/1/src/cpu/x86/mtrr/earlymtrr.... 
PS1, Line 50: int
> unsigned int […]

I kept the same data types as get_free_var_mtrr. get_var_mtrr_count also returns an int. If you think this needs clean up, we can do that in a followup.

[Aaron Durbin (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

Patch Set 4: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr

Picasso does not define the state of variable MTRRs on boot. Add a
helper function to clear all MTRRs.

BUG=b:147042464
TEST=Build trembyle

Signed-off-by: Raul E Rangel 
Change-Id: I21b887ce12849a95ddd8f1698028fb6bbfb4a7f6
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40764
Reviewed-by: Aaron Durbin 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/cpu/x86/mtrr/earlymtrr.c
M src/include/cpu/x86/mtrr.h
2 files changed, 15 insertions(+), 0 deletions(-)

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

diff --git a/src/cpu/x86/mtrr/earlymtrr.c b/src/cpu/x86/mtrr/earlymtrr.c
index 4d14a8d..e400359 100644
--- a/src/cpu/x86/mtrr/earlymtrr.c
+++ b/src/cpu/x86/mtrr/earlymtrr.c
@@ -42,3 +42,17 @@
    maskm.hi = (1 << (cpu_phys_address_size() - 32)) - 1;
    wrmsr(MTRR_PHYS_MASK(reg), maskm);
 }
+
+void clear_all_var_mtrr(void)
+{
+	msr_t mtrr = {0, 0};
+	int vcnt;
+	int i;
+
+	vcnt = get_var_mtrr_count();
+
+	for (i = 0; i < vcnt; i++) {
+		wrmsr(MTRR_PHYS_MASK(i), mtrr);
+		wrmsr(MTRR_PHYS_BASE(i), mtrr);
+	}
+}
diff --git a/src/include/cpu/x86/mtrr.h b/src/include/cpu/x86/mtrr.h
index 07db3cb..50148ff 100644
--- a/src/include/cpu/x86/mtrr.h
+++ b/src/include/cpu/x86/mtrr.h
@@ -107,6 +107,7 @@
 void set_var_mtrr(unsigned int reg, unsigned int base, unsigned int size,
    unsigned int type);
 int get_free_var_mtrr(void);
+void clear_all_var_mtrr(void);

asmlinkage void display_mtrrs(void);

[9elements QA (Code Review)]

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

Change subject: src/cpu/x86/mtrr/earlymtrr: Add clear_all_var_mtrr
......................................................................

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/2840
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2839
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2838
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2837

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