Re: [PATCH v4 09/10] vpci/msi: Free MSI resources when init_msi() fails

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

On 2025/5/22 15:12, Roger Pau Monné wrote:
> On Thu, May 22, 2025 at 02:21:16AM +0000, Chen, Jiqian wrote:
>> On 2025/5/21 19:23, Roger Pau Monné wrote:
>>> On Wed, May 21, 2025 at 07:00:37AM +0000, Chen, Jiqian wrote:
>>>> On 2025/5/20 17:43, Roger Pau Monné wrote:
>>>>> On Tue, May 20, 2025 at 11:14:27AM +0200, Jan Beulich wrote:
>>>>>> On 20.05.2025 11:09, Roger Pau Monné wrote:
>>>>>>> On Tue, May 20, 2025 at 08:40:28AM +0200, Jan Beulich wrote:
>>>>>>>> On 09.05.2025 11:05, Jiqian Chen wrote:
>>>>>>>>> When init_msi() fails, the previous new changes will hide MSI
>>>>>>>>> capability, it can't rely on vpci_deassign_device() to remove
>>>>>>>>> all MSI related resources anymore, those resources must be
>>>>>>>>> removed in cleanup function of MSI.
>>>>>>>>
>>>>>>>> That's because vpci_deassign_device() simply isn't called anymore?
>>>>>>>> Could do with wording along these lines then. But (also applicable
>>>>>>>> to the previous patch) - doesn't this need to come earlier? And is
>>>>>>>> it sufficient to simply remove the register intercepts? Don't you
>>>>>>>> need to put in place ones dropping all writes and making all reads
>>>>>>>> return either 0 or ~0 (covering in particular Dom0, while for DomU-s
>>>>>>>> this may already be the case by default behavior)?
>>>>>>>
>>>>>>> For domUs this is already the default behavior.
>>>>>>>
>>>>>>> For dom0 I think it should be enough to hide the capability from the
>>>>>>> linked list, but not hide all the capability related
>>>>>>> registers.  IMO a well behaved dom0 won't try to access capabilities
>>>>>>> disconnected from the linked list,
>>>>>>
>>>>>> Just that I've seen drivers knowing where their device has certain
>>>>>> capabilities, thus not bothering to look up the respective
>>>>>> capability.
>>>>>
>>>>> OK, so let's make the control register read-only in case of failure.
>>>>>
>>>>> If MSI(-X) is already enabled we should also make the entries
>>>>> read-only, and while that's not very complicated for MSI, it does get
>>>>> more convoluted for MSI-X.  I'm fine with just making the control
>>>>> register read-only for the time being.
>>>> If I understand correctly, I need to avoid control register being removed 
>>>> and set the write hook of control register to be vpci_ignored_write and 
>>>> avoid freeing vpci->msi?
>>>>
>>>> "
>>>>      if ( !msi_pos || !vpci->msi )
>>>>          return;
>>>>
>>>> +    spin_lock(&vpci->lock);
>>>> +    control = vpci_get_register(vpci, msi_control_reg(msi_pos), 2);
>>>> +    if ( control )
>>>> +        control->write = vpci_ignored_write;
>>>> +    spin_unlock(&vpci->lock);
>>>> +
>>>>      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 - msi_control_reg(msi_pos);
>>>> +    start = msi_control_reg(msi_pos) + 2;
>>>> +    size = end - start;
>>>>
>>>> -    vpci_remove_registers(vpci, msi_control_reg(msi_pos), size);
>>>> -    XFREE(vpci->msi);
>>>> +    vpci_remove_registers(vpci, start, size);
>>>
>>> I think you want to first purge all the MSI range, and then add the
>>> control register, also you want to keep the XFREE(), and set the
>>> register as:
>> Understood.
>>
>>>
>>> vpci_add_register(vpci, vpci_hw_read16, NULL, msi_control_reg(msi_pos),
>>>                   2, NULL);
>> And one more question, how do I process return value of vpci_add_register 
>> since definition of cleanup hook is "void"?
>> Print a error message if fail?
> 
> Well, we should consider the cleanup function returning an error code.
> vpci_remove_registers() can also fail, and the error is currently
> ignored.  Both cases should result in failing to assign the device to
> the domain IMO.
OK, will change in next version.
Thank you!

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.