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

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

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