Message-ID: <b1a72abe-6da0-b782-0269-65388f663e26@nvidia.com>
Date:   Wed, 20 May 2020 23:16:32 +0530
From:   Vidya Sagar <vidyas@...dia.com>
To:     Thierry Reding <thierry.reding@...il.com>,
        Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>
CC:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        "jingoohan1@...il.com" <jingoohan1@...il.com>,
        "Andrew Murray" <amurray@...goodpenguin.co.uk>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "jonathanh@...dia.com" <jonathanh@...dia.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kthota@...dia.com" <kthota@...dia.com>,
        "mmaddireddy@...dia.com" <mmaddireddy@...dia.com>,
        "sagar.tv@...il.com" <sagar.tv@...il.com>,
        "Alan Mikhak" <alan.mikhak@...ive.com>
Subject: Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory
 resource size >4GB



On 20-May-20 4:47 PM, Thierry Reding wrote:
> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
>> On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
>> <lorenzo.pieralisi@....com> wrote:
>>
>>> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
>>>>
>>>>
>>>> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>>>>>> [+cc Alan; please cc authors of relevant commits,
>>>>>> updated Andrew's email address]
>>>>>>
>>>>>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>>>>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
>>>>>>> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>>>>>>>    memory resources also come under this category where sizes can go beyond
>>>>>>> 4GB. Avoid logging a warning for prefetchable memory resources.
>>>>>>>
>>>>>>> Signed-off-by: Vidya Sagar <vidyas@...dia.com>
>>>>>>> ---
>>>>>>>    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> 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's not what the code is doing. pci_parse_request_of_pci_range() will
> traverse over the whole list of resources that it can find for the given
> host controller and checks whether one of the resources defines prefetch
> memory (note the res_valid |= ...). The error will only be returned if
> no prefetchable memory region was found.
> 
> dw_pcie_host_init() will then again traverse the list of resources and
> it will typically encounter two resource of type IORESOURCE_MEM, one for
> non-prefetchable memory and another for prefetchable memory.
> 
> Vidya's patch is to differentiate between these two resources and allow
> prefetchable memory regions to exceed sizes of 4 GiB.
> 
> That said, I wonder if there isn't a bigger problem at hand here. From
> looking at the code it doesn't seem like the DWC driver makes any
> distinction between prefetchable and non-prefetchable memory. Or at
> least it doesn't allow both to be stored in struct pcie_port.
> 
> Am I missing something? Or can anyone explain how we're programming the
> apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> what Vidya was referring to when he said: "we are not using an outbound
> ATU translation channel for prefetchable memory".
> 
> It looks to me like we're also getting partially lucky, or perhaps that
> is by design, in that Tegra194 defines PCI regions in the following
> order: I/O, prefetchable memory, non-prefetchable memory. That means
> that the DWC core code will overwrite prefetchable memory data with that
> of non-prefetchable memory and hence the non-prefetchable region ends up
> stored in struct pcie_port and is then used to program the ATU outbound
> channel.
Well,it is by design. I mean, since the code is not differentiating 
between prefetchable and non-prefetchable regions, I ordered the entries 
in 'ranges' property in such a way that 'prefetchable' comes first 
followed by 'non-prefetchable' entry so that ATU region is used for 
generating the translation required for 'non-prefetchable' region (which 
is a non 1-to-1 mapping)

> 
>> 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.
>>
>> Having this in mind, Vidya's change will not make the expected result
>> aimed by him.
> 
> Given the above I think it does. We've also seen this patch eliminate a
> warning that we were seeing before, so I think it has the intended
> effect.
> 
>> 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's pretty common to have this type of prefetchable memory
> region when you connect discrete GPUs to PCIe. It's not unusual for
> high-end GPUs to have 8 GiB or even more dedicated video memory and
> those will typically be mapped to a prefetchable memory region on
> the PCI device.
> 
> Thierry
> 

Powered by blists - more mailing lists