Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 6:
(4 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. […]
Don't we resume into DRAM (relocatable ramstage) anyway? Or am I confusing things there?
Generally, for the OS and lower, there's no point in worrying about a DRAM compromise between suspend/resume. I don't know if there is for coreboot.
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@159 PS6, Line 159: }
Why are we duplicating this sequence? A helper function can easily initialize the fmrd object that i […]
If the global to store the cache is a struct mem_region_device, we shouldn't even need a helper... we could just put this into the top of find_fmap_directory().
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 then we're loading it from flash 3 times each boot (in addition to all the times we need it in pre-RAM code)? Why?
If you're worried about S3 resume, we could check for ENV_ROMSTAGE here, but for the other two stages I see no reason to reread it?
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@236 PS6, Line 236: printk(BIOS_ERR, "Failed to read FMAP into CBMEM\n"); This should probably die() because otherwise you'd be running around with a cache full of invalid data (or you'd have to cbmem_entry_remove() it again).