Kyösti Mälkki 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:
(8 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.
The name is just killing me now. […]
Name is suitable for the purpose it is used in rules.h. I would like to keep the window open for separate bootblock/verstage/romstage approach, as nobody wants to answer questions on the mailing list about picasso VBOOT build process.
ARMs run romstage (and bootblock and verstage) from SRAM, maybe this can be generalised even more and drop 'depends on ARCH_X86'.
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 29: . += CONFIG_EARLYRAM_BSP_STACK_SIZE; Do you really need to execute APs in romstage?
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/early_dram.ld@... PS6, Line 58: ap_sipi_vector_in_rom = (_start16bit >> 12) & 0xff; Maybe just set ap_sipi_vector_in_rom to 0 and comment as unused. The related assert in failover.ld will disappear with next release.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... File src/arch/x86/include/arch/cpu.h:
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/include/arch/c... PS6, Line 298: asmlinkage void soc_hybrid_romstage_entry(uint32_t bist, uint64_t early_tsc); I don't see why bist and early_tsc should be forwarded to a function in soc/?
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 […]
AGESA/binaryPI C_ENVIRONMENT_BOOTBLOCK implementation would skip assembly_entry.S, like amd/stoneyridge does, for BSPs.
Maybe you should bring early_stack.S from CB:33759 into this commit.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 59: call soc_hybrid_romstage_entry Why soc_ prefix? Can you design this from day one such that code that is not specific to silicon is not under soc/.
TBH this looks very much like booblock_c_entry_bist from every aspect now, the point where you enter C after in protected mode? While the ChromeOS device may not need it, I would prefer if you kept most of the separate bootblock/romstage separation as an option.
https://review.coreboot.org/c/coreboot/+/35035/6/src/arch/x86/reset_in_dram_... PS6, Line 61: /* This is here for linking purposes. */ Is this really needed?
https://review.coreboot.org/c/coreboot/+/35035/6/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35035/6/src/cpu/Kconfig@32 PS6, Line 32: depends on RESET_VECTOR_IN_RAM Do you need a static value for this?
I would prefer these grouped under single 'if RESET_VECTOR_IN_RAM'.