11 comments:
File src/commonlib/bsd/cbfs_mcache.c:
Patch Set #10, Line 12: values in big-endian
Why not transition these to host endian after the cache is built?
Patch Set #10, 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.
Patch Set #10, 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?
Patch Set #10, Line 74: assert(size >= sizeof(uint32_t));
also assert mcache is aligned to at least uint32_t as well?
Patch Set #10, Line 116: entry
While the unions are carefully crafted, I think it's clearer to do:
memcpy(mdata_out, &entry->file, data_offset);
Patch Set #10, Line 116: memcpy(mdata_out, entry, data_offset);
Again, how are we ensuring data_offset does not exceed sizeof(cbfs_mdata) ?
Patch Set #10, Line 91: depends on VBOOT
Shouldn't this also depend on !NO_CBFS_MCACHE?
Patch Set #10, Line 98: automatically be 0
How? Is that Kconfig makes this value 0 with !VBOOT?
Patch Set #10, Line 28: if (err == CB_CBFS_CACHE_FULL)
Should we print something here indicating one is going back to the boot media for metadata?
Patch Set #10, Line 277: /* Ensure we always init RO mcache, even if first file is from RW. */
Why? No explanation is provided.
Patch Set #10, 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?
To view, visit change 38423. To unsubscribe, or for help writing mail filters, visit settings.