Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32262 )
Change subject: vboot: refactor OPROM code ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/#/c/32262/3/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/#/c/32262/3/src/security/vboot/common.c@125 PS3, Line 125: int vboot_declares_display(void)
Well, if we want to have a function that fulfills both needs, it could be written like: […]
Yes, I think the latter option is reasonable.
https://review.coreboot.org/#/c/32262/4/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32262/4/src/security/vboot/misc.h@40 PS4, Line 40: uint32_t buffer_size; nit: maybe change these two to u16 so the whole thing can still fit 16 bytes (which we align the work buffer behind it to)?
https://review.coreboot.org/#/c/32262/4/src/security/vboot/misc.h@47 PS4, Line 47: #define VBOOT_FLAG_DISPLAY_REQUESTED (1 << 0) Maybe prefix this VB_WD_FLAG instead to have clearer differentiation from the dozens of other kinds of flags vboot has in various places?
https://review.coreboot.org/#/c/32262/3/src/security/vboot/misc.h File src/security/vboot/misc.h:
https://review.coreboot.org/#/c/32262/3/src/security/vboot/misc.h@37 PS3, Line 37: uint8_t declares_display;
What's the most acceptable format? I haven't really come across any bit-fields in working with core […]
I think either is fine. We tend to not use bitfields for hardware registers (with a few exceptions like libpayload's USB stacks), but for flags that are handled purely in software they should be okay. It's just a personal preference thing.