[PATCH v10 0/4] Support hiding capability when its initialization fails

[PATCH v10 0/4] Support hiding capability when its initialization fails

[Jiqian Chen]

Hi,

This series is to
emulate extended capability list for dom0;
hide legacy and extended capability when its initialization fails;
above two parts had been merged.
remove all related registers and other resources when initializing capability fails, including patch #1, #2, #3, #4.

Best regards,
Jiqian Chen.
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
---
Jiqian Chen (4):
  vpci: Refactor vpci_remove_register to remove matched registers
  vpci/rebar: Free Rebar resources when init_rebar() fails
  vpci/msi: Free MSI resources when init_msi() fails
  vpci/msix: Free MSIX resources when init_msix() fails

 tools/tests/vpci/main.c  |  8 ++---
 xen/drivers/vpci/msi.c   | 46 ++++++++++++++++++++++++++-
 xen/drivers/vpci/msix.c  | 44 +++++++++++++++++++++++++-
 xen/drivers/vpci/rebar.c | 41 +++++++++++++++++-------
 xen/drivers/vpci/vpci.c  | 68 ++++++++++++++++++++++++----------------
 xen/include/xen/vpci.h   |  4 +--
 6 files changed, 165 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH v10 1/4] vpci: Refactor vpci_remove_register to remove matched registers

[Jiqian Chen]

vpci_remove_register() only supports removing a register in a time,
but the follow-on changes need to remove all registers within a range.
So, refactor it to support removing all registers in a given region.

And it is no issue to remove a non exist register, so remove the
__must_check prefix.

Note: two test cases don't math the new logic of
vpci_remove_registers(), then modify them.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
---
v9->v10 changes:
* Since logic change of vpci_remove_registers() affect the test cases of
  tools/tests/vpci/main.c, change them to match the new function logic.
* Remove Roger's Reviewed-by since patch changed.

v8->v9 changes:
v7->v8 changes:
v6->v7 changes:
No.

v5->v6 changes:
* Modify commit message.
* Add Roger's Reviewed-by.

v4->v5 changes:
No.

v3->v4 changes:
* Use list_for_each_entry_safe instead of list_for_each_entry.
* Return ERANGE if overlap.

v2->v3 changes:
* Add new check to return error if registers overlap but not inside range.

v1->v2 changes:
new patch

Best regards,
Jiqian Chen.
---
 tools/tests/vpci/main.c |  8 ++++----
 xen/drivers/vpci/vpci.c | 38 ++++++++++++++++++++------------------
 xen/include/xen/vpci.h  |  4 ++--
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/tools/tests/vpci/main.c b/tools/tests/vpci/main.c
index 33223db3eb77..2ef8d4e03f1d 100644
--- a/tools/tests/vpci/main.c
+++ b/tools/tests/vpci/main.c
@@ -132,10 +132,10 @@ static void vpci_write32_mask(const struct pci_dev *pdev, unsigned int reg,
                                   rsvdz_mask))
 
 #define VPCI_REMOVE_REG(off, size)                                          \
-    assert(!vpci_remove_register(test_pdev.vpci, off, size))
+    assert(!vpci_remove_registers(test_pdev.vpci, off, size))
 
 #define VPCI_REMOVE_INVALID_REG(off, size)                                  \
-    assert(vpci_remove_register(test_pdev.vpci, off, size))
+    assert(vpci_remove_registers(test_pdev.vpci, off, size))
 
 /* Read a 32b register using all possible sizes. */
 void multiread4_check(unsigned int reg, uint32_t val)
@@ -402,10 +402,10 @@ main(int argc, char **argv)
     VPCI_REMOVE_REG(28, 1);
     VPCI_REMOVE_REG(24, 4);
     VPCI_REMOVE_REG(12, 2);
+    VPCI_REMOVE_REG(16, 2);
+    VPCI_REMOVE_REG(30, 2);
 
     VPCI_REMOVE_INVALID_REG(20, 1);
-    VPCI_REMOVE_INVALID_REG(16, 2);
-    VPCI_REMOVE_INVALID_REG(30, 2);
 
     return 0;
 }
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index fd02718b47ea..91d40be5bc4c 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -152,7 +152,7 @@ static int vpci_capability_hide(const struct pci_dev *pdev, unsigned int cap)
 
     prev_r->private = next_r->private;
     /*
-     * Not calling vpci_remove_register() here is to avoid redoing
+     * Not calling vpci_remove_registers() here is to avoid redoing
      * the register search.
      */
     list_del(&next_r->node);
@@ -160,7 +160,7 @@ static int vpci_capability_hide(const struct pci_dev *pdev, unsigned int cap)
     xfree(next_r);
 
     if ( !is_hardware_domain(pdev->domain) )
-        return vpci_remove_register(vpci, offset + PCI_CAP_LIST_ID, 1);
+        return vpci_remove_registers(vpci, offset + PCI_CAP_LIST_ID, 1);
 
     return 0;
 }
@@ -549,34 +549,36 @@ int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
     return 0;
 }
 
-int vpci_remove_register(struct vpci *vpci, unsigned int offset,
-                         unsigned int size)
+int vpci_remove_registers(struct vpci *vpci, unsigned int start,
+                          unsigned int size)
 {
-    const struct vpci_register r = { .offset = offset, .size = size };
-    struct vpci_register *rm;
+    struct vpci_register *rm, *tmp;
+    unsigned int end = start + size;
 
     spin_lock(&vpci->lock);
-    list_for_each_entry ( rm, &vpci->handlers, node )
+    list_for_each_entry_safe ( rm, tmp, &vpci->handlers, node )
     {
-        int cmp = vpci_register_cmp(&r, rm);
-
-        /*
-         * NB: do not use a switch so that we can use break to
-         * get out of the list loop earlier if required.
-         */
-        if ( !cmp && rm->offset == offset && rm->size == size )
+        /* Remove rm if rm is inside the range. */
+        if ( rm->offset >= start && rm->offset + rm->size <= end )
         {
             list_del(&rm->node);
-            spin_unlock(&vpci->lock);
             xfree(rm);
-            return 0;
+            continue;
         }
-        if ( cmp <= 0 )
+
+        /* Return error if registers overlap but not inside. */
+        if ( rm->offset + rm->size > start && rm->offset < end )
+        {
+            spin_unlock(&vpci->lock);
+            return -ERANGE;
+        }
+
+        if ( start < rm->offset )
             break;
     }
     spin_unlock(&vpci->lock);
 
-    return -ENOENT;
+    return 0;
 }
 
 /* Wrappers for performing reads/writes to the underlying hardware. */
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 17cfecb0aabf..514a0ce39133 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -70,8 +70,8 @@ static inline int __must_check vpci_add_register(struct vpci *vpci,
                                   size, data, 0, 0, 0, 0);
 }
 
-int __must_check vpci_remove_register(struct vpci *vpci, unsigned int offset,
-                                      unsigned int size);
+int vpci_remove_registers(struct vpci *vpci, unsigned int start,
+                          unsigned int size);
 
 /* Generic read/write handlers for the PCI config space. */
 uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size);
-- 
2.34.1

[PATCH v10 2/4] vpci/rebar: Free Rebar resources when init_rebar() fails

[Jiqian Chen]

When init_rebar() fails, current logic return fail and free Rebar-related
resources in vpci_deassign_device(). But the previous new changes will
hide Rebar capability and return success, it can't reach
vpci_deassign_device() to remove resources if hiding success, so those
resources must be removed in cleanup function of Rebar.

To do that, implement cleanup function for Rebar.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v9->v10 changes:
v8->v9 changes:
No.

v7->v8 changes:
* Add Roger's Reviewed-by.

v6->v7 changes:
* Change the pointer parameter of cleanup_rebar() to be const.
* Print error when vpci_remove_registers() fail in cleanup_rebar().

v5->v6 changes:
No.

v4->v5 changes:
* Change definition "static void cleanup_rebar" to "static int cf_check cleanup_rebar" since cleanup hook is changed to be int.

v3->v4 changes:
* Change function name from fini_rebar() to cleanup_rebar().
* Change the error number to be E2BIG and ENXIO in init_rebar().

v2->v3 changes:
* Use fini_rebar() to remove all register instead of in the failure path of init_rebar();

v1->v2 changes:
* Called vpci_remove_registers() to remove all possible registered registers instead of using a array to record all registered register.

Best regards,
Jiqian Chen.
---
 xen/drivers/vpci/rebar.c | 41 +++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
index 3c18792d9bcd..39ab9c2eb7d9 100644
--- a/xen/drivers/vpci/rebar.c
+++ b/xen/drivers/vpci/rebar.c
@@ -49,6 +49,32 @@ static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
     bar->guest_addr = bar->addr;
 }
 
+static int cf_check cleanup_rebar(const struct pci_dev *pdev)
+{
+    int rc;
+    uint32_t ctrl;
+    unsigned int nbars;
+    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
+                                                        PCI_EXT_CAP_ID_REBAR);
+
+    if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
+    {
+        ASSERT_UNREACHABLE();
+        return 0;
+    }
+
+    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
+    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
+
+    rc = vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
+                               PCI_REBAR_CTRL(nbars - 1));
+    if ( rc )
+        printk(XENLOG_ERR "%pd %pp: fail to remove Rebar handlers rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+
+    return rc;
+}
+
 static int cf_check init_rebar(struct pci_dev *pdev)
 {
     uint32_t ctrl;
@@ -80,7 +106,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
         {
             printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
                    pdev->domain, &pdev->sbdf, index);
-            continue;
+            return -E2BIG;
         }
 
         bar = &pdev->vpci->header.bars[index];
@@ -88,7 +114,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
         {
             printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
                    pdev->domain, &pdev->sbdf, index);
-            continue;
+            return -ENXIO;
         }
 
         rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
@@ -97,14 +123,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
         {
             printk(XENLOG_ERR "%pd %pp: BAR%u fail to add reg of REBAR_CTRL rc=%d\n",
                    pdev->domain, &pdev->sbdf, index, rc);
-            /*
-             * Ideally we would hide the ReBar capability on error, but code
-             * for doing so still needs to be written. Use continue instead
-             * to keep any already setup register hooks, as returning an
-             * error will cause the hardware domain to get unmediated access
-             * to all device registers.
-             */
-            continue;
+            return rc;
         }
 
         bar->resizable_sizes =
@@ -118,7 +137,7 @@ static int cf_check init_rebar(struct pci_dev *pdev)
 
     return 0;
 }
-REGISTER_VPCI_EXTCAP(REBAR, init_rebar, NULL);
+REGISTER_VPCI_EXTCAP(REBAR, init_rebar, cleanup_rebar);
 
 /*
  * Local variables:
-- 
2.34.1

[PATCH v10 3/4] vpci/msi: Free MSI resources when init_msi() fails

[Jiqian Chen]

When init_msi() fails, current logic return fail and free MSI-related
resources in vpci_deassign_device(). But the previous new changes will
hide MSI capability and return success, it can't reach
vpci_deassign_device() to remove resources if hiding success, so those
resources must be removed in cleanup function of MSI.

To do that, implement cleanup function for MSI.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v9->v10 changes:
No.

v8->v9 changes:
* Add Roger's Reviewed-by.

v7->v8 changes:
* Add a comment to describe why "-2" in cleanup_msi().
* Given the code in vpci_remove_registers() an error in the removal of
  registers would likely imply memory corruption, at which point it's
  best to fully disable the device. So, Rollback the last two modifications of v7.

v6->v7 changes:
* Change the pointer parameter of cleanup_msi() to be const.
* When vpci_remove_registers() in cleanup_msi() fails, not to return
  directly, instead try to free msi and re-add ctrl handler.
* Pass pdev->vpci into vpci_add_register() instead of pdev->vpci->msi in
  init_msi() since we need that every handler realize that msi is NULL
  when msi is free but handlers are still in there.

v5->v6 changes:
No.

v4->v5 changes:
* Change definition "static void cleanup_msi" to "static int cf_check cleanup_msi"
  since cleanup hook is changed to be int.
* Add a read-only register for MSI Control Register in the end of cleanup_msi.

v3->v4 changes:
* Change function name from fini_msi() to cleanup_msi().
* Remove unnecessary comment.
* Change to use XFREE to free vpci->msi.

v2->v3 changes:
* Remove all fail path, and use fini_msi() hook instead.
* Change the method to calculating the size of msi registers.

v1->v2 changes:
* Added a new function fini_msi to free all MSI resources instead of using an array
  to record registered registers.

Best regards,
Jiqian Chen.
---
 xen/drivers/vpci/msi.c | 46 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index c3eba4e14870..ad5138c4cb5e 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -193,6 +193,50 @@ static void cf_check mask_write(
     msi->mask = val;
 }
 
+static int cf_check cleanup_msi(const struct pci_dev *pdev)
+{
+    int rc;
+    unsigned int end;
+    struct vpci *vpci = pdev->vpci;
+    const unsigned int msi_pos = pdev->msi_pos;
+    const unsigned int ctrl = msi_control_reg(msi_pos);
+
+    if ( !msi_pos || !vpci->msi )
+        return 0;
+
+    if ( vpci->msi->masking )
+        end = msi_pending_bits_reg(msi_pos, vpci->msi->address64);
+    else
+        /*
+         * "-2" here is to cut the reserved 2 bytes of Message Data when
+         * there is no masking support.
+         */
+        end = msi_mask_bits_reg(msi_pos, vpci->msi->address64) - 2;
+
+    rc = vpci_remove_registers(vpci, ctrl, end - ctrl);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "%pd %pp: fail to remove MSI handlers rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+        ASSERT_UNREACHABLE();
+        return rc;
+    }
+
+    XFREE(vpci->msi);
+
+    /*
+     * The driver may not traverse the capability list and think device
+     * supports MSI by default. So here let the control register of MSI
+     * be Read-Only is to ensure MSI disabled.
+     */
+    rc = vpci_add_register(vpci, vpci_hw_read16, NULL, ctrl, 2, NULL);
+    if ( rc )
+        printk(XENLOG_ERR "%pd %pp: fail to add MSI ctrl handler rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+
+    return rc;
+}
+
 static int cf_check init_msi(struct pci_dev *pdev)
 {
     unsigned int pos = pdev->msi_pos;
@@ -270,7 +314,7 @@ static int cf_check init_msi(struct pci_dev *pdev)
 
     return 0;
 }
-REGISTER_VPCI_CAP(MSI, init_msi, NULL);
+REGISTER_VPCI_CAP(MSI, init_msi, cleanup_msi);
 
 void vpci_dump_msi(void)
 {
-- 
2.34.1

[PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jiqian Chen]

When MSI-X initialization fails vPCI will hide the capability, but
remove of handlers and data won't be performed until the device is
deassigned.  Introduce a MSI-X cleanup hook that will be called when
initialization fails to cleanup MSI-X related hooks and free it's
associated data.

As all supported capabilities have been switched to use the cleanup
hooks call those from vpci_deassign_device() instead of open-code the
capability specific cleanup in there.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v9->v10 changes:
* Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().

v8->v9 changes:
* Modify commit message.
* Call cleanup_msix() in vpci_deassign_device() to remove the open-code to cleanup msix datas.
* In cleanup_msix(), move "list_del(&vpci->msix->next);" above for loop of iounmap msix tables.

v7->v8 changes:
* Given the code in vpci_remove_registers() an error in the removal of
  registers would likely imply memory corruption, at which point it's
  best to fully disable the device. So, Rollback the last two modifications of v7.

v6->v7 changes:
* Change the pointer parameter of cleanup_msix() to be const.
* When vpci_remove_registers() in cleanup_msix() fails, not to return
  directly, instead try to free msix and re-add ctrl handler.
* Pass pdev->vpci into vpci_add_register() instead of pdev->vpci->msix in
  init_msix() since we need that every handler realize that msix is NULL
  when msix is freed but handlers are still in there.

v5->v6 changes:
* Change the logic to add dummy handler when !vpci->msix in cleanup_msix().

v4->v5 changes:
* Change definition "static void cleanup_msix" to "static int cf_check cleanup_msix"
  since cleanup hook is changed to be int.
* Add a read-only register for MSIX Control Register in the end of cleanup_msix().

v3->v4 changes:
* Change function name from fini_msix() to cleanup_msix().
* Change to use XFREE to free vpci->msix.
* In cleanup function, change the sequence of check and remove action according to
  init_msix().

v2->v3 changes:
* Remove unnecessary clean operations in fini_msix().

v1->v2 changes:
new patch.

Best regards,
Jiqian Chen.
---
 xen/drivers/vpci/msix.c | 44 ++++++++++++++++++++++++++++++++++++++++-
 xen/drivers/vpci/vpci.c | 30 +++++++++++++++++++---------
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 54a5070733aa..3aa271445d12 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
+static int cf_check cleanup_msix(const struct pci_dev *pdev)
+{
+    int rc;
+    struct vpci *vpci = pdev->vpci;
+    const unsigned int msix_pos = pdev->msix_pos;
+
+    if ( !msix_pos )
+        return 0;
+
+    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+        ASSERT_UNREACHABLE();
+        return rc;
+    }
+
+    if ( vpci->msix )
+    {
+        list_del(&vpci->msix->next);
+        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
+            if ( vpci->msix->table[i] )
+                iounmap(vpci->msix->table[i]);
+
+        XFREE(vpci->msix);
+    }
+
+    /*
+     * The driver may not traverse the capability list and think device
+     * supports MSIX by default. So here let the control register of MSIX
+     * be Read-Only is to ensure MSIX disabled.
+     */
+    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
+                           msix_control_reg(msix_pos), 2, NULL);
+    if ( rc )
+        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+
+    return rc;
+}
+
 static int cf_check init_msix(struct pci_dev *pdev)
 {
     struct domain *d = pdev->domain;
@@ -710,7 +752,7 @@ static int cf_check init_msix(struct pci_dev *pdev)
      */
     return vpci_make_msix_hole(pdev);
 }
-REGISTER_VPCI_CAP(MSIX, init_msix, NULL);
+REGISTER_VPCI_CAP(MSIX, init_msix, cleanup_msix);
 
 /*
  * Local variables:
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 91d40be5bc4c..ebc4fdcd2600 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
                     &pdev->domain->vpci_dev_assigned_map);
 #endif
 
+    for ( i = 0; i < NUM_VPCI_INIT; i++ )
+    {
+        const vpci_capability_t *capability = &__start_vpci_array[i];
+        const unsigned int cap = capability->id;
+        unsigned int pos = 0;
+
+        if ( !capability->is_ext )
+            pos = pci_find_cap_offset(pdev->sbdf, cap);
+        else if ( is_hardware_domain(pdev->domain) )
+            pos = pci_find_ext_capability(pdev->sbdf, cap);
+
+        if ( pos && capability->cleanup )
+        {
+            int rc = capability->cleanup(pdev);
+            if ( rc )
+                printk(XENLOG_ERR "%pd %pp: clean %s cap %u fail rc=%d\n",
+                       pdev->domain, &pdev->sbdf,
+                       capability->is_ext ? "extended" : "legacy", cap, rc);
+        }
+    }
+
     spin_lock(&pdev->vpci->lock);
     while ( !list_empty(&pdev->vpci->handlers) )
     {
@@ -332,19 +353,10 @@ void vpci_deassign_device(struct pci_dev *pdev)
         xfree(r);
     }
     spin_unlock(&pdev->vpci->lock);
-    if ( pdev->vpci->msix )
-    {
-        list_del(&pdev->vpci->msix->next);
-        for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->table); i++ )
-            if ( pdev->vpci->msix->table[i] )
-                iounmap(pdev->vpci->msix->table[i]);
-    }
 
     for ( i = 0; i < ARRAY_SIZE(pdev->vpci->header.bars); i++ )
         rangeset_destroy(pdev->vpci->header.bars[i].mem);
 
-    xfree(pdev->vpci->msix);
-    xfree(pdev->vpci->msi);
     xfree(pdev->vpci);
     pdev->vpci = NULL;
 }
-- 
2.34.1

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jan Beulich]

On 05.08.2025 05:49, Jiqian Chen wrote:
> When MSI-X initialization fails vPCI will hide the capability, but
> remove of handlers and data won't be performed until the device is
> deassigned.  Introduce a MSI-X cleanup hook that will be called when
> initialization fails to cleanup MSI-X related hooks and free it's
> associated data.
> 
> As all supported capabilities have been switched to use the cleanup
> hooks call those from vpci_deassign_device() instead of open-code the
> capability specific cleanup in there.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v9->v10 changes:
> * Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().

Isn't this rather an omission in an earlier change, and hence may want to
come separately and with a Fixes: tag?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
>                      &pdev->domain->vpci_dev_assigned_map);
>  #endif
>  
> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
> +    {
> +        const vpci_capability_t *capability = &__start_vpci_array[i];
> +        const unsigned int cap = capability->id;
> +        unsigned int pos = 0;
> +
> +        if ( !capability->is_ext )
> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
> +        else if ( is_hardware_domain(pdev->domain) )
> +            pos = pci_find_ext_capability(pdev->sbdf, cap);

What's the point of doing this when ...

> +        if ( pos && capability->cleanup )

... ->cleanup is NULL? Don't you want to have

        if ( !capability->cleanup )
            continue;

earlier on?

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Chen, Jiqian]

On 2025/8/5 16:10, Jan Beulich wrote:
> On 05.08.2025 05:49, Jiqian Chen wrote:
>> When MSI-X initialization fails vPCI will hide the capability, but
>> remove of handlers and data won't be performed until the device is
>> deassigned.  Introduce a MSI-X cleanup hook that will be called when
>> initialization fails to cleanup MSI-X related hooks and free it's
>> associated data.
>>
>> As all supported capabilities have been switched to use the cleanup
>> hooks call those from vpci_deassign_device() instead of open-code the
>> capability specific cleanup in there.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v9->v10 changes:
>> * Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().
> 
> Isn't this rather an omission in an earlier change, and hence may want to
> come separately and with a Fixes: tag?
This is not really an omission, after all, all the cleanup hooks were implemented at the end of this series.
And judging from the commit message(which was written by Roger in v8), Roger also agreed to add them in this patch.

> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>                      &pdev->domain->vpci_dev_assigned_map);
>>  #endif
>>  
>> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> +    {
>> +        const vpci_capability_t *capability = &__start_vpci_array[i];
>> +        const unsigned int cap = capability->id;
>> +        unsigned int pos = 0;
>> +
>> +        if ( !capability->is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>> +        else if ( is_hardware_domain(pdev->domain) )
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
> 
> What's the point of doing this when ...
> 
>> +        if ( pos && capability->cleanup )
> 
> ... ->cleanup is NULL? Don't you want to have
> 
>         if ( !capability->cleanup )
>             continue;
> 
> earlier on?
Will change in next version.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jan Beulich]

On 05.08.2025 10:27, Chen, Jiqian wrote:
> On 2025/8/5 16:10, Jan Beulich wrote:
>> On 05.08.2025 05:49, Jiqian Chen wrote:
>>> When MSI-X initialization fails vPCI will hide the capability, but
>>> remove of handlers and data won't be performed until the device is
>>> deassigned.  Introduce a MSI-X cleanup hook that will be called when
>>> initialization fails to cleanup MSI-X related hooks and free it's
>>> associated data.
>>>
>>> As all supported capabilities have been switched to use the cleanup
>>> hooks call those from vpci_deassign_device() instead of open-code the
>>> capability specific cleanup in there.
>>>
>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>>> ---
>>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>>> ---
>>> v9->v10 changes:
>>> * Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().
>>
>> Isn't this rather an omission in an earlier change, and hence may want to
>> come separately and with a Fixes: tag?
> This is not really an omission, after all, all the cleanup hooks were implemented at the end of this series.
> And judging from the commit message(which was written by Roger in v8), Roger also agreed to add them in this patch.

I disagree. Of the two xfree()-s you remove here from vpci_deassign_device(),
one should have been removed by patch 3 already. Which would require the
part of the patch here to be put in place earlier on.

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jan Beulich]

On 05.08.2025 05:49, Jiqian Chen wrote:
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
> +{
> +    int rc;
> +    struct vpci *vpci = pdev->vpci;
> +    const unsigned int msix_pos = pdev->msix_pos;
> +
> +    if ( !msix_pos )
> +        return 0;
> +
> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
> +               pdev->domain, &pdev->sbdf, rc);
> +        ASSERT_UNREACHABLE();
> +        return rc;
> +    }
> +
> +    if ( vpci->msix )
> +    {
> +        list_del(&vpci->msix->next);
> +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
> +            if ( vpci->msix->table[i] )
> +                iounmap(vpci->msix->table[i]);
> +
> +        XFREE(vpci->msix);
> +    }
> +
> +    /*
> +     * The driver may not traverse the capability list and think device
> +     * supports MSIX by default. So here let the control register of MSIX
> +     * be Read-Only is to ensure MSIX disabled.
> +     */
> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
> +                           msix_control_reg(msix_pos), 2, NULL);
> +    if ( rc )
> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
> +               pdev->domain, &pdev->sbdf, rc);

Here as well as for MSI: Wouldn't this better be limited to the init-failure
case? No point in adding a register hook (and possibly emitting a misleading
log message) when we're tearing down anyway. IOW I think the ->cleanup()
hook needs a boolean parameter, unless the distinction of the two cases can
be (reliably) inferred from some other property.

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
>                      &pdev->domain->vpci_dev_assigned_map);
>  #endif
>  
> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
> +    {
> +        const vpci_capability_t *capability = &__start_vpci_array[i];
> +        const unsigned int cap = capability->id;
> +        unsigned int pos = 0;
> +
> +        if ( !capability->is_ext )
> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
> +        else if ( is_hardware_domain(pdev->domain) )
> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
> +
> +        if ( pos && capability->cleanup )
> +        {
> +            int rc = capability->cleanup(pdev);
> +            if ( rc )
> +                printk(XENLOG_ERR "%pd %pp: clean %s cap %u fail rc=%d\n",
> +                       pdev->domain, &pdev->sbdf,
> +                       capability->is_ext ? "extended" : "legacy", cap, rc);
> +        }
> +    }

With this imo the patch subject is now wrong, too.

Jan

Re: [PATCH v10 2/4] vpci/rebar: Free Rebar resources when init_rebar() fails

[Jan Beulich]

On 05.08.2025 05:49, Jiqian Chen wrote:
> --- a/xen/drivers/vpci/rebar.c
> +++ b/xen/drivers/vpci/rebar.c
> @@ -49,6 +49,32 @@ static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>      bar->guest_addr = bar->addr;
>  }
>  
> +static int cf_check cleanup_rebar(const struct pci_dev *pdev)
> +{
> +    int rc;
> +    uint32_t ctrl;
> +    unsigned int nbars;
> +    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
> +                                                        PCI_EXT_CAP_ID_REBAR);
> +
> +    if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return 0;
> +    }
> +
> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
> +
> +    rc = vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
> +                               PCI_REBAR_CTRL(nbars - 1));
> +    if ( rc )
> +        printk(XENLOG_ERR "%pd %pp: fail to remove Rebar handlers rc=%d\n",
> +               pdev->domain, &pdev->sbdf, rc);

MSI and MSI-X (now) have ASSERT_UNREACHABLE() on their respective paths. Is
there a reason this shouldn't be done here as well?

MSI and MSI-X further have another add-register below here, to ensure the
control register cannot be written. Again - is there a reason the same
shouldn't be done here? (If so, I think this may want putting in a comment.)

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Chen, Jiqian]

On 2025/8/5 16:43, Jan Beulich wrote:
> On 05.08.2025 05:49, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +    struct vpci *vpci = pdev->vpci;
>> +    const unsigned int msix_pos = pdev->msix_pos;
>> +
>> +    if ( !msix_pos )
>> +        return 0;
>> +
>> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
>> +    if ( rc )
>> +    {
>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
>> +               pdev->domain, &pdev->sbdf, rc);
>> +        ASSERT_UNREACHABLE();
>> +        return rc;
>> +    }
>> +
>> +    if ( vpci->msix )
>> +    {
>> +        list_del(&vpci->msix->next);
>> +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
>> +            if ( vpci->msix->table[i] )
>> +                iounmap(vpci->msix->table[i]);
>> +
>> +        XFREE(vpci->msix);
>> +    }
>> +
>> +    /*
>> +     * The driver may not traverse the capability list and think device
>> +     * supports MSIX by default. So here let the control register of MSIX
>> +     * be Read-Only is to ensure MSIX disabled.
>> +     */
>> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
>> +                           msix_control_reg(msix_pos), 2, NULL);
>> +    if ( rc )
>> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
>> +               pdev->domain, &pdev->sbdf, rc);
> 
> Here as well as for MSI: Wouldn't this better be limited to the init-failure
> case? No point in adding a register hook (and possibly emitting a misleading
> log message) when we're tearing down anyway. IOW I think the ->cleanup()
> hook needs a boolean parameter, unless the distinction of the two cases can
> be (reliably) inferred from some other property.
To make these changes, can I add a new patch as the last patch of this series?
And the new patch will do:
1. add a boolean parameter for cleanup hook to separate whose calls cleanup(during initialization or during deassign device).
2. call all cleanup hooks in vpci_deassign_device().
3. remove the MSI/MSIX specific free actions in vpci_deassign_device().

> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -321,6 +321,27 @@ void vpci_deassign_device(struct pci_dev *pdev)
>>                      &pdev->domain->vpci_dev_assigned_map);
>>  #endif
>>  
>> +    for ( i = 0; i < NUM_VPCI_INIT; i++ )
>> +    {
>> +        const vpci_capability_t *capability = &__start_vpci_array[i];
>> +        const unsigned int cap = capability->id;
>> +        unsigned int pos = 0;
>> +
>> +        if ( !capability->is_ext )
>> +            pos = pci_find_cap_offset(pdev->sbdf, cap);
>> +        else if ( is_hardware_domain(pdev->domain) )
>> +            pos = pci_find_ext_capability(pdev->sbdf, cap);
>> +
>> +        if ( pos && capability->cleanup )
>> +        {
>> +            int rc = capability->cleanup(pdev);
>> +            if ( rc )
>> +                printk(XENLOG_ERR "%pd %pp: clean %s cap %u fail rc=%d\n",
>> +                       pdev->domain, &pdev->sbdf,
>> +                       capability->is_ext ? "extended" : "legacy", cap, rc);
>> +        }
>> +    }
> 
> With this imo the patch subject is now wrong, too.
> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v10 2/4] vpci/rebar: Free Rebar resources when init_rebar() fails

[Chen, Jiqian]

On 2025/8/5 16:48, Jan Beulich wrote:
> On 05.08.2025 05:49, Jiqian Chen wrote:
>> --- a/xen/drivers/vpci/rebar.c
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -49,6 +49,32 @@ static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>>      bar->guest_addr = bar->addr;
>>  }
>>  
>> +static int cf_check cleanup_rebar(const struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +    uint32_t ctrl;
>> +    unsigned int nbars;
>> +    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
>> +                                                        PCI_EXT_CAP_ID_REBAR);
>> +
>> +    if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        return 0;
>> +    }
>> +
>> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>> +
>> +    rc = vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
>> +                               PCI_REBAR_CTRL(nbars - 1));
>> +    if ( rc )
>> +        printk(XENLOG_ERR "%pd %pp: fail to remove Rebar handlers rc=%d\n",
>> +               pdev->domain, &pdev->sbdf, rc);
> 
> MSI and MSI-X (now) have ASSERT_UNREACHABLE() on their respective paths. Is
> there a reason this shouldn't be done here as well?
Will add.

> 
> MSI and MSI-X further have another add-register below here, to ensure the
> control register cannot be written. Again - is there a reason the same
> shouldn't be done here? (If so, I think this may want putting in a comment.)
Since extended capabilities are only exposed to dom0, and dom0 has no limitations to access devices.
It since there is not much point in adding such a handler for rebar.

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v10 2/4] vpci/rebar: Free Rebar resources when init_rebar() fails

[Jan Beulich]

On 06.08.2025 05:39, Chen, Jiqian wrote:
> On 2025/8/5 16:48, Jan Beulich wrote:
>> On 05.08.2025 05:49, Jiqian Chen wrote:
>>> --- a/xen/drivers/vpci/rebar.c
>>> +++ b/xen/drivers/vpci/rebar.c
>>> @@ -49,6 +49,32 @@ static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>>>      bar->guest_addr = bar->addr;
>>>  }
>>>  
>>> +static int cf_check cleanup_rebar(const struct pci_dev *pdev)
>>> +{
>>> +    int rc;
>>> +    uint32_t ctrl;
>>> +    unsigned int nbars;
>>> +    unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
>>> +                                                        PCI_EXT_CAP_ID_REBAR);
>>> +
>>> +    if ( !rebar_offset || !is_hardware_domain(pdev->domain) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        return 0;
>>> +    }
>>> +
>>> +    ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>>> +    nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>> +
>>> +    rc = vpci_remove_registers(pdev->vpci, rebar_offset + PCI_REBAR_CAP(0),
>>> +                               PCI_REBAR_CTRL(nbars - 1));
>>> +    if ( rc )
>>> +        printk(XENLOG_ERR "%pd %pp: fail to remove Rebar handlers rc=%d\n",
>>> +               pdev->domain, &pdev->sbdf, rc);
>>
>> MSI and MSI-X (now) have ASSERT_UNREACHABLE() on their respective paths. Is
>> there a reason this shouldn't be done here as well?
> Will add.
> 
>>
>> MSI and MSI-X further have another add-register below here, to ensure the
>> control register cannot be written. Again - is there a reason the same
>> shouldn't be done here? (If so, I think this may want putting in a comment.)
> Since extended capabilities are only exposed to dom0, and dom0 has no limitations to access devices.
> It since there is not much point in adding such a handler for rebar.

While the effect is different from MSI / MSI-X, isn't it still a problem if
Dom0 changed BAR sizes entirely behind Xen's back? Which could be prevented
by similarly discarding writes to the control register, as is done for the
other two?

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jan Beulich]

On 06.08.2025 05:35, Chen, Jiqian wrote:
> On 2025/8/5 16:43, Jan Beulich wrote:
>> On 05.08.2025 05:49, Jiqian Chen wrote:
>>> --- a/xen/drivers/vpci/msix.c
>>> +++ b/xen/drivers/vpci/msix.c
>>> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>      return 0;
>>>  }
>>>  
>>> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
>>> +{
>>> +    int rc;
>>> +    struct vpci *vpci = pdev->vpci;
>>> +    const unsigned int msix_pos = pdev->msix_pos;
>>> +
>>> +    if ( !msix_pos )
>>> +        return 0;
>>> +
>>> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
>>> +    if ( rc )
>>> +    {
>>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
>>> +               pdev->domain, &pdev->sbdf, rc);
>>> +        ASSERT_UNREACHABLE();
>>> +        return rc;
>>> +    }
>>> +
>>> +    if ( vpci->msix )
>>> +    {
>>> +        list_del(&vpci->msix->next);
>>> +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
>>> +            if ( vpci->msix->table[i] )
>>> +                iounmap(vpci->msix->table[i]);
>>> +
>>> +        XFREE(vpci->msix);
>>> +    }
>>> +
>>> +    /*
>>> +     * The driver may not traverse the capability list and think device
>>> +     * supports MSIX by default. So here let the control register of MSIX
>>> +     * be Read-Only is to ensure MSIX disabled.
>>> +     */
>>> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
>>> +                           msix_control_reg(msix_pos), 2, NULL);
>>> +    if ( rc )
>>> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
>>> +               pdev->domain, &pdev->sbdf, rc);
>>
>> Here as well as for MSI: Wouldn't this better be limited to the init-failure
>> case? No point in adding a register hook (and possibly emitting a misleading
>> log message) when we're tearing down anyway. IOW I think the ->cleanup()
>> hook needs a boolean parameter, unless the distinction of the two cases can
>> be (reliably) inferred from some other property.
> To make these changes, can I add a new patch as the last patch of this series?
> And the new patch will do:
> 1. add a boolean parameter for cleanup hook to separate whose calls cleanup(during initialization or during deassign device).
> 2. call all cleanup hooks in vpci_deassign_device().
> 3. remove the MSI/MSIX specific free actions in vpci_deassign_device().

The outline looks okay, but imo it shouldn't be last in the series. Instead I
think it wants to come ahead of the last three patches; whether it's patch 1
or patch 2 doesn't really matter. Then (3) would be taken care of incrementally,
as ->cleanup hooks are added.

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Roger Pau Monné]

On Tue, Aug 05, 2025 at 10:40:00AM +0200, Jan Beulich wrote:
> On 05.08.2025 10:27, Chen, Jiqian wrote:
> > On 2025/8/5 16:10, Jan Beulich wrote:
> >> On 05.08.2025 05:49, Jiqian Chen wrote:
> >>> When MSI-X initialization fails vPCI will hide the capability, but
> >>> remove of handlers and data won't be performed until the device is
> >>> deassigned.  Introduce a MSI-X cleanup hook that will be called when
> >>> initialization fails to cleanup MSI-X related hooks and free it's
> >>> associated data.
> >>>
> >>> As all supported capabilities have been switched to use the cleanup
> >>> hooks call those from vpci_deassign_device() instead of open-code the
> >>> capability specific cleanup in there.
> >>>
> >>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> >>> ---
> >>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> >>> ---
> >>> v9->v10 changes:
> >>> * Call all cleanup hook in vpci_deassign_device() instead of cleanup_msix().
> >>
> >> Isn't this rather an omission in an earlier change, and hence may want to
> >> come separately and with a Fixes: tag?
> > This is not really an omission, after all, all the cleanup hooks were implemented at the end of this series.
> > And judging from the commit message(which was written by Roger in v8), Roger also agreed to add them in this patch.
> 
> I disagree. Of the two xfree()-s you remove here from vpci_deassign_device(),
> one should have been removed by patch 3 already. Which would require the
> part of the patch here to be put in place earlier on.

I don't think a Fixes tag is strictly required - the previous
functionality will not lead to malfunction, albeit it's best to use
the cleanup hooks introduced here.  Up until the hooks are executed as
part of vpci_deassign_device() the xfree() calls need to remain in
place.

Conceptually it would have been better to add the calls to the
->cleanup() hooks in vpci_deassign_device() in the same patch that
added the vpci_init_capabilities() ->init() and ->cleanup() logic.

It was my bad for noting this only when reviewing patch 8, and then
not asking for it to be placed in the right patch.

Thanks, Roger.

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Roger Pau Monné]

On Tue, Aug 05, 2025 at 10:43:09AM +0200, Jan Beulich wrote:
> On 05.08.2025 05:49, Jiqian Chen wrote:
> > --- a/xen/drivers/vpci/msix.c
> > +++ b/xen/drivers/vpci/msix.c
> > @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
> >      return 0;
> >  }
> >  
> > +static int cf_check cleanup_msix(const struct pci_dev *pdev)
> > +{
> > +    int rc;
> > +    struct vpci *vpci = pdev->vpci;
> > +    const unsigned int msix_pos = pdev->msix_pos;
> > +
> > +    if ( !msix_pos )
> > +        return 0;
> > +
> > +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> > +    if ( rc )
> > +    {
> > +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
> > +               pdev->domain, &pdev->sbdf, rc);
> > +        ASSERT_UNREACHABLE();
> > +        return rc;
> > +    }
> > +
> > +    if ( vpci->msix )
> > +    {
> > +        list_del(&vpci->msix->next);
> > +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
> > +            if ( vpci->msix->table[i] )
> > +                iounmap(vpci->msix->table[i]);
> > +
> > +        XFREE(vpci->msix);
> > +    }
> > +
> > +    /*
> > +     * The driver may not traverse the capability list and think device
> > +     * supports MSIX by default. So here let the control register of MSIX
> > +     * be Read-Only is to ensure MSIX disabled.
> > +     */
> > +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
> > +                           msix_control_reg(msix_pos), 2, NULL);
> > +    if ( rc )
> > +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
> > +               pdev->domain, &pdev->sbdf, rc);
> 
> Here as well as for MSI: Wouldn't this better be limited to the init-failure
> case? No point in adding a register hook (and possibly emitting a misleading
> log message) when we're tearing down anyway. IOW I think the ->cleanup()
> hook needs a boolean parameter, unless the distinction of the two cases can
> be (reliably) inferred from some other property.

I don't think we have any signal in pci_dev itself that notices
whether the device is being deassigned, in which case it does need an
extra boolean parameter to notice whether to add the r/o handler.

I'm also wondering whether we want to limit this hiding to the
hardware domain only, and for domUs fail the operation instead, and
fail to assign the device.  That can be adjusted in a later patch
though.

Thanks, Roger.

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Jan Beulich]

On 06.08.2025 10:22, Roger Pau Monné wrote:
> On Tue, Aug 05, 2025 at 10:43:09AM +0200, Jan Beulich wrote:
>> On 05.08.2025 05:49, Jiqian Chen wrote:
>>> --- a/xen/drivers/vpci/msix.c
>>> +++ b/xen/drivers/vpci/msix.c
>>> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>>      return 0;
>>>  }
>>>  
>>> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
>>> +{
>>> +    int rc;
>>> +    struct vpci *vpci = pdev->vpci;
>>> +    const unsigned int msix_pos = pdev->msix_pos;
>>> +
>>> +    if ( !msix_pos )
>>> +        return 0;
>>> +
>>> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
>>> +    if ( rc )
>>> +    {
>>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
>>> +               pdev->domain, &pdev->sbdf, rc);
>>> +        ASSERT_UNREACHABLE();
>>> +        return rc;
>>> +    }
>>> +
>>> +    if ( vpci->msix )
>>> +    {
>>> +        list_del(&vpci->msix->next);
>>> +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
>>> +            if ( vpci->msix->table[i] )
>>> +                iounmap(vpci->msix->table[i]);
>>> +
>>> +        XFREE(vpci->msix);
>>> +    }
>>> +
>>> +    /*
>>> +     * The driver may not traverse the capability list and think device
>>> +     * supports MSIX by default. So here let the control register of MSIX
>>> +     * be Read-Only is to ensure MSIX disabled.
>>> +     */
>>> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
>>> +                           msix_control_reg(msix_pos), 2, NULL);
>>> +    if ( rc )
>>> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
>>> +               pdev->domain, &pdev->sbdf, rc);
>>
>> Here as well as for MSI: Wouldn't this better be limited to the init-failure
>> case? No point in adding a register hook (and possibly emitting a misleading
>> log message) when we're tearing down anyway. IOW I think the ->cleanup()
>> hook needs a boolean parameter, unless the distinction of the two cases can
>> be (reliably) inferred from some other property.
> 
> I don't think we have any signal in pci_dev itself that notices
> whether the device is being deassigned, in which case it does need an
> extra boolean parameter to notice whether to add the r/o handler.
> 
> I'm also wondering whether we want to limit this hiding to the
> hardware domain only, and for domUs fail the operation instead, and
> fail to assign the device.  That can be adjusted in a later patch
> though.

Yes, DomU wants handling as you say. Iirc there are other open issues with
DomU support, though. Hence yes, "later" ought to suffice here. Perhaps
worth annotating with a fixme, though, to be able to easily spot all the
places that require adjustment.

Jan

Re: [PATCH v10 4/4] vpci/msix: Free MSIX resources when init_msix() fails

[Roger Pau Monné]

On Wed, Aug 06, 2025 at 10:30:53AM +0200, Jan Beulich wrote:
> On 06.08.2025 10:22, Roger Pau Monné wrote:
> > On Tue, Aug 05, 2025 at 10:43:09AM +0200, Jan Beulich wrote:
> >> On 05.08.2025 05:49, Jiqian Chen wrote:
> >>> --- a/xen/drivers/vpci/msix.c
> >>> +++ b/xen/drivers/vpci/msix.c
> >>> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
> >>> +{
> >>> +    int rc;
> >>> +    struct vpci *vpci = pdev->vpci;
> >>> +    const unsigned int msix_pos = pdev->msix_pos;
> >>> +
> >>> +    if ( !msix_pos )
> >>> +        return 0;
> >>> +
> >>> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> >>> +    if ( rc )
> >>> +    {
> >>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
> >>> +               pdev->domain, &pdev->sbdf, rc);
> >>> +        ASSERT_UNREACHABLE();
> >>> +        return rc;
> >>> +    }
> >>> +
> >>> +    if ( vpci->msix )
> >>> +    {
> >>> +        list_del(&vpci->msix->next);
> >>> +        for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
> >>> +            if ( vpci->msix->table[i] )
> >>> +                iounmap(vpci->msix->table[i]);
> >>> +
> >>> +        XFREE(vpci->msix);
> >>> +    }
> >>> +
> >>> +    /*
> >>> +     * The driver may not traverse the capability list and think device
> >>> +     * supports MSIX by default. So here let the control register of MSIX
> >>> +     * be Read-Only is to ensure MSIX disabled.
> >>> +     */
> >>> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
> >>> +                           msix_control_reg(msix_pos), 2, NULL);
> >>> +    if ( rc )
> >>> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler rc=%d\n",
> >>> +               pdev->domain, &pdev->sbdf, rc);
> >>
> >> Here as well as for MSI: Wouldn't this better be limited to the init-failure
> >> case? No point in adding a register hook (and possibly emitting a misleading
> >> log message) when we're tearing down anyway. IOW I think the ->cleanup()
> >> hook needs a boolean parameter, unless the distinction of the two cases can
> >> be (reliably) inferred from some other property.
> > 
> > I don't think we have any signal in pci_dev itself that notices
> > whether the device is being deassigned, in which case it does need an
> > extra boolean parameter to notice whether to add the r/o handler.
> > 
> > I'm also wondering whether we want to limit this hiding to the
> > hardware domain only, and for domUs fail the operation instead, and
> > fail to assign the device.  That can be adjusted in a later patch
> > though.
> 
> Yes, DomU wants handling as you say. Iirc there are other open issues with
> DomU support, though. Hence yes, "later" ought to suffice here. Perhaps
> worth annotating with a fixme, though, to be able to easily spot all the
> places that require adjustment.

Sometimes I don't take into account that vPCI is also supposed to be
used by domUs in the long run and forget about that aspect when
reviewing patches

Roger.