On Wed, Jul 14 2021, Jason Gunthorpe <jgg@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> 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(-)
>
(...)
> @@ -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/
> + */
> + 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;
> }
