diff mbox series

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

Message ID 20210527150541.3130505-2-punitagrawal@gmail.com (mailing list archive)
State New
Headers show
Series Fixup non-prefetchable 64-bit host bridge windows | expand

Commit Message

Punit Agrawal May 27, 2021, 3:05 p.m. UTC
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(-)

Comments

Bjorn Helgaas May 27, 2021, 4:21 p.m. UTC | #1
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
>
Rob Herring May 27, 2021, 4:38 p.m. UTC | #2
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
>
Punit Agrawal May 28, 2021, 12:42 p.m. UTC | #3
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
Punit Agrawal May 28, 2021, 1:24 p.m. UTC | #4
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
diff mbox series

Patch

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