Received-SPF: pass (google.com: domain of linux-kernel+bounces-208095-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 10 Jun 2024 12:42:27 +0100
From: Daire McNamara <daire.mcnamara@microchip.com>
To: Bjorn Helgaas <helgaas@kernel.org>
CC: <linux-pci@vger.kernel.org>, Conor Dooley <conor.dooley@microchip.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>, Krzysztof
 =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>, Rob Herring <robh@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>, <linux-kernel@vger.kernel.org>,
	<linux-riscv@lists.infradead.org>
Subject: Re: [PATCH 1/2] PCI: microchip: Fix outbound address translation
 tables
Message-ID: <Zmbl9ZYyJCI9dXyE@daire-X570>
References: <20240531085333.2501399-2-daire.mcnamara@microchip.com>
 <20240603184516.GA687362@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240603184516.GA687362@bhelgaas>

On Mon, Jun 03, 2024 at 01:45:16PM -0500, Bjorn Helgaas wrote:
> On Fri, May 31, 2024 at 09:53:32AM +0100, Daire McNamara wrote:
> > On Microchip PolarFire SoC (MPFS) the PCIe Root Port can be behind one of
> > three general-purpose Fabric Interface Controller (FIC) buses that
> > encapsulate an AXI-M interface. That FIC is responsible for managing
> > the translations of the upper 32-bits of the AXI-M address. On MPFS,
> > the Root Port driver needs to take account of that outbound address
> > translation done by the parent FIC bus before setting up its own
> > outbound address translation tables.  In all cases on MPFS,
> > the remaining outbound address translation tables are 32-bit only.
> > 
> > Limit the outbound address translation tables to 32-bit only.
> > 
> > Fixes: 6f15a9c9f941 ("PCI: microchip: Add Microchip Polarfire PCIe controller driver")
> > 
> > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> > ---
> >  drivers/pci/controller/pcie-microchip-host.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-microchip-host.c b/drivers/pci/controller/pcie-microchip-host.c
> > index 137fb8570ba2..0795cd122a4a 100644
> > --- a/drivers/pci/controller/pcie-microchip-host.c
> > +++ b/drivers/pci/controller/pcie-microchip-host.c
> > @@ -983,7 +983,8 @@ static int mc_pcie_setup_windows(struct platform_device *pdev,
> >  		if (resource_type(entry->res) == IORESOURCE_MEM) {
> >  			pci_addr = entry->res->start - entry->offset;
> >  			mc_pcie_setup_window(bridge_base_addr, index,
> > -					     entry->res->start, pci_addr,
> > +					     entry->res->start & 0xffffffff,
> > +					     pci_addr & 0xffffffff,
> >  					     resource_size(entry->res));
> 
> Is this masking something that the PCI core needs to be aware of when
> it allocates address space for BARs?
I don't believe so.
> 
> The PCI core knows about the CPU physical address range of each bridge
> window and the corresponding PCI address range.  From this patch, it
> looks like only the low 32 bits of the CPU address are used by the
> Root Port.  That might not be a problem as long as the windows
> described by DT are correct and none of them overlap after masking out
> the upper 32 bits.  But for example, if DT has windows like this:
> 
>   [mem 0x1'0000'0000-0x1'8000'0000]
>   [mem 0x2'0000'0000-0x2'8000'0000]
> 
> the PCI core will assume they are valid and non-overlapping, when
> IIUC, they *do* overlap.
True, but I can't see how that could happen on any real system - in my mind,
a PolarFire Soc designer (or indeed any designer on any chip) will know where
its rootport is actually attached in its memory map. On PolarFire SoC, for
example, a designer can only reach a rootport over a FIC, and - if they were
to attach to the rootport over two FICs at the same time, that would be a
blunder and would be picked up during design phase.  I can't imagine any
reason anyone would release a product with that arrangement.

> 
> But also only the low 32 bits of the PCI address are used, and it
> seems like the PCI core will need to know that so it doesn't program a
> 64-bit BAR with a value above 4GB?
Yeah, I'll send around a v2 shortly to address this - I was rather
over-zealous when I prevented that.
> 
> >  			index++;
> >  		}
> > @@ -1117,8 +1118,8 @@ static int mc_platform_init(struct pci_config_window *cfg)
> >  	int ret;
> >  
> >  	/* Configure address translation table 0 for PCIe config space */
> > -	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start,
> > -			     cfg->res.start,
> > +	mc_pcie_setup_window(bridge_base_addr, 0, cfg->res.start & 0xffffffff,
> > +			     cfg->res.start & 0xffffffff,
> >  			     resource_size(&cfg->res));
> >  
> >  	/* Need some fixups in config space */
> > -- 
> > 2.34.1
> > 
>