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

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

[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>
---
 drivers/pci/setup-res.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index d8ca40a97693..d21fa04fa44d 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -439,10 +439,11 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
 
 	/* Check if the new config works by trying to assign everything. */
-	ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
-	if (ret)
-		goto error_resize;
-
+	if (dev->bus->self) {
+		ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
+		if (ret)
+			goto error_resize;
+	}
 	return 0;
 
 error_resize:
-- 
2.17.1

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

[Christian König]

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>.

May I ask where you found that condition?

Thanks,
Christian.

> ---
>   drivers/pci/setup-res.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index d8ca40a97693..d21fa04fa44d 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -439,10 +439,11 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>   	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
>   
>   	/* Check if the new config works by trying to assign everything. */
> -	ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
> -	if (ret)
> -		goto error_resize;
> -
> +	if (dev->bus->self) {
> +		ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
> +		if (ret)
> +			goto error_resize;
> +	}
>   	return 0;
>   
>   error_resize:

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

[Ard Biesheuvel]

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.

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

[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.

Bjorn, did you have any thoughts on this? I could make
pci_reassign_bridge_resources() take a pci_bus, and handle the root
bus as a special case. Alternatively, pci_resize_resource() could make
the distinction, but it will probably need to duplicate some of the
reassignment that goes on in pci_reassign_bridge_resources() as well.

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

[Christian König]

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.

Regards,
Christian.

> Bjorn, did you have any thoughts on this? I could make
> pci_reassign_bridge_resources() take a pci_bus, and handle the root
> bus as a special case. Alternatively, pci_resize_resource() could make
> the distinction, but it will probably need to duplicate some of the
> reassignment that goes on in pci_reassign_bridge_resources() as well.

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

[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?

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

[Ard Biesheuvel]

On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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?

OK, so obviously, the current code already releases and reassigns
resources on the same device.

What I am not getting is how this works with more complex topologies, e.g.,

RP 1
multi function device (e.g., GPU + HDMI)
GPU BAR 0 256M
GPU BAR 1 16 M
HDMI BAR 0 16 KB

RP 2
some other endpoint using MMIO64 BARs

So in this case, RP1 will get a prefetchable bridge BAR window of 512
M, and RP2 may get one that is directly adjacent to that. When
resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
reshuffled, right?

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

[Christian König]

Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>> 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?
> OK, so obviously, the current code already releases and reassigns
> resources on the same device.
>
> What I am not getting is how this works with more complex topologies, e.g.,
>
> RP 1
> multi function device (e.g., GPU + HDMI)
> GPU BAR 0 256M
> GPU BAR 1 16 M
> HDMI BAR 0 16 KB
>
> RP 2
> some other endpoint using MMIO64 BARs
>
> So in this case, RP1 will get a prefetchable bridge BAR window of 512
> M, and RP2 may get one that is directly adjacent to that. When
> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
> reshuffled, right?

Correct, yes. Because you not only need to configure the BARs of the 
device(s), but also the bridges in between to get the routing right.

Just wrote another mail with an example how this works on amdgpu. What 
aids us in amdgpu is that the devices only has two 64bit BARs, the 
FB/VRAM which we want to resize and the doorbell.

I can easily disable access to the doorbell BAR temporary in amdgpu, 
otherwise the whole resize wouldn't work at all.

Regards,
Christian.

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

[Ard Biesheuvel]

On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
>
> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
> > On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >> 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?
> > OK, so obviously, the current code already releases and reassigns
> > resources on the same device.
> >
> > What I am not getting is how this works with more complex topologies, e.g.,
> >
> > RP 1
> > multi function device (e.g., GPU + HDMI)
> > GPU BAR 0 256M
> > GPU BAR 1 16 M
> > HDMI BAR 0 16 KB
> >
> > RP 2
> > some other endpoint using MMIO64 BARs
> >
> > So in this case, RP1 will get a prefetchable bridge BAR window of 512
> > M, and RP2 may get one that is directly adjacent to that. When
> > resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
> > reshuffled, right?
>
> Correct, yes. Because you not only need to configure the BARs of the
> device(s), but also the bridges in between to get the routing right.
>

Right. But my point is really the RP2 is not a bridge in between.

> Just wrote another mail with an example how this works on amdgpu. What
> aids us in amdgpu is that the devices only has two 64bit BARs, the
> FB/VRAM which we want to resize and the doorbell.
>
> I can easily disable access to the doorbell BAR temporary in amdgpu,
> otherwise the whole resize wouldn't work at all.
>

OK, so the example is clear, and I understand how you have to walk the
path up to the root bus to reconfigure the bridge BARs on each
upstream bridge.

But at each level, the BAR space that is being reassigned may be in
use by another device already, no? RP2 in my example is a sibling of
RP1, so the walk up to the root will not traverse it. If RP2's  bridge
BARs conflict with the resized BAR below RP1, will the resize simply
fail?

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

[Christian König]

Am 26.04.20 um 12:59 schrieb Ard Biesheuvel:
> On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
>> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
>>> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> 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?
>>> OK, so obviously, the current code already releases and reassigns
>>> resources on the same device.
>>>
>>> What I am not getting is how this works with more complex topologies, e.g.,
>>>
>>> RP 1
>>> multi function device (e.g., GPU + HDMI)
>>> GPU BAR 0 256M
>>> GPU BAR 1 16 M
>>> HDMI BAR 0 16 KB
>>>
>>> RP 2
>>> some other endpoint using MMIO64 BARs
>>>
>>> So in this case, RP1 will get a prefetchable bridge BAR window of 512
>>> M, and RP2 may get one that is directly adjacent to that. When
>>> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
>>> reshuffled, right?
>> Correct, yes. Because you not only need to configure the BARs of the
>> device(s), but also the bridges in between to get the routing right.
>>
> Right. But my point is really the RP2 is not a bridge in between.
>
>> Just wrote another mail with an example how this works on amdgpu. What
>> aids us in amdgpu is that the devices only has two 64bit BARs, the
>> FB/VRAM which we want to resize and the doorbell.
>>
>> I can easily disable access to the doorbell BAR temporary in amdgpu,
>> otherwise the whole resize wouldn't work at all.
>>
> OK, so the example is clear, and I understand how you have to walk the
> path up to the root bus to reconfigure the bridge BARs on each
> upstream bridge.
>
> But at each level, the BAR space that is being reassigned may be in
> use by another device already, no? RP2 in my example is a sibling of
> RP1, so the walk up to the root will not traverse it. If RP2's  bridge
> BARs conflict with the resized BAR below RP1, will the resize simply
> fail?

Yes exactly that. When BARs on upstream bridges can't be extended 
because some other downstream BAR can't move around we just abort the 
resize.

Regards,
Christian.

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

[Ard Biesheuvel]

On Sun, 26 Apr 2020 at 13:27, Christian König <christian.koenig@amd.com> wrote:
>
> Am 26.04.20 um 12:59 schrieb Ard Biesheuvel:
> > On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
> >> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
> >>> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>> 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?
> >>> OK, so obviously, the current code already releases and reassigns
> >>> resources on the same device.
> >>>
> >>> What I am not getting is how this works with more complex topologies, e.g.,
> >>>
> >>> RP 1
> >>> multi function device (e.g., GPU + HDMI)
> >>> GPU BAR 0 256M
> >>> GPU BAR 1 16 M
> >>> HDMI BAR 0 16 KB
> >>>
> >>> RP 2
> >>> some other endpoint using MMIO64 BARs
> >>>
> >>> So in this case, RP1 will get a prefetchable bridge BAR window of 512
> >>> M, and RP2 may get one that is directly adjacent to that. When
> >>> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
> >>> reshuffled, right?
> >> Correct, yes. Because you not only need to configure the BARs of the
> >> device(s), but also the bridges in between to get the routing right.
> >>
> > Right. But my point is really the RP2 is not a bridge in between.
> >
> >> Just wrote another mail with an example how this works on amdgpu. What
> >> aids us in amdgpu is that the devices only has two 64bit BARs, the
> >> FB/VRAM which we want to resize and the doorbell.
> >>
> >> I can easily disable access to the doorbell BAR temporary in amdgpu,
> >> otherwise the whole resize wouldn't work at all.
> >>
> > OK, so the example is clear, and I understand how you have to walk the
> > path up to the root bus to reconfigure the bridge BARs on each
> > upstream bridge.
> >
> > But at each level, the BAR space that is being reassigned may be in
> > use by another device already, no? RP2 in my example is a sibling of
> > RP1, so the walk up to the root will not traverse it. If RP2's  bridge
> > BARs conflict with the resized BAR below RP1, will the resize simply
> > fail?
>
> Yes exactly that. When BARs on upstream bridges can't be extended
> because some other downstream BAR can't move around we just abort the
> resize.
>

OK.

So how does this work with, e.g., other functions on the same
endpoint, like the audio adapter that is exposed for the audio part of
the HDMI port? Without releasing its BAR resource, it may end up
overlapping, no? And you cannot really release it without
collaboration from the driver, afaict.

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

[Christian König]

Am 26.04.20 um 14:08 schrieb Ard Biesheuvel:
> On Sun, 26 Apr 2020 at 13:27, Christian König <christian.koenig@amd.com> wrote:
>> Am 26.04.20 um 12:59 schrieb Ard Biesheuvel:
>>> On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
>>>> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
>>>>> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>> 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?
>>>>> OK, so obviously, the current code already releases and reassigns
>>>>> resources on the same device.
>>>>>
>>>>> What I am not getting is how this works with more complex topologies, e.g.,
>>>>>
>>>>> RP 1
>>>>> multi function device (e.g., GPU + HDMI)
>>>>> GPU BAR 0 256M
>>>>> GPU BAR 1 16 M
>>>>> HDMI BAR 0 16 KB
>>>>>
>>>>> RP 2
>>>>> some other endpoint using MMIO64 BARs
>>>>>
>>>>> So in this case, RP1 will get a prefetchable bridge BAR window of 512
>>>>> M, and RP2 may get one that is directly adjacent to that. When
>>>>> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
>>>>> reshuffled, right?
>>>> Correct, yes. Because you not only need to configure the BARs of the
>>>> device(s), but also the bridges in between to get the routing right.
>>>>
>>> Right. But my point is really the RP2 is not a bridge in between.
>>>
>>>> Just wrote another mail with an example how this works on amdgpu. What
>>>> aids us in amdgpu is that the devices only has two 64bit BARs, the
>>>> FB/VRAM which we want to resize and the doorbell.
>>>>
>>>> I can easily disable access to the doorbell BAR temporary in amdgpu,
>>>> otherwise the whole resize wouldn't work at all.
>>>>
>>> OK, so the example is clear, and I understand how you have to walk the
>>> path up to the root bus to reconfigure the bridge BARs on each
>>> upstream bridge.
>>>
>>> But at each level, the BAR space that is being reassigned may be in
>>> use by another device already, no? RP2 in my example is a sibling of
>>> RP1, so the walk up to the root will not traverse it. If RP2's  bridge
>>> BARs conflict with the resized BAR below RP1, will the resize simply
>>> fail?
>> Yes exactly that. When BARs on upstream bridges can't be extended
>> because some other downstream BAR can't move around we just abort the
>> resize.
>>
> OK.
>
> So how does this work with, e.g., other functions on the same
> endpoint, like the audio adapter that is exposed for the audio part of
> the HDMI port? Without releasing its BAR resource, it may end up
> overlapping, no? And you cannot really release it without
> collaboration from the driver, afaict.

Correct yes. The trick we use with AMD GPUs is that the audio endpoint, 
GPU MMIO register etc.. are all 32bit resources. The only two 64bit 
resources we have are the FB/VRAM and the doorbell BAR.

Since we usually need to resize the FB/VRAM BAR much larger than the 
32bit/4GB limit anyway we separate the 32bit resources from the 64bit 
resources.

This works since bridges have separate 32bit and 64bit windows for 32bit 
and 64bit resources.

But yes if you have a mix of 64bit devices behind a single bridge this 
will currently fail rather obviously.

What we could maybe do to improve this is to teach the resources 
assignment code in the PCI subsystem to take resize-able BARs into 
account. This way we could trigger resource reallocation before drivers 
load and start using the BARs in question.

Regards,
Christian.

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

[Ard Biesheuvel]

On Sun, 26 Apr 2020 at 14:55, Christian König <christian.koenig@amd.com> wrote:
>
> Am 26.04.20 um 14:08 schrieb Ard Biesheuvel:
> > On Sun, 26 Apr 2020 at 13:27, Christian König <christian.koenig@amd.com> wrote:
> >> Am 26.04.20 um 12:59 schrieb Ard Biesheuvel:
> >>> On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
> >>>> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
> >>>>> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>>>> 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?
> >>>>> OK, so obviously, the current code already releases and reassigns
> >>>>> resources on the same device.
> >>>>>
> >>>>> What I am not getting is how this works with more complex topologies, e.g.,
> >>>>>
> >>>>> RP 1
> >>>>> multi function device (e.g., GPU + HDMI)
> >>>>> GPU BAR 0 256M
> >>>>> GPU BAR 1 16 M
> >>>>> HDMI BAR 0 16 KB
> >>>>>
> >>>>> RP 2
> >>>>> some other endpoint using MMIO64 BARs
> >>>>>
> >>>>> So in this case, RP1 will get a prefetchable bridge BAR window of 512
> >>>>> M, and RP2 may get one that is directly adjacent to that. When
> >>>>> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
> >>>>> reshuffled, right?
> >>>> Correct, yes. Because you not only need to configure the BARs of the
> >>>> device(s), but also the bridges in between to get the routing right.
> >>>>
> >>> Right. But my point is really the RP2 is not a bridge in between.
> >>>
> >>>> Just wrote another mail with an example how this works on amdgpu. What
> >>>> aids us in amdgpu is that the devices only has two 64bit BARs, the
> >>>> FB/VRAM which we want to resize and the doorbell.
> >>>>
> >>>> I can easily disable access to the doorbell BAR temporary in amdgpu,
> >>>> otherwise the whole resize wouldn't work at all.
> >>>>
> >>> OK, so the example is clear, and I understand how you have to walk the
> >>> path up to the root bus to reconfigure the bridge BARs on each
> >>> upstream bridge.
> >>>
> >>> But at each level, the BAR space that is being reassigned may be in
> >>> use by another device already, no? RP2 in my example is a sibling of
> >>> RP1, so the walk up to the root will not traverse it. If RP2's  bridge
> >>> BARs conflict with the resized BAR below RP1, will the resize simply
> >>> fail?
> >> Yes exactly that. When BARs on upstream bridges can't be extended
> >> because some other downstream BAR can't move around we just abort the
> >> resize.
> >>
> > OK.
> >
> > So how does this work with, e.g., other functions on the same
> > endpoint, like the audio adapter that is exposed for the audio part of
> > the HDMI port? Without releasing its BAR resource, it may end up
> > overlapping, no? And you cannot really release it without
> > collaboration from the driver, afaict.
>
> Correct yes. The trick we use with AMD GPUs is that the audio endpoint,
> GPU MMIO register etc.. are all 32bit resources. The only two 64bit
> resources we have are the FB/VRAM and the doorbell BAR.
>
> Since we usually need to resize the FB/VRAM BAR much larger than the
> 32bit/4GB limit anyway we separate the 32bit resources from the 64bit
> resources.
>
> This works since bridges have separate 32bit and 64bit windows for 32bit
> and 64bit resources.
>
> But yes if you have a mix of 64bit devices behind a single bridge this
> will currently fail rather obviously.
>

OK, thanks for confirming.

> What we could maybe do to improve this is to teach the resources
> assignment code in the PCI subsystem to take resize-able BARs into
> account. This way we could trigger resource reallocation before drivers
> load and start using the BARs in question.
>

Yes, that would probably be better.

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

[Christian König]

Am 26.04.20 um 15:04 schrieb Ard Biesheuvel:
> On Sun, 26 Apr 2020 at 14:55, Christian König <christian.koenig@amd.com> wrote:
>> Am 26.04.20 um 14:08 schrieb Ard Biesheuvel:
>>> On Sun, 26 Apr 2020 at 13:27, Christian König <christian.koenig@amd.com> wrote:
>>>> Am 26.04.20 um 12:59 schrieb Ard Biesheuvel:
>>>>> On Sun, 26 Apr 2020 at 12:53, Christian König <christian.koenig@amd.com> wrote:
>>>>>> Am 26.04.20 um 12:46 schrieb Ard Biesheuvel:
>>>>>>> On Sun, 26 Apr 2020 at 11:58, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>>>>> 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?
>>>>>>> OK, so obviously, the current code already releases and reassigns
>>>>>>> resources on the same device.
>>>>>>>
>>>>>>> What I am not getting is how this works with more complex topologies, e.g.,
>>>>>>>
>>>>>>> RP 1
>>>>>>> multi function device (e.g., GPU + HDMI)
>>>>>>> GPU BAR 0 256M
>>>>>>> GPU BAR 1 16 M
>>>>>>> HDMI BAR 0 16 KB
>>>>>>>
>>>>>>> RP 2
>>>>>>> some other endpoint using MMIO64 BARs
>>>>>>>
>>>>>>> So in this case, RP1 will get a prefetchable bridge BAR window of 512
>>>>>>> M, and RP2 may get one that is directly adjacent to that. When
>>>>>>> resizing GPU BAR 0 to 4 GB, the whole hierarchy needs to be
>>>>>>> reshuffled, right?
>>>>>> Correct, yes. Because you not only need to configure the BARs of the
>>>>>> device(s), but also the bridges in between to get the routing right.
>>>>>>
>>>>> Right. But my point is really the RP2 is not a bridge in between.
>>>>>
>>>>>> Just wrote another mail with an example how this works on amdgpu. What
>>>>>> aids us in amdgpu is that the devices only has two 64bit BARs, the
>>>>>> FB/VRAM which we want to resize and the doorbell.
>>>>>>
>>>>>> I can easily disable access to the doorbell BAR temporary in amdgpu,
>>>>>> otherwise the whole resize wouldn't work at all.
>>>>>>
>>>>> OK, so the example is clear, and I understand how you have to walk the
>>>>> path up to the root bus to reconfigure the bridge BARs on each
>>>>> upstream bridge.
>>>>>
>>>>> But at each level, the BAR space that is being reassigned may be in
>>>>> use by another device already, no? RP2 in my example is a sibling of
>>>>> RP1, so the walk up to the root will not traverse it. If RP2's  bridge
>>>>> BARs conflict with the resized BAR below RP1, will the resize simply
>>>>> fail?
>>>> Yes exactly that. When BARs on upstream bridges can't be extended
>>>> because some other downstream BAR can't move around we just abort the
>>>> resize.
>>>>
>>> OK.
>>>
>>> So how does this work with, e.g., other functions on the same
>>> endpoint, like the audio adapter that is exposed for the audio part of
>>> the HDMI port? Without releasing its BAR resource, it may end up
>>> overlapping, no? And you cannot really release it without
>>> collaboration from the driver, afaict.
>> Correct yes. The trick we use with AMD GPUs is that the audio endpoint,
>> GPU MMIO register etc.. are all 32bit resources. The only two 64bit
>> resources we have are the FB/VRAM and the doorbell BAR.
>>
>> Since we usually need to resize the FB/VRAM BAR much larger than the
>> 32bit/4GB limit anyway we separate the 32bit resources from the 64bit
>> resources.
>>
>> This works since bridges have separate 32bit and 64bit windows for 32bit
>> and 64bit resources.
>>
>> But yes if you have a mix of 64bit devices behind a single bridge this
>> will currently fail rather obviously.
>>
> OK, thanks for confirming.
>
>> What we could maybe do to improve this is to teach the resources
>> assignment code in the PCI subsystem to take resize-able BARs into
>> account. This way we could trigger resource reallocation before drivers
>> load and start using the BARs in question.
>>
> Yes, that would probably be better.

The problem is that this approach usually doesn't work with your primary 
video device.

Either the vga, vesa or efi driver are already using the BAR during 
boot. So you need to kick those out first or otherwise the resize just 
results in a crash.

That's the reason why we haven't gone down that route so far. But I 
think that improving the size estimation for 64bit BARs would still be a 
good idea anyway.

Regards,
Christian.

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

[Christian König]

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.

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

[Bjorn Helgaas]

On Tue, Apr 21, 2020 at 06:22:56PM +0200, Ard Biesheuvel wrote:
> 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>

Applied with Christian's reviewed-by to pci/resource for v5.8, thanks!

> ---
>  drivers/pci/setup-res.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index d8ca40a97693..d21fa04fa44d 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -439,10 +439,11 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>  	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
>  
>  	/* Check if the new config works by trying to assign everything. */
> -	ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
> -	if (ret)
> -		goto error_resize;
> -
> +	if (dev->bus->self) {
> +		ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
> +		if (ret)
> +			goto error_resize;
> +	}
>  	return 0;
>  
>  error_resize:
> -- 
> 2.17.1
>