Re: [PATCH 2/2] vfio: Replace the iommu notifier with a device list

[Jason Gunthorpe <jgg@nvidia.com>]

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