Re: [PATCH] vpci: Add resizable bar support

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

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 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.
So, based on your method, I have below changes:
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 045aa4bdadc8..50f19b18a232 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -356,7 +356,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t 
cmd, bool rom_only)

         ASSERT(rangeset_is_empty(bar->mem));

-        if ( bar->type != VPCI_BAR_ROM && header->bars_resizable &&
+        if ( bar->type != VPCI_BAR_ROM && bar->resizable &&
              (cmd & PCI_COMMAND_MEMORY) )
         {
             uint64_t addr, size;
@@ -779,6 +779,8 @@ static int cf_check init_header(struct pci_dev *pdev)
     int rc;
     bool mask_cap_list = false;
     bool is_hwdom = is_hardware_domain(pdev->domain);
+    unsigned int rebar_offset;
+    uint32_t ctrl, ctrl_nbars;

     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));

@@ -894,8 +896,15 @@ 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);
+    rebar_offset = pci_find_ext_capability(pdev->sbdf, PCI_EXT_CAP_ID_REBAR);
+    if ( rebar_offset ) {
+        ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
+        ctrl_nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> 
PCI_REBAR_CTRL_NBAR_SHIFT;
+        for ( int i = 0; i < ctrl_nbars; i++, rebar_offset += 8 ) {
+            ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL);
+            bars[ctrl & PCI_REBAR_CTRL_BAR_IDX].resizable = true;
+        }
+    }

     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 c543a2b86778..7ce360b8ac69 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -617,4 +617,10 @@
 #define  PCI_SRIOV_VFM_MO      0x2     /* Active.MigrateOut */
 #define  PCI_SRIOV_VFM_AV      0x3     /* Active.Available */

+/* Resizable BARs CTRL */
+#define PCI_REBAR_CTRL         8       /* control register */
+#define  PCI_REBAR_CTRL_BAR_IDX                0x00000007  /* BAR index */
+#define  PCI_REBAR_CTRL_NBAR_MASK      0x000000E0  /* # of resizable BARs */
+#define  PCI_REBAR_CTRL_NBAR_SHIFT     5           /* shift for # of BARs */
+
 #endif /* LINUX_PCI_REGS_H */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 45ebc1bb3356..d1d6e182da75 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -112,6 +112,8 @@ struct vpci {
             bool prefetchable : 1;
             /* Store whether the BAR is mapped into guest p2m. */
             bool enabled      : 1;
+            /* Bar Resizable. */
+            bool resizable    : 1;
         } bars[PCI_HEADER_NORMAL_NR_BARS + 1];
         /* At most 6 BARS + 1 expansion ROM BAR. */

@@ -129,8 +131,6 @@ 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;


> 
>> And I have one question: where does your patch do writing the resizing size 
>> into hardware?
> 
> dom0 has unrestricted access to the resize capability, so the value
> written by dom0 is propagated to the hardware without modification.
> 
> I would be wary of exposing the resize capability to untrusted
> domains, as allowing a domU to change the size of BARs can lead to
> overlapping if the hardware domain hasn't accounted for the increase
> in BAR size.
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Attachment: rebar_dmesg.txt
Description: rebar_dmesg.txt

Attachment: rebar_xl_dmesg.txt
Description: rebar_xl_dmesg.txt