Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 5:
(11 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.
Yeah, I think I had planned to document this somewhere and must have forgotten. Added a description.
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. […]
I'm trying to be really thrifty with space in this cache because CBFS headers can be very small and there can be hundreds of files, so every bit of per-file overhead quickly multiplies into a notable percentage. The tokens are really an edge case for unusual situations in this design, for a tightly packed CBFS without any holes you won't need any (other than the terminating one). See the new description I'm adding at the top for details.
I tried writing it so that it would always leave space for the final token at first but what I came up with somehow ended up more complicated than this. You probably want to check that you're not running over the end of the cache when walking it anyway, and at that point you might as well make use of that check by keeping things simple here.
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.
See above, I feel we might as well save the 4 bytes if we can.
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.
The goal of the design is that things are still usable when the cache is too small. This could particularly be relevant for a vboot RW update -- the RW mcache is (necessarily) built by RO, so if you want to add extra files with your update and end up overflowing the cache size hardcoded in RO you would be SOL without this. With this, you're just making accesses to a few files a little slower. (Note that cbfs_mcache_lookup() still has access to the master_hash for this reason, so the fallback path is still just as secure.)
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? […]
See above... this is necessary to support the cache full case. (I'm not printing an error again because I already printed one after building the cache, I thought once was enough.)
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. […]
Yes, this is basically the same walking code as in cbfs_mcache_lookup() above. Every "slot" in the mcache is either a CBFS file header or a token. The new documentation I'm adding to the top of the file will explain this.
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?
It's just a consequence of my cbfs_walk() design. I want it (eventually) to be usable by cbfstool for generating the master_hash as well, so I have the CBFS_WALK_WRITEBACK_HASH option. That's why the master_hash argument to cbfs_walk() cannot be const, and since this is passed through to there this cannot be const either (although this function will not set CBFS_WALK_WRITEBACK_HASH and so the master_hash argument here will never be written to). I could also make it const here and cast the const away explicitly in the code but I think it's easier to just leave it like this.
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? […]
Those two or that vboot hasn't executed yet. Technically it would also return NULL when vboot_locate_firmware() returns NULL (e.g. FMAP slot not found), but in that case vboot should've rebooted into recovery mode before we get here (or actually, now that I look at it it just does a die() in that case).
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. […]
Well... we're not exactly. There's still a check for ROMSTAGE_OR_BEFORE below, so if we're in ramstage that path won't be taken either. In that case ro.mcache_size would stay 0, cbfs_mcache_lookup() would immediately act as if the cache was full and fall back to reading from SPI. (This isn't really a case that's ever meant to happen since that CBMEM entry should always be there in ramstage, but I think it would do the right thing if it did.)
Basically, this code is trying to handle 4 cases:
1. bootblock/verstage: always use memlayout mcache 2. romstage before CBMEM init: use memlayout mcache 3. romstage after CBMEM init: use CBMEM mcache (in this case I guess it could fall back to memlayout if the CBMEM entry was broken for some reason, but in this case that would also not be a problem) 4. postcar/ramstage: always use CBMEM (should always exist here)
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?
I don't think there's a problem? When SEPARATE_VERSTAGE is true, the bootblock loads the verstage -- so it will build the RO mcache at that point (at the latest), before any vboot stuff runs. Doesn't matter if the verstage returns after that or not, both stages will access the valid cache in SRAM/CAR.
But there might actually be a problem with SEPARATE_VERSTAGE=n now that I think of it, because then I guess it's possible that the very first file we're loading is already an RW file and we've never initialized the RO mcache. I'll move this to the top to prevent that.
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?
Well, we would if I added a NULL check below. Okay, I can do that.