Change in coreboot[master]: drivers/intel/fsp2_0: Allow platform to increase mem overhead
Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
drivers/intel/fsp2_0: Allow platform to increase mem overhead
Add a call and a weak function for an additional amount of DRAM to be considered overhead by the SOC.
This change provides a convenient solution for reserving additional memory, e.g. for AMD TSEG, without requiring explicit passing of additional values to FSP via UPD.
TEST=Implement in Picasso and verify FSP recognizes additional TOLUM requirements.
Change-Id: Ia291273b9b7fda0e34be7879473eabd7b53b47b4 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/drivers/intel/fsp2_0/include/fsp/api.h M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 9 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/35383/1
diff --git a/src/drivers/intel/fsp2_0/include/fsp/api.h b/src/drivers/intel/fsp2_0/include/fsp/api.h index b905b69..2c3a566 100644 --- a/src/drivers/intel/fsp2_0/include/fsp/api.h +++ b/src/drivers/intel/fsp2_0/include/fsp/api.h @@ -58,6 +58,9 @@ void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version); void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd);
+/* Allow the platform to increase BootLoaderTolumSize */ +size_t platform_mem_overhead_size(void); + /* * The following functions are used when FSP_PLATFORM_MEMORY_SETTINGS_VERSION * is employed allowing the mainboard and SoC to supply their own version diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 1386d2c..349a188 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -41,6 +41,8 @@ CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), "for TPM MRC hash functionality, vboot must start in bootblock");
+__weak size_t platform_mem_overhead_size(void) { return 0; } + static void save_memory_training_data(bool s3wake, uint32_t fsp_version) { size_t mrc_data_size; @@ -289,8 +291,11 @@
arch_upd = &fspm_upd.FspmArchUpd;
- /* Reserve enough memory under TOLUD to save CBMEM header */ + /* Reserve enough memory under TOLUD to save CBMEM header and any + * additional requirements of the platform implementation. + */ arch_upd->BootLoaderTolumSize = cbmem_overhead_size(); + arch_upd->BootLoaderTolumSize += platform_mem_overhead_size();
/* Fill common settings on behalf of chipset. */ if (fsp_fill_common_arch_params(arch_upd, s3wake, fsp_version,
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG I'm curious. Why couldn't TSEG just be carved out of cbmem if it's not tied to the memory controller init at FSP-M time?
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... PS1, Line 44: __weak size_t platform_mem_overhead_size(void) { return 0; } I'm curious to see your cbmem_top() implementation in dealing with this.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(2 comments)
Let me experiment with moving TSEG etc. into cbmem, then I may abandon this.
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
I'm curious. […]
I don't recall now why I put Stoney's TSEG above cbmem_top() -- could be I was trying to mimic an Intel memory map for the first AMD implementation. The only drawback I see is TSEG_MASK will require a 128K alignment (meh). Assuming we don't botch the settings, it should still be adequately protected WRT accesses, cache contents, etc.
I've also been trying to recall why I decided to reserve Stoney's BERT data above cbmem_top and not allocated in cbmem. I'm pretty sure I'd thought through a scenario of why that was a good idea but it's lost at the moment. It seems benign to put it into cbmem. It's not interesting until well into ramstage.
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... PS1, Line 44: __weak size_t platform_mem_overhead_size(void) { return 0; }
I'm curious to see your cbmem_top() implementation in dealing with this.
It doesn't affect cbmem_top(): https://review.coreboot.org/c/coreboot/+/34423/12/src/soc/amd/picasso/memmap... I wasn't able to come up with a method for reading the actual top of usable memory out of hardware, like on Intel systems. Although we have the TOM MSR, when UMA is < 4GB TOM doesn't move. So if we rely on TOM alone, we might R/W UMA instead of system memory. (There must surely be some way to detect it, but I don't think I have access to that spec.) So the only way I have for really discerning the right value for cbmem_top() is by parsing AGESA's HOBs. As much as I dislike this, it's not that different from how the old AGESA provided the information.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Patch Set 1:
(2 comments)
Let me experiment with moving TSEG etc. into cbmem, then I may abandon this.
Got it. I wanted to better understand your thinking/constraints.
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
I don't recall now why I put Stoney's TSEG above cbmem_top() -- could be I was trying to mimic an In […]
I was wondering if there were alignment requirements. If so, then this route may indeed be necessary. Depending on how the caching (MTRRs) and protections interact it may be worth not interleaving the regions then.
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... PS1, Line 44: __weak size_t platform_mem_overhead_size(void) { return 0; }
It doesn't affect cbmem_top(): https://review.coreboot. […]
I see that you are carving out the BERT and TSEG region from the upper end of memory below 4GiB. Presumably platform_mem_overhead_size() returns CONFIG_SMM_TSEG_SIZE + BERT_REGION_MAX_SIZE ?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
I was wondering if there were alignment requirements. […]
TSEG has its own typing, independent of the MTRRs, so I don't believe that should be an issue. And SMM typing gets first crack; the MTRRs don't need to be programmed around it. As far as the alignment is concerned, it just means we burn a little DRAM. But in ST we ALIGN_DOWN from TSEG/BERT 8MB to calculate cbmem_top. I think the math typically works out that we don't burn any memory there, however we were comfortable with up to 8MB (more actually, but let's not get too far into the weeds). If we allocate in cbmem instead, then the worst case scenario seems much better.
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35383/1/src/drivers/intel/fsp2_0/me... PS1, Line 44: __weak size_t platform_mem_overhead_size(void) { return 0; }
I see that you are carving out the BERT and TSEG region from the upper end of memory below 4GiB. […]
Yep, that's what I was doing as a quick experiment.
Marshall Dawson has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Abandoned
pursuing a different direction.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
TSEG has its own typing, independent of the MTRRs, so I don't believe that should be an issue. […]
Thanks for the thought on stuffing TSEG into cbmem. I'm happy with the way that's behaving and it helps simplify things. I'm going to let this patch go.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
Thanks for the thought on stuffing TSEG into cbmem. […]
Just a reminder; cbmem_top_init_once() calls quick_ram_check() which requires rw-access to u32 immediate below cbmem_top(). That might fail if TSEG is locked (after soft reset or on S3 resume), I remember seeing clear_tvalid() implementation in picasso/memmap.c.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35383 )
Change subject: drivers/intel/fsp2_0: Allow platform to increase mem overhead ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35383/1//COMMIT_MSG@13 PS1, Line 13: AMD TSEG
Just a reminder; cbmem_top_init_once() calls quick_ram_check() which requires rw-access to u32 immed […]
Thanks for the reminder. If we propagate this to stoney, that'll need to be considered. For any FSP2 system, however, the FSP Reserved Memory is the first entry.
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)