Change in coreboot[master]: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
The documentation for StackBase and StackSize in FSPM_ARCH_UPD is confusing. Previously the region was shared for heap and stack, starting with FSP2.1 only for heap (or 'temporary RAM') for HOBs.
Moving the allocation outside DCACHE_BSP_STACK_SIZE allows use of stack guards and reduces amount of reserved CAR for bootblock and verstage, as the new allocation in .bss is only taken in romstage.
Change-Id: I4cffcc73a89cb97ab7759dd373196ce9753a6307 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp2_0/memory_init.c M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 3 files changed, 15 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/35233/1
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 0797c2e..dbaecdb 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -34,6 +34,8 @@ #include <fsp/memory_init.h> #include <types.h>
+static char temp_ram[0x20000]; + /* TPM MRC hash functionality depends on vboot starting before memory init. */ _Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), @@ -161,6 +163,7 @@
return CB_SUCCESS; } + static enum cb_err setup_fsp_stack_frame(FSPM_ARCH_UPD *arch_upd, const struct memranges *memmap) { @@ -168,17 +171,6 @@ uintptr_t stack_end;
/* - * FSP 2.1 version would use same stack as coreboot instead of - * setting up seprate stack frame. FSP 2.1 would not relocate stack - * top and does not reinitialize stack pointer. - */ - if (CONFIG(FSP_USES_CB_STACK)) { - arch_upd->StackBase = (void *)_car_stack_start; - arch_upd->StackSize = CONFIG_DCACHE_BSP_STACK_SIZE; - return CB_SUCCESS; - } - - /* * FSPM_UPD passed here is populated with default values * provided by the blob itself. We let FSPM use top of CAR * region of the size it requests. @@ -197,8 +189,19 @@ bool s3wake, uint32_t fsp_version, const struct memranges *memmap) { - if (setup_fsp_stack_frame(arch_upd, memmap)) + /* + * FSP 2.1 version would use same stack as coreboot instead of + * setting up separate stack frame. FSP 2.1 would not relocate stack + * top and does not reinitialize stack pointer. The parameters passed + * as StackBase and StackSize are actually for temporary RAM and HOBs + * and are not related to FSP stack at all. + */ + if (CONFIG(FSP_USES_CB_STACK)) { + arch_upd->StackBase = temp_ram; + arch_upd->StackSize = sizeof(temp_ram); + } else if (setup_fsp_stack_frame(arch_upd, memmap)) { return CB_ERR; + }
fsp_fill_mrc_cache(arch_upd, fsp_version);
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index d949fff..2c5a884 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -119,7 +119,6 @@
config DCACHE_BSP_STACK_SIZE hex - default 0x20000 if FSP_USES_CB_STACK default 0x4000 help The amount of anticipated stack usage in CAR by bootblock and diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 1bd478c..399a59c 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -70,7 +70,6 @@
config DCACHE_BSP_STACK_SIZE hex - default 0x20000 if FSP_USES_CB_STACK default 0x4000 help The amount of anticipated stack usage in CAR by bootblock and
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
Please make sure you test this on either Comet Lake or Ice Lake. Subrata should be able to help with that.
FSP SiliconInit() will go find and use HOBs that were previously produced during MemoryInit(), but at the very end of the MemoryInit() we migrate the FSP code and data into permanent memory anyway, so I believe this will work.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... PS1, Line 37: 0x20000 I think it would be best to have a config option that can be provided by SoC to determine the size of the temp ram. It should ideally match integration guide for that platform.
https://review.coreboot.org/c/coreboot/+/35233/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/1/src/soc/intel/cannonlake/Kc... PS1, Line 122: 0x20000 I believe we would still need a larger stack with FSP_USES_CB_STACK. I have posted a question on CB:35165 to understand the size requirement for stack.
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... PS1, Line 37: 0x20000 Agreed with Furquan. We shouldn't hard-code this to 128KB. For example the server version of Copper/Ice Lake has more channels of DDR memory so it definitely will use more than the 128KB that Comet/Ice Lake mobile use.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
It causes hang inside FSP (during FSP-M init) in both CNL/ICL where earlier DACHE_BSP_STACK_SIZE was 0x20000 (~128KB)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
It causes hang inside FSP (during FSP-M init) in both CNL/ICL where earlier DACHE_BSP_STACK_SIZE was 0x20000 (~128KB)
Not surprised. Like Furquan commented, we probably still need some increase for DCACHE_BSP_STACK_SIZE but did not find what the correct value would be. Also it has not been documented if (the ill-named) StackBase has to be aligned to StackSize in UPD.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
It causes hang inside FSP (during FSP-M init) in both CNL/ICL where earlier DACHE_BSP_STACK_SIZE was 0x20000 (~128KB)
Not surprised. Like Furquan commented, we probably still need some increase for DCACHE_BSP_STACK_SIZE but did not find what the correct value would be. Also it has not been documented if (the ill-named) StackBase has to be aligned to StackSize in UPD.
DCACHE_BSP_STACK_SIZE size ~128KB should be enough for FSP to run.
Hello Patrick Rudolph, Nathaniel L Desimone, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35233
to look at the new patch set (#2).
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
The documentation for StackBase and StackSize in FSPM_ARCH_UPD is confusing. Previously the region was shared for heap and stack, starting with FSP2.1 only for heap (or 'temporary RAM') for HOBs.
Moving the allocation outside DCACHE_BSP_STACK_SIZE allows use of stack guards and reduces amount of reserved CAR for bootblock and verstage, as the new allocation in .bss is only taken in romstage.
Change-Id: I4cffcc73a89cb97ab7759dd373196ce9753a6307 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 4 files changed, 27 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/35233/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
It causes hang inside FSP (during FSP-M init) in both CNL/ICL where earlier DACHE_BSP_STACK_SIZE was 0x20000 (~128KB)
Not surprised. Like Furquan commented, we probably still need some increase for DCACHE_BSP_STACK_SIZE but did not find what the correct value would be. Also it has not been documented if (the ill-named) StackBase has to be aligned to StackSize in UPD.
DCACHE_BSP_STACK_SIZE size ~128KB should be enough for FSP to run.
Patch set 2 again stuck inside FSP-M
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Subrata, can you take over this one and have the allocations match with the specs like Furguan suggests?
It causes hang inside FSP (during FSP-M init) in both CNL/ICL where earlier DACHE_BSP_STACK_SIZE was 0x20000 (~128KB)
Not surprised. Like Furquan commented, we probably still need some increase for DCACHE_BSP_STACK_SIZE but did not find what the correct value would be. Also it has not been documented if (the ill-named) StackBase has to be aligned to StackSize in UPD.
DCACHE_BSP_STACK_SIZE size ~128KB should be enough for FSP to run.
Patch set 2 again stuck inside FSP-M
Once HEAP is moved away from this allocation, we should be allowed to reduce this. By how much, Intel has not said.
Hello Patrick Rudolph, Nathaniel L Desimone, Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35233
to look at the new patch set (#3).
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
The documentation for StackBase and StackSize in FSPM_ARCH_UPD is confusing. Previously the region was shared for heap and stack, starting with FSP2.1 only for heap (or 'temporary RAM') for HOBs.
Moving the allocation outside DCACHE_BSP_STACK_SIZE allows use of stack guards and reduces amount of reserved CAR for bootblock and verstage, as the new allocation in .bss is only taken in romstage.
Change-Id: I4cffcc73a89cb97ab7759dd373196ce9753a6307 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/35233/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Someone should run w/ this on Google's side to see if it alleviates the current issues. I suspect it will, but we should confirm.
https://review.coreboot.org/c/coreboot/+/35233/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35233/3//COMMIT_MSG@16 PS3, Line 16: Could you please add the following?
BUG=b:140268415
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... PS3, Line 157: 0x10000 Did we establish that this size is valid for all platforms using 2.1? i.e., should this go in here or in the SoC Kconfig?
Hello Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Subrata Banik, Tim Wawrzynczak, Shelley Chen, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35233
to look at the new patch set (#4).
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
The documentation for StackBase and StackSize in FSPM_ARCH_UPD is confusing. Previously the region was shared for heap and stack, starting with FSP2.1 only for heap (or 'temporary RAM') for HOBs.
Moving the allocation outside DCACHE_BSP_STACK_SIZE allows use of stack guards and reduces amount of reserved CAR for bootblock and verstage, as the new allocation in .bss is only taken in romstage.
BUG=b:140268415
Change-Id: I4cffcc73a89cb97ab7759dd373196ce9753a6307 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 20 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/35233/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 4:
Patch Set 3: Code-Review+2
(1 comment)
Someone should run w/ this on Google's side to see if it alleviates the current issues. I suspect it will, but we should confirm.
Confirmed that cherry-picking this into ToT still boots Helios.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35233/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35233/3//COMMIT_MSG@16 PS3, Line 16:
Could you please add the following? […]
Done
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... PS3, Line 157: 0x10000
Did we establish that this size is valid for all platforms using 2.1? i.e. […]
Cleanest would be to explicitly have only SoC set the value. I would leave it to Intel to move this and sync it with their integration guides once they reach agreement:
CB:35233 Sep 03 07:36 Nathaniel comments 0x20000 for Cometlake, more for Copperlake.
CB:35165 Sep 03 09:05 Subrata comments 0x11000 for Cometlake, but decides to use 0x10000 instead.
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... PS1, Line 37: 0x20000
I think it would be best to have a config option that can be provided by SoC to determine the size o […]
Done
https://review.coreboot.org/c/coreboot/+/35233/1/src/drivers/intel/fsp2_0/me... PS1, Line 37: 0x20000
Agreed with Furquan. We shouldn't hard-code this to 128KB. […]
Done
https://review.coreboot.org/c/coreboot/+/35233/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/1/src/soc/intel/cannonlake/Kc... PS1, Line 122: 0x20000
I believe we would still need a larger stack with FSP_USES_CB_STACK. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35233/4/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/4/src/drivers/intel/fsp2_0/Kc... PS4, Line 158: depends on FSP_USES_CB_STACK It would be good to have a help text explaining what this is used for. I am okay if Intel wants to do this in follow-up CL when adding individual sizes to each SoC.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 4:
Submit at your own discretion. I have no resources to improve this further wrt. allocation sizes or terminology.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 4:
Patch Set 4:
Submit at your own discretion. I have no resources to improve this further wrt. allocation sizes or terminology.
I have to repeat what I wrote in CB:35165, I think in general FSP_USES_CB_STACK was a step to wrong direction wrt. amount of CAR space that is reserved. I did not look for argumentation or motivation for the change.
More importantly; the way car.ld is laid out requires DCACHE_BSP_STACK_SIZE parameter to stay constant across bootblock, verstage and romstage. Updates to FSP-M will not be allowed to have increased requirement for the stack size on systems with read-only bootblock.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 6:
More importantly; the way car.ld is laid out requires DCACHE_BSP_STACK_SIZE parameter to stay constant across bootblock, verstage and romstage. Updates to FSP-M will not be allowed to have increased requirement for the stack size on systems with read-only bootblock.
Yes, your analysis is right. It was the case even before FSP_USES_CB_STACK was enabled i.e. stack remaining the same across bootblock, verstage and romstage. But, it wasn't a problem because the stack was used only by coreboot and hence more predictable usage. However, I am sure we had other problems with FSP changing the stack pointer and using its own separate stack.
Yes, updates to FSP-M will not be allowed with read-only bootblock. But couple of things: a. Before the platform gets to a stable phase, I am hoping that the FSP changes stabilize and so the stack usage is more predictable (or well documented). b. Also, the current requirements seem to be for FSP with debug enabled. So, for in production systems, there should still be enough space for growth.
It might make sense to have a debug option to check how close FSP is to using up the stack, so that the resource allocation can be better handled by coreboot.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 6:
Patch Set 4:
Submit at your own discretion. I have no resources to improve this further wrt. allocation sizes or terminology.
I am going ahead and pushing this change in for now. Haven't seen any one else complain here.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/3/src/drivers/intel/fsp2_0/Kc... PS3, Line 157: 0x10000
Cleanest would be to explicitly have only SoC set the value. […]
Done
https://review.coreboot.org/c/coreboot/+/35233/4/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35233/4/src/drivers/intel/fsp2_0/Kc... PS4, Line 158: depends on FSP_USES_CB_STACK
It would be good to have a help text explaining what this is used for. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 6:
I do not understand why this change was rebased on top of CB:34882. I don't think it is really required before this change goes in.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
Patch Set 6:
Patch Set 6:
I do not understand why this change was rebased on top of CB:34882. I don't think it is really required before this change goes in.
I made some comments in CB:34882 already, no dependency here.
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK
The documentation for StackBase and StackSize in FSPM_ARCH_UPD is confusing. Previously the region was shared for heap and stack, starting with FSP2.1 only for heap (or 'temporary RAM') for HOBs.
Moving the allocation outside DCACHE_BSP_STACK_SIZE allows use of stack guards and reduces amount of reserved CAR for bootblock and verstage, as the new allocation in .bss is only taken in romstage.
BUG=b:140268415
Change-Id: I4cffcc73a89cb97ab7759dd373196ce9753a6307 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35233 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/fsp2_0/Kconfig M src/drivers/intel/fsp2_0/memory_init.c 2 files changed, 20 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig index 3fb39d5..541ec47 100644 --- a/src/drivers/intel/fsp2_0/Kconfig +++ b/src/drivers/intel/fsp2_0/Kconfig @@ -152,6 +152,11 @@ without reinitializing stack pointer. This feature is supported Icelake onwards.
+config FSP_TEMP_RAM_SIZE + hex + default 0x10000 + depends on FSP_USES_CB_STACK + config VERIFY_HOBS bool "Verify the FSP hand-off-blocks" default n diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 0797c2e..9789c96 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -34,6 +34,8 @@ #include <fsp/memory_init.h> #include <types.h>
+static uint8_t temp_ram[CONFIG_FSP_TEMP_RAM_SIZE] __aligned(sizeof(uint64_t)); + /* TPM MRC hash functionality depends on vboot starting before memory init. */ _Static_assert(!CONFIG(FSP2_0_USES_TPM_MRC_HASH) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK), @@ -161,6 +163,7 @@
return CB_SUCCESS; } + static enum cb_err setup_fsp_stack_frame(FSPM_ARCH_UPD *arch_upd, const struct memranges *memmap) { @@ -168,17 +171,6 @@ uintptr_t stack_end;
/* - * FSP 2.1 version would use same stack as coreboot instead of - * setting up seprate stack frame. FSP 2.1 would not relocate stack - * top and does not reinitialize stack pointer. - */ - if (CONFIG(FSP_USES_CB_STACK)) { - arch_upd->StackBase = (void *)_car_stack_start; - arch_upd->StackSize = CONFIG_DCACHE_BSP_STACK_SIZE; - return CB_SUCCESS; - } - - /* * FSPM_UPD passed here is populated with default values * provided by the blob itself. We let FSPM use top of CAR * region of the size it requests. @@ -197,8 +189,19 @@ bool s3wake, uint32_t fsp_version, const struct memranges *memmap) { - if (setup_fsp_stack_frame(arch_upd, memmap)) + /* + * FSP 2.1 version would use same stack as coreboot instead of + * setting up separate stack frame. FSP 2.1 would not relocate stack + * top and does not reinitialize stack pointer. The parameters passed + * as StackBase and StackSize are actually for temporary RAM and HOBs + * and are not related to FSP stack at all. + */ + if (CONFIG(FSP_USES_CB_STACK)) { + arch_upd->StackBase = temp_ram; + arch_upd->StackSize = sizeof(temp_ram); + } else if (setup_fsp_stack_frame(arch_upd, memmap)) { return CB_ERR; + }
fsp_fill_mrc_cache(arch_upd, fsp_version);
Lean Sheng Tan has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/35233 )
Change subject: intel/fsp2_0: Move temporary RAM to .bss with FSP_USES_CB_STACK ......................................................................
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Lean Sheng Tan (Code Review)
-
Nathaniel L Desimone (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)