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

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

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.