On Wed, 13 Jan 2021 12:26:09 -0700
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
> On Mon, 11 Jan 2021 16:12:18 -0500
> Steven Sistare <steven.sistare@xxxxxxxxxx> wrote:
>
> > On 1/8/2021 2:39 PM, Alex Williamson wrote:
> > > On Tue, 5 Jan 2021 07:36:51 -0800
> > > Steve Sistare <steven.sistare@xxxxxxxxxx> wrote:
> > >
> > >> Add a function that detects if an iommu_group has a valid container.
> > >>
> > >> Signed-off-by: Steve Sistare <steven.sistare@xxxxxxxxxx>
> > >> ---
> > >> 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.
A callback into the iommu backend would also allow you to use a
waitqueue and wake_up for the blocking mechanism while vaddr is
invalid. Seems like an improvement over the polling model. Thanks,
Alex
