Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP
Platforms using postcar are with RELOCATABLE_RAMSTAGE=y. They don't benefit from having low-memory set as writeback-cacheable.
This also fixes regression from CB:34893 that caused some random hangs with more recent intel SoCs in ramstage.
Change-Id: Ia66910a6c85286f5c05823b87d48edc7e4ad9541 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/postcar_loader.c 1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/35161/1
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 6a7d389..61a9d52 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -125,9 +125,6 @@ if (pcf->skip_common_mtrr) return;
- /* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ - postcar_frame_add_mtrr(pcf, 0, CACHE_TMP_RAMTOP, MTRR_TYPE_WRBACK); - /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(pcf, MTRR_TYPE_WRPROT); }
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1: Code-Review-1
kindly help to write test on what all board you have seen this is fixing regression ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
kindly help to write test on what all board you have seen this is fixing regression ?
Personally I have not witnessed this to fix a regressionn on any board. Going by the reports and feedback in CB:34893. To remove this WB CACHE_TMP_RAMPTOP and to generally take a different approach with this common_mtrrs() was discussed with Aaron even before the regressions were discovered.
As for test coverage; toolchain, architecture and subsystem maintenance (and maintainers) often take the liberty of not targeting for 100% test coverage on hardware before code merges. That's the only way we ever get anything to evolve around here. Take it to the mailing list or coreboot leadership if you want to see that change, but as an organisation coreboot does not currently run any automated testrigs that I know of.
That said: Subrata, you had been added as reviewer to CB:34893 Aug 18th. I only see your feedback on it Aug 27th, the day after it was submitted to master. In the future, to avoid see regressions on master branch, I advice you take the time to checkout any changes you see touch your codebase and run them on your rigs.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1:
That said: Subrata, you had been added as reviewer to CB:34893 Aug 18th. I only see your feedback on it Aug 27th, the day after it was submitted to master. In the future, to avoid see regressions on master branch, I advice you take the time to checkout any changes you see touch your codebase and run them on your rigs.
You have merged the CL within 2 hr after +2 by Aaron, i didn't expect this CL will get +2 so soon hence didn't paid much attentions over the things i have in my hand. if you would be kept your CL after +2 for atleast in my day time, i would have given a heads up after verification. IMHO, TEST line is must.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... PS1, Line 129: Do we think there's a benefit for some platform to have this? If so, we can just add a field that positively directs the common code to do this in strut postcar_frame.
Something like:
if (pcf->enable_caching_low_memory) postcar_frame_add_mtrr(...);
If we don't want to bother I'm perfectly nuking it all together. Let me know, and I can throw a CL together.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... PS1, Line 129:
Do we think there's a benefit for some platform to have this? If so, we can just add a field that po […]
Seems PARALLEL_MP init sets final MTRRs for BSP before memcpy() of smmstub. So no, I don't have any platform in mind that would benefit from this.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... PS1, Line 129:
Seems PARALLEL_MP init sets final MTRRs for BSP before memcpy() of smmstub. […]
Sounds good. Thanks for the confirmation, Kyösti.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... File src/arch/x86/postcar_loader.c:
https://review.coreboot.org/c/coreboot/+/35161/1/src/arch/x86/postcar_loader... PS1, Line 129:
Sounds good. Thanks for the confirmation, Kyösti.
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1: -Code-Review
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35161/1//COMMIT_MSG@14 PS1, Line 14: Do you mind adding the following line to commit message:
BUG=b:140250314
This is just for internal tracking. If you want, I can edit and update the commit message. Thanks!
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 1: Code-Review+2
Hello Aaron Durbin, Subrata Banik, Arthur Heymans, Kane Chen, Tim Wawrzynczak, Shelley Chen, David Wu, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35161
to look at the new patch set (#2).
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP
Platforms using postcar are with RELOCATABLE_RAMSTAGE=y. They don't benefit from having low-memory set as writeback-cacheable.
This also fixes regression from CB:34893 that caused some random hangs with more recent intel SoCs in ramstage.
BUG=b:140250314
Change-Id: Ia66910a6c85286f5c05823b87d48edc7e4ad9541 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/postcar_loader.c 1 file changed, 0 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/35161/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35161/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35161/1//COMMIT_MSG@14 PS1, Line 14:
Do you mind adding the following line to commit message: […]
Done
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/35161 )
Change subject: arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP ......................................................................
arch/x86: Remove WB attribute from 0..CACHE_TMP_RAMTOP
Platforms using postcar are with RELOCATABLE_RAMSTAGE=y. They don't benefit from having low-memory set as writeback-cacheable.
This also fixes regression from CB:34893 that caused some random hangs with more recent intel SoCs in ramstage.
BUG=b:140250314
Change-Id: Ia66910a6c85286f5c05823b87d48edc7e4ad9541 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35161 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/arch/x86/postcar_loader.c 1 file changed, 0 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c index 6a7d389..61a9d52 100644 --- a/src/arch/x86/postcar_loader.c +++ b/src/arch/x86/postcar_loader.c @@ -125,9 +125,6 @@ if (pcf->skip_common_mtrr) return;
- /* Cache RAM as WB from 0 -> CACHE_TMP_RAMTOP. */ - postcar_frame_add_mtrr(pcf, 0, CACHE_TMP_RAMTOP, MTRR_TYPE_WRBACK); - /* Cache the ROM as WP just below 4GiB. */ postcar_frame_add_romcache(pcf, MTRR_TYPE_WRPROT); }
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Subrata Banik (Code Review)