> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Thursday, March 2, 2023 1:49 AM
>
> On Wed, Mar 01, 2023 at 02:04:00PM +0000, Liu, Yi L wrote:
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index 2a13442add43..ed3ffe7ceb3f 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -777,6 +777,11 @@ void vfio_device_group_unregister(struct
> vfio_device *device)
> > mutex_unlock(&device->group->device_lock);
> > }
> >
> > +bool vfio_device_group_uses_container(struct vfio_device *device)
> > +{
> > + return READ_ONCE(device->group->container);
> > +}
>
> As I said this should take in the vfio_device_file because as long as
> a vfio_device_file exists then group->contianer is required to be stable.
Ok, let me store vfio_group in vfio_devcie_file instead of reach
it by df->device->group.
btw. With vfio_group stored in vfio_device_file, it looks like
the is_cdev_device flag (introduced in patch 14) is not necessary
now, we can always define the group pointer in vfio_device_file
even group code is compiled out, then we can use this group
pointer to check if the vfio_device_file is used in the group path
or the cdev path. Is it?
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 121a75fadceb..4b5b17e8aaa1 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -422,9 +422,22 @@ static int vfio_device_first_open(struct
> vfio_device_file *df)
> > if (!try_module_get(device->dev->driver->owner))
> > return -ENODEV;
> >
> > + /*
> > + * The handling here depends on what the user is using.
> > + *
> > + * If user uses iommufd in the group compat mode or the
> > + * cdev path, call vfio_iommufd_bind().
> > + *
> > + * If user uses container in the group legacy mode, call
> > + * vfio_device_group_use_iommu().
> > + *
> > + * If user doesn't use iommufd nor container, this is
> > + * the noiommufd mode in the cdev path, nothing needs
> > + * to be done here just go ahead to open device.
> > + */
> > if (iommufd)
> > ret = vfio_iommufd_bind(device, iommufd);
> > - else
> > + else if (vfio_device_group_uses_container(device))
> > ret = vfio_device_group_use_iommu(device);
>
> But yes, this makes alot more sense..
>
> Jason
Regards,
Yi Liu
