Re: [PATCH V1 3/5] vfio: detect closed container

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 11 Jan 2021 16:12:18 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 1/8/2021 2:39 PM, Alex Williamson wrote:
> > On Tue,  5 Jan 2021 07:36:51 -0800
> > Steve Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> Add a function that detects if an iommu_group has a valid container.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  drivers/vfio/vfio.c  | 12 ++++++++++++
> >>  include/linux/vfio.h |  1 +
> >>  2 files changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index 262ab0e..f89ab80 100644
> >> --- a/drivers/vfio/vfio.c
> >> +++ b/drivers/vfio/vfio.c
> >> @@ -61,6 +61,7 @@ struct vfio_container {
> >>  	struct vfio_iommu_driver	*iommu_driver;
> >>  	void				*iommu_data;
> >>  	bool				noiommu;
> >> +	bool				closed;
> >>  };
> >>  
> >>  struct vfio_unbound_dev {
> >> @@ -1223,6 +1224,7 @@ static int vfio_fops_release(struct inode *inode, struct file *filep)
> >>  
> >>  	filep->private_data = NULL;
> >>  
> >> +	container->closed = true;
> >>  	vfio_container_put(container);
> >>  
> >>  	return 0;
> >> @@ -2216,6 +2218,16 @@ void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> >>  }
> >>  EXPORT_SYMBOL_GPL(vfio_group_set_kvm);
> >>  
> >> +bool vfio_iommu_group_contained(struct iommu_group *iommu_group)
> >> +{
> >> +	struct vfio_group *group = vfio_group_get_from_iommu(iommu_group);
> >> +	bool ret = group && group->container && !group->container->closed;
> >> +
> >> +	vfio_group_put(group);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(vfio_iommu_group_contained);  
> > 
> > This seems like a pointless interface, the result is immediately stale.
> > Anything that relies on the container staying open needs to add itself
> > as a user.  We already have some interfaces for that, ex.
> > vfio_group_get_external_user().  Thanks,  
> 
> The significant part here is container->closed, which is only set if userland closes the
> container fd, which is a one-way trip -- the fd for this instance can never be opened 
> again.  The container object may still have other references, and not be destroyed yet.
> In patch 5, kernel code that waits for the RESUME ioctl calls this accessor to test if 
> the ioctl will never arrive.

Ok, that makes sense, the "contained" naming notsomuch.  You mention on
5/5 that you considered defining a new backend interface to call from
the core, but considered it overkill.  I'm not sure what you're
imagining that would be overkill.  We can pretty simply add an optional
callback to struct vfio_iommu_drivers_ops that would allow the iommu
driver to be notified when the container fd is released.  That seems
quite simple and avoids the inverted calling structure presented here.
Thanks,

Alex