Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38423 )
Change subject: cbfs: Add metadata cache ......................................................................
Patch Set 18:
(1 comment)
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)
There are other ways such that RW_SECTION_A/B only has the next stage in its cbfs with another cbfs with the rest of the files.
RO -> RW_SECTION_A -> RW_SECTION_FULL
Yeah, but... do we want that? I feel like this would add a grotesque overcomplication to our otherwise neat and clean RO/RW CBFS concept to solve what I really think is a non-issue. I mean part of the whole reason I'm doing all of this is to maintain backwards-compatibility to old tooling, old payloads, etc. If we suddenly start changing CBFS section names and splitting things up into multiple pieces I might as well throw all of this away and design a new file system from scratch that's more suited for verification.
I don't follow your assertion of needing to throw all of this away. I don't think what I suggested should be applied to every user of coreboot. But we do have a real need in Chrome OS with RO/RW split. The surface area exposure of RW from RO should be minimized as much as possible. And the suggestion I made is about minimizing that exposure. I do not subscribe to the notion that falling back to boot media for lookups because of an increase in number of files in RW is a 'non-issue'. I actually think it goes against the larger intent and push to having TOCTOU capable system. In the end, we're just steering to the cbfs at well-defined boundaries. I don't think that is a major change that necessitates throwing everything out.
We have realities that pre-RAM environments are resource constrained. That environment necessitates making decisions to optimize for those constraints. Having mutliple cbfs that are walked through is not a huge leap to handle that. I will admit that our tooling around building cbfses is not great at all. I think we probably want to invest in better tools for helping aid in building all these things up.
This can be implemented over time, but I don't see why we would disregard a solution to a problem you're already expecting to happen. We should plug that gap, imo, if we think this is going to be common.
I think we need flexibility in chaining cbfses, though, because not everything fits into a simple solution space. Furquan brought up the CSE issue in the doc. It's a problem we do need to solve and not ignore because it doesn't fit into the cleanliness bucket. Having that chaining capability allows for flexibility in designing solutions to problems we need to tackle.
I find the argument about wasting time hard to swallow since you are explicitly allowing CACHE_FULL to be a 1st class citizen on chrome os which inherently requires a re-walk of the entire metadata of the cbfs to re-validate the metadata for anything that falls out of the cache.
But you're comparing a common case time waste to an extremely rare case here. I mean in how many RW updates in Chrome OS history did we actually add a new file that didn't exist in the old version? And even then it would most likely fit because there's most likely a bunch of slack in the cache space anyway.
I was going off of your assertion you expect the fallback to be the answer. My assumption is that we should never hit that fallback because it's not appropriate for our product. That is exactly why I suggested we add an option to fail in that case because things went terribly bad.
Historical updates aren't very helpful guidance since I anticipate updates to be happening more frequently along with features added to assist in product improvements and consistency across the fleet.
I think it's important to have as little brittleness from the RO binding to RW, and we're now enforcing a tight coupling that is forcing some not so optimal decisions.
I'm confused... isn't the point of having the fallback exactly that there's no tight coupling (the RW will still always work even if it exceeds the bounds that were presumed in the RO)?
I think that depends on definition of "work", though fallback definitely works in the sense of being load and use files not hit in metadata cache. That is a policy decision, though, that I don't believe we should employ. I don't think its appropriate to go back to boot media for lookups. We should be striving for doing those operations in-core.
Also, I don't think there's anything "brittle" about the fallback path -- it is not unsafe or insecure or in any way worse than using the mcache other than a tiny speed hit, and I would continue to sleep perfectly sound if we actually did end up hitting that very unlikely set of circumstances to trigger it on a board. That's the reason I implemented this fallback because I *don't* want people to have to worry about what happens if their update overflows the cache. And the speed hit is really just the cost of a normal CBFS lookup (without CONFIG_TOCTOU_SAFETY, which we're not planning to enable, the metadata hash doesn't get checked again in that path), the very thing we have always been doing all the time before this patch anyway, and only for the few files that actually exceed the cache (which on Chrome OS we could for example order to be files like ecrw that aren't needed on every boot).
I was referring to the coupling of RO and RW. Right now RO takes an offset/size and signature checks the whole thing. Data published by RO is the vboot result containing those attributes. That's not a ton of surface area. However, when building out RW cache and assuming a single cbfs, we now are running up against constraints of the pre-RAM environment and forcing those decisions leads to more limitations in the constraints. I think we should optimize for keeping dependencies on RO from RW as minimal as possible.
Our threat model in Chrome OS is changing and needs to change for business purposes. We should not just accept that since we did something in the past we shouldn't improve for the future. We can design solutions that allow people not to worry about pre-RAM mcache overflows. In order to order files one needs to characterize temporal access patterns and have the tools place files accordingly. That's the same type of work needed for arranging files in multiple cbfses based on access/use as well, fwiw.
So basically, the way I see this is that I uploaded an optimization patch here that can make booting maybe ~20ms faster, and what we're discussing right now is "but what if the stars align and something happens that probably never actually happened in practice in the past 5 years and cause this to only make the boot 19ms faster on one board?". From that point of view, I'm kinda surprised how concerned you are about this case.
Again, that was based on your indication you wanted to allow for it which I think is not what we should be doing. I was not basing my stance on performance what so ever.
Moreover, anything not hitting in the mcache needs to have a complete re-walk and recalculation of the metadata to determine it's legit metadata. Or were you wanting to leave the window of opportunity open from when the anchor was checked and when every file is accessed (and assumed the metadata is still correct)? I feel like it's not that much more effort to achieve that situation from ever happening. That can definitely be added later, but I don't think we should balk at achieving that goal.
especially fixing the size of the metadata cache when adding to cbmem when we have memory to play with to handle the full cache size.
I mean, if this would help allay your concerns I'm happy to add code to the CBMEM hook that would rebuild the full cache in DRAM if the pre-RAM cache had overflowed (although I think that the cumulated extra boot time from loading all the mcache building code into romstage on every board would far, far exceed the time saved in these very rare cases).
I don't think that's a great solution either when we can plug the gap completely.
Also, FWIW we can really enforce requirements as high as we want on the Chrome OS side. The amount of real cache used is printed in the log so if you want us to always have at least XX% free space in our RW mcache or room for at least Y files, I can write you a FAFT test to guarantee that.
Yes, and die() or whatever when we hit fallback path for Chrome OS. We should always be hitting the mcache. And making that happen will require a little more work/effort, but I don't think it's impossible to achieve.