Change in coreboot[master]: soc/amd/picasso: Refactor workbuf check into separate function

[Martin Roth (Code Review)]

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

Change subject: soc/amd/picasso: Refactor workbuf check into separate function
......................................................................

soc/amd/picasso: Refactor workbuf check into separate function

BUG=None
TEST=Build
Branch=Zork

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

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

diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index 316396d..1626c55 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -113,6 +113,33 @@
    	wrmsr(S3_RESUME_EIP_MSR, s3_resume_entry);
 }

+#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
+static void verify_workbuf(void)
+{
+	if (*(uint32_t *)_vboot2_work == VB2_SHARED_DATA_MAGIC) {
+		cmos_write(0x00, CMOS_RECOVERY_BYTE);
+		return;
+	}
+
+	/*
+	 * If CMOS is valid and 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 and halt.
+	 */
+	if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
+			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();
+}
+#else
+static void verify_workbuf(void) {}
+#endif
+
 asmlinkage void bootblock_c_entry(uint64_t base_timestamp)
 {
    set_caching();
@@ -132,26 +159,9 @@
    u32 val = cpuid_eax(1);
    printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

-#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 and halt.
-		 */
-		if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
-				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();
-	} else {
-		cmos_write(0x00, CMOS_RECOVERY_BYTE);
+	if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) {
+		verify_workbuf();
    }
-#endif

fch_early_init();
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46112 )

Change subject: soc/amd/picasso: Refactor workbuf check into separate function
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46112/1/src/soc/amd/picasso/bootblo... 
PS1, Line 162: 	if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) {
braces {} are not necessary for single statement blocks

[Martin Roth (Code Review)]

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

Change subject: soc/amd/picasso: Refactor workbuf check into separate function
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46112/1/src/soc/amd/picasso/bootblo... 
PS1, Line 115: 
             : #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
I'm not crazy about this, but there are things that aren't defined if VBOOT_STARTS_BEFORE_BOOTBLOCK isn't being used.
1) _vboot2_work is only define if VBOOT of some sort is being used.
2) CMOS_RECOVERY_BYTE is defined from a config option that depends on VBOOT_STARTS_BEFORE_BOOTBLOCK.

In the next patch, the transfer block is also only defined if VBOOT is being used.

If desired, I can refactor to change all of this, but I'm not sure how valuable it really is.

[Aaron Durbin (Code Review)]

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

Change subject: soc/amd/picasso: Refactor workbuf check into separate function
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46112/1/src/soc/amd/picasso/bootblo... 
PS1, Line 115: 
             : #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
> I'm not crazy about this, but there are things that aren't defined if VBOOT_STARTS_BEFORE_BOOTBLOCK  […]

If you add a vboot_bootblock.c or some such, you can just move all this into that file and conditionally ad it: bootblock-$(VBOOT) += vboot_bootblock.c

The only call that would be required is the conditional you have below in bootblock_soc_init(). I whipped a patch up for you.

diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc
index 1e9ba4a124..b7b6cfe9ad 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -23,6 +23,8 @@ bootblock-y += gpio.c
 bootblock-y += smi_util.c
 bootblock-y += config.c
 bootblock-y += reset.c
+bootblock-y += pmutil.c
+bootblock-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += vboot_transfer.c

romstage-y += i2c.c
 romstage-y += romstage.c
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index 470002789f..2ef589762c 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -132,19 +132,8 @@ void bootblock_soc_init(void)
        u32 val = cpuid_eax(1);
        printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

-#if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
-       if (*(uint32_t *)_vboot2_work != VB2_SHARED_DATA_MAGIC) {
-               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();
-       } else {
-               cmos_write(0x00, CMOS_RECOVERY_BYTE);
-       }
-#endif
+       if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK))
+               verify_psp_transfer_buf();

fch_early_init();
 }
diff --git a/src/soc/amd/picasso/include/soc/psp_transfer.h b/src/soc/amd/picasso/include/soc/psp_transfer.h
index be88ce876d..fdd7bd1fd9 100644
--- a/src/soc/amd/picasso/include/soc/psp_transfer.h
+++ b/src/soc/amd/picasso/include/soc/psp_transfer.h
@@ -41,6 +41,10 @@ struct transfer_info_struct {

_Static_assert(sizeof(struct transfer_info_struct) == TRANSFER_INFO_SIZE, \
                "TRANSFER_INFO_SIZE is incorrect");
+
+/* Make sure the PSP transferred information over to x86 side. */
+void verify_psp_transfer_buf(void);
+
 #endif

#endif /* PSP_VERSTAGE_PSP_TRANSFER_H */
diff --git a/src/soc/amd/picasso/vboot_transfer.c b/src/soc/amd/picasso/vboot_transfer.c
new file mode 100644
index 0000000000..666c5b1479
--- /dev/null
+++ b/src/soc/amd/picasso/vboot_transfer.c
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <amdblocks/reset.h>
+#include <console/console.h>
+#include <pc80/mc146818rtc.h>
+#include <security/vboot/vbnv.h>
+#include <security/vboot/symbols.h>
+#include <soc/psp_transfer.h>
+#include <2struct.h>
+
+void verify_psp_transfer_buf(void)
+{
+       if (*(uint32_t *)_vboot2_work == VB2_SHARED_DATA_MAGIC) {
+               cmos_write(0x00, CMOS_RECOVERY_BYTE);
+               return;
+       }
+
+       /*
+        * If CMOS is valid and 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 and halt.
+        */
+       if (cmos_read(RTC_CLK_ALTCENTURY) != 0xff &&
+                       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/+/46112 )

Change subject: soc/amd/picasso: Refactor workbuf check into separate function
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/46112/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46112/1//COMMIT_MSG@7 
PS1, Line 7: soc/amd/picasso: Refactor workbuf check into separate function
why are you refactoring?

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Martin Roth (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Marshall Dawson, Eric Peers, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

soc/amd/picasso: Refactor transfer buffer check

The transfer buffer check had gotten large enough to deserve a function
of its own, so break it out.

BUG=None
TEST=Build
Branch=Zork

Signed-off-by: Martin Roth 
Change-Id: Idf46f8edb6b70c63f623522e2bcd2f22d6d4790b
---
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
A src/soc/amd/picasso/bootblock/vboot_bootblock.c
M src/soc/amd/picasso/include/soc/psp_transfer.h
4 files changed, 39 insertions(+), 25 deletions(-)

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

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46112 )

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

Patch Set 2:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46112/2/src/soc/amd/picasso/bootblo... 
PS2, Line 130: 	if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) {
braces {} are not necessary for single statement blocks

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Aaron Durbin (Code Review)]

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/46112/2/src/soc/amd/picasso/bootblo... 
PS2, Line 130: 	if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) {
> braces {} are not necessary for single statement blocks

This is the only thing I see.

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Edward O'Callaghan (Code Review)]

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Martin Roth (Code Review)]

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/46112/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/46112/1//COMMIT_MSG@7 
PS1, Line 7: soc/amd/picasso: Refactor workbuf check into separate function
> why are you refactoring?

Done

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

https://review.coreboot.org/c/coreboot/+/46112/1/src/soc/amd/picasso/bootblo... 
PS1, Line 115: 
             : #if CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)
> If you add a vboot_bootblock. […]

Done

Change in coreboot[master]: soc/amd/picasso: Refactor transfer buffer check

[Edward O'Callaghan (Code Review)]

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

Change subject: soc/amd/picasso: Refactor transfer buffer check
......................................................................

soc/amd/picasso: Refactor transfer buffer check

The transfer buffer check had gotten large enough to deserve a function
of its own, so break it out.

BUG=None
TEST=Build
Branch=Zork

Signed-off-by: Martin Roth 
Change-Id: Idf46f8edb6b70c63f623522e2bcd2f22d6d4790b
Reviewed-on: https://review.coreboot.org/c/coreboot/+/46112
Reviewed-by: Aaron Durbin 
Reviewed-by: Marshall Dawson 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M src/soc/amd/picasso/Makefile.inc
M src/soc/amd/picasso/bootblock/bootblock.c
A src/soc/amd/picasso/bootblock/vboot_bootblock.c
M src/soc/amd/picasso/include/soc/psp_transfer.h
4 files changed, 39 insertions(+), 25 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 9c5d4d0..1ed1529 100644
--- a/src/soc/amd/picasso/Makefile.inc
+++ b/src/soc/amd/picasso/Makefile.inc
@@ -24,6 +24,7 @@
 bootblock-y += config.c
 bootblock-y += pmutil.c
 bootblock-y += reset.c
+bootblock-$(CONFIG_VBOOT_STARTS_BEFORE_BOOTBLOCK) += bootblock/vboot_bootblock.c

romstage-y += i2c.c
 romstage-y += romstage.c
diff --git a/src/soc/amd/picasso/bootblock/bootblock.c b/src/soc/amd/picasso/bootblock/bootblock.c
index a450544..dfd5364 100644
--- a/src/soc/amd/picasso/bootblock/bootblock.c
+++ b/src/soc/amd/picasso/bootblock/bootblock.c
@@ -16,14 +16,8 @@
 #include <soc/i2c.h>
 #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)
-#include <2struct.h>
-#endif
-
 asmlinkage void bootblock_resume_entry(void);

/* PSP performs the memory training and setting up DRAM map prior to x86 cores
@@ -133,26 +127,9 @@
    u32 val = cpuid_eax(1);
    printk(BIOS_DEBUG, "Family_Model: %08x\n", val);

-#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();
-	} else {
-		cmos_write(0x00, CMOS_RECOVERY_BYTE);
+	if (CONFIG(VBOOT_STARTS_BEFORE_BOOTBLOCK)) {
+		verify_psp_transfer_buf();
    }
-#endif

fch_early_init();
 }
diff --git a/src/soc/amd/picasso/bootblock/vboot_bootblock.c b/src/soc/amd/picasso/bootblock/vboot_bootblock.c
new file mode 100644
index 0000000..4c3ae4a
--- /dev/null
+++ b/src/soc/amd/picasso/bootblock/vboot_bootblock.c
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <amdblocks/reset.h>
+#include <console/console.h>
+#include <pc80/mc146818rtc.h>
+#include <security/vboot/vbnv.h>
+#include <security/vboot/symbols.h>
+#include <soc/psp_transfer.h>
+#include <2struct.h>
+
+void verify_psp_transfer_buf(void)
+{
+	if (*(uint32_t *)_vboot2_work == VB2_SHARED_DATA_MAGIC) {
+		cmos_write(0x00, CMOS_RECOVERY_BYTE);
+		return;
+	}
+
+	/*
+	 * If CMOS is valid and 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 and halt.
+	 */
+	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();
+}
diff --git a/src/soc/amd/picasso/include/soc/psp_transfer.h b/src/soc/amd/picasso/include/soc/psp_transfer.h
index be88ce8..fdd7bd1 100644
--- a/src/soc/amd/picasso/include/soc/psp_transfer.h
+++ b/src/soc/amd/picasso/include/soc/psp_transfer.h
@@ -41,6 +41,10 @@

_Static_assert(sizeof(struct transfer_info_struct) == TRANSFER_INFO_SIZE, \
    	"TRANSFER_INFO_SIZE is incorrect");
+
+/* Make sure the PSP transferred information over to x86 side. */
+void verify_psp_transfer_buf(void);
+
 #endif

#endif	/* PSP_VERSTAGE_PSP_TRANSFER_H */