Re: [PATCH] vpci: Add resizable bar support

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 14, 2024 at 06:11:46AM +0000, Chen, Jiqian wrote:
> On 2024/11/13 18:30, Roger Pau Monné wrote:
> > On Wed, Nov 13, 2024 at 10:00:33AM +0000, Chen, Jiqian wrote:
> >> On 2024/11/13 17:30, Roger Pau Monné wrote:
> >>> On Wed, Nov 13, 2024 at 04:00:27PM +0800, Jiqian Chen wrote:
> >>>> Some devices, like discrete GPU of amd, support resizable bar capability,
> >>>> but vpci of Xen doesn't support this feature, so they fail to resize bars
> >>>> and then cause probing failure.
> >>>>
> >>>> According to PCIe spec, each bar that support resizing has two registers,
> >>>> PCI_REBAR_CAP and PCI_REBAR_CTRL, so add these two registers and their
> >>>> corresponding handler into vpci.
> >>>>
> >>>> PCI_REBAR_CAP is RO, only provide reading.
> >>>>
> >>>> PCI_REBAR_CTRL only has bar size is RW, so add write function to support
> >>>> setting the new size.
> >>>
> >>> I think the logic to handle resizable BAR could be much simpler.  Some
> >>> time ago I've made a patch to add support for it, but due to lack of
> >>> hardware on my side to test it I've never submitted it.
> >>>
> >>> My approach would be to detect the presence of the
> >>> PCI_EXT_CAP_ID_REBAR capability in init_header(), and if the
> >>> capability is present force the sizing of BARs each time they are
> >>> mapped in modify_bars().  I don't think we need to trap accesses to
> >>> the capability itself, as resizing can only happen when memory
> >>> decoding is not enabled for the device.  It's enough to fetch the size
> >>> of the BARs ahead of each enabling of memory decoding.
> >>>
> >>> Note that memory decoding implies mapping the BARs into the p2m, which
> >>> is already an expensive operation, the extra sizing is unlikely to
> >>> make much of a difference performance wise.
> >>>
> >>> I've found the following on my git tree and rebased on top of staging:
> >> OK.
> >> Do you need me to validate your patch in my environment?
> > 
> > Yes please, I have no way to test it.  Let's see what others think
> > about the different approaches.
> There are some errors with your method.
> I attached the dmesg and xl dmesg logs.
> From the dmesg logs, it seems that 0000:03:00.0 has addresse overlap with 
> 0000:03:00.1

I'm attempting to go over Linux dmesg, and so far got the following
information about the BAR positions for 0000:03:00.0 and 0000:03:00.1:

[    1.101522] pci 0000:03:00.0: [1002:73ff] type 00 class 0x030000 PCIe Legacy 
Endpoint
[    1.102416] pci 0000:03:00.0: BAR 0 [mem 0xfc90000000-0xfc9fffffff 64bit 
pref]
[    1.105592] pci 0000:03:00.0: BAR 2 [mem 0xfca0000000-0xfca01fffff 64bit 
pref]
[    1.106665] pci 0000:03:00.0: BAR 4 [io  0x2000-0x20ff]
[    1.109858] pci 0000:03:00.0: BAR 5 [mem 0xd0600000-0xd06fffff]
[    1.112962] pci 0000:03:00.0: ROM [mem 0xfffe0000-0xffffffff pref]
[    1.113543] pci 0000:03:00.0: PME# supported from D1 D2 D3hot D3cold
[    1.114031] pci 0000:03:00.0: 63.008 Gb/s available PCIe bandwidth, limited 
by 8.0 GT/s PCIe x8 link at 0000:00:01.1 (capable of 252.048 Gb/s with 16.0 
GT/s PCIe x16 link)
[    1.115074] pci 0000:03:00.1: [1002:ab28] type 00 class 0x040300 PCIe Legacy 
Endpoint
[    1.115207] pci 0000:03:00.1: BAR 0 [mem 0xd0700000-0xd0703fff]
[    1.115962] pci 0000:03:00.1: PME# supported from D1 D2 D3hot D3cold
[  120.049678] amdgpu 0000:03:00.0: BAR 2 [mem 0xfca0000000-0xfca01fffff 64bit 
pref]: releasing
[  120.049682] amdgpu 0000:03:00.0: BAR 0 [mem 0xfc90000000-0xfc9fffffff 64bit 
pref]: releasing
[  120.049815] amdgpu 0000:03:00.0: BAR 0 [mem 0xa00000000-0xbffffffff 64bit 
pref]: assigned
[  120.049833] amdgpu 0000:03:00.0: BAR 2 [mem 0x900000000-0x9001fffff 64bit 
pref]: assigned

So the only BAR from 0000:03:00.1 is always at 0xd0700000-0xd0703fff,
and BARs for 0000:03:00.0 are relocated from:

0xfca0000000-0xfca01fffff -> 0x900000000-0x9001fffff (same size)
0xfc90000000-0xfc9fffffff -> 0xa00000000-0xbffffffff (resized)

So I see no overlap between the BARs of 0000:03:00.0 and 0000:03:00.1,
but maybe I'm missing some position changes, following Linux dmesg is
not easy.

Regards, Roger.