[RESEND PATCH v12 0/3] Support hiding capability when its initialization fails

[RESEND PATCH v12 0/3] Support hiding capability when its initialization fails

[Jiqian Chen]

Hi,

Re-send to review since last send didn't get response and passed months.

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 Rebar #1, MSI #2, MSIX #3.

Best regards,
Jiqian Chen.
---
cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
Jiqian Chen (3):
  vpci/rebar: Implement cleanup function for Rebar
  vpci/msi: Implement cleanup function for MSI
  vpci/msix: Implement cleanup function for MSI-X

 xen/drivers/vpci/msi.c   | 55 ++++++++++++++++++++++++++++++++++++++-
 xen/drivers/vpci/msix.c  | 44 ++++++++++++++++++++++++++++++-
 xen/drivers/vpci/rebar.c | 56 +++++++++++++++++++++++++++++++++-------
 xen/drivers/vpci/vpci.c  |  9 -------
 4 files changed, 144 insertions(+), 20 deletions(-)

-- 
2.34.1

[RESEND PATCH v12 1/3] 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>
---
v11->v12 changes:
* In cleanup_rebar(), move the check "if ( !hide )" above the vpci_remove_registers().
* In init_rebar(), change return rc to continue when "if ( index >= PCI_HEADER_NORMAL_NR_BARS )" and
  "if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )"
* Remove "!rebar_offset" check in cleanup_rebar() since all currenty caller already do that.

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.

rebar
---
 xen/drivers/vpci/rebar.c | 56 +++++++++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
index 3c18792d9bcd..209091c00987 100644
--- a/xen/drivers/vpci/rebar.c
+++ b/xen/drivers/vpci/rebar.c
@@ -49,6 +49,51 @@ 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 ( !hide )
+        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;
+    }
+
+    /*
+     * 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;
@@ -97,14 +142,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 +156,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

Re: [RESEND PATCH v12 1/3] vpci/rebar: Implement cleanup function for Rebar

[Roger Pau Monné]

On Mon, Dec 08, 2025 at 04:18:13PM +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>

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

Thanks, Roger.

[RESEND PATCH v12 2/3] 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>
---
v11->v12 changes:
* In cleanup_msi(), move "if ( !hide )" above vpci_remove_registers()
  since deassign device will do removing registers itself.
* Read address64 and mask info from hardware since they are not reliable
  when init_msi fails.

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

diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index c3eba4e14870..181ec902dffb 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -193,6 +193,59 @@ 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 ( !hide )
+    {
+        XFREE(vpci->msi);
+        return 0;
+    }
+
+    if ( vpci->msi )
+    {
+        uint16_t control = pci_conf_read16(pdev->sbdf, ctrl);
+        bool address64 = is_64bit_address(control);
+
+        if ( is_mask_bit_support(control) )
+            end = msi_pending_bits_reg(msi_pos, 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, 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 +323,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 07c7071d0a17..7aaf015f63d4 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -368,7 +368,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

Re: [RESEND PATCH v12 2/3] vpci/msi: Implement cleanup function for MSI

[Roger Pau Monné]

On Mon, Dec 08, 2025 at 04:18:14PM +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>
> ---
> v11->v12 changes:
> * In cleanup_msi(), move "if ( !hide )" above vpci_remove_registers()
>   since deassign device will do removing registers itself.
> * Read address64 and mask info from hardware since they are not reliable
>   when init_msi fails.
> 
> 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  | 55 ++++++++++++++++++++++++++++++++++++++++-
>  xen/drivers/vpci/vpci.c |  1 -
>  2 files changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
> index c3eba4e14870..181ec902dffb 100644
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -193,6 +193,59 @@ 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;

Nit: I think you could narrow the scope of end and define it inside
the if ( vpci->msi ) { ... } block?

> +    struct vpci *vpci = pdev->vpci;
> +    const unsigned int msi_pos = pdev->msi_pos;
> +    const unsigned int ctrl = msi_control_reg(msi_pos);
> +
> +    if ( !hide )
> +    {
> +        XFREE(vpci->msi);
> +        return 0;
> +    }
> +
> +    if ( vpci->msi )
> +    {
> +        uint16_t control = pci_conf_read16(pdev->sbdf, ctrl);
> +        bool address64 = is_64bit_address(control);
> +
> +        if ( is_mask_bit_support(control) )
> +            end = msi_pending_bits_reg(msi_pos, 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, 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);

Strictly speaking (also in the previous patch), we only need to do
this hiding for the hardware domain.  For domUs access to the control
register would be ignored by default.

Would you be OK to add an:

if ( !is_hardware_domain(pdev->domain) )
    return 0;

Ahead of the call to add the vpci_hw_read16 trap register?

Thanks, Roger.

Re: [RESEND PATCH v12 2/3] vpci/msi: Implement cleanup function for MSI

[Chen, Jiqian]

On 2026/1/21 16:56, Roger Pau Monné wrote:
> On Mon, Dec 08, 2025 at 04:18:14PM +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>
>> ---
>> v11->v12 changes:
>> * In cleanup_msi(), move "if ( !hide )" above vpci_remove_registers()
>>   since deassign device will do removing registers itself.
>> * Read address64 and mask info from hardware since they are not reliable
>>   when init_msi fails.
>>
>> 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  | 55 ++++++++++++++++++++++++++++++++++++++++-
>>  xen/drivers/vpci/vpci.c |  1 -
>>  2 files changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index c3eba4e14870..181ec902dffb 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -193,6 +193,59 @@ 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;
> 
> Nit: I think you could narrow the scope of end and define it inside
> the if ( vpci->msi ) { ... } block?
Will change.

> 
>> +    struct vpci *vpci = pdev->vpci;
>> +    const unsigned int msi_pos = pdev->msi_pos;
>> +    const unsigned int ctrl = msi_control_reg(msi_pos);
>> +
>> +    if ( !hide )
>> +    {
>> +        XFREE(vpci->msi);
>> +        return 0;
>> +    }
>> +
>> +    if ( vpci->msi )
>> +    {
>> +        uint16_t control = pci_conf_read16(pdev->sbdf, ctrl);
>> +        bool address64 = is_64bit_address(control);
>> +
>> +        if ( is_mask_bit_support(control) )
>> +            end = msi_pending_bits_reg(msi_pos, 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, 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);
> 
> Strictly speaking (also in the previous patch), we only need to do
Extended capabilities are not expose for domUs currently, and all the places call cleanup_rebar already check "!is_hardware_domain(pdev->domain)", so rebar may not need this ?
msix.c needs this too, I think.

> this hiding for the hardware domain.  For domUs access to the control
> register would be ignored by default.
> 
> Would you be OK to add an:
> 
> if ( !is_hardware_domain(pdev->domain) )
>     return 0;
> 
> Ahead of the call to add the vpci_hw_read16 trap register?
OK, will change in next version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

[RESEND PATCH v12 3/3] 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>
---
v11->v12 changes:
* In cleanup_msix(), move check "if ( !hide )" above vpci_remove_registers().
* Remove the check "!pdev->msix_pos" since current callers already do that.

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

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 032e471bb1c0..8dcf2cf9d598 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -656,6 +656,48 @@ 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 ( 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);
+    if ( rc )
+    {
+        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+        ASSERT_UNREACHABLE();
+        return rc;
+    }
+
+    /*
+     * 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;
@@ -751,7 +793,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 7aaf015f63d4..3c9bebcbe977 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -356,18 +356,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: [RESEND PATCH v12 3/3] vpci/msix: Implement cleanup function for MSI-X

[Roger Pau Monné]

On Mon, Dec 08, 2025 at 04:18:15PM +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>
> ---
> v11->v12 changes:
> * In cleanup_msix(), move check "if ( !hide )" above vpci_remove_registers().
> * Remove the check "!pdev->msix_pos" since current callers already do that.
> 
> 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 | 44 ++++++++++++++++++++++++++++++++++++++++-
>  xen/drivers/vpci/vpci.c |  8 --------
>  2 files changed, 43 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 032e471bb1c0..8dcf2cf9d598 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -656,6 +656,48 @@ 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 ( 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);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
> +               pdev->domain, &pdev->sbdf, rc);
> +        ASSERT_UNREACHABLE();
> +        return rc;
> +    }
> +
> +    /*
> +     * 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);

Like the previous patch, I don't think this last bit is relevant for
domUs?  Only the hardware domain needs to have the control register
explicitly handled.

I don't mind adjusting at commit if we agree.

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

Thanks, Roger.

Re: [RESEND PATCH v12 3/3] vpci/msix: Implement cleanup function for MSI-X

[Chen, Jiqian]

On 2026/1/21 17:25, Roger Pau Monné wrote:
> On Mon, Dec 08, 2025 at 04:18:15PM +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>
>> ---
>> v11->v12 changes:
>> * In cleanup_msix(), move check "if ( !hide )" above vpci_remove_registers().
>> * Remove the check "!pdev->msix_pos" since current callers already do that.
>>
>> 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 | 44 ++++++++++++++++++++++++++++++++++++++++-
>>  xen/drivers/vpci/vpci.c |  8 --------
>>  2 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 032e471bb1c0..8dcf2cf9d598 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -656,6 +656,48 @@ 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 ( 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);
>> +    if ( rc )
>> +    {
>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers rc=%d\n",
>> +               pdev->domain, &pdev->sbdf, rc);
>> +        ASSERT_UNREACHABLE();
>> +        return rc;
>> +    }
>> +
>> +    /*
>> +     * 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);
> 
> Like the previous patch, I don't think this last bit is relevant for
> domUs?  Only the hardware domain needs to have the control register
> explicitly handled.
> 
> I don't mind adjusting at commit if we agree.
I agree with you.
Thank you for help to make changes of this and previous patch when you submit.

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

-- 
Best regards,
Jiqian Chen.