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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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.

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

XFREE();

Thanks, Roger.