From: Yi Liu yi.l.liu@intel.com

dev_iommu_ops() is essentially only used in iommu subsystem, so move to a private header to avoid being abused by other drivers.

Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Yi Liu yi.l.liu@intel.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommu-priv.h | 18 ++++++++++++++++++ drivers/iommu/iommu.c | 2 ++ include/linux/iommu.h | 11 ----------- 3 files changed, 20 insertions(+), 11 deletions(-) create mode 100644 drivers/iommu/iommu-priv.h

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h new file mode 100644 index 000000000000..9e1497027cff --- /dev/null +++ b/drivers/iommu/iommu-priv.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __LINUX_IOMMU_PRIV_H +#define __LINUX_IOMMU_PRIV_H + +#include <linux/iommu.h> + +static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) +{ + /* + * Assume that valid ops must be installed if iommu_probe_device() + * has succeeded. The device ops are essentially for internal use + * within the IOMMU subsystem itself, so we should be able to trust + * ourselves not to misuse the helper. + */ + return dev->iommu->iommu_dev->ops; +} +#endif /* __LINUX_IOMMU_PRIV_H */ diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 4b5a21ac5e88..a18b7f1a4e6e 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -35,6 +35,8 @@

#include "iommu-sva.h"

+#include "iommu-priv.h" + static struct kset *iommu_group_kset; static DEFINE_IDA(iommu_group_ida);

diff --git a/include/linux/iommu.h b/include/linux/iommu.h index a8063f26ff69..cb586d054c57 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -445,17 +445,6 @@ static inline void iommu_iotlb_gather_init(struct iommu_iotlb_gather *gather) }; }

-static inline const struct iommu_ops *dev_iommu_ops(struct device *dev) -{ - /* - * Assume that valid ops must be installed if iommu_probe_device() - * has succeeded. The device ops are essentially for internal use - * within the IOMMU subsystem itself, so we should be able to trust - * ourselves not to misuse the helper. - */ - return dev->iommu->iommu_dev->ops; -} - extern int bus_iommu_probe(struct bus_type *bus); extern bool iommu_present(struct bus_type *bus); extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);

To prepare for an access->ioas replacement, move iommufd_access_create() call into vfio_iommufd_emulated_bind(), making it symmetric with the __vfio_iommufd_access_destroy() call in vfio_iommufd_emulated_unbind(). This means an access is created/destroyed by the bind()/unbind(), and the vfio_iommufd_emulated_attach_ioas() only updates the access->ioas pointer.

Since there's no longer an ioas_id input for iommufd_access_create(), add a new helper iommufd_access_set_ioas() to set access->ioas. We can later add a "replace" feature simply to the new iommufd_access_set_ioas() too.

Leaving the access->ioas in vfio_iommufd_emulated_attach_ioas(), however, can introduce some potential of a race condition during pin_/unpin_pages() call where access->ioas->iopt is getting referenced. So, add an ioas_lock to protect it.

Note that the "refcount_dec(&access->ioas->obj.users)" line is also moved to the new iommufd_access_set_ioas() from iommufd_access_destroy_object() for symmetry. Without this change, the old_ioas would also lose the track of its refcount when the replace support is added.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/device.c | 100 ++++++++++++++++++------ drivers/iommu/iommufd/iommufd_private.h | 1 + drivers/iommu/iommufd/selftest.c | 5 +- drivers/vfio/iommufd.c | 27 ++++--- include/linux/iommufd.h | 3 +- 5 files changed, 97 insertions(+), 39 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index d81f93a321af..f4bd6f532a90 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -418,9 +418,9 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) struct iommufd_access *access = container_of(obj, struct iommufd_access, obj);

- iopt_remove_access(&access->ioas->iopt, access); + iommufd_access_set_ioas(access, 0); iommufd_ctx_put(access->ictx); - refcount_dec(&access->ioas->obj.users); + mutex_destroy(&access->ioas_lock); }

/** @@ -437,12 +437,10 @@ void iommufd_access_destroy_object(struct iommufd_object *obj) * The provided ops are required to use iommufd_access_pin_pages(). */ struct iommufd_access * -iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id, +iommufd_access_create(struct iommufd_ctx *ictx, const struct iommufd_access_ops *ops, void *data) { struct iommufd_access *access; - struct iommufd_object *obj; - int rc;

/* * There is no uAPI for the access object, but to keep things symmetric @@ -455,33 +453,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); iommufd_object_finalize(ictx, &access->obj); return access; -out_put_ioas: - refcount_dec(&access->ioas->obj.users); -out_abort: - iommufd_object_abort(ictx, &access->obj); - return ERR_PTR(rc); } EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD);

@@ -500,6 +483,50 @@ void iommufd_access_destroy(struct iommufd_access *access) } EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);

+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); + } + + access->ioas = new_ioas; + mutex_unlock(&access->ioas_lock); + + return 0; + +out_unlock: + mutex_unlock(&access->ioas_lock); + if (new_ioas) + iommufd_put_object(obj); + return rc; +} +EXPORT_SYMBOL_NS_GPL(iommufd_access_set_ioas, IOMMUFD); + /** * iommufd_access_notify_unmap - Notify users of an iopt to stop using it * @iopt: iopt to work on @@ -550,8 +577,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 = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; unsigned long last_iova; struct iopt_area *area;

@@ -559,6 +586,13 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, WARN_ON(check_add_overflow(iova, length - 1, &last_iova))) return;

+ mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) iopt_area_remove_access( @@ -568,6 +602,7 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, min(last_iova, iopt_area_last_iova(area)))); up_read(&iopt->iova_rwsem); WARN_ON(!iopt_area_contig_done(&iter)); + mutex_unlock(&access->ioas_lock); } EXPORT_SYMBOL_NS_GPL(iommufd_access_unpin_pages, IOMMUFD);

@@ -613,8 +648,8 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, unsigned long length, struct page **out_pages, unsigned int flags) { - struct io_pagetable *iopt = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; unsigned long last_iova; struct iopt_area *area; int rc; @@ -629,6 +664,13 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, if (check_add_overflow(iova, length - 1, &last_iova)) return -EOVERFLOW;

+ mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return -ENOENT; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) { unsigned long last = min(last_iova, iopt_area_last_iova(area)); @@ -659,6 +701,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, }

up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return 0;

err_remove: @@ -673,6 +716,7 @@ int iommufd_access_pin_pages(struct iommufd_access *access, unsigned long iova, iopt_area_last_iova(area)))); } up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return rc; } EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD); @@ -692,8 +736,8 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_pin_pages, IOMMUFD); int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, void *data, size_t length, unsigned int flags) { - struct io_pagetable *iopt = &access->ioas->iopt; struct iopt_area_contig_iter iter; + struct io_pagetable *iopt; struct iopt_area *area; unsigned long last_iova; int rc; @@ -703,6 +747,13 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, if (check_add_overflow(iova, length - 1, &last_iova)) return -EOVERFLOW;

+ mutex_lock(&access->ioas_lock); + if (!access->ioas) { + mutex_unlock(&access->ioas_lock); + return -ENOENT; + } + iopt = &access->ioas->iopt; + down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) { unsigned long last = min(last_iova, iopt_area_last_iova(area)); @@ -729,6 +780,7 @@ int iommufd_access_rw(struct iommufd_access *access, unsigned long iova, rc = -ENOENT; err_out: up_read(&iopt->iova_rwsem); + mutex_unlock(&access->ioas_lock); return rc; } EXPORT_SYMBOL_NS_GPL(iommufd_access_rw, IOMMUFD); diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 222e86591f8a..2f4bb106bac6 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -261,6 +261,7 @@ struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; struct iommufd_ioas *ioas; + struct mutex ioas_lock; const struct iommufd_access_ops *ops; void *data; unsigned long iova_alignment; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index cfb5fe9a5e0e..db4011bdc8a9 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -571,7 +571,7 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd, }

access = iommufd_access_create( - ucmd->ictx, ioas_id, + ucmd->ictx, (flags & MOCK_FLAGS_ACCESS_CREATE_NEEDS_PIN_PAGES) ? &selftest_access_ops_pin : &selftest_access_ops, @@ -580,6 +580,9 @@ static int iommufd_test_create_access(struct iommufd_ucmd *ucmd, rc = PTR_ERR(access); goto out_put_fdno; } + rc = iommufd_access_set_ioas(access, ioas_id); + if (rc) + goto out_destroy; cmd->create_access.out_access_fd = fdno; rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); if (rc) diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 026f81a87dd7..dc9feab73db7 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -141,10 +141,19 @@ static const struct iommufd_access_ops vfio_user_ops = { int vfio_iommufd_emulated_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, u32 *out_device_id) { + struct iommufd_access *user; + lockdep_assert_held(&vdev->dev_set->lock);

- vdev->iommufd_ictx = ictx; iommufd_ctx_get(ictx); + user = iommufd_access_create(vdev->iommufd_ictx, &vfio_user_ops, vdev); + if (IS_ERR(user)) { + iommufd_ctx_put(vdev->iommufd_ictx); + return PTR_ERR(user); + } + iommufd_access_set_ioas(user, 0); + vdev->iommufd_access = user; + vdev->iommufd_ictx = ictx; return 0; } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind); @@ -168,22 +177,14 @@ EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_unbind);

int vfio_iommufd_emulated_attach_ioas(struct vfio_device *vdev, u32 *pt_id) { - struct iommufd_access *user; - lockdep_assert_held(&vdev->dev_set->lock);

if (!vdev->iommufd_ictx) return -EINVAL; + if (!vdev->iommufd_access) + return -ENOENT;

- if (vdev->iommufd_access) - return -EBUSY; - - user = iommufd_access_create(vdev->iommufd_ictx, *pt_id, &vfio_user_ops, - vdev); - if (IS_ERR(user)) - return PTR_ERR(user); - vdev->iommufd_access = user; - return 0; + return iommufd_access_set_ioas(vdev->iommufd_access, *pt_id); } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_attach_ioas);

@@ -194,6 +195,6 @@ void vfio_iommufd_emulated_detach_ioas(struct vfio_device *vdev) if (!vdev->iommufd_ictx || !vdev->iommufd_access) return;

- __vfio_iommufd_access_destroy(vdev); + iommufd_access_set_ioas(vdev->iommufd_access, 0); } EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_detach_ioas); diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h index 9672cf839687..f9bac6f9db2e 100644 --- a/include/linux/iommufd.h +++ b/include/linux/iommufd.h @@ -46,9 +46,10 @@ enum { };

struct iommufd_access * -iommufd_access_create(struct iommufd_ctx *ictx, u32 ioas_id, +iommufd_access_create(struct iommufd_ctx *ictx, const struct iommufd_access_ops *ops, void *data); void iommufd_access_destroy(struct iommufd_access *access); +int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id);

void iommufd_ctx_get(struct iommufd_ctx *ictx);

Support an access->ioas replacement in iommufd_access_set_ioas(), which sets the access->ioas to NULL provisionally so that any further incoming iommufd_access_pin_pages() callback can be blocked.

Then, call access->ops->unmap() to clean up the entire iopt. To allow an 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 above.

Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/device.c | 16 ++++++++++++++-- drivers/iommu/iommufd/iommufd_private.h | 1 + 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index f4bd6f532a90..10ce47484ffa 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) iommufd_ref_to_users(obj); }

+ /* + * 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 (cur_ioas) { + if (new_ioas) { + 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); }

+ access->ioas_unpin = new_ioas; access->ioas = new_ioas; mutex_unlock(&access->ioas_lock);

@@ -587,11 +599,11 @@ void iommufd_access_unpin_pages(struct iommufd_access *access, return;

mutex_lock(&access->ioas_lock); - if (!access->ioas) { + if (!access->ioas_unpin) { mutex_unlock(&access->ioas_lock); return; } - iopt = &access->ioas->iopt; + iopt = &access->ioas_unpin->iopt;

down_read(&iopt->iova_rwsem); iopt_for_each_contig_area(&iter, area, iopt, iova, last_iova) diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 2f4bb106bac6..593138bb37b8 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -261,6 +261,7 @@ struct iommufd_access { struct iommufd_object obj; struct iommufd_ctx *ictx; struct iommufd_ioas *ioas; + struct iommufd_ioas *ioas_unpin; struct mutex ioas_lock; const struct iommufd_access_ops *ops; void *data;

Because list_del() is together with iopt_table_remove_domain(), it makes sense to have list_add_tail() together with iopt_table_add_domain().

Also place the mutex outside the iommufd_device_do_attach() call, similar to what's in the iommufd_device_auto_get_domain() function.

Co-developed-by: Yi Liu yi.l.liu@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com Reviewed-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/device.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 10ce47484ffa..b8c3e3baccb5 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -200,6 +200,8 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, phys_addr_t sw_msi_start = PHYS_ADDR_MAX; int rc;

+ lockdep_assert_held(&hwpt->ioas->mutex); + mutex_lock(&hwpt->devices_lock);

/* @@ -243,6 +245,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, hwpt->domain); if (rc) goto out_detach; + list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list); } }

@@ -304,7 +307,6 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, rc = iommufd_device_do_attach(idev, hwpt); if (rc) goto out_abort; - list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);

mutex_unlock(&ioas->mutex); iommufd_object_finalize(idev->ictx, &hwpt->obj); @@ -343,13 +345,11 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj);

+ mutex_lock(&hwpt->ioas->mutex); rc = iommufd_device_do_attach(idev, hwpt); + mutex_unlock(&hwpt->ioas->mutex); if (rc) goto out_put_pt_obj; - - mutex_lock(&hwpt->ioas->mutex); - list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list); - mutex_unlock(&hwpt->ioas->mutex); break; } case IOMMUFD_OBJ_IOAS: {

Remove the vdev->iommufd_attached check, since the kernel can internally handle a replacement of the IO page table now.

Also update the VFIO_DEVICE_ATTACH_IOMMUFD_PT kdoc in the uAPI header.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/vfio/iommufd.c | 3 --- include/uapi/linux/vfio.h | 6 ++++++ 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index dc9feab73db7..8b719d9424b8 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -97,9 +97,6 @@ int vfio_iommufd_physical_attach_ioas(struct vfio_device *vdev, u32 *pt_id) if (!vdev->iommufd_device) return -EINVAL;

- if (vdev->iommufd_attached) - return -EBUSY; - rc = iommufd_device_attach(vdev->iommufd_device, pt_id); if (rc) return rc; diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index c86cfe442884..69f3ceb18d7d 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -236,6 +236,12 @@ struct vfio_device_bind_iommufd { * * Undo by VFIO_DEVICE_DETACH_IOMMUFD_PT or device fd close. * + * If a vfio device is currently attached to a valid hw_pagetable, without doing + * a VFIO_DEVICE_DETACH_IOMMUFD_PT, a second VFIO_DEVICE_ATTACH_IOMMUFD_PT ioctl + * passing in another hw_pagetable (hwpt) id is allowed. This action, also known + * as a hw_pagetable replacement, will replace the device's currently attached + * hw_pagetable with a new hw_pagetable corresponding to the given pt_id. + * * @argsz: user filled size of this data. * @flags: must be 0. * @pt_id: Input the target id which can represent an ioas or a hwpt

Hi Nic,

From: Nicolin Chen nicolinc@nvidia.com Sent: Wednesday, February 8, 2023 5:18 AM

iommu_group_replace_domain() is introduced to support use cases where an iommu_group can be attached to a new domain without getting detached from the old one. This replacement feature will be useful, for cases such as:

1. vPASID mode, when a guest wants to replace a single pasid (PASID=0) table with a larger table (PASID=N)
2. Nesting mode, when switching the attaching device from an S2 domain to an S1 domain, or when switching between relevant S1 domains.

as it allows these cases to switch seamlessly without a DMA disruption.

So, call iommu_group_replace_domain() in the iommufd_device_do_attach(). And add a __iommmufd_device_detach helper to allow the replace routine to do a partial detach on the current hwpt that's being replaced. Though the updated locking logic is overcomplicated, it will be eased, once those iopt_table_add/remove_ioas and list_add/del calls are moved to hwpt's allocation/destroy() functions in the coming nesting series, as that'll depend on a new ->domain_alloc_user op in the iommu core.

Also, block replace operations that are from/to auto_domains, i.e. only user-allocated hw_pagetables can be replaced or replaced with.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

drivers/iommu/iommufd/device.c | 101 +++++++++++++++++------- drivers/iommu/iommufd/iommufd_private.h | 2 + 2 files changed, 76 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index b8c3e3baccb5..8a9834fc129a 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -9,6 +9,8 @@ #include "io_pagetable.h" #include "iommufd_private.h"

+MODULE_IMPORT_NS(IOMMUFD_INTERNAL);

static bool allow_unsafe_interrupts; module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); MODULE_PARM_DESC( @@ -194,9 +196,61 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, return false; }

+/**

• • __iommmufd_device_detach - Detach a device from idev->hwpt to

new_hwpt

This function doesn't do anything to make this device attached to new_hwpt. It is done in the iommufd_device_attach_ioas(). New_hwpt here indicates if this detach requires to do some extra thing. E.g. remove reserved iova from the idev->hwpt->ioas. So may just say " Detach a device from idev->hwpt", and explain the usage of new_hwpt in the below.

• • @idev: device to detach
• • @new_hwpt: new hw_pagetable to attach (pass in NULL for a simple

detach)

The new hw_pagetable to be attached.

• • @detach_group: flag to call iommu_detach_group
• • This is a cleanup helper shared by the replace and detach routines.

Comparing

• • to a detach routine, a replace routine only needs a partial detach

procedure:

• • it does not need the iommu_detach_group(); it will attach the device to

a new

• • hw_pagetable after a partial detach from the currently attached

hw_pagetable,

• • so certain steps can be skipped if two hw_pagetables have the same

IOAS.

• */

+static void __iommmufd_device_detach(struct iommufd_device *idev,

• 		     struct iommufd_hw_pagetable
  

*new_hwpt,

• 		     bool detach_group)
  

+{

• struct iommufd_hw_pagetable *hwpt = idev->hwpt;
• struct iommufd_ioas *new_ioas = NULL;
• if (new_hwpt)

• new_ioas = new_hwpt->ioas;
  

• mutex_lock(&hwpt->devices_lock);
• list_del(&idev->devices_item);
• if (hwpt->ioas != new_ioas)

• mutex_lock(&hwpt->ioas->mutex);
  

The lock order is mostly hwpt->ioas->mutex and then hwpt->devices_lock. See the iommufd_device_auto_get_domain(). If possible, may switch the order sequence here. Also, rename hwpt to be cur_hwpt, this may help reviewers to distinguish it from the hwpt in the caller of this function. It looks to be a deadlock at first look, but not after closer reading.

• if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {

• if (list_empty(&hwpt->devices)) {
  

• 	iopt_table_remove_domain(&hwpt->ioas->iopt,
  

• 				 hwpt->domain);
  

• 	list_del(&hwpt->hwpt_item);
  

• }
  

• if (detach_group)
  

• 	iommu_detach_group(hwpt->domain, idev->group);
  

• }
• if (hwpt->ioas != new_ioas) {

• iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev-
  

...

dev);

• mutex_unlock(&hwpt->ioas->mutex);
  

• }
• mutex_unlock(&hwpt->devices_lock);
• if (hwpt->auto_domain)

• iommufd_object_destroy_user(idev->ictx, &hwpt->obj);
  

• else

• refcount_dec(&hwpt->obj.users);
  

• idev->hwpt = NULL;
• refcount_dec(&idev->obj.users);

+}

static int iommufd_device_do_attach(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt) {

• struct iommufd_hw_pagetable *cur_hwpt = idev->hwpt; phys_addr_t sw_msi_start = PHYS_ADDR_MAX; int rc;

@@ -236,7 +290,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, * the group once for the first device that is in the group. */ if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {

• rc = iommu_attach_group(hwpt->domain, idev->group);
  

• rc = iommu_group_replace_domain(idev->group, hwpt-
  

...

domain);

if (rc)
	goto out_iova;

@@ -249,6 +303,10 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, } }

• /* Replace the cur_hwpt without iommu_detach_group() */
• if (cur_hwpt)

• __iommmufd_device_detach(idev, hwpt, false);
  

• idev->hwpt = hwpt; refcount_inc(&hwpt->obj.users); list_add(&idev->devices_item, &hwpt->devices);

@@ -256,7 +314,10 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, return 0;

out_detach:

• iommu_detach_group(hwpt->domain, idev->group);

• if (cur_hwpt)

• iommu_group_replace_domain(idev->group, cur_hwpt-
  

...

domain);

• else

• iommu_detach_group(hwpt->domain, idev->group);
  

out_iova: iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev); out_unlock: @@ -345,6 +406,13 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj);

• if (idev->hwpt == hwpt)
  

• 	goto out_done;
  

• if (idev->hwpt && idev->hwpt->auto_domain) {
  

• 	rc = -EBUSY;
  

This means if device was attached to an auto_created hwpt, then we cannot replace it with a user allocated hwpt? If yes, this means the replace is not available until user hwpt support, which is part of nesting.

• 	goto out_put_pt_obj;
  

• }
  

• mutex_lock(&hwpt->ioas->mutex); rc = iommufd_device_do_attach(idev, hwpt); mutex_unlock(&hwpt->ioas->mutex);

@@ -356,6 +424,8 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) struct iommufd_ioas *ioas = container_of(pt_obj, struct iommufd_ioas, obj);

• if (idev->hwpt)
  

• 	return -EBUSY;
  

So we don't allow ioas replacement for physical devices. Is it? Looks like emulated devices allows it.

rc = iommufd_device_auto_get_domain(idev, ioas);
if (rc)
	goto out_put_pt_obj;

@@ -367,6 +437,7 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) }

refcount_inc(&idev->obj.users); +out_done: *pt_id = idev->hwpt->obj.id; rc = 0;

@@ -385,31 +456,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_attach, IOMMUFD); */ void iommufd_device_detach(struct iommufd_device *idev) {

• struct iommufd_hw_pagetable *hwpt = idev->hwpt;
• mutex_lock(&hwpt->ioas->mutex);
• mutex_lock(&hwpt->devices_lock);
• list_del(&idev->devices_item);
• if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {

• if (list_empty(&hwpt->devices)) {
  

• 	iopt_table_remove_domain(&hwpt->ioas->iopt,
  

• 				 hwpt->domain);
  

• 	list_del(&hwpt->hwpt_item);
  

• }
  

• iommu_detach_group(hwpt->domain, idev->group);
  

• }
• iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev);
• mutex_unlock(&hwpt->devices_lock);
• mutex_unlock(&hwpt->ioas->mutex);
• if (hwpt->auto_domain)

• iommufd_object_destroy_user(idev->ictx, &hwpt->obj);
  

• else

• refcount_dec(&hwpt->obj.users);
  

• idev->hwpt = NULL;
• refcount_dec(&idev->obj.users);

• __iommmufd_device_detach(idev, NULL, true);

} EXPORT_SYMBOL_NS_GPL(iommufd_device_detach, IOMMUFD);

diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 593138bb37b8..200c783800ad 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -9,6 +9,8 @@ #include <linux/refcount.h> #include <linux/uaccess.h>

+#include "../iommu-priv.h"

struct iommu_domain; struct iommu_group; struct iommu_option; -- 2.39.1

Regards, Yi Liu

On Thu, Feb 09, 2023 at 04:10:04AM +0000, Tian, Kevin wrote:

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Wednesday, February 8, 2023 5:18 AM

A driver that doesn't implement ops->dma_unmap shouldn't be allowed to do vfio_pin/unpin_pages(), though it can use vfio_dma_rw() to access an iova range. Deny !ops->dma_unmap cases in vfio_pin/unpin_pages().

Suggested-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com

---

drivers/vfio/vfio_main.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index 8559c3dfb335..c7f3251ad6e5 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -1543,6 +1543,8 @@ int vfio_pin_pages(struct vfio_device *device, dma_addr_t iova,

if (iova > ULONG_MAX)
	return -EINVAL;

• if (!device->ops->dma_unmap)
  

• 	return -EINVAL;
  

  /*
  • VFIO ignores the sub page offset, npages is from the start

of * a PAGE_SIZE chunk of IOVA. The caller is expected to recover @@ -1580,6 +1582,8 @@ void vfio_unpin_pages(struct vfio_device *device, dma_addr_t iova, int npage) if (device->iommufd_access) { if (WARN_ON(iova > ULONG_MAX)) return;

• if (!device->ops->dma_unmap)
  

• 	return;
  

IMHO this restriction applies to both iommufd and legacy container.

Yeah that makes sense

Jason

On Thu, Feb 09, 2023 at 02:56:39AM +0000, Tian, Kevin wrote:

External email: Use caution opening links or attachments

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Wednesday, February 8, 2023 5:18 AM

@@ -141,10 +141,19 @@ static const struct iommufd_access_ops vfio_user_ops = { int vfio_iommufd_emulated_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx, u32 *out_device_id) {

• struct iommufd_access *user;
  

• lockdep_assert_held(&vdev->dev_set->lock);
  
  

• vdev->iommufd_ictx = ictx;
  iommufd_ctx_get(ictx);
  

• user = iommufd_access_create(vdev->iommufd_ictx, &vfio_user_ops,
  

vdev);

• if (IS_ERR(user)) {
  

•         iommufd_ctx_put(vdev->iommufd_ictx);
  

•         return PTR_ERR(user);
  

• }
  

• iommufd_access_set_ioas(user, 0);
  

this is not required since ioas has been NULL after creation.

Will drop it.

otherwise,

Reviewed-by: Kevin Tian kevin.tian@intel.com

And add this too.

Thanks! Nic

On Tue, 2023-02-07 at 13:17 -0800, Nicolin Chen wrote: ...snip...

diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c index 026f81a87dd7..dc9feab73db7 100644 --- a/drivers/vfio/iommufd.c +++ b/drivers/vfio/iommufd.c @@ -141,10 +141,19 @@ static const struct iommufd_access_ops vfio_user_ops = {  int vfio_iommufd_emulated_bind(struct vfio_device *vdev,                                struct iommufd_ctx *ictx, u32 *out_device_id)  { +       struct iommufd_access *user;

lockdep_assert_held(&vdev->dev_set->lock);   -       vdev->iommufd_ictx = ictx;         iommufd_ctx_get(ictx); +       user = iommufd_access_create(vdev->iommufd_ictx, &vfio_user_ops, vdev); +       if (IS_ERR(user)) { +               iommufd_ctx_put(vdev->iommufd_ictx);

Matthew noticed a vfio-ccw and -ap regression that blames this patch.

Probably both the iommufd_access_create() and iommufd_ctx_put() calls want the ictx variable itself, instead of the (uninitialized) pointer in the vfio_device. (At least that gets -ccw and -ap working again.)

Thanks, Eric

+               return PTR_ERR(user); +       } +       iommufd_access_set_ioas(user, 0); +       vdev->iommufd_access = user; +       vdev->iommufd_ictx = ictx;         return 0;  }  EXPORT_SYMBOL_GPL(vfio_iommufd_emulated_bind);

On Thu, Feb 09, 2023 at 03:13:08AM +0000, Tian, Kevin wrote:

...

--- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct iommufd_access *access, u32 ioas_id) iommufd_ref_to_users(obj); }

• /*
  

•  * 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 (cur_ioas) {
  

•         if (new_ioas) {
  

•                 mutex_unlock(&access->ioas_lock);
  

•                 access->ops->unmap(access->data, 0, ULONG_MAX);
  

•                 mutex_lock(&access->ioas_lock);
  

•         }
  

why does above only apply to a valid new_ioas? this is the cleanup on cur_ioas then required even when new_ioas=NULL.

Though it'd make sense to put it in the common path, our current detach routine doesn't call this unmap. If we do so, it'd become something new to the normal detach routine. Or does this mean the detach routine has been missing an unmap call so far?

Thanks Nic

On Wed, Feb 08, 2023 at 08:08:42AM +0000, Liu, Yi L wrote:

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Wednesday, February 8, 2023 5:18 AM

iommu_group_replace_domain() is introduced to support use cases where an iommu_group can be attached to a new domain without getting detached from the old one. This replacement feature will be useful, for cases such as:

1. vPASID mode, when a guest wants to replace a single pasid (PASID=0) table with a larger table (PASID=N)
2. Nesting mode, when switching the attaching device from an S2 domain to an S1 domain, or when switching between relevant S1 domains.

as it allows these cases to switch seamlessly without a DMA disruption.

So, call iommu_group_replace_domain() in the iommufd_device_do_attach(). And add a __iommmufd_device_detach helper to allow the replace routine to do a partial detach on the current hwpt that's being replaced. Though the updated locking logic is overcomplicated, it will be eased, once those iopt_table_add/remove_ioas and list_add/del calls are moved to hwpt's allocation/destroy() functions in the coming nesting series, as that'll depend on a new ->domain_alloc_user op in the iommu core.

Also, block replace operations that are from/to auto_domains, i.e. only user-allocated hw_pagetables can be replaced or replaced with.

Signed-off-by: Nicolin Chen nicolinc@nvidia.com

drivers/iommu/iommufd/device.c | 101 +++++++++++++++++------- drivers/iommu/iommufd/iommufd_private.h | 2 + 2 files changed, 76 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index b8c3e3baccb5..8a9834fc129a 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -9,6 +9,8 @@ #include "io_pagetable.h" #include "iommufd_private.h"

+MODULE_IMPORT_NS(IOMMUFD_INTERNAL);

static bool allow_unsafe_interrupts; module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR); MODULE_PARM_DESC( @@ -194,9 +196,61 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, return false; }

+/**

• • __iommmufd_device_detach - Detach a device from idev->hwpt to

new_hwpt

This function doesn't do anything to make this device attached to new_hwpt. It is done in the iommufd_device_attach_ioas(). New_hwpt here indicates if this detach requires to do some extra thing. E.g. remove reserved iova from the idev->hwpt->ioas. So may just say " Detach a device from idev->hwpt", and explain the usage of new_hwpt in the below.

Yea. You are right.

...

• • @idev: device to detach
• • @new_hwpt: new hw_pagetable to attach (pass in NULL for a simple

detach)

The new hw_pagetable to be attached.

OK.

...

• • @detach_group: flag to call iommu_detach_group
• • This is a cleanup helper shared by the replace and detach routines.

Comparing

• • to a detach routine, a replace routine only needs a partial detach

procedure:

• • it does not need the iommu_detach_group(); it will attach the device to

a new

• • hw_pagetable after a partial detach from the currently attached

hw_pagetable,

• • so certain steps can be skipped if two hw_pagetables have the same

IOAS.

• */

+static void __iommmufd_device_detach(struct iommufd_device *idev,

•                              struct iommufd_hw_pagetable
  

*new_hwpt,

•                              bool detach_group)
  

+{

• struct iommufd_hw_pagetable *hwpt = idev->hwpt;
  

• struct iommufd_ioas *new_ioas = NULL;
  

• if (new_hwpt)
  

•         new_ioas = new_hwpt->ioas;
  

• mutex_lock(&hwpt->devices_lock);
  

• list_del(&idev->devices_item);
  

• if (hwpt->ioas != new_ioas)
  

•         mutex_lock(&hwpt->ioas->mutex);
  

The lock order is mostly hwpt->ioas->mutex and then hwpt->devices_lock. See the iommufd_device_auto_get_domain(). If possible, may switch the order sequence here.

Yea, I know it's a bit strange. Yet...

Our nesting series simplifies this part to: if (cur_ioas != new_ioas) { mutex_lock(&hwpt->ioas->mutex); iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev); mutex_unlock(&hwpt->ioas->mutex); }

So, here is trying to avoid something like: if (cur_ioas != new_ioas) mutex_lock(&hwpt->ioas->mutex); // doing something if (cur_ioas != new_ioas) iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev); // doing something if (cur_ioas != new_ioas) mutex_unlock(&hwpt->ioas->mutex);

Also, rename hwpt to be cur_hwpt, this may help reviewers to distinguish it from the hwpt in the caller of this function. It looks to be a deadlock at first look, but not after closer reading.

Sure.

...

@@ -345,6 +406,13 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj);

•         if (idev->hwpt == hwpt)
  

•                 goto out_done;
  

•         if (idev->hwpt && idev->hwpt->auto_domain) {
  

•                 rc = -EBUSY;
  

This means if device was attached to an auto_created hwpt, then we cannot replace it with a user allocated hwpt? If yes, this means the replace is not available until user hwpt support, which is part of nesting.

After aligning with Jason, this limit here might be wrong, as we should be able to support replacing an IOAS. I'd need to take a closer look and fix it in v3.

...

•         if (idev->hwpt)
  

•                 return -EBUSY;
  

So we don't allow ioas replacement for physical devices. Is it? Looks like emulated devices allows it.

This was to avoid an replace with an auto_domain. Similarly, it's likely wrong, as I replied above.

Thanks Nic

On Fri, Feb 10, 2023 at 01:46:18AM +0000, Tian, Kevin wrote:

...

From: Jason Gunthorpe jgg@nvidia.com Sent: Thursday, February 9, 2023 9:24 PM

On Thu, Feb 09, 2023 at 03:23:47AM +0000, Tian, Kevin wrote:

...

...

From: Nicolin Chen nicolinc@nvidia.com Sent: Wednesday, February 8, 2023 5:18 AM

Because list_del() is together with iopt_table_remove_domain(), it makes sense to have list_add_tail() together with iopt_table_add_domain().

Also place the mutex outside the iommufd_device_do_attach() call,

similar

...

...

to what's in the iommufd_device_auto_get_domain() function.

Co-developed-by: Yi Liu yi.l.liu@intel.com Signed-off-by: Yi Liu yi.l.liu@intel.com Reviewed-by: Kevin Tian kevin.tian@intel.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com

shouldn't this be a separate bug fix and backported? double adding a list item would certainly clobber the list...

AFAIK there is no bug, this is just reorganizing things

there is semantics change.

here is the current code:

case IOMMUFD_OBJ_HW_PAGETABLE: { struct iommufd_hw_pagetable *hwpt = container_of(pt_obj, struct iommufd_hw_pagetable, obj);

rc = iommufd_device_do_attach(idev, hwpt);
if (rc)
	goto out_put_pt_obj;

mutex_lock(&hwpt->ioas->mutex);
list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
mutex_unlock(&hwpt->ioas->mutex);
break;

}

Above means every attach to hwpt will try to add the hwpt to the list tail. Isn't it a bug?

Yes, that looks like a bug..

But this patch isn't the right way to fix that.

The HWPT should be permanently linked to the IOAS as long as it exists, and the linkage should happen when it is first created.

So attaching a HWPT to another device should never re-link it to the ioas, thus delete these lines here.

However it looks like iommufd_device_detach() is technically wrong too, it should only detach the IOAS and HWPT if it is going to destroy the HWPT. We can't hit those kinds of bugs ATM because we cannot create naked HWPTs that are not autodomains.

Maybe something like this.. I'll look closer next week

Jason

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index d81f93a321afcb..4e87a44533048a 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -279,7 +279,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, */ mutex_lock(&ioas->mutex); list_for_each_entry(hwpt, &ioas->hwpt_list, hwpt_item) { - if (!hwpt->auto_domain) + if (!hwpt->auto_domain || iommufd_object_alive(&hwpt->obj)) continue;

rc = iommufd_device_do_attach(idev, hwpt); @@ -304,6 +304,7 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev, rc = iommufd_device_do_attach(idev, hwpt); if (rc) goto out_abort; + list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);

mutex_unlock(&ioas->mutex); @@ -346,10 +347,6 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) rc = iommufd_device_do_attach(idev, hwpt); if (rc) goto out_put_pt_obj; - - mutex_lock(&hwpt->ioas->mutex); - list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list); - mutex_unlock(&hwpt->ioas->mutex); break; } case IOMMUFD_OBJ_IOAS: { @@ -390,14 +387,8 @@ void iommufd_device_detach(struct iommufd_device *idev) mutex_lock(&hwpt->ioas->mutex); mutex_lock(&hwpt->devices_lock); list_del(&idev->devices_item); - if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) { - if (list_empty(&hwpt->devices)) { - iopt_table_remove_domain(&hwpt->ioas->iopt, - hwpt->domain); - list_del(&hwpt->hwpt_item); - } + if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) iommu_detach_group(hwpt->domain, idev->group); - } iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev); mutex_unlock(&hwpt->devices_lock); mutex_unlock(&hwpt->ioas->mutex); diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index 43d473989a0667..b11738bbdff7ec 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -13,6 +13,11 @@ void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)

WARN_ON(!list_empty(&hwpt->devices));

+ mutex_lock(&hwpt->ioas->mutex); + iopt_table_remove_domain(&hwpt->ioas->iopt, hwpt->domain); + list_del(&hwpt->hwpt_item); + mutex_unlock(&hwpt->ioas->mutex); + iommu_domain_free(hwpt->domain); refcount_dec(&hwpt->ioas->obj.users); mutex_destroy(&hwpt->devices_lock);

On Fri, Feb 10, 2023 at 02:11:23AM +0000, Tian, Kevin wrote:

My confusion is that we have different flows between detach/attach and replace.

today with separate detach+attach we have following flow:

    Remove device from current hwpt;
    if (last_device in hwpt) {
            Remove hwpt domain from current iopt;
            if (last_device in group)
                    detach group from hwpt domain;
    }

    if (first device in group) {
            attach group to new hwpt domain;
            if (first_device in hwpt)
                    Add hwpt domain to new iopt;
    Add device to new hwpt;

but replace flow is different on the detach part:

    if (first device in group) {
            replace group's domain from current hwpt to new hwpt;
            if (first_device in hwpt)
                    Add hwpt domain to new iopt;
    }

    Remove device from old hwpt;
    if (last_device in old hwpt)
            Remove hwpt domain from old iopt;

    Add device to new hwpt;

I'm yet to figure out whether we have sufficient lock protection to prevent other paths from using old iopt/hwpt to find the device which is already attached to a different domain.

With Jason's new series, the detach() routine is lighter now.

I wonder if it'd be safer now to do the detach() call after iommu_group_replace_domain()?

Thanks Nic

@@ -196,10 +198,41 @@ static bool iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt, return false; }

+/** + * __iommufd_device_detach - Detach a device from idev->hwpt + * @idev: device to detach + * @detach_group: flag to call iommu_detach_group + * + * This is a cleanup helper shared by the replace and detach routines. Comparing + * to a detach routine, a replace call does not need the iommu_detach_group(). + */ +static void __iommufd_device_detach(struct iommufd_device *idev, + bool detach_group) +{ + struct iommufd_hw_pagetable *hwpt = idev->hwpt; + + mutex_lock(&hwpt->devices_lock); + list_del(&idev->devices_item); + if (detach_group && !iommufd_hw_pagetable_has_group(hwpt, idev->group)) + iommu_detach_group(hwpt->domain, idev->group); + iopt_remove_reserved_iova(&hwpt->ioas->iopt, idev->dev); + mutex_unlock(&hwpt->devices_lock); + + if (hwpt->auto_domain) + iommufd_object_destroy_user(idev->ictx, &hwpt->obj); + else + refcount_dec(&hwpt->obj.users); + + idev->hwpt = NULL; + + refcount_dec(&idev->obj.users); +} + /* On success this consumes a hwpt reference from the caller */ static int iommufd_device_do_attach(struct iommufd_device *idev, struct iommufd_hw_pagetable *hwpt) { + struct iommufd_hw_pagetable *cur_hwpt = idev->hwpt; phys_addr_t sw_msi_start = PHYS_ADDR_MAX; int rc;

@@ -237,7 +270,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, * the group once for the first device that is in the group. */ if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) { - rc = iommu_attach_group(hwpt->domain, idev->group); + rc = iommu_group_replace_domain(idev->group, hwpt->domain); if (rc) goto out_iova;

@@ -249,6 +282,10 @@ static int iommufd_device_do_attach(struct iommufd_device *idev, } }

+ /* Detach from the cur_hwpt without iommu_detach_group() */ + if (cur_hwpt) + __iommufd_device_detach(idev, false); + idev->hwpt = hwpt; /* The HWPT reference from the caller is moved to this list */ list_add(&idev->devices_item, &hwpt->devices);