Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 5:
(5 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:
Yeah, I think I had planned to document this somewhere and must have forgotten. Added a description.
Done
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 26: if (args->end - args->mcache < sizeof(*token))
I'm trying to be really thrifty with space in this cache because CBFS headers can be very small and […]
edit: Sorry, forgot I still had to update these comments before pressing publish. I found a way to follow your suggestion without wasting extra space by overwriting the CBFS header magic (which we don't need anymore once we get here) with the information I used to store in these tokens. So every entry stores its offset explicitly now, and I also made sure there's always space for the terminator.
https://review.coreboot.org/c/coreboot/+/38423/4/src/commonlib/bsd/cbfs_mcac... PS4, Line 42: if (offset != args->offset)
See above, I feel we might as well save the 4 bytes if we can.
edit: disregard other comment, done.
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. […]
Done
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) {
Yes, this is basically the same walking code as in cbfs_mcache_lookup() above. […]
edit: New struct mcache_entry should make this clearer now.