Change in coreboot[master]: vboot: Disallow separate verstage after romstage, try to cla...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/18983 )
Change subject: vboot: Disallow separate verstage after romstage, try to clarify logic ......................................................................
Patch Set 2:
(9 comments)
https://review.coreboot.org/#/c/18983/2/src/arch/x86/bootblock_simple.c File src/arch/x86/bootblock_simple.c:
PS2, Line 32: *
errant edit
Whoops, rebase mistake. Will fix.
https://review.coreboot.org/#/c/18983/2/src/mainboard/google/gru/Kconfig File src/mainboard/google/gru/Kconfig:
Line 52: select SPI_TPM if GRU_HAS_TPM2
Why leave this here when you put it in board specific options?
Yeah... not quite sure what I did here. Might have also gotten screwed up during a rebase. I guess I should keep both TPM options under VBOOT.
https://review.coreboot.org/#/c/18983/2/src/mainboard/intel/kunimitsu/Kconfi... File src/mainboard/intel/kunimitsu/Kconfig:
PS2, Line 39: LID_SWITCH
src/vendorcode/google/chromeos/Kconfig still has the LID_SWITCH Kconfig. Wh
Right, I guess I would need to leave LID_SWITCH under CHROMEOS.
I wasn't quite sure whether to pull LID_SWITCH and WIPEOUT_SUPPORTED into vboot, because they are both used in vboot code but don't really have much to do with verification. But when looking at this again, leaving them out would mean that we'd need to have both 'config VBOOT' and 'config CHROMEOS' sections for almost every board... that seems too annoying, so I'll pull them over too.
https://review.coreboot.org/#/c/18983/2/src/vboot/Kconfig File src/vboot/Kconfig:
Line 28:
Why not just if VBOOT all of these options instead of adding depends on for
Right, I guess that makes more sense.
Line 313: endmenu # Verified Boot (vboot)
All this lifted verbatim?
The GBB and key stuff, yes. FWID_MODEL and _VERSION were renamed from CHROMEOS_ to VBOOT_.
https://review.coreboot.org/#/c/18983/2/src/vboot/Makefile.inc File src/vboot/Makefile.inc:
Line 267: $(CBFSTOOL) $(obj)/coreboot.rom write -u -r VBLOCK_B -f $(obj)/VBLOCK_B.bin
Is all this lifted verbatim?
Yes, except for the naming change with FWID_MODEL and _VERSION.
https://review.coreboot.org/#/c/18983/2/src/vboot/vboot_loader.c File src/vboot/vboot_loader.c:
Line 33: IS_ENABLED(CONFIG_VBOOT_STARTS_IN_BOOTBLOCK),
Why isn't this SEPARATE_VERSTAGE && STARTS_IN_BOOTBLOCK ? That's what you w
I want to assert that SEPARATE_VERSTAGE requires STARTS_IN_BOOTBLOCK. But if I just wrote _Static_assert(SEPARATE_VERSTAGE && STARTS_IN_BOOTBLOCK), this code would fail to compile for STARTS_IN_ROMSTAGE configurations. So I'm asserting "either you don't have SEPARATE_VERSTAGE, or (if you do) you use STARTS_IN_BOOTBLOCK", which is the logical equivalent to "if SEPARATE_VERSTAGE, then STARTS_IN_BOOTBLOCK" (I can't really use a C if() here, and I didn't want to mess it up with preprocessor #ifs).
Line 37: "return from verstage only makes sense for separate verstages");
Didn't you enforce these in Kconfig? Is this what you are talking about sel
Yes. Kconfig "depends on" is ignored by "select" directives, which is what we usually use to set these options.
Line 90: /* Note: this path is not used for VBOOT_SEPARATE_VERSTAGE */
This comment is now wrong. When ENV_VERSTAGE and !RETURN_FROM_VERSTAGE this
Ugh, right, yeah. Actually, I had first changed verstage.c because I thought it was broken for !RETURN_FROM_VERSTAGE and then also changed this comment... and when I finally understood how it actually worked I forgot to realize that the comment had been right after all.
-
Julius Werner (Code Review)