Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 10:
(11 comments)
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... File src/commonlib/bsd/cbfs_mcache.c:
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 12: values in big-endian Why not transition these to host endian after the cache is built?
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 14: absolute offset What's the reason behind storing absolute? I think it'd be good to provide the explanation for the decision. The rdev in cbfs_boot_device object would be a sub region of the full device so those absolute offsets can exceed the sub region size. Or maybe I'm confused by this terminology. The offset is actually relative to the rdev of the cbfs_boot_device object.
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 70: .end = mcache + size - sizeof(uint32_t), /* leave space for terminating magic */ Shouldn't we be doing ALIGN_DOWN(size - sizeof(uint32_t), MCACHE_ALIGNMENT) to ensure we meet alignment assumptions?
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 74: assert(size >= sizeof(uint32_t)); also assert mcache is aligned to at least uint32_t as well?
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 116: entry While the unions are carefully crafted, I think it's clearer to do:
memcpy(mdata_out, &entry->file, data_offset);
https://review.coreboot.org/c/coreboot/+/38423/10/src/commonlib/bsd/cbfs_mca... PS10, Line 116: memcpy(mdata_out, entry, data_offset); Again, how are we ensuring data_offset does not exceed sizeof(cbfs_mdata) ?
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/Kconfig File src/lib/Kconfig:
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/Kconfig@91 PS10, Line 91: depends on VBOOT Shouldn't this also depend on !NO_CBFS_MCACHE?
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/Kconfig@98 PS10, Line 98: automatically be 0 How? Is that Kconfig makes this value 0 with !VBOOT?
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) Should we print something here indicating one is going back to the boot media for metadata?
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c@277 PS10, Line 277: /* Ensure we always init RO mcache, even if first file is from RW. */ Why? No explanation is provided.
https://review.coreboot.org/c/coreboot/+/38423/10/src/lib/cbfs.c@295 PS10, Line 295: if (!CONFIG(NO_CBFS_MCACHE)) { I see we're packing RO and RW into the same area. I feel like we could be more explicit by just having 2 regions instead of open coding the math and pointer arithmetic to try and pack things into a single region. The boundaries are already fixed so why not just be more clear?