Marshall Dawson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31160
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
soc/amd/stoneyridge: Reboot if missing MRC cache info
AGESA doesn't detect invalid NV data during AmdInitResume(). In cases where the data has been erased, or cannot be found, reboot the system. Otherwise the user will experience a hang when cbmem isn't recovered and the postcar frame cannot be initialized.
BUG=b:122725586 TEST=Write S3 NV save data with 0xff and force reboot
Change-Id: Ib3cf2515f300decd3de198f7741660d95ee4c744 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/s3/s3_resume.c 1 file changed, 21 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31160/1
diff --git a/src/soc/amd/common/block/s3/s3_resume.c b/src/soc/amd/common/block/s3/s3_resume.c index 04414e8..adbda64 100644 --- a/src/soc/amd/common/block/s3/s3_resume.c +++ b/src/soc/amd/common/block/s3/s3_resume.c @@ -16,7 +16,9 @@
#include <stage_cache.h> #include <mrc_cache.h> +#include <reset.h> #include <console/console.h> +#include <soc/southbridge.h> #include <amdblocks/s3_resume.h>
/* Training data versioning is not supported or tracked. */ @@ -33,11 +35,25 @@ } *base = rdev_mmap_full(&rdev); *size = region_device_sz(&rdev); - if (!*base || !*size) - printk(BIOS_ERR, "Error: S3 NV data not found\n"); - else - printk(BIOS_SPEW, "S3 NV data @0x%p 0x%0zx total bytes\n", - *base, *size); + if (!*base || !*size) { + printk(BIOS_ERR, "Error: S3 NV data not found, rebooting...\n"); + set_pm1cnt_s5(); + board_reset(); + } + + int i; + uint8_t erased = 0xff; + uint8_t *s3nv = *base; + for (i = 0 ; i < *size ; i++) + erased &= *(s3nv + i); + + if (erased == 0xff) { + printk(BIOS_ERR, "Error: S3 NV data invalid, rebooting...\n"); + set_pm1cnt_s5(); + board_reset(); + } + + printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", *base, *size); }
void get_s3vol_info(void **base, size_t *size)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31160/1/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/1/src/soc/amd/common/block/s3/s3_resum... PS1, Line 34: return; Wouldn't we want to reboot here as well?
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31160
to look at the new patch set (#2).
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
soc/amd/stoneyridge: Reboot if missing MRC cache info
AGESA doesn't detect invalid NV data during AmdInitResume(). In cases where the data has been erased, or cannot be found, reboot the system. Otherwise the user will experience a hang when cbmem isn't recovered and the postcar frame cannot be initialized.
BUG=b:122725586 TEST=Write S3 NV save data with 0xff and force reboot
Change-Id: Ib3cf2515f300decd3de198f7741660d95ee4c744 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/s3/s3_resume.c 1 file changed, 24 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31160/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31160/1/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/1/src/soc/amd/common/block/s3/s3_resum... PS1, Line 34: return;
Wouldn't we want to reboot here as well?
As a matter of style, I agree. I haven't looked to see whether mrc_cache_get_current() is kind enough to write base/size to 0 if there's an error. I'll add it.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 2:
(1 comment)
Does this influence the resume time?
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... PS2, Line 50: ; Remove the space?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 2:
Does this influence the resume time?
It seems to add about 26 usecs.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... PS2, Line 50: size And do we need to check the full size? Would 16 or 32 bytes be sufficient?
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... PS2, Line 50: size
And do we need to check the full size? Would 16 or 32 bytes be sufficient?
Good call. It's not like we're trying to fully check the integrity, although that might be a nice todo.
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31160
to look at the new patch set (#3).
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
soc/amd/stoneyridge: Reboot if missing MRC cache info
AGESA doesn't detect invalid NV data during AmdInitResume(). In cases where the data has been erased, or cannot be found, reboot the system. Otherwise the user will experience a hang when cbmem isn't recovered and the postcar frame cannot be initialized.
BUG=b:122725586 TEST=Write S3 NV save data with 0xff and force reboot
Change-Id: Ib3cf2515f300decd3de198f7741660d95ee4c744 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/s3/s3_resume.c 1 file changed, 24 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31160/3
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/2/src/soc/amd/common/block/s3/s3_resum... PS2, Line 50: size
Good call. […]
That should probably happen inside AGESA itself. I think the seeming lack of any sort of check is a significant oversight.
https://review.coreboot.org/#/c/31160/3/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/3/src/soc/amd/common/block/s3/s3_resum... PS3, Line 50: for (i = 0 ; i < 4 ; i++) Address Paul's comment?
for (i = 0; i < 4; i++)
Hello build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31160
to look at the new patch set (#4).
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
soc/amd/stoneyridge: Reboot if missing MRC cache info
AGESA doesn't detect invalid NV data during AmdInitResume(). In cases where the data has been erased, or cannot be found, reboot the system. Otherwise the user will experience a hang when cbmem isn't recovered and the postcar frame cannot be initialized.
BUG=b:122725586 TEST=Write S3 NV save data with 0xff and force reboot
Change-Id: Ib3cf2515f300decd3de198f7741660d95ee4c744 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/s3/s3_resume.c 1 file changed, 24 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/31160/4
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31160/3/src/soc/amd/common/block/s3/s3_resum... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/#/c/31160/3/src/soc/amd/common/block/s3/s3_resum... PS3, Line 50: for (i = 0 ; i < 4 ; i++)
Address Paul's comment? […]
I took it more as a suggestion than a request. I don't think we have hard and fast rules on separation within a for() statement, and the above is always my preference. But meh, I don't want to argue over it.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
Patch Set 4: Code-Review+2
Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31160 )
Change subject: soc/amd/stoneyridge: Reboot if missing MRC cache info ......................................................................
soc/amd/stoneyridge: Reboot if missing MRC cache info
AGESA doesn't detect invalid NV data during AmdInitResume(). In cases where the data has been erased, or cannot be found, reboot the system. Otherwise the user will experience a hang when cbmem isn't recovered and the postcar frame cannot be initialized.
BUG=b:122725586 TEST=Write S3 NV save data with 0xff and force reboot
Change-Id: Ib3cf2515f300decd3de198f7741660d95ee4c744 Signed-off-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-on: https://review.coreboot.org/c/31160 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com --- M src/soc/amd/common/block/s3/s3_resume.c 1 file changed, 24 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/common/block/s3/s3_resume.c b/src/soc/amd/common/block/s3/s3_resume.c index 04414e8..eb0148f 100644 --- a/src/soc/amd/common/block/s3/s3_resume.c +++ b/src/soc/amd/common/block/s3/s3_resume.c @@ -16,28 +16,44 @@
#include <stage_cache.h> #include <mrc_cache.h> +#include <reset.h> #include <console/console.h> +#include <soc/southbridge.h> #include <amdblocks/s3_resume.h>
/* Training data versioning is not supported or tracked. */ #define DEFAULT_MRC_VERSION 0
+static void reboot_from_resume(const char *message) /* Does not return */ +{ + printk(BIOS_ERR, "%s", message); + set_pm1cnt_s5(); + board_reset(); +} + void get_s3nv_info(void **base, size_t *size) { struct region_device rdev;
if (mrc_cache_get_current(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, - &rdev)) { - printk(BIOS_ERR, "mrc_cache_get_current returned error\n"); - return; - } + &rdev)) + reboot_from_resume("mrc_cache_get_current error, rebooting.\n"); + *base = rdev_mmap_full(&rdev); *size = region_device_sz(&rdev); if (!*base || !*size) - printk(BIOS_ERR, "Error: S3 NV data not found\n"); - else - printk(BIOS_SPEW, "S3 NV data @0x%p 0x%0zx total bytes\n", - *base, *size); + reboot_from_resume("Error: S3 NV data not found, rebooting.\n"); + + /* Read 16 bytes to infer if the NV has been erased from flash. */ + int i; + uint32_t erased = 0xffffffff; + for (i = 0; i < 4; i++) + erased &= read32((uint32_t *)*base + i); + + if (erased == 0xffffffff) + reboot_from_resume("Error: S3 NV data invalid, rebooting.\n"); + + printk(BIOS_SPEW, "S3 NV data @0x%p, 0x%0zx bytes\n", *base, *size); }
void get_s3vol_info(void **base, size_t *size)
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)
-
Paul Menzel (Code Review)