Attention is currently required from: Jakub Czapiga. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59497 )
Change subject: libpatload: Implement new CBFS access API ......................................................................
Patch Set 1:
(17 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/ba08e7ee_14ddca40 PS1, Line 56: **********************************************************************************************/ I think it might be easier (for review and just generally keeping track of things) to just put all of this into separate files (e.g. this could go in a <cbfs_new.h> that is just chain-included from here, and then eventually when we remove the old stuff we rename that to <cbfs.h> -- or the other way round, move the old stuff to <cbfs_legacy.h> and chain-include it for now).
https://review.coreboot.org/c/coreboot/+/59497/comment/5f83cf01_3c4c60ee PS1, Line 77: * cbfs_unmap() after it is done using the mapping to free up the memory, if possible. Rather than duplicate all this, maybe just refer to the coreboot header for documentation?
https://review.coreboot.org/c/coreboot/+/59497/comment/bab4f2da_dd00f07c PS1, Line 79: * void cbfs_alloc(char *name, cbfs_allocator_t allocator, void *arg, size_t *size_out): Loads We don't have that one here.
https://review.coreboot.org/c/coreboot/+/59497/comment/c0e8a3cc_5159e2c0 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 think this should all be simplified... e.g. you could have a common primitive like _cbfs_load(const char *name, void *buf, size_t *size_inout, bool force_ro), and when |buf| is NULL that just means it should malloc() a buffer instead.
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/5b8ad8e3_e05de37a PS1, Line 53: /* This function needs to be implemented to use _cbfs_alloc() based API */ What does this mean? This is all supposed to map back to libpayload_boot_device_read(), of course.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/0985ae72_b1099799 PS1, Line 58: if (CONFIG(LP_NO_CBFS_MCACHE)) I'm not sure it's worth having a Kconfig here, I'd just say if the mcache exists in CBMEM it should use it, if not then not.
https://review.coreboot.org/c/coreboot/+/59497/comment/6577f2bb_d63409e5 PS1, Line 65: (void *) Use phys_to_virt() whenever accessing pointers from lib_sysinfo (some payloads relocate themselves after parsing the coreboot tables, and set up virtual memory mappings).
https://review.coreboot.org/c/coreboot/+/59497/comment/95c6545c_e48b296e PS1, Line 80: cbfs_get_boot_device(true); This was only needed for mcache building in coreboot, shouldn't apply here.
https://review.coreboot.org/c/coreboot/+/59497/comment/dde18333_c22ad519 PS1, Line 83: cbfs_dev_size nit: a bit odd you wouldn't just use rw.dev.size here if you do it below
https://review.coreboot.org/c/coreboot/+/59497/comment/54ee7b2b_cd787c9b PS1, Line 84: if (lib_sysinfo.cbfs_offset != 0 && lib_sysinfo.cbfs_size != 0) { This should never fail. If vboot is disabled in coreboot, these will just point to the RO CBFS.
https://review.coreboot.org/c/coreboot/+/59497/comment/bc257348_4ee73568 PS1, Line 92: cbfs_boot_device_find_mcache(&rw, false); Shouldn't this be inside the !cbfs_dev_size() check (mcache would also be cached already if the rest is cached)?
https://review.coreboot.org/c/coreboot/+/59497/comment/b7cef46c_739045d5 PS1, Line 96: cbfs_boot_device_find_mcache(&ro, false); Same here. There are specific reasons in coreboot for why it needs to try to regenerate the mcache again on every call (see the comment there), reasons which don't apply here.
https://review.coreboot.org/c/coreboot/+/59497/comment/5f25e389_7c67156e PS1, Line 102: uint32_t size = 0; nit: Don't really need these, can just pass pointers &ro.dev.offset / &ro.dev.size directly.
https://review.coreboot.org/c/coreboot/+/59497/comment/8ee6e146_784be9fd PS1, Line 105: die("Cannot locate primary CBFS"); This should probably be an error that's passed back up to the caller for libpayload, rather than die().
https://review.coreboot.org/c/coreboot/+/59497/comment/c63d99bd_604a3d68 PS1, Line 131: cbfs_err_t cbfs_walk(cbfs_dev_t dev, No, sorry, not like this. The point of commonlib/bsd/ was that all that code could be directly included/linked into libpayload, so that we no longer need to have all this awful duplication. Please change the libpayload Makefile so that directly inclusion/linking is possible (only for commonlib/bsd/, not for the rest of commonlib/).
File payloads/libpayload/tests/libcbfs/Makefile.inc:
PS1: license
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 that useful to duplicate the whole test? I think you only need to test the aspects that aren't covered by coreboot's tests yet.
-
Julius Werner (Code Review)