Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 12:
(3 comments)
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)
I can add that if you feel it's necessary, but cbfs_mcache_build() already prints an error when buil […]
I think we should have a Kconfig to fail so we don't fall back at all. For our deployments I don't think we should ever fall back on the boot media for the walk. It's way too subtle, and I think we'll be setting ourselves up for someone missing the hints and shipping something we shouldn't.
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c@295 PS10, Line 295: if (!CONFIG(NO_CBFS_MCACHE)) {
I came up with this design to allow users more flexibility for how to split up the two caches. […]
fwiw, vboot_loader has the similar calculation dance but for the RW piece. I was actually thrown off in thinking we had duplicated things. So it's not just in one place. We have implementation details in multiple files related to the same logic. I wonder if a helper could be provided for dealing with the similar logic.
https://review.coreboot.org/c/coreboot/+/38423/12/src/security/vboot/vboot_l... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/38423/12/src/security/vboot/vboot_l... PS12, Line 39: die("Failed to build RW mcache."); /* TODO: -> recovery? */ I would think we would want die or go to recovery if the cache is full.