Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... File src/commonlib/bsd/cbfs_mcache.c:
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 72: ERROR("mcache overflow, should increase CBFS_MCACHE size!\n");
I changed my mind a bit about how this works -- I still think we need to support cases with RW updat […]
(Continued in https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c#28)
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 123:
Changed now as mentioned above. […]
Done
https://review.coreboot.org/c/coreboot/+/38423/16/src/commonlib/bsd/cbfs_mca... File src/commonlib/bsd/cbfs_mcache.c:
https://review.coreboot.org/c/coreboot/+/38423/16/src/commonlib/bsd/cbfs_mca... PS16, Line 136: if (entry->magic != MCACHE_MAGIC_FILE) {
We check for corrupted mcache above. […]
Mostly because I'm not sure what to return tbh, I didn't want to blow up this function to use cb_err_t and an out-parameter just for this. The mcache is in memory and unless there's memory corruption it shouldn't be possible for it to get corrupted, so the check above is basically just an assert.
Actually, let me just change that to an assert and then I can do that here as well.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/include/c... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/include/c... PS4, Line 98: struct vb2_hash *master_hash);
It's just a consequence of my cbfs_walk() design. […]
Ack
https://review.coreboot.org/c/coreboot/+/38423/16/src/include/cbfs.h File src/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/38423/16/src/include/cbfs.h@52 PS16, Line 52: find
fill?
Well it's finding the mcache, I think that's the most important thing about what it does, that it is using that result to fill out the member fields in the passed struct is more of an implementation detail. If I called it cbfs_boot_device_fill_mcache() I think it would sound like it was filling the actual mcache (which it doesn't), I want to avoid that impression.
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c@299 PS4, Line 299: const struct cbfs_boot_device *rw = vboot_get_cbfs_boot_device();
Those two or that vboot hasn't executed yet. […]
Ack
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c@310 PS4, Line 310: cbmem_entry_find
Well... we're not exactly. […]
Ack
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c@324 PS4, Line 324: ENV_BOOTBLOCK
I don't think there's a problem? When SEPARATE_VERSTAGE is true, the bootblock loads the verstage -- […]
Done
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)
Or move the cache once we're in RW? I don't feel like it's a good policy to count on not hitting the cache. That seems not ideal to me. I understand where you are coming from for future proofing, but it seems like not a great crutch. I'll think on it some more, but I don't think we should count on that situation being our 1st and only option.
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).
https://review.coreboot.org/c/coreboot/+/38423/4/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/38423/4/src/security/vboot/vboot_lo... PS4, Line 46: return; /* No RW mcache if we're staying in RO. */
Well, we would if I added a NULL check below. Okay, I can do that.
Done