Message-ID: <cecdf77a-4588-e625-90c5-c4f4b937b184@amd.com>
Date:   Fri, 17 Dec 2021 13:45:47 +0100
From:   Christian König <christian.koenig@....com>
To:     Michał Winiarski <michal.winiarski@...el.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Bjorn Helgaas <bhelgaas@...gle.com>
Cc:     Ard Biesheuvel <ardb@...nel.org>,
        "Michael J . Ruhl" <michael.j.ruhl@...el.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>
Subject: Re: [PATCH 2/2] PCI: Don't fail BAR resize if nothing is reassigned

Am 17.12.21 um 12:23 schrieb Michał Winiarski:
> On 16.12.2021 08:12, Christian König wrote:
>> Am 15.12.21 um 15:16 schrieb Michał Winiarski:
>>> When pci_reassign_bridge_resources returns -ENOENT, it means that no
>>> resources needed to be "moved". This can happen when the resource was
>>> resized to be smaller, and it's completely fine - there's no need to 
>>> treat
>>> this as an error and go back to the original BAR size.
>>
>> Well that doesn't make much sense as far as I can see.
>>
>> Drivers mandatory need to call pci_release_resource() on all 
>> resources which might need to move for a resize, including the one 
>> which is about to be resized.
>
> Since IOV BARs have their own memory-decoding enabled bit, which is 
> usually tied to the lifetime of virtual functions, the PF driver could 
> do IOV BAR resize during its lifetime (without releasing its own 
> resources).

I know, but that is totally irrelevant. See below.

>> When you get -ENOENT from pci_reassign_bridge_resources() it just 
>> means that the function was not able to do it's work because the 
>> driver failed to release it's resources before the resize.
>>
>> Technically we could indeed skip this step if the new size is smaller 
>> than the old size, but then the question is why would somebody resize 
>> in the first place? The freed up address space is not usable if you 
>> don't do this.
>
> With regular BAR, the size of MMIO resource is equal to bar_size.
> With IOV BAR, the size of MMIO resource is equal to iov_bar_size * 
> total_vfs.
>
> It means that the driver could use the pci_resize_resource in two 
> ways, it could just call it like for the native BAR - overall MMIO 
> resource is going to grow, or it could limit its total_vfs (overall 
> MMIO resource is going to shrink, but from VF perspective, its 
> individual BAR is going to be larger).
> [SNIP]

> No changes in resource size, we started with 4G and we end up with 4G 
> after resize (but those 2 VFs can now use 2G BAR).
>
> Does that make sense?

Well no, I already had a good understanding of what you are doing here. 
But that still doesn't really fit what the function is supposed to be doing.

See even when you reduce the number of virtual functions and increase 
the BAR of the remaining functions you previously *must* manually 
release some of the BARs. And even if it's just the VF BAR.

So there is either something wrong in your driver code using this or we 
have an implementation error in the handling of VF BARs in the resize 
function (which is certainly possible).

Regards,
Christian.

>
> Thanks
> -Michał
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Michał Winiarski <michal.winiarski@...el.com>
>>> ---
>>>   drivers/pci/setup-res.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
>>> index 1946e52e7678a..5de5129055e0a 100644
>>> --- a/drivers/pci/setup-res.c
>>> +++ b/drivers/pci/setup-res.c
>>> @@ -484,7 +484,7 @@ int pci_resize_resource(struct pci_dev *dev, int 
>>> resno, int size)
>>>       /* Check if the new config works by trying to assign 
>>> everything. */
>>>       if (dev->bus->self) {
>>>           ret = pci_reassign_bridge_resources(dev->bus->self, 
>>> res->flags);
>>> -        if (ret)
>>> +        if (ret && ret != -ENOENT)
>>>               goto error_resize;
>>>       }
>>>       return 0;
>>
>

Powered by blists - more mailing lists