Change in ...coreboot[master]: coreboot: add post code for failure to load next stage

[Keith Short (Code Review)]

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

coreboot: add post code for failure to load next stage

Add a new post code, POST_INVALID_ROM, used when coreboot fails to
locate or validate a resource that is stored in ROM.

BUG=b:124401932
BRANCH=sarien
TEST=build coreboot for sarien and arcada platforms

Change-Id: Ie6de6590595d8fcdc57ad156237fffa03d5ead38
Signed-off-by: Keith Short 
---
M src/arch/x86/postcar_loader.c
M src/include/console/post_codes.h
M src/lib/prog_loaders.c
M src/security/vboot/vboot_logic.c
4 files changed, 25 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/32770/1

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index d62487e..9975931 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -140,14 +140,20 @@
    	.prog = prog,
    };

-	if (prog_locate(prog))
+	if (prog_locate(prog)) {
+		post_code(POST_INVALID_ROM);
    	die("Failed to locate after CAR program.\n");
-	if (rmodule_stage_load(&rsl))
+	}
+	if (rmodule_stage_load(&rsl)) {
+		post_code(POST_INVALID_ROM);
    	die("Failed to load after CAR program.\n");
+	}

/* Set the stack pointer within parameters of the program loaded. */
-	if (rsl.params == NULL)
+	if (rsl.params == NULL) {
+		post_code(POST_INVALID_ROM);
    	die("No parameters found in after CAR program.\n");
+	}

finalize_load(rsl.params, pcf->stack);

diff --git a/src/include/console/post_codes.h b/src/include/console/post_codes.h
index f482ae9..89a65ca 100644
--- a/src/include/console/post_codes.h
+++ b/src/include/console/post_codes.h
@@ -319,6 +319,13 @@
 #define POST_JUMPING_TO_PAYLOAD			0xf3

/**
+ * \brief Invalid or corrupt ROM
+ *
+ * Set if firmware failed to find or validate a resource that is stored in ROM.
+ */
+#define POST_INVALID_ROM			0xE0
+
+/**
  * \brief TPM failure
  *
  * An error with the TPM, either unexepcted state or communications failure.
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 3b77712..16eb746 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -68,8 +68,10 @@
    prog_run(&romstage);

fail:
-	if (CONFIG(BOOTBLOCK_CONSOLE))
+	if (CONFIG(BOOTBLOCK_CONSOLE)) {
+		post_code(POST_INVALID_ROM);
    	die("Couldn't load romstage.\n");
+	}
    halt();
 }

diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index df34490..c61dfa9 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -319,8 +319,10 @@
    /* Enable measured boot mode */
    if (CONFIG(VBOOT_MEASURED_BOOT) &&
    	!(ctx.flags & VB2_CONTEXT_S3_RESUME)) {
-		if (vboot_init_crtm() != VB2_SUCCESS)
+		if (vboot_init_crtm() != VB2_SUCCESS) {
+			post_code(POST_INVALID_ROM);
    		die("Initializing measured boot mode failed!");
+		}
    }

if (get_recovery_mode_switch()) {
@@ -391,8 +393,10 @@

printk(BIOS_INFO, "Phase 4\n");
    rv = locate_firmware(&ctx, &fw_main);
-	if (rv)
+	if (rv) {
+		post_code(POST_INVALID_ROM);
    	die("Failed to read FMAP to locate firmware");
+	}

rv = hash_body(&ctx, &fw_main);
    save_if_needed(&ctx);

[Lijian Zhao (Code Review)]

Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: coreboot: add post code for failure to load next stage
......................................................................

Patch Set 1:

Define a big die_notify within wilco ec to add post code instead of touch all the pieces in coreboot? Though the compare string and then send out different post code will not be pretty.

[Keith Short (Code Review)]

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Define a big die_notify within wilco ec to add post code instead of touch all the pieces in coreboot? Though the compare string and then send out different post code will not be pretty.

Nearly all the die() calls within coreboot do not update the post code, so the actual failure can only be inferred from the operation attempted (e.g. POST_FSP_MEMORY_EXIT would imply a memory failure).  This change stack adds new explicit post codes on calls to die() that could possibly happen due to a hardware failure.

[Aaron Durbin (Code Review)]

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

Patch Set 1:

(1 comment)
> Patch Set 1:
> > Patch Set 1:
> >
> > Define a big die_notify within wilco ec to add post code instead of touch all the pieces in coreboot? Though the compare string and then send out different post code will not be pretty.
>
> Nearly all the die() calls within coreboot do not update the post code, so the actual failure can only be inferred from the operation attempted (e.g. POST_FSP_MEMORY_EXIT would imply a memory failure).  This change stack adds new explicit post codes on calls to die() that could possibly happen due to a hardware failure.

I think what Lance was referring to was to add a helper function/macro that does keeps the open coding from occurring at the call sites. e.g. die_with_postcode(int postcode, const char *msg); or something like that.

https://review.coreboot.org/#/c/32770/1/src/include/console/post_codes.h 
File src/include/console/post_codes.h:

https://review.coreboot.org/#/c/32770/1/src/include/console/post_codes.h@326 
PS1, Line 326: xE
hex constants seem to be lower case in this file (aside from AGESA ones below). Please be consistent.

[Keith Short (Code Review)]

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > Define a big die_notify within wilco ec to add post code instead of touch all the pieces in coreboot? Though the compare string and then send out different post code will not be pretty.
> >
> > Nearly all the die() calls within coreboot do not update the post code, so the actual failure can only be inferred from the operation attempted (e.g. POST_FSP_MEMORY_EXIT would imply a memory failure).  This change stack adds new explicit post codes on calls to die() that could possibly happen due to a hardware failure.
>
> I think what Lance was referring to was to add a helper function/macro that does keeps the open coding from occurring at the call sites. e.g. die_with_postcode(int postcode, const char *msg); or something like that.

Sounds good, I'll add this to the bottom of the stack and refactor.

[Keith Short (Code Review)]

Hello Aaron Durbin, build bot (Jenkins),

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

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

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

coreboot: add post code for failure to load next stage

Add a new post code, POST_INVALID_ROM, used when coreboot fails to
locate or validate a resource that is stored in ROM.

BUG=b:124401932
BRANCH=sarien
TEST=build coreboot for sarien and arcada platforms

Change-Id: Ie6de6590595d8fcdc57ad156237fffa03d5ead38
Signed-off-by: Keith Short 
---
M src/arch/x86/postcar_loader.c
M src/include/console/post_codes.h
M src/lib/prog_loaders.c
M src/security/vboot/vboot_logic.c
4 files changed, 19 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/32770/2

[Aaron Durbin (Code Review)]

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

Change subject: coreboot: add post code for failure to load next stage
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Keith Short (Code Review)]

Hello Aaron Durbin, build bot (Jenkins),

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

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

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

Change subject: post_code: add post code for failure to load next stage
......................................................................

post_code: add post code for failure to load next stage

Add a new post code, POST_INVALID_ROM, used when coreboot fails to
locate or validate a resource that is stored in ROM.

BUG=b:124401932
BRANCH=sarien
TEST=build coreboot for sarien and arcada platforms

Change-Id: Ie6de6590595d8fcdc57ad156237fffa03d5ead38
Signed-off-by: Keith Short 
---
M src/arch/x86/postcar_loader.c
M src/include/console/post_codes.h
M src/lib/prog_loaders.c
M src/security/vboot/vboot_logic.c
4 files changed, 19 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/32770/3

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Martin Roth (Code Review)]

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

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 3: Code-Review+2

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@166 
PS3, Line 166: 	die("Ramstage was not loaded!\n");
why isn't it used here?

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@199 
PS3, Line 199: 		die("Unsupported payload type.\n");
why isn't it used here?

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@205 
PS3, Line 205: 		die("Payload not loaded.\n");
why isn't it used here?

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Jett Rink (Code Review)]

Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/#/c/32770/1/src/include/console/post_codes.h 
File src/include/console/post_codes.h:

https://review.coreboot.org/#/c/32770/1/src/include/console/post_codes.h@326 
PS1, Line 326: xE
> hex constants seem to be lower case in this file (aside from AGESA ones below). […]

Done

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@166 
PS3, Line 166: 	die("Ramstage was not loaded!\n");
> why isn't it used here?

For this specific CL, we are introducing POST_INVALID_ROM and this error scenario would be related to RAM (instead of ROM).

I don't think CL should modify this die statement

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@199 
PS3, Line 199: 		die("Unsupported payload type.\n");
> why isn't it used here?

This appears to be in ramstage so INVALID_ROM seems inappropriate

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@205 
PS3, Line 205: 		die("Payload not loaded.\n");
> why isn't it used here?

This appears to be in ramstage so INVALID_ROM seems inappropriate

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 3: Code-Review-1

(2 comments)

https://review.coreboot.org/#/c/32770/3/src/include/console/post_codes.h 
File src/include/console/post_codes.h:

https://review.coreboot.org/#/c/32770/3/src/include/console/post_codes.h@324 
PS3, Line 324:  * Set if firmware failed to find or validate a resource that is stored in ROM.
Technically the boot media can be written on all platforms, using internal or external programmers. It's not a ROM.

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@166 
PS3, Line 166: 	die("Ramstage was not loaded!\n");
> For this specific CL, we are introducing POST_INVALID_ROM and this error scenario would be related t […]

1. You seem to mix ROM with romstage.
2. Both ramstage and romstage are stored in the same boot media (ROM) and read from it, so here's the point here?

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Martin Roth (Code Review)]

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

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/32770/3/src/include/console/post_codes.h 
File src/include/console/post_codes.h:

https://review.coreboot.org/#/c/32770/3/src/include/console/post_codes.h@324 
PS3, Line 324:  * Set if firmware failed to find or validate a resource that is stored in ROM.
> Technically the boot media can be written on all platforms, using internal or external programmers. […]

Sorry, I'm not sure I understand what you're objecting to here.  Are you objecting to calling the SPI ROM a ROM?  Yeah, it can be updated but we call it ROM all over the place.  Should we rename romstage for the same reason?  flashstage maybe?  This is getting a bit pedantic.  :)

Typically when I hear "boot media" I think what the OS is booting from, not the BIOS.

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@166 
PS3, Line 166: 	die("Ramstage was not loaded!\n");
> You seem to mix ROM with romstage. […]

I agree with Patrick.  coreboot is reading ramstage out of the ROM chip.  This is the identical situation as loading romstage.

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Jett Rink (Code Review)]

Jett Rink has uploaded a new patch set (#4) to the change originally created by Keith Short. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

post_code: add post code for failure to load next stage

Add a new post code, POST_INVALID_ROM, used when coreboot fails to
locate or validate a resource that is stored in ROM.

BUG=b:124401932
BRANCH=sarien
TEST=build coreboot for sarien and arcada platforms

Change-Id: Ie6de6590595d8fcdc57ad156237fffa03d5ead38
Signed-off-by: Keith Short 
---
M Documentation/POSTCODES
M src/arch/x86/postcar_loader.c
M src/include/console/post_codes.h
M src/lib/prog_loaders.c
M src/security/vboot/vboot_logic.c
5 files changed, 24 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/32770/4

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Jett Rink (Code Review)]

Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/32770/3/src/lib/prog_loaders.c@166 
PS3, Line 166: 	die("Ramstage was not loaded!\n");
> I agree with Patrick.  coreboot is reading ramstage out of the ROM chip. […]

Thank you both for the clarification! I updated the 3 references.

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Aaron Durbin (Code Review)]

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

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 10: Code-Review+2

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

Patch Set 10: Code-Review+2

Change in ...coreboot[master]: post_code: add post code for failure to load next stage

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32770 )

Change subject: post_code: add post code for failure to load next stage
......................................................................

post_code: add post code for failure to load next stage

Add a new post code, POST_INVALID_ROM, used when coreboot fails to
locate or validate a resource that is stored in ROM.

BUG=b:124401932
BRANCH=sarien
TEST=build coreboot for sarien and arcada platforms

Change-Id: Ie6de6590595d8fcdc57ad156237fffa03d5ead38
Signed-off-by: Keith Short 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32770
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
Reviewed-by: Duncan Laurie 
---
M Documentation/POSTCODES
M src/arch/x86/postcar_loader.c
M src/include/console/post_codes.h
M src/lib/prog_loaders.c
M src/security/vboot/vboot_logic.c
5 files changed, 24 insertions(+), 9 deletions(-)

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

diff --git a/Documentation/POSTCODES b/Documentation/POSTCODES
index 5d337b6..2340fac 100644
--- a/Documentation/POSTCODES
+++ b/Documentation/POSTCODES
@@ -16,6 +16,7 @@
 0x66 Devices have been enumerated
 0x88 Devices have been configured
 0x89 Devices have been enabled
+0xe0 Boot media (e.g. SPI ROM) is corrupt
 0xf8 Entry into elf boot
 0xf3 Jumping to payload

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index d62487e..e5d0cea 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -141,13 +141,16 @@
    };

if (prog_locate(prog))
-		die("Failed to locate after CAR program.\n");
+		die_with_post_code(POST_INVALID_ROM,
+				   "Failed to locate after CAR program.\n");
    if (rmodule_stage_load(&rsl))
-		die("Failed to load after CAR program.\n");
+		die_with_post_code(POST_INVALID_ROM,
+				   "Failed to load after CAR program.\n");

/* Set the stack pointer within parameters of the program loaded. */
    if (rsl.params == NULL)
-		die("No parameters found in after CAR program.\n");
+		die_with_post_code(POST_INVALID_ROM,
+				   "No parameters found in after CAR program.\n");

finalize_load(rsl.params, pcf->stack);

diff --git a/src/include/console/post_codes.h b/src/include/console/post_codes.h
index f482ae9..775f78d 100644
--- a/src/include/console/post_codes.h
+++ b/src/include/console/post_codes.h
@@ -319,6 +319,13 @@
 #define POST_JUMPING_TO_PAYLOAD			0xf3

/**
+ * \brief Invalid or corrupt ROM
+ *
+ * Set if firmware failed to find or validate a resource that is stored in ROM.
+ */
+#define POST_INVALID_ROM			0xe0
+
+/**
  * \brief TPM failure
  *
  * An error with the TPM, either unexepcted state or communications failure.
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 3b77712..a21663f 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -69,7 +69,8 @@

fail:
    if (CONFIG(BOOTBLOCK_CONSOLE))
-		die("Couldn't load romstage.\n");
+		die_with_post_code(POST_INVALID_ROM,
+				   "Couldn't load romstage.\n");
    halt();
 }

@@ -162,7 +163,7 @@
    prog_run(&ramstage);

fail:
-	die("Ramstage was not loaded!\n");
+	die_with_post_code(POST_INVALID_ROM, "Ramstage was not loaded!\n");
 }

#ifdef __RAMSTAGE__ // gc-sections should take care of this
@@ -195,13 +196,14 @@
    		break;
    	} /* else fall-through */
    default:
-		die("Unsupported payload type.\n");
+		die_with_post_code(POST_INVALID_ROM,
+				   "Unsupported payload type.\n");
    	break;
    }

out:
    if (prog_entry(payload) == NULL)
-		die("Payload not loaded.\n");
+		die_with_post_code(POST_INVALID_ROM, "Payload not loaded.\n");
 }

void payload_run(void)
diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index d4ad327..6263100 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -320,7 +320,8 @@
    if (CONFIG(VBOOT_MEASURED_BOOT) &&
    	!(ctx.flags & VB2_CONTEXT_S3_RESUME)) {
    	if (vboot_init_crtm() != VB2_SUCCESS)
-			die("Initializing measured boot mode failed!");
+			die_with_post_code(POST_INVALID_ROM,
+				"Initializing measured boot mode failed!");
    }

if (get_recovery_mode_switch()) {
@@ -395,7 +396,8 @@
    printk(BIOS_INFO, "Phase 4\n");
    rv = locate_firmware(&ctx, &fw_main);
    if (rv)
-		die("Failed to read FMAP to locate firmware");
+		die_with_post_code(POST_INVALID_ROM,
+			"Failed to read FMAP to locate firmware");

rv = hash_body(&ctx, &fw_main);
    save_if_needed(&ctx);