Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
soc/amd/picasso: Add basic MTRR settings to hybrid romstage
Program the system flash as WP to allow it to be cacheable. This was done during bootblock on previous systems. Since this code and its data are in DRAM, set them to WP. Prior to running ramstage, set its anticipated target location as WP also.
Change-Id: I73222097a6a859827c4f91851ab55300cb0d9dcc Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/picasso/include/soc/cpu.h M src/soc/amd/picasso/romstage.c 2 files changed, 75 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/35297/1
diff --git a/src/soc/amd/picasso/include/soc/cpu.h b/src/soc/amd/picasso/include/soc/cpu.h index 66e6650..15f319f 100644 --- a/src/soc/amd/picasso/include/soc/cpu.h +++ b/src/soc/amd/picasso/include/soc/cpu.h @@ -25,8 +25,18 @@
#define ROMSTAGE_SIZE (64 * KiB) /* Enforced by Makefile.inc for PSP info */ #define ROMSTAGE_TOP (CONFIG_ROMSTAGE_ADDR + ROMSTAGE_SIZE) +#define EARLYRAM_TOP (ROMSTAGE_TOP + CONFIG_EARLYRAM_SIZE) #if ROMSTAGE_TOP & 0xffff # error Top of the BIOS binary image must be 64KB aligned #endif
+#define EARLY_DRAM_MTRR_BASE \ + ALIGN_DOWN(CONFIG_ROMSTAGE_ADDR, 32 * MiB) +#define EARLY_DRAM_MTRR_SIZE \ + ALIGN_UP(ROMSTAGE_SIZE + CONFIG_EARLYRAM_SIZE, 32 * MiB) +#define EARLY_DRAM_MTRR_TOP \ + (EARLY_DRAM_MTRR_BASE + EARLY_DRAM_MTRR_SIZE) + +#define EARLY_RAMSTAGE_MTRR_SZ (16 * MiB) + #endif /* __PICASSO_CPU_H__ */ diff --git a/src/soc/amd/picasso/romstage.c b/src/soc/amd/picasso/romstage.c index 92361f9..b4c8013 100644 --- a/src/soc/amd/picasso/romstage.c +++ b/src/soc/amd/picasso/romstage.c @@ -21,8 +21,10 @@ #include <delay.h> #include <pc80/mc146818rtc.h> #include <cpu/x86/msr.h> +#include <cpu/x86/mtrr.h> #include <cpu/x86/smm.h> #include <cpu/x86/bist.h> +#include <cpu/amd/mtrr.h> #include <cpu/amd/msr.h> #include <cbmem.h> #include <console/console.h> @@ -56,10 +58,68 @@ fch_early_init(); }
+static int set_early_mtrrs(void) +{ + int mtrr; + + mtrr = get_free_var_mtrr(); + if (mtrr < 0) + return 1; + + set_var_mtrr(mtrr, EARLY_DRAM_MTRR_BASE, EARLY_DRAM_MTRR_SIZE, + MTRR_TYPE_WRBACK); + + mtrr = get_free_var_mtrr(); + if (mtrr < 0) + return 1; + + set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, + MTRR_TYPE_WRPROT); + return 0; +} + +static void set_mtrrs_for_ramstage(void) +{ + uintptr_t smm_base; + size_t smm_size; + uintptr_t smm_top; + uintptr_t mem_top; + uintptr_t ramstage_wb_base; + size_t ramstage_wb_size; + int mtrr; + + smm_region(&smm_base, &smm_size); + smm_top = smm_base + smm_size; + + mem_top = (uintptr_t)cbmem_top(); + + /* Cache anticipated ramstage location through the top of TSEG */ + ramstage_wb_base = mem_top - EARLY_RAMSTAGE_MTRR_SZ; + ramstage_wb_size = smm_top - ramstage_wb_base; + + /* Make sure MTRR base and size are usable */ + if (ramstage_wb_size != 1 << fms(ramstage_wb_size)) { + ramstage_wb_size = 1 << (1 + fms(ramstage_wb_size)); + ramstage_wb_base = smm_top - ramstage_wb_size; + } + if (mem_top - EARLY_RAMSTAGE_MTRR_SZ < EARLY_DRAM_MTRR_TOP) { + printk(BIOS_WARNING, "Warning: Skipping ramstage cacheable due to configuration\n"); + return; + } + + mtrr = get_free_var_mtrr(); + if (mtrr >= 0) + set_var_mtrr(mtrr, ramstage_wb_base, ramstage_wb_size, + MTRR_TYPE_WRBACK); + else + printk(BIOS_WARNING, "Warning: Unable to make ramstage cacheable\n"); +} + asmlinkage void soc_hybrid_romstage_entry(uint32_t bist, uint64_t early_tsc) { uintptr_t top_of_mem; int s3_resume; + int early_mtrr_err;
post_code(0x40); if (CONFIG(COLLECT_TIMESTAMPS)) { @@ -70,6 +130,7 @@ /* Many of these tasks typically happen in bootblock, but execution * begins in romstage for this device. */ post_code(0x41); + early_mtrr_err = set_early_mtrrs();
post_code(0x42); romstage_soc_early_init(); @@ -96,6 +157,9 @@ u32 val = cpuid_eax(1); printk(BIOS_DEBUG, "Family_Model: %08x\n", val);
+ if (early_mtrr_err) + printk(BIOS_WARNING, "Early MTRRs were not set properly\n"); + post_code(0x48); if (!s3_resume && CONFIG(ELOG_BOOT_COUNT)) boot_count_increment(); @@ -111,6 +175,7 @@ printk(BIOS_ERR, "Failed to set romstage handoff data\n");
post_code(0x4b); + set_mtrrs_for_ramstage(); run_ramstage();
post_code(0x50); /* Should never see this post code. */
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35297/1/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/35297/1/src/soc/amd/picasso/romstag... PS1, Line 67: 1 nitpick ... but errors are normally negative values.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35297/1/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/35297/1/src/soc/amd/picasso/romstag... PS1, Line 67: 1
nitpick ... but errors are normally negative values.
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Patch Set 3:
(8 comments)
I think it is worth splitting the romstage cache setup and the ramstage cache setup to speed up the review. About the ramstage caching set up. It might be useful to have a look at CB:35993, which dealt with a somewhat similar issue about finding the right base/size for one MTRR to cover a region. Another path would be to reuse the functions doing the computations for setting up MTRR to set up in postcar stage, it dealt with the logic of having to cache regions quite well and was flexible in the amount of MTRR it used.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 67: EARLY_DRAM_MTRR_BASE Check if aligned to EARLY_DRAM_MTRR_SIZE.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 67: EARLY_DRAM_MTRR_SIZE Check with preprocessor IS_POWER_OF_2() (from commonlib).
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 66: : set_var_mtrr(mtrr, EARLY_DRAM_MTRR_BASE, EARLY_DRAM_MTRR_SIZE, : MTRR_TYPE_WRBACK); : : mtrr = get_free_var_mtrr(); : if (mtrr < 0) : return -1; : : set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, : MTRR_TYPE_WRPROT); Does this all work? What is the state of CR0 and MTRR_DEF_TYPE_MSR at this point?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 79: set_mtrrs_for_ramstage You want to cache a sufficiently large area below cbmem_top and also TSEG above since the stage cache is often put there I suppose?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 94: /* Cache anticipated ramstage location through the top of TSEG */ Don't you want to cache TSEG too? Usually we set up a stage cache there?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 99: amstage_wb_size != 1 << fms(ramstage_wb_size) IS_POWER_OF_2() from commonlib.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 101: smm_top - ramstage_wb_size how do you know this is aligned to ramstage_wb_size?
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 113: Warning: Unable to make ramstage cacheable\n Be a little more specific? "No MTRR available to make ramstage cacheable?"
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... File src/soc/amd/picasso/romstage.c:
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 66: : set_var_mtrr(mtrr, EARLY_DRAM_MTRR_BASE, EARLY_DRAM_MTRR_SIZE, : MTRR_TYPE_WRBACK); : : mtrr = get_free_var_mtrr(); : if (mtrr < 0) : return -1; : : set_var_mtrr(mtrr, FLASH_BASE_ADDR, CONFIG_ROM_SIZE, : MTRR_TYPE_WRPROT);
Does this all work? What is the state of CR0 and MTRR_DEF_TYPE_MSR at this point?
Hmm, not as well as I thought. Re. CR0, I assume you're asking about CD and NW. I cleared those in CB:35035 reset_in_dram_crt0.S line 27. However, the default type was not set, and setting it results it in quick_ram_check_or_die() failing due to cbmem_top returning a goofed value. Will debug tomorrow.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 79: set_mtrrs_for_ramstage
You want to cache a sufficiently large area below cbmem_top and also TSEG above since the stage cach […]
Yes. This replaces the postcar_frame stuff. FWIW in CB:36114, TSEG moves inside cbmem instead of above it.
https://review.coreboot.org/c/coreboot/+/35297/3/src/soc/amd/picasso/romstag... PS3, Line 94: /* Cache anticipated ramstage location through the top of TSEG */
Don't you want to cache TSEG too? Usually we set up a stage cache there?
That's the idea. This was intended to be analogous to what postcar_enable_tseg_cache() would aim to do.
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35297 )
Change subject: soc/amd/picasso: Add basic MTRR settings to hybrid romstage ......................................................................
Abandoned
We're no longer planning to begin in romstage. Solution will only begin with bootblock.
-
Arthur Heymans (Code Review) -
Marshall Dawson (Code Review)
-
Richard Spiegel (Code Review)