Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41814 )
Change subject: security/vboot: Add option to run verstage before bootblock ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/Kconfig@... PS7, Line 22: config HAVE_PRE_BOOTBLOCK_VBOOT_SUPPORT
You shouldn't need this. The VBOOT_STARTS_IN... […]
Done
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/Makefile... File src/security/vboot/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/Makefile... PS7, Line 100: verstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE) += verstage.c
nit: maybe move this down to the other !STARTS_BEFORE_BOOTBLOCK block below to keep the stuff belong […]
I'd rather keep the source files at the top. I wouldn't look for a source file down below. If you have a strong preference for moving it, I can, but to me it makes more sense to keep it here.
I haven't tested this, but I think we could probably eliminate the ifeq with something like: "verstage-$(CONFIG_VBOOT_SEPARATE_VERSTAGE)$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += verstage.c"
That would give us verstage-yy in the failing condition, which I don't think would get added.
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/Makefile... PS7, Line 147: ifeq ($(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK),)
You've moved this out of an if (SEPARATE_VERSTAGE) block, it needs to stay in there, or it'll try ad […]
thanks. I tested building, but missed that.
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/common.c File src/security/vboot/common.c:
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/common.c... PS7, Line 23: (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK) && ENV_SEPARATE_VERSTAGE)))
I think a lot of these statements would become much simpler if you use !STARTS_IN_ROMSTAGE rather th […]
Done
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/vboot_lo... File src/security/vboot/vboot_loader.c:
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/vboot_lo... PS7, Line 16: _Static_assert( (!CONFIG(VBOOT_SEPARATE_VERSTAGE)) || (CONFIG(VBOOT_SEPARATE_VERSTAGE) &&
space prohibited after that open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/41814/7/src/security/vboot/vboot_lo... PS7, Line 17: (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK))),
Can be simplified to […]
Done
-
Martin Roth (Code Review)