On 27.07.2023 02:56, Volodymyr Babchuk wrote:
> Hi Roger,
>
> Roger Pau Monné <roger....@citrix.com> writes:
>
>> On Wed, Jul 26, 2023 at 01:17:58AM +0000, Volodymyr Babchuk wrote:
>>>
>>> Hi Roger,
>>>
>>> Roger Pau Monné <roger....@citrix.com> writes:
>>>
>>>> On Thu, Jul 20, 2023 at 12:32:31AM +0000, Volodymyr Babchuk wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>>> @@ -498,6 +537,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
>>>>> unsigned int size,
>>>>> ASSERT(data_offset < size);
>>>>> }
>>>>> spin_unlock(&pdev->vpci->lock);
>>>>> + unlock_locks(d);
>>>>
>>>> There's one issue here, some handlers will cal pcidevs_lock(), which
>>>> will result in a lock over inversion, as in the previous patch we
>>>> agreed that the locking order was pcidevs_lock first, d->pci_lock
>>>> after.
>>>>
>>>> For example the MSI control_write() handler will call
>>>> vpci_msi_arch_enable() which takes the pcidevs lock. I think I will
>>>> have to look into using a dedicated lock for MSI related handling, as
>>>> that's the only place where I think we have this pattern of taking the
>>>> pcidevs_lock after the d->pci_lock.
>>>
>>> I'll mention this in the commit message. Is there something else that I
>>> should do right now?
>>
>> Well, I don't think we want to commit this as-is with a known lock
>> inversion.
>>
>> The functions that require the pcidevs lock are:
>>
>> pt_irq_{create,destroy}_bind()
>> unmap_domain_pirq()
>>
>> AFAICT those functions require the lock in order to assert that the
>> underlying device doesn't go away, as they do also use d->event_lock
>> in order to get exclusive access to the data fields. Please double
>> check that I'm not mistaken.
>
> You are right, all three function does not access any of PCI state
> directly. However...
>
>> If that's accurate you will have to check the call tree that spawns
>> from those functions in order to modify the asserts to check for
>> either the pcidevs or the per-domain pci_list lock being taken.
>
> ... I checked calls for PT_IRQ_TYPE_MSI case, there is only call that
> bothers me: hvm_pi_update_irte(), which calls IO-MMU code via
> vmx_pi_update_irte():
>
> amd_iommu_msi_msg_update_ire() or msi_msg_write_remap_rte().
>
> Both functions read basic pdev fields like sbfd or type. I see no
> problem there, as values of those fields are not supposed to be changed.
But whether fields are basic or will never change doesn't matter when
the pdev struct itself suddenly disappears.
Jan
> Also those function use own locks to protect shared state. But as IO-MMU
> code is quite convoluted it is hard to be sure that it is safe to call
> those functions without holding pdevs_lock. All I can say is that those
> functions and their callees have no ASSERT(pcidevs_locked()).
>
