Re: Subject: [PATCH 1/1] PCI: of: avoid warning for 4 GiB non-prefetchable

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 04, 2025 at 09:15:07AM +0000, Wannes Bouwen (Nokia) wrote:
> > On Mon, Mar 03, 2025 at 10:35:41AM +0000, Wannes Bouwen (Nokia)
> > wrote:
> > > > On Fri, Feb 28, 2025 at 05:01:51PM -0600, Rob Herring wrote:
> > > > > On Fri, Feb 28, 2025 at 12:27 PM Bjorn Helgaas <helgaas@kernel.org>
> > wrote:
> > > > > > On Thu, Nov 14, 2024 at 02:05:08PM +0000, Wannes Bouwen (Nokia)
> > > > wrote:
> > > > > > > Subject: [PATCH 1/1] PCI: of: avoid warning for 4 GiB
> > > > > > > non-prefetchable windows.
> > > > > > >
> > > > > > > According to the PCIe spec, non-prefetchable memory supports
> > > > > > > only 32-bit BAR registers and are hence limited to 4 GiB. In
> > > > > > > the kernel there is a check that prints a warning if a
> > > > > > > non-prefetchable resource exceeds the 32-bit limit.
> > > > > > >
> > > > > > > This check however prints a warning when a 4 GiB window on the
> > > > > > > host bridge is used. This is perfectly possible according to
> > > > > > > the PCIe spec, so in my opinion the warning is a bit too
> > > > > > > strict. This changeset subtracts 1 from the resource_size to
> > > > > > > avoid printing a warning in the case of a 4 GiB non-prefetchable
> > window.
> > > > > > >
> > > > > > > Signed-off-by: Wannes Bouwen <wannes.bouwen@nokia.com>
> > > > > > > ---
> > > > > > >  drivers/pci/of.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c index
> > > > > > > dacea3fc5128..ccbb1f1c2212 100644
> > > > > > > --- a/drivers/pci/of.c
> > > > > > > +++ b/drivers/pci/of.c
> > > > > > > @@ -622,7 +622,7 @@ static int
> > > > > > > pci_parse_request_of_pci_ranges(struct
> > > > device *dev,
> > > > > > >             res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > > > > > >
> > > > > > >             if (!(res->flags & IORESOURCE_PREFETCH))
> > > > > > > -               if (upper_32_bits(resource_size(res)))
> > > > > > > +               if (upper_32_bits(resource_size(res) - 1))
> > > > > > >                     dev_warn(dev, "Memory resource size
> > > > > > > exceeds max for 32 bits\n");
> > > > > >
> > > > > > I guess this relies on the fact that BARs must be a power of two
> > > > > > in size, right?  So anything where the upper 32 bits of the size
> > > > > > are non-zero is either 0x1_0000_0000 (4GiB window that we
> > > > > > shouldn't warn about), or 0x2_0000_0000 or bigger (where we *do*
> > > > > > want to warn about it).
> > > > > >
> > > > > > But it looks like this is used for host bridge resources, which
> > > > > > are windows, not BARs, so they don't have to be a power of two
> > > > > > size.  A window of size 0x1_8000_0000 is perfectly legal and
> > > > > > would fit the criteria for this warning, but this patch would turn off the
> > warning.
> > > > >
> > > > > 0x1_8000_0000 - 1 = 0x1_7fff_ffff
> > > > >
> > > > > So that would still work. Maybe you read it as the subtract being
> > > > > after upper_32_bits()?
> > > >
> > > > Right, sorry.  I guess a better example would be something like this:
> > > >
> > > >   [mem 0x2000_0000-0x21ff_ffff] -> [pci 0x0_ff00_0000-0x1_00ff_ffff]
> > > >
> > > > where the size is only 0x0200_0000, so we wouldn't warn about it,
> > > > but half of the window is above 4G on PCI.
> > > >
> > > > > > I don't really understand this warning in the first place,
> > > > > > though.  It was added by fede8526cc48 ("PCI: of: Warn if
> > > > > > non-prefetchable memory aperture size is > 32-bit").  But I
> > > > > > think the real issue would be related to the highest address,
> > > > > > not the size.  For example, an aperture of 0x0_c000_0000 -
> > > > > > 0x1_4000_0000 is only 0x8000_0000 in size, but the upper half of
> > > > > > it it would be invalid for non-prefetchable 32-bit BARs.
> > > > >
> > > > > Are we talking CPU addresses or PCI addresses? For CPU addresses,
> > > > > it would be perfectly fine to be above 4G as long as PCI addresses
> > > > > are below 4G, right?
> > > >
> > > > Yes, CPU addresses can be above 4G; all that matters for this is the
> > > > PCI address.
> > > >
> > > > I think what's important is the largest PCI address in the window,
> > > > not the size.
> > >
> > > I agree. The check is I believe in place to make sure the host bridge
> > > non-prefetchable window does not exceed the 4 GiB boundary.
> > > This is not possible due to the register map of PCIe configuration
> > > space type 1 (register 0x20). I guess the check would be more correct
> > > if we just check the end address of the resource instead of the size?
> > > Something like this?
> > >
> > > @@ -622,7 +622,7 @@ static int pci_parse_request_of_pci_ranges(struct
> > device *dev,
> > >             res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> > >
> > >             if (!(res->flags & IORESOURCE_PREFETCH))
> > > -               if (upper_32_bits(resource_size(res)))
> > > +               if (upper_32_bits(res->end))
> > 
> > res->end is a CPU address.  All that matters here is the PCI address,
> > which is different.
> > 
> > You would need pcibios_resource_to_bus() on res, and look at the region.end
> > it returns.
> 
> I've moved the check to devm_of_pci_get_host_bridge_resources() instead of using
> pcibios_resource_to_bus() as suggested. Let me know what you think of this.
> 
> Subject: [PATCH] Subject: [PATCH 1/1] PCI: of: avoid warning for 4 GiB
>  non-prefetchable windows.
> 
> According to the PCIe spec, 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 to avoid warnings for 4 GiB windows.

Seems reasonable to me.  Can you post as a patch (not in the middle of
this conversation)?

Are you seeing this warning on some system?  If so, can we include the
warning and kind of system?

Please include a PCIe spec citation ("PCIe r6.0, sec x.y.z") for the
actual restriction here.

Nits: Add "()" after function names.

> Signed-off-by: Wannes Bouwen <wannes.bouwen@nokia.com>
> ---
>  drivers/pci/of.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 7a806f5c0d20..6523b6dabaa7 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -400,6 +400,10 @@ 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, "Memory resource size exceeds max for 32 bits\n");
>         }
> 
>         pci_add_resource_offset(resources, res, res->start - range.pci_addr);
> @@ -619,10 +623,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;
>         }
>     }