Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB

From: Vidya Sagar
Date: Wed May 20 2020 - 13:51:59 EST



On 20-May-20 6:46 PM, Thierry Reding wrote:
On Wed, May 20, 2020 at 12:06:40PM +0100, Lorenzo Pieralisi wrote:
On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:

[...]

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 42fbfe2a1b8f..a29396529ea4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
pp->mem = win->res;
pp->mem->name = "MEM";
mem_size = resource_size(pp->mem);
- if (upper_32_bits(mem_size))
+ if (upper_32_bits(mem_size) &&
+ !(win->res->flags & IORESOURCE_PREFETCH))
dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
pp->mem_size = mem_size;
pp->mem_bus_addr = pp->mem->start - win->offset;

That warning was added for a reason - why should not we log legitimate
warnings ? AFAIU having resources larger than 4GB can lead to undefined
behaviour given the current ATU programming API.
Yeah. I'm all for a warning if the size is larger than 4GB in case of
non-prefetchable window as one of the ATU outbound translation
channels is being used,

Is it true for all DWC host controllers ? Or there may be another
exception whereby we would be forced to disable this warning altogether
?

but, we are not employing any ATU outbound translation channel for

What does this mean ? "we are not employing any ATU outbound...", is
this the tegra driver ? And what guarantees that this warning is not
legitimate on DWC host controllers that do use the ATU outbound
translation for prefetchable windows ?

Can DWC maintainers chime in and clarify please ?

Before this code section, there is the following function call
pci_parse_request_of_pci_ranges(), which performs a simple validation for
the IORESOURCE_MEM resource type.
This validation checks if the resource is marked as prefetchable, if so,
an error message "non-prefetchable memory resource required" is given and
a return code with the -EINVAL value.

That code checks if there is *at least* a non-prefetchable resource,
that's all it does.

In other words, to reach the code that Vidya is changing, it can be only
if the resource is a non-prefetchable, any prefetchable resource will be
blocked by the previous call, if I'm not mistaken.

I think you are mistaken sorry.

Having this in mind, Vidya's change will not make the expected result
aimed by him.

I think Vidya's patch does what he expects, the question is whether
it is widely applicable to ALL DWC hosts, that's what I want to know.

I don't see any problem by having resources larger than 4GB, from what
I'm seeing in the databook there isn't any restricting related to that as
long they don't consume the maximum space that is addressable by the
system (depending on if they are 32-bit or 64-bit system address).

To be honest, I'm not seeing a system that could have this resource
larger than 4GB, but it might exist some exception that I don't know of,
that's why I accepted Alan's patch to warn the user that the resource
exceeds the maximum for the 32 bits so that he can be aware that he
*might* be consuming the maximum space addressable.

I think it is most certainly a possibility to have > 4GB prefetchable
address spaces so we ought to fix this for good. I still have to
understand how the DWC host detects the memory region to be programmed
into the ATU given that there is more than one but only 1 ATU memory
region AFAICS.

Probably best to wait for Vidya to confirm since I'm not altogether
familiar with PCI on Tegra194, but looking at the DTS files and the
Tegra194 TRM, the prefetchable memory regions are set to a range in
0x1200000000-0x1fffffffff which is a region of the address map that
is reserved for "PCIe aperture for > 32-bit OS". Part of that is in
use for non-prefetchable memory (and ends up being programmed into
the ATU) whereas a much larger part is used for prefetchable memory
and is not programmed anywhere, as far as I can tell.Yes. That is true. In case of Tegra194, for 1-to-1 memory translations,
there is no need to use any ATU regions at all as the HW is capable of generating a memory transaction on the bus on its own for any CPU generated reads/writes falling in these apertures and are not captured by ATU windows for generating other (config/IO) types of transactions. Since a part of 64-bit region is used for mapping non-prefetchable BARs of endpoints (which are only 32-bit in size), a translation is required from 64-bit CPU/AXI address to 32-bit PCIe bus address and ATU region is used precisely for this purpose. Since there is no need for ATU translation to do 1-to-1 memory mapping, rest of the 64-bit aperture is used for mapping prefetchable BARs. I'm not sure if this HW behavior is same across DWC implementations. I took a quick look at the 'ranges' property of other DWC implementations and none of them have a region marked as prefetchable...! I'm not sure how kernel handles mapping an endpoint's prefetchable BAR in this case (does it use the 'non-prefetchable' mapping of 'ranges' property for 'prefetchable' BARs as well??)


But I think given that this is a designated region of the address
map this is probably automatically redirected to the PCIe controller.
What I don't know is if that's something Tegra-specific or whether all
instantiations have something similar set up.

Thierry