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
