Change in coreboot[master]: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value.
This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/romstage.c 1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/1
diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 89052d6..1816e80 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -20,8 +20,6 @@ #include <program_loading.h> #include <timestamp.h>
-#define DCACHE_RAM_ROMSTAGE_STACK_SIZE 0x2000 - static void romstage_main(unsigned long bist) { int i; @@ -29,11 +27,13 @@ const u32 stack_guard = 0xdeadbeef; u32 *stack_base; u32 size; + const u32 max_dcache_stack_size = CONFIG_DCACHE_BSP_STACK_SIZE ? + CONFIG_DCACHE_BSP_STACK_SIZE : 0x2000;
/* Size of unallocated CAR. */ size = ALIGN_DOWN(_car_stack_size, 16);
- size = MIN(size, DCACHE_RAM_ROMSTAGE_STACK_SIZE); + size = MIN(size, max_dcache_stack_size); if (size < DCACHE_RAM_ROMSTAGE_STACK_SIZE) printk(BIOS_DEBUG, "Romstage stack size limited to 0x%x!\n", size);
Hello Kyösti Mälkki, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34976
to look at the new patch set (#2).
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value.
This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/romstage.c 1 file changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 2:
(1 comment)
Maybe you can move the guards upwards here too, 256 above _car_stack_start ?
https://review.coreboot.org/c/coreboot/+/34976/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/2/src/cpu/intel/car/romstage.... PS2, Line 30: const u32 max_dcache_stack_size = CONFIG_DCACHE_BSP_STACK_SIZE ? Better:
const size_t stack_size = MAX(CONFIG_DCACHE_BSP_STACK_SIZE, 0x2000);
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34976
to look at the new patch set (#3).
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value. In many configurations with C_ENVIRONMENT_BOOTBLOCK the stack_base is at the base of CAR. If the stack grows too large it operates out of CAR, typically resulting in a hang. Therefore the stack guards are moved up 256 bytes to at least provide a warning when the romstage is dangerously close of running out of stack.
This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/romstage.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/3
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34976
to look at the new patch set (#4).
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value. This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
In many configurations with C_ENVIRONMENT_BOOTBLOCK the stack_base is at the base of CAR. If the stack grows too large it operates out of CAR, typically resulting in a hang. Therefore the stack guards are moved up 256 bytes to at least provide a warning when the romstage is dangerously close of running out of stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/romstage.c 1 file changed, 6 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/4
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... PS3, Line 41: of CAR. To make stack guards useful, move them up 256 bytes */ Official comment style is with * at start of line?
Hello Kyösti Mälkki, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34976
to look at the new patch set (#5).
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value. This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
In many configurations with C_ENVIRONMENT_BOOTBLOCK the stack_base is at the base of CAR. If the stack grows too large it operates out of CAR, typically resulting in a hang. Therefore the stack guards are moved up 256 bytes to at least provide a warning when the romstage is dangerously close of running out of stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/car/romstage.c 1 file changed, 7 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... PS4, Line 35: size = MIN(size, stack_size); looking at the linker script _car_stack_size is the same as CONFIG_DCACHE_BSP_STACK_SIZE. Why do you need this check?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... PS4, Line 35: size = MIN(size, stack_size);
looking at the linker script _car_stack_size is the same as CONFIG_DCACHE_BSP_STACK_SIZE. […]
There are 2 definitions of _car_stack_{start,end} in car.ld.
It's there for cases where CONFIG_DCACHE_BSP_STACK_SIZE is not defined (platforms without C_ENVIRONMENT_BOOTBLOCK).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34976/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34976/5//COMMIT_MSG@16 PS5, Line 16: the stack_base is at the base of CAR. If the stack grows too large it nit: rewrap lines ?
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... PS3, Line 41: of CAR. To make stack guards useful, move them up 256 bytes */
Official comment style is with * at start of line?
Ah. Crap, this was the proper comment style. Just end the sentence with dot.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34976/2/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/2/src/cpu/intel/car/romstage.... PS2, Line 30: const u32 max_dcache_stack_size = CONFIG_DCACHE_BSP_STACK_SIZE ?
Better: […]
Done
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/4/src/cpu/intel/car/romstage.... PS4, Line 35: size = MIN(size, stack_size);
There are 2 definitions of _car_stack_{start,end} in car.ld. […]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
I will cherry-pick this onto one of my branches and fix the pending style issues.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end If we can rely on _car_stack_end why wouldn't we utilize _car_stack_start ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
If we can rely on _car_stack_end why wouldn't we utilize _car_stack_start ?
For !C_ENV_BOOTBLOCK case, _car_stack_start is at _car_relocatable_data_end. We want that case to have guard at _car_stack_end - 0x2000 + 256. This file is not used with CAR_GLOBAL_MIGRATION=n, so lzma scratchpad will not be on the stack.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
For !C_ENV_BOOTBLOCK case, _car_stack_start is at _car_relocatable_data_end. […]
But why assume 0x2000?
Also, why not just base the location of the guard on _car_stack_start. Is that not the limit we care about?
I don't understand what makes the guards useful at a 256 byte offset, but that's separate. I think that rationale should be captured with an explanation instead of an assertion.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
But why assume 0x2000? […]
_car_stack_end - 0x2000 was the location of the stack guards before this change (DCACHE_RAM_ROMSTAGE_STACK_SIZE). Along the years we fixed some native raminits to honour that stack allocation without smashing the guards. Makes sense to maintain that?
I believe commit message explains +256 rationale already. With C_ENV_BOOTBLOCK and no VBOOT, attempting to use more than DCACHE_BSP_STACK_SIZE is likely to crash in the middle of romstage as stack would extend outside CAR. If stack usage only gradually increased with code changes, we have a period of commits were we get the error in logs instead of a sudden death with a completely innocent looking change.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
_car_stack_end - 0x2000 was the location of the stack guards before this change (DCACHE_RAM_ROMSTAGE_STACK_SIZE). Along the years we fixed some native raminits to honour that stack allocation without smashing the guards. Makes sense to maintain that?
But why open code the 0x2000? Don't we have all that parameterized? Regardless, why can't _car_stack_start be utilized since that is the base of the stack itself?
I'm not suggesting we don't have guards. I'm just questioning the point of subtracting things from _car_stack_end when _car_stack_start already exists.
I believe commit message explains +256 rationale already. With C_ENV_BOOTBLOCK and no VBOOT, attempting to use more than DCACHE_BSP_STACK_SIZE is likely to crash in the middle of romstage as stack would extend outside CAR. If stack usage only gradually increased with code changes, we have a period of commits were we get the error in logs instead of a sudden death with a completely innocent looking change.
If there is no intermediate checks between setting up the guards getting to the checking it doesn't matter if things are offset 256 bytes or not. I'm not in agreement with the argument that putting them there makes things more useful. If the argument is that stack usage over time will trigger the guard check the same is true just sitting at the base as well. anyway, despite the rationale being the commit description, I do think the same reasoning should be in the comment.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 23: Are you okay with 0x2000 if we keep this line and add comment?
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
_car_stack_end - 0x2000 was the location of the stack guards before this change (DCACHE_RAM_ROMSTAGE_STACK_SIZE). Along the years we fixed some native raminits to honour that stack allocation without smashing the guards. Makes sense to maintain that?
But why open code the 0x2000? Don't we have all that parameterized? Regardless, why can't _car_stack_start be utilized since that is the base of the stack itself?
Because we wanted to have constrained stack usage in romstage and move stuff to CAR_GLOBALs or .bss when possible.
I'm not suggesting we don't have guards. I'm just questioning the point of subtracting things from _car_stack_end when _car_stack_start already exists.
But _car_stack_start is not a parameterized distance away from _car_stack_end in the case of !C_ENVIRONMENT_BOOTBLOCK.
I believe commit message explains +256 rationale already. With C_ENV_BOOTBLOCK and no VBOOT, attempting to use more than DCACHE_BSP_STACK_SIZE is likely to crash in the middle of romstage as stack would extend outside CAR. If stack usage only gradually increased with code changes, we have a period of commits were we get the error in logs instead of a sudden death with a completely innocent looking change.
If there is no intermediate checks between setting up the guards getting to the checking it doesn't matter if things are offset 256 bytes or not. I'm not in agreement with the argument that putting them there makes things more useful. If the argument is that stack usage over time will trigger the guard check the same is true just sitting at the base as well. anyway, despite the rationale being the commit description, I do think the same reasoning should be in the comment.
If you smashed the guard at the base, you likely never reach the code throwing the error. If you smashed the guard at base+256, there is some chance execution flow is still able to return to romstage_main() to throw the errors and boot normally.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
_car_stack_end - 0x2000 was the location of the stack guards before this change (DCACHE_RAM_ROMSTAGE_STACK_SIZE). Along the years we fixed some native raminits to honour that stack allocation without smashing the guards. Makes sense to maintain that?
But why open code the 0x2000? Don't we have all that parameterized? Regardless, why can't _car_stack_start be utilized since that is the base of the stack itself?
Because we wanted to have constrained stack usage in romstage and move stuff to CAR_GLOBALs or .bss when possible.
I'm not suggesting we don't have guards. I'm just questioning the point of subtracting things from _car_stack_end when _car_stack_start already exists.
But _car_stack_start is not a parameterized distance away from _car_stack_end in the case of !C_ENVIRONMENT_BOOTBLOCK.
But it's also off limits according to the linker, no? A comment would be useful explaining the reasoning. It might also be helpful to print the distance between 0x2000 notion and _car_stack_start to know how much of the area is being wasted.
I believe commit message explains +256 rationale already. With C_ENV_BOOTBLOCK and no VBOOT, attempting to use more than DCACHE_BSP_STACK_SIZE is likely to crash in the middle of romstage as stack would extend outside CAR. If stack usage only gradually increased with code changes, we have a period of commits were we get the error in logs instead of a sudden death with a completely innocent looking change.
If there is no intermediate checks between setting up the guards getting to the checking it doesn't matter if things are offset 256 bytes or not. I'm not in agreement with the argument that putting them there makes things more useful. If the argument is that stack usage over time will trigger the guard check the same is true just sitting at the base as well. anyway, despite the rationale being the commit description, I do think the same reasoning should be in the comment.
If you smashed the guard at the base, you likely never reach the code throwing the error. If you smashed the guard at base+256, there is some chance execution flow is still able to return to romstage_main() to throw the errors and boot normally.
Sure. Your assumption is predicated on that the encroachment will be observed over time, and that argument would also hold as 256 bytes is encroached at the base. Sounds to me like you just want the guard area to be larger -- effectively providing a 512 byte padding to observe changes over time with the assumption that the encroachment is incremental vs blowing by it.
Kyösti Mälkki has uploaded a new patch set (#6) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value. This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
In many configurations with C_ENVIRONMENT_BOOTBLOCK the stack_base is at the base of CAR. If the stack grows too large it operates out of CAR, typically resulting in a hang. Therefore the stack guards are extended to cover 256 bytes at the base to at least provide a warning when the romstage is dangerously close of running out of stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/intel/car/romstage.c 1 file changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/76/34976/6
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34976/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34976/5//COMMIT_MSG@16 PS5, Line 16: the stack_base is at the base of CAR. If the stack grows too large it
nit: rewrap lines ?
Done
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/3/src/cpu/intel/car/romstage.... PS3, Line 41: of CAR. To make stack guards useful, move them up 256 bytes */
Ah. Crap, this was the proper comment style. Just end the sentence with dot.
Ack
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 23:
Are you okay with 0x2000 if we keep this line and add comment?
Done
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
_car_stack_end - 0x2000 was the location of the stack guards before this change (DCACHE_RAM_RO […]
Increased guards to 256 bytes but forgot to add new comment.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 6: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... File src/cpu/intel/car/romstage.c:
https://review.coreboot.org/c/coreboot/+/34976/5/src/cpu/intel/car/romstage.... PS5, Line 43: _car_stack_end
Increased guards to 256 bytes but forgot to add new comment.
Ack
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34976 )
Change subject: cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK ......................................................................
cpu/intel/car: Make stack guards more useful on C_ENV_BOOTBLOCK
With C_ENVIRONMENT_BOOTBLOCK, CONFIG_DCACHE_BSP_STACK_SIZE needs to be set to define a stack region that can be shared over all stages using CAR. It makes sense to use that Kconfig option's value instead of a hardcoded value. This will result in less false positives when the stack size is big, for instance with FSP using the coreboot stack.
In many configurations with C_ENVIRONMENT_BOOTBLOCK the stack_base is at the base of CAR. If the stack grows too large it operates out of CAR, typically resulting in a hang. Therefore the stack guards are extended to cover 256 bytes at the base to at least provide a warning when the romstage is dangerously close of running out of stack.
Change-Id: I2ce1dda4d1f254e6c36de4d3fea26e12c34195ff Signed-off-by: Arthur Heymans arthur@aheymans.xyz Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34976 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/intel/car/romstage.c 1 file changed, 7 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/cpu/intel/car/romstage.c b/src/cpu/intel/car/romstage.c index 2880145..e24225e 100644 --- a/src/cpu/intel/car/romstage.c +++ b/src/cpu/intel/car/romstage.c @@ -21,6 +21,8 @@ #include <program_loading.h> #include <timestamp.h>
+/* If we do not have a constrained _car_stack region size, use the + following as a guideline for acceptable stack usage. */ #define DCACHE_RAM_ROMSTAGE_STACK_SIZE 0x2000
static struct postcar_frame early_mtrrs; @@ -43,16 +45,18 @@ static void romstage_main(unsigned long bist) { int i; - const int num_guards = 4; + const int num_guards = 64; const u32 stack_guard = 0xdeadbeef; u32 *stack_base; u32 size; + const size_t stack_size = MAX(CONFIG_DCACHE_BSP_STACK_SIZE, + DCACHE_RAM_ROMSTAGE_STACK_SIZE);
/* Size of unallocated CAR. */ size = ALIGN_DOWN(_car_stack_size, 16);
- size = MIN(size, DCACHE_RAM_ROMSTAGE_STACK_SIZE); - if (size < DCACHE_RAM_ROMSTAGE_STACK_SIZE) + size = MIN(size, stack_size); + if (size < stack_size) printk(BIOS_DEBUG, "Romstage stack size limited to 0x%x!\n", size);
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Rudolph (Code Review)