Change in ...coreboot[master]: soc/intel/skylake/romstage: Increase size of postcar stack
Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33434
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
soc/intel/skylake/romstage: Increase size of postcar stack
I increase the size oof the postcar stack to prevent a stack overflow during the measured boot feature. After common string functions have been moved from inline into .c file (https://review.coreboot.org/c/coreboot/+/32901), I experienced a stack overflow in the postcar stage while verifiying the romstage during measured boot. To prevent this, the stack size should be increased. To play it safe, it should be increased to 8 KiB - though this is open for discussion.
Change-Id: I6f1a4631bcadfb8c7d1de5bf0919e40990a65606 Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/skylake/romstage/romstage_fsp20.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/33434/1
diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 2819c6f..0eff793 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -156,7 +156,7 @@ pmc_set_disb(); if (!s3wake) save_dimm_info(); - if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 8*KiB)) die("Unable to initialize postcar frame.\n");
/*
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
Please update all platforms that use 1KiB, as they are likely affected as well.
The previous value was 5KiB, but it was decided that we don't need that much stack, as the cbmem space is wasted. I'd go for 4KiB as compromise. In all cases that should be documented as comment right on top of postcar_frame_init(), to prevent accidental changes in the future.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/33434/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33434/1//COMMIT_MSG@9 PS1, Line 9: I increase the size oof the postcar stack to prevent a stack overflow of
https://review.coreboot.org/#/c/33434/1//COMMIT_MSG@14 PS1, Line 14: measured boot. To prevent this, the stack size should be increased. To verifying
I am not so familiar about measured boots, you say postcar is verifying 'romstage'? We have generally used ROMSTAGE_RAM_STACK_SIZE 0x5000 for postcar.
Typically, it's the unlzma() that eats a lot of stack, and I am surprised that 1 KiB ever worked here.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 1:
Ah.. I found https://review.coreboot.org/c/coreboot/+/20536/ so ulzman() scratchpad should be in BSS for postcar stage and not consume stack.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
soc/intel/skylake/romstage: Increase size of postcar stack
I increase the size oof the postcar stack to prevent a stack overflow during the measured boot feature. After common string functions have been moved from inline into .c file (https://review.coreboot.org/c/coreboot/+/32901), I experienced a stack overflow in the postcar stage while verifiying the romstage during measured boot. To prevent this, the stack size should be increased. To play it safe, it should be increased to 8 KiB - though this is open for discussion.
Change-Id: I6f1a4631bcadfb8c7d1de5bf0919e40990a65606 Signed-off-by: Christian Walter christian.walter@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33434 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/skylake/romstage/romstage_fsp20.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 2819c6f..0eff793 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -156,7 +156,7 @@ pmc_set_disb(); if (!s3wake) save_dimm_info(); - if (postcar_frame_init(&pcf, 1*KiB)) + if (postcar_frame_init(&pcf, 8*KiB)) die("Unable to initialize postcar frame.\n");
/*
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Why was it merged? Not all comments have been processed yet? Do I need to give -2 on everything that I comment at?
Kyösti Mälkki has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Patch Set 2:
Why was it merged? Not all comments have been processed yet? Do I need to give -2 on everything that I comment at?
I quess this was merged because it had a +2 and was not updated for two days, so a split-second check would not notice there's pending issues. I usually give a -1 to such changes so that not everything is green (I am unworthy of -2 powers).
I agree this change was prematurely merged, though.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Yes, I merged it as part of last week's batch, and while I typically check for open comments, this one slipped through. sorry about that.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Should we maybe think about using a different submission method? I always find it a little jarring that a patch just gets instantly merged at the push of a button with no warning. In Chromium OS, we have submission tied to the CI system, so the author sets a "Commit-Ready" flag in Gerrit and after a couple of hours the CI system will merge it... gives other people a chance to say "wait, no!".
We have the 24h waiting period defined in our Gerrit guidelines anyway. How about we add a Commit-Ready flag that's set by the author and a simple bot that will merge all CLs after that flag has been set for 24 hours (and that have a +2, a Verified+1 and no -2, or maybe even no -1)? With that we could maybe also open up submission to more people (it's currently very restricted) and save Patrick the time to always do this manually. (Manual submissions could still be available to core developers for emergency reverts and the like.)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Patch Set 2:
Should we maybe think about using a different submission method? I always find it a little jarring that a patch just gets instantly merged at the push of a button with no warning. In Chromium OS, we have submission tied to the CI system, so the author sets a "Commit-Ready" flag in Gerrit and after a couple of hours the CI system will merge it... gives other people a chance to say "wait, no!".
We have the 24h waiting period defined in our Gerrit guidelines anyway. How about we add a Commit-Ready flag that's set by the author and a simple bot that will merge all CLs after that flag has been set for 24 hours (and that have a +2, a Verified+1 and no -2, or maybe even no -1)? With that we could maybe also open up submission to more people (it's currently very restricted) and save Patrick the time to always do this manually. (Manual submissions could still be available to core developers for emergency reverts and the like.)
I am afraid I don't understand how this would have helped here. The change was submitted after about two days of inactivity, without any negative code-review scores. Wouldn't the bot have submitted the change right away, even if there are unresolved comments?
IMHO, the -1 is used too little. It's a very nice balance - doesn't block, but is visible right away (red sticks out significantly). Sure, it is "volatile" (gets lost on a new patchset), but so is a +2.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
I am afraid I don't understand how this would have helped here. The change was submitted after about two days of inactivity, without any negative code-review scores. Wouldn't the bot have submitted the change right away, even if there are unresolved comments?
The idea is that you can actually see that the patch is about to go in. The author has to manually mark it Commit-Ready. It's a clear signal that the discussion is considered over and anyone who disagrees can intervene and stop the process (e.g. by setting a -1 or something that stops the bot). Right now the way it works is that someone +2s the patch, someone else has another comment and then it's unclear whether the author still wants to change something due to that comment or the patch is going to get merged as is. (Also, I think in general it's just nicer when users trigger submission for their own patches... it allows you to hold back if you think of something you still wanna change after you uploaded it.)
IMHO, the -1 is used too little. It's a very nice balance - doesn't block, but is visible right away (red sticks out significantly). Sure, it is "volatile" (gets lost on a new patchset), but so is a +2.
I guess, but people just don't use it that way in practice. I think it just feels too aggressive for most people.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Patch Set 2: Right now the way it works is that someone +2s the patch, someone else has another comment and then it's unclear whether the author still wants to change something due to that comment or the patch is going to get merged as is.
I always found CQ+2 in Chrome OS jarring: In addition to "I pushed the change for review", "it got the necessary reviews", "nobody gave it any negative review" we now also need a signal for "I really, really, really want to get this thing merged, this time for real"?
We have the WIP flag, we have CR-1. How many toggles do we need?
IMHO, the -1 is used too little. It's a very nice balance - doesn't block, but is visible right away (red sticks out significantly). Sure, it is "volatile" (gets lost on a new patchset), but so is a +2.
I guess, but people just don't use it that way in practice. I think it just feels too aggressive for most people.
Should we setup -0.5? ;)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33434 )
Change subject: soc/intel/skylake/romstage: Increase size of postcar stack ......................................................................
Patch Set 2:
Patch Set 2:
IMHO, the -1 is used too little. It's a very nice balance - doesn't block, but is visible right away (red sticks out significantly). Sure, it is "volatile" (gets lost on a new patchset), but so is a +2.
I guess, but people just don't use it that way in practice. I think it just feels too aggressive for most people.
Should we setup -0.5? ;)
IMHO it would be useful for the Reviewers group (e.g. me, unworthy of -2 powers) since it allows a differentiation between, e.g. "Don't forget about this!" and "I only gave a -1 because I can't give a -2".
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Christian Walter (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)