On Tue, 14 Feb 2023 01:55:17 +0000 "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote: > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > Sent: Tuesday, February 14, 2023 3:47 AM > > > > On Mon, 13 Feb 2023 07:13:33 -0800 > > Yi Liu <yi.l.liu@xxxxxxxxx> 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? It seems necessary unless we want to litter the code with #ifdefs. > > 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. I understand the mechanics, I still find VFIO_ENABLE_GROUP redundant and unnecessary. Also, Kconfig should not allow a configuration without either VFIO_GROUP or VFIO_DEVICE_CDEV as this is not functional. Deselecting VFIO_GROUP should select VFIO_DEVICE_CDEV, but VFIO_DEVICE_CDEV should be an optional addition to VFIO_GROUP. Thanks, Alex
