Change in coreboot[master]: Revert "mb/google/zork: Enable psp_verstage"

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Julius Werner, Aaron Durbin,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Revert "mb/google/zork: Enable psp_verstage"

This reverts commit c35d4fa377fdf1a967ed426024d3886302d2081f.

Reason for revert: Breaks S3 resume on trembyle.

Change-Id: Ic086c60d8e6edc9c0dff65e2569999a314c7c4ba
---
M src/mainboard/google/zork/Kconfig
1 file changed, 2 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43066/1

diff --git a/src/mainboard/google/zork/Kconfig b/src/mainboard/google/zork/Kconfig
index a6c866d..19225d7 100644
--- a/src/mainboard/google/zork/Kconfig
+++ b/src/mainboard/google/zork/Kconfig
@@ -100,6 +100,8 @@
 config VBOOT
    select EC_GOOGLE_CHROMEEC_SWITCHES
    select VBOOT_LID_SWITCH
+	select VBOOT_STARTS_IN_BOOTBLOCK
+	select VBOOT_SEPARATE_VERSTAGE

config VBOOT_VBNV_OFFSET
    hex
@@ -121,22 +123,6 @@
    hex
    default 0x50

-config PICASSO_FW_A_POSITION
-	hex
-	default 0xFF031040
-	depends on VBOOT_SLOTS_RW_AB && VBOOT_STARTS_BEFORE_BOOTBLOCK
-	help
-	  Location of the AMD firmware in the RW_A region. This is the
-	  start of the RW-A region + 64 bytes for the cbfs header.
-
-config PICASSO_FW_B_POSITION
-	hex
-	default 0xFF3CF040
-	depends on VBOOT_SLOTS_RW_AB && VBOOT_STARTS_BEFORE_BOOTBLOCK
-	help
-	  Location of the AMD firmware in the RW_B region. This is the
-	  start of the RW-A region + 64 bytes for the cbfs header.
-
 config VARIANT_HAS_FW_CONFIG
    bool
    help
@@ -165,20 +151,4 @@
    default 2 if BOARD_GOOGLE_VILBOZ
    default VARIANT_MIN_BOARD_ID_V3_SCHEMATICS

-config VBOOT_STARTS_BEFORE_BOOTBLOCK
-	bool "PSP verstage"
-	default y if VBOOT
-	help
-	  Firmware verification happens before the main processor is brought
-	  online.
-
-config VBOOT_STARTS_IN_BOOTBLOCK
-	bool "X86 verstage (in bootblock)"
-	depends on  VBOOT && ! VBOOT_STARTS_BEFORE_BOOTBLOCK
-	select VBOOT_SEPARATE_VERSTAGE
-	help
-	  Firmware verification happens during the end of or right after the
-	  bootblock. This implies that a static VBOOT2_WORK() buffer must be
-	  allocated in memlayout.
-
 endif # BOARD_GOOGLE_BASEBOARD_TREMBYLE || BOARD_GOOGLE_BASEBOARD_DALBOZ

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Julius Werner, Aaron Durbin,

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

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

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Revert "mb/google/zork: Enable psp_verstage"

This reverts commit c35d4fa377fdf1a967ed426024d3886302d2081f.

Reason for revert: Breaks S3 resume on trembyle.

Signed-off-by: Raul E Rangel 
Change-Id: Ic086c60d8e6edc9c0dff65e2569999a314c7c4ba
---
M src/mainboard/google/zork/Kconfig
1 file changed, 2 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43066/2

[Aaron Durbin (Code Review)]

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43066/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43066/2//COMMIT_MSG@11 
PS2, Line 11: Reason for revert: Breaks S3 resume on trembyle.
What's the bug tracking the symptoms?

[Eric Peers (Code Review)]

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 2:

bug for tracking the issue: https://buganizer.corp.google.com/issues/160834101

still need the actual symptoms/logs.

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Julius Werner, Aaron Durbin,

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

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

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Revert "mb/google/zork: Enable psp_verstage"

This reverts commit c35d4fa377fdf1a967ed426024d3886302d2081f.

Reason for revert: Breaks S3 resume on trembyle.

Using the non debug PSP, we get no serial output on resume.

BUG=b:160834101

Signed-off-by: Raul E Rangel 
Change-Id: Ic086c60d8e6edc9c0dff65e2569999a314c7c4ba
---
M src/mainboard/google/zork/Kconfig
1 file changed, 2 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/43066/3

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43066 )

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 4:

CB:42523 showed x86-centric view of memory space is still leaking to psp_verstage builds.

[Martin Roth (Code Review)]

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 4: Code-Review+1

While I'd prefer to just change the flag to use the bootblock verstage instead of reverting the full patch, I'm ok with reverting this if that's what others feel needs to happen.  I'm working to debug the issue, so I hope to have it cleared up today.

[Aaron Durbin (Code Review)]

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

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 4: Code-Review+2

Kconfig flip would work as Martin suggested as well.

[Raul Rangel (Code Review)]

Raul Rangel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43066 )

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Abandoned

This sat around way to long to be useful...

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43066 )

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 4:

So which commit is the S3 regression fix?

There are 2+ more things wrong with the psp-verstage build. One of them building PCI code into psp-verstage, although on a codepath that should not ever be reached. One of them the build just failing when psp-verstage using the option visible in menuconfig.

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43066 )

Change subject: Revert "mb/google/zork: Enable psp_verstage"
......................................................................

Patch Set 4:
> Patch Set 4:
>
> So which commit is the S3 regression fix?
>
> There are 2+ more things wrong with the psp-verstage build. One of them building PCI code into psp-verstage, although on a codepath that should not ever be reached. One of them the build just failing when psp-verstage using the option visible in menuconfig.

Sorry for not posting the link earlier. It was late and I just wanted to make sure this didn't get merged. https://review.coreboot.org/c/coreboot/+/43334/1