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.