Nico Huber 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/17//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35035/17//COMMIT_MSG@20 PS17, Line 20: Remove the postcar stage.
probably some remains from an older version of this patch. […]
Sorry, I wasn't clear. I just meant the wording. The postcar stage is indeed disable in case of RESET_VECTOR_IN_RAM (by a depends on !...) by this patch. So that should be mentioned here.
It's hard to overlook in the huge patch because it's implicit and just one line... ;)
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: /*
i don't particularly like this, since it just adds another line for no gain, but changed for consist […]
The other style option is /* and */ on the first and last line without preceding *s. i.e.
/* multi- line comment */
However, for top-level comments, I prefer the current style with wasted space.
https://review.coreboot.org/c/coreboot/+/35035/17/src/arch/x86/early_ram.ld@... PS17, Line 18: _PAD_FOR_ALIGNS = 0x20;
Nico, Arthur, here's what this is for and I would like a prettier solution, of course. […]
Thanks, that was clear to me already, though. What I'm asking for is the specific number `0x20` aka. 32. I looked into `memlayout.h` and the only region with an explicit ALIGN_COUNTER() is the new EARLYRAM_STACK(). It's not using 32, though, but 16.
And now that I looked further into it, do we need the explicit ALIGN_ COUNTER() at all? Do we really expect any size to not be a multiple of 16? Aren't the assertions enough?
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`.
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>
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?
Where did you ask? did you try the coreboot or flashrom mailing list? Took me less than a minute to find it, cb_check_image() in `cbtables.c` (I'm not a fan of that code, but it's there).
Another example is the flashupdate feature in FILO.
Well, I guess we can expect that every company that uses coreboot has built some infrastructure around it. Having the `id` section at a specific location was always an invariant for coreboot on x86.
I once had to deal with it because on Apollo Lake there is no CBFS at the end of the `coreboot.rom`. It turned out that there were such assumptions all over the place. I can't say how it looks like in other companies, but I would be surprised if we are the only one.
I don't say it has to be in the bootblock. If, for instance, you have nothing else that has to be at the end of CBFS, you could just put it there as usual.
(marking as resolved because any solution wouldn't belong into this change)