7 comments:
Patch Set #3, Line 36: .code64
We can't have the feature of this CL work with RAMSTAGE_X86_64. It would be broken.
Patch Set #3, 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.
Patch Set #3, Line 45: ramstage_start:
This doesn't need to be in the else. It's just a symbol.
Patch Set #3, 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.
File src/arch/x86/ramstage_loader.c:
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.
Patch Set #3, 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.
File src/soc/intel/cannonlake/Kconfig:
Patch Set #3, 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?
To view, visit change 34752. To unsubscribe, or for help writing mail filters, visit settings.