Change in coreboot[master]: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemag...
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and 1 lock bit. If lock bit is set, then systemagent.asl would end up reporting the base address of low MMIO incorrectly i.e. off by 1.
This change masks the lower 20 bits of TOLUD and TOM registers when exposing it in the ACPI tables to ensure that the base address of low MMIO region is reported correctly.
Change-Id: I11b3ef8deda21930998471ab6e712da4c62f5b02 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/broadwell/acpi/systemagent.asl 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41471/1
diff --git a/src/soc/intel/broadwell/acpi/systemagent.asl b/src/soc/intel/broadwell/acpi/systemagent.asl index c73322f..f37af41 100644 --- a/src/soc/intel/broadwell/acpi/systemagent.asl +++ b/src/soc/intel/broadwell/acpi/systemagent.asl @@ -147,13 +147,19 @@
// Fix up PCI memory region // Start with Top of Lower Usable DRAM + // Lower 20 bits of TOLUD register need to be masked since they contain lock and + // reserved bits. Store (^MCHC.TLUD, Local0) + And (Local0, ShiftLeft(0xfff, 20), Local0) Store (^MCHC.MEBA, Local1)
// Check if ME base is equal If (LEqual (Local0, Local1)) { // Use Top Of Memory instead Store (^MCHC.TOM, Local0) + // Lower 20 bits of TOM register need to be masked since they contain lock and + // reserved bits. + And (Local0, ShiftLeft(0x7ffff, 20), Local0) }
Store (Local0, PMIN)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/1/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/1/src/soc/intel/broadwell/acp... PS1, Line 153: And (Local0, ShiftLeft(0xfff, 20), Local0) Why use the old syntax here?
Hello build bot (Jenkins), Subrata Banik, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41471
to look at the new patch set (#2).
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and 1 lock bit. If lock bit is set, then systemagent.asl would end up reporting the base address of low MMIO incorrectly i.e. off by 1.
This change masks the lower 20 bits of TOLUD and TOM registers when exposing it in the ACPI tables to ensure that the base address of low MMIO region is reported correctly.
Change-Id: I11b3ef8deda21930998471ab6e712da4c62f5b02 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/broadwell/acpi/systemagent.asl 1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41471/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/1/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/1/src/soc/intel/broadwell/acp... PS1, Line 153: And (Local0, ShiftLeft(0xfff, 20), Local0)
Why use the old syntax here?
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 2: Code-Review+2
Hello build bot (Jenkins), Angel Pons, Subrata Banik, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41471
to look at the new patch set (#5).
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and 1 lock bit. If lock bit is set, then systemagent.asl would end up reporting the base address of low MMIO incorrectly i.e. off by 1.
This change masks the lower 20 bits of TOLUD and TOM registers when exposing it in the ACPI tables to ensure that the base address of low MMIO region is reported correctly.
Change-Id: I11b3ef8deda21930998471ab6e712da4c62f5b02 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/broadwell/acpi/systemagent.asl 1 file changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/71/41471/5
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl
Lower 20bits of TOLUD and TOLM registers include 19 reserved bits and 1 lock bit. If lock bit is set, then systemagent.asl would end up reporting the base address of low MMIO incorrectly i.e. off by 1.
This change masks the lower 20 bits of TOLUD and TOM registers when exposing it in the ACPI tables to ensure that the base address of low MMIO region is reported correctly.
Change-Id: I11b3ef8deda21930998471ab6e712da4c62f5b02 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41471 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/broadwell/acpi/systemagent.asl 1 file changed, 6 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/broadwell/acpi/systemagent.asl b/src/soc/intel/broadwell/acpi/systemagent.asl index 74a25c1..258e6e7 100644 --- a/src/soc/intel/broadwell/acpi/systemagent.asl +++ b/src/soc/intel/broadwell/acpi/systemagent.asl @@ -147,13 +147,17 @@
// Fix up PCI memory region // Start with Top of Lower Usable DRAM - Local0 = ^MCHC.TLUD + // Lower 20 bits of TOLUD register need to be masked since they contain lock and + // reserved bits. + Local0 = ^MCHC.TLUD & (0xfff << 20) Local1 = ^MCHC.MEBA
// Check if ME base is equal If (Local0 == Local1) { // Use Top Of Memory instead - Local0 = ^MCHC.TOM + // Lower 20 bits of TOM register need to be masked since they contain lock and + // reserved bits. + Local0 = ^MCHC.TOM & (0x7ffff << 20) }
PMIN = Local0
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3610 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3609 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3608 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3607
Please note: This test is under development and might not be accurate at all!
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
Why was this not fixed in the whole tree?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... PS6, Line 156: If (Local0 == Local1) { I don't think any chipset code reserves this region. I don't know why we can't repurpose the ME space in this case. In most cases it is covered by remapped DRAM maybe that is allowed but PCI resources are not? In any case, we need to align C and ASL code.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... PS6, Line 156: If (Local0 == Local1) {
I don't think any chipset code reserves this region. I don't know why we […]
Thinking further about it, I don't think this reservation is necessary. If we had exactly 4GiB memory installed. ME would cover the space directly below 4GiB. Which includes things that are forwarded to DMI for sure, e.g. ROM and the RCBA which was often at 0xfed1c000.
Intel documentation writers don't seem to know it very well, either. Just saw a memory map chapter in the TGL BIOS Spec, which says all the stolen regions have to be reserved, and the list includes the ME space. That doesn't make any sense in most cases :-/
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
Patch Set 6:
Why was this not fixed in the whole tree?
Was any platform missed? I had done a grep for TLUD in *.asl files:
# git grep -l TOLUD *.asl src/northbridge/intel/gm45/acpi/hostbridge.asl src/northbridge/intel/i945/acpi/hostbridge.asl src/northbridge/intel/pineview/acpi/hostbridge.asl src/northbridge/intel/x4x/acpi/hostbridge.asl src/soc/intel/apollolake/acpi/northbridge.asl src/soc/intel/broadwell/acpi/systemagent.asl src/soc/intel/common/block/acpi/acpi/northbridge.asl src/soc/intel/skylake/acpi/systemagent.asl
Out of these northbridge/intel seem to handle the TOLUD in a very different way where they refer to only the address bits in TOLUD. Hence, no masking required.
apollolake was already doing the right thing.
I pushed changes for broadwell, skylake and intel/common.
Were there more platforms that required changes?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Why was this not fixed in the whole tree?
Was any platform missed? I had done a grep for TLUD in *.asl files:
Um, yes, `git grep TLUD reveals them. :)
Were there more platforms that required changes?
Haswell (which is most likely exactly the same as Broadwell in separate dirs) and Sandy Bridge at least.
I would like to have some conclusion about the ME range, though, before we continue. Should we drop that? It seems safe enough, to me...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6:
Patch Set 6:
Why was this not fixed in the whole tree?
Was any platform missed? I had done a grep for TLUD in *.asl files:
Um, yes, `git grep TLUD reveals them. :)
Were there more platforms that required changes?
Haswell (which is most likely exactly the same as Broadwell in separate dirs) and Sandy Bridge at least.
Ah yes. I see those now. Pushed changes here: https://review.coreboot.org/c/coreboot/+/41976
I would like to have some conclusion about the ME range, though, before we continue. Should we drop that? It seems safe enough, to me...
I have pushed the changes for sandybridge and haswell since they fix a known issue. Let me revisit your comment on the ME range.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... PS6, Line 156: If (Local0 == Local1) {
I don't think any chipset code reserves this region.
Are you saying that it should never be the case that TOLUD == MEBA?
I don't know why we can't repurpose the ME space in this case.
Repurpose ME space for performing allocation for other devices? That wouldn't really work. Or, maybe I misunderstood your comment?
If we had exactly 4GiB memory installed. ME would cover the space directly
below 4GiB. Which includes things that are forwarded to DMI for sure, e.g. ROM and the RCBA which was often at 0xfed1c000.
In that case, that part of the DRAM would be remapped above 4G and hence ME region would still be below TOLM?
Just saw a memory map chapter in the TGL BIOS Spec, which says all the stolen
regions have to be reserved, and the list includes the ME space. That doesn't make any sense in most cases
Why is that?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41471 )
Change subject: soc/intel/broadwell: Mask lower 20 bits of TOLUD and TOLM in systemagent.asl ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... File src/soc/intel/broadwell/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/41471/6/src/soc/intel/broadwell/acp... PS6, Line 156: If (Local0 == Local1) {
I don't think any chipset code reserves this region. […]
The ME stolen memory is always directly below TOM and TOM is always the amount of installed memory. AFAIK, it is never mapped into the host memory space.
In the 4GiB case, it lives in DRAM at 0xfe000000..0xffffffff.
There is another inconsistency: Assuming we would have 3GiB installed, ME is 0xbe000000..0xbfffffff. If we can live with a 1GiB memory hole, TOLUD could be at 0xce000000, too. And this code here would use TOM (0xc0000000) instead. But if we'd set a 2GiB hole, TOLUD would be 0x80000000, and this code would ignore the ME range, that is still at 0xbe000000.
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)