Re: [PATCH v5 09/10] vpci/msi: Free MSI resources when init_msi() fails

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

On 2025/6/5 23:19, Roger Pau Monné wrote:
> On Mon, May 26, 2025 at 05:45:58PM +0800, Jiqian Chen wrote:
>> When init_msi() fails, current logic return fail and free MSI-related
>> resources in vpci_deassign_device(). But the previous new changes will
>> hide MSI capability and return success, it can't reach
>> vpci_deassign_device() to remove resources if hiding success, so those
>> resources must be removed in cleanup function of MSI.
>>
>> To do that, implement cleanup function for MSI.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> 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 | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
>> index 2d45c7867de7..4e106c39efae 100644
>> --- a/xen/drivers/vpci/msi.c
>> +++ b/xen/drivers/vpci/msi.c
>> @@ -193,6 +193,33 @@ static void cf_check mask_write(
>>      msi->mask = val;
>>  }
>>  
>> +static int cf_check cleanup_msi(struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +    unsigned int end, size;
>> +    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 )
> 
> Possibly same request as the previous patch, msi_pos should be after
> the PCI standard header.
msi_pos <= PCI_CAPABILITY_LIST ?
Or msi_pos < 0x40 ?

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.