Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 11:
(4 comments)
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/coreboot... PS9, Line 205: 0x0034
Ack
Actually, I agree that putting this at the bottom of this list (i.e. below CB_TAG_VPD) would probably be better, so that at least that part is ordered. It's already hard enough to figure out what the highest used tag is, no need to make it even harder.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/libc/coreboot.c@... PS9, Line 91: struct
OK but start small, as I am not an authority in coreboot at all. […]
There are no hard rules on const. It's certainly nicer to use it judiciously if you want to, but many people don't bother. I agree that since this pattern repeats a lot in this file, you should probably pick to what the other functions do and maybe constify the whole file afterwards if you want.
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/9/src/lib/coreboot_table.c@549 PS9, Line 549: CONFIG(VBOOT)
jwerner pushed a huge CL to clean up this inconsistency: […]
Yeah, CONFIG(VBOOT) should be used in all .c files which is enforced by the linter (Makefiles would still use CONFIG_VBOOT). I guess you have a point about grepping if you want to just find everything at once... but the change is already through and I think the advantages still outweigh (the IS_ENABLED(CONFIG_...) thing really got tedious imo). FWIW when I grep for config options I usually grep without the CONFIG_ (e.g. just "VBOOT") anyway, because that's the only way you also see depends on VBOOT or default y if VBOOT in Kconfig files (for VBOOT this would be annoying but most Kconfig options have a more unique name).
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/31887/9/src/security/vboot/misc.h@32 PS9, Line 32: CPU architecture agnostic
Ack
The architecture agnostic is mostly a warning that you shouldn't use things like size_t in here (e.g. that's why it's not using coreboot's usual 'struct region'). We have some platforms (like t210) where earlier stages run on a different processor with different address width then later ones.