Change in coreboot[master]: soc/amd/picasso: Die if the workbuf is missing two boots in a row

[Martin Roth (Code Review)]

Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

soc/amd/picasso: Die if the workbuf is missing two boots in a row

BUG=b:169199392
TEST=Corrupt vboot signature to force an error, see that the system
halts instead of rebooting forever.
BRANCH=Zork

Signed-off-by: Martin Roth 
Change-Id: I949f94e78d25720f6cd7e81de8d030084e267f29
---
M src/soc/amd/picasso/bootblock/bootblock.c
1 file changed, 9 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/45964/1

diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index 4700027..a8303f5 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -134,10 +134,17 @@

#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
    if (*(uint32_t *)_vboot2_work != VB2_SHARED_DATA_MAGIC) {
+		/*
+		 * If the system has already been rebooted once, but still returns here,
+		 * instead of rebooting to verstage again, assume that the system is in
+		 * a reboot loop, so halt instead.
+		 */
+		if ((!vbnv_cmos_failed()) && cmos_read(CMOS_RECOVERY_BYTE) ==
+				CMOS_RECOVERY_MAGIC_VAL)
+			die("Error: Reboot into recovery was unsuccessful.  Halting.");
+
    	printk(BIOS_ERR, "ERROR: VBOOT workbuf not valid.\n");
-
    	printk(BIOS_DEBUG, "Signature: %#08x\n", *(uint32_t *)_vboot2_work);
-
    	cmos_init(0);
    	cmos_write(CMOS_RECOVERY_MAGIC_VAL, CMOS_RECOVERY_BYTE);
    	warm_reset();

[Eric Peers (Code Review)]

Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 1: Code-Review+1

What's going on with the unstable logs? Do we need to kick off another build?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/1/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/1/src/soc/amd/picasso/bootblo... 
PS1, Line 142: 		if ((!vbnv_cmos_failed()) && cmos_read(CMOS_RECOVERY_BYTE) ==
src/soc/amd/picasso/bootblock/bootblock.c:142:9: error: implicit declaration of function 'vbnv_cmos_failed' [-Werror=implicit-function-declaration]
   if ((!vbnv_cmos_failed()) && cmos_read(CMOS_RECOVERY_BYTE) ==
         ^~~~~~~~~~~~~~~~

[Eric Peers (Code Review)]

Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 1: -Code-Review

got it. Thanks!

console output was truncating the logs:
https://qa.coreboot.org/job/coreboot-gerrit/143888/consoleText

console text on the failure was showing the issue:
https://qa.coreboot.org/job/coreboot-gerrit/143888/consoleText

[Martin Roth (Code Review)]

Hello build bot (Jenkins), Kangheui Won, Eric Peers, Rob Barnes, Aaron Durbin,

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

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

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

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

soc/amd/picasso: Die if the workbuf is missing two boots in a row

BUG=b:169199392
TEST=Corrupt vboot signature to force an error, see that the system
halts instead of rebooting forever.
BRANCH=Zork

Signed-off-by: Martin Roth 
Change-Id: I949f94e78d25720f6cd7e81de8d030084e267f29
---
M src/soc/amd/picasso/bootblock/bootblock.c
1 file changed, 9 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/45964/2

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
PS2, Line 142: 		if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
src/soc/amd/picasso/pmutil.c already has vbnv_cmos_failed() implementation. #include'ing  <security/vboot/vbnv.h> and adding pmutil.c to bootblock should have been sufficient.

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
PS2, Line 142: 		if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
> src/soc/amd/picasso/pmutil.c already has vbnv_cmos_failed() implementation. […]

I agree, but vbnv_cmos_failed is just this one cmos read, which I'm already using in this function, so I just pulled the cmos read in.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
PS2, Line 142: 		if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
> I agree, but vbnv_cmos_failed is just this one cmos read, which I'm already using in this function,  […]

Sure, but that's the point of the function. Just because the picasso implementation is using that method doesn't mean we should expand it anywhere it would be used. We have that wrapper for the consistency of the code base.

[Martin Roth (Code Review)]

Hello build bot (Jenkins), Kangheui Won, Eric Peers, Rob Barnes, Aaron Durbin,

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

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

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

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

soc/amd/picasso: Die if the workbuf is missing two boots in a row

BUG=b:169199392
TEST=Corrupt vboot signature to force an error, see that the system
halts instead of rebooting forever.
BRANCH=Zork

Signed-off-by: Martin Roth 
Change-Id: I949f94e78d25720f6cd7e81de8d030084e267f29
---
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/45964/3

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/2/src/soc/amd/picasso/bootblo... 
PS2, Line 142: 		if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
> Sure, but that's the point of the function. […]

Done

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 3: Code-Review+2

[Martin Roth (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Kangheui Won, Eric Peers, Rob Barnes, Aaron Durbin,

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

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

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

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

soc/amd/picasso: Die if the workbuf is missing two boots in a row

BUG=b:169199392
TEST=Corrupt vboot signature to force an error, see that the system
halts instead of rebooting forever.
BRANCH=Zork

Signed-off-by: Martin Roth 
Change-Id: I949f94e78d25720f6cd7e81de8d030084e267f29
---
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
2 files changed, 11 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/45964/4

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 4: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45964/1/src/soc/amd/picasso/bootblo... 
File src/soc/amd/picasso/bootblock/bootblock.c:

https://review.coreboot.org/c/coreboot/+/45964/1/src/soc/amd/picasso/bootblo... 
PS1, Line 142: 		if ((!vbnv_cmos_failed()) && cmos_read(CMOS_RECOVERY_BYTE) ==
> src/soc/amd/picasso/bootblock/bootblock. […]

Done

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

Patch Set 4: Code-Review+2

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45964 )

Change subject: soc/amd/picasso: Die if the workbuf is missing two boots in a row
......................................................................

soc/amd/picasso: Die if the workbuf is missing two boots in a row

BUG=b:169199392
TEST=Corrupt vboot signature to force an error, see that the system
halts instead of rebooting forever.
BRANCH=Zork

Signed-off-by: Martin Roth 
Change-Id: I949f94e78d25720f6cd7e81de8d030084e267f29
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45964
Reviewed-by: Marshall Dawson 
Reviewed-by: Edward O'Callaghan 
Reviewed-by: Aaron Durbin 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
2 files changed, 11 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved
  Marshall Dawson: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 1e9ba4a..9c5d4d0 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -22,6 +22,7 @@
 bootblock-y += gpio.c
 bootblock-y += smi_util.c
 bootblock-y += config.c
+bootblock-y += pmutil.c
 bootblock-y += reset.c

romstage-y += i2c.c
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index 4700027..a450544 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -17,6 +17,7 @@
 #include <amdblocks/amd_pci_mmconf.h>
 #include <acpi/acpi.h>
 #include <security/vboot/symbols.h>
+#include <security/vboot/vbnv.h>

/* vboot includes directory may not be in include path if vboot is not enabled */
 #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
@@ -134,10 +135,17 @@

#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
    if (*(uint32_t *)_vboot2_work != VB2_SHARED_DATA_MAGIC) {
+		/*
+		 * If the system has already been rebooted once, but still returns here,
+		 * instead of rebooting to verstage again, assume that the system is in
+		 * a reboot loop, so halt instead.
+		 */
+		if ((!vbnv_cmos_failed()) && cmos_read(CMOS_RECOVERY_BYTE) ==
+				CMOS_RECOVERY_MAGIC_VAL)
+			die("Error: Reboot into recovery was unsuccessful.  Halting.");
+
    	printk(BIOS_ERR, "ERROR: VBOOT workbuf not valid.\n");
-
    	printk(BIOS_DEBUG, "Signature: %#08x\n", *(uint32_t *)_vboot2_work);
-
    	cmos_init(0);
    	cmos_write(CMOS_RECOVERY_MAGIC_VAL, CMOS_RECOVERY_BYTE);
    	warm_reset();