Date:   Thu, 16 Feb 2023 17:35:46 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     guo.ziliang@zte.com.cn
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, chen.lin5@zte.com.cn,
        Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>
Subject: Re: =?utf-8?B?562U5aSNOiBbUEFUQ0hdIFBDSTog?= =?utf-8?Q?of=3A_War?=
 =?utf-8?Q?n?= if bridge base/limit region overlaps with system ram region
Message-ID: <20230216233546.GA3354802@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202301091635256312056@zte.com.cn>
Precedence: bulk

[+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, &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