Julius Werner 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...
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.
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.)
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.
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 by other tools, please pick something other than deadbeef which is way too overused (e.g. 0xCB1DD1BC or something like that).
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).
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.)