[Intel-gfx] [PATCH v2 07/11] vfio-iommufd: Support iommufd for physical VFIO devices

[Tian, Kevin]

> From: Jason Gunthorpe <jgg at nvidia.com>
> Sent: Wednesday, November 9, 2022 1:52 AM
> 
> On Tue, Nov 08, 2022 at 03:41:25PM +0800, Yi Liu wrote:
> > On 2022/11/8 14:10, Nicolin Chen wrote:
> > > On Mon, Nov 07, 2022 at 08:52:51PM -0400, Jason Gunthorpe wrote:
> > >
> > > > @@ -795,6 +800,10 @@ static int vfio_device_first_open(struct
> vfio_device *device)
> > > >   		ret = vfio_group_use_container(device->group);
> > > >   		if (ret)
> > > >   			goto err_module_put;
> > > > +	} else if (device->group->iommufd) {
> > > > +		ret = vfio_iommufd_bind(device, device->group->iommufd);
> > >
> > > Here we check device->group->iommufd...
> > >
> > > > +		if (ret)
> > > > +			goto err_module_put;
> > > >   	}
> > > >   	device->kvm = device->group->kvm;
> > > > @@ -812,6 +821,7 @@ static int vfio_device_first_open(struct
> vfio_device *device)
> > > >   	device->kvm = NULL;
> > > >   	if (device->group->container)
> > > >   		vfio_group_unuse_container(device->group);
> > > > +	vfio_iommufd_unbind(device);
> > >
> > > ...yet, missing here, which could result in kernel oops.
> > >
> > > Should probably add something similar:
> > > +	if (device->group->iommufd)
> > > +		vfio_iommufd_unbind(device);
> > >
> > > Or should check !vdev->iommufd_device inside the ->unbind.
> >
> > this check was in prior version, but removed in this version. any
> > special reason? Jason?
> 
> Oooh, this makes more sense - Kevin pointed out the check was wrong:
> 
> > > +void vfio_iommufd_unbind(struct vfio_device *vdev)
> > > +{
> > > +	lockdep_assert_held(&vdev->dev_set->lock);
> > > +
> > > +	if (!vdev->iommufd_device)
> > > +		return;
> 
> > there is no iommufd_device in the emulated path...
> 
> And he is right, so I dropped it. But really the check was just
> misspelled, it was supposed to be "device->group->iommufd" because the
> caller assumed it.
> 
> Still, I think the right way to fix it is to lift the check as we
> don't touch group->iommufd in iommufd.c
> 

yes this is the right fix.