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

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

[Wannes Bouwen (Nokia)]

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

            break;
--
2.39.3

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

[Bjorn Helgaas]

[+cc Vidya and reviewers of fede8526cc48]

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.

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.

Bjorn

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

[Rob Herring]

On Fri, Feb 28, 2025 at 12:27 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Vidya and reviewers of fede8526cc48]
>
> 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()?


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

Rob

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

[Bjorn Helgaas]

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.

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

[Wannes Bouwen (Nokia)]

> 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))
                    dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");

            break;

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

[Bjorn Helgaas]

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.

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

[Wannes Bouwen (Nokia)]

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

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

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

[Bjorn Helgaas]

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