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 20:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage.
Sorry, I wasn't clear. I just meant the wording. The postcar stage is […]
Oh, I see what you mean. postcar was removed from the execution flow of any of these processors, but not removed out of coreboot.
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@27 PS17, Line 27: they can be linked with their .data and .bss as normal
fwiw this is not specific to running a stage from dram, but can be done on all stages not running XI […]
Agree. Better wording for my intention was preram stages, not all stages. I'll describe it better.
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35035/15/Makefile.inc@1162 PS15, Line 1162: # Ignore segments for .car.data or .earlyram.data while adding romstage.
any news on this? can this be closed?
I don't see anything asserting empty .data. If I missed it, or if it's in review I believe it can stay separate from this change.
However, given the current nature of this patch, the change here is no longer a requirement. We're keeping the .data section in romstage and there seems to be no harm in telling cbfs to strip a nonexistent .car.data.
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/14/src/arch/x86/early_ram.ld@... PS14, Line 32: /* Account for the size of .near_reset_vector regardless if it's used */ : _NEAR_RESET_VECTOR = CONFIG_X86_RESET_VECTOR - 0x1000 + 0x10; :
With the bootblock approach, I'm nowhere close to needing >64K. […]
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld File src/arch/x86/early_ram.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 6: /*
The other style option is /* and */ on the first and last line without preceding *s. i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Agree, none of the other macros use ALIGN_COUNTER. The REGION macro also verifies the alignment. […]
Ah, you're mostly right 😊 Although the stack itself must be aligned, the region in memory may be unaligned. However, it looks like I can't remove all alignment here. Although the reset vector is on a 16-byte boundary, the subtraction below doesn't result in a nice address -- the FMAP_SIZE value below is 0xb6. So even if EARLYRAM_STACK is a don't-care, console and timestamps will subsequently fail their asserts of 4 and 8.
I'll use a more natural looking way to align it here and remove the alignment from memlayout.h.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 23: _PAD_FOR_ALIGNS
See description above.
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 30: #endif
We can also assert here that `. <= CONFIG_X86_RESET_VECTOR`.
Done
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 25: EARLYRAM_STACK(., _STACK_SIZE) : PRERAM_CBMEM_CONSOLE(., _CONSOLE_SIZE) : TIMESTAMP(., _TIMESTAMPS_SIZE) : #if !CONFIG(NO_FMAP_CACHE) : FMAP_CACHE(., FMAP_SIZE) : #endif
Can we add that in a follow-on change? You may know that the PSP will be able to run vboot before t […]
Ack.
Thinking a little more about this, the first implementation will run vboot on the PSP itself and it'll ship the workbuf to DRAM for us. We need to provide a target address for the PSP to use and give that to amdfwtool, ensuring it matches a region we declare here.
A vboot implementation I don't want to rule out is one that runs more like the traditional model, i.e. bootblock->verstage->romstage. In that case, a VBOOT2_WORK region anywhere we declare it should be fine. No need to coordinate with the PSP's amdfw.rom build.
BTW option 1 above will only be available on certain SKUs. Option 2 does not afford the ability to verify the memory init functionality (ABLs).