Julius Werner 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:
(5 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... options are always selected directly from SoC or mainboard code. The way you can do that (without turning things on inadvertently when VBOOT itself is disabled) is by overriding the config VBOOT option to add another select statement in your SoC Kconfig like this:
config VBOOT selects VBOOT_STARTS_BEFORE_BOOTBLOCK
(See how the same thing is done in src/soc/intel/cannonlake/Kconfig, for example.)
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 belonging to that together?
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 adding verstage.elf in !SEPARATE_VERSTAGE builds.
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 than STARTS_IN_BOOTBLOCK || STARTS_BEFORE_BOOTBLOCK, because those often go together. For example this could be
if(wb == NULL && !CONFIG(VBOOT_STARTS_IN_ROMSTAGE) && preram_symbols_available())
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 17: (CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK))), Can be simplified to
(!CONFIG(VBOOT_SEPARATE_VERSTAGE) || CONFIG(VBOOT_STARTS_IN_BOOTBLOCK) || CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)