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 7:
(16 comments)
File payloads/libpayload/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/cdb331a0_93f5665b 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... […]
Right. the name is pretty straightforward, and it is also in coreboots <cbfs.h> :)
https://review.coreboot.org/c/coreboot/+/59497/comment/d34742d3_452095de PS7, Line 40: #include <cbfs_boot_device.h>
Please move this to the includes at the top of the file.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/9e955468_5559feaa PS7, Line 120: >
Even though file sizes shouldn't be zero, I would write >= here to have the proper symmetry with the […]
Done
File payloads/libpayload/libcbfs/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/26f7fec2_732cee99 PS7, Line 19: bool "Enable CBFS files verification"
nit: I'd say "Enable CBFS verification" to match coreboot.
Done
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/14e0c1e8_3486a58f 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.
Eh... Reading be difficult... Sorry
https://review.coreboot.org/c/coreboot/+/59497/comment/4d8b5cd8_d035ccd9 PS5, Line 34: if (!rw.mcache_size) {
Well, mcache_size is always used to determine whether the mcache is valid or not, and that doesn't r […]
I do not know, what I was thinking. It should be obvious. Thanks
https://review.coreboot.org/c/coreboot/+/59497/comment/777bdc3b_5c7b2dab PS5, Line 49: if (!ro.mcache_size) {
Don't think you need to check this either.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/917c7455_b6316c08 PS5, Line 96: void *file_hash_unused
I'm leaving it like that for now, because I'm working on compiling vboot_reference lib with libpaylo […]
Done
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/96480af1_e15f6572 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.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/620871c7_6d278862 PS7, Line 43: ro.dev.offset = (size_t)phys_to_virt(ro.dev.offset);
same
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/91dce732_b718e36e PS7, Line 109: size
nit: I'd maybe call it in_size for clarity
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/e1c5229e_bdc701e9 PS7, Line 123: return size;
Careful with the early return now... […]
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/7340cdd9_6f9c9447 PS7, Line 131: if (cbfs_file_hash_mismatch(buffer, size, mdata))
This must happen after the boot_device_read(), obviously. […]
Thank you for this implementation. It looks a lot nicer than mine. Regarding the verification: It was not ready yet. I pushed this version of patch too early.
https://review.coreboot.org/c/coreboot/+/59497/comment/9495cb8b_999961ee PS7, Line 139: ERROR("Failed to read contents of file\n");
Need to free() scratch here.
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/6d42be3f_dccba6e7 PS7, Line 185: {
Forgot to delete the brace
Done
https://review.coreboot.org/c/coreboot/+/59497/comment/85baa7de_51f3b827 PS7, Line 197: NULL
You gotta actually pass it if you want to use it, though.
Done