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 4:
(3 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 should be at boot time because there's nothing else that could access DRAM. […]
It really depends on when one wants to access it. If it's only during the boot sequence then one should be explicitly writing the code to handle that. Right now it's all interleaved in the common which I don't think is correct.
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)) { This is all quite messy. Please use a local variable or just remove the CAR_GLOBAL decorations. It'd be cleaner to also break up this logic into some helper functions which would also contain the policies (along w/ comments) you are wanting to do.
e.g. I think you want to use the cache except in SMM? And then when you aren't in SMM you want this storage to be in cbmem. Why aren't you using cbmem's callbacks to see this at the correct time?
https://github.com/coreboot/coreboot/blob/master/src/include/cbmem.h#L119
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.