Re: [PATCH v6 7/8] vpci/msi: Free MSI resources when init_msi() fails

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.06.2025 11:29, Jiqian Chen wrote:
> --- a/xen/drivers/vpci/msi.c
> +++ b/xen/drivers/vpci/msi.c
> @@ -193,6 +193,33 @@ static void cf_check mask_write(
>      msi->mask = val;
>  }
>  
> +static int cf_check cleanup_msi(struct pci_dev *pdev)
> +{
> +    int rc;
> +    unsigned int end, size;
> +    struct vpci *vpci = pdev->vpci;
> +    const unsigned int msi_pos = pdev->msi_pos;
> +    const unsigned int ctrl = msi_control_reg(msi_pos);
> +
> +    if ( !msi_pos || !vpci->msi )
> +        return 0;
> +
> +    if ( vpci->msi->masking )
> +        end = msi_pending_bits_reg(msi_pos, vpci->msi->address64);
> +    else
> +        end = msi_mask_bits_reg(msi_pos, vpci->msi->address64) - 2;
> +
> +    size = end - ctrl;
> +
> +    rc = vpci_remove_registers(vpci, ctrl, size);
> +    if ( rc )
> +        return rc;

This is a difficult one: It's not a good idea to simply return here, yet
at the same time the handling of the register we're unable to remove may
still require e.g. ...

> +    XFREE(vpci->msi);

... this. There may therefore be more work required, such that in the
end we're able to ...

> +    return vpci_add_register(pdev->vpci, vpci_hw_read16, NULL, ctrl, 2, 
> NULL);

... try this at least on a best effort basis.

More generally: I don't think failure here (or in other .cleanup hook
functions) may go entirely silently.

Jan