[+cc Lizhi]

On Thu, Sep 05, 2024 at 06:43:35PM +0200, Andrea della Porta wrote:

On 17:26 Tue 03 Sep     , Bjorn Helgaas wrote:

quoted

On Mon, Aug 26, 2024 at 09:51:02PM +0200, Andrea della Porta wrote:

quoted

On 10:24 Wed 21 Aug     , Bjorn Helgaas wrote:

quoted

On Tue, Aug 20, 2024 at 04:36:05PM +0200, Andrea della Porta wrote:

quoted

The of_pci_set_address() function parses devicetree PCI range
specifier assuming the address is 'sanitized' at the origin,
i.e. without checking whether the incoming address is 32 or 64
bit has specified in the flags.  In this way an address with no
OF_PCI_ADDR_SPACE_MEM64 set in the flags could leak through and
the upper 32 bits of the address will be set too, and this
violates the PCI specs stating that in 32 bit address the upper
bit should be zero.

quoted

quoted

I don't understand this code, so I'm probably missing something.  It
looks like the interesting path here is:

  of_pci_prop_ranges
    res = &pdev->resource[...];
    for (j = 0; j < num; j++) {
      val64 = res[j].start;
      of_pci_set_address(..., val64, 0, flags, false);
 +      if (OF_PCI_ADDR_SPACE_MEM64)
 +        prop[1] = upper_32_bits(val64);
 +      else
 +        prop[1] = 0;

...

quoted

However, the CPU physical address space and the PCI bus address are
not the same.  Generic code paths should account for that different by
applying an offset (the offset will be zero on many platforms where
CPU and PCI bus addresses *look* the same).

So a generic code path like of_pci_prop_ranges() that basically copies
a CPU physical address to a PCI bus address looks broken to me.

Hmmm, I'd say that a translation from one bus type to the other is
going on nonetheless, and this is done in the current upstream function
as well. This patch of course does not add the translation (which is
already in place), just to do it avoiding generating inconsistent address.

I think I was looking at this backwards.  I assumed we were *parsing"
a "ranges" property, but I think in fact we're *building* a "ranges"
property to describe an existing PCI device (either a PCI-to-PCI
bridge or an endpoint).  For such devices there is no address
translation.

Any address translation would only occur at a PCI host bridge that has
CPU address space on the upstream side and PCI address space on the
downstream side.

Since (IIUC), we're building "ranges" for a device in the interior of
a PCI hierarchy where address translation doesn't happen, I think both
the parent and child addresses in "ranges" should be in the PCI
address space.

But right now, I think they're both in the CPU address space, and we
basically do this:

  of_pci_prop_ranges(struct pci_dev *pdev, ...)
    res = &pdev->resource[...];
    for (j = 0; j < num; j++) {   # iterate through BARs or windows
      val64 = res[j].start;       # CPU physical address
      # <convert to PCI address space>
      of_pci_set_address(..., rp[i].parent_addr, val64, ...)
        rp[i].parent_addr = val64
      if (pci_is_bridge(pdev))
        memcpy(rp[i].child_addr, rp[i].parent_addr)
      else
        rp[i].child_addr[0] = j   # child addr unset/unused

Here "res" is a PCI BAR or bridge window, and it contains CPU physical
addresses, so "val64" is a CPU physical address.  It looks to me like
we should convert to a PCI bus address at the point noted above, based
on any translation described by the PCI host bridge.  That *should*
naturally result in a 32-bit value if OF_PCI_ADDR_SPACE_MEM64 is not
set.

quoted

Maybe my expectation of this being described in DT is mistaken.

Not sure what you mean here, the address being translated are coming from
DT, in fact they are described by "ranges" properties.

Right, for my own future reference since I couldn't find a generic
description of "ranges" in Documentation/devicetree/:

[1] https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#ranges