Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 5:
(6 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?
It really depends on when one wants to access it. […]
SMM doesn't link in cbmem functions, so it would never use the cache. I was thinking about S3 resume attacks, too.
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@32 PS4, Line 32: static struct fmap *fmap_c CAR_GLOBAL;
I'm not a CAR_GLOBAL expert but I think(?) these days you don't need it for cases like this anymore.
Leaving this in as there are still FSP1.0 platforms and AMD? that needs CAR_GLOBAL.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) {
This is all quite messy. Please use a local variable or just remove the CAR_GLOBAL decorations. […]
I can't cache in SMM as the cbmem functions aren't linked in.
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. */
Why handle this here instead of inside find_fmap_directory()? That way the fmap_find_region_name() b […]
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?
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@111 PS4, Line 111: car_set_var(fmap_c, cbmem_add(CBMEM_ID_FMAP, s));
Probably cleaner to create the cache in a ROMSTAGE_CBMEM_INIT_HOOK().
Done
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@141 PS4, Line 141: rdev_munmap(&fmrd, area);
This is not correct now since you conditionalized where area is being set.
Done