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

On Thu, Jul 15, 2021 at 03:00:55PM -0600, Alex Williamson wrote:

On Wed, 14 Jul 2021 21:20:38 -0300
Jason Gunthorpe [off-list ref] wrote:

quoted

+/*
+ * 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.

quoted

+	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!

quoted

+	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:

quoted

+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