Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40376 )
Change subject: util/cbfstool: Add support for filling ID region using FMAP ......................................................................
Patch Set 2:
Patch Set 2:
I started looking at the id because of the recent changes that are going in for supporting the Picasso platform: https://review.coreboot.org/c/coreboot/+/35035/21/src/arch/x86/memlayout.ld#.... I don't think this change functionally helps that platform, but it helps keep the code clean w.r.t. different pieces being added. Do these details really need to be added to bootblock/decompressor regions?
I think they were there mostly because that guarantees they're not going to be compressed, but I agree, they don't really need to be embedded in any stage.
Why not CBFS? I am not sure how/if tools use this information. Adding to CBFS would require tools to either understand cbfs walking or use some other marker to identify where the details are present. Adding FMAP entry allows the tools to use FMAP as the source of truth to locate the right offset.
I'm pretty sure no tools use this at all right now. I do think CBFS would be the more appropriate place to put this (and would make the implementation easier because we already have that mechanism to add C structs as files). In fact there are other things that would probably be worth pulling into CBFS in the long term (e.g. the vboot FWIDs or the GBB), but at least there we have existing tooling that would need to be updated blocking that. For this thing where you are changing where it lives anyway, I think putting it into CBFS would make more sense. FMAP is fundamentally not a great/scalable way to organize things and I think things should only need to be FMAP sections if they have a good reason not to be CBFS files.
For things that might want to read this in the future, I don't see why we would assume they're more likely to have an FMAP parser than a CBFS parser. (In fact, I bet more people would have cbfstool than dump_fmap installed. Also, if we really care about people being able to access this easily without complicated parsers, the best approach would probably be to put a big, unique magic number in front and then store it uncompressed in CBFS... then you can just scan the whole image for that number if necessary.)
I am thinking of tools like flashrom which already understand FMAP and can make use of it to get to this information. I think the support in flashrom for reading this region is broken, but I don't think we want to add a cbfs parser to flashrom for getting to this region.