Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 11:
(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?
Same reason as mentioned in https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/..., I think to avoid confusion it's just better to have a hard rule that anything in a union cbfs_mdata always stays in big-endian.
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 d […]
Yes, when I say absolute I mean absolute to the cbfs_dev_t (as opposed to say relative to the previous header or something, which I considered but decided absolute is easier to deal with). For coreboot that's equivalent to an rdev for the CBFS FMAP region. This code only knows about cbfs_dev_t, it doesn't know anything about the implementation beyond that (which could be a file in the case of cbfstool, or anything else really). Clarified comment.
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 alignm […]
Done
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?
Done
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) ?
This also goes back to https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri...: cbfs_walk() considers any header invalid that doesn't fit within cbfs_mdata and refuses to even parse the rest of it (so in this case build_walker() is never called on it and it doesn't end up in the mcache).
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: […]
Done
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?
Done
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?
Yes, Kconfig still outputs macros for configs with unmet dependencies, it just forces them to n or 0 or "" (depending on the type).
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?
I can add that if you feel it's necessary, but cbfs_mcache_build() already prints an error when building if the cache is too small. I think that's really the important point where it should be reported and then later it's not that useful to say it again and again whenever a file from beyond the cached part is actually read. (Note that the normal log message for a found file is still different between mcache and normal lookup, so it's still possible to tell from the log where a specific file came from, there's just not an explicit error message on a separate line again for each file.)
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.
Because otherwise the RO mcache might never get initialized. I don't want to link the mcache building code into every stage just in case that stage is the first where it's needed. Expanded the comment a little, let me know if that helps.
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 came up with this design to allow users more flexibility for how to split up the two caches. The CBFS_MCACHE region is declared in SoC memlayouts, but different boards using that SoC (or different images running on that board) may prefer different splits. For example, on Chrome OS we have all our UI assets in the RO CBFS, so there tend to be a lot more files in our RO CBFS than in our RW. For other users a 50:50 split probably makes more sense, or they may even want to give more weight to RW (e.g. the Facebook guys who don't use recovery mode and thus don't have the post-verstage stages in RO). That's why I set the default for CBFS_MCACHE_RW_PERCENTAGE to 25 for CONFIG_CHROMEOS and to 50 otherwise.
This is the only code that deals with those raw percentages and everything else is just operating on the cbfs_boot_device structures set up here, so I'd say it's about as encapsulated as it can be already.