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

[Nicolin Chen <nicolinc@nvidia.com>]

Hi Jason/Kevin,

On Tue, Mar 14, 2023 at 01:20:52AM -0700, Nicolin Chen wrote:
> On Fri, Mar 10, 2023 at 01:36:22PM -0400, Jason Gunthorpe wrote:
> > On Wed, Mar 08, 2023 at 05:13:36AM -0800, Yi Liu wrote:
> > 
> > > +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id)
> > > +{
> > > +	struct iommufd_ioas *new_ioas = NULL, *cur_ioas;
> > > +	struct iommufd_ctx *ictx = access->ictx;
> > > +	struct iommufd_object *obj;
> > > +	int rc = 0;
> > > +
> > > +	if (ioas_id) {
> > > +		obj = iommufd_get_object(ictx, ioas_id, IOMMUFD_OBJ_IOAS);
> > > +		if (IS_ERR(obj))
> > > +			return PTR_ERR(obj);
> > > +		new_ioas = container_of(obj, struct iommufd_ioas, obj);
> > > +	}
> > > +
> > > +	mutex_lock(&access->ioas_lock);
> > > +	cur_ioas = access->ioas;
> > > +	if (cur_ioas == new_ioas)
> > > +		goto out_unlock;
> > > +
> > > +	if (new_ioas) {
> > > +		rc = iopt_add_access(&new_ioas->iopt, access);
> > > +		if (rc)
> > > +			goto out_unlock;
> > > +		iommufd_ref_to_users(obj);
> > > +	}
> > > +
> > > +	if (cur_ioas) {
> > > +		iopt_remove_access(&cur_ioas->iopt, access);
> > > +		refcount_dec(&cur_ioas->obj.users);
> > > +	}
> > 
> > This should match the physical side with an add/remove/replace
> > API. Especially since remove is implicit in destroy this series only
> > needs the add API
> 
> I assume that the API would be iommufd_access_attach,
> iommufd_access_detach, and iommufd_access_replace(). And there
> might be an iommufd_access_change_pt(access, pt, bool replace)?
> 
> > And the locking shouldn't come in another patch that brings the
> > replace/remove since with just split add we don't need it.
> 
> Hmm. The iommufd_access_detach would be needed in the following
> cdev series, while the iommufd_access_replace would be need in
> my replace series. So, that would make the API be divided into
> three series.
> 
> Perhaps we can have iommufd_access_attach/detach in this series
> along with a vfio_iommufd_emulated_detach_ioas, and the locking
> will come with another patch in replace series?

I recall that we previously concluded that the unbind() is safe
to go without doing access->ops->unmap, because close_device()
would be called prior to the unbind().

But, to add the vfio_iommufd_emulated_detach_ioas() in the cdev
series, we'd need the access->ops->unmap call, and the locking
and "ioas_unpin" too in the detach and attach APIs, right?

I tried a bit of some update, across this series, cdev series,
and the replace series. Though we might be able to simplify a
bit of this patch/series, yet it doesn't seem to simplify the
changes overall, particularly in the following cdev series for
having an unmap() call and the locking.

Then the replace API would mostly overlap with the attach API,
by only having an additional detach(), which means actually we
only need an iommufd_access_attach API to cover both cases...

Can you please take a look at the final access APIs that I've
attached at the end of the email for things mentioned above?
Hopefully we can confirm and put them correctly into the next
version of the three series.

Thanks
Nic

-----------------------------------------------------------------------
static void __iommufd_access_detach(struct iommufd_access *access)
{
	struct iommufd_ioas *cur_ioas = access->ioas;

	lockdep_assert_held(&access->ioas_lock);
	/*
	 * Set ioas to NULL to block any further iommufd_access_pin_pages().
	 * iommufd_access_unpin_pages() can continue using access->ioas_unpin.
	 */
	access->ioas = NULL;

	if (access->ops->unmap) {
		mutex_unlock(&access->ioas_lock);
		access->ops->unmap(access->data, 0, ULONG_MAX);
		mutex_lock(&access->ioas_lock);
	}
	iopt_remove_access(&cur_ioas->iopt, access);
	refcount_dec(&cur_ioas->obj.users);
}

static int iommufd_access_change_pt(struct iommufd_access *access, u32 ioas_id)
{
	struct iommufd_ioas *new_ioas, *cur_ioas;
	struct iommufd_object *obj;
	int rc = 0;

	obj = iommufd_get_object(access->ictx, ioas_id, IOMMUFD_OBJ_IOAS);
	if (IS_ERR(obj))
		return PTR_ERR(obj);
	new_ioas = container_of(obj, struct iommufd_ioas, obj);

	mutex_lock(&access->ioas_lock);
	cur_ioas = access->ioas;
	if (cur_ioas == new_ioas)
		goto out_unlock;

	rc = iopt_add_access(&new_ioas->iopt, access);
	if (rc)
		goto out_unlock;
	iommufd_ref_to_users(obj);

	if (cur_ioas)
		__iommufd_access_detach(access);
	access->ioas_unpin = new_ioas;
	access->ioas = new_ioas;
	mutex_unlock(&access->ioas_lock);
	return 0;

out_unlock:
	mutex_unlock(&access->ioas_lock);
	iommufd_put_object(obj);
	return rc;
}

int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id)
{
	return iommufd_access_change_pt(access, ioas_id);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);

/* Identical to iommufd_access_attach now... */
int iommufd_access_replace(struct iommufd_access *access, u32 ioas_id)
{
	return iommufd_access_change_pt(access, ioas_id);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_replace, IOMMUFD);

void iommufd_access_detach(struct iommufd_access *access)
{
	mutex_lock(&access->ioas_lock);
	if (WARN_ON(!access->ioas))
		goto out;
	__iommufd_access_detach(access);
out:
	access->ioas_unpin = NULL;
	mutex_unlock(&access->ioas_lock);
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD);