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 6:
(10 comments)
Patchset:
PS6: Sorry, it's basically good but then I noticed a bunch of other stuff again...
File payloads/libpayload/libc/fmap.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/de82ef94_014c185a PS6, Line 34: #include <fmap_serialized.h> While you're here would you mind also getting rid of this version of the file and have libpayload use the one in commonlib/bsd instead?
https://review.coreboot.org/c/coreboot/+/59494/comment/6fd5d2e4_515954d1 PS6, Line 41: int This should then also be cb_err_t
https://review.coreboot.org/c/coreboot/+/59494/comment/39efc3fc_d54608a3 PS6, Line 50: 0 CB_SUCCESS
https://review.coreboot.org/c/coreboot/+/59494/comment/38b56ea1_73f0d13e PS6, Line 53: -1 Probably not worth making a new type for this, so maybe just reuse CB_CBFS_NOT_FOUND here?
https://review.coreboot.org/c/coreboot/+/59494/comment/83a4005a_d8b8d444 PS6, Line 73: int I'm actually surprised the compiler doesn't complain, but this should match the declaration (cb_err_t).
File payloads/libpayload/tests/libc/fmap_locate_area-test.c:
https://review.coreboot.org/c/coreboot/+/59494/comment/250f5c80_a23a60d9 PS6, Line 14: extern struct fmap *_fmap_cache; This no longer belongs here
https://review.coreboot.org/c/coreboot/+/59494/comment/7e4fa92e_3a8eab72 PS6, Line 98: _fmap_cache = NULL; Why not use reset_fmap_cache() for these?
https://review.coreboot.org/c/coreboot/+/59494/comment/d0c011ac_d104e5ba PS6, Line 100: /* Different number of areas in fmap_cache and fmap_offset */ Okay so now the comment is correct but I'm still not sure why we want to test this? I would say the fact that the FMAP cache pointer is again cached locally in the compilation unit is a total implementation detail. If someone changed the implementation to not do that, that doesn't mean it's suddenly broken. So I don't know why we'd want the test to fail in that case.
https://review.coreboot.org/c/coreboot/+/59494/comment/e273ec3e_af83c78c PS6, Line 101: lib_sysinfo.fmap_offset = (uint64_t)fmap_buffer; This is super weird anyway. fmap_offset is an offset on flash, here you're treating it like a pointer to memory?
You don't have a real flash in this test, nor does the whole FMAP implementation ever try to access the flash (or read fmap_offset). fmap_offset is just completely irrelevant to this test.