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

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

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.

Thanks, Roger.