On 2023-02-16 23:35, Bjorn Helgaas wrote:
[+cc Joerg, Will, Robin]
On Mon, Jan 09, 2023 at 04:35:25PM +0800, guo.ziliang@xxxxxxxxxx wrote:
bridge base/limit(memory behind in lspci info, outbound pcie address/size)
region is used to route outbound mem read/write transaction to ep. This
base/limit region also may filter out inbound transactions which will
result in inbound(eg: dma) transaction fail.
For example, if bridge base/limit is [0x20000000, 0x203fffff], system ram
is [0x20000000, 0x27ffffff]. The inbound mapping is usually 1:1 equal
mapping. When allocated system ram for inbound tansaction is 0x20004000
(any in bridge base/limit), this inbound transactions will be filter out.
AER may report 'UnsupReq' on inbound mem read/write transactions if address
is in this base/limit region, but not all pcie AER enabled or work well. We
warn it also in bridge pci address setting phase.
Signed-off-by: Chen Lin <chen.lin5@xxxxxxxxxx>
This would need the 0-day warnings cleaned up, of course.
---
drivers/pci/setup-bus.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index b4096598dbcb..1a9f527d2317 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -608,6 +608,24 @@ static void pci_setup_bridge_io(struct pci_dev *bridge)
pci_write_config_dword(bridge, PCI_IO_BASE_UPPER16, io_upper16);
}
+static void check_bridge_region_overlaps_systemram(struct pci_dev *bridge,
+ struct pci_bus_region *region)
+{
+ int is_ram;
+
+ /*
+ * bridge base/limit(memory behind) region may filter out inbound
+ * transactions which will result in inbound(eg: dma) fail of ep.
+ * AER may report it if enabled, we warn it also.
+ */
+ is_ram = region_intersects(region->start, region->end - region->start + 1,
+ IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
+ if (is_ram == REGION_INTERSECTS) {
+ pci_warn(bridge, "%#012llx..%#012llx bridge base/limit region overlaps with system ram, may result in inbound fail\n",
+ region->start, region->end);
This compares PCI bus addresses (from struct pci_bus_region) with CPU
physical addresses (the struct resources used by region_intersects()).
But I don't think you can do that directly because an IOMMU might map
those PCI bus addresses to something different before a DMA gets to
system RAM.
I see that you say "The inbound mapping is usually 1:1 equal mapping"
above, so maybe I'm missing something. Maybe the IOMMU folks will
clue me in.
IOMMUs typically wouldn't be reflected here - in fact they would
typically hide this issue anyway, since inbound DMA would then be to
virtual addresses anywhere in PCI Mem space (and we try our best to
carve out the regions used for outbound resources). However there
certainly exist systems where the PCI host bridge itself makes a static
non-identity translation between PCI Mem space and system PA space, with
potentially different mappings for inbound vs. outbound as well. So
indeed, this code looks wrong - at the very least any consideration of
region->offset is missing (assuming that's initialised correctly in this
context), but that still won't account for inbound translation.
In fact this is really the same thing as in the recent discussions of
the MSI thing in the DWC driver - it's all a matter of whether a bus
address may overlap a valid DMA address or not. A mechanism for making
that check properly would go hand-in-hand with the mechanism we need for
allocating such bus addresses for inline MSI widgets.
Thanks,
Robin.
+ }
+}
+
static void pci_setup_bridge_mmio(struct pci_dev *bridge)
{
struct resource *res;
@@ -621,6 +639,7 @@ static void pci_setup_bridge_mmio(struct pci_dev *bridge)
l = (region.start >> 16) & 0xfff0;
l |= region.end & 0xfff00000;
pci_info(bridge, " bridge window %pR\n", res);
+ check_bridge_region_overlaps_systemram(bridge, ®ion);
} else {
l = 0x0000fff0;
}
@@ -652,6 +671,7 @@ static void pci_setup_bridge_mmio_pref(struct pci_dev *bridge)
lu = upper_32_bits(region.end);
}
pci_info(bridge, " bridge window %pR\n", res);
+ check_bridge_region_overlaps_systemram(bridge, ®ion);
} else {
l = 0x0000fff0;
}
--
2.15.2
