Re: 答复: [PATCH] PCI: of: Warn if bridge base/limit region overlaps with system ram region

[Robin Murphy]

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, &region);
> >   	} 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, &region);
> >   	} else {
> >   		l = 0x0000fff0;
> >   	}
> > --
> > 2.15.2