Keith Short has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32771
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
coreboot: add post code for critical CBFS failures
Add a new post code POST_INVALID_CBS, used when coreboot fails to locate or validate a resource that is stored in CBFS.
BUG=b:124401932 BRANCH=sarien TEST=build coreboot for sarien and arcada platforms
Change-Id: If1c8b92889040f9acd6250f847db02626809a987 Signed-off-by: Keith Short keithshort@chromium.org --- M src/arch/x86/postcar_loader.c M src/include/console/post_codes.h M src/soc/intel/quark/romstage/fsp2_0.c 3 files changed, 13 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32771/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 9975931..e2a01cd 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -141,17 +141,17 @@ };
if (prog_locate(prog)) { - post_code(POST_INVALID_ROM); + post_code(POST_INVALID_CBFS); die("Failed to locate after CAR program.\n"); } if (rmodule_stage_load(&rsl)) { - post_code(POST_INVALID_ROM); + post_code(POST_INVALID_CBFS); die("Failed to load after CAR program.\n"); }
/* Set the stack pointer within parameters of the program loaded. */ if (rsl.params == NULL) { - post_code(POST_INVALID_ROM); + post_code(POST_INVALID_CBFS); die("No parameters found in after CAR program.\n"); }
diff --git a/src/include/console/post_codes.h b/src/include/console/post_codes.h index 89a65ca..637083a 100644 --- a/src/include/console/post_codes.h +++ b/src/include/console/post_codes.h @@ -326,6 +326,13 @@ #define POST_INVALID_ROM 0xE0
/** + * \brief Invalid or corrupt CBFS + * + * Set if firmware failed to find or validate a resource that is stored in CBFS. + */ +#define POST_INVALID_CBFS 0xE1 + +/** * \brief TPM failure * * An error with the TPM, either unexepcted state or communications failure. diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index 31e130a..280ae37 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -116,8 +116,10 @@
/* Locate the RMU data file in flash */ rmu_data = locate_rmu_file(&rmu_data_len); - if (!rmu_data) + if (!rmu_data) { + post_code(POST_INVALID_CBFS); die("Microcode file (rmu.bin) not found."); + }
/* Locate the configuration data from devicetree.cb */ dev = pcidev_path_on_root(LPC_DEV_FUNC);
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
Patch Set 1: Code-Review+2
Hello Patrick Rudolph, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32771
to look at the new patch set (#2).
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
coreboot: add post code for critical CBFS failures
Add a new post code POST_INVALID_CBS, used when coreboot fails to locate or validate a resource that is stored in CBFS.
BUG=b:124401932 BRANCH=sarien TEST=build coreboot for sarien and arcada platforms
Change-Id: If1c8b92889040f9acd6250f847db02626809a987 Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/console/post_codes.h M src/soc/intel/quark/romstage/fsp2_0.c 2 files changed, 9 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32771/2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32771/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32771/2//COMMIT_MSG@9 PS2, Line 9: POST_INVALID_CBS _CBFS
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... PS2, Line 120: die_with_post_code(POST_INVALID_CBFS Unrelated
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: coreboot: add post code for critical CBFS failures ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... PS2, Line 120: die_with_post_code(POST_INVALID_CBFS
Unrelated
Sorry, topic and commit message are wrong. Please update them first.
Hello Patrick Rudolph, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32771
to look at the new patch set (#3).
Change subject: post_code: add post code for critical CBFS failures ......................................................................
post_code: add post code for critical CBFS failures
Add a new post code POST_INVALID_CBFS, used when coreboot fails to locate or validate a resource that is stored in CBFS.
BUG=b:124401932 BRANCH=sarien TEST=build coreboot for sarien and arcada platforms
Change-Id: If1c8b92889040f9acd6250f847db02626809a987 Signed-off-by: Keith Short keithshort@chromium.org --- M src/include/console/post_codes.h M src/soc/intel/quark/romstage/fsp2_0.c 2 files changed, 9 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32771/3
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS, why is only one platform updated?
Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
why is only one platform updated?
Can you clarify? Are you asking why this change stack only provides die_notify() on the sarien platform? Currently this is the only platform with an EC that defines LED flashing codes for specific boot failures.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
Can you clarify? Are you asking why this change stack only provides die_notify() on the sarien plat […]
That's not clear from the commit message. As you are touching shared code and post codes can be read without an EC that defines LED flashing code, a broader code coverage is desirable.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
That's not clear from the commit message. […]
I think this gets back to the question I asked on the mailing list of what the expectations from the coreboot community are for companies doing work in the project. It seems like you're asking that improvements done in one place be done everywhere. While that's a great goal, I don't think that's really been expected or required up to this point. Let's discuss this in the next leadership meeting along with the other topics of what's expected from a company working in the coreboot project.
We've got 620 instances of 'die(', so it's a lot to expect someone to go through and upgrade them all, or even to figure out what needs to be updated.
Keith, with all that said, could you make this same fix in soc/intel/quark/romstage/fsp1_1.c?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
Patch Set 3:
(1 comment)
Oop, that file was deleted. I was looking in an old tree. Nevermind about that Keith.
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/32771/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32771/2//COMMIT_MSG@9 PS2, Line 9: POST_INVALID_CBS
_CBFS
Done
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/2/src/soc/intel/quark/romstage/fsp2_0.... PS2, Line 120: die_with_post_code(POST_INVALID_CBFS
Sorry, topic and commit message are wrong. Please update them first.
Done
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
I think this gets back to the question I asked on the mailing list of what the expectations from the […]
Ack. No extra file to update
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
Ack. […]
The commit message doesn't state that you only touch one platform, so I assume you want to update all.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
The commit message doesn't state that you only touch one platform, so I assume you want to update al […]
@Martin I'm fine if you don't update all occurrences, but you need to communicate why. In this case as there are to many of them and the help of the community and platform maintainers is needed. If you document the post codes in Documentation and advertise the new feature on the mailing list, everybody can contribute and improve the situation.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
@Martin […]
Sure - None of what you just said was clear from your original statement of "a broader code coverage is needed", but now that I understand what you're asking for, it's more actionable.
Jett, could you update the commit message for each of these to specify the platform and then update coreboot/Documentation/POSTCODES.
Jett Rink has uploaded a new patch set (#4) to the change originally created by Keith Short. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
post_code: add post code for critical CBFS failures
Add a new post code POST_INVALID_CBFS, used when coreboot fails to locate or validate a resource that is stored in CBFS.
BUG=b:124401932 BRANCH=sarien TEST=build coreboot for sarien and arcada platforms
Change-Id: If1c8b92889040f9acd6250f847db02626809a987 Signed-off-by: Keith Short keithshort@chromium.org --- M Documentation/POSTCODES M src/include/console/post_codes.h M src/soc/intel/quark/romstage/fsp2_0.c 3 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/32771/4
Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... File src/soc/intel/quark/romstage/fsp2_0.c:
https://review.coreboot.org/#/c/32771/3/src/soc/intel/quark/romstage/fsp2_0.... PS3, Line 120: die_with_post_code(POST_INVALID_CBFS,
Sure - None of what you just said was clear from your original statement of "a broader code coverage […]
Add documentation in Documentation/POSTCODES for each CL
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
Patch Set 4: Code-Review+2
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32771 )
Change subject: post_code: add post code for critical CBFS failures ......................................................................
post_code: add post code for critical CBFS failures
Add a new post code POST_INVALID_CBFS, used when coreboot fails to locate or validate a resource that is stored in CBFS.
BUG=b:124401932 BRANCH=sarien TEST=build coreboot for sarien and arcada platforms
Change-Id: If1c8b92889040f9acd6250f847db02626809a987 Signed-off-by: Keith Short keithshort@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32771 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M Documentation/POSTCODES M src/include/console/post_codes.h M src/soc/intel/quark/romstage/fsp2_0.c 3 files changed, 10 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/Documentation/POSTCODES b/Documentation/POSTCODES index 2340fac..162e863 100644 --- a/Documentation/POSTCODES +++ b/Documentation/POSTCODES @@ -17,6 +17,7 @@ 0x88 Devices have been configured 0x89 Devices have been enabled 0xe0 Boot media (e.g. SPI ROM) is corrupt +0xe1 Resource stored within CBFS is corrupt 0xf8 Entry into elf boot 0xf3 Jumping to payload
diff --git a/src/include/console/post_codes.h b/src/include/console/post_codes.h index 775f78d..7bd1ee0 100644 --- a/src/include/console/post_codes.h +++ b/src/include/console/post_codes.h @@ -326,6 +326,13 @@ #define POST_INVALID_ROM 0xe0
/** + * \brief Invalid or corrupt CBFS + * + * Set if firmware failed to find or validate a resource that is stored in CBFS. + */ +#define POST_INVALID_CBFS 0xe1 + +/** * \brief TPM failure * * An error with the TPM, either unexepcted state or communications failure. diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index 2ec16c9..e4abcc0 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -116,7 +116,8 @@ /* Locate the RMU data file in flash */ rmu_data = locate_rmu_file(&rmu_data_len); if (!rmu_data) - die("Microcode file (rmu.bin) not found."); + die_with_post_code(POST_INVALID_CBFS, + "Microcode file (rmu.bin) not found.");
/* Locate the configuration data from devicetree.cb */ dev = pcidev_path_on_root(LPC_DEV_FUNC);
-
Duncan Laurie (Code Review) -
Jett Rink (Code Review)
-
Keith Short (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)