Re: [PATCH 11/13] vfio: Add cdev for vfio_device

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 17, 2023 at 05:49:40AM -0800, Yi Liu wrote:
> This allows user to directly open a vfio device w/o using the legacy
> container/group interface, as a prerequisite for supporting new iommu
> features like nested translation.
> 
> The device fd opened in this manner doesn't have the capability to access
> the device as the fops open() doesn't open the device until the successful
> BIND_IOMMUFD which be added in next patch.
> 
> With this patch, devices registered to vfio core have both group and device
> interface created.
> 
> - group interface : /dev/vfio/$groupID
> - device interface: /dev/vfio/devices/vfioX  (X is the minor number and
> 					      is unique across devices)
> 
> Given a vfio device the user can identify the matching vfioX by checking
> the sysfs path of the device. Take PCI device (0000:6a:01.0) for example,
> /sys/bus/pci/devices/0000\:6a\:01.0/vfio-dev/vfio0/dev contains the
> major:minor of the matching vfioX.
> 
> Userspace then opens the /dev/vfio/devices/vfioX and checks with fstat
> that the major:minor matches.
> 
> The vfio_device cdev logic in this patch:
> *) __vfio_register_dev() path ends up doing cdev_device_add() for each
>    vfio_device;
> *) vfio_unregister_group_dev() path does cdev_device_del();
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  drivers/vfio/vfio_main.c | 103 ++++++++++++++++++++++++++++++++++++---
>  include/linux/vfio.h     |   7 ++-
>  2 files changed, 102 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 78725c28b933..6068ffb7c6b7 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -43,6 +43,9 @@
>  static struct vfio {
>  	struct class			*device_class;
>  	struct ida			device_ida;
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> +	dev_t                           device_devt;
> +#endif

It is a bit strange to see this called CONFIG_IOMMUFD, maybe we should
have a CONFIG_VFIO_DEVICE_FILE that depends on IOMMUFD?

Please try to use a plain 'if (IS_ENABLED())' for more of these

It probably isn't worth saving a few bytes in memory to complicate all
the code, so maybe just always include things like this.

> @@ -156,7 +159,11 @@ static void vfio_device_release(struct device *dev)
>  			container_of(dev, struct vfio_device, device);
>  
>  	vfio_release_device_set(device);
> +#if IS_ENABLED(CONFIG_IOMMUFD)
> +	ida_free(&vfio.device_ida, MINOR(device->device.devt));
> +#else
>  	ida_free(&vfio.device_ida, device->index);
> +#endif

A vfio_device_get_index() would help this

Jason