Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33775
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
soc/intel/apollolake/romstage: Increase size of postcar stack
If you currently activate the measured boot on an Apollo Lake mainboard, you will run into a stack overflow during postcar. Such a behavior has already been observed on the Sky Lake platform and a corresponding patch has been made for this (https://review.coreboot.org/c/coreboot/+/33434). This issue occurs since the patch for the correct timestamp value in postcar comes up (https://review.coreboot.org/c/coreboot/+/32726 and https://review.coreboot.org/c/coreboot/+/32881). By increasing the stack size for postcar the issue is solved.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/soc/intel/apollolake/romstage.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/33775/1
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 5bf501d..da8e581 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -240,7 +240,7 @@ else printk(BIOS_ERR, "Failed to determine variable data\n");
- if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 2*KiB)) die("Unable to initialize postcar frame.\n");
mainboard_save_dimm_info();
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1: Code-Review-1
-1 because ppl tend to merge to early.
Comments of previous commit still apply. Please change all Intel platforms at once that use 1 KiB of stack as they are likely affected as well.
I'm fine with either values, but maybe it's time to use a common define instead of using a different value on each platform. In the end the postcar stage is the very same.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
I would increase all platforms currently use 1KiB to 4KiB, as suggested by Patrick.
src/soc/intel/apollolake/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/icelake/romstage/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/cannonlake/romstage/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/quark/romstage/fsp2_0.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/denverton_ns/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/amd/stoneyridge/romstage.c -> postcar_frame_init(&pcf, 1*KiB)
For this I would use the Define ‘ROMSTAGE_COMMON_RAM_STACK_SIZE’, declared in src/include/memlayout.h. This could then be used as the default setting for the postcar stack size.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
Hey Kyösti. What do you think about this chage for stoneyridge?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
Patch Set 1:
I would increase all platforms currently use 1KiB to 4KiB, as suggested by Patrick.
src/soc/intel/apollolake/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/icelake/romstage/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/cannonlake/romstage/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/quark/romstage/fsp2_0.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/intel/denverton_ns/romstage.c -> postcar_frame_init(&pcf, 1*KiB) src/soc/amd/stoneyridge/romstage.c -> postcar_frame_init(&pcf, 1*KiB)
For this I would use the Define ‘ROMSTAGE_COMMON_RAM_STACK_SIZE’, declared in src/include/memlayout.h. This could then be used as the default setting for the postcar stack size.
Sound good to me. Please add a comment as well to make sure that it won't be accidentally decreased again.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/intel/apollolake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
Mario, your plan sounds good to me.
Hello Kyösti Mälkki, Werner Zeh, Patrick Rudolph, build bot (Jenkins), Aaron Durbin, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33775
to look at the new patch set (#2).
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
soc/{amd, intel}: Increase size of postcar stack
If you currently activate the measured boot on an Apollo Lake mainboard, you will run into a stack overflow during postcar. Such a behavior has already been observed on the Sky Lake platform and a corresponding patch has been made for this (https://review.coreboot.org/c/coreboot/+/33434). This issue occurs since the patch for the correct timestamp value in postcar comes up (https://review.coreboot.org/c/coreboot/+/32726 and https://review.coreboot.org/c/coreboot/+/32881). By increasing the stack size for postcar the issue is solved.
This patch adds a general define for it and sets the value to 4KiB. All platforms that have used less than this 4KiB so far will be adjusted to the new value. This value should be the lowest limit for the postcar stack size.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com --- M src/include/memlayout.h M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/quark/romstage/fsp2_0.c 7 files changed, 25 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/33775/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2: Code-Review+1
Tested on UP² (Intel Apollolake).
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG@16 PS2, Line 16: size for postcar the issue is solved. Please use commit hashes and the summaries for references (or Change-Id) and not the URLs.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2: Code-Review-1
(5 comments)
I would rather see CB:33434 reverted first if these are really about the same issue. IMHO that was merged too early with insufficient details and poor argumentation.
I guess hashes just easily eat excess of 512 bytes of stack and the original 1 KiB was borderline sufficient with VBOOT in postcar? Do we want to use MAYBE_STATIC under security/vboot instead and move those hashes to .bss?
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG@9 PS2, Line 9: If you currently activate the measured boot on an Apollo Lake mainboard, Replace 'currently' with the commit regression first appears with (CB:32901 ?).
Can you give a set of Kconfig variables "activate measured boot" implies? Is this 'VBOOT=y, VBOOT_MEASURED_BOOT=y'. BTW, the latter is documented 'experimental'.
It sounds like to mention the platform is entirely irrelevant here.
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG@16 PS2, Line 16: size for postcar the issue is solved. The already merged one references a different culprit (CB:32901) while this one refers to (CB:32726 and CB:32881). Or maybe there is language barrier here, I don't understand the connection to timestamps.
And all of these are really irrelevant if this about attempting to squeeze excess of 512 bytes of hashes on 1 KiB of stack...
https://review.coreboot.org/#/c/33775/2//COMMIT_MSG@21 PS2, Line 21: stack size. Previously it was argumented CB:32901 increased the stack usage in postcar. It would have been nice to know a number and if there is some bad call pattern with VBOOT_MEASURED_BOOT=y that should be fixed instead.
Nevertheless, I am fine with 4 KiB default here if applied as a #define with comment like you have done in this commit.
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@45 PS2, Line 45: #ifndef POSTCAR_COMMON_RAM_STACK_SIZE Remove the guard, if you need customizeable preprocessor variable, use Kconfigs.
And arch/cpu.h might be better place, next to postcar_frame_init() prototype.
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@46 PS2, Line 46: #define POSTCAR_COMMON_RAM_STACK_SIZE 0x1000 Well.. this ends up allocated as CBMEM_ID_ROMSTAGE_RAM_STACK, but I am fine with keeping the CBMEM ID name as-is.
From what I know, all the cases of ROMSTAGE_RAM_STACK_SIZE used with postcar_frame_init() can be replaced with this new POSTCAR_COMMON_RAM_STACK_SIZE.
The only case where 0x5000 remains required is POSTCAR_STAGE=n, lib/romstage_stack.c most likely.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@45 PS2, Line 45: #ifndef POSTCAR_COMMON_RAM_STACK_SIZE
Remove the guard, if you need customizeable preprocessor variable, use Kconfigs. […]
Seems like a good place. But we could also flip this around. Pass in 0 for stack size (still allowing someone to override the default). Then in postcar_frame_init() do this:
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 732b767bf6..3deb99bcb0 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -48,6 +48,9 @@ int postcar_frame_init(struct postcar_frame *pcf, size_t stack_size) { void *stack;
+ if (stack_size == 0) + stack_size = 4*KiB; + stack = cbmem_add(CBMEM_ID_ROMSTAGE_RAM_STACK, stack_size); if (stack == NULL) { printk(BIOS_ERR, "Couldn't add %zd byte stack in cbmem.\n",
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
(1 comment)
LetCB:33775
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@45 PS2, Line 45: #ifndef POSTCAR_COMMON_RAM_STACK_SIZE
Seems like a good place. But we could also flip this around. […]
The original argumentation when this stack was reduced to 1 KiB was about LZMA scrathpad having being moved to .bss in case POSTCAR_STAGE=y. Thus my question, do we want to use MAYBE_STATIC in similar fashion for VBOOT and not increase this stack size at all.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@45 PS2, Line 45: #ifndef POSTCAR_COMMON_RAM_STACK_SIZE
The original argumentation when this stack was reduced to 1 KiB was about LZMA scrathpad having bein […]
What specific place would we add the decoration? Is that in vboot itself? Eating 4KiB might be the easier and more straight forward approach.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
Mario, I can update this if you did not already start addressing the comments.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
Patch Set 2:
Mario, I can update this if you did not already start addressing the comments.
Hi Kyösti, if you have a better solution for this, please feel free to update it. We only wanted to solve our issue, which occurred with the described patches. Thank you all for reviewing.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: soc/{amd, intel}: Increase size of postcar stack ......................................................................
Patch Set 2:
As we don't know if stack overflowed at build time and we don't have checks or guards at runtime, it would be nice to have both. A big stack and motivation to reduce overall stack usage in coreboot.
Kyösti Mälkki has uploaded a new patch set (#3) to the change originally created by Mario Scheithauer. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
arch/x86: Adjust size of postcar stack
With VBOOT=y && VBOOT_MEASURED_BOOT=y message digest will be allocated from the stack and 1 KiB reserve used with the recent platforms was no longer sufficient.
The comment of LZMA scratchpad consuming stack was obsolete for postcar, so these can be reduced to same 4 KiB.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/cpu.h M src/cpu/intel/haswell/romstage.c M src/drivers/intel/fsp1_1/car.c M src/mainboard/emulation/qemu-i440fx/romstage.c M src/mainboard/emulation/qemu-q35/romstage.c M src/northbridge/intel/e7505/memmap.c M src/northbridge/intel/gm45/ram_calc.c M src/northbridge/intel/i440bx/ram_calc.c M src/northbridge/intel/i945/ram_calc.c M src/northbridge/intel/nehalem/ram_calc.c M src/northbridge/intel/pineview/ram_calc.c M src/northbridge/intel/sandybridge/ram_calc.c M src/northbridge/intel/x4x/ram_calc.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/romstage.c M src/soc/intel/broadwell/romstage/romstage.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/quark/romstage/fsp2_0.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 22 files changed, 29 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/33775/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33775/3/src/arch/x86/include/arch/cpu.h File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/#/c/33775/3/src/arch/x86/include/arch/cpu.h@321 PS3, Line 321: #define POSTCAR_COMMON_RAM_STACK_SIZE 4*KiB Macros with complex values should be enclosed in parentheses
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33775/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33775/3//COMMIT_MSG@12 PS3, Line 12: was no longer sufficient. I did not entirely understand the original argumentation and or references to commits with regression. What do we need to say here to make it comprehensible?
https://review.coreboot.org/#/c/33775/3/src/drivers/intel/fsp1_1/car.c File src/drivers/intel/fsp1_1/car.c:
https://review.coreboot.org/#/c/33775/3/src/drivers/intel/fsp1_1/car.c@74 PS3, Line 74: aligned_ram = ALIGN_DOWN(romstage_ram_stack_bottom(), alignment); This needs some fix, but I did not understand the purpose of this.
Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 3:
I have tested the latest changes with vboot enabled on siemens/mc_apl5 mainboard and it looks good so far. With the last change, all platforms will run with the same setting (4KiB), except src/drivers/amd/agesa/romstage.c.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/33775/2/src/include/memlayout.h@45 PS2, Line 45: #ifndef POSTCAR_COMMON_RAM_STACK_SIZE
What specific place would we add the decoration? Is that in vboot itself? Eating 4KiB might be the e […]
What do people think about the implementation I proposed above?
Kyösti Mälkki has uploaded a new patch set (#4) to the change originally created by Mario Scheithauer. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
arch/x86: Adjust size of postcar stack
With VBOOT=y && VBOOT_MEASURED_BOOT=y message digest will be allocated from the stack and 1 KiB reserve used with the recent platforms was no longer sufficient.
The comment of LZMA scratchpad consuming stack was obsolete for postcar, so these can be reduced to same 4 KiB.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c M src/cpu/intel/haswell/romstage.c M src/drivers/intel/fsp1_1/car.c M src/mainboard/emulation/qemu-i440fx/romstage.c M src/mainboard/emulation/qemu-q35/romstage.c M src/northbridge/intel/e7505/memmap.c M src/northbridge/intel/gm45/ram_calc.c M src/northbridge/intel/i440bx/ram_calc.c M src/northbridge/intel/i945/ram_calc.c M src/northbridge/intel/nehalem/ram_calc.c M src/northbridge/intel/pineview/ram_calc.c M src/northbridge/intel/sandybridge/ram_calc.c M src/northbridge/intel/x4x/ram_calc.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/romstage.c M src/soc/intel/broadwell/romstage/romstage.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/quark/romstage/fsp2_0.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 23 files changed, 35 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/33775/4
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/romstage.c:
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-i440fx/... PS4, Line 17: #include <arch/cpu.h> You do not need this include anymore since you have removed the define for the default stack size in this patch set. If you have kept it for the prototype (in order to avoid nested includes) than you should add it for all the other files which use postcar_frame_init() and miss this include to be consisten:
src/northbridge/intel/i440bx/ram_calc.c src/northbridge/intel/pineview/ram_calc.c src/soc/intel/broadwell/romstage/romstage.c src/soc/intel/cannonlake/romstage/romstage.c src/soc/intel/denverton_ns/romstage.c src/soc/intel/icelake/romstage/romstage.c src/soc/intel/quark/romstage/fsp2_0.c
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-q35/rom... File src/mainboard/emulation/qemu-q35/romstage.c:
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-q35/rom... PS4, Line 17: #include <arch/cpu.h> Same here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-i440fx/... File src/mainboard/emulation/qemu-i440fx/romstage.c:
https://review.coreboot.org/#/c/33775/4/src/mainboard/emulation/qemu-i440fx/... PS4, Line 17: #include <arch/cpu.h>
You do not need this include anymore since you have removed the define for the default stack size in […]
Right, I did not spot the missing header elsewhere, I will update once there is review for the parent (FSP1_1) commit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 4: Code-Review+2
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 4:
So we really want to use 0 as a default value? Wouldn't it makes more sense, if we use a define like DEFAULT_STACK_SIZE so it's more readable?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 4:
Patch Set 4:
So we really want to use 0 as a default value? Wouldn't it makes more sense, if we use a define like DEFAULT_STACK_SIZE so it's more readable?
Given that postcar should remain simple and not call BLOBs, I would be willing to remove the stack_size parameter (now 0) from the call entirely, once I figure out what to do with AGESA/binaryPI here.
Kyösti Mälkki has uploaded a new patch set (#5) to the change originally created by Mario Scheithauer. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
arch/x86: Adjust size of postcar stack
With VBOOT=y && VBOOT_MEASURED_BOOT=y message digest will be allocated from the stack and 1 KiB reserve used with the recent platforms was no longer sufficient.
The comment of LZMA scratchpad consuming stack was obsolete for postcar, so these can be reduced to same 4 KiB.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c M src/cpu/intel/haswell/romstage.c M src/drivers/intel/fsp1_1/car.c M src/mainboard/emulation/qemu-i440fx/romstage.c M src/mainboard/emulation/qemu-q35/romstage.c M src/northbridge/intel/e7505/memmap.c M src/northbridge/intel/gm45/ram_calc.c M src/northbridge/intel/i440bx/ram_calc.c M src/northbridge/intel/i945/ram_calc.c M src/northbridge/intel/nehalem/ram_calc.c M src/northbridge/intel/pineview/ram_calc.c M src/northbridge/intel/sandybridge/ram_calc.c M src/northbridge/intel/x4x/ram_calc.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/romstage.c M src/soc/intel/broadwell/romstage/romstage.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/quark/romstage/fsp2_0.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 23 files changed, 42 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/33775/5
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 5: Code-Review+2
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
Patch Set 5: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33775 )
Change subject: arch/x86: Adjust size of postcar stack ......................................................................
arch/x86: Adjust size of postcar stack
With VBOOT=y && VBOOT_MEASURED_BOOT=y message digest will be allocated from the stack and 1 KiB reserve used with the recent platforms was no longer sufficient.
The comment of LZMA scratchpad consuming stack was obsolete for postcar, so these can be reduced to same 4 KiB.
Change-Id: Iba1fb5bfad6946f316feac2d8c998a782142a56a Signed-off-by: Mario Scheithauer mario.scheithauer@siemens.com Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33775 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Werner Zeh werner.zeh@siemens.com --- M src/arch/x86/include/arch/cpu.h M src/arch/x86/postcar_loader.c M src/cpu/intel/haswell/romstage.c M src/drivers/intel/fsp1_1/car.c M src/mainboard/emulation/qemu-i440fx/romstage.c M src/mainboard/emulation/qemu-q35/romstage.c M src/northbridge/intel/e7505/memmap.c M src/northbridge/intel/gm45/ram_calc.c M src/northbridge/intel/i440bx/ram_calc.c M src/northbridge/intel/i945/ram_calc.c M src/northbridge/intel/nehalem/ram_calc.c M src/northbridge/intel/pineview/ram_calc.c M src/northbridge/intel/sandybridge/ram_calc.c M src/northbridge/intel/x4x/ram_calc.c M src/soc/amd/stoneyridge/romstage.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/romstage.c M src/soc/intel/broadwell/romstage/romstage.c M src/soc/intel/cannonlake/romstage/romstage.c M src/soc/intel/denverton_ns/romstage.c M src/soc/intel/icelake/romstage/romstage.c M src/soc/intel/quark/romstage/fsp2_0.c M src/soc/intel/skylake/romstage/romstage_fsp20.c 23 files changed, 42 insertions(+), 55 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Werner Zeh: Looks good to me, approved
diff --git a/src/arch/x86/include/arch/cpu.h b/src/arch/x86/include/arch/cpu.h index 75f1f43..38066c1 100644 --- a/src/arch/x86/include/arch/cpu.h +++ b/src/arch/x86/include/arch/cpu.h @@ -309,8 +309,9 @@ };
/* - * Initialize postcar_frame object allocating stack size in cbmem - * with the provided size. Returns 0 on success, < 0 on error. + * Initialize postcar_frame object allocating stack from cbmem, + * with stack_size == 0, default 4 KiB is allocated. + * Returns 0 on success, < 0 on error. */ int postcar_frame_init(struct postcar_frame *pcf, size_t stack_size);
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 732b767..35e139f 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -48,6 +48,15 @@ { void *stack;
+ /* + * Use default postcar stack size of 4 KiB. This value should + * not be decreased, because if mainboards use vboot, 1 KiB will + * not be enough anymore. + */ + + if (stack_size == 0) + stack_size = 4 * KiB; + stack = cbmem_add(CBMEM_ID_ROMSTAGE_RAM_STACK, stack_size); if (stack == NULL) { printk(BIOS_ERR, "Couldn't add %zd byte stack in cbmem.\n", diff --git a/src/cpu/intel/haswell/romstage.c b/src/cpu/intel/haswell/romstage.c index 3cbdf44..47b9976 100644 --- a/src/cpu/intel/haswell/romstage.c +++ b/src/cpu/intel/haswell/romstage.c @@ -38,8 +38,6 @@ #include <cpu/intel/romstage.h> #include "haswell.h"
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -48,7 +46,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/drivers/intel/fsp1_1/car.c b/src/drivers/intel/fsp1_1/car.c index 84ea7d1..1e89a8e 100644 --- a/src/drivers/intel/fsp1_1/car.c +++ b/src/drivers/intel/fsp1_1/car.c @@ -25,8 +25,6 @@ #include <program_loading.h> #include <timestamp.h>
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -35,7 +33,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_mtrr(&pcf, CACHE_ROM_BASE, CACHE_ROM_SIZE, diff --git a/src/mainboard/emulation/qemu-i440fx/romstage.c b/src/mainboard/emulation/qemu-i440fx/romstage.c index e1d4f62..6b1883c 100644 --- a/src/mainboard/emulation/qemu-i440fx/romstage.c +++ b/src/mainboard/emulation/qemu-i440fx/romstage.c @@ -14,6 +14,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <stdint.h> #include <cbmem.h> #include <console/console.h> @@ -31,11 +32,7 @@
timestamp_add_now(TS_START_ROMSTAGE);
- /** - * The LZMA decoder needs about 4 KiB stack. - * Leave 1 KiB stack for general postcar code. - */ - if (postcar_frame_init(&pcf, 5 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/** diff --git a/src/mainboard/emulation/qemu-q35/romstage.c b/src/mainboard/emulation/qemu-q35/romstage.c index 3e0870f..e409ad1 100644 --- a/src/mainboard/emulation/qemu-q35/romstage.c +++ b/src/mainboard/emulation/qemu-q35/romstage.c @@ -14,6 +14,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <stdint.h> #include <cbmem.h> #include <console/console.h> @@ -32,11 +33,7 @@
timestamp_add_now(TS_START_ROMSTAGE);
- /** - * The LZMA decoder needs about 4 KiB stack. - * Leave 1 KiB stack for general postcar code. - */ - if (postcar_frame_init(&pcf, 5 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/** diff --git a/src/northbridge/intel/e7505/memmap.c b/src/northbridge/intel/e7505/memmap.c index f7a08bc..d450065 100644 --- a/src/northbridge/intel/e7505/memmap.c +++ b/src/northbridge/intel/e7505/memmap.c @@ -35,8 +35,6 @@ return (void *)tolm; }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -45,7 +43,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* diff --git a/src/northbridge/intel/gm45/ram_calc.c b/src/northbridge/intel/gm45/ram_calc.c index c1307c4..c614082 100644 --- a/src/northbridge/intel/gm45/ram_calc.c +++ b/src/northbridge/intel/gm45/ram_calc.c @@ -123,8 +123,6 @@ return (void *) top_of_ram; }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -133,7 +131,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/i440bx/ram_calc.c b/src/northbridge/intel/i440bx/ram_calc.c index 09a3b03..495ca86 100644 --- a/src/northbridge/intel/i440bx/ram_calc.c +++ b/src/northbridge/intel/i440bx/ram_calc.c @@ -15,6 +15,7 @@
#define __SIMPLE_DEVICE__
+#include <arch/cpu.h> #include <device/pci_ops.h> #include <cbmem.h> #include <console/console.h> @@ -67,8 +68,6 @@ return (void *)tom; }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -77,7 +76,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/i945/ram_calc.c b/src/northbridge/intel/i945/ram_calc.c index 752c8f9..525a5b9 100644 --- a/src/northbridge/intel/i945/ram_calc.c +++ b/src/northbridge/intel/i945/ram_calc.c @@ -88,8 +88,6 @@ return ggc2uma[gms] << 10; }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -98,7 +96,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/nehalem/ram_calc.c b/src/northbridge/intel/nehalem/ram_calc.c index e32190f..ba37610 100644 --- a/src/northbridge/intel/nehalem/ram_calc.c +++ b/src/northbridge/intel/nehalem/ram_calc.c @@ -43,8 +43,6 @@ return (void *) smm_region_start(); }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -53,7 +51,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/pineview/ram_calc.c b/src/northbridge/intel/pineview/ram_calc.c index a789956..d1b43aa 100644 --- a/src/northbridge/intel/pineview/ram_calc.c +++ b/src/northbridge/intel/pineview/ram_calc.c @@ -16,6 +16,7 @@
#define __SIMPLE_DEVICE__
+#include <arch/cpu.h> #include <device/pci_ops.h> #include <device/device.h> #include <device/pci_def.h> @@ -137,8 +138,6 @@
}
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -147,7 +146,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/sandybridge/ram_calc.c b/src/northbridge/intel/sandybridge/ram_calc.c index 5cda8a3..343ae62 100644 --- a/src/northbridge/intel/sandybridge/ram_calc.c +++ b/src/northbridge/intel/sandybridge/ram_calc.c @@ -43,8 +43,6 @@ return (void *) smm_region_start(); }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -53,7 +51,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/northbridge/intel/x4x/ram_calc.c b/src/northbridge/intel/x4x/ram_calc.c index 3714969..be9c10f 100644 --- a/src/northbridge/intel/x4x/ram_calc.c +++ b/src/northbridge/intel/x4x/ram_calc.c @@ -134,8 +134,6 @@ return (void *) top_of_ram; }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -144,7 +142,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Cache the ROM as WP just below 4GiB. */ diff --git a/src/soc/amd/stoneyridge/romstage.c b/src/soc/amd/stoneyridge/romstage.c index 12ee2a8..000d100 100644 --- a/src/soc/amd/stoneyridge/romstage.c +++ b/src/soc/amd/stoneyridge/romstage.c @@ -153,7 +153,7 @@ printk(BIOS_ERR, "Failed to set romstage handoff data\n");
post_code(0x44); - if (postcar_frame_init(&pcf, 1 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 5bf501d..5d65a00 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -240,7 +240,7 @@ else printk(BIOS_ERR, "Failed to determine variable data\n");
- if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
mainboard_save_dimm_info(); diff --git a/src/soc/intel/baytrail/romstage/romstage.c b/src/soc/intel/baytrail/romstage/romstage.c index 5621dd1..7e2bb64 100644 --- a/src/soc/intel/baytrail/romstage/romstage.c +++ b/src/soc/intel/baytrail/romstage/romstage.c @@ -238,8 +238,6 @@ romstage_handoff_init(prev_sleep_state == ACPI_S3); }
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* setup_stack_and_mtrrs() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use. */ static void platform_enter_postcar(void) @@ -247,7 +245,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/soc/intel/broadwell/romstage/romstage.c b/src/soc/intel/broadwell/romstage/romstage.c index 9bca716..9c86809 100644 --- a/src/soc/intel/broadwell/romstage/romstage.c +++ b/src/soc/intel/broadwell/romstage/romstage.c @@ -16,6 +16,7 @@ #include <stddef.h> #include <stdint.h> #include <arch/cbfs.h> +#include <arch/cpu.h> #include <bootblock_common.h> #include <bootmode.h> #include <cbmem.h> @@ -34,8 +35,6 @@ #include <soc/romstage.h> #include <soc/spi.h>
-#define ROMSTAGE_RAM_STACK_SIZE 0x5000 - /* platform_enter_postcar() determines the stack to use after * cache-as-ram is torn down as well as the MTRR settings to use, * and continues execution in postcar stage. */ @@ -44,7 +43,7 @@ struct postcar_frame pcf; uintptr_t top_of_ram;
- if (postcar_frame_init(&pcf, ROMSTAGE_RAM_STACK_SIZE)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n"); /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(&pcf, MTRR_TYPE_WRPROT); diff --git a/src/soc/intel/cannonlake/romstage/romstage.c b/src/soc/intel/cannonlake/romstage/romstage.c index 9dadb2d..94b9899 100644 --- a/src/soc/intel/cannonlake/romstage/romstage.c +++ b/src/soc/intel/cannonlake/romstage/romstage.c @@ -13,6 +13,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <cpu/x86/mtrr.h> #include <cbmem.h> #include <console/console.h> @@ -146,7 +147,7 @@ pmc_set_disb(); if (!s3wake) save_dimm_info(); - if (postcar_frame_init(&pcf, 1 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* diff --git a/src/soc/intel/denverton_ns/romstage.c b/src/soc/intel/denverton_ns/romstage.c index 4477c92..6d8eaab 100644 --- a/src/soc/intel/denverton_ns/romstage.c +++ b/src/soc/intel/denverton_ns/romstage.c @@ -14,6 +14,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <arch/io.h> #include <cbmem.h> #include <cf9_reset.h> @@ -161,7 +162,7 @@ display_fsp_smbios_memory_info_hob(); #endif
- if (postcar_frame_init(&pcf, 1 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* diff --git a/src/soc/intel/icelake/romstage/romstage.c b/src/soc/intel/icelake/romstage/romstage.c index b0eeb2e..514e5e8 100644 --- a/src/soc/intel/icelake/romstage/romstage.c +++ b/src/soc/intel/icelake/romstage/romstage.c @@ -13,6 +13,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <cpu/x86/mtrr.h> #include <cbmem.h> #include <console/console.h> @@ -131,7 +132,7 @@ pmc_set_disb(); if (!s3wake) save_dimm_info(); - if (postcar_frame_init(&pcf, 1 * KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index 5ebbacb..a8bd26e 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -13,6 +13,7 @@ * GNU General Public License for more details. */
+#include <arch/cpu.h> #include <arch/symbols.h> #include <console/console.h> #include <cbmem.h> @@ -61,7 +62,7 @@ /* Initialize the PCIe bridges */ pcie_init();
- if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/* Locate the top of RAM */ diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 6ff59ba..fafa343 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -158,7 +158,7 @@ pmc_set_disb(); if (!s3wake) save_dimm_info(); - if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 0)) die("Unable to initialize postcar frame.\n");
/*