Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
SMM doesn't link in cbmem functions, so it would never use the cache. I was thinking about S3 resume attacks, too.
On resume one should re-seed the cache from its source if one believes DRAM can be compromised.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) {
I can't cache in SMM as the cbmem functions aren't linked in.
OK. It's certainly possible to cache things in SMM as well if one wanted. However, that is separate from my suggestion about using the cbmem callbacks for seeding the cache.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@108 PS4, Line 108: /* Try to cache FMAP. Might fail if CBMEM isn't up yet. */
I tried to but with CAR_GLOBAL it's difficult to implement. Also does the SPI code for writing MRC cache depend on the device type? rdev vs mdev?
What do the MRC cache dependencies have to do with this code? I'm confused as to why you are asking.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@32 PS6, Line 32: static const struct cbmem_entry *fmap_cache CAR_GLOBAL; I think it's cleaner to have struct mem_region_device as your cache. One can check if it's initialized by region_device_sz(&cache.rdev) != 0.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@159 PS6, Line 159: } Why are we duplicating this sequence? A helper function can easily initialize the fmrd object that is passed into it instead of duplicating the logic. Similarly, one can apply the desired policy -- like not using cache in SMM, e.g.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@221 PS6, Line 221: Can't you check fmap_cache value and return early here?
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) { I think we should always re-seed the cache in cbmem. But one needs to ensure sizes match of the cbmem_entry and size of region directory.