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:
What is the motivation for this? Are we just trying to save a few bytes of bootblock space? (Otherwise, the extra hassle of having to put yet another FMAP section everywhere seems more like a downside to me...)
Why does this have to be done by cbfstool? Wouldn't it be easier to just put this as a struct in a C file, compile it, objcopy to binary and use cbfstool write?
(And if the goal just is to save bootblock space we could also have that C file and just put it as a separate file into CBFS -- we already have cbfs-files-processor-struct to make that easy. FMAP sections really only need to be used when alignment to erase blocks is important, I don't see how that would apply here.)
The motivation here is not to save space. The number of bytes that will be saved is just too less to warrant the effort.
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?
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.