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#56. 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.

View Change

To view, visit change 40376. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic5c55a49025508ff1f27c6f48d8e420f19d88ec2
Gerrit-Change-Number: 40376
Gerrit-PatchSet: 2
Gerrit-Owner: Furquan Shaikh <furquan@google.com>
Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org>
Gerrit-CC: Julius Werner <jwerner@chromium.org>
Gerrit-Comment-Date: Wed, 15 Apr 2020 03:45:40 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment