Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31887 )
Change subject: vboot: make vboot workbuf available to payload ......................................................................
Patch Set 9:
(6 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
Should this be at the bottom for ordering?
Doesn't really seem to be in order of memory layout... so probably makes more sense to group by related items as I've done here.
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/#/c/31887/9/payloads/libpayload/include/sysinfo.... PS9, Line 100: void *vboot_workbuf;
Does coreboot have a convention of commenting struct members? It doesn't look like it, but I thought […]
As it writing a comment for each member? Doesn't look like it... would it make sense to add some description here?
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
const?
If it makes sense to use const here, perhaps we can do that for all functions in coreboot.c in a follow-up CL.
Actually, I'm still not so certain on how const should be used. What is the rule people typically use to decide whether or not to include it? E.g.
* Everywhere and anywhere that the pointer/value is not modified * Only function signatures where there should be a contract of the function not modifying the pointer/value * Never?
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/31887/5/src/lib/coreboot_table.c@212 PS5, Line 212: #if CONFIG(VBOOT)
Re your penultimate comment, yes, the compiler will drop the code if it can tell it isn't used. […]
Ack
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)
I saw the thread on this and it seems like it is decided to use the CONFIG() macro. […]
Yeah, seems like both Aaron and Julius are in heavy favour of using C conditionals wherever possible, in order to reduce bit rot. When we use the preprocessor to comment out specific sections, it can also be easy to introduce typos, etc. in sections of code that don't necessarily get compiled.
What kind of situations are you talking about for searching?
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
CPU-architecture-agnostic […]
Well, I suppose it would be more accurate to say that vboot_working_data "is placed just before the start of the vboot work buffer". Would it be clearer to update the text to this?
But AFAIK everything else still holds true.