Change in ...coreboot[master]: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS

[Julius Werner (Code Review)]

Julius Werner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32112

Change subject: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
......................................................................

bootmode: display_init_required() to check VBOOT and OPROM_MATTERS

Skipping display init on normal-mode boot is a vboot feature, not
specific to Chrome OS. Fix the code in display_init_required() to check
CONFIG_VBOOT rather than CONFIG_CHROMEOS now that the two aren't always
the same anymore.

Also add a check to guarantee at compile time that VBOOT_OPROM_MATTERS
is enabled on all platforms that make a check to this function (when
VBOOT is also enabled). The whole display skipping mechanism is based on
the oprom_needed NVRAM flag, and skipping display init without enabling
the option to tell vboot that it needs to pay attention to that flag
would make the whole thing not work right.

Change-Id: I5d6421509bdcdaa61b78015af3fa6241fe75bb7f
Signed-off-by: Julius Werner 
---
M src/lib/bootmode.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/32112/1

diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c
index e402536..b7ab649 100644
--- a/src/lib/bootmode.c
+++ b/src/lib/bootmode.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */

+#include <assert.h>
 #include <bootmode.h>
 #include <vendorcode/google/chromeos/chromeos.h>

@@ -35,8 +36,12 @@
 int display_init_required(void)
 {
    /* For Chrome OS always honor vboot_handoff_skip_display_init(). */
-	if (CONFIG(CHROMEOS))
+	if (CONFIG(VBOOT)) {
+		if (!CONFIG(VBOOT_OPROM_MATTERS))
+			dead_code("If you call display_init_required() anywhere"
+				  ", you need to select VBOOT_OPROM_MATTERS!");
    	return !vboot_handoff_skip_display_init();
+	}

/* By default always initialize display. */
    return 1;

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
......................................................................

Patch Set 1: Code-Review+2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG@7 
PS1, Line 7: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
Statement please:
> Check VBOOT and OPROM_MATTERS in display_init_required()

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG@14 
PS1, Line 14: VBOOT_OPROM_MATTERS
Prefix with CONFIG_ as done in the paragraph above?

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32112

to look at the new patch set (#2).

Change subject: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
......................................................................

bootmode: display_init_required() to check VBOOT and OPROM_MATTERS

Skipping display init on normal-mode boot is a vboot feature, not
specific to Chrome OS. Fix the code in display_init_required() to check
CONFIG_VBOOT rather than CONFIG_CHROMEOS now that the two aren't always
the same anymore.

Also add a check to guarantee at compile time that VBOOT_OPROM_MATTERS
is enabled on all platforms that make a check to this function (when
VBOOT is also enabled). The whole display skipping mechanism is based on
the oprom_needed NVRAM flag, and skipping display init without enabling
the option to tell vboot that it needs to pay attention to that flag
would make the whole thing not work right.

Change-Id: I5d6421509bdcdaa61b78015af3fa6241fe75bb7f
Signed-off-by: Julius Werner 
---
M src/lib/bootmode.c
1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/32112/2

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG@7 
PS1, Line 7: bootmode: display_init_required() to check VBOOT and OPROM_MATTERS
> Statement please: […]

There's only so much I can fit on one line when the function name already takes up half of it. Suggestions welcome but I couldn't come up with anything that doesn't drop substance in favor of grammar nitpicks.

https://review.coreboot.org/#/c/32112/1//COMMIT_MSG@14 
PS1, Line 14: VBOOT_OPROM_MATTERS
> Prefix with CONFIG_ as done in the paragraph above?

Sure, will do.

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Julius Werner (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32112

to look at the new patch set (#3).

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

Skipping display init on normal-mode boot is a vboot feature, not
specific to Chrome OS. Fix the code in display_init_required() and
pci_dev_init() to check CONFIG_VBOOT rather than CONFIG_CHROMEOS now
that the two aren't always the same anymore.

Also add a check to guarantee at compile time that
CONFIG_VBOOT_OPROM_MATTERS is enabled on all platforms that make a check
to this function (when CONFIG_VBOOT is also enabled). The whole display
skipping mechanism is based on the oprom_needed NVRAM flag, and skipping
display init without enabling the option to tell vboot that it needs to
pay attention to that flag would make the whole thing not work right.

Change-Id: I5d6421509bdcdaa61b78015af3fa6241fe75bb7f
Signed-off-by: Julius Werner 
---
M src/device/pci_device.c
M src/lib/bootmode.c
2 files changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/32112/3

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

Patch Set 3: Code-Review+1

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c 
File src/lib/bootmode.c:

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c@38 
PS3, Line 38: Chrome OS
vboot

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c@42 
PS3, Line 42: dead_code
No strong opinion here in any way, but that name is confusing me in this context. it's more like "break_build()" ;-)

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Julius Werner (Code Review)]

Hello Paul Menzel, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/32112

to look at the new patch set (#4).

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

Skipping display init on normal-mode boot is a vboot feature, not
specific to Chrome OS. Fix the code in display_init_required() and
pci_dev_init() to check CONFIG_VBOOT rather than CONFIG_CHROMEOS now
that the two aren't always the same anymore.

Also add a check to guarantee at compile time that
CONFIG_VBOOT_OPROM_MATTERS is enabled on all platforms that make a check
to this function (when CONFIG_VBOOT is also enabled). The whole display
skipping mechanism is based on the oprom_needed NVRAM flag, and skipping
display init without enabling the option to tell vboot that it needs to
pay attention to that flag would make the whole thing not work right.

Change-Id: I5d6421509bdcdaa61b78015af3fa6241fe75bb7f
Signed-off-by: Julius Werner 
---
M src/device/pci_device.c
M src/lib/bootmode.c
2 files changed, 8 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/32112/4

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c 
File src/lib/bootmode.c:

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c@38 
PS3, Line 38: Chrome OS
> vboot

Done

https://review.coreboot.org/#/c/32112/3/src/lib/bootmode.c@42 
PS3, Line 42: dead_code
> No strong opinion here in any way, but that name is confusing me in this context. […]

Sure, that sounds fine too... feel free to upload a CL to rename it if you want. But this patch is just using the existing macro (and there are other uses in the repo) so I can't rename it here.

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

Patch Set 4: Code-Review+1

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32112 )

Change subject: vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS
......................................................................

vboot: Change oprom checks to CONFIG_VBOOT, assert OPROM_MATTERS

Skipping display init on normal-mode boot is a vboot feature, not
specific to Chrome OS. Fix the code in display_init_required() and
pci_dev_init() to check CONFIG_VBOOT rather than CONFIG_CHROMEOS now
that the two aren't always the same anymore.

Also add a check to guarantee at compile time that
CONFIG_VBOOT_OPROM_MATTERS is enabled on all platforms that make a check
to this function (when CONFIG_VBOOT is also enabled). The whole display
skipping mechanism is based on the oprom_needed NVRAM flag, and skipping
display init without enabling the option to tell vboot that it needs to
pay attention to that flag would make the whole thing not work right.

Change-Id: I5d6421509bdcdaa61b78015af3fa6241fe75bb7f
Signed-off-by: Julius Werner 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32112
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: Patrick Georgi 
---
M src/device/pci_device.c
M src/lib/bootmode.c
2 files changed, 8 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 5f908d2..b5453c7 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -780,7 +780,7 @@
     */
    should_run = display_init_required();

-	if (!should_run && CONFIG(CHROMEOS))
+	if (!should_run && CONFIG(VBOOT))
    	should_run = vboot_wants_oprom();

if (!should_run)
diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c
index e402536..052eb8f 100644
--- a/src/lib/bootmode.c
+++ b/src/lib/bootmode.c
@@ -13,6 +13,7 @@
  * GNU General Public License for more details.
  */

+#include <assert.h>
 #include <bootmode.h>
 #include <vendorcode/google/chromeos/chromeos.h>

@@ -34,9 +35,13 @@

int display_init_required(void)
 {
-	/* For Chrome OS always honor vboot_handoff_skip_display_init(). */
-	if (CONFIG(CHROMEOS))
+	/* For vboot always honor vboot_handoff_skip_display_init(). */
+	if (CONFIG(VBOOT)) {
+		/* Must always select OPROM_MATTERS when using this function. */
+		if (!CONFIG(VBOOT_OPROM_MATTERS))
+			dead_code();
    	return !vboot_handoff_skip_display_init();
+	}

/* By default always initialize display. */
    return 1;