Re: [PATCH 03/11] PCI: of_property: Sanitize 32 bit PCI address parsed from DT

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

On Fri, Oct 18, 2024 at 02:41:11PM +0200, Andrea della Porta wrote:
> On 20:08 Mon 07 Oct     , Bjorn Helgaas wrote:
> ... 

> > Yes, this is exactly the problem.  The pci@0 parent and child
> > addresses in "ranges" are both in the PCI address space.  But we
> > start with pdev->resource[N], which is a CPU address.  To get the PCI
> > address, we need to apply pci_bus_address().  If the host bridge
> > windows are set up correctly, the window->offset used in
> > pcibios_resource_to_bus() should yield the PCI bus address.
> 
> You mean something like this, I think:
> 
> @@ -129,7 +129,7 @@ static int of_pci_prop_ranges(struct pci_dev *pdev, struct of_changeset *ocs,
>                 if (of_pci_get_addr_flags(&res[j], &flags))
>                         continue;
>  
> -               val64 = res[j].start;
> +               val64 = pci_bus_address(pdev, &res[j] - pdev->resource);
>                 of_pci_set_address(pdev, rp[i].parent_addr, val64, 0, flags,
>                                    false);
>                 if (pci_is_bridge(pdev)) {

Yes.

> > I think it should look like this:
> > 
> >   pci@0: <0x82000000 0x0 0x00000000 0x82000000 0x0 0x00000000 0x0 0x600000>;
> 
> indeed, with the above patch applied, the result is exactly as you expected.
> ...

> > > > But I don't think it works in general because there's no
> > > > requirement that the host bridge address translation be that
> > > > simple.  For example, if we have two host bridges, and we want
> > > > each to have 2GB of 32-bit PCI address space starting at 0x0,
> > > > it might look like this:
> > > > 
> > > >   0x00000002_00000000 -> PCI 0x00000000 (subtract 0x00000002_00000000)
> > > >   0x00000002_80000000 -> PCI 0x00000000 (subtract 0x00000002_80000000)
> > > > 
> > > > In this case simply ignoring the high 32 bits of the CPU
> > > > address isn't the correct translation for the second host
> > > > bridge.  I think we should look at each host bridge's
> > > > "ranges", find the difference between its parent and child
> > > > addresses, and apply the same difference to everything below
> > > > that bridge.
> > > 
> > > Not sure I've got this scenario straight: can you please provide
> > > the topology and the bit setting (32/64 bit) for those ranges?
> > > Also, is this scenario coming from a real use case or is it
> > > hypothetical?
> > 
> > This scenario is purely hypothetical, but it's a legal topology
> > that we should handle correctly.  It's two host bridges, with
> > independent PCI hierarchies below them:
> > 
> >   Host bridge A: [mem 0x2_00000000-0x2_7fffffff window] (bus address 0x00000000-0x7fffffff)
> >   Host bridge B: [mem 0x2_80000000-0x2_ffffffff window] (bus address 0x00000000-0x7fffffff)
> > 
> > Bridge A has an MMIO aperture at CPU addresses
> > 0x2_00000000-0x2_7fffffff, and when it initiates PCI transactions on
> > its secondary side, the PCI address is CPU_addr - 0x2_00000000.
> > 
> > Similarly, bridge B has an MMIO aperture at CPU addresses 
> > 0x2_80000000-0x2_ffffffff, and when it initiates PCI transactions on 
> > its secondary side, the PCI address is CPU_addr - 0x2_80000000.
> > 
> > Both hierarchies use PCI bus addresses in the 0x00000000-0x7fffffff
> > range.  In a topology like this, you can't convert a bus address back
> > to a CPU address unless you know which hierarchy it's in.
> > pcibios_bus_to_resource() takes a pci_bus pointer, which tells you
> > which hierarchy (and which host bridge address translation) to use.
> 
> Agreed. While I think about how to adjust that specific patch,i
> let's drop it from this patchset since the aforementioned change is
> properly fixing the translation issue.

OK.  I assume you mean to drop the "PCI: of_property: Sanitize 32 bit
PCI address parsed from DT" patch?  Or replace it with the
pci_bus_address() addition above?

Bjorn