Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c@28 PS10, Line 28: if (err == CB_CBFS_CACHE_FULL)
Well, again, verstage already needs to have the RW cache but its memory layout is defined by RO code, so if it doesn't fit there it doesn't fit. The only way to have the RW cache defined by RW code is to make verstage explicitly avoid using the cache and look up the romstage directly on flash, which both wastes time for an extra lookup on flash that we normally wouldn't need and would be very hard to work into the CBFS APIs where I'd really like to make the cache transparent to higher layers. I don't think this is enough a problem in practice to make that worth it. I also think leaving the fallback option available is nice even if we will try to make sure we'll never hit it in Chrome OS, because other coreboot users may have other requirements. For some, there may be a need to store such a large amount of files that they cannot reasonably fit a full cache anywhere, but they may still benefit from a partial cache (especially since those files would likely be payload-specific stuff that gets added at the end, and they can order their cbfstool add invocations to put the stuff that's not needed in performance-critical paths in the region that would overflow the cache).
There are other ways such that RW_SECTION_A/B only has the next stage in its cbfs with another cbfs with the rest of the files.
RO -> RW_SECTION_A -> RW_SECTION_FULL
That the first RW is a small trampoline w.r.t. metadata (one file) which is signed w/ the signature in vblock. You would be extending the concept of the hash anchor to the first stage of RW like you have for bootblock in the current patchset (or just throw it in another file of that smaller cbfs that is vblock covered).
I find the argument about wasting time hard to swallow since you are explicitly allowing CACHE_FULL to be a 1st class citizen on chrome os which inherently requires a re-walk of the entire metadata of the cbfs to re-validate the metadata for anything that falls out of the cache.
I think it's important to have as little brittleness from the RO binding to RW, and we're now enforcing a tight coupling that is forcing some not so optimal decisions.
FWIW, I'm fine w/ fallback for different users of coreboot, but I don't think it's the right answer for Chrome OS. I believe there are other ways to optimize the dependencies and bindings. I think a lot of it starts from understanding the access patterns of files (which you eluded to) and chaining everything correspondingly. I agree it's simpler to throw everything into one big cbfs, but it's forcing us down particular paths where the trade-offs are not optimal when you introduce this hard coupling of RO->RW, especially fixing the size of the metadata cache when adding to cbmem when we have memory to play with to handle the full cache size.