| Message ID | 5d7d275ff12c1c991ac80392b19f1ebf5214177d.1690440730.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cover-letter: Add IO page table replacement support | expand |
On Thu, Jul 27, 2023 at 12:23:08AM -0700, Nicolin Chen wrote: > The complication of the mutex and refcount will be amplified after we > introduce the replace support for access. So, add a preparatory change > of a constitutive helper iommufd_access_change_ioas(), to take care of > the existing iommufd_access_attach() and iommufd_access_detach(). > > Also, update the unprotect routine in iommufd_access_destroy_object() > to calling the new iommufd_access_change_ioas() helper. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/iommufd/device.c | 112 ++++++++++++++++++++------------- > 1 file changed, 69 insertions(+), 43 deletions(-) > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c > index 7a3e8660b902..d9680a247e1c 100644 > --- a/drivers/iommu/iommufd/device.c > +++ b/drivers/iommu/iommufd/device.c > @@ -684,17 +684,69 @@ void iommufd_device_detach(struct iommufd_device *idev) > } > EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD); > > +static int iommufd_access_change_ioas(struct iommufd_access *access, > + struct iommufd_ioas *new_ioas) > +{ > + u32 iopt_access_list_id = access->iopt_access_list_id; > + struct iommufd_ioas *cur_ioas = access->ioas; > + int rc; > + > + lockdep_assert_held(&access->ioas_lock); > + > + /* We are racing with a concurrent detach, bail */ > + if (cur_ioas != access->ioas_unpin) > + return -EBUSY; > + > + if (IS_ERR(new_ioas)) > + return PTR_ERR(new_ioas); > + > + if (cur_ioas == new_ioas) { > + /* Do not forget to put since we allow a duplication */ > + iommufd_put_object(&new_ioas->obj); > + return 0; > + } > + > + /* > + * 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 (new_ioas) { > + rc = iopt_add_access(&new_ioas->iopt, access); > + if (rc) { > + iommufd_put_object(&new_ioas->obj); > + access->ioas = cur_ioas; > + return rc; > + } > + iommufd_ref_to_users(&new_ioas->obj); Kevin's suggestion to just open code the refcount_inc here And have a wrapper func that does: iommufd_access_change_ioas_id(struct iommufd_access *access, u32 id) { struct iommufd_ioas *ioas = iommufd_get_ioas(ictx, ioas_id); int rc; if (IS_ERR(ioas)) return PTR_ERR(ioas); rc = iommufd_access_change_ioas(access, ioas); iommufd_put_object(&ioas->obj); return rc; } Does looks cleaner Then we delete iommufd_ref_to_users() as there are no users (once all the branches are merged). Logic looks OK otherwise Jason
On Thu, Jul 27, 2023 at 11:40:17AM -0300, Jason Gunthorpe wrote: > On Thu, Jul 27, 2023 at 12:23:08AM -0700, Nicolin Chen wrote: > > + if (new_ioas) { > > + rc = iopt_add_access(&new_ioas->iopt, access); > > + if (rc) { > > + iommufd_put_object(&new_ioas->obj); > > + access->ioas = cur_ioas; > > + return rc; > > + } > > + iommufd_ref_to_users(&new_ioas->obj); > > Kevin's suggestion to just open code the refcount_inc here Will replace this iommufd_ref_to_users with a refcount_inc in v10. > And have a wrapper func that does: > > iommufd_access_change_ioas_id(struct iommufd_access *access, u32 id) > { > struct iommufd_ioas *ioas = iommufd_get_ioas(ictx, ioas_id); > int rc; > > if (IS_ERR(ioas)) > return PTR_ERR(ioas); > rc = iommufd_access_change_ioas(access, ioas); > iommufd_put_object(&ioas->obj); > return rc; > } > > Does looks cleaner I see. So we can drop iommufd_put_object(&new_ioas->obj) in iommufd_access_change_ioas(). > Then we delete iommufd_ref_to_users() as there are no users (once all > the branches are merged). Ack. Nicolin
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 7a3e8660b902..d9680a247e1c 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -684,17 +684,69 @@ void iommufd_device_detach(struct iommufd_device *idev) } EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD); +static int iommufd_access_change_ioas(struct iommufd_access *access, + struct iommufd_ioas *new_ioas) +{ + u32 iopt_access_list_id = access->iopt_access_list_id; + struct iommufd_ioas *cur_ioas = access->ioas; + int rc; + + lockdep_assert_held(&access->ioas_lock); + + /* We are racing with a concurrent detach, bail */ + if (cur_ioas != access->ioas_unpin) + return -EBUSY; + + if (IS_ERR(new_ioas)) + return PTR_ERR(new_ioas); + + if (cur_ioas == new_ioas) { + /* Do not forget to put since we allow a duplication */ + iommufd_put_object(&new_ioas->obj); + return 0; + } + + /* + * 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 (new_ioas) { + rc = iopt_add_access(&new_ioas->iopt, access); + if (rc) { + iommufd_put_object(&new_ioas->obj); + access->ioas = cur_ioas; + return rc; + } + iommufd_ref_to_users(&new_ioas->obj); + } + + if (cur_ioas) { + 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, iopt_access_list_id); + refcount_dec(&cur_ioas->obj.users); + } + + access->ioas = new_ioas; + access->ioas_unpin = new_ioas; + + return 0; +} + void iommufd_access_destroy_object(struct iommufd_object *obj) { struct iommufd_access *access = container_of(obj, struct iommufd_access, obj); - if (access->ioas) { - iopt_remove_access(&access->ioas->iopt, access, - access->iopt_access_list_id); - refcount_dec(&access->ioas->obj.users); - access->ioas = NULL; - } + mutex_lock(&access->ioas_lock); + if (access->ioas) + WARN_ON(iommufd_access_change_ioas(access, NULL)); + mutex_unlock(&access->ioas_lock); iommufd_ctx_put(access->ictx); } @@ -761,60 +813,34 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD); void iommufd_access_detach(struct iommufd_access *access) { - struct iommufd_ioas *cur_ioas = access->ioas; + int rc; mutex_lock(&access->ioas_lock); - if (WARN_ON(!access->ioas)) - goto out; - /* - * 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) { + if (WARN_ON(!access->ioas)) { mutex_unlock(&access->ioas_lock); - access->ops->unmap(access->data, 0, ULONG_MAX); - mutex_lock(&access->ioas_lock); + return; } - iopt_remove_access(&cur_ioas->iopt, access, - access->iopt_access_list_id); - refcount_dec(&cur_ioas->obj.users); -out: - access->ioas_unpin = NULL; + rc = iommufd_access_change_ioas(access, NULL); + WARN_ON(rc); mutex_unlock(&access->ioas_lock); } EXPORT_SYMBOL_NS_GPL(iommufd_access_detach, IOMMUFD); int iommufd_access_attach(struct iommufd_access *access, u32 ioas_id) { - struct iommufd_ioas *new_ioas; - int rc = 0; + struct iommufd_ctx *ictx = access->ictx; + int rc; mutex_lock(&access->ioas_lock); - if (WARN_ON(access->ioas || access->ioas_unpin)) { + if (WARN_ON(access->ioas)) { mutex_unlock(&access->ioas_lock); return -EINVAL; } - new_ioas = iommufd_get_ioas(access->ictx, ioas_id); - if (IS_ERR(new_ioas)) { - mutex_unlock(&access->ioas_lock); - return PTR_ERR(new_ioas); - } - - rc = iopt_add_access(&new_ioas->iopt, access); - if (rc) { - mutex_unlock(&access->ioas_lock); - iommufd_put_object(&new_ioas->obj); - return rc; - } - iommufd_ref_to_users(&new_ioas->obj); - - access->ioas = new_ioas; - access->ioas_unpin = new_ioas; + rc = iommufd_access_change_ioas(access, + iommufd_get_ioas(ictx, ioas_id)); mutex_unlock(&access->ioas_lock); - return 0; + return rc; } EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
The complication of the mutex and refcount will be amplified after we introduce the replace support for access. So, add a preparatory change of a constitutive helper iommufd_access_change_ioas(), to take care of the existing iommufd_access_attach() and iommufd_access_detach(). Also, update the unprotect routine in iommufd_access_destroy_object() to calling the new iommufd_access_change_ioas() helper. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/device.c | 112 ++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 43 deletions(-)