Hi Bjorn,

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

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;

OF_PCI_ADDR_SPACE_MEM64 tells us about the size of the PCI bus
address, but the address (val64) is a CPU physical address, not a PCI
bus address, so I don't understand why of_pci_set_address() should use
OF_PCI_ADDR_SPACE_MEM64 to clear part of the CPU address.

It all starts from of_pci_prop_ranges(), that is the caller of
of_pci_set_address().

quoted

val64 (i.e. res[j].start) is the address part of a struct resource
that has its own flags.  Those flags are directly translated to
of_pci_range flags by of_pci_get_addr_flags(), so any
IORESOURCE_MEM_64 / IORESOURCE_MEM in the resource flag will
respectively become OF_PCI_ADDR_SPACE_MEM64 /
OF_PCI_ADDR_SPACE_MEM32 in pci range.

quoted

What is advertised as 32 bit at the origin (val64) should not become
a 64 bit PCI address at the output of of_pci_set_address(), so the
upper 32 bit portion should be dropped. 

quoted

This is explicitly stated in [1] (see page 5), where a space code of 0b10
implies that the upper 32 bit of the address must be zeroed out.

OK, I was confused and thought IORESOURCE_MEM_64 was telling us
something about the *CPU* address, but it's actually telling us
something about what *PCI bus* addresses are possible, i.e., whether
it's a 32-bit BAR or a 64-bit BAR.

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.

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.

Many thanks,
Andrea

Bjorn