Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPU differs ......................................................................
nb/intel/sandybridge: Void MRC cache if CPU differs
Native raminit asserts that the DIMMs haven't been replaced before reusing the saved training data. However, it does not check if the CPU is still the same, so it can end up happily reusing data from an Ivy Bridge CPU onto a Sandy Bridge CPU, which runs the raminit_ivy.c code path. This can make the CPU run in unsupported configurations, which may result in an unstable system, or a failure to boot.
To prevent that, ensure that the stored CPUID matches the CPUID of the installed CPU. If they differ, print a message and do not use the saved data. As it does not pose a problem for a regular boot, but precludes resuming from S3, use different loglevels depending on the bootpath.
Tested on Asus P8Z77-V LX2 with an i7-2600 and an i5-3330, works well.
Change-Id: Ib0691f1f849b567579f6afa845c9460e14f8fa27 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit.c 1 file changed, 19 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39734/1
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index f083f37..ecdcc75 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -241,7 +241,7 @@ ramctr_timing ctrl; spd_raw_data spds[4]; struct region_device rdev; - ramctr_timing *ctrl_cached; + ramctr_timing *ctrl_cached = NULL;
MCHBAR32(SAPMCTL) |= 1;
@@ -268,14 +268,25 @@
/* Try to find timings in MRC cache */ err = mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev); - if (err || (region_device_sz(&rdev) < sizeof(ctrl))) { - if (s3resume) { - /* Failed S3 resume, reset to come up cleanly */ - system_reset(); - } - ctrl_cached = NULL; - } else { + + if (!err && !(region_device_sz(&rdev) < sizeof(ctrl))) ctrl_cached = rdev_mmap_full(&rdev); + + /* Before reusing training data, assert that the CPU has not been replaced */ + if (ctrl_cached && cpuid != ctrl_cached->cpu) { + + /* It is not really worrying on a cold boot, but fatal when resuming from S3 */ + printk(s3resume ? BIOS_ALERT : BIOS_NOTICE, + "CPUID %x differs from stored CPUID %x, CPU was replaced!\n", + cpuid, ctrl_cached->cpu); + + /* Invalidate the stored data, it likely does not apply to the current CPU */ + ctrl_cached = NULL; + } + + if (s3resume && !ctrl_cached) { + /* S3 resume is impossible, reset to come up cleanly */ + system_reset(); }
/* Verify MRC cache for fast boot */
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPU differs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG@7 PS2, Line 7: CPU CPU generations? It would still fail if a SandyBridge CPU was replaced with a SandyBridge CPU.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPU differs ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG@7 PS2, Line 7: CPU
CPU generations? […]
Well, it will detect something if the other SNB CPU has a different CPUID...
Wait, if I'm checking the CPUID, why not use `CPUID`?
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39734
to look at the new patch set (#4).
Change subject: nb/intel/sandybridge: Void MRC cache if CPUID differs ......................................................................
nb/intel/sandybridge: Void MRC cache if CPUID differs
Native raminit asserts that the DIMMs haven't been replaced before reusing the saved training data. However, it does not check if the CPU is still the same, so it can end up happily reusing data from an Ivy Bridge CPU onto a Sandy Bridge CPU, which runs the raminit_ivy.c code path. This can make the CPU run in unsupported configurations, which may result in an unstable system, or a failure to boot.
To prevent that, ensure that the stored CPUID matches the CPUID of the installed CPU. If they differ, print a message and do not use the saved data. As it does not pose a problem for a regular boot, but precludes resuming from S3, use different loglevels depending on the bootpath.
Tested on Asus P8Z77-V LX2 with an i7-2600 and an i5-3330, works well.
Change-Id: Ib0691f1f849b567579f6afa845c9460e14f8fa27 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit.c 1 file changed, 19 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/39734/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPUID differs ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39734/2//COMMIT_MSG@7 PS2, Line 7: CPU
Well, it will detect something if the other SNB CPU has a different CPUID... […]
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPUID differs ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39734 )
Change subject: nb/intel/sandybridge: Void MRC cache if CPUID differs ......................................................................
nb/intel/sandybridge: Void MRC cache if CPUID differs
Native raminit asserts that the DIMMs haven't been replaced before reusing the saved training data. However, it does not check if the CPU is still the same, so it can end up happily reusing data from an Ivy Bridge CPU onto a Sandy Bridge CPU, which runs the raminit_ivy.c code path. This can make the CPU run in unsupported configurations, which may result in an unstable system, or a failure to boot.
To prevent that, ensure that the stored CPUID matches the CPUID of the installed CPU. If they differ, print a message and do not use the saved data. As it does not pose a problem for a regular boot, but precludes resuming from S3, use different loglevels depending on the bootpath.
Tested on Asus P8Z77-V LX2 with an i7-2600 and an i5-3330, works well.
Change-Id: Ib0691f1f849b567579f6afa845c9460e14f8fa27 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39734 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M src/northbridge/intel/sandybridge/raminit.c 1 file changed, 19 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 2a3e4d7..9b524a3 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -239,7 +239,7 @@ ramctr_timing ctrl; spd_raw_data spds[4]; struct region_device rdev; - ramctr_timing *ctrl_cached; + ramctr_timing *ctrl_cached = NULL;
MCHBAR32(SAPMCTL) |= 1;
@@ -266,14 +266,25 @@
/* Try to find timings in MRC cache */ err = mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev); - if (err || (region_device_sz(&rdev) < sizeof(ctrl))) { - if (s3resume) { - /* Failed S3 resume, reset to come up cleanly */ - system_reset(); - } - ctrl_cached = NULL; - } else { + + if (!err && !(region_device_sz(&rdev) < sizeof(ctrl))) ctrl_cached = rdev_mmap_full(&rdev); + + /* Before reusing training data, assert that the CPU has not been replaced */ + if (ctrl_cached && cpuid != ctrl_cached->cpu) { + + /* It is not really worrying on a cold boot, but fatal when resuming from S3 */ + printk(s3resume ? BIOS_ALERT : BIOS_NOTICE, + "CPUID %x differs from stored CPUID %x, CPU was replaced!\n", + cpuid, ctrl_cached->cpu); + + /* Invalidate the stored data, it likely does not apply to the current CPU */ + ctrl_cached = NULL; + } + + if (s3resume && !ctrl_cached) { + /* S3 resume is impossible, reset to come up cleanly */ + system_reset(); }
/* Verify MRC cache for fast boot */
-
Angel Pons (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)