[Intel-gfx] [PATCH v5 09/19] vfio/pci: Allow passing zero-length fd array in VFIO_DEVICE_PCI_HOT_RESET

[Liu, Yi L]

> From: Liu, Yi L <yi.l.liu at intel.com>
> Sent: Sunday, March 5, 2023 10:49 PM
> 
> > From: Alex Williamson <alex.williamson at redhat.com>
> > Sent: Saturday, March 4, 2023 12:56 AM
> >
> > On Fri, 3 Mar 2023 06:36:35 +0000
> > "Tian, Kevin" <kevin.tian at intel.com> wrote:
> >
> > > > From: Liu, Yi L <yi.l.liu at intel.com>
> > > > Sent: Thursday, March 2, 2023 10:20 PM
> > > >
> > > > > From: Jason Gunthorpe <jgg at nvidia.com>
> > > > > Sent: Thursday, March 2, 2023 8:35 PM
> > > > >
> > > > > On Thu, Mar 02, 2023 at 09:55:46AM +0000, Tian, Kevin wrote:
> > > > > > > From: Liu, Yi L <yi.l.liu at intel.com>
> > > > > > > Sent: Thursday, March 2, 2023 2:07 PM
> > > > > > >
> > > > > > > > -		if (!vfio_dev_in_groups(cur_vma, groups)) {
> > > > > > > > +		if (cur_vma->vdev.open_count &&
> > > > > > > > +		    !vfio_dev_in_groups(cur_vma, groups) &&
> > > > > > > > +		    !vfio_dev_in_iommufd_ctx(cur_vma,
> > > > iommufd_ctx)) {
> > > > > > >
> > > > > > > Hi Alex, Jason,
> > > > > > >
> > > > > > > There is one concern on this approach which is related to the
> > > > > > > cdev noiommu mode. As patch 16 of this series, cdev path
> > > > > > > supports noiommu mode by passing a negative iommufd to
> > > > > > > kernel. In such case, the vfio_device is not bound to a valid
> > > > > > > iommufd. Then the check in vfio_dev_in_iommufd_ctx() is
> > > > > > > to be broken.
> > > > > > >
> > > > > > > An idea is to add a cdev_noiommu flag in vfio_device, when
> > > > > > > checking the iommufd_ictx, also check this flag. If all the opened
> > > > > > > devices in the dev_set have vfio_device->cdev_noiommu==true,
> > > > > > > then the reset is considered to be doable. But there is a special
> > > > > > > case. If devices in this dev_set are opened by two applications
> > > > > > > that operates in cdev noiommu mode, then this logic is not able
> > > > > > > to differentiate them. In that case, should we allow the reset?
> > > > > > > It seems to ok to allow reset since noiommu mode itself means
> > > > > > > no security between the applications that use it. thoughts?
> > > > > > >
> > > > > >
> > > > > > Probably we need still pass in a valid iommufd (instead of using
> > > > > > a negative value) in noiommu case to mark the ownership so the
> > > > > > check in the reset path can correctly catch whether an opened
> > > > > > device belongs to this user.
> > > > >
> > > > > There should be no iommufd at all in no-iommu mode
> > > > >
> > > > > Adding one just to deal with noiommu reset seems pretty sad :\
> > > > >
> > > > > no-iommu is only really used by dpdk, and it doesn't invoke
> > > > > VFIO_DEVICE_PCI_HOT_RESET at all.
> > > >
> > > > Does it happen to be or by design, this ioctl is not needed by dpdk?
> >
> > I can't think of a reason DPDK couldn't use hot-reset.  If we want to
> > make it a policy, it should be enforced by code, but creating that
> > policy based on a difficulty in supporting that mode with iommufd isn't
> > great.
> 
> Makes sense. A userspace driver should have the chance to reset
> device.
> 
> >
> > > use of noiommu should be discouraged.
> > >
> > > if only known noiommu user doesn't use it then having certain
> > > new restriction for noiommu in the hot reset path might be an
> > > acceptable tradeoff.
> > >
> > > but again needs Alex's input as he knows all the history about
> > > noiommu. 😊
> >
> > No-IOMMU mode was meant to be a minimally invasive code change to
> > re-use the vfio device interface, or alternatively avoid extending
> > uio-pci-generic to support MSI/X, with better logging/tainting to know
> > when userspace is driving devices without IOMMU protection, and as a
> > means to promote a transition to standard support of vfio.  AFAIK,
> > there are still environments without v/IOMMU that make use of no-
> iommu
> > mode.  Thanks,
> 
> This makes Jason's remark (noiommu should not use iommufd at all) much
> more reasonable. If there is no v/IOMMU, then no iommufd at all.

A correction. A system without iommu can still have iommufd. But
I it doesn’t change the direction here.

> If no iommufd is used in the no-iommu mode, this approach cannot
> tell two applications that are operating in no-iommu mode. If we allow
> reset, it may make no-iommu mode more weak. So perhaps we need
> to have another approach for this ownership check.
> 
> How about falling back to prior solution. Allow userspace to pass a set
> of device fd, and the kernel just checks the opened devices in the dev_set,
> all the opened devices should be included in the device fd set. If not all
> of them are included, fail it.
> 
> Regards,
> Yi Liu
>