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

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

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@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> 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.