> In subject, capitalize "Fix" and drop period at end.
>
Okay.
> On Fri, Jun 20, 2025 at 09:32:35AM +0000, Wannes Bouwen (Nokia) wrote:
> >
> > [v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.
>
> Drop this.
>
Ok, will drop.
> > According to the PCIe spec (PCIe r6.3, sec 7.5.1.3.8),
> > non-prefetchable memory supports only 32-bit host bridge windows (both
> > base address as limit address).
>
> 7.5.1.3.8 is about PCI-to-PCI bridge windows, not host bridge windows.
>
How I understand it:
- Section 7.5.1.3 Type 1 Configuration Space Header is applicable for PCIe Switch and Root Ports. Logically a PCIe port acts as a PCI-PCI bridge.
- The term host bridge windows is indeed maybe not fully correct here. It has to do with the limitation on the memory address / memory range in the PCIe Switch / Root ports to 32 bits.
- The host bridge windows are used to configure the Type 1 Configuration Space in the end.
> I'm confused about what's going on here. The word "prefetch" doesn't even
> appear in PCIe r7.0, but historically, issue was that we have to be careful about
> putting a non-prefetchable BAR in a prefetchable window because a read
> (which might be a prefetch) in a non-prefetchable BAR is allowed to have side
> effects.
>
> But if we put a prefetchable BAR in a non-prefetchable window, nothing bad
> happens other than performance might be bad.
>
> Are we trying to warn about a potential performance problem? Or is there
> some functional problem here?
>
The warning here is just about the size of the 32 bit window in the Type 1 Configuration Space Header. It has in principle nothing to do with how we map the BARs on these windows.
> > In the kernel there is a check that prints a warning if a
> > non-prefetchable resource's size exceeds the 32-bit limit.
> >
> > The check currently checks the size of the resource, while actually
> > the check should be done on the PCIe end address of the
> > non-prefetchable window.
> >
> > Move the check to devm_of_pci_get_host_bridge_resources() where the
> > PCIe addresses are available and use the end address instead of the
> > size of the window.
>
> Are you seeing an issue here? Can we include a dmesg snippet that illustrates
> it?
>
We see below warning because we define a 4 GiB non-prefetchable host bridge window.
[ 341.213850] armada8k-pcie f2620000.pcie: host bridge /cp0/pcie@f2620000 ranges:
[ 341.213883] armada8k-pcie f2620000.pcie: MEM 0x0400000000..0x04ffffffff -> 0x0000000000
[ 341.213894] armada8k-pcie f2620000.pcie: MEM 0x0500000000..0x05ffffffff -> 0x0100000000
[ 341.213903] armada8k-pcie f2620000.pcie: Memory resource size exceeds max for 32 bits
> > Fixes: fede8526cc48 (PCI: of: Warn if non-prefetchable memory aperture
> > size is > 32-bit)
> > Signed-off-by: Wannes Bouwen <wannes.bouwen@xxxxxxxxx>
> > ---
> >
> > v4:
> > - Update warning text
> >
> > v3:
> > - Update subject and description + add changelog
> >
> > v2:
> > - Use PCI address range instead of window size to check that window is
> > within a 32bit boundary.
> >
> > ---
> > drivers/pci/of.c | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c index
> > 3579265f1198..16405985a53a 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -400,6 +400,13 @@ static int
> devm_of_pci_get_host_bridge_resources(struct device *dev,
> > *io_base = range.cpu_addr;
> > } else if (resource_type(res) == IORESOURCE_MEM) {
> > res->flags &= ~IORESOURCE_MEM_64;
> > +
> > + if (!(res->flags & IORESOURCE_PREFETCH))
> > + if (upper_32_bits(range.pci_addr + range.size - 1))
> > + dev_warn(dev,
> > + "host bridge non-prefetchable window: pci range
> end address exceeds 32 bit boundary %pR"
> > + " (pci address range [%#012llx-%#012llx])\n",
> > + res, range.pci_addr,
> > + range.pci_addr + range.size - 1);
>
> I gave you bad advice because I hadn't looked earlier in this function.
> devm_of_pci_get_host_bridge_resources() printed this
> earlier:
>
> MEM %#012llx..%#012llx -> %#012llx
>
> where the first part is basically the %pR information in a different format and
> the last part is the bus address, and I think a warning here should look similar,
> e.g.,
>
> dev_warn(dev, "Bus address %#012llx..%#012llx end is past 32-bit
> boundary\n",
>
I will update the warning.
> > }
> >
> > pci_add_resource_offset(resources, res, res->start -
> > range.pci_addr); @@ -622,10 +629,6 @@ static int
> pci_parse_request_of_pci_ranges(struct device *dev,
> > case IORESOURCE_MEM:
> > res_valid |= !(res->flags &
> > IORESOURCE_PREFETCH);
> >
> > - if (!(res->flags & IORESOURCE_PREFETCH))
> > - if (upper_32_bits(resource_size(res)))
> > - dev_warn(dev, "Memory resource size exceeds max for
> 32 bits\n");
> > -
> > break;
> > }
> > }
> > --
> > 2.43.5
