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. Thanks, Alex > config VFIO_CONTAINER > bool "Support for the VFIO container /dev/vfio/vfio" > select VFIO_IOMMU_TYPE1 if MMU && (X86 || S390 || ARM || ARM64) > + depends on VFIO_GROUP > default y > help > The VFIO container is the classic interface to VFIO for establishing > > Regards, > Yi Liu
