Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35035 )
Change subject: arch/x86: Implement RESET_VECTOR_IN_RAM ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig File src/arch/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Kconfig@87 PS6, Line 87: reset vector in RAM instead of the traditional 0xfffffff0 location. This should have been documented that the assumption is that romstage is the first stage when it is selected.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/Makefile.inc@2... PS6, Line 223: ifneq ($(CONFIG_RESET_VECTOR_IN_RAM),y) Is this guard needed? Presumably C_ENVIRONMENT_BOOTBLOCK=n when RESET_VECTOR_IN_RAM=y? And vice versa?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld File src/arch/x86/early_dram.ld:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 25: _earlyram_region_start = . ; I don't understand why we're just shoving this region after the program. Any reason why we can't just assign a section for the stacks?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... File src/arch/x86/reset_in_dram_crt0.S:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 37: jmp soc_reassign_stack /* expect returning jmp to zero_stacks */ Can we use a different ABI to indicate return symbol? like another register's value such as ebp, e.g.?
I know it's necessary, but I also feel like it's weird to zero out the stacks in this path as well. They don't technically require zero'ing out. But .bss certainly does, but I don't see that in this patch. And we have no provision for .bss with this feature? That seems really broken. I similarly also feel like we could modify assembly_entry.S to accommodate things. Admittedly that's just a gut feel.