[PATCH v11 0/5] Support hiding capability when its initialization fails

[PATCH v11 0/5] 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, #5.

Best regards,
Jiqian Chen.
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
cc: Anthony PERARD <anthony.perard@vates.tech>
---
Jiqian Chen (5):
  vpci: Use cleanup to free capability resource during deassign
  vpci: Refactor vpci_remove_register to remove matched registers
  vpci/rebar: Implement cleanup function for Rebar
  vpci/msi: Implement cleanup function for MSI
  vpci/msix: Implement cleanup function for MSI-X

 tools/tests/vpci/main.c  |  8 ++---
 xen/drivers/vpci/msi.c   | 49 ++++++++++++++++++++++++++-
 xen/drivers/vpci/msix.c  | 47 +++++++++++++++++++++++++-
 xen/drivers/vpci/rebar.c | 66 ++++++++++++++++++++++++++++++------
 xen/drivers/vpci/vpci.c  | 72 ++++++++++++++++++++++++----------------
 xen/include/xen/vpci.h   |  6 ++--
 6 files changed, 200 insertions(+), 48 deletions(-)

-- 
2.34.1

[PATCH v11 1/5] vpci: Use cleanup to free capability resource during deassign

[Jiqian Chen]

As cleanup hook of all supported capabilities will be implemented in
follow-on changes, so to pre-call hook in vpci_deassign_device(), and
the capability specific clean open-code in there will be removed by
follow-on corresponding capability changes.

Since vpci_deassign_device() and vpci_init_capabilities() require
different cleanup actions, add a boolean parameter to cleanup hook
to distinguish them.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v10->v11 changes:
new patch.
---
 xen/drivers/vpci/vpci.c | 25 ++++++++++++++++++++++++-
 xen/include/xen/vpci.h  |  2 +-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index fd02718b47ea..120a919f08e3 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -280,7 +280,7 @@ static int vpci_init_capabilities(struct pci_dev *pdev)
 
             if ( capability->cleanup )
             {
-                rc = capability->cleanup(pdev);
+                rc = capability->cleanup(pdev, true);
                 if ( rc )
                 {
                     printk(XENLOG_ERR "%pd %pp: clean %s cap %u fail rc=%d\n",
@@ -321,6 +321,29 @@ 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->cleanup )
+            continue;
+
+        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 )
+        {
+            int rc = capability->cleanup(pdev, false);
+            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) )
     {
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 17cfecb0aabf..4b7b9298c4e8 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -17,7 +17,7 @@ typedef struct {
     unsigned int id;
     bool is_ext;
     int (* init)(struct pci_dev *pdev);
-    int (* cleanup)(const struct pci_dev *pdev);
+    int (* cleanup)(const struct pci_dev *pdev, bool hide);
 } vpci_capability_t;
 
 #define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
-- 
2.34.1

[PATCH v11 2/5] 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>
---
v10->v11 changes:
No.

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 120a919f08e3..6ecb70052b93 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;
 }
@@ -572,34 +572,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 4b7b9298c4e8..9ae75d946af4 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 v11 3/5] vpci/rebar: Implement cleanup function for Rebar

[Jiqian Chen]

When Rebar initialization fails, vPCI hides the capability, but
removing handlers and datas won't be performed until the device is
deassigned. So, implement Rebar cleanup hook that will be called to
cleanup Rebar related handlers and free it's associated data when
initialization fails.

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v10->v11 changes:
* Add ASSERT_UNREACHABLE() when vpci_remove_registers() fails
* When hide == true, add handlers to let Rebar ctrl be RO.
* Remove Roger's Reviewed-by since patch change.

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 | 66 +++++++++++++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
index 3c18792d9bcd..91d5369d75e2 100644
--- a/xen/drivers/vpci/rebar.c
+++ b/xen/drivers/vpci/rebar.c
@@ -49,6 +49,57 @@ 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, bool hide)
+{
+    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);
+        ASSERT_UNREACHABLE();
+        return rc;
+    }
+
+    if ( !hide )
+        return 0;
+
+    /*
+     * The driver may not traverse the capability list and think device
+     * supports Rebar by default. So here let the control register of Rebar
+     * be Read-Only is to ensure Rebar disabled.
+     */
+    for ( unsigned int i = 0; i < nbars; i++ )
+    {
+        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, NULL,
+                               rebar_offset + PCI_REBAR_CTRL(i), 4, NULL);
+        if ( rc )
+        {
+            printk(XENLOG_ERR
+                   "%pd %pp: fail to add Rebar ctrl handler rc=%d\n",
+                   pdev->domain, &pdev->sbdf, rc);
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
 static int cf_check init_rebar(struct pci_dev *pdev)
 {
     uint32_t ctrl;
@@ -80,7 +131,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 +139,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 +148,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 +162,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 v11 4/5] vpci/msi: Implement cleanup function for MSI

[Jiqian Chen]

When MSI initialization fails, vPCI hides the capability, but
removing handlers and datas won't be performed until the device is
deassigned. So, implement MSI cleanup hook that will be called to
cleanup MSI related handlers and free it's associated data when
initialization fails.

Since cleanup function of MSI is implemented, delete the open-code
in vpci_deassign_device().

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v10->v11 changes:
* Add hide paratemer to cleanup_msi().
* Check hide, if false return directly instead of letting ctrl RO.
* Delete xfree(pdev->vpci->msi); in vpci_deassign_device().
* Remove Roger's Reviewed-by since patch change.

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  | 49 ++++++++++++++++++++++++++++++++++++++++-
 xen/drivers/vpci/vpci.c |  1 -
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index c3eba4e14870..6ab45b9ba655 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -193,6 +193,53 @@ static void cf_check mask_write(
     msi->mask = val;
 }
 
+static int cf_check cleanup_msi(const struct pci_dev *pdev, bool hide)
+{
+    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);
+
+    if ( !hide )
+        return 0;
+
+    /*
+     * 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 +317,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)
 {
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 6ecb70052b93..3122847524d2 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -367,7 +367,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
         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

[PATCH v11 5/5] vpci/msix: Implement cleanup function for MSI-X

[Jiqian Chen]

When MSI-X initialization fails, vPCI hides the capability, but
removing handlers and datas won't be performed until the device is
deassigned. So, implement MSI-X cleanup hook that will be called
to cleanup MSI-X related handlers and free it's associated data when
initialization fails.

Since cleanup function of MSI-X is implemented, delete the open-code
in vpci_deassign_device().

Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
v10->v11 changes:
* Move calling all cleanup hook in vpci_deassign_device() out of this patch.
* Add hide parameter to cleanup_msix().
* Check hide, if it is false, return directly instead of letting ctrl RO.

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 | 47 ++++++++++++++++++++++++++++++++++++++++-
 xen/drivers/vpci/vpci.c |  8 -------
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 54a5070733aa..287aafda9157 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -655,6 +655,51 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
     return 0;
 }
 
+static int cf_check cleanup_msix(const struct pci_dev *pdev, bool hide)
+{
+    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);
+    }
+
+    if ( !hide )
+        return 0;
+
+    /*
+     * 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 +755,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 3122847524d2..394d75490db9 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -355,18 +355,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);
     pdev->vpci = NULL;
 }
-- 
2.34.1

Re: [PATCH v11 1/5] vpci: Use cleanup to free capability resource during deassign

[Jan Beulich]

On 08.08.2025 10:03, Jiqian Chen wrote:
> @@ -321,6 +321,29 @@ 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->cleanup )
> +            continue;
> +
> +        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 )
> +        {
> +            int rc = capability->cleanup(pdev, false);
> +            if ( rc )

Nit: Blank line between declaration(s) and statement(s) please. (Likely
easy enough to adjust while committing, if no other need for a v12
arises.)

Jan

Re: [PATCH v11 1/5] vpci: Use cleanup to free capability resource during deassign

[Chen, Jiqian]

On 2025/8/8 16:58, Jan Beulich wrote:
> On 08.08.2025 10:03, Jiqian Chen wrote:
>> @@ -321,6 +321,29 @@ 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->cleanup )
>> +            continue;
>> +
>> +        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 )
>> +        {
>> +            int rc = capability->cleanup(pdev, false);
>> +            if ( rc )
> 
> Nit: Blank line between declaration(s) and statement(s) please. (Likely
> easy enough to adjust while committing, if no other need for a v12
> arises.)
Thanks.
BTW, do I need for-4.21 flag if I expect this series to be merged before 4.21 release?

> 
> Jan

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 1/5] vpci: Use cleanup to free capability resource during deassign

[Jan Beulich]

On 11.08.2025 06:04, Chen, Jiqian wrote:
> On 2025/8/8 16:58, Jan Beulich wrote:
>> On 08.08.2025 10:03, Jiqian Chen wrote:
>>> @@ -321,6 +321,29 @@ 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->cleanup )
>>> +            continue;
>>> +
>>> +        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 )
>>> +        {
>>> +            int rc = capability->cleanup(pdev, false);
>>> +            if ( rc )
>>
>> Nit: Blank line between declaration(s) and statement(s) please. (Likely
>> easy enough to adjust while committing, if no other need for a v12
>> arises.)
> Thanks.
> BTW, do I need for-4.21 flag if I expect this series to be merged before 4.21 release?

I wouldn't say "need", but adding such a tag may now be advisable.

Jan

Re: [PATCH v11 1/5] vpci: Use cleanup to free capability resource during deassign

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:33PM +0800, Jiqian Chen wrote:
> As cleanup hook of all supported capabilities will be implemented in
> follow-on changes, so to pre-call hook in vpci_deassign_device(), and
> the capability specific clean open-code in there will be removed by
> follow-on corresponding capability changes.
> 
> Since vpci_deassign_device() and vpci_init_capabilities() require
> different cleanup actions, add a boolean parameter to cleanup hook
> to distinguish them.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>

I think it's missing:

Requested-by: Jan Beulich <jbeulich@suse.com>

With the newline below added:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I can do it on commit.

Thanks, Roger.

Re: [PATCH v11 2/5] vpci: Refactor vpci_remove_register to remove matched registers

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:34PM +0800, Jiqian Chen wrote:
> 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>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v11 3/5] vpci/rebar: Implement cleanup function for Rebar

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:35PM +0800, Jiqian Chen wrote:
> When Rebar initialization fails, vPCI hides the capability, but
> removing handlers and datas won't be performed until the device is
> deassigned. So, implement Rebar cleanup hook that will be called to
> cleanup Rebar related handlers and free it's associated data when
> initialization fails.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v10->v11 changes:
> * Add ASSERT_UNREACHABLE() when vpci_remove_registers() fails
> * When hide == true, add handlers to let Rebar ctrl be RO.
> * Remove Roger's Reviewed-by since patch change.
> 
> 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 | 66 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 55 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> index 3c18792d9bcd..91d5369d75e2 100644
> --- a/xen/drivers/vpci/rebar.c
> +++ b/xen/drivers/vpci/rebar.c
> @@ -49,6 +49,57 @@ 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, bool hide)
> +{
> +    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);
> +        ASSERT_UNREACHABLE();
> +        return rc;
> +    }
> +
> +    if ( !hide )
> +        return 0;

Now that the handler can differentiate between calls to hide the
capability versus calls from device deassign, do we need to call
vpci_remove_registers() for the non-hiding case?

The non-hiding case is only used from vpci_deassign_device(), and just
after having called all the cleanup hooks that function purges any
remaining registered handlers.  It would be OK to do something like:

static int cf_check cleanup_rebar(const struct pci_dev *pdev, bool hide)
{
    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;
    }

    if ( !hide )
        return 0;

    ... remove handler + mask register ...

Thoughts?

> +
> +    /*
> +     * The driver may not traverse the capability list and think device
> +     * supports Rebar by default. So here let the control register of Rebar
> +     * be Read-Only is to ensure Rebar disabled.
> +     */
> +    for ( unsigned int i = 0; i < nbars; i++ )
> +    {
> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, NULL,
> +                               rebar_offset + PCI_REBAR_CTRL(i), 4, NULL);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR
> +                   "%pd %pp: fail to add Rebar ctrl handler rc=%d\n",
> +                   pdev->domain, &pdev->sbdf, rc);
> +            return rc;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int cf_check init_rebar(struct pci_dev *pdev)
>  {
>      uint32_t ctrl;
> @@ -80,7 +131,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 +139,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;

I'm unsure we want to return an error here and in the check above,
given this capability is dom0 only, we might want to just skip the BAR
and continue, aiming for the other resizable BARs to be functional?

Thanks, Roger.

Re: [PATCH v11 4/5] vpci/msi: Implement cleanup function for MSI

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:36PM +0800, Jiqian Chen wrote:
> When MSI initialization fails, vPCI hides the capability, but
> removing handlers and datas won't be performed until the device is
> deassigned. So, implement MSI cleanup hook that will be called to
> cleanup MSI related handlers and free it's associated data when
> initialization fails.
> 
> Since cleanup function of MSI is implemented, delete the open-code
> in vpci_deassign_device().
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v10->v11 changes:
> * Add hide paratemer to cleanup_msi().
> * Check hide, if false return directly instead of letting ctrl RO.
> * Delete xfree(pdev->vpci->msi); in vpci_deassign_device().
> * Remove Roger's Reviewed-by since patch change.
> 
> 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  | 49 ++++++++++++++++++++++++++++++++++++++++-
>  xen/drivers/vpci/vpci.c |  1 -
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index c3eba4e14870..6ab45b9ba655 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -193,6 +193,53 @@ static void cf_check mask_write(
>      msi->mask = val;
>  }
>  
> +static int cf_check cleanup_msi(const struct pci_dev *pdev, bool hide)
> +{
> +    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);
> +
> +    if ( !hide )
> +        return 0;

Kind of the same comment as in the previous patch, for the non-hide
case there's likely no reason to perform the removal of the registers,
as the caller will take care of that.  The function could be
short-circuited earlier as:

if ( !hide )
{
    XFREE(vpci->msi);
    return 0;
}

Thanks, Roger.

Re: [PATCH v11 4/5] vpci/msi: Implement cleanup function for MSI

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:36PM +0800, Jiqian Chen wrote:
> When MSI initialization fails, vPCI hides the capability, but
> removing handlers and datas won't be performed until the device is
> deassigned. So, implement MSI cleanup hook that will be called to
> cleanup MSI related handlers and free it's associated data when
> initialization fails.
> 
> Since cleanup function of MSI is implemented, delete the open-code
> in vpci_deassign_device().
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v10->v11 changes:
> * Add hide paratemer to cleanup_msi().
> * Check hide, if false return directly instead of letting ctrl RO.
> * Delete xfree(pdev->vpci->msi); in vpci_deassign_device().
> * Remove Roger's Reviewed-by since patch change.
> 
> 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  | 49 ++++++++++++++++++++++++++++++++++++++++-
>  xen/drivers/vpci/vpci.c |  1 -
>  2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index c3eba4e14870..6ab45b9ba655 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -193,6 +193,53 @@ static void cf_check mask_write(
>      msi->mask = val;
>  }
>  
> +static int cf_check cleanup_msi(const struct pci_dev *pdev, bool hide)
> +{
> +    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;

I'm afraid the above is not correct, even if vpci->msi == NULL we
still want to hide the capability when requested to do so, that would
be the case if the memory alloc of vpci->msi fails in init_msi().

This should be:

if ( !msi_pos )
{
    ASSERT_UNREACHABLE();
    return 0;
}

if ( !hide )
{
    XFREE(vpci->msi);
    return 0;
}



> +
> +    if ( vpci->msi->masking )

You cannot assume that masking has been correctly set, depending on
where init_msi() fails masking will be uninitialized, same with
address64.

I think the logic would still be correct, because if ->masking and
->address64 is not initialized the register handlers won't be setup
either, but feels fragile.  Ideally cleanup_msi() shouldn't use the
contents of vpci->msi, because they are likely not properly
initialized.

Thanks, Roger.

Re: [PATCH v11 5/5] vpci/msix: Implement cleanup function for MSI-X

[Roger Pau Monné]

On Fri, Aug 08, 2025 at 04:03:37PM +0800, Jiqian Chen wrote:
> When MSI-X initialization fails, vPCI hides the capability, but
> removing handlers and datas won't be performed until the device is
> deassigned. So, implement MSI-X cleanup hook that will be called
> to cleanup MSI-X related handlers and free it's associated data when
> initialization fails.
> 
> Since cleanup function of MSI-X is implemented, delete the open-code
> in vpci_deassign_device().
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> v10->v11 changes:
> * Move calling all cleanup hook in vpci_deassign_device() out of this patch.
> * Add hide parameter to cleanup_msix().
> * Check hide, if it is false, return directly instead of letting ctrl RO.
> 
> 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 | 47 ++++++++++++++++++++++++++++++++++++++++-
>  xen/drivers/vpci/vpci.c |  8 -------
>  2 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 54a5070733aa..287aafda9157 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -655,6 +655,51 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>      return 0;
>  }
>  
> +static int cf_check cleanup_msix(const struct pci_dev *pdev, bool hide)
> +{
> +    int rc;
> +    struct vpci *vpci = pdev->vpci;
> +    const unsigned int msix_pos = pdev->msix_pos;
> +
> +    if ( !msix_pos )
> +        return 0;

Like with the MSI capability, is it possible to get called and
pdev->msix_pos be 0?

I would also avoid the call to vpci_remove_registers() if !hide, I
think you can change the order of the cleanup, so it's:

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);
}

if ( !hide )
    return 0;

rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
...

Thanks, Roger.

Re: [PATCH v11 3/5] vpci/rebar: Implement cleanup function for Rebar

[Chen, Jiqian]

On 2025/8/29 18:22, Roger Pau Monné wrote:
> On Fri, Aug 08, 2025 at 04:03:35PM +0800, Jiqian Chen wrote:
>> When Rebar initialization fails, vPCI hides the capability, but
>> removing handlers and datas won't be performed until the device is
>> deassigned. So, implement Rebar cleanup hook that will be called to
>> cleanup Rebar related handlers and free it's associated data when
>> initialization fails.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v10->v11 changes:
>> * Add ASSERT_UNREACHABLE() when vpci_remove_registers() fails
>> * When hide == true, add handlers to let Rebar ctrl be RO.
>> * Remove Roger's Reviewed-by since patch change.
>>
>> 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 | 66 +++++++++++++++++++++++++++++++++-------
>>  1 file changed, 55 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
>> index 3c18792d9bcd..91d5369d75e2 100644
>> --- a/xen/drivers/vpci/rebar.c
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -49,6 +49,57 @@ 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, bool hide)
>> +{
>> +    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);
>> +        ASSERT_UNREACHABLE();
>> +        return rc;
>> +    }
>> +
>> +    if ( !hide )
>> +        return 0;
> 
> Now that the handler can differentiate between calls to hide the
> capability versus calls from device deassign, do we need to call
> vpci_remove_registers() for the non-hiding case?
> 
> The non-hiding case is only used from vpci_deassign_device(), and just
> after having called all the cleanup hooks that function purges any
> remaining registered handlers.  It would be OK to do something like:
> 
> static int cf_check cleanup_rebar(const struct pci_dev *pdev, bool hide)
> {
>     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;
>     }
> 
>     if ( !hide )
>         return 0;
> 
>     ... remove handler + mask register ...
> 
> Thoughts?
Got it.
But why not moving it above the first check " if ( !rebar_offset || !is_hardware_domain(pdev->domain) )" ?

> 
>> +
>> +    /*
>> +     * The driver may not traverse the capability list and think device
>> +     * supports Rebar by default. So here let the control register of Rebar
>> +     * be Read-Only is to ensure Rebar disabled.
>> +     */
>> +    for ( unsigned int i = 0; i < nbars; i++ )
>> +    {
>> +        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, NULL,
>> +                               rebar_offset + PCI_REBAR_CTRL(i), 4, NULL);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR
>> +                   "%pd %pp: fail to add Rebar ctrl handler rc=%d\n",
>> +                   pdev->domain, &pdev->sbdf, rc);
>> +            return rc;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int cf_check init_rebar(struct pci_dev *pdev)
>>  {
>>      uint32_t ctrl;
>> @@ -80,7 +131,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 +139,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;
> 
> I'm unsure we want to return an error here and in the check above,
> given this capability is dom0 only, we might want to just skip the BAR
> and continue, aiming for the other resizable BARs to be functional?
Why here need to use continue, but below vpci_add_register() fail return error?

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 4/5] vpci/msi: Implement cleanup function for MSI

[Chen, Jiqian]

On 2025/8/29 18:41, Roger Pau Monné wrote:
> On Fri, Aug 08, 2025 at 04:03:36PM +0800, Jiqian Chen wrote:
>> When MSI initialization fails, vPCI hides the capability, but
>> removing handlers and datas won't be performed until the device is
>> deassigned. So, implement MSI cleanup hook that will be called to
>> cleanup MSI related handlers and free it's associated data when
>> initialization fails.
>>
>> Since cleanup function of MSI is implemented, delete the open-code
>> in vpci_deassign_device().
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v10->v11 changes:
>> * Add hide paratemer to cleanup_msi().
>> * Check hide, if false return directly instead of letting ctrl RO.
>> * Delete xfree(pdev->vpci->msi); in vpci_deassign_device().
>> * Remove Roger's Reviewed-by since patch change.
>>
>> 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  | 49 ++++++++++++++++++++++++++++++++++++++++-
>>  xen/drivers/vpci/vpci.c |  1 -
>>  2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index c3eba4e14870..6ab45b9ba655 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -193,6 +193,53 @@ static void cf_check mask_write(
>>      msi->mask = val;
>>  }
>>  
>> +static int cf_check cleanup_msi(const struct pci_dev *pdev, bool hide)
>> +{
>> +    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;
> 
> I'm afraid the above is not correct, even if vpci->msi == NULL we
> still want to hide the capability when requested to do so, that would
> be the case if the memory alloc of vpci->msi fails in init_msi().
> 
> This should be:
> 
> if ( !msi_pos )
> {
>     ASSERT_UNREACHABLE();
>     return 0;
> }
> 
> if ( !hide )
> {
>     XFREE(vpci->msi);
>     return 0;
> }
Will change.

> 
> 
> 
>> +
>> +    if ( vpci->msi->masking )
> 
> You cannot assume that masking has been correctly set, depending on
> where init_msi() fails masking will be uninitialized, same with
> address64.
> 
> I think the logic would still be correct, because if ->masking and
> ->address64 is not initialized the register handlers won't be setup
> either, but feels fragile.  Ideally cleanup_msi() shouldn't use the
> contents of vpci->msi, because they are likely not properly
> initialized.
Would it better to change to get mask and address64 info from hardware ctrl register of msi when vpci->msi is not NULL?

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 5/5] vpci/msix: Implement cleanup function for MSI-X

[Chen, Jiqian]

On 2025/8/29 19:06, Roger Pau Monné wrote:
> On Fri, Aug 08, 2025 at 04:03:37PM +0800, Jiqian Chen wrote:
>> When MSI-X initialization fails, vPCI hides the capability, but
>> removing handlers and datas won't be performed until the device is
>> deassigned. So, implement MSI-X cleanup hook that will be called
>> to cleanup MSI-X related handlers and free it's associated data when
>> initialization fails.
>>
>> Since cleanup function of MSI-X is implemented, delete the open-code
>> in vpci_deassign_device().
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@citrix.com>
>> ---
>> v10->v11 changes:
>> * Move calling all cleanup hook in vpci_deassign_device() out of this patch.
>> * Add hide parameter to cleanup_msix().
>> * Check hide, if it is false, return directly instead of letting ctrl RO.
>>
>> 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 | 47 ++++++++++++++++++++++++++++++++++++++++-
>>  xen/drivers/vpci/vpci.c |  8 -------
>>  2 files changed, 46 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 54a5070733aa..287aafda9157 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -655,6 +655,51 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +static int cf_check cleanup_msix(const struct pci_dev *pdev, bool hide)
>> +{
>> +    int rc;
>> +    struct vpci *vpci = pdev->vpci;
>> +    const unsigned int msix_pos = pdev->msix_pos;
>> +
>> +    if ( !msix_pos )
>> +        return 0;
> 
> Like with the MSI capability, is it possible to get called and
> pdev->msix_pos be 0?
Since currently only vpci_deassign_device and vpci_init_capabilities call cleanup hook,
and they both have check to prevent pos being 0, so if you think check here is redundancy, I will delete it in next version,
MSI and Rebar cleanup hook are the same as here.

> 
> I would also avoid the call to vpci_remove_registers() if !hide, I
> think you can change the order of the cleanup, so it's:
> 
> 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);
> }
> 
> if ( !hide )
>     return 0;
> 
> rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> ...
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.