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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.06.2025 11:49, Chen, Jiqian wrote:
> 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?

Not sure, sorry. By "more" I really meant "more" (which may just be code
auditing, results of which would need writing down, but which may also
involve further code changes; see below).

>     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);

As I tried to indicate in my earlier reply, the freeing of this struct is
safe only if the failure above would not leave any register handlers in
place which still (without appropriate checking) use this struct.

>     /*
>      * 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);

You're losing the earlier error here, if there was one. If this one
succeeds, ...

>     if ( rc )
>         printk(XENLOG_ERR "%pd %pp: add dummy msi ctrl handler fail rc=%d\n",
>                pdev->domain, &pdev->sbdf, rc);
> 
>     return rc;

... the caller would (wrongly) get success back.

Jan