Change in coreboot[master]: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G
This change sets the base for MMIO above 4G to TOUDD. It matches what is used by resource allocator if MMIO resources are allocated above 4G and also matches the expectation in northbridge.asl.
BUG=b:149186922 TEST=Verified that kernel does not complain about MMIO windows above 4G.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ibbbfbdad867735a43cf57c256bf206a3f040f383 --- M src/soc/intel/common/block/systemagent/systemagent.c 1 file changed, 31 insertions(+), 28 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/41155/1
diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index d0e171d..9354c25 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -42,34 +42,6 @@ }
/* - * This function will get above 4GB mmio enable config specific to soc. - * - * Return values: - * 0 = Above 4GB memory is not enable - * 1 = Above 4GB memory is enable - */ -static int get_enable_above_4GB_mmio(void) -{ - const struct soc_intel_common_config *common_config; - common_config = chip_get_common_soc_structure(); - - return common_config->enable_above_4GB_mmio; -} - -/* Fill MMIO resource above 4GB into GNVS */ -void sa_fill_gnvs(global_nvs_t *gnvs) -{ - if (get_enable_above_4GB_mmio()) { - gnvs->e4gm = 1; - gnvs->a4gb = ABOVE_4GB_MEM_BASE_ADDRESS; - gnvs->a4gs = ABOVE_4GB_MEM_BASE_SIZE; - printk(BIOS_DEBUG, - "PCI space above 4GB MMIO is from 0x%llx to len = 0x%llx\n", - gnvs->a4gb, gnvs->a4gs); - } -} - -/* * Add all known fixed MMIO ranges that hang off the host bridge/memory * controller device. */ @@ -125,6 +97,37 @@ *result = value; }
+/* + * This function will get above 4GB mmio enable config specific to soc. + * + * Return values: + * 0 = Above 4GB memory is not enable + * 1 = Above 4GB memory is enable + */ +static int get_enable_above_4GB_mmio(void) +{ + const struct soc_intel_common_config *common_config; + common_config = chip_get_common_soc_structure(); + + return common_config->enable_above_4GB_mmio; +} + +/* Fill MMIO resource above 4GB into GNVS */ +void sa_fill_gnvs(global_nvs_t *gnvs) +{ + if (!get_enable_above_4GB_mmio()) + return; + + struct device *sa_dev = pcidev_path_on_root(SA_DEVFN_ROOT); + + gnvs->e4gm = 1; + sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb); + gnvs->a4gs = ABOVE_4GB_MEM_BASE_SIZE; + printk(BIOS_DEBUG, "PCI space above 4GB MMIO is from 0x%llx to len = 0x%llx\n", + gnvs->a4gb, gnvs->a4gs); +} + + static void sa_get_mem_map(struct device *dev, uint64_t *values) { int i;
Hello Duncan Laurie, Subrata Banik, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41155
to look at the new patch set (#2).
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G
This change sets the base for MMIO above 4G to TOUDD. It matches what is used by resource allocator if MMIO resources are allocated above 4G and also matches the expectation in northbridge.asl. This change also gets rid of the macro ABOVE_4GB_MEM_BASE_ADDRESS since it is now unused.
BUG=b:149186922 TEST=Verified that kernel does not complain about MMIO windows above 4G.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ibbbfbdad867735a43cf57c256bf206a3f040f383 --- M src/soc/intel/apollolake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/include/soc/iomap.h M src/soc/intel/jasperlake/include/soc/iomap.h M src/soc/intel/skylake/include/soc/iomap.h M src/soc/intel/tigerlake/include/soc/iomap.h 7 files changed, 31 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/41155/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41155/2//COMMIT_MSG@9 PS2, Line 9: TOUDD TOUUD.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 56: enable_above_4GB_mmio so we can get rid of this?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 56: enable_above_4GB_mmio
so we can get rid of this?
guess not as it's used in next patch.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 56: enable_above_4GB_mmio
guess not as it's used in next patch.
Oh. And below. I clearly wasn't reading closely enough...
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio()) Why wouldn't we always report the usable address space above 4GiB? It seems odd to have this option. Subrata?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
Why wouldn't we always report the usable address space above 4GiB? It seems odd to have this option. […]
That is what I am trying to track down. I found this CL from Subrata: https://review.coreboot.org/c/coreboot/+/38125, but I don't really understand the commit message "Publishing unnecessary 4GB above MMIO resource with wrong base and size is causing problem while working with discrete GPU."
What do you mean by wrong base and size?
BTW, Intel just reported that they are seeing a different behavior w.r.t. hotplug driver even with USB4 if this patch is used v/s only reporting the allocated MMIO window above 4G. I have asked for more details here: https://b.corp.google.com/issues/149186922#comment37
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
That is what I am trying to track down. I found this CL from Subrata: https://review.coreboot. […]
while enabling DGPU with coreboot and chrome, i have seen that device with higher memory requirement might try to access > 4GB memory. And causing such issue when Above 4GB base and limit size is incorrect.
Unable to boot with dGPU on IA platform with below error:
[ 2.297425] pcieport 0000:00:1c.0: PCI bridge to [bus 05] [ 2.302858] pcieport 0000:00:1c.0: bridge window [io 0x2000-0x2fff] [ 2.309427] pcieport 0000:00:1c.0: bridge window [mem 0xb2000000-0xb20fffff] [ 2.316679] pcieport 0000:00:1c.0: bridge window [mem 0x840000000-0x8c01fffff 64bit pref] [ 2.325072] pcieport 0000:00:1c.0: PCI bridge to [bus 05] [ 2.330502] pcieport 0000:00:1c.0: bridge window [io 0x2000-0x2fff] [ 2.337062] pcieport 0000:00:1c.0: bridge window [mem 0xb2000000-0xb20fffff] [ 2.344317] pcieport 0000:00:1c.0: bridge window [mem 0xa0000000-0xb01fffff 64bit pref] [ 2.352541] [drm] Not enough PCI address space for a large BAR.
After making sure we have right base and limit like below CL, we don't see such issue as mentioned above.
https://review.coreboot.org/c/coreboot/+/38125/10/src/soc/intel/cannonlake/i...
Also i could see an FSP UPD which does some additional programming if we like to enable memory above 4GB.
https://github.com/otcshare/CCG-TGL-Generic-SiC/blob/master/TigerLakeFspPkg/...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
And causing such issue when Above 4GB base and limit size is incorrect.
What were the MMIO addresses that were advertised in ACPI tables before and after your change? Do you have dumps of the tables?
After making sure we have right base and limit like below CL, we don't see such issue as mentioned above.
If there was an error in reporting and now it is fixed, why not always advertise the correct MMIO window available above 4G? Why is it made conditional?
Also i could see an FSP UPD which does some additional programming if we like to enable memory above 4GB.
That seems to be used only for ACPI table generation. I didn't really see any different programming happening with that UPD?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
And causing such issue when Above 4GB base and limit size is incorrect.
What were the MMIO addresses that were advertised in ACPI tables before and after your change? Do you have dumps of the tables?
It can be calculated easily
Before https://review.coreboot.org/c/coreboot/+/38125/10/src/soc/intel/common/block... #define BASE_32GB 0x800000000 #define SIZE_16GB 0x400000000
Store (BASE_32GB, MMIN) Store (SIZE_16GB, MLEN)
After if we have enable_above_4GB_mmio = 0 then didn't broadcast anything for upper 4GB as size is 0
If (LEqual (A4GS, 0)) { CreateQwordField (MCRS, PM02._LEN, MSEN) Store (0, MSEN) }
Else when enable_above_4GB_mmio = 1
#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
above address will get broadcast ed. Align with Intel RC code
After making sure we have right base and limit like below CL, we don't see such issue as mentioned above.
If there was an error in reporting and now it is fixed, why not always advertise the correct MMIO window available above 4G? Why is it made conditional?
I was making this conditional to remain parity with FSP/RC implementation.
Also i could see an FSP UPD which does some additional programming if we like to enable memory above 4GB.
That seems to be used only for ACPI table generation. I didn't really see any different programming happening with that UPD?
I will check this in more details, i think there were some programming required to board case this range as well at host bridge side thats the reason this UPD exist inside RC code else won't include any UPD if just meant for ACPI table generation as ACPI table is not part of RC ocde
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
After making sure we have right base and limit like below CL, we don't see such issue as mentioned above.
If there was an error in reporting and now it is fixed, why not always advertise the correct MMIO window available above 4G? Why is it made conditional?
I was making this conditional to remain parity with FSP/RC implementation.
FSP/RC is oriented around Windows and legacy requirements. It's not necessarily applicable to things outside of that. Could you please look into always advertising the correct decode window above 4GiB? I think that's the simplest and most straight forward way to go. I honestly can't envision any situation where advertising that window would cause things to misbehave unless it's an OS limitation.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
After making sure we have right base and limit like below CL, we don't see such issue as menti […]
Pushed a patch here: https://review.coreboot.org/c/coreboot/+/41276
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
. I honestly can't envision any situation where advertising that window would cause things to misbehave unless it's an OS limitation.
We can try with DGPU kind of usage where device needs higher memory and it tries to bind with memory above 4GB and eventually results into reboot issue due to "[ 2.352541] [drm] Not enough PCI address space for a large BAR." as mentioned above during my exercise with DGPU card.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
. […]
Subrata - would you be able to try out this complete patch series on your setup and provide logs for the failure?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
Subrata - would you be able to try out this complete patch series on your setup and provide logs for […]
@Furquan, I don't think I have my DGPU setup in lab connected remotely and as you know we are not going to office regularly. I will share an update once i could validate this patch series.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... PS3, Line 79: #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB) TGL supports 512GiB of address space (39 bits). So size should really be 512GiB - TOUUD.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
@Furquan, I don't think I have my DGPU setup in lab connected remotely and as you know we are not go […]
Subrata, I'm confused by the not enough room comment. If we always advertised valid address space limits it should not be limited.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... PS3, Line 79: #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
TGL supports 512GiB of address space (39 bits). So size should really be 512GiB - TOUUD.
I was actually wondering about the same thing. How are all these sizes calculated and why are they static? Currently, these are used to set the total size of high MMIO: gnvs->a4gs = ABOVE_4GB_MEM_BASE_SIZE;
which seems wrong.. Shouldn't this be: gnvs->a4gs = cpu_phys_address_size() - gnvs->a4gb;?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 4: Code-Review+2
We can land this, but I think we should get rid of the conditional entirely.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... File src/soc/intel/tigerlake/include/soc/iomap.h:
https://review.coreboot.org/c/coreboot/+/41155/3/src/soc/intel/tigerlake/inc... PS3, Line 79: #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
I was actually wondering about the same thing. […]
Done: https://review.coreboot.org/c/coreboot/+/41347
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41155/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41155/2//COMMIT_MSG@9 PS2, Line 9: TOUDD
TOUUD.
Done
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
Subrata, I'm confused by the not enough room comment. […]
I looked at the "Not enough PCI address space for a large BAR" message in kernel and it seems to come from pci_resize_resource() with return value -ENOSPC. This is basically when pci_reassign_bridge_resources() fails to assign resources to the device. So, it does look like providing the right MMIO window above 4G should allow the kernel to allocate resources and should not lead to the not enough room error. Let's move forward with this change for now and once you are able to test this, we can revisit this to see if there are any problems.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
I looked at the "Not enough PCI address space for a large BAR" message in kernel and it seems to com […]
@Furquan, i have verified on TGL with DGPU that with enable_above_4GB_mmio=1 i don't see such issue that mean you can actually remove those conditions and always publish those resources unconditionally.
regarding those static base and size, i was looking at RC code and making this ASL changes, i believe it could be improved as you and Aaron said above
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
@Furquan, i have verified on TGL with DGPU that with enable_above_4GB_mmio=1 i don't see such issue […]
I don't entirely understand the impact here, so I'm going by rough consensus: Furquan thinks this can go in and be touched up in a follow up commit. Subrata, do you agree?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/2/src/soc/intel/common/block/... PS2, Line 118: if (!get_enable_above_4GB_mmio())
I don't entirely understand the impact here, so I'm going by rough consensus: Furquan thinks this ca […]
Sure Patrick
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G
This change sets the base for MMIO above 4G to TOUDD. It matches what is used by resource allocator if MMIO resources are allocated above 4G and also matches the expectation in northbridge.asl. This change also gets rid of the macro ABOVE_4GB_MEM_BASE_ADDRESS since it is now unused.
BUG=b:149186922 TEST=Verified that kernel does not complain about MMIO windows above 4G.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: Ibbbfbdad867735a43cf57c256bf206a3f040f383 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41155 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/apollolake/include/soc/iomap.h M src/soc/intel/cannonlake/include/soc/iomap.h M src/soc/intel/common/block/systemagent/systemagent.c M src/soc/intel/icelake/include/soc/iomap.h M src/soc/intel/jasperlake/include/soc/iomap.h M src/soc/intel/skylake/include/soc/iomap.h M src/soc/intel/tigerlake/include/soc/iomap.h 7 files changed, 31 insertions(+), 34 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/soc/intel/apollolake/include/soc/iomap.h b/src/soc/intel/apollolake/include/soc/iomap.h index 49cdaca..e79331a 100644 --- a/src/soc/intel/apollolake/include/soc/iomap.h +++ b/src/soc/intel/apollolake/include/soc/iomap.h @@ -45,7 +45,6 @@ #define EARLY_I2C_BASE_ADDRESS 0xfe020000 #define EARLY_I2C_BASE(x) (EARLY_I2C_BASE_ADDRESS + (0x1000 * (x)))
-#define ABOVE_4GB_MEM_BASE_ADDRESS (128ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (64ULL * GiB)
#endif /* _SOC_APOLLOLAKE_IOMAP_H_ */ diff --git a/src/soc/intel/cannonlake/include/soc/iomap.h b/src/soc/intel/cannonlake/include/soc/iomap.h index ec60d0b..9d13d84 100644 --- a/src/soc/intel/cannonlake/include/soc/iomap.h +++ b/src/soc/intel/cannonlake/include/soc/iomap.h @@ -54,7 +54,6 @@
#define HECI1_BASE_ADDRESS 0xfeda2000
-#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
/* PTT registers */ diff --git a/src/soc/intel/common/block/systemagent/systemagent.c b/src/soc/intel/common/block/systemagent/systemagent.c index e7230bc..3da837c 100644 --- a/src/soc/intel/common/block/systemagent/systemagent.c +++ b/src/soc/intel/common/block/systemagent/systemagent.c @@ -41,34 +41,6 @@ }
/* - * This function will get above 4GB mmio enable config specific to soc. - * - * Return values: - * 0 = Above 4GB memory is not enable - * 1 = Above 4GB memory is enable - */ -static int get_enable_above_4GB_mmio(void) -{ - const struct soc_intel_common_config *common_config; - common_config = chip_get_common_soc_structure(); - - return common_config->enable_above_4GB_mmio; -} - -/* Fill MMIO resource above 4GB into GNVS */ -void sa_fill_gnvs(global_nvs_t *gnvs) -{ - if (get_enable_above_4GB_mmio()) { - gnvs->e4gm = 1; - gnvs->a4gb = ABOVE_4GB_MEM_BASE_ADDRESS; - gnvs->a4gs = ABOVE_4GB_MEM_BASE_SIZE; - printk(BIOS_DEBUG, - "PCI space above 4GB MMIO is from 0x%llx to len = 0x%llx\n", - gnvs->a4gb, gnvs->a4gs); - } -} - -/* * Add all known fixed MMIO ranges that hang off the host bridge/memory * controller device. */ @@ -124,6 +96,37 @@ *result = value; }
+/* + * This function will get above 4GB mmio enable config specific to soc. + * + * Return values: + * 0 = Above 4GB memory is not enable + * 1 = Above 4GB memory is enable + */ +static int get_enable_above_4GB_mmio(void) +{ + const struct soc_intel_common_config *common_config; + common_config = chip_get_common_soc_structure(); + + return common_config->enable_above_4GB_mmio; +} + +/* Fill MMIO resource above 4GB into GNVS */ +void sa_fill_gnvs(global_nvs_t *gnvs) +{ + if (!get_enable_above_4GB_mmio()) + return; + + struct device *sa_dev = pcidev_path_on_root(SA_DEVFN_ROOT); + + gnvs->e4gm = 1; + sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb); + gnvs->a4gs = ABOVE_4GB_MEM_BASE_SIZE; + printk(BIOS_DEBUG, "PCI space above 4GB MMIO is from 0x%llx to len = 0x%llx\n", + gnvs->a4gb, gnvs->a4gs); +} + + static void sa_get_mem_map(struct device *dev, uint64_t *values) { int i; diff --git a/src/soc/intel/icelake/include/soc/iomap.h b/src/soc/intel/icelake/include/soc/iomap.h index 06f68f0..6971a3d 100644 --- a/src/soc/intel/icelake/include/soc/iomap.h +++ b/src/soc/intel/icelake/include/soc/iomap.h @@ -48,7 +48,6 @@ #define VTD_BASE_ADDRESS 0xFED90000 #define VTD_BASE_SIZE 0x00004000
-#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
/* diff --git a/src/soc/intel/jasperlake/include/soc/iomap.h b/src/soc/intel/jasperlake/include/soc/iomap.h index f2300a2e..3ee06a2 100644 --- a/src/soc/intel/jasperlake/include/soc/iomap.h +++ b/src/soc/intel/jasperlake/include/soc/iomap.h @@ -70,7 +70,6 @@ #define VTD_BASE_ADDRESS 0xfed90000 #define VTD_BASE_SIZE 0x00004000
-#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
#define MCH_BASE_ADDRESS 0xfea80000 diff --git a/src/soc/intel/skylake/include/soc/iomap.h b/src/soc/intel/skylake/include/soc/iomap.h index afded55..a3db5c0 100644 --- a/src/soc/intel/skylake/include/soc/iomap.h +++ b/src/soc/intel/skylake/include/soc/iomap.h @@ -61,7 +61,6 @@ #define PTT_TXT_BASE_ADDRESS 0xfed30800 #define PTT_PRESENT 0x00070000
-#define ABOVE_4GB_MEM_BASE_ADDRESS (128ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (64ULL * GiB)
/* diff --git a/src/soc/intel/tigerlake/include/soc/iomap.h b/src/soc/intel/tigerlake/include/soc/iomap.h index 13583d5..70f908d 100644 --- a/src/soc/intel/tigerlake/include/soc/iomap.h +++ b/src/soc/intel/tigerlake/include/soc/iomap.h @@ -76,7 +76,6 @@ #define VTD_BASE_ADDRESS 0xfed90000 #define VTD_BASE_SIZE 0x00004000
-#define ABOVE_4GB_MEM_BASE_ADDRESS (256ULL * GiB) #define ABOVE_4GB_MEM_BASE_SIZE (256ULL * GiB)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 2/2/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : FAIL : https://lava.9esec.io/r/3407 "QEMU x86 q35/ich9" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/3406 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3405 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3404
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/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb); What if <= 4GiB memory are installed?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
What if <= 4GiB memory are installed?
As per EDS, minimum value for TOUUD is 4GB. That should take care of setting the high MMIO region correctly.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
As per EDS, minimum value for TOUUD is 4GB. […]
Ah, thanks. That rule is new to me. Looks like it was introduced with Sandy Bridge. I've peeked into certain refcode, it doesn't seem to know that rule either. Will take some time until I have a device with only 4GiB ready to test.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41155/6//COMMIT_MSG@9 PS6, Line 9: TOUDD TOUUD
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
Ah, thanks. That rule is new to me. Looks like it was introduced with Sandy […]
I think this rule only applies when remapping is enabled. If it is not, it should be set to TOM minus ME size, which might be below 4 GiB if TOM is 4 GiB...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
I've peeked into certain refcode, it doesn't seem to know that rule either.
We can definitely add a check to ensure that MMIO space starts from a max of TOUUD and 4GiB to ensure we don't accidentally set High MMIO below 4G boundary.
I think this rule only applies when remapping is enabled. If it is not, it should be set to TOM minus ME size, which might be below 4 GiB if TOM is 4 GiB...
TOUUD gets set to TOM - ME size if remapping is disabled, But the line at the end of the paragraph says restriction is that it should be 4GiB minimum.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
I've peeked into certain refcode, it doesn't seem to know that rule either. […]
What it says:
"BIOS Restriction: Minimum value for TOUUD is 4GB."
At first, I read it as, don't write anything below 4GiB here. But on second thought it could also mean, if the BIOS reads this, it should assume a 4GiB minimum.
I've also realized by now, that one would actually need less than 4GiB to run into problems here. Which is probably very rare with the supported chipsets? With exactly 4GiB, we would remap part of the memory and TOUUD would be the top of the remapping above 4G. Only if the memory is that small that there is nothing to remap, this could be an issue.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
I've also realized by now, that one would actually need less than 4GiB
to run into problems here. Which is probably very rare with the supported chipsets?
That is correct.
Only if the memory is that small that there is nothing to remap, this could be an issue.
We can definitely add a check to ensure that TOUUD is not < 4G. If it is, then assume high MMIO base as 4GiB.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41155 )
Change subject: soc/intel/common/block/systemagent: Use TOUUD as base for MMIO above 4G ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/41155/6/src/soc/intel/common/block/... PS6, Line 123: sa_read_map_entry(sa_dev, &sa_memory_map[SA_TOUUD_REG], &gnvs->a4gb);
I've also realized by now, that one would actually need less than 4GiB […]
Right, if TOUUD is not above 4 GiB, there should be no remapped memory.
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)