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

[Bjorn Helgaas <helgaas@kernel.org>]

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.