Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47602 )
Change subject: lib/version: Use .id section and remove id.S files ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47602/3//COMMIT_MSG@17 PS3, Line 17: Order of strings is now undefined.
This would be easy to fix if you just used three separate sections...
True but not necessary if the strings are for hexdump convenience only and the reverse-offsets get filled by some other method.
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc File src/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/Makefile.inc@225 PS3, Line 225: bootblock-y += flash_id.c
If this is only used on x86 please put it in arch/x86.
If we wanted an approach where cbfstool can access the version strings within the ELF, this could be all-y. I know there are alternative and maybe simpler solutions with separate FMAP headers(?) and CBFS textfiles.
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c File src/lib/flash_id.c:
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@17 PS3, Line 17: union __ptr version;
Are these even needed here? I'm not 100% sure what you're trying to do but it seems that you ultimately expect readers to just use the reverse_offset anyway, right (so it's backwards-compatible)? So what are these pointers good for? (Since they're in the runtime address space they're not going to be useful to an external tool parsing the file anyway.)
I was anticipating maintaining backwards compatibility for flashrom is required and the reverse offsets below filled in. Parsing the ELF, one can still translate the runtime addresses to file offsets.
If you just have these here so cbfstool can use them to calculate the reverse offsets, I don't think you need that. cbfstool gets the whole unstripped ELF as input so it should be able to just find the location of those strings in the symbol table.
I only saw bootblock.bin for cbfstool input file, so this would need to change. Even if ELF .strtab (?) was parsed, I don't know if a dependency between a variable name in coreboot proper and util/cbfstool would be allowed. So the approach I was taking is definining a serialized struct with a magic tag.
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/flash_id.c@26 PS3, Line 26: .magic = 0xdeadbeef,
Is this needed/useful for anything? If you want a real magic that can actually be searched and found […]
Different magic for sure, if implemented.
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/c/coreboot/+/47602/3/src/lib/program.ld@18 PS3, Line 18: #if !(ENV_X86 && ENV_BOOTBLOCK)
I think this is problematic because now you're using this section name for stuff that is linked into every stage, but the linker script only explicitly mentions it in the bootblock. For all other stages it will be an orphaned section and the linker will basically just stuff it wherever it feels like (I think that's usually at the end but better not to rely on that).
Did you read this as !ENV_X86 && ENV_BOOTBLOCK ?
Only x86 bootblocks were excluded here because they traditionally had these strings near the end of coreboot.rom.
I think the simplest solution is to just it up here for all stages -- it doesn't really matter where exactly it goes. You just need to use ENV_BOOTBLOCK to control the KEEP(). (Also, please keep the existing condition with the decompressor... when the bootblock is compressed these should be put into the decompressor stub instead if the point is to see them in a hex editor.)
I need to rebase on CB:46834 to move .id.keep for x86 bootblocks. I would use KEEP() for every stage here if cbfstool would parse the ELF for these strings.