Hello Jan,
Jan Beulich <[email protected]> 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.
