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:
(3 comments)
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 we wanted an approach where cbfstool can access the version strings within the ELF, this could be […]
Yeah, I don't think we would want to build further upon this, I think the separate CBFS file solution is so much better (extensible, easier to parse and doesn't waste bytes in loaded images). If there's a strong desire to maintain the existing .id section for backwards-compatibility, I think this patch is fine to do just that. But I don't think we should expand it to other architectures.
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 ultim […]
Oh right, I forgot bootblocks were added as bins. You could do it in the Makefile while it's still an ELF rather than in cbfstool, I guess? For example override the `bootblock.raw.bin: bootblock.raw.elf` rule for x86, nm|grep the symbol addresses from $<, do some math, dd conv=notrunc them into $@ to the well-known offsets (from the end of the file). Maybe too complicated to save the bytes of a few pointers, idk.
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)
Did you read this as !ENV_X86 && ENV_BOOTBLOCK ?
Oh, yes I did, sorry. But that does add the KEEP() to all stages and I don't see why we'd want that? Why would cbfstool need to parse this out of each stage separately? Feels like just wasting space...