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:
(13 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/a88835d5_a39bcf2d PS5, Line 15: /* For documentation look in the main coreboot cbfs header files in paths:
I think you can just refer to <cbfs.h> directly, that's where the main API is declared. […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/211b7162_26cab954 PS5, Line 28: order where possible, since mapping backends often don't support more complicated cases */
The second sentence is not true for libpayload.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/ca070e92_4d25245e PS5, Line 35: /* Defined in include/cbfs_glue.h */
I think it would be fine to just include <cbfs_glue.h> here, too.
I didn't want to include cbfs_glue.h here, because it defines ERROR, LOG and DEBUG macros for commonlib/bsd/cbfs_private.h, and it might collide with payloads defines. I introduces the include/cbfs_boot_device.h to deal with it. cbfs_glue.h and cbfs.h can both include it without collisions.
https://review.coreboot.org/c/coreboot/+/59497/comment/4925dcbd_b7332bd5 PS5, Line 43: ssize_t _cbfs_boot_lookup(const char *name, bool force_ro, union cbfs_mdata *mdata);
You only need this if you also duplicate the cbfs_get_type()/cbfs_get_size()/cbfs_file_exists() APIs […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/415021ec_a1724dac PS5, Line 63: void *ret = _cbfs_load(name, buf, &size, false);
nit: why not just […]
Done
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/db2d3aac_58482e8f PS5, Line 87: #define CBFS_ATTRIBUTE_ALIGN 4
Not sure why some of this needs to be added here? Since this file is supposed to be deleted anyway, […]
Done
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/ae5a25b3_7f863039 PS5, Line 10: #define CB_CBFS_CORE_WITH_LZ4
Sorry, I meant these things too when I said "get rid of the cruft". […]
Sorry, I forgot to remove these defines after I moved legacy implementation to its own file. Thanks for noticing :)
https://review.coreboot.org/c/coreboot/+/59497/comment/521c8b63_4494c601 PS5, Line 23: #ifndef __SMM__
This doesn't belong here either.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/f37801ae_f4acbef6 PS5, Line 34: if (!rw.mcache_size) {
I don't think there's a point checking this here?
I'd leave it here. Without it if payload will perform relocation and will fill `virtual_offset` (arch/virtual.h), NULL from `cbfs_*_mcache_offset` will be translated to `virtual_offset`, and we do not want that.
https://review.coreboot.org/c/coreboot/+/59497/comment/165fdf91_c4f628ec PS5, Line 45: !lib_sysinfo.fmap_offset
Checking this here shouldn't be needed?
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/63420bd8_6fe7a1ec PS5, Line 170: *size_inout = MIN(size, *size_inout);
You shouldn't need the MIN() here... if the buffer is too small, we need to fail anyway. […]
Done. I also moved this assignment after call to `cbfs_load_and_decompress()`, because it should have no effect if the file will not be decompressed correctly.
https://review.coreboot.org/c/coreboot/+/59497/comment/0788dbf8_646be048 PS5, Line 172: loc = malloc(size);
nit: rather than introducing `loc` I think you can just do buf = malloc(... […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/f0267e43_0fedf88b PS5, Line 182: if (cbfs_load_and_decompress(offset, size, loc, size, compression, NULL) != size) {
Be careful with the size parameters here. The first size should be the *compressed* file size in CBFS (i.e. be32toh(mdata.h.len))
Fixed :)
he second one should be the size of the allocated buffer (I guess you can use `size` here for simplicity, even though that might be smaller, since we know it's gonna be enough anyway.
I think, that after adding if-statement for `size_inout` it will be safe enough. As you said, we will know, that `size` bytes should suffice.
-
Jakub Czapiga (Code Review)