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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2025/6/18 22:45, Jan Beulich wrote:
> 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.
Does below meet your modification expectations?

    rc = vpci_remove_registers(vpci, ctrl, size);
    if ( rc )
        printk(XENLOG_ERR "%pd %pp: remove msi handlers fail rc=%d\n",
               pdev->domain, &pdev->sbdf, rc);

    XFREE(vpci->msi);

    /*
     * The driver may not traverse the capability list and think device
     * supports MSI by default. So here let the control register of MSI
     * be Read-Only is to ensure MSI disabled.
     */
    rc = vpci_add_register(vpci, vpci_hw_read16, NULL, ctrl, 2, NULL);
    if ( rc )
        printk(XENLOG_ERR "%pd %pp: add dummy msi ctrl handler fail rc=%d\n",
               pdev->domain, &pdev->sbdf, rc);

    return rc;

> 
> Jan

-- 
Best regards,
Jiqian Chen.