Attention is currently required from: Jakub Czapiga, Paul Menzel. 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 7:
(15 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/935b7e17_2771a67a PS7, Line 24: /* Removes a previously allocated CBFS mapping. */ (It's kinda weird that this has a documentation comment when everything else doesn't, though... I'd say this also counts under "look in coreboot's <cbfs.h>".)
https://review.coreboot.org/c/coreboot/+/59497/comment/4f2a98a4_56e4ffbd PS7, Line 40: #include <cbfs_boot_device.h> Please move this to the includes at the top of the file.
https://review.coreboot.org/c/coreboot/+/59497/comment/4c129a81_6995f891 PS7, Line 120: > Even though file sizes shouldn't be zero, I would write >= here to have the proper symmetry with the "failed" tests in the functions above.
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/f8e9d3bc_49e98e04 PS5, Line 35: /* Defined in include/cbfs_glue.h */
I didn't want to include cbfs_glue. […]
Oh, okay, that's a good point.
File payloads/libpayload/libcbfs/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/0ef12ac4_ab7d9c53 PS7, Line 19: bool "Enable CBFS files verification" nit: I'd say "Enable CBFS verification" to match coreboot.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/47c8190c_bba6a77f PS5, Line 10: #define CB_CBFS_CORE_WITH_LZ4
You can just always include <lzma.h> and <lz4.h>, you don't need to guard that.
https://review.coreboot.org/c/coreboot/+/59497/comment/dc2d27d3_75c3110f PS5, Line 34: if (!rw.mcache_size) {
I'd leave it here. […]
Well, mcache_size is always used to determine whether the mcache is valid or not, and that doesn't relocate.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/83e910e0_6454c586 PS7, Line 28: rw.dev.offset = (size_t)phys_to_virt(lib_sysinfo.cbfs_offset); Nonono, phys_to_virt() is for pointers, this is a flash offset.
https://review.coreboot.org/c/coreboot/+/59497/comment/b889fd6e_d467405e PS7, Line 43: ro.dev.offset = (size_t)phys_to_virt(ro.dev.offset); same
https://review.coreboot.org/c/coreboot/+/59497/comment/bb6807bd_78c86a4c PS7, Line 109: size nit: I'd maybe call it in_size for clarity
https://review.coreboot.org/c/coreboot/+/59497/comment/544c2392_43966d6f PS7, Line 123: return size; Careful with the early return now... if you already introduce hashing, then you must have it here as well.
https://review.coreboot.org/c/coreboot/+/59497/comment/bf3c6b8a_e8228e09 PS7, Line 131: if (cbfs_file_hash_mismatch(buffer, size, mdata)) This must happen after the boot_device_read(), obviously.
Maybe this whole thing can be written nicely as:
void *load = buffer;
if (compression != CBFS_COMPRESS_NONE) { load = malloc(size); if (!load) { ERROR(...); return 0; } }
if (boot_device_read(load, offset, size) != size) { ERROR(...); goto out; }
if (cbfs_file_hash_mismatch(load, size, mdata)) goto out;
switch (compression) { case CBFS_COMPRESS_NONE: out_size = size; break; case CBFS_COMPRESS_LZ4: if (!CONFIG(LP_LZ4)) goto out; out_size = ulz4fn(...); break; case ... }
out: if (load != buffer) free(load); return out_size;
(This is omitting the buffer_size < size check for COMPRESS_NONE which is already checked by the only caller in this case.)
https://review.coreboot.org/c/coreboot/+/59497/comment/509c537f_a635dadd PS7, Line 139: ERROR("Failed to read contents of file\n"); Need to free() scratch here.
https://review.coreboot.org/c/coreboot/+/59497/comment/b6fe7b11_163ca487 PS7, Line 185: { Forgot to delete the brace
https://review.coreboot.org/c/coreboot/+/59497/comment/b0706503_dd32055e PS7, Line 197: NULL You gotta actually pass it if you want to use it, though.