[PATCH 0/2] Fixup non-prefetchable 64-bit host bridge windows

[PATCH 0/2] Fixup non-prefetchable 64-bit host bridge windows

[Punit Agrawal]

Hi,

The couple of patches in the set are looking to address the issue of
failure to allocate bus addresses for endpoints requiring
non-prefetchable memory range on RK3399. The issue surfaced as a
fallout of commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to
resource flags for 64-bit memory addresses").

The fix requires a change in the PCI ranges attributes for host bridge
windows advertised in the device tree (Patch 2) but as there are
already existing platforms out there, a device tree fix alone isn't
sufficient.

Patch 1 introduces an override to drop 64-bit attribute if a host
bridge window lies entirely in the 32bit address range. This overcomes
the limitation of PCI-to-PCI bridges that can only map
non-prefetchable windows in the 32bit address space.

The patches are based on v5.13-rc3 and have been tested on a
RockPro64.

Thanks,
Punit

[0] https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com

Punit Agrawal (2):
  PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB
  arm64: dts: rockchip: Update PCI host bridge window to 32-bit address
    memory

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +-
 drivers/pci/of.c                         | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH 1/2] PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB

[Punit Agrawal]

Some host bridges advertise non-prefetable memory windows that are
entirely located below 4GB but are marked as 64-bit address memory.

Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource
flags for 64-bit memory addresses"), the OF PCI range parser takes a
stricter view and treats 64-bit address ranges as advertised while
before such ranges were treated as 32-bit.

A PCI-to-PCI bridges cannot forward 64-bit non-prefetchable memory
ranges. As a result, the change in behaviour due to the commit causes
allocation failure for devices that are connected behind PCI host
bridges modelled as PCI-to-PCI bridge and require non-prefetchable bus
addresses.

In order to not break platforms, override the 64-bit flag for
non-prefetchable memory ranges that lie entirely below 4GB.

Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh+dt@kernel.org>
---
 drivers/pci/of.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index da5b414d585a..b9d0bee5a088 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -565,10 +565,14 @@ 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 (!(res->flags & IORESOURCE_PREFETCH)) {
 				if (upper_32_bits(resource_size(res)))
 					dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");
-
+				if ((res->flags & IORESOURCE_MEM_64) && !upper_32_bits(res->end)) {
+					dev_warn(dev, "Overriding 64-bit flag for non-prefetchable memory below 4GB\n");
+					res->flags &= ~IORESOURCE_MEM_64;
+				}
+			}
 			break;
 		}
 	}
-- 
2.30.2

Re: [PATCH 1/2] PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB

[Bjorn Helgaas]

On Fri, May 28, 2021 at 12:05:41AM +0900, Punit Agrawal wrote:
> Some host bridges advertise non-prefetable memory windows that are
> entirely located below 4GB but are marked as 64-bit address memory.
> 
> Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource
> flags for 64-bit memory addresses"), the OF PCI range parser takes a
> stricter view and treats 64-bit address ranges as advertised while
> before such ranges were treated as 32-bit.

Conceptually, I'm not sure why we need IORESOURCE_MEM_64 at all on
resources we get from DT.  I think the main point of IORESOURCE_MEM_64
is to convey the information that "this register, e.g., a PCI BAR, has
space for 64-bit values if you need to write to it."

When we're parsing this from DT, I think we're just getting a fixed
value and there's no concept of writing anything back, so it doesn't
seem like we should need to know how wide the hardware register is, or
even whether there *is* a hardware register.

But I'm sure the PCI resource allocation code probably depends on
IORESOURCE_MEM_64 in those host bridge windows in very ugly ways.

> A PCI-to-PCI bridges cannot forward 64-bit non-prefetchable memory
> ranges. As a result, the change in behaviour due to the commit causes
> allocation failure for devices that are connected behind PCI host
> bridges modelled as PCI-to-PCI bridge and require non-prefetchable bus
> addresses.
> 
> In order to not break platforms, override the 64-bit flag for
> non-prefetchable memory ranges that lie entirely below 4GB.
> 
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> ---
>  drivers/pci/of.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index da5b414d585a..b9d0bee5a088 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -565,10 +565,14 @@ 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 (!(res->flags & IORESOURCE_PREFETCH)) {
>  				if (upper_32_bits(resource_size(res)))
>  					dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");
> -
> +				if ((res->flags & IORESOURCE_MEM_64) && !upper_32_bits(res->end)) {
> +					dev_warn(dev, "Overriding 64-bit flag for non-prefetchable memory below 4GB\n");

Maybe "Clearing 64-bit flag"?

Can you include %pR, so we see the resource in question?

Unrelated but close by, would be nice if the preceding warning ("size
exceeds max") also included %pR.

> +					res->flags &= ~IORESOURCE_MEM_64;
> +				}
> +			}
>  			break;
>  		}
>  	}
> -- 
> 2.30.2
> 

Re: [PATCH 1/2] PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB

[Punit Agrawal]

Hi Bjorn,

Thanks for taking a look.

Bjorn Helgaas <helgaas@kernel.org> writes:

> On Fri, May 28, 2021 at 12:05:41AM +0900, Punit Agrawal wrote:
>> Some host bridges advertise non-prefetable memory windows that are
>> entirely located below 4GB but are marked as 64-bit address memory.
>> 
>> Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource
>> flags for 64-bit memory addresses"), the OF PCI range parser takes a
>> stricter view and treats 64-bit address ranges as advertised while
>> before such ranges were treated as 32-bit.
>
> Conceptually, I'm not sure why we need IORESOURCE_MEM_64 at all on
> resources we get from DT.  I think the main point of IORESOURCE_MEM_64
> is to convey the information that "this register, e.g., a PCI BAR, has
> space for 64-bit values if you need to write to it."
>
> When we're parsing this from DT, I think we're just getting a fixed
> value and there's no concept of writing anything back, so it doesn't
> seem like we should need to know how wide the hardware register is, or
> even whether there *is* a hardware register.
>
> But I'm sure the PCI resource allocation code probably depends on
> IORESOURCE_MEM_64 in those host bridge windows in very ugly ways.

Thanks for the explanation. From what I can tell, the IORESOURCE_MEM_64
flag is used in pci_bus_alloc_resource() to allocate from high PCI
addresses. Without the flag allocations above 4GB will fail. Not sure
that's legitimate use of the flag though.

>> A PCI-to-PCI bridges cannot forward 64-bit non-prefetchable memory
>> ranges. As a result, the change in behaviour due to the commit causes
>> allocation failure for devices that are connected behind PCI host
>> bridges modelled as PCI-to-PCI bridge and require non-prefetchable bus
>> addresses.
>> 
>> In order to not break platforms, override the 64-bit flag for
>> non-prefetchable memory ranges that lie entirely below 4GB.
>> 
>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
>> Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>>  drivers/pci/of.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index da5b414d585a..b9d0bee5a088 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -565,10 +565,14 @@ 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 (!(res->flags & IORESOURCE_PREFETCH)) {
>>  				if (upper_32_bits(resource_size(res)))
>>  					dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");
>> -
>> +				if ((res->flags & IORESOURCE_MEM_64) && !upper_32_bits(res->end)) {
>> +					dev_warn(dev, "Overriding 64-bit flag for non-prefetchable memory below 4GB\n");
>
> Maybe "Clearing 64-bit flag"?
>
> Can you include %pR, so we see the resource in question?

I'll follow your suggestions in the next update.

>
> Unrelated but close by, would be nice if the preceding warning ("size
> exceeds max") also included %pR.

Makes sense. I'll add the resource print to improve the message.

Thanks,
Punit

>
>> +					res->flags &= ~IORESOURCE_MEM_64;
>> +				}
>> +			}
>>  			break;
>>  		}
>>  	}
>> -- 
>> 2.30.2
>> 
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 1/2] PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB

[Rob Herring]

On Thu, May 27, 2021 at 10:06 AM Punit Agrawal <punitagrawal@gmail.com> wrote:
>
> Some host bridges advertise non-prefetable memory windows that are
> entirely located below 4GB but are marked as 64-bit address memory.
>
> Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource
> flags for 64-bit memory addresses"), the OF PCI range parser takes a
> stricter view and treats 64-bit address ranges as advertised while
> before such ranges were treated as 32-bit.
>
> A PCI-to-PCI bridges cannot forward 64-bit non-prefetchable memory
> ranges. As a result, the change in behaviour due to the commit causes
> allocation failure for devices that are connected behind PCI host
> bridges modelled as PCI-to-PCI bridge and require non-prefetchable bus
> addresses.
>
> In order to not break platforms, override the 64-bit flag for
> non-prefetchable memory ranges that lie entirely below 4GB.
>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
> Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> ---
>  drivers/pci/of.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index da5b414d585a..b9d0bee5a088 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -565,10 +565,14 @@ 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 (!(res->flags & IORESOURCE_PREFETCH)) {
>                                 if (upper_32_bits(resource_size(res)))
>                                         dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");

Based on Ard's explanation, doesn't this need to also check for
!IORESOURCE_MEM_64?

> -
> +                               if ((res->flags & IORESOURCE_MEM_64) && !upper_32_bits(res->end)) {

res->end is the CPU address space. Isn't it the PCI address space we care about?

> +                                       dev_warn(dev, "Overriding 64-bit flag for non-prefetchable memory below 4GB\n");
> +                                       res->flags &= ~IORESOURCE_MEM_64;
> +                               }
> +                       }
>                         break;
>                 }
>         }
> --
> 2.30.2
>

Re: [PATCH 1/2] PCI: of: Override 64-bit flag for non-prefetchable memory below 4GB

[Punit Agrawal]

Rob Herring <robh+dt@kernel.org> writes:

> On Thu, May 27, 2021 at 10:06 AM Punit Agrawal <punitagrawal@gmail.com> wrote:
>>
>> Some host bridges advertise non-prefetable memory windows that are
>> entirely located below 4GB but are marked as 64-bit address memory.
>>
>> Since commit 9d57e61bf723 ("of/pci: Add IORESOURCE_MEM_64 to resource
>> flags for 64-bit memory addresses"), the OF PCI range parser takes a
>> stricter view and treats 64-bit address ranges as advertised while
>> before such ranges were treated as 32-bit.
>>
>> A PCI-to-PCI bridges cannot forward 64-bit non-prefetchable memory
>> ranges. As a result, the change in behaviour due to the commit causes
>> allocation failure for devices that are connected behind PCI host
>> bridges modelled as PCI-to-PCI bridge and require non-prefetchable bus
>> addresses.
>>
>> In order to not break platforms, override the 64-bit flag for
>> non-prefetchable memory ranges that lie entirely below 4GB.
>>
>> Suggested-by: Ard Biesheuvel <ardb@kernel.org>
>> Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> ---
>>  drivers/pci/of.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>> index da5b414d585a..b9d0bee5a088 100644
>> --- a/drivers/pci/of.c
>> +++ b/drivers/pci/of.c
>> @@ -565,10 +565,14 @@ 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 (!(res->flags & IORESOURCE_PREFETCH)) {
>>                                 if (upper_32_bits(resource_size(res)))
>>                                         dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");
>
> Based on Ard's explanation, doesn't this need to also check for
> !IORESOURCE_MEM_64?

Right - I was too focussed on the below case.
>
>> -
>> +                               if ((res->flags & IORESOURCE_MEM_64) && !upper_32_bits(res->end)) {
>
> res->end is the CPU address space. Isn't it the PCI address space we
> care about?

Indeed. I suspect the easiest way to check PCI addresses would be to
move the check to where the range property is being parsed.

I'll address both the comments with the next update.

Thanks,
Punit

>
>> +                                       dev_warn(dev, "Overriding 64-bit flag for non-prefetchable memory below 4GB\n");
>> +                                       res->flags &= ~IORESOURCE_MEM_64;
>> +                               }
>> +                       }
>>                         break;
>>                 }
>>         }
>> --
>> 2.30.2
>>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH 2/2] arm64: dts: rockchip: Update PCI host bridge window to 32-bit address memory

[Punit Agrawal]

The PCIe host bridge on RK3399 advertises a single 64-bit memory
address range even though it lies entirely below 4GB.

Previously the OF PCI range parser treated 64-bit ranges more
leniently (i.e., as 32-bit), but since commit 9d57e61bf723 ("of/pci:
Add IORESOURCE_MEM_64 to resource flags for 64-bit memory addresses")
the code takes a stricter view and treats the ranges as advertised in
the device tree (i.e, as 64-bit).

The change in behaviour causes failure when allocating bus addresses
to devices connected behind a PCI-to-PCI bridge that require
non-prefetchable memory ranges. The allocation failure was observed
for certain Samsung NVMe drives connected to RockPro64 boards.

Update the host bridge window attributes to treat it as 32-bit address
memory. This fixes the allocation failure observed since commit
9d57e61bf723.

Reported-by: Alexandru Elisei <alexandru.elisei@arm.com>
Link: https://lore.kernel.org/r/7a1e2ebc-f7d8-8431-d844-41a9c36a8911@arm.com
Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Rob Herring <robh+dt@kernel.org>
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 634a91af8e83..4b854eb21f72 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -227,7 +227,7 @@ pcie0: pcie@f8000000 {
 		       <&pcie_phy 2>, <&pcie_phy 3>;
 		phy-names = "pcie-phy-0", "pcie-phy-1",
 			    "pcie-phy-2", "pcie-phy-3";
-		ranges = <0x83000000 0x0 0xfa000000 0x0 0xfa000000 0x0 0x1e00000>,
+		ranges = <0x82000000 0x0 0xfa000000 0x0 0xfa000000 0x0 0x1e00000>,
 			 <0x81000000 0x0 0xfbe00000 0x0 0xfbe00000 0x0 0x100000>;
 		resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
 			 <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>,
-- 
2.30.2