* [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region @ 2023-01-06 8:47 guo.ziliang 2023-01-06 12:18 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: guo.ziliang @ 2023-01-06 8:47 UTC (permalink / raw) To: bhelgaas; +Cc: linux-pci, linux-kernel, chen.lin5, guo.ziliang From: Chen Lin <chen.lin5@zte.com.cn> 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 host bridge pci address detection phase. Signed-off-by: Chen Lin <chen.lin5@zte.com.cn> --- drivers/pci/of.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 196834ed44fe..82e09af6c638 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -314,6 +314,8 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, dev_dbg(dev, "Parsing ranges property...\n"); for_each_of_pci_range(&parser, &range) { + int is_ram; + /* Read next ranges element */ if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) range_type = "IO"; @@ -332,6 +334,18 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) continue; + /* + * 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(range.pci_addr, range.size, + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE); + if (is_ram == REGION_INTERSECTS) { + dev_warn(dev, "%#012llx..%#012llx bridge base/limit region overlaps with system ram, may result in inbound fail\n", + range.pci_addr, range.pci_addr + range.size - 1); + } + err = of_pci_range_to_resource(&range, dev_node, &tmp_res); if (err) continue; -- 2.15.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region 2023-01-06 8:47 [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region guo.ziliang @ 2023-01-06 12:18 ` Bjorn Helgaas 2023-01-09 8:35 ` 答复: " guo.ziliang 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2023-01-06 12:18 UTC (permalink / raw) To: guo.ziliang; +Cc: bhelgaas, linux-pci, linux-kernel, chen.lin5 On Fri, Jan 06, 2023 at 04:47:33PM +0800, guo.ziliang@zte.com.cn wrote: > From: Chen Lin <chen.lin5@zte.com.cn> > 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 host bridge pci address detection phase. Is this a DT-specific thing? It sounds like it should apply to PCI bridges in general. > Signed-off-by: Chen Lin <chen.lin5@zte.com.cn> > --- > drivers/pci/of.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index 196834ed44fe..82e09af6c638 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -314,6 +314,8 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > > dev_dbg(dev, "Parsing ranges property...\n"); > for_each_of_pci_range(&parser, &range) { > + int is_ram; > + > /* Read next ranges element */ > if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > range_type = "IO"; > @@ -332,6 +334,18 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev, > if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) > continue; > > + /* > + * 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(range.pci_addr, range.size, > + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE); > + if (is_ram == REGION_INTERSECTS) { > + dev_warn(dev, "%#012llx..%#012llx bridge base/limit region overlaps with system ram, may result in inbound fail\n", > + range.pci_addr, range.pci_addr + range.size - 1); > + } > + > err = of_pci_range_to_resource(&range, dev_node, &tmp_res); > if (err) > continue; > -- > 2.15.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* 答复: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region 2023-01-06 12:18 ` Bjorn Helgaas @ 2023-01-09 8:35 ` guo.ziliang 2023-02-16 23:35 ` Bjorn Helgaas 0 siblings, 1 reply; 5+ messages in thread From: guo.ziliang @ 2023-01-09 8:35 UTC (permalink / raw) To: helgaas; +Cc: bhelgaas, linux-pci, linux-kernel, chen.lin5 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(+) 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; } -- 2.15.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: 答复: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region 2023-01-09 8:35 ` 答复: " guo.ziliang @ 2023-02-16 23:35 ` Bjorn Helgaas 2023-02-20 12:25 ` Robin Murphy 0 siblings, 1 reply; 5+ messages in thread From: Bjorn Helgaas @ 2023-02-16 23:35 UTC (permalink / raw) To: guo.ziliang Cc: bhelgaas, linux-pci, linux-kernel, chen.lin5, Joerg Roedel, Will Deacon, Robin Murphy [+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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 答复: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region 2023-02-16 23:35 ` Bjorn Helgaas @ 2023-02-20 12:25 ` Robin Murphy 0 siblings, 0 replies; 5+ messages in thread From: Robin Murphy @ 2023-02-20 12:25 UTC (permalink / raw) To: Bjorn Helgaas, guo.ziliang Cc: bhelgaas, linux-pci, linux-kernel, chen.lin5, Joerg Roedel, Will Deacon 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-20 12:27 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-06 8:47 [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region guo.ziliang 2023-01-06 12:18 ` Bjorn Helgaas 2023-01-09 8:35 ` 答复: " guo.ziliang 2023-02-16 23:35 ` Bjorn Helgaas 2023-02-20 12:25 ` Robin Murphy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).