Attention is currently required from: 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 2:
(15 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/fffbfcce_01d5fcf2 PS1, Line 56: **********************************************************************************************/
I think it might be easier (for review and just generally keeping track of things) to just put all o […]
Done. The legacy API is now in the include/cbfs_legacy.h, and it is included at the bootom of include/cbfs.h
https://review.coreboot.org/c/coreboot/+/59497/comment/f85305c5_41677d0d 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?
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/2342a0c4_c8239569 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.
Done
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/76a3e712_28f27ec1 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.
I forgot to remove it. Done :)
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/63a4d8aa_584e6721 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 […]
Hmm. It seems reasonable. If one does not want the CBFS MCache, then they can disable it in the coreboot config.
https://review.coreboot.org/c/coreboot/+/59497/comment/090a0be6_c3d39b21 PS1, Line 65: (void *)
Use phys_to_virt() whenever accessing pointers from lib_sysinfo (some payloads relocate themselves a […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/8822b0be_77237f2a PS1, Line 80: cbfs_get_boot_device(true);
This was only needed for mcache building in coreboot, shouldn't apply here.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/ea647100_d9d99fa5 PS1, Line 83: cbfs_dev_size
nit: a bit odd you wouldn't just use rw.dev. […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/45d8b0d3_395e4ae0 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.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/a2d82992_cb385db7 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 […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/816884ea_133d8d24 PS1, Line 96: cbfs_boot_device_find_mcache(&ro, false);
Same here. […]
The MCache will not "re-generate" on every call. This function check if it is set, and if not, then it performs its actions to find the MCache. But you are right, this call should be below the ro.dev.* sets.
https://review.coreboot.org/c/coreboot/+/59497/comment/aa68a26b_787d811d PS1, Line 102: uint32_t size = 0;
nit: Don't really need these, can just pass pointers &ro.dev.offset / &ro.dev.size directly.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/89fc3431_399b2b0f 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 […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/fb979c13_afd9e60d PS1, Line 131: cbfs_err_t cbfs_walk(cbfs_dev_t dev,
No, sorry, not like this. […]
Ok, so I did it by changing the macro in the main Makefile. I didn't want to create symlink to the commonlib/bsd in the libpayload, because I am nos sure, what policy is there on weak/hard links to directories. Now commonlib will be compiled under payloads/libpayload/build/<abs-path-to-commonlib/bsd/*.c>. Currently I do not have another idea, how to solve it in a different way.
File payloads/libpayload/tests/libcbfs/Makefile.inc:
PS1:
license
Done