[coreboot-gerrit] Change in coreboot[master]: vboot: Disallow separate verstage after romstage, try to cla...

[Julius Werner (Code Review)]

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/Kconfig
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.


-- 
To view, visit https://review.coreboot.org/18983
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf60040af4eff711d9b80ee0e5950ce05958b3aa
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Julius Werner <jwerner at chromium.org>
Gerrit-Reviewer: Patrick Georgi <pgeorgi at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes