On 17.02.2023 02:20, Volodymyr Babchuk wrote:
> Jan Beulich <jbeul...@suse.com> writes:
>> On 27.01.2023 00:56, Stefano Stabellini wrote:
>>> On Wed, 31 Aug 2022, Volodymyr Babchuk wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -1641,6 +1641,7 @@ void iommu_dev_iotlb_flush_timeout(struct domain *d,
>>>> struct pci_dev *pdev)
>>>> {
>>>> pcidevs_lock();
>>>>
>>>> + /* iommu->ats_list_lock is taken by the caller of this function */
>>>
>>> This is a locking inversion. In all other places we take pcidevs_lock
>>> first, then ats_list_lock lock. For instance look at
>>> xen/drivers/passthrough/pci.c:deassign_device that is called with
>>> pcidevs_locked and then calls iommu_call(... reassign_device ...) which
>>> ends up taking ats_list_lock.
>>>
>>> This is the only exception. I think we need to move the
>>> spin_lock(ats_list_lock) from qinval.c to here.
>>
>> First question here is what the lock is meant to protect: Just the list,
>> or also the ATS state change (i.e. serializing enable and disable against
>> one another). In the latter case the lock also wants naming differently.
>
> My intention was to protect list only. But I believe you are right and
> we should protect the whole state. I'll rename the lock to ats_lock.
>
>> Second question is who is to acquire the lock. Why isn't this done _in_
>> {en,dis}able_ats_device() themselves? That would then allow to further
>> reduce the locked range, because at least the pci_find_ext_capability()
>> call and the final logging can occur without the lock held.
>
> You are right, I'll extended {en,dis}able_ats_device() API to pass
> pointer to the lock.
Hmm, that'll make for an odd interface. I was wondering in the past
already why we don't have a backref from the PCI dev to its controlling
IOMMU (might be ambiguous and hence better left unset for bridges,
especially host ones, but I think ATS is being fiddled with only for
leaf devices; would need double checking of course).
Jan
