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 <[email protected]>
> ---
> cc: "Roger Pau Monné" <[email protected]>
> ---
> 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.
