| Message ID | 202301091635256312056@zte.com.cn |
|---|---|
| State | New |
| Headers | show |
| Series | 答复: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region | expand |
[+cc Joerg, Will, Robin] On Mon, Jan 09, 2023 at 04:35:25PM +0800, guo.ziliang@zte.com.cn 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@zte.com.cn> 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. > + } > +} > + > 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
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@zte.com.cn 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@zte.com.cn> > > 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
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); + } +} + 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; }
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@zte.com.cn> --- drivers/pci/setup-bus.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)