Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
drivers/amd/agesa/romstage: Only mark cbmem as UC if needed
Now cbmem is flushed to dram before calling postcar stage.
UNTESTED.
Change-Id: Iaa0d154e2c5b2052027d07ad26e31f3ff63ae9f3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/mtrr_fixme.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37197/1
diff --git a/src/drivers/amd/agesa/mtrr_fixme.c b/src/drivers/amd/agesa/mtrr_fixme.c index bbb9eb0..9e63425 100644 --- a/src/drivers/amd/agesa/mtrr_fixme.c +++ b/src/drivers/amd/agesa/mtrr_fixme.c @@ -51,6 +51,10 @@ if (s3resume) return;
+ /* We worry about cbmem hitting dram later */ + if (clflush_supported()) + return; + /* For normal path, INIT_POST has returned with all * memory set WB cacheable. But we need CBMEM as UC * to make CAR teardown with invalidation without
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37197
to look at the new patch set (#3).
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
drivers/amd/agesa/romstage: Only mark cbmem as UC if needed
Now cbmem is flushed to dram before calling postcar stage.
UNTESTED.
Change-Id: Iaa0d154e2c5b2052027d07ad26e31f3ff63ae9f3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/drivers/amd/agesa/mtrr_fixme.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37197/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 7: Code-Review+1
Tested on apu1 (fam14 agesa) and apu2 (fam16 binaryPI)
Hello build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37197
to look at the new patch set (#8).
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
drivers/amd/agesa/romstage: Only mark cbmem as UC if needed
Now cbmem is flushed to dram before calling postcar stage.
Tested on apu1 (fam14 agesa) and apu2 (fam16 binaryPI). Tested on Lenovo G505S (15h) (reaches payload), but might have other problems in master
Change-Id: Iaa0d154e2c5b2052027d07ad26e31f3ff63ae9f3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/amd/agesa/family14/Kconfig M src/cpu/amd/agesa/family15tn/Kconfig M src/cpu/amd/agesa/family16kb/Kconfig M src/drivers/amd/agesa/mtrr_fixme.c 4 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37197/8
Hello build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37197
to look at the new patch set (#11).
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
drivers/amd/agesa/romstage: Only mark cbmem as UC if needed
Now cbmem is flushed to dram before calling postcar stage.
Tested on apu1 (fam14 agesa) and apu2 (fam16 binaryPI). Tested on Lenovo G505S (15h) (reaches payload), but might have other problems in master
Change-Id: Iaa0d154e2c5b2052027d07ad26e31f3ff63ae9f3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/amd/agesa/family14/Kconfig M src/cpu/amd/agesa/family15tn/Kconfig M src/cpu/amd/agesa/family16kb/Kconfig M src/drivers/amd/agesa/mtrr_fixme.c 4 files changed, 12 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37197/11
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12: Code-Review+1
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
I have recently noticed that after apu2 migration to postcar the MTRRs became inconsistent across all cores. The APs have an additional UC entry probably for cbmem?, which stays programmed up to the CPU init in ramstage. Since the postcar frame is built only from WB entries, the BSP has one entry less than APs which causes this inconsistence. Would this patch resolve this issue? Or is this something different?
Anyway I will test this.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
I have recently noticed that after apu2 migration to postcar the MTRRs became inconsistent across all cores. The APs have an additional UC entry probably for cbmem?, which stays programmed up to the CPU init in ramstage. Since the postcar frame is built only from WB entries, the BSP has one entry less than APs which causes this inconsistence. Would this patch resolve this issue? Or is this something different?
Anyway I will test this.
Inconsistent MTRR is acceptable for a short amount of time like during the firmware. If the MTRRs are still incosistent when Linux boots that generally results in hangs.
Postcar (which is in dmar/cbmem) invalidates cache so it must hit dram before that. Making cbmem UC makes that possible but CLFLUSHing it to RAM (if possible) is a cleaner solution. It might also result in (small) boot speedups. This patch would indeed avoid making an UC MTRR 'hole' for cbmem.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12: Code-Review+1
Patch Set 12:
Patch Set 12:
I have recently noticed that after apu2 migration to postcar the MTRRs became inconsistent across all cores. The APs have an additional UC entry probably for cbmem?, which stays programmed up to the CPU init in ramstage. Since the postcar frame is built only from WB entries, the BSP has one entry less than APs which causes this inconsistence. Would this patch resolve this issue? Or is this something different?
Anyway I will test this.
Inconsistent MTRR is acceptable for a short amount of time like during the firmware. If the MTRRs are still incosistent when Linux boots that generally results in hangs.
Postcar (which is in dmar/cbmem) invalidates cache so it must hit dram before that. Making cbmem UC makes that possible but CLFLUSHing it to RAM (if possible) is a cleaner solution. It might also result in (small) boot speedups. This patch would indeed avoid making an UC MTRR 'hole' for cbmem.
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Can you maybe share a bootlog with DISPLAY_MTRR set?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Can you maybe share a bootlog with DISPLAY_MTRR set?
https://pastebin.ubuntu.com/p/6yFNJQwK6g/
build is dirty since apu2 dos not have DISPLAY_MTRR and HAVE_DISPLAY_MTRR configured by default and also added APIC ID before each MTRR dump. Here's the diff: https://pastebin.ubuntu.com/p/567t6zHDsZ/
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Can you maybe share a bootlog with DISPLAY_MTRR set?
https://pastebin.ubuntu.com/p/6yFNJQwK6g/
build is dirty since apu2 dos not have DISPLAY_MTRR and HAVE_DISPLAY_MTRR configured by default and also added APIC ID before each MTRR dump. Here's the diff: https://pastebin.ubuntu.com/p/567t6zHDsZ/
I see what is going on. The BSP MTRR are modified in postcar, but the AP ones are not. Then during the CPU init no syncing is done. So the BSP MTRR don't match the AP ones. The parallel mp init handles this automatically. So moving away from the lapic init would automatically fix it.
Another option is to save the BSP MTRR in model_xx_init() and apply it to the APs.
Anyway, all this is not really related to this patch, is it?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 12:
Patch Set 12:
There must be something in AGESA that still sets the UC entries, keep getting the inconsistent MTRRs (but Linux fixes it by itself.) I will reinvestigate this and probably the uC mapping can be dropped for APs (IIRC it's 1MB from the top of RAM).
I dug a little bit more and notice that the InitPost sets additional MTRRs on BSP and sync APs MTRRs with BSP at some point. This results in [TOP_MEM - 16MB; TOP_MEM) var MTRR to be set as UC while there exists a [0 ; TOP_MEM) WB mapping with var MTRR. After investigating the AGESA code I have, it seems to be the C6 region storage allocated in InitPost (probably C6 is enabled as required per CPU boost). Since there is no UMA on apu2, this is the only reason I suspect. So the question is now whether we really should drop the UC mappings on BSP in the recover_postcar_frame function? Kyösti since you are the author of the mentioned function, maybe you could advice on this?
Can you maybe share a bootlog with DISPLAY_MTRR set?
https://pastebin.ubuntu.com/p/6yFNJQwK6g/
build is dirty since apu2 dos not have DISPLAY_MTRR and HAVE_DISPLAY_MTRR configured by default and also added APIC ID before each MTRR dump. Here's the diff: https://pastebin.ubuntu.com/p/567t6zHDsZ/
I see what is going on. The BSP MTRR are modified in postcar, but the AP ones are not. Then during the CPU init no syncing is done. So the BSP MTRR don't match the AP ones. The parallel mp init handles this automatically. So moving away from the lapic init would automatically fix it.
Another option is to save the BSP MTRR in model_xx_init() and apply it to the APs.
It would be better to do the other way around: restore the lost UC entry on BSP, since it seems to be some HW init part needed for functional C6 state.
Anyway, all this is not really related to this patch, is it?
No it isn't. Sorry if I started a too long discussion in an inappropriate place. Thank you for your opinions.
Hello build bot (Jenkins), Michał Żygowski, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37197
to look at the new patch set (#15).
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
drivers/amd/agesa/romstage: Only mark cbmem as UC if needed
Now cbmem is flushed to dram before calling postcar stage.
Tested on apu1 (fam14 agesa) and apu2 (fam16 binaryPI). Tested on Lenovo G505S (15h) (reaches payload), but might have other problems in master
Change-Id: Iaa0d154e2c5b2052027d07ad26e31f3ff63ae9f3 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/amd/agesa/family14/Kconfig M src/cpu/amd/agesa/family15tn/Kconfig M src/cpu/amd/agesa/family16kb/Kconfig M src/drivers/amd/agesa/mtrr_fixme.c 4 files changed, 13 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37197/15
Attention is currently required from: Arthur Heymans. Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37197 )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Patch Set 17: Code-Review+1
(1 comment)
File src/drivers/amd/agesa/mtrr_fixme.c:
https://review.coreboot.org/c/coreboot/+/37197/comment/db517a55_910b05b5 PS17, Line 48: /* TODO: Don't all AGESA platforms supported CLFLUSH ? */ According to BKDG for fam14 and newer CLFLUSH is supported. Haven't checked older families as they are not in current master. Also the BKDGs explicitly state that CLFLUSH may be used to migrate data to memory after BIOS initializes the DRAM and is done with using CAR. I think it is safe to remove this comment.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/37197?usp=email )
Change subject: drivers/amd/agesa/romstage: Only mark cbmem as UC if needed ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Martin L Roth (Code Review)
-
Michał Żygowski (Code Review)