Re: [PATCH v3 11/11] vpci/msix: Add function to clean MSIX resources

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

On 2025/5/8 18:04, Roger Pau Monné wrote:
> On Mon, Apr 21, 2025 at 02:19:03PM +0800, Jiqian Chen wrote:
>> When init_msix() fails, it needs to clean all MSIX resources.
>> So, add a new function to do that.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
>> ---
>> v2->v3 changes:
>> * Remove unnecessary clean operations in fini_msix().
>>
>> v1->v2 changes:
>> new patch.
>>
>> Best regards,
>> Jiqian Chen.
>> ---
>>  xen/drivers/vpci/msix.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
>> index 0228ffd9fda9..e322c260f6bc 100644
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -703,6 +703,25 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
>>      return 0;
>>  }
>>  
>> +static void fini_msix(struct pci_dev *pdev)
>> +{
>> +    struct vpci *vpci = pdev->vpci;
>> +    unsigned int msix_pos = pdev->msix_pos;
>> +
>> +    if ( !msix_pos || !vpci->msix )
> 
> That's not fully correct here.  See how in init_msix() vpci->msix is
> set at the tail of the function, after having added the register
> handlers.
Thanks! You are more meticulous. I didn't notice that.
Will change in next version.

> 
> I think you instead want:
> 
> if ( !msix_pos )
>     return;
> 
> vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> 
> if ( !(vpci->msix )
>     return;
> 
> list_del(&vpci->msix->next);
> ...
> 
>> +        return;
>> +
>> +    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]);
>> +
> 
> Since you have added to all previous cleanup functions, do you also
> need a comment here to mention the capability header is not handled?
> 
> TBH I'm not sure whether that's relevant in the context here (and
> other cleanup functions), as the capability header traps are not added
> by the REGISTER_VPCI_{LEGACY,EXTENDED}_CAP() init hooks either, so it
> would seem asymmetric for the cleanup hook to attempt to remove those
> in the first place.
Indeed, you are right.
For symmetry consistency, I should not have to add these comments.
I will remove them for MSI and Rebar in next version.

> 
>> +    vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
>> +    xfree(vpci->msix);
>> +    vpci->msix = NULL;
> 
> XFREE();
> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.