Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 4:
(12 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 8: There should be a format description of this cache.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 26: if (args->end - args->mcache < sizeof(*token)) This seems like we're dangling lots of conditions on the state of teh cache. Why can't we be more explicit that every file entry needs a token as well as needing space for the entry indicating termination of the cbfs? If those things aren't met we have failed entirely -- and propagate that situation up the stack?
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 42: if (offset != args->offset) Why is this conditional? I feel like we should always keep a token marker for every file.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 54: args->offset = ALIGN_UP(offset + data_offset + data_length, CBFS_ALIGNMENT); I'm not clear why we would be keeping track of this at all. The walker should just spit offsets out that are consumed and logged.
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"); This should be a major error.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 123: By default we are falling back to the boot media? Why? Shouldn't we have an error when this occurs? Likewise there isn't a comment nor a console output indicating this explicit case was hit.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 135: if (memcmp(mdata->h.magic, CBFS_FILE_MAGIC, sizeof(mdata->h.magic)) != 0) { This needs some commentary. From what I can tell there's a token if there's no cbfs metadata, correct?
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); Why isn't master_hash const?
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(); When this returns NULL what semantics is it indicating?
- recovery? - vboot isn't enabled?
What else?
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c@310 PS4, Line 310: cbmem_entry_find This failing is a broader problem. I don't think we should fall back into ENV_ROMSTAGE_OR_BEFORE case.
https://review.coreboot.org/c/coreboot/+/38423/4/src/lib/cbfs.c@324 PS4, Line 324: ENV_BOOTBLOCK How do you envision this stuff working with separate verstage and NO_RETURN_FROM_VERSTAGE?
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. */ Dont' we already have this covered with vboot_get_cbfs_boot_device() returning NULL?