Re: [PATCH v5 10/10] vpci/msix: Free MSIX resources when init_msix() fails

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

On Fri, Jun 06, 2025 at 08:44:02AM +0000, Chen, Jiqian wrote:
> On 2025/6/5 23:34, Roger Pau Monné wrote:
> > On Mon, May 26, 2025 at 05:45:59PM +0800, Jiqian Chen wrote:
> >> When init_msix() fails, current logic return fail and free MSIX-related
> >> resources in vpci_deassign_device(). But the previous new changes will
> >> hide MSIX 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 MSIX.
> >>
> >> To do that, implement cleanup function for MSIX.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> > 
> > 
> > 
> >> ---
> >> cc: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> >> ---
> >> v4->v5 changes:
> >> * Change definition "static void cleanup_msix" to "static int cf_check 
> >> cleanup_msix" since cleanup hook is changed to be int.
> >> * Add a read-only register for MSIX Control Register in the end of 
> >> cleanup_msix().
> >>
> >> v3->v4 changes:
> >> * Change function name from fini_msix() to cleanup_msix().
> >> * Change to use XFREE to free vpci->msix.
> >> * In cleanup function, change the sequence of check and remove action 
> >> according to init_msix().
> >>
> >> v2->v3 changes:
> >> * Remove unnecessary clean operations in fini_msix().
> >>
> >> v1->v2 changes:
> >> new patch.
> >>
> >> Best regards,
> >> Jiqian Chen.
> >> ---
> >>  xen/drivers/vpci/msix.c | 29 ++++++++++++++++++++++++++++-
> >>  1 file changed, 28 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> >> index 674815ead025..cf79320d3b6f 100644
> >> --- a/xen/drivers/vpci/msix.c
> >> +++ b/xen/drivers/vpci/msix.c
> >> @@ -655,6 +655,33 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>      return 0;
> >>  }
> >>  
> >> +static int cf_check cleanup_msix(struct pci_dev *pdev)
> >> +{
> >> +    int rc;
> >> +    struct vpci *vpci = pdev->vpci;
> >> +    const unsigned int msix_pos = pdev->msix_pos;
> >> +
> >> +    if ( !msix_pos )
> Need to change this check as previous patch, I think.

No, please ignore those previous requests, sorry.

> >> +        return 0;
> >> +
> >> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> >> +    if ( rc )
> >> +        return rc;
> >> +
> >> +    if ( !vpci->msix )
> >> +        return 0;
> > 
> > You cannot short-circuit here, as it then prevents adding the dummy
> > handlers done in the last return.
> > 
> >> +
> >> +    for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
> >> +        if ( vpci->msix->table[i] )
> >> +            iounmap(vpci->msix->table[i]);
> >> +
> >> +    list_del(&vpci->msix->next);
> >> +    XFREE(vpci->msix);
> >> +
> >> +    return vpci_add_register(pdev->vpci, vpci_hw_read16, NULL,
> >> +                             msix_control_reg(msix_pos), 2, NULL);
> > 
> > The above needs to be added even if !vpci->msix.
> Oh, right.
> I will change to
> 
>     if ( vpci->msix )
>     {
>         for ( unsigned int i = 0; i < ARRAY_SIZE(vpci->msix->table); i++ )
>             if ( vpci->msix->table[i] )
>                 iounmap(vpci->msix->table[i]);
> 
>         list_del(&vpci->msix->next);
>         XFREE(vpci->msix);
>     }

LGTM, thanks.

Roger.