Re: [PATCH 09/13] vfio/pci: Reorganize VFIO_DEVICE_PCI_HOT_RESET to use the device set

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 15, 2021 at 03:00:55PM -0600, Alex Williamson wrote:
> On Wed, 14 Jul 2021 21:20:38 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> > +/*
> > + * We need to get memory_lock for each device, but devices can share mmap_lock,
> > + * therefore we need to zap and hold the vma_lock for each device, and only then
> > + * get each memory_lock.
> > + */
> > +static int vfio_hot_reset_device_set(struct vfio_pci_device *vdev,
> > +				     struct vfio_pci_group_info *groups)
> > +{
> > +	struct vfio_device_set *dev_set = vdev->vdev.dev_set;
> > +	struct vfio_pci_device *cur_mem =
> > +		list_first_entry(&dev_set->device_list, struct vfio_pci_device,
> > +				 vdev.dev_set_list);
> 
> We shouldn't be looking at the list outside of the lock, if the first
> entry got removed we'd break our unwind code.
> 
> > +	struct vfio_pci_device *cur_vma;
> > +	struct vfio_pci_device *cur;
> > +	bool is_mem = true;
> > +	int ret;
> >  
> > -	if (pci_dev_driver(pdev) != &vfio_pci_driver) {
> > -		vfio_device_put(device);
> > -		return -EBUSY;
> > +	mutex_lock(&dev_set->lock);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oh, righto, this is an oopsie!

> > +	list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list)
> > +		up_write(&cur->memory_lock);
> > +	mutex_unlock(&dev_set->lock);
> > +
> > +	return ret;
> 
> 
> Isn't the above section actually redundant to below, ie. we could just
> fall through after the pci_reset_bus()?  Thanks,

It could, but I thought it was less confusing this way due to how
oddball the below is:

> > +err_undo:
> > +	list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) {
> > +		if (cur == cur_mem)
> > +			is_mem = false;
> > +		if (cur == cur_vma)
> > +			break;
> > +		if (is_mem)
> > +			up_write(&cur->memory_lock);
> > +		else
> > +			mutex_unlock(&cur->vma_lock);
> > +	}

But either works, do want it switch in v2?

Thanks,
Jason