Re: [PATCH] PCI: allow pci_resize_resource() to be used on devices on the root bus

["Christian König" <christian.koenig@amd.com>]

Am 26.04.20 um 11:58 schrieb Ard Biesheuvel:
> On Sun, 26 Apr 2020 at 11:08, Christian König <christian.koenig@amd.com> wrote:
>> Am 25.04.20 um 19:32 schrieb Ard Biesheuvel:
>>> On Tue, 21 Apr 2020 at 19:07, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> On Tue, 21 Apr 2020 at 18:43, Christian König <christian.koenig@amd.com> wrote:
>>>>> Am 21.04.20 um 18:22 schrieb Ard Biesheuvel:
>>>>>> When resizing a BAR, pci_reassign_bridge_resources() is invoked to
>>>>>> bring the bridge windows of parent bridges in line with the new BAR
>>>>>> assignment.
>>>>>>
>>>>>> This assumes that the device whose BAR is being resized lives on a
>>>>>> subordinate bus, but this is not necessarily the case. A device may
>>>>>> live on the root bus, in which case dev->bus->self is NULL, and
>>>>>> passing a NULL pci_dev pointer to pci_reassign_bridge_resources()
>>>>>> will cause it to crash.
>>>>>>
>>>>>> So let's make the call to pci_reassign_bridge_resources() conditional
>>>>>> on whether dev->bus->self is non-NULL in the first place.
>>>>>>
>>>>>> Fixes: 8bb705e3e79d84e7 ("PCI: Add pci_resize_resource() for resizing BARs")
>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>> Sounds like it makes sense, patch is
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>.
>>>> Thanks Christian.
>>>>
>>>>> May I ask where you found that condition?
>>>>>
>>>> In this particular case, it was on an ARM board with funky PCIe IP
>>>> that does not expose a root port in its bus hierarchy.
>>>>
>>>> But in the general case, PCIe endpoints can be integrated into the
>>>> root complex, in which case they appear on the root bus, and there is
>>>> no reason such endpoints shouldn't be allowed to have resizable BARs.
>>> Actually, looking at this more carefully, I think
>>> pci_reassign_bridge_resources() needs to do /something/ to ensure that
>>> the resources are reshuffled if needed when the resized BAR overlaps
>>> with another one.
>> The resized BAR never overlaps with an existing one since to resize a
>> BAR it needs to be deallocated and disabled. This is done as a
>> precaution to avoid potential incorrect routing and decode of memory access.
>>
>> The call to pci_reassign_bridge_resources() is only there to change the
>> resources of the upstream bridge to the device which is resized and not
>> to adjust the resources of the device itself.
>>
> So does that mean that BAR resizing is only possible if no other BARs,
> either on the same device or on other ones, need to be moved?

Well we don't move any other BARs which currently has resources 
assigned. Otherwise it could happen that we change BARs while somebody 
is using them.

See for example how we use this in amdgpu_device_resize_fb_bar().

1. First we make sure that SW isn't accessing the doorbell any more and 
then release the addresses allocated to the FB/VRAM (0) and the doobell 
(2) BAR:

>         /* Free the VRAM and doorbell BAR, we most likely need to move 
> both. */
>         amdgpu_device_doorbell_fini(adev);
>         if (adev->asic_type >= CHIP_BONAIRE)
>                 pci_release_resource(adev->pdev, 2);
>
>         pci_release_resource(adev->pdev, 0);

2. Then we resize the FB/VRAM BAR to whatever the driver thinks is 
appropriate:

> r = pci_resize_resource(adev->pdev, 0, rbar_size);

3. And last we restart SW access to the doorbell and double check if 
resizing either worked ok or we could fallback to the old configuration:

>         /* When the doorbell or fb BAR isn't available we have no 
> chance of
>          * using the device.
>          */
>         r = amdgpu_device_doorbell_init(adev);
>         if (r || (pci_resource_flags(adev->pdev, 0) & IORESOURCE_UNSET))
>                 return -ENODEV;

Regards,
Christian.