> From: Alex Williamson <alex.williamson@xxxxxxxxxx> > Sent: Thursday, February 16, 2023 4:09 AM > > On Wed, 15 Feb 2023 07:54:31 +0000 > "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote: > > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > Sent: Tuesday, February 14, 2023 11:47 PM > > > > > > 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. > > > > I've moved it to the header file and call cdev_device_add() > > under #if (IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV))". > > > > > > > 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. > > > > How about below? As Jason's remark on patch 0003, cdev is not available > > for SPAPR. > > > > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig > > index 0476abf154f2..96535adc2301 100644 > > --- a/drivers/vfio/Kconfig > > +++ b/drivers/vfio/Kconfig > > @@ -4,6 +4,8 @@ menuconfig VFIO > > select IOMMU_API > > depends on IOMMUFD || !IOMMUFD > > select INTERVAL_TREE > > + select VFIO_GROUP if SPAPR_TCE_IOMMU > > + select VFIO_DEVICE_CDEV if !VFIO_GROUP && (X86 || S390 || ARM > || ARM64) > > select VFIO_CONTAINER if IOMMUFD=n > > help > > VFIO provides a framework for secure userspace device drivers. > > @@ -14,7 +16,8 @@ menuconfig VFIO > > if VFIO > > config VFIO_DEVICE_CDEV > > bool "Support for the VFIO cdev /dev/vfio/devices/vfioX" > > depends on IOMMUFD && (X86 || S390 || ARM || ARM64) > > + default !VFIO_GROUP > > help > > The VFIO device cdev is another way for userspace to get device > > access. Userspace gets device fd by opening device cdev under > > @@ -23,9 +26,21 @@ config VFIO_DEVICE_CDEV > > > > If you don't know what to do here, say N. > > > > +config VFIO_GROUP > > + bool "Support for the VFIO group /dev/vfio/$group_id" > > + default y > > + help > > + VFIO group is legacy interface for userspace. As the introduction > > + of VFIO device cdev interface, this can be N. For now, before > > + userspace applications are fully converted to new vfio device cdev > > + interface, this should be Y. > > + > > + If you don't know what to do here, say Y. > > + > > I think this does the correct thing, but I'll reserve final judgment > until I can try to break it ;) > > This message needs some tuning though, we're not far enough along the > path of cdev access to consider the group interface "legacy" (imo) or > expect that there are any userspace applications converted. There are > also multiple setting recommendations to befuddle a layperson. Perhaps: > > VFIO group support provides the traditional model for accessing > devices through VFIO and is used by the majority of userspace > applications and drivers making use of VFIO. > > If you don't know what to do here, say Y. Got it. I'll update it to my branch first. 😊 Regards, Yi Liu
