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.c...@amd.com>
>> ---
>> cc: "Roger Pau Monné" <roger....@citrix.com>
>> ---
>> 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.
