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

[Vidya Sagar <vidyas@nvidia.com>]

On 23-May-20 11:00 PM, Vidya Sagar wrote:
> 
> 
> On 22-May-20 7:36 PM, Thierry Reding wrote:
>> On Fri, May 22, 2020 at 02:32:49PM +0100, Lorenzo Pieralisi wrote:
>>> On Fri, May 22, 2020 at 02:06:55PM +0200, Thierry Reding wrote:
>>>> On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
>>>>> On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
>>>>>>
>>>>>>
>>>>>> 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@arm.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@nvidia.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)
>>>>>
>>>>> You are getting lucky with your 'design'. Relying on order is fragile
>>>>> (except of course in the places in DT where order is defined, but 
>>>>> ranges
>>>>> is not one of them).
>>>>
>>>> Yeah, I think the DWC core should be improved to differentiate between
>>>> the two types of memory resources. There shouldn't be a need to encode
>>>> any ordering because the type is already part of the value in the
>>>> ranges property.
>>>
>>> DWC resources handling is broken beyond belief. In practical terms, I
>>> think the best thing I can do is dropping:
>>>
>>> 9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 
>>> 32-bits")
>>>
>>> from my pci/dwc branch. However, the ATU programming API must be fixed
>>> and this reliance on DT entries ordering avoided - it is really bad
>>> practice (and it prevents us from reworking kernel code in ways that are
>>> legitimate but would break owing to DT assumptions).
>>>
>>> So yes, the DWC host bridge code must be updated asap - this is not
>>> acceptable.
>>
>> Vidya, would you have any spare cycles to look into this a bit since
>> you're already familiar? I think for starters it would be good to add
>> a special case to the IORESOURCE_MEM case in dw_pcie_host_init() that
>> deals with IORESOURCE_PREFETCH set and then store the result in a
>> separate struct resource in struct pcie_port, something roughly along
>> the lines of:
>>
>>     struct pcie_port {
>>         ...
>>         struct resource *mem;
>>         struct resource *prefetch;
>>         ...
>>     };
>>
>>     ...
>>
>>     int dw_pcie_host_init(struct pcie_port *pp)
>>     {
>>         ...
>>         resource_list_for_each_entry(win, &bridge->windows) {
>>             switch (resource_type(win->res)) {
>>             ...
>>             case IORESOURCE_MEM:
>>                 if (win->res.flags & IORESOURCE_PREFETCH) {
>>                     pp->prefetch = win->res;
>>                     ...
>>                 } else {
>>                     pp->mem = win->res;
>>                     ...
>>                 }
>>                 break;
>>             ...
>>         }
>>         ...
>>     }
>>
>> I suppose for the non-prefetchable memory we could leave the warning in
>> because they can never be larger than 32 bits anyway. Then again, I'm
>> not sure the check is actually fully correct. My recollection is that
>> non-prefetchable memory needs to be completely within the 4 GiB range,
>> rather than just the base and the size. So I think something like the
>> base starting at 3 GiB and then spanning 2 GiB would be valid according
>> to the current check, but I don't think it's valid according to the
>> specification.
>>
>> The other interesting datapoint to have would be whether the DWC core
>> always has 1:1 mappings for prefetchable memory. If so, I think it might
>> be useful to still parse them, even if nothing in the driver is using
>> them. But I don't know what would be a good way to find out if that's
>> really the case.
>>
>> I also saw, like you did, that none of the other, non-Tegra device trees
>> specify any prefetchable memory for the DWC, so I don't understand how
>> they would work. Perhaps they just don't support prefetchable memory?
>>
>> If you don't have the time to do this I could possibly take a stab at it
>> but there are a few other things I need to look into, so I probably
>> won't get around to this within the next two or so weeks.
> Sure. I'll try to come up with a patch set to address this at DWC core 
> level.
New patch set for review is available @ 
http://patchwork.ozlabs.org/project/linux-pci/list/?series=180799

Thanks,
Vidya Sagar
> 
> Thanks,
> Vidya Sagar
>>
>> Thierry
>>