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

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

On Wed, Aug 06, 2025 at 10:30:53AM +0200, Jan Beulich wrote:
> On 06.08.2025 10:22, Roger Pau Monné wrote:
> > On Tue, Aug 05, 2025 at 10:43:09AM +0200, Jan Beulich wrote:
> >> On 05.08.2025 05:49, Jiqian Chen wrote:
> >>> --- a/xen/drivers/vpci/msix.c
> >>> +++ b/xen/drivers/vpci/msix.c
> >>> @@ -655,6 +655,48 @@ int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +static int cf_check cleanup_msix(const struct pci_dev *pdev)
> >>> +{
> >>> +    int rc;
> >>> +    struct vpci *vpci = pdev->vpci;
> >>> +    const unsigned int msix_pos = pdev->msix_pos;
> >>> +
> >>> +    if ( !msix_pos )
> >>> +        return 0;
> >>> +
> >>> +    rc = vpci_remove_registers(vpci, msix_control_reg(msix_pos), 2);
> >>> +    if ( rc )
> >>> +    {
> >>> +        printk(XENLOG_ERR "%pd %pp: fail to remove MSIX handlers 
> >>> rc=%d\n",
> >>> +               pdev->domain, &pdev->sbdf, rc);
> >>> +        ASSERT_UNREACHABLE();
> >>> +        return rc;
> >>> +    }
> >>> +
> >>> +    if ( vpci->msix )
> >>> +    {
> >>> +        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]);
> >>> +
> >>> +        XFREE(vpci->msix);
> >>> +    }
> >>> +
> >>> +    /*
> >>> +     * The driver may not traverse the capability list and think device
> >>> +     * supports MSIX by default. So here let the control register of MSIX
> >>> +     * be Read-Only is to ensure MSIX disabled.
> >>> +     */
> >>> +    rc = vpci_add_register(vpci, vpci_hw_read16, NULL,
> >>> +                           msix_control_reg(msix_pos), 2, NULL);
> >>> +    if ( rc )
> >>> +        printk(XENLOG_ERR "%pd %pp: fail to add MSIX ctrl handler 
> >>> rc=%d\n",
> >>> +               pdev->domain, &pdev->sbdf, rc);
> >>
> >> Here as well as for MSI: Wouldn't this better be limited to the 
> >> init-failure
> >> case? No point in adding a register hook (and possibly emitting a 
> >> misleading
> >> log message) when we're tearing down anyway. IOW I think the ->cleanup()
> >> hook needs a boolean parameter, unless the distinction of the two cases can
> >> be (reliably) inferred from some other property.
> > 
> > I don't think we have any signal in pci_dev itself that notices
> > whether the device is being deassigned, in which case it does need an
> > extra boolean parameter to notice whether to add the r/o handler.
> > 
> > I'm also wondering whether we want to limit this hiding to the
> > hardware domain only, and for domUs fail the operation instead, and
> > fail to assign the device.  That can be adjusted in a later patch
> > though.
> 
> Yes, DomU wants handling as you say. Iirc there are other open issues with
> DomU support, though. Hence yes, "later" ought to suffice here. Perhaps
> worth annotating with a fixme, though, to be able to easily spot all the
> places that require adjustment.

Sometimes I don't take into account that vPCI is also supposed to be
used by domUs in the long run and forget about that aspect when
reviewing patches

Roger.