Re: [PATCH v3 00/15] Add vfio_device cdev for iommufd support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux