Hi Bjorn,
On 17:28 Fri 18 Oct , Bjorn Helgaas wrote:
> 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?
I'm planning to replace that patch with the above mentioned pci_bus_address()
addition. However, I think the 32 bit sanitization is still useful to prevent
wrongly encoded address to linger around, but I defer it to a subsequent standalone
patch, after figuring out the dual bridge scenario that you proposed.
>
> Bjorn
Many thanks,
Andrea
