Change in coreboot[master]: soc/amd/picasso: If psp_verstage is in RO, don't reset on error

List overview All Threads
Download

newer

older

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

Change in coreboot[master]:...

Martin Roth (Code Review)

21 Aug 2020 21 Aug '20

3:53 a.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

soc/amd/picasso: If psp_verstage is in RO, don't reset on error

If there's already been an error and PSP_verstage is booting to RO, don't reset the system. It may be that the error is fatal, but if the system is stuck, don't intentionally force it into a reboot loop.

BUG=None TEST=Force an error, still boots to RO instead of going into a boot loop

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 10 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44652/1

diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index 85fd74b..be515b1 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -29,6 +29,12 @@ subcode += PSP_VBOOT_ERROR_SUBCODE; svc_write_postcode(subcode);

+ /* Continue booting from RO */ + if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) { + printk(BIOS_ERR, "Already in recovery mode. Staying in RO.\n"); + return; + } + vb2api_fail(ctx, VB2_RECOVERY_RO_UNSPECIFIED, (int)subcode); vboot_save_data(ctx);

@@ -231,21 +237,20 @@

verstage_main();

- vb2api_relocate(_vboot2_work, _vboot2_work, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE, - &ctx); + ctx = vboot_get_context(); retval = check_cmos_recovery(); if (retval) - goto err; + reboot_into_recovery(ctx, retval);

post_code(POSTCODE_SAVE_BUFFERS); retval = save_buffers(&ctx); if (retval) - goto err; + reboot_into_recovery(ctx, retval);

post_code(POSTCODE_UPDATE_BOOT_REGION); retval = update_boot_region(ctx); if (retval) - goto err; + reboot_into_recovery(ctx, retval);

post_code(POSTCODE_UNMAP_SPI_ROM); if (boot_dev.base) { @@ -260,9 +265,6 @@

printk(BIOS_DEBUG, "Leaving verstage on PSP\n"); svc_exit(retval); - -err: - reboot_into_recovery(ctx, retval); }

const struct region_device *boot_device_ro(void)

Attachments:

attachment.htm (text/html — 4.9 KB)

Show replies by date

Aaron Durbin (Code Review)

21 Aug 21 Aug

5:10 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44652/1/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:

https://review.coreboot.org/c/coreboot/+/44652/1/src/soc/amd/picasso/psp_ver... PS1, Line 32: /* Continue booting from RO */ You should add the 'why' here from the commit description for fuller context.

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 1 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Fri, 21 Aug 2020 17:10:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Martin Roth (Code Review)

24 Aug 24 Aug

3:59 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

soc/amd/picasso: If psp_verstage is in RO, don't reset on error

BUG=None TEST=Force an error, still boots to RO instead of going into a boot loop

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44652/2

Martin Roth (Code Review)

3:59 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44652/1/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:

https://review.coreboot.org/c/coreboot/+/44652/1/src/soc/amd/picasso/psp_ver... PS1, Line 32: /* Continue booting from RO */

...

You should add the 'why' here from the commit description for fuller context.

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Mon, 24 Aug 2020 15:59:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

4 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... PS2, Line 33: * If there's an error but the PSP_verstage is already booting to RO, code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... PS2, Line 35: * the system is stuck, don't intentionally force it into a reboot loop. code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... PS2, Line 36: */ code indent should use tabs where possible

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Mon, 24 Aug 2020 16:00:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Aaron Durbin (Code Review)

11:41 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... PS2, Line 36: */

...

code indent should use tabs where possible

These are valid indention comments.

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 2 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Mon, 24 Aug 2020 23:41:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

Martin Roth (Code Review)

25 Aug 25 Aug

3:49 p.m.

Hello build bot (Jenkins),

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

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

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

soc/amd/picasso: If psp_verstage is in RO, don't reset on error

BUG=None TEST=Force an error, still boots to RO instead of going into a boot loop

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44652/3

Martin Roth (Code Review)

3:50 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:

https://review.coreboot.org/c/coreboot/+/44652/2/src/soc/amd/picasso/psp_ver... PS2, Line 36: */

...

These are valid indention comments.

Done. The gerrit editor did me wrong.

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 3 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Aaron Durbin adurbin@chromium.org Gerrit-Comment-Date: Tue, 25 Aug 2020 15:50:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Comment-In-Reply-To: Aaron Durbin adurbin@chromium.org Gerrit-MessageType: comment

Aaron Durbin (Code Review)

4:23 p.m.

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

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

Patch Set 3: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 3 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 25 Aug 2020 16:23:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Aaron Durbin (Code Review)

4:23 p.m.

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44652 )

Change subject: soc/amd/picasso: If psp_verstage is in RO, don't reset on error ......................................................................

soc/amd/picasso: If psp_verstage is in RO, don't reset on error

BUG=None TEST=Force an error, still boots to RO instead of going into a boot loop

Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Reviewed-on: https://review.coreboot.org/c/coreboot/+/44652 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 14 insertions(+), 8 deletions(-)

Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved

diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index 005c8b0..d071aa6 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -29,6 +29,16 @@ subcode += PSP_VBOOT_ERROR_SUBCODE; svc_write_postcode(subcode);

+ /* + * If there's an error but the PSP_verstage is already booting to RO, + * don't reset the system. It may be that the error is fatal, but if + * the system is stuck, don't intentionally force it into a reboot loop. + */ + if (ctx->flags & VB2_CONTEXT_RECOVERY_MODE) { + printk(BIOS_ERR, "Already in recovery mode. Staying in RO.\n"); + return; + } + vb2api_fail(ctx, VB2_RECOVERY_RO_UNSPECIFIED, (int)subcode); vboot_save_data(ctx);

@@ -231,21 +241,20 @@

verstage_main();

post_code(POSTCODE_SAVE_BUFFERS); retval = save_buffers(&ctx); if (retval) - goto err; + reboot_into_recovery(ctx, retval);

post_code(POSTCODE_UPDATE_BOOT_REGION); retval = update_boot_region(ctx); if (retval) - goto err; + reboot_into_recovery(ctx, retval);

post_code(POSTCODE_UNMAP_SPI_ROM); if (boot_dev.base) { @@ -260,9 +269,6 @@

printk(BIOS_DEBUG, "Leaving verstage on PSP\n"); svc_exit(retval); - -err: - reboot_into_recovery(ctx, retval); }

const struct region_device *boot_device_ro(void)

-- To view, visit https://review.coreboot.org/c/coreboot/+/44652 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibb6794fefe9d482850ca31b1d3b0d145fcd8bb8f Gerrit-Change-Number: 44652 Gerrit-PatchSet: 4 Gerrit-Owner: Martin Roth martinroth@google.com Gerrit-Reviewer: Aaron Durbin adurbin@chromium.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: merged

1582

days inactive

1586

days old

coreboot-gerrit@coreboot.org

9 comments

3 participants

tags (0)

participants (3)

Aaron Durbin (Code Review)
build bot (Jenkins) (Code Review)
Martin Roth (Code Review)