[PATCH v2 0/2] Fix a pair of setup bus bugs.

[PATCH v2 0/2] Fix a pair of setup bus bugs.

[Logan Gunthorpe]

Hey,

This is largely a resend to get some more attention. I've attempted to clean
up and expand on the commit message of the first commit because the
bug is a bit of a nightmare to explain and follow. There's a lot more
information on the first commit in the original thread here[1] including
instructions on how to reproduce it in QEMU.

The second patch fixes an unrelated bug, with similar symptoms, in
the same code. It was a lot easier to debug and the reasoning should
hopefully be easier to follow, but I don't think it was reviewed much
during the first posting due to the nightmare in the first patch.

Thanks,

Logan

[1] https://lore.kernel.org/lkml/de3e34d8-2ac3-e89b-30f1-a18826ce5d7d@deltatee.com/T/#m96ba95de4678146ed46b602e8bfd6ac08a588fa2

Logan Gunthorpe (2):
  PCI: Prevent 64-bit resources from being counted in 32-bit bridge
    region
  PCI: Fix disabling of bridge BARs when assigning bus resources

 drivers/pci/setup-bus.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

--
2.20.1

[PATCH v2 1/2] PCI: Prevent 64-bit resources from being counted in 32-bit bridge region

[Logan Gunthorpe]

In some situations (described below), hierarchies of 32-bit resources can
fail to be assigned when the kernel has to attempt to assign a large
64-bit resource. When this happens, lspci will report
some PCI BAR resources as 'ignored' and some PCI Bridge windows
being left unset. Sample lspci lines may look like:

  Memory behind bridge: fff00000-000fffff

or

  Region 0: Memory at <ignored> (32-bit, non-prefetchable) [size=256K]

lspci reports a BAR as 'ignored' when the kernel does not populate
the struct resource and the corresponding entry in either
/proc/bus/pci/devices or /sys/bus/pci/devices/.../resource are all zero.
Any device driver that depends on one of these BARs are likely to fail
initializing and the device will not be usable. Typically when this
happens, the underlying Base Address Registers in the configuration
space are still set to whatever the firmware set them to, it's only
the kernel's view of this that is wrong.

The possible situations where this can happen will be a bit varied and
depend highly on the exact hierarchy, what the firmware has assigned
and what the kernel must do to properly re-assign resources. In the
setup that first hit this bug, it failed only with the 'pci=realloc'
command line parameter. The bug has also been hackily reproduced with
QEMU[1] without the realloc parameter.

The following things are required to hit this bug:

1) A large 64-bit prefetchable BAR that can't be assigned in any
   pass of pci_assign_unassigned_bridge_resources(). The resource must
   be large enough that it will not be able to fit with-in the 32-bit
   region. This resource may or may not be assignable into the 64-bit
   prefetchable region after additional passes.

2) A victim 32-bit non-prefetchable BAR that is a neighbor of the
   large BAR (so typically it will have to be behind a switch). When
   the bug is hit, this BAR's struct resource will not be assign and
   lspci will report it as ignored.

3) There must exist a 64-bit prefetchable window for the original large
   BAR to fit in. Which generally implies there is no 32-bit
   prefetchable window.

4) The kernel has to have a reason to re-assign the heirarchy that
   contains both BARs.

The cause of this bug is in __pci_bus_size_bridges() which tries to
calculate the total resource space required for each of the bridge windows
(typically IO, 64-bit, and 32-bit / non-prefetchable). The code, as
written, tries to allocate all the 64-bit prefetchable resources
followed by all the remaining resources. It uses three calls to
pbus_size_mem() for this:

  1) If bridge has a 64-bit prefetchable window, find the size of all
     64-bit prefetchable resources below the bridge

  2) If bridge has no 64-bit prefetchable window, find the size
     of all prefetchable resources below the bridge

  3) Find the size of everything else (non-prefetchable resources plus
     any prefetchable ones that couldn't be accommodated above)

By the requirement (3) above, the system has a 64-bit prefetchable
window, so the large 64-bit BAR *should* be assigned to the 64-bit
prefetchable region. However, if the 64-bit bus resource has already
been assigned, then this call to pbus_size_mem() will fail. (See
the find_free_bus_resource() helper). When the first call fails, it falls
to the second call but, by requirement (3) above, there is no 32-bit
prefetchable window so this call also fails. Thus, it falls to the last
call which tries to fit all the resources into the 32-bit
catch-all window. However, because of requirement (1), the large
BAR will overfill this region and cause the victim 32-bit BAR to not
be assignable.

Looking at the first call to pbus_size_mem(): there are only two reasons
for it to fail: if there is no 64-bit/prefetchable bridge window, or if that
window is already assigned. We know the former case can't be true because,
in __pci_bus_size_bridges(), its existence is checked before making the call.
So if the pbus_size_mem() call in question fails, the window must already
be assigned, and in this case, the code should not try to assign
64-bit resources into the 32-bit catch-all window.

Thus, the fix for the bug is to ensure mask, type2 and type3 are set in
cases where a 64-bit resource exists even if pbus_size_mem() fails. Once
we do this, the large BAR resource will never be attempted to be
assigned to the 32-bit catch-all window and the victim BAR will still
be correctly assigned.

[1] https://lore.kernel.org/lkml/de3e34d8-2ac3-e89b-30f1-a18826ce5d7d@deltatee.com/T/#u

Reported-by: Kit Chow <kchow@gigaio.com>
Fixes: 5b28541552ef ("PCI: Restrict 64-bit prefetchable bridge windows to 64-bit resources")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index ec44a0f3a7ac..0eb40924169b 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1228,21 +1228,20 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
 		prefmask = IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		if (b_res[2].flags & IORESOURCE_MEM_64) {
 			prefmask |= IORESOURCE_MEM_64;
-			ret = pbus_size_mem(bus, prefmask, prefmask,
+			pbus_size_mem(bus, prefmask, prefmask,
 				  prefmask, prefmask,
 				  realloc_head ? 0 : additional_mem_size,
 				  additional_mem_size, realloc_head);
 
 			/*
-			 * If successful, all non-prefetchable resources
-			 * and any 32-bit prefetchable resources will go in
-			 * the non-prefetchable window.
+			 * Given the existence of a 64-bit resource for this
+			 * bus, all non-prefetchable resources and any 32-bit
+			 * prefetchable resources will go in the
+			 * non-prefetchable window.
 			 */
-			if (ret == 0) {
-				mask = prefmask;
-				type2 = prefmask & ~IORESOURCE_MEM_64;
-				type3 = prefmask & ~IORESOURCE_PREFETCH;
-			}
+			mask = prefmask;
+			type2 = prefmask & ~IORESOURCE_MEM_64;
+			type3 = prefmask & ~IORESOURCE_PREFETCH;
 		}
 
 		/*
-- 
2.20.1

[PATCH v2 2/2] PCI: Fix disabling of bridge BARs when assigning bus resources

[Logan Gunthorpe]

One odd quirk of PLX switches is that their upstream bridge port has
256K of space allocated behind its BAR0 (most other bridge
implementations do not report any BAR space). The lspci for such  device
looks like:

  04:00.0 PCI bridge: PLX Technology, Inc. PEX 8724 24-Lane, 6-Port PCI
            Express Gen 3 (8 GT/s) Switch, 19 x 19mm FCBGA (rev ca)
	    (prog-if 00 [Normal decode])
      Physical Slot: 1
      Flags: bus master, fast devsel, latency 0, IRQ 30, NUMA node 0
      Memory at 90a00000 (32-bit, non-prefetchable) [size=256K]
      Bus: primary=04, secondary=05, subordinate=0a, sec-latency=0
      I/O behind bridge: 00002000-00003fff
      Memory behind bridge: 90000000-909fffff
      Prefetchable memory behind bridge: 0000380000800000-0000380000bfffff
      Kernel driver in use: pcieport

It's not clear what the purpose of the memory at 0x90a00000 is, and
currently the kernel never actually uses it for anything. In most cases,
it's safely ignored and does not cause a problem.

However, when the kernel assigns the resource addresses (with the
pci=realloc command line parameter, for example) it can inadvertently
disable the struct resource corresponding to the bar. When this happens,
lspci will report this memory as ignored:

   Region 0: Memory at <ignored> (32-bit, non-prefetchable) [size=256K]

This is because the kernel reports a zero start address and zero flags
in the corresponding sysfs resource file and in /proc/bus/pci/devices.
Investigation with 'lspci -x', however shows the bios-assigned address
will still be programmed in the device's BAR registers.

In many cases, this still isn't a problem. Nothing uses the memory,
so nothing is affected. However, a big problem shows up when an IOMMU
is in use: the IOMMU will not reserve this space in the IOVA because the
kernel no longer thinks the range is valid. (See
dmar_init_reserved_ranges() for the Intel implementation of this.)

Without the proper reserved range, we have a situation where a DMA
mapping may occasionally allocate an IOVA which the PCI bus will actually
route to a BAR in the PLX switch. This will result in some random DMA
writes not actually writing to the RAM they are supposed to, or random
DMA reads returning all FFs from the PLX BAR when it's supposed to have
read from RAM.

The problem is caused in pci_assign_unassigned_root_bus_resources().
When any resource from a bridge device fails to get assigned, the code
sets the resource's flags to zero. This makes sense for bridge resources,
as they will be re-enabled later, but for regular BARs, it disables them
permanently. To fix the problem, we only set the flags to zero for
bridge resources and treat any other resources like non-bridge devices.

Reported-by: Kit Chow <kchow@gigaio.com>
Fixes: da7822e5ad71 ("PCI: update bridge resources to get more big ranges when allocating space (again)")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yinghai Lu <yinghai@kernel.org>
---
 drivers/pci/setup-bus.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 0eb40924169b..7adbd4bedd16 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1784,11 +1784,16 @@ void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus)
 	/* restore size and flags */
 	list_for_each_entry(fail_res, &fail_head, list) {
 		struct resource *res = fail_res->res;
+		int idx;
 
 		res->start = fail_res->start;
 		res->end = fail_res->end;
 		res->flags = fail_res->flags;
-		if (fail_res->dev->subordinate)
+
+		idx = res - &fail_res->dev->resource[0];
+		if (fail_res->dev->subordinate &&
+		    idx >= PCI_BRIDGE_RESOURCES &&
+		    idx <= PCI_BRIDGE_RESOURCE_END)
 			res->flags = 0;
 	}
 	free_list(&fail_head);
-- 
2.20.1