Re: [PATCH] vpci: Add resizable bar support

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

On Thu, Nov 14, 2024 at 04:52:18PM +0100, Roger Pau Monné wrote:
> 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
> 
> Do you have the output of lspci with the BAR sizes/positions before
> and after the resizing?
> 
> > 
> > I think there is a place that needs to be modified regarding your method,
> > although this modification does not help with the above-mentioned errors,
> > it is that whether to support resizing is specific to which bar, rather 
> > than just determining whether there is a Rebar capability.
> 
> Do we really need such fine-grained information?  It should be
> harmless (even if not strictly necessary) to size all BARs on the
> device before enabling memory decoding, even if some of them do not
> support resizing.
> 
> I might have to provide a patch with additional messages to see what's
> going on.

One nit that I've noticed with the patch I gave you previously is that
the check for a size change in modify_bars() should be done ahead of
pci_check_bar(), otherwise the check is possibly using an outdated
size.

I've also added a debug message to notify when a BAR register is
written and report the new address.  This is done unconditionally, but
if you think it's too chatty you can limit to only printing for the
device that has the ReBAR capability.

Thanks, Roger.

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ef6c965c081c..d49d3588993b 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -346,6 +346,30 @@ static int modify_bars(const struct pci_dev *pdev, 
uint16_t cmd, bool rom_only)
              bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
             continue;
 
+        if ( bar->type != VPCI_BAR_ROM && header->bars_resizable &&
+             (cmd & PCI_COMMAND_MEMORY) )
+        {
+            uint64_t addr, size;
+
+            pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + i * 4,
+                             &addr, &size, 0);
+
+            if ( bar->addr != addr )
+                printk(XENLOG_G_ERR
+                       "%pp: BAR#%u address mismatch %#lx vs %#lx\n",
+                       &pdev->sbdf, i, bar->addr, addr);
+
+            if ( bar->size != size )
+            {
+                printk(XENLOG_G_DEBUG
+                       "%pp: detected BAR#%u size change (%#lx -> %#lx)\n",
+                       &pdev->sbdf, i, bar->size, size);
+                bar->size = size;
+                end = PFN_DOWN(bar->addr + size - 1);
+                end_guest = PFN_DOWN(bar->guest_addr + size - 1);
+            }
+        }
+
         if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
         {
             printk(XENLOG_G_WARNING
@@ -601,6 +625,9 @@ static void cf_check bar_write(
     /* Update guest address, so hardware domain BAR is identity mapped. */
     bar->guest_addr = bar->addr;
 
+gprintk(XENLOG_DEBUG, "%pp: updated BAR%zu address: %#lx\n",
+                    &pdev->sbdf, bar - pdev->vpci->header.bars + hi, 
bar->addr);
+
     /* Make sure Xen writes back the same value for the BAR RO bits. */
     if ( !hi )
     {
@@ -870,6 +897,9 @@ static int cf_check init_header(struct pci_dev *pdev)
     if ( pdev->ignore_bars )
         return 0;
 
+    header->bars_resizable = pci_find_ext_capability(pdev->sbdf,
+                                                     PCI_EXT_CAP_ID_REBAR);
+
     cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
 
     /*
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index 250ba106dbd3..c543a2b86778 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -459,6 +459,7 @@
 #define PCI_EXT_CAP_ID_ARI     14
 #define PCI_EXT_CAP_ID_ATS     15
 #define PCI_EXT_CAP_ID_SRIOV   16
+#define PCI_EXT_CAP_ID_REBAR   21
 
 /* Advanced Error Reporting */
 #define PCI_ERR_UNCOR_STATUS   4       /* Uncorrectable Error Status */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 41e7c3bc2791..45ebc1bb3356 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -129,6 +129,8 @@ struct vpci {
          * upon to know whether BARs are mapped into the guest p2m.
          */
         bool bars_mapped      : 1;
+        /* Device has the Resizable BARs capability. */
+        bool bars_resizable   : 1;
         /* FIXME: currently there's no support for SR-IOV. */
     } header;