Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34752 )
Change subject: arch/x86: Attempt to boot without postcar stage/phase ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S File src/arch/x86/c_start.S:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@36 PS3, Line 36: .code64 We can't have the feature of this CL work with RAMSTAGE_X86_64. It would be broken.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@42 PS3, Line 42: _start: Can we get away with #include'ing exit_car.S at this point? Will 2 symbols declared back to back with the same name? There's section mismatch in exit_car.S and line 34 above, but we can fix that. Just wondering how to make this bit less kludgy.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@45 PS3, Line 45: ramstage_start: This doesn't need to be in the else. It's just a symbol.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/c_start.S@48 PS3, Line 48: lgdt %cs:gdtaddr Do we want to load 2 different gdts successively? Or remove the first one in exit_car.S? It at least needs a comment here and exit_car.S to note what's happening and the conclusion for handling the seemingly dual gdt initialization.
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... File src/arch/x86/ramstage_loader.c:
https://review.coreboot.org/c/coreboot/+/34752/3/src/arch/x86/ramstage_loade... PS3, Line 91: Why is this file essentially duplicated? Can we not modify postcar_loader to handle the 2 different variants? I don't think we should be duplicating the exact same logic with name changes and different timestamp parameters.
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/34752/3/src/lib/program.ld@100 PS3, Line 100: #if ENV_RMODULE || ENV_POSTCAR || CONFIG(RAMSTAGE_LOADS_FROM_ROMSTAGE) when ramstage is relocatable it already has this section. Why wouldn't we just have RAMSTAGE_LOADS_FROM_ROMSTAGE depend on relocatable ramstage? Currently, you are just getting lucky that things are linked properly because I don't see any changes that enforce such a dependency in this patch.
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/34752/3/src/soc/intel/cannonlake/Kc... PS3, Line 81: select RAMSTAGE_LOADS_FROM_POSTCAR we wouldn't be switching over all these SoC with this patch. We'd want to evaluate it and enable it separately. Similarly, if this is a direction we want to move why wouldn't it apply every where and nuke postcar all together?