Re: [PATCH 1/2] PCI: microchip: Fix outbound address translation tables

[Daire McNamara]

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@xxxxxxxxxxxxx>
> > ---
> >  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
> > 
>