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

[Tian, Kevin]

> 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?

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. 😊

> 
> > I'd say as long as VFIO_DEVICE_PCI_HOT_RESET works if only one vfio
> > device is open using a empty list (eg we should ensure that the
> > invoking cdev itself is allowed) then I think it is OK.
> 
> Sorry, which empty list are your referring?
> 

I guess it refers to zero-length fd array.

But IMHO this restriction better only applies to the case where
noiommu device (iommufd_ctx=NULL) exists in the device set.

otherwise we still compare iommufd_ctx when multiple devices
are opened.

Then the impact to noiommu case is just that user cannot do
hot reset when it opens multiple devices in a same set.