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 4:
(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? It should be at boot time because there's nothing else that could access DRAM. I guess it may be an issue for x86 SMM if you're worried that the OS would use this as an attack vector into SMM. Maybe ask the x86 guys if they think this is a concern (or if there's any platform-specific way they could force this memory to be read-only to the OS), or maybe just skip the caching path for ENV_SMM for now.
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.
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() below can't share it. Why not just make find_fmap_directory() do a mem_region_device_ro_init() on the CBMEM area if it exists?
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().