[v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

[v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

[Wannes Bouwen (Nokia)]

 
[v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

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).

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.

Fixes: fede8526cc48 (PCI: of: Warn if non-prefetchable memory aperture
size is > 32-bit)
Signed-off-by: Wannes Bouwen <wannes.bouwen@nokia.com>
---

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);
 		}
 
 		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

Re: [v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

[Bjorn Helgaas]

In subject, capitalize "Fix" and drop period at end.

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.

> 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.

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?

> 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?

> Fixes: fede8526cc48 (PCI: of: Warn if non-prefetchable memory aperture
> size is > 32-bit)
> Signed-off-by: Wannes Bouwen <wannes.bouwen@nokia.com>
> ---
> 
> 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",

>  		}
>  
>  		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

RE: [v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

[Wannes Bouwen (Nokia)]

> 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@nokia.com>
> > ---
> >
> > 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