Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39304 )
Change subject: cbfs: Simplify load/map API names, remove type arguments ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39304/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39304/4//COMMIT_MSG@24 PS4, Line 24: known
Some of those are only known by way of Kconfig check. […]
Sorry, I'm not sure what you're saying... are you just arguing that I should add a cbfs_unmap() to all the existing cbfs_map() (even the ones in Intel FSP drivers or src/soc code for x86 platforms?), or are you saying you don't like this approach at all and want to keep the map_with_leak() name? I'm just trying to make this API less of a mouthful because it is used all over the place, and to align it with the rdev_mmap()/rdev_munmap() naming scheme.
https://review.coreboot.org/c/coreboot/+/39304/4/src/include/cbfs.h File src/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/39304/4/src/include/cbfs.h@27 PS4, Line 27: often
"can't". Without knowing which rdev the mapping is connected to then things fall over. […]
This comment is just to remind people about the behavior of mem_pool_alloc()/mem_pool_free() (which is in practice the only non-trivial mapping backend we have). As mentioned elsewhere I want to rely on the assumption that both RO and RW CBFS are always chained from boot_device_ro().
https://review.coreboot.org/c/coreboot/+/39304/4/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/39304/4/src/lib/cbfs.c@96 PS4, Line 96: cares about which chained subregion something was mapped from. */
I don't think this is always true in practice. […]
See discussion in https://review.coreboot.org/c/coreboot/+/39327/3/src/lib/cbfs.c#62
https://review.coreboot.org/c/coreboot/+/39304/4/src/security/vboot/ec_sync.... File src/security/vboot/ec_sync.c:
https://review.coreboot.org/c/coreboot/+/39304/4/src/security/vboot/ec_sync.... PS4, Line 397:
Why wouldn't we unamp here?
We're returning the pointer to the caller, its contents must remain valid. The vboot API has no way to return this memory so this is one of those cases where we need to leak a mapping. It's just a hash so it shouldn't be a big deal. Added comment to document.