Change in coreboot[master]: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37562 )

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

Some return codes were missed when implementing this initially; the vboot
logic can require the system to command the EC to reboot to its RO, switch
RW slots or it can require a poweroff of the SoC.  This patch appropriately
handles these return codes.

BUG=b:145768046
BRANCH=firmware-hatch-12672.B
TEST=ODM verified this patch fixes the issues seen.

Change-Id: I2748cf626d49c255cb0274cb336b072dcdf8cded
Signed-off-by: Tim Wawrzynczak 
---
M src/security/vboot/ec_sync.c
1 file changed, 37 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/37562/1

diff --git a/src/security/vboot/ec_sync.c b/src/security/vboot/ec_sync.c
index 8a3ba71..6ef2edb 100644
--- a/src/security/vboot/ec_sync.c
+++ b/src/security/vboot/ec_sync.c
@@ -16,6 +16,7 @@
 #include <console/console.h>
 #include <delay.h>
 #include <ec/google/chromeec/ec.h>
+#include <halt.h>
 #include <security/vboot/misc.h>
 #include <security/vboot/vbnv.h>
 #include <security/vboot/vboot_common.h>
@@ -60,9 +61,43 @@
    retval = vb2api_ec_sync(ctx);
    vboot_save_nvdata_only(ctx);

-	if (retval != VB2_SUCCESS) {
-		printk(BIOS_ERR, "EC software sync failed (%#x), rebooting\n", retval);
+	switch (retval) {
+	case VBERROR_EC_REBOOT_TO_RO_REQUIRED:
+		printk(BIOS_INFO, "EC Reboot requested. Doing cold reboot\n");
+		if (google_chromeec_reboot(0, EC_REBOOT_COLD,
+					   EC_REBOOT_FLAG_ON_AP_SHUTDOWN))
+			vboot_reboot();
+		else
+			poweroff();
+		break;
+
+	case VBERROR_EC_REBOOT_TO_SWITCH_RW:
+		printk(BIOS_INFO, "Switch EC slot requested. Doing cold"
+		       "reboot\n");
+		if (google_chromeec_reboot(0, EC_REBOOT_COLD,
+					   EC_REBOOT_FLAG_SWITCH_RW_SLOT))
+			vboot_reboot();
+		else
+			poweroff();
+		break;
+
+	case VBERROR_SHUTDOWN_REQUESTED:
+		printk(BIOS_INFO, "EC requests shutdown, halting\n");
+		poweroff();
+		break;
+
+	case VBERROR_REBOOT_REQUIRED:
+		printk(BIOS_INFO, "Reboot requested. Doing warm reboot\n");
    	vboot_reboot();
+		break;
+
+	default:
+		if (retval != VB2_SUCCESS) {
+			printk(BIOS_ERR, "EC software sync failed (%#x),"
+			       " rebooting\n", retval);
+			vboot_reboot();
+		}
+		break;
    }

timestamp_add_now(TS_END_EC_SYNC);

[Furquan Shaikh (Code Review)]

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 1: Code-Review+2

[Julius Werner (Code Review)]

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 1:

(4 comments)

Whoops, yeah, not sure how we all missed that...

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
File src/security/vboot/ec_sync.c:

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 69: 			vboot_reboot();
This seems a bit odd for error handling? If you do want to reboot, you should at least vb2api_fail() first so that we'll drop into recovery mode. Otherwise, I think a die() would also be fine here (it shouldn't really happen).

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 71: 			poweroff();
poweroff() is not available on all coreboot platforms, so putting this here would prevent us from using it more widely. There's no need to do it anyway, you can just halt() and wait for the EC to reboot us.

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 86: 		poweroff();
This has the same problem with poweroff(). Luckily, I don't think the EC sync code needs it?

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 95: 		if (retval != VB2_SUCCESS) {
nit: maybe just put a

case VB2_SUCCESS:
   break;

at the top?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/37562 )

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Removed Code-Review+2 by Furquan Shaikh

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37562 )

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
File src/security/vboot/ec_sync.c:

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 69: 			vboot_reboot();
> This seems a bit odd for error handling? If you do want to reboot, you should at least vb2api_fail() […]

Agreed, that really shouldn't happen, I'll change to just print on an error and halt() regardless.

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 71: 			poweroff();
> poweroff() is not available on all coreboot platforms, so putting this here would prevent us from us […]

Ack

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 86: 		poweroff();
> This has the same problem with poweroff(). […]

Ah yes, that's from battery cutoff, not needed in just this flow.  Removed.

https://review.coreboot.org/c/coreboot/+/37562/1/src/security/vboot/ec_sync.... 
PS1, Line 95: 		if (retval != VB2_SUCCESS) {
> nit: maybe just put a […]

Done

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Julius Werner, SH Kim, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

Some return codes were missed when implementing this initially; the vboot
logic can require the system to command the EC to reboot to its RO, switch
RW slots or it can require a poweroff of the SoC.  This patch appropriately
handles these return codes.

BUG=b:145768046
BRANCH=firmware-hatch-12672.B
TEST=ODM verified this patch fixes the issues seen.

Change-Id: I2748cf626d49c255cb0274cb336b072dcdf8cded
Signed-off-by: Tim Wawrzynczak 
---
M src/security/vboot/ec_sync.c
1 file changed, 27 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/37562/2

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Julius Werner, SH Kim, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

Some return codes were missed when implementing this initially; the vboot
logic can require the system to command the EC to reboot to its RO, switch
RW slots or it can require a poweroff of the SoC.  This patch appropriately
handles these return codes.

BUG=b:145768046
BRANCH=firmware-hatch-12672.B
TEST=ODM verified this patch fixes the issues seen.

Change-Id: I2748cf626d49c255cb0274cb336b072dcdf8cded
Signed-off-by: Tim Wawrzynczak 
---
M src/security/vboot/ec_sync.c
1 file changed, 32 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/37562/3

[Julius Werner (Code Review)]

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
File src/security/vboot/ec_sync.c:

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
PS3, Line 71: EC_REBOOT_FLAG_ON_AP_SHUTDOWN
Actually, sorry, on second thought this seems to be a problem. Did you test this? Does it still work?

I think you'll have to remove the ON_AP_SHUTDOWN (just pass 0 for flags) to make this work now. I think that should be fine -- I see no reason to sync with AP shutdown here.

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
PS3, Line 72: BIOS_ERR
nit: could also be EMERG, like die() uses

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37562 )

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
File src/security/vboot/ec_sync.c:

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
PS3, Line 71: EC_REBOOT_FLAG_ON_AP_SHUTDOWN
> Actually, sorry, on second thought this seems to be a problem. […]

You're right, this won't work since we're not powering down the AP now.  It'll be a no-op effectively and the machine will hang.  Tested the new patch on its way.

https://review.coreboot.org/c/coreboot/+/37562/3/src/security/vboot/ec_sync.... 
PS3, Line 72: BIOS_ERR
> nit: could also be EMERG, like die() uses

Ack

[Tim Wawrzynczak (Code Review)]

Hello Aaron Durbin, Julius Werner, SH Kim, Shelley Chen, build bot (Jenkins), Joel Kitching, Furquan Shaikh,

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

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

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

Some return codes were missed when implementing this initially; the vboot
logic can require the system to command the EC to reboot to its RO, switch
RW slots or it can require a poweroff of the SoC.  This patch appropriately
handles these return codes.

BUG=b:145768046
BRANCH=firmware-hatch-12672.B
TEST=ODM verified this patch fixes the issues seen.

Change-Id: I2748cf626d49c255cb0274cb336b072dcdf8cded
Signed-off-by: Tim Wawrzynczak 
---
M src/security/vboot/ec_sync.c
1 file changed, 32 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/37562/4

[Julius Werner (Code Review)]

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

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

Patch Set 4: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37562 )

Change subject: EC sync: Properly handle VBERROR return codes from vb2api_ec_sync
......................................................................

EC sync: Properly handle VBERROR return codes from vb2api_ec_sync

Some return codes were missed when implementing this initially; the vboot
logic can require the system to command the EC to reboot to its RO, switch
RW slots or it can require a poweroff of the SoC.  This patch appropriately
handles these return codes.

BUG=b:145768046
BRANCH=firmware-hatch-12672.B
TEST=ODM verified this patch fixes the issues seen.

Change-Id: I2748cf626d49c255cb0274cb336b072dcdf8cded
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37562
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/security/vboot/ec_sync.c
1 file changed, 32 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/security/vboot/ec_sync.c b/src/security/vboot/ec_sync.c
index 8a3ba71..ecceff5 100644
--- a/src/security/vboot/ec_sync.c
+++ b/src/security/vboot/ec_sync.c
@@ -16,6 +16,7 @@
 #include <console/console.h>
 #include <delay.h>
 #include <ec/google/chromeec/ec.h>
+#include <halt.h>
 #include <security/vboot/misc.h>
 #include <security/vboot/vbnv.h>
 #include <security/vboot/vboot_common.h>
@@ -60,9 +61,38 @@
    retval = vb2api_ec_sync(ctx);
    vboot_save_nvdata_only(ctx);

-	if (retval != VB2_SUCCESS) {
-		printk(BIOS_ERR, "EC software sync failed (%#x), rebooting\n", retval);
+	switch (retval) {
+	case VB2_SUCCESS:
+		break;
+
+	case VBERROR_EC_REBOOT_TO_RO_REQUIRED:
+		printk(BIOS_INFO, "EC Reboot requested. Doing cold reboot\n");
+		if (google_chromeec_reboot(0, EC_REBOOT_COLD, 0))
+			printk(BIOS_EMERG, "Failed to get EC to cold reboot\n");
+
+		halt();
+		break;
+
+	/* Only for EC-EFS */
+	case VBERROR_EC_REBOOT_TO_SWITCH_RW:
+		printk(BIOS_INFO, "Switch EC slot requested. Doing cold reboot\n");
+		if (google_chromeec_reboot(0, EC_REBOOT_COLD,
+						EC_REBOOT_FLAG_SWITCH_RW_SLOT))
+			printk(BIOS_EMERG, "Failed to get EC to cold reboot\n");
+
+		halt();
+		break;
+
+	case VBERROR_REBOOT_REQUIRED:
+		printk(BIOS_INFO, "Reboot requested. Doing warm reboot\n");
    	vboot_reboot();
+		break;
+
+	default:
+		printk(BIOS_ERR, "EC software sync failed (%#x),"
+			" rebooting\n", retval);
+		vboot_reboot();
+		break;
    }

timestamp_add_now(TS_END_EC_SYNC);