Attention is currently required from: Julius Werner. Jakub Czapiga 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/6a74812b_a1005a9a 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 […]
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/ec704896_e7a10eed 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 […]
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/ab5d0c8a_2a9446d4 PS5, Line 56: _fmap_check_signature
nit: just in general, try to follow the pattern of "function sounds like an action" -> 0 success, ne […]
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/dda4646f_32d10a23 PS5, Line 61: _
I'm not sure why you're prefixing everything with an underscore here... […]
Thanks for clarifying that :)
https://review.coreboot.org/c/coreboot/+/59494/comment/7458956a_ec58e2d3 PS5, Line 63: if (_fmap_cache)
nit: you don't really need to check it on both ends
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/d7f17353_090b2196 PS5, Line 69: _fmap_cache = (struct fmap *)lib_sysinfo.fmap_cache;
phys_to_virt()?
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/b20e7300_7bc15dd6 PS5, Line 73: /* Load FMAP from boot media to internal cache */
outdated comment
Done
File payloads/libpayload/tests/include/mocks/boot_device.h:
https://review.coreboot.org/c/coreboot/+/59494/comment/5144e961_f80c53a6 PS5, Line 8: libpayload_boot_device_read
Looks like you meant to delete this file?
Done
File payloads/libpayload/tests/libc/fmap_locate_area-test.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/033daaba_625d87e2 PS5, Line 101: /* Different contents of fmap_cache and fmap_offset */
This test doesn't test what the comments suggest it does.
Done
https://review.coreboot.org/c/coreboot/+/59494/comment/5a1c4224_f2f58f63 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 ( […]
Ok, I see your point. If there are two areas with same name, then it's implementation-dependent, so basically UB from user's point of view.
File payloads/libpayload/tests/mocks/boot_device.c:
PS5:
This file too?
Done
-
Jakub Czapiga (Code Review)