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

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

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?

> 
> So that you make it strictly hardware read-only, and not use the data
> in vpci->msi.
> 
> Regards, Roger.

-- 
Best regards,
Jiqian Chen.