Change in coreboot[master]: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default
awokd@danwin1210.me has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default
Incorrect comparison operator led to code never executing.
Change-Id: I78061491c16fbb51acfd8347e1eae011ff19a390 Signed-off-by: Joe Moore awokd@danwin1210.me --- M src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36000/1
diff --git a/src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c b/src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c index 57483a9..0bdc7cb 100644 --- a/src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c +++ b/src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c @@ -149,7 +149,7 @@ }
// restore variable MTRR6 and MTRR7 to default states - for (msrno = 0x20F; msrno <= 0x20C; msrno--) // decrement so that the pair is disable before the base is cleared + for (msrno = 0x20F; msrno >= 0x20C; msrno--) // decrement so that the pair is disable before the base is cleared __writemsr (msrno, 0);
// Enable fixed-range and variable-range MTRRs
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 1: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG@7 PS1, Line 7: Fix set Set
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG@10 PS1, Line 10: Does it fix a real problem, or is it just code review?
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 1:
Patch Set 1:
(2 comments)
Per the comments inline in code, it was intended to clear those MTTRs, but never could. Whether clearing them resolves or hopefully doesn't create some other issue is unknown- it's from Coverity code review.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 1:
Leaving the variable MTRRs unclean may lead to MTRR overlapping, which follows the rules below:
1. If the memory types are identical, then that memory type is used. 2. If at least one of the memory types is UC, the UC memory type is used. 3. If at least one of the memory types is WT, and the only other memory type is WB, then the WT memory type is used. 4. If the combination of memory types does not match any above, then the memory type used is undefined.
In the worst case (4) it may lead to undefined behaviours. A sane bootloader, OS should check for that, however how it can determine then, which MTRR is/was correct?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG@10 PS1, Line 10:
Does it fix a real problem, or is it just code review?
Please add a tag line *Found-by* for Coverity, if Coverity found it. Please see `git log` for possible formats.
Hello Jacob Garber, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36000
to look at the new patch set (#2).
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default
Incorrect comparison operator led to code never executing.
Change-Id: I78061491c16fbb51acfd8347e1eae011ff19a390 Signed-off-by: Joe Moore awokd@danwin1210.me Found-by: Coverity CID 1241824 --- M src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/36000/2
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
Patch Set 1:
Please add a tag line *Found-by* for Coverity
Added.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36000/2/src/vendorcode/amd/agesa/f1... File src/vendorcode/amd/agesa/f12/Proc/CPU/cahalt.c:
https://review.coreboot.org/c/coreboot/+/36000/2/src/vendorcode/amd/agesa/f1... PS2, Line 152: for (msrno = 0x20F; msrno >= 0x20C; msrno--) With more or less unmaintained code I think it is better to test this on hardware as fixing some bugs (no-ops) may unravel others...
BTW to any AGESA experts: this code seems to be run on AP's. Does coreboot redo this later/earlier on?
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
Patch Set 2:
I think it is better to test this on hardware
FWIW, this effort was spawned by a mailing list thread wanting AGESA cleanup due to hte quantity of Coverity issues: https://www.mail-archive.com/coreboot@coreboot.org/msg54032.html. I have access to only one of the models in question, so won't be able to test the majority of corrections on actual hardware. How would it be best to proceed?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
I think it is better to test this on hardware
FWIW, this effort was spawned by a mailing list thread wanting AGESA cleanup due to hte quantity of Coverity issues: https://www.mail-archive.com/coreboot@coreboot.org/msg54032.html. I have access to only one of the models in question, so won't be able to test the majority of corrections on actual hardware. How would it be best to proceed?
If you find similar issues across multiple AGESA platforms of which you can only test one, you can have a reasonable expectation that it will work for others too. Otherwise you can always ask on the mailing list for tests, assuming there is no maintainer for these platforms anymore. If after a sufficiently long time no-one seems interested, it might be better to simply drop the platform.
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
Patch Set 2:
If after a sufficiently long time no-one seems interested, it might be better to simply drop the platform.
Agree it would be best to test these types of changes on actual hardware, and understand the need to drop specific platforms if they are no longer in use. However, the email thread is talking about dropping AGESA entirely, which I don't want to happen because I use it. If I triage the Coverity issues for platforms I don't have access to with a proposed bug fix and email to the list, will that be good enough to keep AGESA alive, at least for the platforms still in use? I'm hoping the majority of issues won't be actual bug fixes like this one, just code cleanup, which could make it somewhat moot.
awokd@danwin1210.me has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Patch Set 2:
(1 comment)
Abandoning in favor of https://review.coreboot.org/c/coreboot/+/36187.
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36000/1//COMMIT_MSG@10 PS1, Line 10:
Please add a tag line *Found-by* for Coverity, if Coverity found it. […]
Done
awokd@danwin1210.me has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36000 )
Change subject: vendorcode/amd/agesa/f12/Proc/CPU: Fix set MTRR6 and MTRR7 to default ......................................................................
Abandoned
Abandoning in favor of https://review.coreboot.org/c/coreboot/+/36187.
-
Arthur Heymans (Code Review) -
Michał Żygowski (Code Review)
-
Name of user not set (Code Review)
-
Paul Menzel (Code Review)