Re: [RFC 03/20] vfio: Add vfio_[un]register_device()

[David Gibson]

On Sun, Sep 19, 2021 at 02:38:31PM +0800, Liu Yi L wrote:
> With /dev/vfio/devices introduced, now a vfio device driver has three
> options to expose its device to userspace:
> 
> a)  only legacy group interface, for devices which haven't been moved to
>     iommufd (e.g. platform devices, sw mdev, etc.);
> 
> b)  both legacy group interface and new device-centric interface, for
>     devices which supports iommufd but also wants to keep backward
>     compatibility (e.g. pci devices in this RFC);
> 
> c)  only new device-centric interface, for new devices which don't carry
>     backward compatibility burden (e.g. hw mdev/subdev with pasid);
> 
> This patch introduces vfio_[un]register_device() helpers for the device
> drivers to specify the device exposure policy to vfio core. Hence the
> existing vfio_[un]register_group_dev() become the wrapper of the new
> helper functions. The new device-centric interface is described as
> 'nongroup' to differentiate from existing 'group' stuff.
> 
> TBD: this patch needs to rebase on top of below series from Christoph in
> next version.
> 
> 	"cleanup vfio iommu_group creation"
> 
> Legacy userspace continues to follow the legacy group interface.
> 
> Newer userspace can first try the new device-centric interface if the
> device is present under /dev/vfio/devices. Otherwise fall back to the
> group interface.
> 
> One open about how to organize the device nodes under /dev/vfio/devices/.
> This RFC adopts a simple policy by keeping a flat layout with mixed devname
> from all kinds of devices. The prerequisite of this model is that devnames
> from different bus types are unique formats:
> 
> 	/dev/vfio/devices/0000:00:14.2 (pci)
> 	/dev/vfio/devices/PNP0103:00 (platform)
> 	/dev/vfio/devices/83b8f4f2-509f-382f-3c1e-e6bfe0fa1001 (mdev)

Oof.  I really don't think this is a good idea.  Ensuring that a
format is "unique" in the sense that it can't collide with any of the
other formats, for *every* value of the parameters on both sides is
actually pretty complicated in general.

I think per-type sub-directories would be helpful here, Jason's
suggestion of just sequential numbers would work as well.

> One alternative option is to arrange device nodes in sub-directories based
> on the device type. But doing so also adds one trouble to userspace. The
> current vfio uAPI is designed to have the user query device type via
> VFIO_DEVICE_GET_INFO after opening the device. With this option the user
> instead needs to figure out the device type before opening the device, to
> identify the sub-directory.

Wouldn't this be up to the operator / configuration, rather than the
actual software though?  I would assume that typically the VFIO
program would be pointed at a specific vfio device node file to use,
e.g.
	my-vfio-prog -d /dev/vfio/pci/0000:0a:03.1

Or more generally, if you're expecting userspace to know a name in a
uniqu pattern, they can equally well know a "type/name" pair.

> Another tricky thing is that "pdev. vs. mdev"
> and "pci vs. platform vs. ccw,..." are orthogonal categorizations. Need
> more thoughts on whether both or just one category should be used to define
> the sub-directories.
> 
> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> ---
>  drivers/vfio/vfio.c  | 137 +++++++++++++++++++++++++++++++++++++++----
>  include/linux/vfio.h |   9 +++
>  2 files changed, 134 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 84436d7abedd..1e87b25962f1 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -51,6 +51,7 @@ static struct vfio {
>  	struct cdev			device_cdev;
>  	dev_t				device_devt;
>  	struct mutex			device_lock;
> +	struct list_head		device_list;
>  	struct idr			device_idr;
>  } vfio;
>  
> @@ -757,7 +758,7 @@ void vfio_init_group_dev(struct vfio_device *device, struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(vfio_init_group_dev);
>  
> -int vfio_register_group_dev(struct vfio_device *device)
> +static int __vfio_register_group_dev(struct vfio_device *device)
>  {
>  	struct vfio_device *existing_device;
>  	struct iommu_group *iommu_group;
> @@ -794,8 +795,13 @@ int vfio_register_group_dev(struct vfio_device *device)
>  	/* Our reference on group is moved to the device */
>  	device->group = group;
>  
> -	/* Refcounting can't start until the driver calls register */
> -	refcount_set(&device->refcount, 1);
> +	/*
> +	 * Refcounting can't start until the driver call register. Don’t
> +	 * start twice when the device is exposed in both group and nongroup
> +	 * interfaces.
> +	 */
> +	if (!refcount_read(&device->refcount))

Is there a possible race here with something getting in and
incrementing the refcount between the read and set?

> +		refcount_set(&device->refcount, 1);
>  
>  	mutex_lock(&group->device_lock);
>  	list_add(&device->group_next, &group->device_list);
> @@ -804,7 +810,78 @@ int vfio_register_group_dev(struct vfio_device *device)
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> +
> +static int __vfio_register_nongroup_dev(struct vfio_device *device)
> +{
> +	struct vfio_device *existing_device;
> +	struct device *dev;
> +	int ret = 0, minor;
> +
> +	mutex_lock(&vfio.device_lock);
> +	list_for_each_entry(existing_device, &vfio.device_list, vfio_next) {
> +		if (existing_device == device) {
> +			ret = -EBUSY;
> +			goto out_unlock;

This indicates a bug in the caller, doesn't it?  Should it be a BUG or
WARN instead?

> +		}
> +	}
> +
> +	minor = idr_alloc(&vfio.device_idr, device, 0, MINORMASK + 1, GFP_KERNEL);
> +	pr_debug("%s - mnior: %d\n", __func__, minor);
> +	if (minor < 0) {
> +		ret = minor;
> +		goto out_unlock;
> +	}
> +
> +	dev = device_create(vfio.device_class, NULL,
> +			    MKDEV(MAJOR(vfio.device_devt), minor),
> +			    device, "%s", dev_name(device->dev));
> +	if (IS_ERR(dev)) {
> +		idr_remove(&vfio.device_idr, minor);
> +		ret = PTR_ERR(dev);
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * Refcounting can't start until the driver call register. Don’t
> +	 * start twice when the device is exposed in both group and nongroup
> +	 * interfaces.
> +	 */
> +	if (!refcount_read(&device->refcount))
> +		refcount_set(&device->refcount, 1);
> +
> +	device->minor = minor;
> +	list_add(&device->vfio_next, &vfio.device_list);
> +	dev_info(device->dev, "Creates Device interface successfully!\n");
> +out_unlock:
> +	mutex_unlock(&vfio.device_lock);
> +	return ret;
> +}
> +
> +int vfio_register_device(struct vfio_device *device, u32 flags)
> +{
> +	int ret = -EINVAL;
> +
> +	device->minor = -1;
> +	device->group = NULL;
> +	atomic_set(&device->opened, 0);
> +
> +	if (flags & ~(VFIO_DEVNODE_GROUP | VFIO_DEVNODE_NONGROUP))
> +		return ret;
> +
> +	if (flags & VFIO_DEVNODE_GROUP) {
> +		ret = __vfio_register_group_dev(device);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (flags & VFIO_DEVNODE_NONGROUP) {
> +		ret = __vfio_register_nongroup_dev(device);
> +		if (ret && device->group)
> +			vfio_unregister_device(device);
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(vfio_register_device);
>  
>  /**
>   * Get a reference to the vfio_device for a device.  Even if the
> @@ -861,13 +938,14 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
>  /*
>   * Decrement the device reference count and wait for the device to be
>   * removed.  Open file descriptors for the device... */
> -void vfio_unregister_group_dev(struct vfio_device *device)
> +void vfio_unregister_device(struct vfio_device *device)
>  {
>  	struct vfio_group *group = device->group;
>  	struct vfio_unbound_dev *unbound;
>  	unsigned int i = 0;
>  	bool interrupted = false;
>  	long rc;
> +	int minor = device->minor;
>  
>  	/*
>  	 * When the device is removed from the group, the group suddenly
> @@ -878,14 +956,20 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>  	 * solve this, we track such devices on the unbound_list to bridge
>  	 * the gap until they're fully unbound.
>  	 */
> -	unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
> -	if (unbound) {
> -		unbound->dev = device->dev;
> -		mutex_lock(&group->unbound_lock);
> -		list_add(&unbound->unbound_next, &group->unbound_list);
> -		mutex_unlock(&group->unbound_lock);
> +	if (group) {
> +		/*
> +		 * If caller hasn't called vfio_register_group_dev(), this
> +		 * branch is not necessary.
> +		 */
> +		unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
> +		if (unbound) {
> +			unbound->dev = device->dev;
> +			mutex_lock(&group->unbound_lock);
> +			list_add(&unbound->unbound_next, &group->unbound_list);
> +			mutex_unlock(&group->unbound_lock);
> +		}
> +		WARN_ON(!unbound);
>  	}
> -	WARN_ON(!unbound);
>  
>  	vfio_device_put(device);
>  	rc = try_wait_for_completion(&device->comp);
> @@ -910,6 +994,21 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>  		}
>  	}
>  
> +	/* nongroup interface related cleanup */
> +	if (minor >= 0) {
> +		mutex_lock(&vfio.device_lock);
> +		list_del(&device->vfio_next);
> +		device->minor = -1;
> +		device_destroy(vfio.device_class,
> +			       MKDEV(MAJOR(vfio.device_devt), minor));
> +		idr_remove(&vfio.device_idr, minor);
> +		mutex_unlock(&vfio.device_lock);
> +	}
> +
> +	/* No need go further if no group. */
> +	if (!group)
> +		return;
> +
>  	mutex_lock(&group->device_lock);
>  	list_del(&device->group_next);
>  	group->dev_counter--;
> @@ -935,6 +1034,18 @@ void vfio_unregister_group_dev(struct vfio_device *device)
>  	/* Matches the get in vfio_register_group_dev() */
>  	vfio_group_put(group);
>  }
> +EXPORT_SYMBOL_GPL(vfio_unregister_device);
> +
> +int vfio_register_group_dev(struct vfio_device *device)
> +{
> +	return vfio_register_device(device, VFIO_DEVNODE_GROUP);
> +}
> +EXPORT_SYMBOL_GPL(vfio_register_group_dev);
> +
> +void vfio_unregister_group_dev(struct vfio_device *device)
> +{
> +	vfio_unregister_device(device);
> +}
>  EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
>  
>  /**
> @@ -2447,6 +2558,7 @@ static int vfio_init_device_class(void)
>  
>  	mutex_init(&vfio.device_lock);
>  	idr_init(&vfio.device_idr);
> +	INIT_LIST_HEAD(&vfio.device_list);
>  
>  	/* /dev/vfio/devices/$DEVICE */
>  	vfio.device_class = class_create(THIS_MODULE, "vfio-device");
> @@ -2542,6 +2654,7 @@ static int __init vfio_init(void)
>  static void __exit vfio_cleanup(void)
>  {
>  	WARN_ON(!list_empty(&vfio.group_list));
> +	WARN_ON(!list_empty(&vfio.device_list));
>  
>  #ifdef CONFIG_VFIO_NOIOMMU
>  	vfio_unregister_iommu_driver(&vfio_noiommu_ops);
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 4a5f3f99eab2..9448b751b663 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -26,6 +26,7 @@ struct vfio_device {
>  	struct list_head group_next;
>  	int minor;
>  	atomic_t opened;
> +	struct list_head vfio_next;
>  };
>  
>  /**
> @@ -73,6 +74,14 @@ enum vfio_iommu_notify_type {
>  	VFIO_IOMMU_CONTAINER_CLOSE = 0,
>  };
>  
> +/* The device can be opened via VFIO_GROUP_GET_DEVICE_FD */
> +#define VFIO_DEVNODE_GROUP	BIT(0)
> +/* The device can be opened via /dev/sys/devices/${DEVICE} */
> +#define VFIO_DEVNODE_NONGROUP	BIT(1)
> +
> +extern int vfio_register_device(struct vfio_device *device, u32 flags);
> +extern void vfio_unregister_device(struct vfio_device *device);
> +
>  /**
>   * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
>   */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature