Marshall Dawson 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 […]
The name is just killing me now. Do you have an opinion on changing it so something more descriptive now that the entry and infrastructure is in arch? AFAIK this is very AMD/PSP-specific, and when I chose the name the implementation was way down in the soc directory.
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 vers […]
Ack. No longer needed.
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. […]
Do you mean within the program map instead of outside? At the moment, the elf file is only 64K and there's not adequate room for everything I'd like to have. I'll have to dig up some changes I once had that allowed the file to be any n*64K.
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.?
OK
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.
I can skip zeroing. They come up w/garbage and I was allowing that to bother me.
And we have no provision for .bss with this feature? That seems really broken.
It took me a bit to recognize that .bss is mapped within the lowest and highest boundaries of the program. As a result, it's getting inherently zeroed due to the compression and build process. I agree it should be explicit instead.
I similarly also feel like we could modify assembly_entry.S to accommodate things. Admittedly that's just a gut feel.
I'll take another look. The first item I see that will need to be addressed is the _car_stack_end symbol which we won't have. The next is that assembly_entry.S clears _bss through _ebss, with the comment that it's not shared -- oh, maybe the comment meant not shared across stages.
...Because .bss _is_ "shared" among cores, and AMD APs begin at the reset vector just like the BSP. So we'd need to make sure APs don't re-zero .bss. Hmm, we would have a bug on stoney ridge due to that, if not for the fact we send APs directly out of bootblock into the middle of romstage. (Kyösti, I think that will be a problem as Family 14h/15h/16h and earlier work toward C_ENVIRONMENT_BOOTBLOCK too.) * So how about checking for BSP before clearing .bss in assembly_entry.S? Or did I miss something obvious?