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);
"
>
> Thanks, Roger.
--
Best regards,
Jiqian Chen.
