Change in coreboot[master]: soc/amd/picasso: remove save/restore MTRRs around FSP-M

[Aaron Durbin (Code Review)]

Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42104 )

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

soc/amd/picasso: remove save/restore MTRRs around FSP-M

AGESA FSP-M implementation is now not updating MTRRs out from
under the caller. As such, remove the save/restore of MTRRs
from the FSP-M call.

BUG=b:155426691

Change-Id: I14f3b18dd373ce17957ef3857920e1c4e2901bbe
Signed-off-by: Aaron Durbin 
---
M src/soc/amd/picasso/Makefile.inc
D src/soc/amd/picasso/include/soc/mtrr.h
D src/soc/amd/picasso/mtrr.c
M src/soc/amd/picasso/romstage.c
4 files changed, 0 insertions(+), 138 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/42104/1

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index ce0a7a8..9f15557 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -30,7 +30,6 @@
 romstage-y += southbridge.c
 romstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c
 romstage-y += psp.c
-romstage-y += mtrr.c
 romstage-y += config.c

verstage-y += gpio.c
diff --git a/src/soc/amd/picasso/include/soc/mtrr.h b/src/soc/amd/picasso/include/soc/mtrr.h
deleted file mode 100644
index 8746e18..0000000
--- a/src/soc/amd/picasso/include/soc/mtrr.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#ifndef __PICASSO_MTRR_H__
-#define __PICASSO_MTRR_H__
-
-void picasso_save_mtrrs(void);
-void picasso_restore_mtrrs(void);
-
-#endif /* __PICASSO_MTRR_H__ */
diff --git a/src/soc/amd/picasso/mtrr.c b/src/soc/amd/picasso/mtrr.c
deleted file mode 100644
index d2f8ff3..0000000
--- a/src/soc/amd/picasso/mtrr.c
+++ /dev/null
@@ -1,111 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <assert.h>
-#include <commonlib/bsd/helpers.h>
-#include <console/console.h>
-#include <cpu/x86/mtrr.h>
-#include <cpu/amd/mtrr.h>
-#include <soc/mtrr.h>
-
-/* Picasso defines 8 Variable MTRRs */
-#define MAX_VARIABLE_MTRRS 8
-#define SYS_CFG_MTRR_BITS ( \
-SYSCFG_MSR_TOM2WB | \
-SYSCFG_MSR_TOM2En | \
-SYSCFG_MSR_MtrrVarDramEn | \
-SYSCFG_MSR_MtrrFixDramModEn | \
-SYSCFG_MSR_MtrrFixDramEn \
-)
-
-static const unsigned int fixed_mtrr_offsets[] = {
-	MTRR_FIX_64K_00000,
-	MTRR_FIX_16K_80000,
-	MTRR_FIX_16K_A0000,
-	MTRR_FIX_4K_C0000,
-	MTRR_FIX_4K_C8000,
-	MTRR_FIX_4K_D0000,
-	MTRR_FIX_4K_D8000,
-	MTRR_FIX_4K_E0000,
-	MTRR_FIX_4K_E8000,
-	MTRR_FIX_4K_F0000,
-	MTRR_FIX_4K_F8000,
-};
-
-static int mtrrs_saved;
-static msr_t sys_cfg;
-static msr_t mtrr_def;
-static msr_t mtrr_base[MAX_VARIABLE_MTRRS];
-static msr_t mtrr_mask[MAX_VARIABLE_MTRRS];
-static msr_t fixed_mtrrs[ARRAY_SIZE(fixed_mtrr_offsets)];
-
-void picasso_save_mtrrs(void)
-{
-	unsigned int i;
-	int mtrrs;
-
-	mtrrs = get_var_mtrr_count();
-
-	ASSERT_MSG(mtrrs == MAX_VARIABLE_MTRRS, "Unexpected number of MTRRs\n");
-
-	for (i = 0; i < MAX_VARIABLE_MTRRS; ++i) {
-		mtrr_base[i] = rdmsr(MTRR_PHYS_BASE(i));
-		mtrr_mask[i] = rdmsr(MTRR_PHYS_MASK(i));
-		printk(BIOS_DEBUG,
-		       "Saving Variable MTRR %d: Base: 0x%08x 0x%08x, Mask: 0x%08x 0x%08x\n", i,
-		       mtrr_base[i].hi, mtrr_base[i].lo, mtrr_mask[i].hi, mtrr_mask[i].lo);
-	}
-
-	for (i = 0; i < ARRAY_SIZE(fixed_mtrr_offsets); ++i) {
-		fixed_mtrrs[i] = rdmsr(fixed_mtrr_offsets[i]);
-		printk(BIOS_DEBUG, "Saving Fixed MTRR %u: 0x%08x 0x%08x\n", i,
-		       fixed_mtrrs[i].hi, fixed_mtrrs[i].lo);
-	}
-
-	mtrr_def = rdmsr(MTRR_DEF_TYPE_MSR);
-	printk(BIOS_DEBUG, "Saving Default Type MTRR: 0x%08x 0x%08x\n", mtrr_def.hi,
-	       mtrr_def.lo);
-
-	sys_cfg = rdmsr(SYSCFG_MSR);
-	printk(BIOS_DEBUG, "Saving SYS_CFG: 0x%08x 0x%08x\n", mtrr_def.hi, mtrr_def.lo);
-
-	mtrrs_saved = 1;
-}
-
-static void update_if_changed(unsigned int offset, msr_t expected)
-{
-	msr_t tmp = rdmsr(offset);
-	if (tmp.lo == expected.lo && tmp.hi == expected.hi)
-		return;
-
-	printk(BIOS_INFO, "MSR %#x was modified: 0x%08x 0x%08x\n", offset, tmp.hi, tmp.lo);
-	wrmsr(offset, expected);
-}
-
-void picasso_restore_mtrrs(void)
-{
-	unsigned int i;
-	msr_t tmp_sys_cfg;
-
-	ASSERT_MSG(mtrrs_saved, "Must save MTRRs before restoring.\n");
-
-	for (i = 0; i < MAX_VARIABLE_MTRRS; ++i) {
-		update_if_changed(MTRR_PHYS_BASE(i), mtrr_base[i]);
-		update_if_changed(MTRR_PHYS_MASK(i), mtrr_mask[i]);
-	}
-
-	for (i = 0; i < ARRAY_SIZE(fixed_mtrr_offsets); ++i)
-		update_if_changed(fixed_mtrr_offsets[i], fixed_mtrrs[i]);
-
-	update_if_changed(MTRR_DEF_TYPE_MSR, mtrr_def);
-
-	tmp_sys_cfg = rdmsr(SYSCFG_MSR);
-
-	/* We only care about the MTRR bits in the SYSCFG register */
-	if ((tmp_sys_cfg.lo & SYS_CFG_MTRR_BITS) != (sys_cfg.lo & SYS_CFG_MTRR_BITS)) {
-		printk(BIOS_INFO, "SYS_CFG was modified: 0x%08x 0x%08x\n", tmp_sys_cfg.hi,
-		       tmp_sys_cfg.lo);
-		tmp_sys_cfg.lo &= ~SYS_CFG_MTRR_BITS;
-		tmp_sys_cfg.lo |= (sys_cfg.lo & SYS_CFG_MTRR_BITS);
-		wrmsr(SYSCFG_MSR, tmp_sys_cfg);
-	}
-}
diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c
index 0bbceec..f3330dd 100644
--- a/src/soc/amd/picasso/romstage.c
+++ b/src/soc/amd/picasso/romstage.c
@@ -12,7 +12,6 @@
 #include <program_loading.h>
 #include <elog.h>
 #include <soc/romstage.h>
-#include <soc/mtrr.h>
 #include <types.h>
 #include "chip.h"
 #include <fsp/api.h>
@@ -22,16 +21,6 @@
    /* By default, don't do anything */
 }

-/* TODO(b/155426691): Make FSP AGESA leave MTRRs alone */
-static void clear_agesa_mtrrs(void)
-{
-	disable_cache();
-
-	picasso_restore_mtrrs();
-
-	enable_cache();
-}
-
 void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version)
 {
    FSP_M_CONFIG *mcfg = &mupd->FspmConfig;
@@ -94,15 +83,9 @@
    u32 val = cpuid_eax(1);
    printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

-	post_code(0x43);
-	picasso_save_mtrrs();
-
    post_code(0x44);
    fsp_memory_init(s3_resume);

-	post_code(0x45);
-	clear_agesa_mtrrs();
-
    post_code(0x46);
    run_ramstage();

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42104 )

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
File src/soc/amd/picasso/romstage.c:

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
PS1, Line 86: 	post_code(0x44);
update post codes?

[Aaron Durbin (Code Review)]

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

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
File src/soc/amd/picasso/romstage.c:

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
PS1, Line 86: 	post_code(0x44);
> update post codes?

I didn't know if anyone was reliant upon the values. I can adjust them.

[Aaron Durbin (Code Review)]

Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Patrick Georgi, Martin Roth, Felix Held,

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

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

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

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

soc/amd/picasso: remove save/restore MTRRs around FSP-M

AGESA FSP-M implementation is now not updating MTRRs out from
under the caller. As such, remove the save/restore of MTRRs
from the FSP-M call.

BUG=b:155426691

Change-Id: I14f3b18dd373ce17957ef3857920e1c4e2901bbe
Signed-off-by: Aaron Durbin 
---
M src/soc/amd/picasso/Makefile.inc
D src/soc/amd/picasso/include/soc/mtrr.h
D src/soc/amd/picasso/mtrr.c
M src/soc/amd/picasso/romstage.c
4 files changed, 1 insertion(+), 139 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/42104/2

[Aaron Durbin (Code Review)]

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

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
File src/soc/amd/picasso/romstage.c:

https://review.coreboot.org/c/coreboot/+/42104/1/src/soc/amd/picasso/romstag... 
PS1, Line 86: 	post_code(0x44);
> I didn't know if anyone was reliant upon the values. I can adjust them.

Done

[Raul Rangel (Code Review)]

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

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 2: Code-Review+2

[Furquan Shaikh (Code Review)]

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

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 2: Code-Review+2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42104 )

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 2: Code-Review-1

see my comment on 41898

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42104 )

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

Patch Set 2: Code-Review+2

reverting an unrelated fsp change fixed things for me

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42104 )

Change subject: soc/amd/picasso: remove save/restore MTRRs around FSP-M
......................................................................

soc/amd/picasso: remove save/restore MTRRs around FSP-M

AGESA FSP-M implementation is now not updating MTRRs out from
under the caller. As such, remove the save/restore of MTRRs
from the FSP-M call.

BUG=b:155426691

Change-Id: I14f3b18dd373ce17957ef3857920e1c4e2901bbe
Signed-off-by: Aaron Durbin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42104
Tested-by: build bot (Jenkins) 
Reviewed-by: Raul Rangel 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Felix Held 
---
M src/soc/amd/picasso/Makefile.inc
D src/soc/amd/picasso/include/soc/mtrr.h
D src/soc/amd/picasso/mtrr.c
M src/soc/amd/picasso/romstage.c
4 files changed, 1 insertion(+), 139 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Raul Rangel: Looks good to me, approved

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 5744c63..3b59684 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -30,7 +30,6 @@
 romstage-y += southbridge.c
 romstage-$(CONFIG_HAVE_SMI_HANDLER) += smi_util.c
 romstage-y += psp.c
-romstage-y += mtrr.c
 romstage-y += config.c

verstage-y += gpio.c
diff --git a/src/soc/amd/picasso/include/soc/mtrr.h b/src/soc/amd/picasso/include/soc/mtrr.h
deleted file mode 100644
index 8746e18..0000000
--- a/src/soc/amd/picasso/include/soc/mtrr.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#ifndef __PICASSO_MTRR_H__
-#define __PICASSO_MTRR_H__
-
-void picasso_save_mtrrs(void);
-void picasso_restore_mtrrs(void);
-
-#endif /* __PICASSO_MTRR_H__ */
diff --git a/src/soc/amd/picasso/mtrr.c b/src/soc/amd/picasso/mtrr.c
deleted file mode 100644
index d2f8ff3..0000000
--- a/src/soc/amd/picasso/mtrr.c
+++ /dev/null
@@ -1,111 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-
-#include <assert.h>
-#include <commonlib/bsd/helpers.h>
-#include <console/console.h>
-#include <cpu/x86/mtrr.h>
-#include <cpu/amd/mtrr.h>
-#include <soc/mtrr.h>
-
-/* Picasso defines 8 Variable MTRRs */
-#define MAX_VARIABLE_MTRRS 8
-#define SYS_CFG_MTRR_BITS ( \
-SYSCFG_MSR_TOM2WB | \
-SYSCFG_MSR_TOM2En | \
-SYSCFG_MSR_MtrrVarDramEn | \
-SYSCFG_MSR_MtrrFixDramModEn | \
-SYSCFG_MSR_MtrrFixDramEn \
-)
-
-static const unsigned int fixed_mtrr_offsets[] = {
-	MTRR_FIX_64K_00000,
-	MTRR_FIX_16K_80000,
-	MTRR_FIX_16K_A0000,
-	MTRR_FIX_4K_C0000,
-	MTRR_FIX_4K_C8000,
-	MTRR_FIX_4K_D0000,
-	MTRR_FIX_4K_D8000,
-	MTRR_FIX_4K_E0000,
-	MTRR_FIX_4K_E8000,
-	MTRR_FIX_4K_F0000,
-	MTRR_FIX_4K_F8000,
-};
-
-static int mtrrs_saved;
-static msr_t sys_cfg;
-static msr_t mtrr_def;
-static msr_t mtrr_base[MAX_VARIABLE_MTRRS];
-static msr_t mtrr_mask[MAX_VARIABLE_MTRRS];
-static msr_t fixed_mtrrs[ARRAY_SIZE(fixed_mtrr_offsets)];
-
-void picasso_save_mtrrs(void)
-{
-	unsigned int i;
-	int mtrrs;
-
-	mtrrs = get_var_mtrr_count();
-
-	ASSERT_MSG(mtrrs == MAX_VARIABLE_MTRRS, "Unexpected number of MTRRs\n");
-
-	for (i = 0; i < MAX_VARIABLE_MTRRS; ++i) {
-		mtrr_base[i] = rdmsr(MTRR_PHYS_BASE(i));
-		mtrr_mask[i] = rdmsr(MTRR_PHYS_MASK(i));
-		printk(BIOS_DEBUG,
-		       "Saving Variable MTRR %d: Base: 0x%08x 0x%08x, Mask: 0x%08x 0x%08x\n", i,
-		       mtrr_base[i].hi, mtrr_base[i].lo, mtrr_mask[i].hi, mtrr_mask[i].lo);
-	}
-
-	for (i = 0; i < ARRAY_SIZE(fixed_mtrr_offsets); ++i) {
-		fixed_mtrrs[i] = rdmsr(fixed_mtrr_offsets[i]);
-		printk(BIOS_DEBUG, "Saving Fixed MTRR %u: 0x%08x 0x%08x\n", i,
-		       fixed_mtrrs[i].hi, fixed_mtrrs[i].lo);
-	}
-
-	mtrr_def = rdmsr(MTRR_DEF_TYPE_MSR);
-	printk(BIOS_DEBUG, "Saving Default Type MTRR: 0x%08x 0x%08x\n", mtrr_def.hi,
-	       mtrr_def.lo);
-
-	sys_cfg = rdmsr(SYSCFG_MSR);
-	printk(BIOS_DEBUG, "Saving SYS_CFG: 0x%08x 0x%08x\n", mtrr_def.hi, mtrr_def.lo);
-
-	mtrrs_saved = 1;
-}
-
-static void update_if_changed(unsigned int offset, msr_t expected)
-{
-	msr_t tmp = rdmsr(offset);
-	if (tmp.lo == expected.lo && tmp.hi == expected.hi)
-		return;
-
-	printk(BIOS_INFO, "MSR %#x was modified: 0x%08x 0x%08x\n", offset, tmp.hi, tmp.lo);
-	wrmsr(offset, expected);
-}
-
-void picasso_restore_mtrrs(void)
-{
-	unsigned int i;
-	msr_t tmp_sys_cfg;
-
-	ASSERT_MSG(mtrrs_saved, "Must save MTRRs before restoring.\n");
-
-	for (i = 0; i < MAX_VARIABLE_MTRRS; ++i) {
-		update_if_changed(MTRR_PHYS_BASE(i), mtrr_base[i]);
-		update_if_changed(MTRR_PHYS_MASK(i), mtrr_mask[i]);
-	}
-
-	for (i = 0; i < ARRAY_SIZE(fixed_mtrr_offsets); ++i)
-		update_if_changed(fixed_mtrr_offsets[i], fixed_mtrrs[i]);
-
-	update_if_changed(MTRR_DEF_TYPE_MSR, mtrr_def);
-
-	tmp_sys_cfg = rdmsr(SYSCFG_MSR);
-
-	/* We only care about the MTRR bits in the SYSCFG register */
-	if ((tmp_sys_cfg.lo & SYS_CFG_MTRR_BITS) != (sys_cfg.lo & SYS_CFG_MTRR_BITS)) {
-		printk(BIOS_INFO, "SYS_CFG was modified: 0x%08x 0x%08x\n", tmp_sys_cfg.hi,
-		       tmp_sys_cfg.lo);
-		tmp_sys_cfg.lo &= ~SYS_CFG_MTRR_BITS;
-		tmp_sys_cfg.lo |= (sys_cfg.lo & SYS_CFG_MTRR_BITS);
-		wrmsr(SYSCFG_MSR, tmp_sys_cfg);
-	}
-}
diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c
index 0bbceec..35d2aa2 100644
--- a/src/soc/amd/picasso/romstage.c
+++ b/src/soc/amd/picasso/romstage.c
@@ -12,7 +12,6 @@
 #include <program_loading.h>
 #include <elog.h>
 #include <soc/romstage.h>
-#include <soc/mtrr.h>
 #include <types.h>
 #include "chip.h"
 #include <fsp/api.h>
@@ -22,16 +21,6 @@
    /* By default, don't do anything */
 }

-/* TODO(b/155426691): Make FSP AGESA leave MTRRs alone */
-static void clear_agesa_mtrrs(void)
-{
-	disable_cache();
-
-	picasso_restore_mtrrs();
-
-	enable_cache();
-}
-
 void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version)
 {
    FSP_M_CONFIG *mcfg = &mupd->FspmConfig;
@@ -95,15 +84,9 @@
    printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

post_code(0x43);
-	picasso_save_mtrrs();
-
-	post_code(0x44);
    fsp_memory_init(s3_resume);

-	post_code(0x45);
-	clear_agesa_mtrrs();
-
-	post_code(0x46);
+	post_code(0x44);
    run_ramstage();

post_code(0x50); /* Should never see this post code. */