| Message ID | 20210527150541.3130505-2-punitagrawal@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fixup non-prefetchable 64-bit host bridge windows | expand |
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 >
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 >
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
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 --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; } }
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(-)