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 8:
(7 comments)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/69b26097_da731833 PS8, Line 211: config CBFS nit: maybe makes sense to just move this into libcbfs/Kconfig as well now?
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/176a30df_ad135bb3 PS5, Line 87: #define CBFS_ATTRIBUTE_ALIGN 4
Done
You're still adding METADATA_MAX_SIZE and a _Static_assert() to cbfs_core.h which I don't think are really necessary there.
File payloads/libpayload/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/f18839ed_1feafde2 PS3, Line 38: {
Oh right, thanks for paying attention!
This, too, this is still open. (Also, I don't know how that second comment slipped in there... I wrote that one first, then I looked at it more closely, and then I wrote the first comment about why this is was bad idea. The second comment must have survived in some other open Gerrit tab and slipped in later or something. Anyway, removing dev->offset from that check is the correct way.)
File payloads/libpayload/libcbfs/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/903331b9_e0b3eafe PS8, Line 16: RO (COREBOOT) region if it isn't available in the active RW region. This should mention that this only makes sense if CONFIG_VBOOT is enabled in coreboot. (Unfortunately we can't just `depends on VBOOT` here, because with the way vboot works right now it would be reasonable to enable vboot in coreboot but disable it in libpayload.)
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/a1624e37_4c119cc2 PS3, Line 60: void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, bool is_ro);
Hmm, well... […]
Sorry, I should have unresolved this, it kinda slipped through now.
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/d4ac07d2_ca2aee3e PS8, Line 111: ERROR("Buffer allocation failed"); nit: would be nice to add mdata->h.filename to all these errors
https://review.coreboot.org/c/coreboot/+/59497/comment/2e0d56b5_18896503 PS8, Line 172: if (!size_inout || *size_inout < out_size) nit: might want to print an error here too?
-
Julius Werner (Code Review)