Attention is currently required from: Paul Menzel, Julius Werner. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59497 )
Change subject: libpayload: Implement new CBFS access API ......................................................................
Patch Set 5:
(23 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59497/comment/031f869a_02ad6c6d PS1, Line 7: libpatload
libpayload
Done
File payloads/libpayload/Makefile:
https://review.coreboot.org/c/coreboot/+/59497/comment/c9d977a4_17606707 PS3, Line 294: $$(subst $(absobj)/,$(obj)/, \
Should probably factor out the generic commonlib support changes into a separate patch.
Done
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/67db4005_886eb6b6 PS3, Line 42: * ---------------------------------------------------------------------------
Since this is a new file, let's also use this opportunity to switch to SPDX here and get rid of the […]
Done
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/2b502b5b_4c334fc1 PS1, Line 137: if (_cbfs_alloc(name, _cbfs_default_allocator, &arg, &size, force_ro, type))
Since we don't have cbfs_alloc() and we don't intend to implement more complicated allocators, I thi […]
Done
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/8e3827d7_d94b8395 PS3, Line 48: #include <commonlib/bsd/cbfs_serialized.h>
Here too (in the headers) I think it would be better to just keep the legacy code and the new code c […]
Done
File payloads/libpayload/include/cbfs_glue.h:
PS3:
license
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/79594e83_ebd1e1e5 PS3, Line 32: static inline size_t cbfs_dev_offset(cbfs_dev_t dev)
nit: not sure you really need this... […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/4565138c_27a9fada PS3, Line 38: {
This also needs to do range checking. I think […]
Done. I accounted for the dev->offset too, as it might better narrow the range.
File payloads/libpayload/libcbfs/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/b3ed0512_1fdb2063 PS3, Line 12: bool
This needs a prompt text to be selectable.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/ba18decd_1815c248 PS3, Line 16: acrive
typo […]
Done
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/fa82ee86_a077499c PS1, Line 131: cbfs_err_t cbfs_walk(cbfs_dev_t dev,
Try just adding these files to libcbfs-srcs directly and bypass the includemakefiles mechanism. […]
Done in CB:59843
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/cf400a58_628c1b1f PS3, Line 27: * SUCH DAMAGE.
Same here
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/b493fabc_6ddf26ee PS3, Line 29: #define LIBPAYLOAD
Can get rid of all this cruft too. This file should never be built outside of libpayload nowadays.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/2af7e5bd_e58ddd06 PS3, Line 60: void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, bool is_ro);
Why are these functions not static? Then you don't need prototypes.
cbfs_boot_device_find_mcache() can be static, yes. But cbfs_get_boot_device() should be possible to mock, so it cannot be static. I changed the latter function to be defined similar way, as in the main coreboot source :)
https://review.coreboot.org/c/coreboot/+/59497/comment/4d4ec0bf_5802358c PS3, Line 91: cbfs_boot_device_find_mcache(&rw, false);
I think rather than making find_mcache() a separate function, it's easier to just set rw.dev. […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/72abb012_6aa7d3b2 PS3, Line 134: ERROR("Metadata hash mismatch for '%s'\n", name);
This doesn't print the name in coreboot because it's not a problem with the particular file, it's a […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/759ac23d_deb7fc16 PS3, Line 141: dev->size = be32toh(mdata->h.len);
cbfs_dev_t is not supposed to be an region_device equivalent and you shouldn't use it like that here […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/21007e94_b9b40820 PS3, Line 156: map
nit: Just because it's not really a mapping here I'd call this different... […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/e61f2c6d_cd0c13a7 PS3, Line 168: #if CONFIG(LP_LZ4)
You should be able to implement this as […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/ff21d475_127d57c3 PS3, Line 178: return 0;
Since decompression works all the same for these algorithms, and there'd be more code duplicated her […]
Done. I also moved checks for compression algorithms support to be executed before malloc or boot_device_read() calls to fail quicker. Please, let me know, what do you think about it :)
https://review.coreboot.org/c/coreboot/+/59497/comment/a0000e40_ef5dd2f9 PS3, Line 266: **********************************************************************************************/
I meant this too with moving to cbfs_legacy.c. […]
Done
File payloads/libpayload/libcbfs/cbfs_mcache.c:
PS3:
This, too, should be pulled in from commonlib.
Done
File payloads/libpayload/tests/libcbfs/cbfs-lookup-test.c:
PS1:
Most of this test tests the code in commonlib/bsd/, so if we're reusing that I don't know if it's th […]
Done
-
Jakub Czapiga (Code Review)