Attention is currently required from: Jakub Czapiga. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59497 )
Change subject: libpayload: Implement new CBFS access API ......................................................................
Patch Set 3:
(20 comments)
File payloads/libpayload/Makefile:
https://review.coreboot.org/c/coreboot/+/59497/comment/bb7e86f3_31880508 PS3, Line 294: $$(subst $(absobj)/,$(obj)/, \ Should probably factor out the generic commonlib support changes into a separate patch.
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/7025f4f6_6f2dc8a7 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 copyrights (they belong to cbfs_legacy.h now).
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/113d1245_f9facf2c 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 completely separate. (I guess if there are naming clashes you may need to include cbfs_serialized.h here in the old header and remove the duplicates, but at least this header shouldn't be needed for any of the new code to make it easier to eventually just delete it.)
File payloads/libpayload/include/cbfs_glue.h:
PS3: license
https://review.coreboot.org/c/coreboot/+/59497/comment/170f5d1f_ff098f26 PS3, Line 32: static inline size_t cbfs_dev_offset(cbfs_dev_t dev) nit: not sure you really need this... the offset is only accessed by libpayload (or payload) which can depend on the internal details of this header. The read() and size() accessors are only abstracted so the commonlib/bsd code can call them.
https://review.coreboot.org/c/coreboot/+/59497/comment/506fa10a_9de74ca9 PS3, Line 38: { This also needs to do range checking. I think
if (offset + size < offset || offset + size > dev->size) return -CB_ERR_ARG;
should be enough.
File payloads/libpayload/libcbfs/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/18f092a8_d900c6eb PS3, Line 12: bool This needs a prompt text to be selectable.
https://review.coreboot.org/c/coreboot/+/59497/comment/68d34b2e_f73fd409 PS3, Line 16: acrive typo
Also, should not reference cbfs_boot_locate (see CB:59682).
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/2de8ab49_c7715a98 PS1, Line 131: cbfs_err_t cbfs_walk(cbfs_dev_t dev,
Ok, so I did it by changing the macro in the main Makefile. […]
Try just adding these files to libcbfs-srcs directly and bypass the includemakefiles mechanism. In order to control where they're put in the build path, add a special case to the src-to-objs function that will map $(coreboottop)/commonlib/bsd (maybe useful to define an extra shorthand for that, e.g. $(commonlib_bsd)) to $(obj)/commonlib/bsd. It looks like src-to-obj was only partially copied from coreboot to libpayload and isn't actually used in create_cc_template, so you'll need to fix that too for this to work.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/ce8fc90f_7d576b6d PS3, Line 27: * SUCH DAMAGE. Same here
https://review.coreboot.org/c/coreboot/+/59497/comment/36899bc7_e7a02f51 PS3, Line 29: #define LIBPAYLOAD Can get rid of all this cruft too. This file should never be built outside of libpayload nowadays.
https://review.coreboot.org/c/coreboot/+/59497/comment/7fb2660e_959ff3fe 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.
https://review.coreboot.org/c/coreboot/+/59497/comment/5b759f4e_1b1c837b 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.mcache and mcache_size here directly?
https://review.coreboot.org/c/coreboot/+/59497/comment/0b17bad2_700bd5fa 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 problem with the whole CBFS. It would maybe make sense to print the FMAP section name here, but that's a bit hard to get at this point. So just don't print anything (the user will see the offsets in earlier log messages and can figure it out from there if necessary).
https://review.coreboot.org/c/coreboot/+/59497/comment/931f8fed_200fe60b 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... it should represent a whole CBFS, not just a file.
I would just make this function return the offset as an ssize_t, and the caller can easily read the size out of mdata itself if needed.
https://review.coreboot.org/c/coreboot/+/59497/comment/b52c1c38_8e9d0514 PS3, Line 156: map nit: Just because it's not really a mapping here I'd call this different... maybe `scratch`?
https://review.coreboot.org/c/coreboot/+/59497/comment/fe682b6a_74992e5c PS3, Line 168: #if CONFIG(LP_LZ4) You should be able to implement this as
if (!CONFIG(LP_LZ4) return 0;
like in coreboot. Prefer C if() over preprocessor #if where possible.
https://review.coreboot.org/c/coreboot/+/59497/comment/36939de8_35c3dd45 PS3, Line 178: return 0; Since decompression works all the same for these algorithms, and there'd be more code duplicated here, maybe it would be cleaner to rearrange this function a bit:
if (compression == CBFS_COMPRESS_NONE) { ...NONE case... return ...; }
map = malloc(in_size); ...everything including libpayload_boot_device_read()...
switch (compression) { case CBFS_COMPRESS_LZMA: if (!CONFIG(LP_LZMA) break; out_size = ulzman(...); break; ... }
free(map); return out_size;
https://review.coreboot.org/c/coreboot/+/59497/comment/ff4e7a69_cb51e3b3 PS3, Line 266: **********************************************************************************************/ I meant this too with moving to cbfs_legacy.c. Basically, move absolutely everything belonging to the old API to the side and start from scratch.
File payloads/libpayload/libcbfs/cbfs_mcache.c:
PS3: This, too, should be pulled in from commonlib.
-
Julius Werner (Code Review)