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.