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

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

On 2025/7/8 23:13, Jan Beulich wrote:
> On 04.07.2025 09:08, Jiqian Chen wrote:
>> ---
>> v6->v7 changes:
>> * Change the pointer parameter of cleanup_msi() to be const.
>> * When vpci_remove_registers() in cleanup_msi() fails, not to return
>>   directly, instead try to free msi and re-add ctrl handler.
>> * Pass pdev->vpci into vpci_add_register() instead of pdev->vpci->msi in
>>   init_msi() since we need that every handler realize that msi is NULL
>>   when msi is free but handlers are still in there.
> 
> Imo this latter change would better have been a separate patch. I'm not
> going to insist though (also I really can't, for not being a maintainer
> of this file).
Thanks. I Will do if Roger has the same opinion.

> 
>> @@ -193,6 +234,42 @@ static void cf_check mask_write(
>>      msi->mask = val;
>>  }
>>  
>> +static int cf_check cleanup_msi(const struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +    unsigned int end;
>> +    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;
> 
> What's this "- 2" for? If there's no masking support, you want to cut off
> _at_ the mask register, not 2 bytes ahead of it? Just like you cut off at
> the pending bits register when there is masking support.
"-2" here is to cut the reserved 2 bytes of Message Data if there is no masking 
support.

> 
>> +    rc = vpci_remove_registers(vpci, ctrl, end - ctrl);
>> +    if ( rc )
>> +        printk(XENLOG_WARNING "%pd %pp: fail to remove MSI handlers 
>> 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: fail to add MSI ctrl handler rc=%d\n",
>> +               pdev->domain, &pdev->sbdf, rc);
> 
> Imo the uses of XENLOG_ERR and XENLOG_WARNING want to change places. The 
> latter
> is extremely likely to be a follow-on failure from the first one failing. Plus
> the latter failing is covered by what you add to control_read(). Which leaves
> as the only case where this is really an error (and XENLOG_ERR might then be
> warranted in both places) if the former succeeds and only the latter fails.
> 
> Hmm, then again vpci_init_capabilities() would too issue an error message in
> that case. Perhaps keep as is then.
I am thinking maybe I need to add a check that "if ( rc == -EEXIST ) return 0;" 
here.
Since in that case, the failure is because vpci_remove_register fails to remove 
control handler and it can do the same thing we need here, so should return 
success.

> 
> Jan

-- 
Best regards,
Jiqian Chen.