Change in ...coreboot[master]: coreboot: Run mainboard specific code before Cr50 reset

List overview All Threads
Download

newer

older

Change in ...coreboot[master]:...

Keith Short (Code Review)

19 Apr 2019 19 Apr '19

9:07 p.m.

Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32365

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

coreboot: Run mainboard specific code before Cr50 reset

When coreboot checks the TPM and key-ladder state it issues a reboot of the Cr50 with a delay parameter. Older Cr50 code doesn't support the delay parameter and reboots immediately, which prevented coreboot from running the mainboard specific code needed for the AP to come back up.

This change calls mainboard_prepare_cr50_reset() prior to sending the VENDOR_CC_IMMEDIATE_RESET command.

This change also removes a false error message from the coreboot log that indicated an "Unexpected Cr50 TPM mode 3".

BUG=b:130830178 BRANCH=none TEST=build coreboot on sarien and grunt platforms. TEST=Run 'gsctool -a -m disable; reboot'. Verify corebot send the VENDOR_CC_IMMEDIATE_RESET command and that the AP boots normally.

Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Signed-off-by: Keith Short keithshort@chromium.org --- M src/vendorcode/google/chromeos/cr50_enable_update.c 1 file changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32365/1

diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index 91a10cb..315f9dc 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -68,7 +68,7 @@ * This is not an expected state, as the Cr50 always sets the TPM mode * to TPM_MODE_ENABLED_TENTATIVE during any TPM reset action. */ - if (tpm_mode != TPM_MODE_ENABLED_TENTATIVE) { + if (!cr50_must_reset && tpm_mode != TPM_MODE_ENABLED_TENTATIVE) { printk(BIOS_NOTICE, "NOTICE: Unexpected Cr50 TPM mode (%d). " "A Cr50 reset is required.\n", tpm_mode); @@ -79,6 +79,14 @@ if (!cr50_must_reset) return 0;

+ /* + * Give mainboard a chance to take action - note that older Cr50 + * firmware didn't support the timeout argument and performed an + * immediate reset. If the Cr50 honors the timeout request then + * mainboard_prepare_cr50_reset() is called again without harm. + */ + mainboard_prepare_cr50_reset(); + ret = tlcl_cr50_immediate_reset(timeout_ms);

if (ret != TPM_SUCCESS) {

Attachments:

attachment.htm (text/html — 4.4 KB)

Show replies by date

Furquan Shaikh (Code Review)

19 Apr 19 Apr

11:48 p.m.

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

Patch Set 1:

(4 comments)

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

https://review.coreboot.org/#/c/32365/1//COMMIT_MSG@18 PS1, Line 18: Unexpected Cr50 TPM mode 3 Isn't TPM mode 3 actually an error: https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master...

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 36: cr50_reset_if_needed Can we split this function into two?

cr50_is_reset_needed() --> returns true like it does right now, but does not reset

cr50_reset_now() --> calls tlcl_cr50_immediate_reset(...)

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 147: cr50_reset_if_needed(C50_RESET_DELAY_MS) This can be then changed to: cr50_reset_reqd = cr50_is_reset_needed();

if (!cr50_reset_reqd) return;

// print message // elog add event

That way the logging would happen irrespective of cr50 firmware version.

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 163: and you can add the reset here: if (cr50_reset_reqd) cr50_reset_now(C50_RESET_DELAY_MS);

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 1 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Fri, 19 Apr 2019 23:48:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Keith Short (Code Review)

22 Apr 22 Apr

7:24 p.m.

Hello Julius Werner, build bot (Jenkins), Matt Delco, Joel Kitching,

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

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

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

coreboot: Run mainboard specific code before Cr50 reset

This change calls mainboard_prepare_cr50_reset() prior to sending the VENDOR_CC_IMMEDIATE_RESET command.

This change also fixes a false error message from the coreboot log that indicated "Unexpected Cr50 TPM mode 3" when the Cr50 key ladder is disabled.

BUG=b:130830178 BRANCH=none TEST=build coreboot on sarien and grunt platforms. TEST=Load Cr50 v3.15, run 'gsctool -a -m disable; reboot'. Verify corebot send the VENDOR_CC_IMMEDIATE_RESET command and that the AP boots normally. Verify event log shows "cr50 Reset Required" TEST=Force Cr50 automatic update. Verify event log shows "cr50 Update Reset".

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32365/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 2 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-MessageType: newpatchset

Keith Short (Code Review)

7:26 p.m.

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32365 )

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

Patch Set 2:

(4 comments)

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

https://review.coreboot.org/#/c/32365/1//COMMIT_MSG@18 PS1, Line 18: Unexpected Cr50 TPM mode 3

...

Isn't TPM mode 3 actually an error: https://chromium.googlesource. […]

When the Cr50 key ladder is disabled, the Cr50 returns VENDOR_RC_INTERNAL_ERROR and does not return the current TPM mode in the TPM response. tlcl_cr50_get_tpm_mode() sets the tpm_mode output parameter to TPM_MODE_INVALID in this path. I'll update the commit message to note that this was only a false error when the key ladder is disabled.

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 36: cr50_reset_if_needed

...

Can we split this function into two? […]

Done, but I skipped creating the cr50_reset_now() helper function and call tlcl_cr50_immediate_reset() directly from enable_update().

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 147: cr50_reset_if_needed(C50_RESET_DELAY_MS)

...

This can be then changed to: […]

Done

https://review.coreboot.org/#/c/32365/1/src/vendorcode/google/chromeos/cr50_... PS1, Line 163:

...

and you can add the reset here: […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 2 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Mon, 22 Apr 2019 19:26:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

7:53 p.m.

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32365/2/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/#/c/32365/2/src/vendorcode/google/chromeos/cr50_... PS2, Line 155: likely fail at the recovery : * screen. fails at recovery screen? or fails verification and ends up at recovery screen?

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 2 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Mon, 22 Apr 2019 19:53:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Keith Short (Code Review)

8:01 p.m.

Hello Julius Werner, build bot (Jenkins), Matt Delco, Joel Kitching,

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

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

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

coreboot: Run mainboard specific code before Cr50 reset

This change calls mainboard_prepare_cr50_reset() prior to sending the VENDOR_CC_IMMEDIATE_RESET command.

This change also fixes a false error message from the coreboot log that indicated "Unexpected Cr50 TPM mode 3" when the Cr50 key ladder is disabled.

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32365/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 3 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-MessageType: newpatchset

Keith Short (Code Review)

8:02 p.m.

Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32365 )

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/32365/2/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/#/c/32365/2/src/vendorcode/google/chromeos/cr50_... PS2, Line 155: likely fail at the recovery : * screen.

...

fails at recovery screen? or fails verification and ends up at recovery screen?

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 3 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Furquan Shaikh furquan@google.com Gerrit-Comment-Date: Mon, 22 Apr 2019 20:02:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh furquan@google.com Gerrit-MessageType: comment

Furquan Shaikh (Code Review)

8:16 p.m.

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

Patch Set 3: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32365/3/src/vendorcode/google/chromeos/cr50_... File src/vendorcode/google/chromeos/cr50_enable_update.c:

https://review.coreboot.org/#/c/32365/3/src/vendorcode/google/chromeos/cr50_... PS3, Line 152: ret != TPM_SUCCESS I believe it is okay if this fails after having called mainboard_prepare_cr50_reset. Currently, there is just one board using it so difficult to speculate what side-effects it could have.

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 3 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Mon, 22 Apr 2019 20:16:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Patrick Georgi (Code Review)

23 Apr 23 Apr

10:21 a.m.

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

Change subject: coreboot: Run mainboard specific code before Cr50 reset ......................................................................

coreboot: Run mainboard specific code before Cr50 reset

This change calls mainboard_prepare_cr50_reset() prior to sending the VENDOR_CC_IMMEDIATE_RESET command.

This change also fixes a false error message from the coreboot log that indicated "Unexpected Cr50 TPM mode 3" when the Cr50 key ladder is disabled.

Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Signed-off-by: Keith Short keithshort@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32365 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/vendorcode/google/chromeos/cr50_enable_update.c 1 file changed, 34 insertions(+), 25 deletions(-)

Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved

diff --git a/src/vendorcode/google/chromeos/cr50_enable_update.c b/src/vendorcode/google/chromeos/cr50_enable_update.c index 91a10cb..f2cdbfd 100644 --- a/src/vendorcode/google/chromeos/cr50_enable_update.c +++ b/src/vendorcode/google/chromeos/cr50_enable_update.c @@ -23,7 +23,7 @@ #include <security/vboot/vboot_common.h> #include <vendorcode/google/chromeos/chromeos.h>

-#define C50_RESET_DELAY_MS 1000 +#define CR50_RESET_DELAY_MS 1000

void __weak mainboard_prepare_cr50_reset(void) {}

@@ -31,12 +31,11 @@ * Check if the Cr50 TPM state requires a chip reset of the Cr50 device. * * Returns 0 if the Cr50 TPM state is good or if the TPM_MODE command is - * unsupported. Returns 1 if the Cr50 was reset. + * unsupported. Returns 1 if the Cr50 requires a reset. */ -static int cr50_reset_if_needed(uint16_t timeout_ms) +static int cr50_is_reset_needed(void) { int ret; - int cr50_must_reset = 0; uint8_t tpm_mode;

ret = tlcl_cr50_get_tpm_mode(&tpm_mode); @@ -53,7 +52,7 @@ * Cr50 indicated a reboot is required to restore TPM * functionality. */ - cr50_must_reset = 1; + return 1; } else if (ret != TPM_SUCCESS) { /* TPM command failed, continue booting. */ printk(BIOS_ERR, @@ -61,7 +60,8 @@ return 0; }

- /* If the TPM mode is not enabled-tentative, then the TPM mode is locked + /* + * If the TPM mode is not enabled-tentative, then the TPM mode is locked * and cannot be changed. Perform a Cr50 reset because vboot may need * to disable TPM as part of booting an untrusted OS. * @@ -72,30 +72,17 @@ printk(BIOS_NOTICE, "NOTICE: Unexpected Cr50 TPM mode (%d). " "A Cr50 reset is required.\n", tpm_mode); - cr50_must_reset = 1; + return 1; }

/* If TPM state is okay, no reset needed. */ - if (!cr50_must_reset) - return 0; - - ret = tlcl_cr50_immediate_reset(timeout_ms); - - if (ret != TPM_SUCCESS) { - /* TPM command failed, continue booting. */ - printk(BIOS_ERR, - "ERROR: Attempt to reset CR50 failed: %x\n", - ret); - return 0; - } - - /* Cr50 is about to be reset, caller needs to prepare */ - return 1; + return 0; }

static void enable_update(void *unused) { int ret; + int cr50_reset_reqd = 0; uint8_t num_restored_headers;

/* Nothing to do on recovery mode. */ @@ -112,7 +99,7 @@ }

/* Reboot in 1000 ms if necessary. */ - ret = tlcl_cr50_enable_update(C50_RESET_DELAY_MS, + ret = tlcl_cr50_enable_update(CR50_RESET_DELAY_MS, &num_restored_headers);

if (ret != TPM_SUCCESS) { @@ -134,9 +121,10 @@ */

/* - * If the Cr50 was not reset, continue booting. + * If the Cr50 doesn't requires a reset, continue booting. */ - if (!cr50_reset_if_needed(C50_RESET_DELAY_MS)) + cr50_reset_reqd = cr50_is_reset_needed(); + if (!cr50_reset_reqd) return;

printk(BIOS_INFO, "Waiting for CR50 reset to enable TPM.\n"); @@ -153,6 +141,27 @@ /* clear current post code avoid chatty eventlog on subsequent boot*/ post_code(0);

+ /* + * Older Cr50 firmware doesn't support the timeout parameter for the + * immediate reset request, so the reset request must be sent after + * the mainboard specific code runs. + */ + if (cr50_reset_reqd) { + ret = tlcl_cr50_immediate_reset(CR50_RESET_DELAY_MS); + + if (ret != TPM_SUCCESS) { + /* + * Reset request failed due to TPM error, continue + * booting but the current boot will likely end up at + * the recovery screen. + */ + printk(BIOS_ERR, + "ERROR: Attempt to reset CR50 failed: %x\n", + ret); + return; + } + } + if (CONFIG(POWER_OFF_ON_CR50_UPDATE)) poweroff(); halt();

-- To view, visit https://review.coreboot.org/c/coreboot/+/32365 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib05c9cfde8e87daffd4233114263de5b30822872 Gerrit-Change-Number: 32365 Gerrit-PatchSet: 4 Gerrit-Owner: Keith Short keithshort@chromium.org Gerrit-Reviewer: Furquan Shaikh furquan@google.com Gerrit-Reviewer: Joel Kitching kitching@google.com Gerrit-Reviewer: Julius Werner jwerner@chromium.org Gerrit-Reviewer: Keith Short keithshort@chromium.org Gerrit-Reviewer: Matt Delco delco@chromium.org Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

1986

days inactive

1990

days old

coreboot-gerrit@coreboot.org

8 comments

3 participants

tags (0)

participants (3)

Furquan Shaikh (Code Review)
Keith Short (Code Review)
Patrick Georgi (Code Review)