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 18:
(5 comments)
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. I'll change the comment and naming here and let's do away with the .near_reset_vector change for now.
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 18: _PAD_FOR_ALIGNS = 0x20;
Please add a comment what exactly this accounts for.
Nico, Arthur, here's what this is for and I would like a prettier solution, of course.
You probably guessed that the intent is to place the regions at l.25 - l.30 as close to the reset vector as possible to keep things dense. However the EARLYRAM_STACK(), PRERAM_CBMEM_CONSOLE(), TIMESTAMP(), etc. macros in memlayout.h perform their own alignments if necessary. This can result in the linker's location counter advancing more than, say CONFIG_PRERAM_CBMEM_CONSOLE_SIZE, if that region would otherwise begin on an unaligned address.
The _PAD_FOR_ALIGNS allows for a little free space to prevent the linker from erroring out as the result of alignment corrections in memlayout.h.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 23: _PAD_FOR_ALIGNS
This is a bit weird? Can't you use an ALIGN macro to align things?
See description above.
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
Don't you want VBOOT2_WORK somewhere in there?
Can we add that in a follow-on change? You may know that the PSP will be able to run vboot before the x86 comes out of reset (certain SKUs only). So while the answer to your question is likely "yes", the mechanism for telling PSP where to put the workbuf is still TBD.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld File src/arch/x86/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/memlayout.ld@... PS17, Line 57: #include <arch/x86/id.ld>
How does it get into the coreboot. […]
At the moment, it's not there and we're not attempting to use it for anything. It seems like Kyosti had suggested the ID was used by flashrom, but I couldn't find that, even after asking around a bit. Can you state the practical reasons for keeping it there?
Our bootblock is compressed inside the flash image. (Inside the amdfw.rom artifact to be precise.) Since the reset vector isn't at 4GB-0x10, there's no need to leave an uncompressed copy of bootblock there. And once we accept that premise, it follows that an ID embedded in the bootblock won't be at the top of flash. Nor will it be immediately readable, as it's compressed.