Attention is currently required from: Jakub Czapiga. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59494 )
Change subject: libpayload/libc/fmap: Implement new FlashMap API ......................................................................
Patch Set 5:
(11 comments)
File payloads/libpayload/libc/fmap.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/3574aaa1_19042eed PS5, Line 38: /* Private fmap cache. Left non-static to make testing easier. */ I thought we didn't do things like this? Can't you just use the #include trick? (Exporting a symbol from the compilation unit can preclude optimizations that the compiler might otherwise be able to do... probably not much in this case, but I'd rather not start making test-only concessions in principle if we can avoid them.)
https://review.coreboot.org/c/coreboot/+/59494/comment/e8732399_ad926d1c PS5, Line 43: for (size_t i = 0; i < fmap->nareas; ++i) { Would be good to sprinkle le32toh() everywhere applicable here, like has recently been added to the coreboot version.
https://review.coreboot.org/c/coreboot/+/59494/comment/1389ba45_f43b8e1a PS5, Line 56: _fmap_check_signature nit: just in general, try to follow the pattern of "function sounds like an action" -> 0 success, negative value error; "function sounds like a question" -> true means yes, false means no. So I would call this something like is_signature_valid() or fmap_signature_valid().
https://review.coreboot.org/c/coreboot/+/59494/comment/8a46289d_3d3b2b14 PS5, Line 61: _ I'm not sure why you're prefixing everything with an underscore here... generally, we don't prefix things with underscores unless there's a special reason. I've used underscores in coreboot's <cbfs.h> for a few things that need to be in the public header for use by inlines but shouldn't be considered a public API that can be called directly... but here these functions are already static, they are already local to the compilation unit, you don't need anything else to signify that they're internal.
https://review.coreboot.org/c/coreboot/+/59494/comment/b683311e_c49de534 PS5, Line 63: if (_fmap_cache) nit: you don't really need to check it on both ends
https://review.coreboot.org/c/coreboot/+/59494/comment/16908965_9b6f19fe PS5, Line 69: _fmap_cache = (struct fmap *)lib_sysinfo.fmap_cache; phys_to_virt()?
https://review.coreboot.org/c/coreboot/+/59494/comment/a3ca7ba6_4b6b8c3d PS5, Line 73: /* Load FMAP from boot media to internal cache */ outdated comment
File payloads/libpayload/tests/include/mocks/boot_device.h:
https://review.coreboot.org/c/coreboot/+/59494/comment/c58f0009_ec8839ec PS5, Line 8: libpayload_boot_device_read Looks like you meant to delete this file?
File payloads/libpayload/tests/libc/fmap_locate_area-test.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/4ff37d7c_cdd253e8 PS5, Line 101: /* Different contents of fmap_cache and fmap_offset */ This test doesn't test what the comments suggest it does.
https://review.coreboot.org/c/coreboot/+/59494/comment/ae02a34c_0a0a7160 PS5, Line 112: static void test_fmap_locate_area_repeated_area(void **state) Honestly don't think we should test this, because it's an invalid FMAP and the result is undefined (e.g. scanning the FMAP back to front wouldn't be an invalid implementation, but it would break this test).
File payloads/libpayload/tests/mocks/boot_device.c:
PS5: This file too?