Re: [PATCH 03/13] vfio: Provide better generic support for open/release vfio_device_ops
From: Cornelia Huck <cohuck@redhat.com>
Date: 2021-07-19 12:59:14
Also in:
dri-devel, intel-gfx, kvm, linux-doc
On Wed, Jul 14 2021, Jason Gunthorpe [off-list ref] wrote:
Currently the driver ops have an open/release pair that is called once each time a device FD is opened or closed. Add an additional set of open/close_device() ops which are called when the device FD is opened for the first time and closed for the last time. An analysis shows that all of the drivers require this semantic. Some are open coding it as part of their reflck implementation, and some are just buggy and miss it completely. To retain the current semantics PCI and FSL depend on, introduce the idea of a "device set" which is a grouping of vfio_device's that share the same lock around opening. The device set is established by providing a 'set_id' pointer. All vfio_device's that provide the same pointer will be joined to the same singleton memory and lock across the whole set. This effectively replaces the oddly named reflck. After conversion the set_id will be sourced from: - A struct device from a fsl_mc_device (fsl) - A struct pci_slot (pci) - A struct pci_bus (pci) - The struct vfio_device (everything) The design ensures that the above pointers are live as long as the vfio_device is registered, so they form reliable unique keys to group vfio_devices into sets. This implementation uses xarray instead of searching through the driver core structures, which simplifies the somewhat tricky locking in this area. Following patches convert all the drivers. Signed-off-by: Yishai Hadas <yishaih@nvidia.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/mdev/vfio_mdev.c | 22 ++++++ drivers/vfio/vfio.c | 144 ++++++++++++++++++++++++++++------ include/linux/mdev.h | 2 + include/linux/vfio.h | 19 +++++ 4 files changed, 165 insertions(+), 22 deletions(-)
(...)
quoted hunk ↗ jump to hunk
@@ -760,6 +829,13 @@ int vfio_register_group_dev(struct vfio_device *device) struct iommu_group *iommu_group; struct vfio_group *group; + /* + * If the driver doesn't specify a set then the device is added to a + * signleton set just for itself.
s/signleton/singleton/
quoted hunk ↗ jump to hunk
+ */ + if (!device->dev_set) + vfio_assign_device_set(device, device); + iommu_group = iommu_group_get(device->dev); if (!iommu_group) return -EINVAL;@@ -1361,7 +1437,8 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) { struct vfio_device *device; struct file *filep; - int ret; + int fdno; + int ret = 0; if (0 == atomic_read(&group->container_users) || !group->container->iommu_driver || !vfio_group_viable(group))@@ -1375,38 +1452,38 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) return PTR_ERR(device); if (!try_module_get(device->dev->driver->owner)) { - vfio_device_put(device); - return -ENODEV; + ret = -ENODEV; + goto err_device_put; } - ret = device->ops->open(device); - if (ret) { - module_put(device->dev->driver->owner); - vfio_device_put(device); - return ret; + mutex_lock(&device->dev_set->lock); + device->open_count++; + if (device->open_count == 1 && device->ops->open_device) { + ret = device->ops->open_device(device); + if (ret) + goto err_undo_count;
Won't that fail for mdev devices, until the patches later in this series have been applied? (i.e. bad for bisect)
+ }
+ mutex_unlock(&device->dev_set->lock);
+
+ if (device->ops->open) {
+ ret = device->ops->open(device);
+ if (ret)
+ goto err_close_device;
}