[Intel-gfx] [PATCH v3 00/15] Add vfio_device cdev for iommufd support

[Liu, Yi L]

> From: Alex Williamson <alex.williamson at redhat.com>
> Sent: Tuesday, February 14, 2023 3:47 AM
> 
> On Mon, 13 Feb 2023 07:13:33 -0800
> Yi Liu <yi.l.liu at intel.com> wrote:
> 
> > Existing VFIO provides group-centric user APIs for userspace. Userspace
> > opens the /dev/vfio/$group_id first before getting device fd and hence
> > getting access to device. This is not the desired model for iommufd. Per
> > the conclusion of community discussion[1], iommufd provides device-
> centric
> > kAPIs and requires its consumer (like VFIO) to be device-centric user
> > APIs. Such user APIs are used to associate device with iommufd and also
> > the I/O address spaces managed by the iommufd.
> >
> > This series first introduces a per device file structure to be prepared
> > for further enhancement and refactors the kvm-vfio code to be prepared
> > for accepting device file from userspace. Then refactors the vfio to be
> > able to handle iommufd binding. This refactor includes the mechanism of
> > blocking device access before iommufd bind, making vfio_device_open()
> be
> > exclusive between the group path and the cdev path. Eventually, adds the
> > cdev support for vfio device, and makes group infrastructure optional as
> > it is not needed when vfio device cdev is compiled.
> >
> > This is also a prerequisite for iommu nesting for vfio device[2].
> >
> > The complete code can be found in below branch, simple test done with
> the
> > legacy group path and the cdev path. Draft QEMU branch can be found
> at[3]
> >
> > https://github.com/yiliu1765/iommufd/tree/vfio_device_cdev_v3
> > (config CONFIG_IOMMUFD=y CONFIG_VFIO_DEVICE_CDEV=y)
> 
> Even using your branch[1], it seems like this has not been tested
> except with cdev support enabled:
> 
> /home/alwillia/Work/linux.git/drivers/vfio/vfio_main.c: In function
> ‘vfio_device_add’:
> /home/alwillia/Work/linux.git/drivers/vfio/vfio_main.c:253:48: error: ‘struct
> vfio_device’ has no member named ‘cdev’; did you mean ‘dev’?
>   253 |                 ret = cdev_device_add(&device->cdev, &device->device);
>       |                                                ^~~~
>       |                                                dev
> /home/alwillia/Work/linux.git/drivers/vfio/vfio_main.c: In function
> ‘vfio_device_del’:
> /home/alwillia/Work/linux.git/drivers/vfio/vfio_main.c:262:42: error: ‘struct
> vfio_device’ has no member named ‘cdev’; did you mean ‘dev’?
>   262 |                 cdev_device_del(&device->cdev, &device->device);
>       |                                          ^~~~
>       |                                          dev

Sorry for it. It is due to the cdev definition is under
"#if IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV)". While, in the code it
uses "if (IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV))".  I think for
readability, it would be better to always define cdev in vfio_device,
and keep the using of cdev in code. How about your taste?

> Additionally the VFIO_ENABLE_GROUP Kconfig option doesn't make much
> sense to me, it seems entirely redundant to VFIO_GROUP.

The intention is to make the group code compiling match existing case.
Currently, if VFIO is configured, group code is by default compiled.
So VFIO_ENABLE_GROUP a hidden option, and VFIO_GROUP an option
for user.  User needs to explicitly config VFIO_GROUP if VFIO_DEVICE_CDEV==y.
If VFIO_DEVICE_CDEV==n, then no matter user configed VFIO_GROUP or not,
the group code shall be compiled.

Regards,
Yi Liu