Re: [Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
> From: Jason Gunthorpe > Sent: Wednesday, April 5, 2023 10:29 PM > > > > > > > > Does this need to go in via iommufd first? There seems to be quite a > > > > bit of churn in iommufd/device.c vs the vfio_mdev_ops branch (ie. it > > > > doesn't apply). Thanks, > > > > > > I think it is best to stay with this series, Yi has to rebase it > > > > The rebased version is here. Shall I resend a version which is rebased on > > top of vfio_mdev_ops? > > > > > https://github.com/yiliu1765/iommufd/commit/d3d8f65c82fe2ca2a7b1a635f4b40b2a > 0971daa9 > > When you post the v10 it should be based on top of the vfio_mdev_ops > and the hot reset series. yes. At least, I see the hot reset series needs to be refreshed w.r.t. the comments from Alex and Eric. Regards, Yi Liu
Re: [Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
On Wed, Apr 05, 2023 at 02:10:19PM +, Liu, Yi L wrote: > > From: Jason Gunthorpe > > Sent: Wednesday, April 5, 2023 7:56 PM > > > > On Tue, Apr 04, 2023 at 04:45:12PM -0600, Alex Williamson wrote: > > > On Sat, 1 Apr 2023 08:18:24 -0700 > > > Yi Liu wrote: > > > > > > > From: Nicolin Chen > > > > > > > > Previously, the detach routine is only done by the destroy(). And it was > > > > called by vfio_iommufd_emulated_unbind() when the device runs close(), > > > > so > > > > all the mappings in iopt were cleaned in that setup, when the call trace > > > > reaches this detach() routine. > > > > > > > > Now, there's a need of a detach uAPI, meaning that it does not only need > > > > a new iommufd_access_detach() API, but also requires > > > > access->ops->unmap() > > > > call as a cleanup. So add one. > > > > > > > > However, leaving that unprotected can introduce some potential of a race > > > > condition during the pin_/unpin_pages() call, where access->ioas->iopt > > > > is > > > > getting referenced. So, add an ioas_lock to protect the context of iopt > > > > referencings. > > > > > > > > Also, to allow the iommufd_access_unpin_pages() callback to happen via > > > > this unmap() call, add an ioas_unpin pointer, so the unpin routine won't > > > > be affected by the "access->ioas = NULL" trick. > > > > > > > > Reviewed-by: Kevin Tian > > > > Tested-by: Terrence Xu > > > > Tested-by: Yanting Jiang > > > > Signed-off-by: Nicolin Chen > > > > Signed-off-by: Yi Liu > > > > --- > > > > drivers/iommu/iommufd/device.c | 76 +++-- > > > > drivers/iommu/iommufd/iommufd_private.h | 2 + > > > > include/linux/iommufd.h | 1 + > > > > 3 files changed, 74 insertions(+), 5 deletions(-) > > > > > > Does this need to go in via iommufd first? There seems to be quite a > > > bit of churn in iommufd/device.c vs the vfio_mdev_ops branch (ie. it > > > doesn't apply). Thanks, > > > > I think it is best to stay with this series, Yi has to rebase it > > The rebased version is here. Shall I resend a version which is rebased on > top of vfio_mdev_ops? > > https://github.com/yiliu1765/iommufd/commit/d3d8f65c82fe2ca2a7b1a635f4b40b2a0971daa9 When you post the v10 it should be based on top of the vfio_mdev_ops and the hot reset series. Jason
Re: [Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
> From: Jason Gunthorpe > Sent: Wednesday, April 5, 2023 7:56 PM > > On Tue, Apr 04, 2023 at 04:45:12PM -0600, Alex Williamson wrote: > > On Sat, 1 Apr 2023 08:18:24 -0700 > > Yi Liu wrote: > > > > > From: Nicolin Chen > > > > > > Previously, the detach routine is only done by the destroy(). And it was > > > called by vfio_iommufd_emulated_unbind() when the device runs close(), so > > > all the mappings in iopt were cleaned in that setup, when the call trace > > > reaches this detach() routine. > > > > > > Now, there's a need of a detach uAPI, meaning that it does not only need > > > a new iommufd_access_detach() API, but also requires access->ops->unmap() > > > call as a cleanup. So add one. > > > > > > However, leaving that unprotected can introduce some potential of a race > > > condition during the pin_/unpin_pages() call, where access->ioas->iopt is > > > getting referenced. So, add an ioas_lock to protect the context of iopt > > > referencings. > > > > > > Also, to allow the iommufd_access_unpin_pages() callback to happen via > > > this unmap() call, add an ioas_unpin pointer, so the unpin routine won't > > > be affected by the "access->ioas = NULL" trick. > > > > > > Reviewed-by: Kevin Tian > > > Tested-by: Terrence Xu > > > Tested-by: Yanting Jiang > > > Signed-off-by: Nicolin Chen > > > Signed-off-by: Yi Liu > > > --- > > > drivers/iommu/iommufd/device.c | 76 +++-- > > > drivers/iommu/iommufd/iommufd_private.h | 2 + > > > include/linux/iommufd.h | 1 + > > > 3 files changed, 74 insertions(+), 5 deletions(-) > > > > Does this need to go in via iommufd first? There seems to be quite a > > bit of churn in iommufd/device.c vs the vfio_mdev_ops branch (ie. it > > doesn't apply). Thanks, > > I think it is best to stay with this series, Yi has to rebase it The rebased version is here. Shall I resend a version which is rebased on top of vfio_mdev_ops? https://github.com/yiliu1765/iommufd/commit/d3d8f65c82fe2ca2a7b1a635f4b40b2a0971daa9 Regards, Yi Liu
Re: [Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
On Tue, Apr 04, 2023 at 04:45:12PM -0600, Alex Williamson wrote: > On Sat, 1 Apr 2023 08:18:24 -0700 > Yi Liu wrote: > > > From: Nicolin Chen > > > > Previously, the detach routine is only done by the destroy(). And it was > > called by vfio_iommufd_emulated_unbind() when the device runs close(), so > > all the mappings in iopt were cleaned in that setup, when the call trace > > reaches this detach() routine. > > > > Now, there's a need of a detach uAPI, meaning that it does not only need > > a new iommufd_access_detach() API, but also requires access->ops->unmap() > > call as a cleanup. So add one. > > > > However, leaving that unprotected can introduce some potential of a race > > condition during the pin_/unpin_pages() call, where access->ioas->iopt is > > getting referenced. So, add an ioas_lock to protect the context of iopt > > referencings. > > > > Also, to allow the iommufd_access_unpin_pages() callback to happen via > > this unmap() call, add an ioas_unpin pointer, so the unpin routine won't > > be affected by the "access->ioas = NULL" trick. > > > > Reviewed-by: Kevin Tian > > Tested-by: Terrence Xu > > Tested-by: Yanting Jiang > > Signed-off-by: Nicolin Chen > > Signed-off-by: Yi Liu > > --- > > drivers/iommu/iommufd/device.c | 76 +++-- > > drivers/iommu/iommufd/iommufd_private.h | 2 + > > include/linux/iommufd.h | 1 + > > 3 files changed, 74 insertions(+), 5 deletions(-) > > Does this need to go in via iommufd first? There seems to be quite a > bit of churn in iommufd/device.c vs the vfio_mdev_ops branch (ie. it > doesn't apply). Thanks, I think it is best to stay with this series, Yi has to rebase it Jason
Re: [Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
On Sat, 1 Apr 2023 08:18:24 -0700 Yi Liu wrote: > From: Nicolin Chen > > Previously, the detach routine is only done by the destroy(). And it was > called by vfio_iommufd_emulated_unbind() when the device runs close(), so > all the mappings in iopt were cleaned in that setup, when the call trace > reaches this detach() routine. > > Now, there's a need of a detach uAPI, meaning that it does not only need > a new iommufd_access_detach() API, but also requires access->ops->unmap() > call as a cleanup. So add one. > > However, leaving that unprotected can introduce some potential of a race > condition during the pin_/unpin_pages() call, where access->ioas->iopt is > getting referenced. So, add an ioas_lock to protect the context of iopt > referencings. > > Also, to allow the iommufd_access_unpin_pages() callback to happen via > this unmap() call, add an ioas_unpin pointer, so the unpin routine won't > be affected by the "access->ioas = NULL" trick. > > Reviewed-by: Kevin Tian > Tested-by: Terrence Xu > Tested-by: Yanting Jiang > Signed-off-by: Nicolin Chen > Signed-off-by: Yi Liu > --- > drivers/iommu/iommufd/device.c | 76 +++-- > drivers/iommu/iommufd/iommufd_private.h | 2 + > include/linux/iommufd.h | 1 + > 3 files changed, 74 insertions(+), 5 deletions(-) Does this need to go in via iommufd first? There seems to be quite a bit of churn in iommufd/device.c vs the vfio_mdev_ops branch (ie. it doesn't apply). Thanks, Alex
[Intel-gfx] [PATCH v9 16/25] iommufd/device: Add iommufd_access_detach() API
From: Nicolin Chen
Previously, the detach routine is only done by the destroy(). And it was
called by vfio_iommufd_emulated_unbind() when the device runs close(), so
all the mappings in iopt were cleaned in that setup, when the call trace
reaches this detach() routine.
Now, there's a need of a detach uAPI, meaning that it does not only need
a new iommufd_access_detach() API, but also requires access->ops->unmap()
call as a cleanup. So add one.
However, leaving that unprotected can introduce some potential of a race
condition during the pin_/unpin_pages() call, where access->ioas->iopt is
getting referenced. So, add an ioas_lock to protect the context of iopt
referencings.
Also, to allow the iommufd_access_unpin_pages() callback to happen via
this unmap() call, add an ioas_unpin pointer, so the unpin routine won't
be affected by the "access->ioas = NULL" trick.
Reviewed-by: Kevin Tian
Tested-by: Terrence Xu
Tested-by: Yanting Jiang
Signed-off-by: Nicolin Chen
Signed-off-by: Yi Liu
---
drivers/iommu/iommufd/device.c | 76 +++--
drivers/iommu/iommufd/iommufd_private.h | 2 +
include/linux/iommufd.h | 1 +
3 files changed, 74 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 04a57aa1ae2c..0eaae60f3537 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -474,6 +474,7 @@ iommufd_access_create(struct iommufd_ctx *ictx,
iommufd_ctx_get(ictx);
iommufd_object_finalize(ictx, >obj);
*id = access->obj.id;
+ mutex_init(>ioas_lock);
return access;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);
@@ -493,26 +494,66 @@ void iommufd_access_destroy(struct iommufd_access *access)
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
+static void __iommufd_access_detach(struct iommufd_access *access)
+{
+ struct iommufd_ioas *cur_ioas = access->ioas;
+
+ lockdep_assert_held(>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(>ioas_lock);
+ access->ops->unmap(access->data, 0, ULONG_MAX);
+ mutex_lock(>ioas_lock);
+ }
+ iopt_remove_access(_ioas->iopt, access);
+ refcount_dec(_ioas->obj.users);
+}
+
+void iommufd_access_detach(struct iommufd_access *access)
+{
+ mutex_lock(>ioas_lock);
+ if (WARN_ON(!access->ioas))
+ goto out;
+ __iommufd_access_detach(access);
+out:
+ access->ioas_unpin = NULL;
+ mutex_unlock(>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;
- if (access->ioas != NULL && access->ioas->obj.id != ioas_id)
+ mutex_lock(>ioas_lock);
+ if (access->ioas != NULL && access->ioas->obj.id != ioas_id) {
+ mutex_unlock(>ioas_lock);
return -EINVAL;
+ }
new_ioas = iommufd_get_ioas(access->ictx, ioas_id);
- if (IS_ERR(new_ioas))
+ if (IS_ERR(new_ioas)) {
+ mutex_unlock(>ioas_lock);
return PTR_ERR(new_ioas);
+ }
rc = iopt_add_access(_ioas->iopt, access);
if (rc) {
+ mutex_unlock(>ioas_lock);
iommufd_put_object(_ioas->obj);
return rc;
}
iommufd_ref_to_users(_ioas->obj);
access->ioas = new_ioas;
+ access->ioas_unpin = new_ioas;
+ mutex_unlock(>ioas_lock);
return 0;
}
EXPORT_SYMBOL_NS_GPL(iommufd_access_attach, IOMMUFD);
@@ -567,8 +608,8 @@ void iommufd_access_notify_unmap(struct io_pagetable *iopt,
unsigned long iova,
void iommufd_access_unpin_pages(struct iommufd_access *access,
unsigned long iova, unsigned long length)
{
- struct io_pagetable *iopt = >ioas->iopt;
struct iopt_area_contig_iter iter;
+ struct io_pagetable *iopt;
unsigned long last_iova;
struct iopt_area *area;
@@ -576,6 +617,13 @@ void iommufd_access_unpin_pages(struct iommufd_access
*access,
WARN_ON(check_add_overflow(iova, length - 1, _iova)))
return;
+ mutex_lock(>ioas_lock);
+ if (!access->ioas_unpin) {
+ mutex_unlock(>ioas_lock);
+ return;
+ }
+ iopt = >ioas_unpin->iopt;
+
down_read(>iova_rwsem);
iopt_for_each_contig_area(, area, iopt, iova, last_iova)
iopt_area_remove_access(
@@ -585,6 +633,7 @@ void iommufd_access_unpin_pages(struct iommufd_access
*access,
min(last_iova, iopt_area_last_iova(area;
up_read(>iova_rwsem);
