Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG@27 PS4, Line 27: stack heap
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@96 PS4, Line 96: CONFIG_C_ENVIRONMENT_BOOTBLOCK
Are we dependent on this? I don't think we are.
Yeah, having this space should be independent.
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... File src/arch/x86/include/arch/symbols.h:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... PS4, Line 64: _car_heap_start - _car_heap_end Isn't this inverted?
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK It would be good to have a help text indicating what this is used for. Also, shouldn't this live in a FSP-related Kconfig file since it is very specific to FSP?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 164: setup_fsp_stack_frame Doing this in setup_fsp_stack_frame() makes it confusing. Can we call setup_fsp_stack_frame() only if !CONFIG(FSP_USES_CB_STACK) and setup the params for heap HOB if CONFIG(FSP_USES_CB_STACK) is true outside of this function?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 178: stack heap?
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 189: vboot I understand the intent here to add vboot to provide a complete picture of the memory layout. But, car.ld actually contains more than just vboot. It would be good to either put in a comment or ... or some indication that there are more components within this space.