Change in coreboot[master]: [UNTESTED] nb/intel/ironlake: Reserve gap betwen TSEG and BGSM
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: [UNTESTED] nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
[UNTESTED] nb/intel/ironlake: Reserve gap betwen TSEG and BGSM
There may be a gap between TSEG and the graphics stolen memory due to the alignment done in `raminit.c`. If we allocate MMIO resources in this range, it misbehaves unpredictably, so reserve it.
Change-Id: If305e9751ebf4edc945cf038ed72698f3696e52d Signed-off-by: Nico Huber nico.h@gmx.de --- M src/northbridge/intel/ironlake/northbridge.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/45325/1
diff --git a/src/northbridge/intel/ironlake/northbridge.c b/src/northbridge/intel/ironlake/northbridge.c index 9b7ea85..cf014fe 100644 --- a/src/northbridge/intel/ironlake/northbridge.c +++ b/src/northbridge/intel/ironlake/northbridge.c @@ -92,7 +92,7 @@
static void mc_read_resources(struct device *dev) { - uint32_t tseg_base; + uint32_t tseg_base, tseg_end; uint64_t touud; uint16_t reg16; int index = 3; @@ -102,6 +102,7 @@ mmconf_resource(dev, 0x50);
tseg_base = pci_read_config32(pcidev_on_root(0, 0), TSEG); + tseg_end = tseg_base + CONFIG_SMM_TSEG_SIZE; touud = pci_read_config16(pcidev_on_root(0, 0), TOUUD);
@@ -131,6 +132,11 @@ pci_read_config32(pcidev_on_root(0, 0), IGD_BASE); gtt_base = pci_read_config32(pcidev_on_root(0, 0), GTT_BASE); + if (gtt_base > tseg_end) { + /* Reserve the gap. MMIO doesn't work in this range. Keep + it uncacheable, though, for easier MTRR allocation. */ + mmio_resource(dev, index++, tseg_end >> 10, (gtt_base - tseg_end) >> 10); + } mmio_resource(dev, index++, gtt_base >> 10, uma_size_gtt << 10); mmio_resource(dev, index++, igd_base >> 10, uma_size_igd << 10);
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45325
to look at the new patch set (#2).
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
nb/intel/ironlake: Reserve gap betwen TSEG and BGSM
There may be a gap between TSEG and the graphics stolen memory due to the alignment done in `raminit.c`. If we allocate MMIO resources in this range, it misbehaves unpredictably, so reserve it.
TEST=Booted Thinkpad X201s, allocated resources are above TOLUD.
Change-Id: If305e9751ebf4edc945cf038ed72698f3696e52d Signed-off-by: Nico Huber nico.h@gmx.de --- M src/northbridge/intel/ironlake/northbridge.c 1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/45325/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */ There should be no gap at all, at least that's what the memory map in 322910-003 says. I'd suspect a bug in raminit code, but I don't mind this as a stop-gap solution. However, I'd print a warning/notice about it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */
There should be no gap at all, at least that's what the memory map in 322910-003 says. […]
"Warning: coreboot did what its code says, everything is alright"? ;) I don't think so. If you suspect that the alignment is unnecessary, you can put a comment or printk where it is configured. But this would be the wrong place, IMHO.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */
"Warning: coreboot did what its code says, everything is alright"? ;) […]
What bugs me is that this only seems cause problems now. How did it work in the past? Maybe the old allocator did something different, even if it was because of a bug. Thus, I would prefer if the logs would have clear indications of known potential problems. For the message, I'd use something like:
printk(BIOS_NOTICE, "Reserving unexpected region between top of TSEG and GTT base.\n");
I don't have a strong opinion on this, though. I wouldn't care at all if I had a reliable reference to verify raminit for this platform, but alas.
This is the same reasoning that led me to make CB:43778 (which I abandoned because it got stalled by review comments, and made CB:44155 which also actually fixes the problem in the meantime). There was no clear indication that `root_port_commit_config` had been called. If there had been such a message from the beginning, it would have been much easier to find the problem. Which CB:30077 could have added, btw.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */
What bugs me is that this only seems cause problems now. […]
Another thing that would work is to mention in the commit message that this gap is extraneous, and shouldn't exist. This would at least suggest that raminit.c might have a bug.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */
Another thing that would work is to mention in the commit message that this gap is extraneous, and s […]
I think we might be talking past each other. This is not a workaround. It's a fix for the code here. One always has to expect alignment gaps. So there is nothing unexpected.
Even with the 64MiB down alignment removed, we'd still have to align TSEG to its size (remember SMRR). With 8MiB TSEG and the 4MiB GTT above it, there is still a 4MiB gap.
It worked with the old allocator because that one chooses one contiguous region to allocate everything. The new one simply puts resources where they fit, therefore relies on accurate reservations.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/northbridge.c:
https://review.coreboot.org/c/coreboot/+/45325/2/src/northbridge/intel/ironl... PS2, Line 137: it uncacheable, though, for easier MTRR allocation. */
I think we might be talking past each other. This is not a workaround. […]
Oh, I understand now. Thank you.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45325 )
Change subject: nb/intel/ironlake: Reserve gap betwen TSEG and BGSM ......................................................................
nb/intel/ironlake: Reserve gap betwen TSEG and BGSM
There may be a gap between TSEG and the graphics stolen memory due to the alignment done in `raminit.c`. If we allocate MMIO resources in this range, it misbehaves unpredictably, so reserve it.
TEST=Booted Thinkpad X201s, allocated resources are above TOLUD.
Change-Id: If305e9751ebf4edc945cf038ed72698f3696e52d Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/45325 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/northbridge/intel/ironlake/northbridge.c 1 file changed, 7 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/ironlake/northbridge.c b/src/northbridge/intel/ironlake/northbridge.c index 9b7ea85..cf014fe 100644 --- a/src/northbridge/intel/ironlake/northbridge.c +++ b/src/northbridge/intel/ironlake/northbridge.c @@ -92,7 +92,7 @@
static void mc_read_resources(struct device *dev) { - uint32_t tseg_base; + uint32_t tseg_base, tseg_end; uint64_t touud; uint16_t reg16; int index = 3; @@ -102,6 +102,7 @@ mmconf_resource(dev, 0x50);
tseg_base = pci_read_config32(pcidev_on_root(0, 0), TSEG); + tseg_end = tseg_base + CONFIG_SMM_TSEG_SIZE; touud = pci_read_config16(pcidev_on_root(0, 0), TOUUD);
@@ -131,6 +132,11 @@ pci_read_config32(pcidev_on_root(0, 0), IGD_BASE); gtt_base = pci_read_config32(pcidev_on_root(0, 0), GTT_BASE); + if (gtt_base > tseg_end) { + /* Reserve the gap. MMIO doesn't work in this range. Keep + it uncacheable, though, for easier MTRR allocation. */ + mmio_resource(dev, index++, tseg_end >> 10, (gtt_base - tseg_end) >> 10); + } mmio_resource(dev, index++, gtt_base >> 10, uma_size_gtt << 10); mmio_resource(dev, index++, igd_base >> 10, uma_size_igd << 10);
-
Angel Pons (Code Review) -
Nico Huber (Code Review)