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 10:
(4 comments)
File payloads/libpayload/Kconfig:
https://review.coreboot.org/c/coreboot/+/59497/comment/f20afe67_18e8cb33 PS8, Line 211: config CBFS
nit: maybe makes sense to just move this into libcbfs/Kconfig as well now?
As you suggested in CB:60080, I moved VBOOT_LIB here. I think, that both should be in same place, as both enable building additional libraries.
File payloads/libpayload/include/cbfs_core.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/086779f2_2a91b80b PS5, Line 87: #define CBFS_ATTRIBUTE_ALIGN 4
You're still adding METADATA_MAX_SIZE and a _Static_assert() to cbfs_core. […]
Ok, I think, that I removed all redefs.
File payloads/libpayload/include/cbfs_glue.h:
https://review.coreboot.org/c/coreboot/+/59497/comment/2eb943ab_7b3e391a PS3, Line 38: {
This, too, this is still open. (Also, I don't know how that second comment slipped in there... […]
Thanks for explaining it. I didn't look at it that way. I believe, we had a problem with overflow checking before. GCC will optimize-out `offset + size < offset and replace as false, for signed types. If offset and size will be unsigned, there should be no problem.
But, should not this code also perform checks including dev->offset?
File payloads/libpayload/libcbfs/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59497/comment/27eb89ad_1ddd934c PS3, Line 60: void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, bool is_ro);
Sorry, I should have unresolved this, it kinda slipped through now.
I marked it as static. Please take a look at current solution (in tests too).
-
Jakub Czapiga (Code Review)