RE: [PATCH v1 1/5] iommufd: Create access in vfio_iommufd_emulated_bind()

["Liu, Yi L" <yi.l.liu@intel.com>]

> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, March 15, 2023 2:52 PM
> 
> > From: Nicolin Chen
> > Sent: Wednesday, March 15, 2023 2:22 PM
> >
> > On Wed, Mar 15, 2023 at 06:16:37AM +0000, Tian, Kevin wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > > From: Nicolin Chen
> > > > Sent: Wednesday, March 15, 2023 2:51 AM
> > > >
> > > > On Fri, Mar 10, 2023 at 02:08:15AM +0000, Tian, Kevin wrote:
> > > > > External email: Use caution opening links or attachments
> > > > >
> > > > >
> > > > > > From: Liu, Yi L <yi.l.liu@intel.com>
> > > > > > Sent: Wednesday, March 8, 2023 9:14 PM
> > > > > >
> > > > > > @@ -449,33 +450,18 @@ iommufd_access_create(struct
> > iommufd_ctx
> > > > *ictx,
> > > > > > u32 ioas_id,
> > > > > >       access->data = data;
> > > > > >       access->ops = ops;
> > > > > >
> > > > > > -     obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > > > > -     if (IS_ERR(obj)) {
> > > > > > -             rc = PTR_ERR(obj);
> > > > > > -             goto out_abort;
> > > > > > -     }
> > > > > > -     access->ioas = container_of(obj, struct iommufd_ioas, obj);
> > > > > > -     iommufd_ref_to_users(obj);
> > > > > > -
> > > > > >       if (ops->needs_pin_pages)
> > > > > >               access->iova_alignment = PAGE_SIZE;
> > > > > >       else
> > > > > >               access->iova_alignment = 1;
> > > > > > -     rc = iopt_add_access(&access->ioas->iopt, access);
> > > > > > -     if (rc)
> > > > > > -             goto out_put_ioas;
> > > > > >
> > > > > >       /* The calling driver is a user until iommufd_access_destroy() */
> > > > > >       refcount_inc(&access->obj.users);
> > > > > > +     mutex_init(&access->ioas_lock);
> > > > > >       access->ictx = ictx;
> > > > > >       iommufd_ctx_get(ictx);
> > > > >
> > > > > this refcnt get should be moved to the start given next patch
> > > > > removes the reference in the caller side.

This change is ok but seems not necessary.

Yes, vfio_iommufd_emulated_bind() will not have reference on the
ictx after the next patch. However, it gets reference only because it
wants to store it in vfio_device. Now, it does not store it. So no get.
I think the caller of vfio_iommufd_emulated_bind() should ensure
the ictx is valid. Also check the physical device bind. So maybe not
necessary to get ictx before calling iommufd_access_create(). This is
the same with the vfio_iommufd_physical_bind() which calls
iommufd_device_bind() without ictx get, and iommufd_device_bind()
also gets ictx in the end.
 
If it's really necessary, maybe let the vfio_iommufd_physical_bind()
and vfio_iommufd_emulated_bind() get/put ictx around calling
iommufd_access_create()/iommufd_device_bind().

> > > >
> > > > I don't feel quite convincing to justify for moving it in this
> > > > patch. Perhaps we should do that in the following patch, where
> > > > it needs this? Or another individual patch moving this alone?
> > > >
> > >
> > > Next patch. I just tried to point out the required change caused
> > > by next patch. 😊
>
> > OK. I made a small individual patch. Posting here so Yi can just
> > squash it into the next patch:
> >
> > From dbfe7457d35ea9a4da9c8e6daa838b101bc8f621 Mon Sep 17 00:00:00
> > 2001
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Date: Tue, 14 Mar 2023 12:51:07 -0700
> > Subject: [PATCH] iommufd/device: Do iommufd_ctx_get() at the top of
> >  iommufd_access_create()
> >
> > The following patch will remove the iommufd_ctx_get() call prior to the
> > iommufd_access_create() call in vfio_iommufd_emulated_bind(),
> expecting
> > iommufd_access_create() call covers the iommufd_ctx_get(). However,
> the
> > iommufd_access_create() only does iommufd_ctx_get() at the end. Thus,
> > move the iommufd_ctx_get() call to the top of iommufd_access_create().
> >
> > Suggested-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > ---
> >  drivers/iommu/iommufd/device.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/iommufd/device.c
> > b/drivers/iommu/iommufd/device.c
> > index 0132803449be..dc1015b02a53 100644
> > --- a/drivers/iommu/iommufd/device.c
> > +++ b/drivers/iommu/iommufd/device.c
> > @@ -649,13 +649,16 @@ iommufd_access_create(struct iommufd_ctx
> *ictx,
> >  {
> >  	struct iommufd_access *access;
> >
> > +	iommufd_ctx_get(ictx);
> >  	/*
> >  	 * There is no uAPI for the access object, but to keep things
> > symmetric
> >  	 * use the object infrastructure anyhow.
> >  	 */
> >  	access = iommufd_object_alloc(ictx, access,
> IOMMUFD_OBJ_ACCESS);
> > -	if (IS_ERR(access))
> > +	if (IS_ERR(access)) {
> > +		iommufd_ctx_put(ictx);
> >  		return access;
> > +	}
> >
> >  	access->data = data;
> >  	access->ops = ops;
> > @@ -668,7 +671,6 @@ iommufd_access_create(struct iommufd_ctx *ictx,
> >  	/* The calling driver is a user until iommufd_access_destroy() */
> >  	refcount_inc(&access->obj.users);
> >  	access->ictx = ictx;
> > -	iommufd_ctx_get(ictx);
> >  	iommufd_object_finalize(ictx, &access->obj);
> >  	return access;
> >  }
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>