On Tue, Jun 07, 2022 at 07:44:37AM +0200, Christoph Hellwig wrote:

On Mon, Jun 06, 2022 at 09:34:36PM -0300, Jason Gunthorpe wrote:

...

• 	if (!list_empty(&iommu->device_list)) {
  

• 		mutex_lock(&iommu->device_list_lock);
  

• 		mutex_unlock(&iommu->lock);
  

• 		list_for_each_entry(device,
  

• 				    &iommu->device_list,
  

• 				    iommu_entry)
  

• 			device->ops->dma_unmap(
  

• 				device, dma->iova, dma->size);
  

• 		mutex_unlock(&iommu->device_list_lock);
  

• 		mutex_lock(&iommu->lock);
  

• 	}
  

I wonder if factoring this into a little helper instead of the very deep indentation might be a bit better for readability.

...

+static void vfio_iommu_type1_register_device(void *iommu_data,

• 			     struct vfio_device *vdev)
  

{ struct vfio_iommu *iommu = iommu_data;

• if (!vdev->ops->dma_unmap)

• return;
  
  

• mutex_lock(&iommu->lock);

• mutex_lock(&iommu->device_list_lock);

• list_add(&vdev->iommu_entry, &iommu->device_list);

• mutex_unlock(&iommu->device_list_lock);

• mutex_unlock(&iommu->lock);

Why do we need both iommu->lock and the device_list_lock everywhere?

Not everwhere, all the readers are using only one of the locks. The list empty calls that were previously unlocked are done under the iommu->lock and only the list iteration was done under the device_list.

Maybe explain the locking scheme somewhere so that people don't have to guess, because it seems to me that just using iommu->lock would be enough right now.

The expectation is that the dma_umap callback will re-enter the type1 driver via vfio_unpin_pages calls and this will recurse back onto the iommu->lock - so it must be dropped before invoking the callback.

I'll add a note

Jason