Kyösti Mälkki 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 9:
(2 comments)
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
Usually you would choose the minimum scope for the change you need, and avoid exposing stuff globally or architecturally without good arguments. A this time this all seems very much tied to FSP2.1 and the allocation you make can be made even within the function fsp_fill_common_arch_params() inside fsp2_0/memory_init.c.
This FSP_USES_CB_STACK now starts to sound like a poor design choice when it has 57% (64/112) increase in the CAR it reserves for romstage. Additionally, with your changes in car.ld, bootblock and verstage will see reserve from CAR for FSP to go from 0 KiB to 176 KiB (112 KiB in car_stack, 64 KiB in car_heap), none of which you need?
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/9/src/arch/x86/car.ld@94 PS9, Line 94: #if CONFIG(FSP_USES_CB_STACK) Why define this for bootblock and verstage, when you only need it for romstage and it can change location between the stages?
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/9/src/drivers/intel/fsp2_0/Kc... PS9, Line 209: default 0x10000 Your previous comment Sep 03 09:05
4. Right now on CML, heap size requirement is 0x11000, will call out in integration guide and subjected to change over platform if any then integration guide should have latest data.
Another thing CB:35237, Sep 03 05:26
No Aaron/Tim. Stack requirement remain same ~128KB and HOB heap requirement is ~64KB. earlier we were actually passing ~128KB to stack and heap both using same arch->upd = stack_size. Now with next integration guide things will be call out correctly.
If we continue to allow 0x4000 for coreboot stack, like we did without FSP_USES_CB_STACK, FSP is currently left with 112 KiB for stack? So DCACHE_BSP_STACK_SIZE is still not synced with documentation?