Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
device/pci_device: Map big PCI bars above 4 GiB
Map BARs bigger than 4GiB to memory space above 4GiB. FIXME: This should probably go into the resource allocator.
Required for Nvidia NV100, which has a 32GiB BAR for accessing the VRAM.
Change-Id: Ief34e5bc1c2133ba7380acc9468d4e1042e6e100 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/device/pci_device.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/46617/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 3623c3b..36af0d9 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -178,6 +178,9 @@ /* 64bit limit. */ resource->limit = 0xffffffffffffffffULL; resource->flags |= IORESOURCE_PCI64; + + if (resource->size >= 0x100000000ULL) + resource->flags |= IORESOURCE_ABOVE_4G; } else { /* Invalid value. */ printk(BIOS_ERR, "Broken BAR with value %lx\n", attr);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c@182 PS1, Line 182: 0x100000000ULL Use the commonlib helper: 4 * GiB?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c@182 PS1, Line 182: 0x100000000ULL Do we have a function call to know how much unallocated space we have below 4G? I guess bars < 4G that don't fit also need this.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG@10 PS1, Line 10: FIXME: This should probably go into the resource allocator. I don't think the resource allocator is the right place to implement this policy. I agree in this case it is straightforward since the device needs > 4GiB of MMIO space. There is no way it can be done below the 4G boundary. However, there can be cases where a device needs say 2GiB of MMIO space. It might be possible to allocate that below 4G boundary or not depending upon what other devices are present on the board and also which device/resource needs to be accessible within coreboot and which device/resource is used only by the OS.
It might be better to have a PCI driver specifically for the device which sets the flag to request allocation above 4G boundary. This allows the mainboard to decide: 1. What devices are used in coreboot? 2. Does the mainboard have combination of devices which is going to require large MMIO allocation and which resource to allocate above the 4G boundary?
In this case(> 4GiB request), I think it's okay to let the common pci_device driver set the flag indicating that the resource request needs to be satisfied by allocation above 4G boundary since it knows that the request is for >= 4GiB of space.
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/46617/1/src/device/pci_device.c@182 PS1, Line 182: 0x100000000ULL
Do we have a function call to know how much unallocated space we have below 4G? I guess bars < 4G th […]
It is possible to get a complete picture of the available space only after read_resources is done. Also, it depends upon how the resource allocator walks and satisfies each resource request. There might also be cases where certain resources are okay to be allocated above 4G boundary but not others (e.g. resources being accessed/used in coreboot).
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG@10 PS1, Line 10: FIXME: This should probably go into the resource allocator.
I don't think the resource allocator is the right place to implement this policy. […]
Tests showed that this change indeed places the big BAR above 4GiB and the resource allocator no more complains about insufficent space, but booting using tianocore as payload fails.
The prefmem PCI bridge window covers both, prefmem BARs below 4GiB and prefmem BARs above 4 GiB, which is wrong. So this definitely needs support in the resource allocator that must place all prefmem BARs behind a bridge in one continuous MMIO region. In case there are 32bit BARs or BARs which must have an address below 4GiB, the allocator must bail out.
In this case it's an external GPU and mainboard code cannot make any assuptions about the devices plugged in.
Instead of having an error prone lookup table of devices that needs a BAR below 4GiB it's time to move to x86_64 in coreboot and deprecate 32bit OS as well.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1: Code-Review-1
Has negative side effects which needs to be fixed first.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46617 )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46617/1//COMMIT_MSG@10 PS1, Line 10: FIXME: This should probably go into the resource allocator.
The prefmem PCI bridge window covers both, prefmem BARs below 4GiB and prefmem BARs above 4 GiB, which is wrong.
So the bridge resource contains a single window but the child device resource allocation happens outside that window? Would you be able to share the logs with this change to show how the resource allocation happens?
So this definitely needs support in the resource allocator that must place all prefmem BARs behind a bridge in one continuous MMIO region.
Yes, if that is not happening, it is a bug. If it is not in a continuous MMIO region, then it means we are allocating outside the window on the bridge. Based on what you said, I am guessing that the downstream device has resources which are varying in sizes -- some which are greater than 4GiB and others which are smaller. That is the reason why the resource allocator ends up allocating non-contiguous MMIO space. This is the reason why I had mentioned that it would be best to have a specific PCI driver for the device which knows that it needs all resources above or below 4GiB boundary and can make appropriate request.
In this case it's an external GPU and mainboard code cannot make any assuptions about the devices plugged in.
But, the PCI driver for the external GPU device can set the request for all its resources to ensure that they get allocated above 4GiB boundary. I agree that we should have checks in resource allocator to ensure that we never do a split allocation. That is just wrong irrespective of what the request is.
Instead of having an error prone lookup table of devices that needs a BAR below 4GiB it's time to move to x86_64 in coreboot and deprecate 32bit OS as well.
I don't disagree, but requires larger discussion and scoping :).
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/46617?usp=email )
Change subject: device/pci_device: Map big PCI bars above 4 GiB ......................................................................
Abandoned
-
Arthur Heymans (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)