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

[Ard Biesheuvel <ardb@kernel.org>]

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.