[v4,07/17] iommufd: Add user-managed hw_pagetable support

Commit Message

Yi Liu Sept. 21, 2023, 7:51 a.m. UTC

From: Nicolin Chen <[e-mail]>

Add a parent hw_pagetable pointer for user-managed hw_pagetables. Similar
to the ioas->mutex, add another mutex in the kernel-managed hw_pagetable
to serialize associating user-managed hw_pagetable allocations. Then, add
user_managed flag too in the struct to ease identifying a HWPT.

Also, add a new allocator iommufd_user_managed_hwpt_alloc() and two pairs
of cleanup functions iommufd_user_managed_hwpt_destroy/abort().

Signed-off-by: Nicolin Chen <[e-mail]>
Signed-off-by: Yi Liu <[e-mail]>
---
 drivers/iommu/iommufd/hw_pagetable.c    | 112 +++++++++++++++++++++++-
 drivers/iommu/iommufd/iommufd_private.h |   6 ++
 2 files changed, 117 insertions(+), 1 deletion(-)

Comments

Tian, Kevin Sept. 26, 2023, 8:14 a.m. UTC | #1

> From: Liu, Yi L <[e-mail]>
> Sent: Thursday, September 21, 2023 3:51 PM
> 
> +static void iommufd_user_managed_hwpt_abort(struct iommufd_object
> *obj)
> +{
> +	struct iommufd_hw_pagetable *hwpt =
> +		container_of(obj, struct iommufd_hw_pagetable, obj);
> +
> +	/* The parent->mutex must be held until finalize is called. */
> +	lockdep_assert_held(&hwpt->parent->mutex);
> +
> +	iommufd_hw_pagetable_destroy(obj);
> +}

Can you elaborate what exactly is protected by parent->mutex?

My gut-feeling that the child just grabs a refcnt on the parent
object. It doesn't change any other data of the parent.

> 
> +/**
> + * iommufd_user_managed_hwpt_alloc() - Get a user-managed
> hw_pagetable
> + * @ictx: iommufd context
> + * @pt_obj: Parent object to an HWPT to associate the domain with
> + * @idev: Device to get an iommu_domain for
> + * @flags: Flags from userspace
> + * @hwpt_type: Requested type of hw_pagetable
> + * @user_data: user_data pointer
> + * @dummy: never used
> + *
> + * Allocate a new iommu_domain (must be IOMMU_DOMAIN_NESTED) and
> return it as
> + * a user-managed hw_pagetable.

Add some text to highlight the requirement being a parent, e.g. not
an auto domain, must be capable of being a parent, etc.

> +	case IOMMUFD_OBJ_HW_PAGETABLE:
> +		parent = container_of(pt_obj, struct iommufd_hw_pagetable,
> obj);
> +		/* No user-managed HWPT on top of an user-managed one
> */
> +		if (parent->user_managed) {
> +			rc = -EINVAL;
> +			goto out_put_pt;
> +		}

move to alloc_fn().

Nicolin Chen Oct. 14, 2023, 12:08 a.m. UTC | #2

On Tue, Sep 26, 2023 at 01:14:47AM -0700, Tian, Kevin wrote:
> > From: Liu, Yi L <[e-mail]>
> > Sent: Thursday, September 21, 2023 3:51 PM
> >
> > +static void iommufd_user_managed_hwpt_abort(struct iommufd_object
> > *obj)
> > +{
> > +     struct iommufd_hw_pagetable *hwpt =
> > +             container_of(obj, struct iommufd_hw_pagetable, obj);
> > +
> > +     /* The parent->mutex must be held until finalize is called. */
> > +     lockdep_assert_held(&hwpt->parent->mutex);
> > +
> > +     iommufd_hw_pagetable_destroy(obj);
> > +}
> 
> Can you elaborate what exactly is protected by parent->mutex?
> 
> My gut-feeling that the child just grabs a refcnt on the parent
> object. It doesn't change any other data of the parent.

Ah, you are right. It's added here just for symmetry so we wouldn't
end up with something like:
	if (!hwpt->user_managed)
		mutex_lock(&hwpt->mutex);
	alloc_fn();
	if (!hwpt->user_managed)
		mutex_unlock(&hwpt->mutex);

Perhaps I should move the pair of mutex calls to the kernel-managed
hwpt allocator.

> > +/**
> > + * iommufd_user_managed_hwpt_alloc() - Get a user-managed
> > hw_pagetable
> > + * @ictx: iommufd context
> > + * @pt_obj: Parent object to an HWPT to associate the domain with
> > + * @idev: Device to get an iommu_domain for
> > + * @flags: Flags from userspace
> > + * @hwpt_type: Requested type of hw_pagetable
> > + * @user_data: user_data pointer
> > + * @dummy: never used
> > + *
> > + * Allocate a new iommu_domain (must be IOMMU_DOMAIN_NESTED) and
> > return it as
> > + * a user-managed hw_pagetable.
> 
> Add some text to highlight the requirement being a parent, e.g. not
> an auto domain, must be capable of being a parent, etc.

OK.

> > +     case IOMMUFD_OBJ_HW_PAGETABLE:
> > +             parent = container_of(pt_obj, struct iommufd_hw_pagetable,
> > obj);
> > +             /* No user-managed HWPT on top of an user-managed one
> > */
> > +             if (parent->user_managed) {
> > +                     rc = -EINVAL;
> > +                     goto out_put_pt;
> > +             }
> 
> move to alloc_fn().

Though being a bit covert, this is actually to avoid a data buffer
allocation in the common pathway before calling alloc_fn(), which
is added in the following patch. And the reason why it's in the
common function is because we previously support a kernel-managed
hwpt allocation with data too.

But now, I think we can just move this sanity and data allocation
together into the user-managed hwpt allocator.

Thanks
Nicolin

Patch

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index b2af68776877..dc3e11a23acf 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -8,6 +8,17 @@ 
 #include "../iommu-priv.h"
 #include "iommufd_private.h"
 
+static void iommufd_user_managed_hwpt_destroy(struct iommufd_object *obj)
+{
+	struct iommufd_hw_pagetable *hwpt =
+		container_of(obj, struct iommufd_hw_pagetable, obj);
+
+	if (hwpt->domain)
+		iommu_domain_free(hwpt->domain);
+
+	refcount_dec(&hwpt->parent->obj.users);
+}
+
 static void iommufd_kernel_managed_hwpt_destroy(struct iommufd_object *obj)
 {
 	struct iommufd_hw_pagetable *hwpt =
@@ -32,6 +43,17 @@  void iommufd_hw_pagetable_destroy(struct iommufd_object *obj)
 	container_of(obj, struct iommufd_hw_pagetable, obj)->destroy(obj);
 }
 
+static void iommufd_user_managed_hwpt_abort(struct iommufd_object *obj)
+{
+	struct iommufd_hw_pagetable *hwpt =
+		container_of(obj, struct iommufd_hw_pagetable, obj);
+
+	/* The parent->mutex must be held until finalize is called. */
+	lockdep_assert_held(&hwpt->parent->mutex);
+
+	iommufd_hw_pagetable_destroy(obj);
+}
+
 static void iommufd_kernel_managed_hwpt_abort(struct iommufd_object *obj)
 {
 	struct iommufd_hw_pagetable *hwpt =
@@ -52,6 +74,82 @@  void iommufd_hw_pagetable_abort(struct iommufd_object *obj)
 	container_of(obj, struct iommufd_hw_pagetable, obj)->abort(obj);
 }
 
+/**
+ * iommufd_user_managed_hwpt_alloc() - Get a user-managed hw_pagetable
+ * @ictx: iommufd context
+ * @pt_obj: Parent object to an HWPT to associate the domain with
+ * @idev: Device to get an iommu_domain for
+ * @flags: Flags from userspace
+ * @hwpt_type: Requested type of hw_pagetable
+ * @user_data: user_data pointer
+ * @dummy: never used
+ *
+ * Allocate a new iommu_domain (must be IOMMU_DOMAIN_NESTED) and return it as
+ * a user-managed hw_pagetable.
+ */
+static struct iommufd_hw_pagetable *
+iommufd_user_managed_hwpt_alloc(struct iommufd_ctx *ictx,
+				struct iommufd_object *pt_obj,
+				struct iommufd_device *idev,
+				u32 flags,
+				enum iommu_hwpt_type hwpt_type,
+				struct iommu_user_data *user_data,
+				bool dummy)
+{
+	struct iommufd_hw_pagetable *parent =
+		container_of(pt_obj, struct iommufd_hw_pagetable, obj);
+	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
+	struct iommufd_hw_pagetable *hwpt;
+	int rc;
+
+	if (!user_data)
+		return ERR_PTR(-EINVAL);
+	if (parent->auto_domain)
+		return ERR_PTR(-EINVAL);
+	if (!parent->nest_parent)
+		return ERR_PTR(-EINVAL);
+	if (hwpt_type == IOMMU_HWPT_TYPE_DEFAULT)
+		return ERR_PTR(-EINVAL);
+
+	if (!ops->domain_alloc_user)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	lockdep_assert_held(&parent->mutex);
+
+	hwpt = iommufd_object_alloc(ictx, hwpt, IOMMUFD_OBJ_HW_PAGETABLE);
+	if (IS_ERR(hwpt))
+		return hwpt;
+
+	refcount_inc(&parent->obj.users);
+	hwpt->parent = parent;
+	hwpt->user_managed = true;
+	hwpt->abort = iommufd_user_managed_hwpt_abort;
+	hwpt->destroy = iommufd_user_managed_hwpt_destroy;
+
+	hwpt->domain = ops->domain_alloc_user(idev->dev, flags, hwpt_type,
+					      parent->domain, user_data);
+	if (IS_ERR(hwpt->domain)) {
+		rc = PTR_ERR(hwpt->domain);
+		hwpt->domain = NULL;
+		goto out_abort;
+	}
+
+	if (WARN_ON_ONCE(hwpt->domain->type != IOMMU_DOMAIN_NESTED)) {
+		rc = -EINVAL;
+		goto out_abort;
+	}
+	/* Driver is buggy by missing cache_invalidate_user in domain_ops */
+	if (WARN_ON_ONCE(!hwpt->domain->ops->cache_invalidate_user)) {
+		rc = -EINVAL;
+		goto out_abort;
+	}
+	return hwpt;
+
+out_abort:
+	iommufd_object_abort_and_destroy(ictx, &hwpt->obj);
+	return ERR_PTR(rc);
+}
+
 int iommufd_hw_pagetable_enforce_cc(struct iommufd_hw_pagetable *hwpt)
 {
 	if (hwpt->enforce_cache_coherency)
@@ -112,10 +210,12 @@  iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx,
 	if (IS_ERR(hwpt))
 		return hwpt;
 
+	mutex_init(&hwpt->mutex);
 	INIT_LIST_HEAD(&hwpt->hwpt_item);
 	/* Pairs with iommufd_hw_pagetable_destroy() */
 	refcount_inc(&ioas->obj.users);
 	hwpt->ioas = ioas;
+	hwpt->nest_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
 	hwpt->abort = iommufd_kernel_managed_hwpt_abort;
 	hwpt->destroy = iommufd_kernel_managed_hwpt_destroy;
 
@@ -194,8 +294,8 @@  int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
 					u32 flags, enum iommu_hwpt_type type,
 					struct iommu_user_data *user_data,
 					bool flag);
+	struct iommufd_hw_pagetable *hwpt, *parent;
 	struct iommu_hwpt_alloc *cmd = ucmd->cmd;
-	struct iommufd_hw_pagetable *hwpt;
 	struct iommufd_object *pt_obj;
 	struct iommufd_device *idev;
 	struct iommufd_ioas *ioas;
@@ -221,6 +321,16 @@  int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
 		mutex = &ioas->mutex;
 		alloc_fn = iommufd_hw_pagetable_alloc;
 		break;
+	case IOMMUFD_OBJ_HW_PAGETABLE:
+		parent = container_of(pt_obj, struct iommufd_hw_pagetable, obj);
+		/* No user-managed HWPT on top of an user-managed one */
+		if (parent->user_managed) {
+			rc = -EINVAL;
+			goto out_put_pt;
+		}
+		mutex = &parent->mutex;
+		alloc_fn = iommufd_user_managed_hwpt_alloc;
+		break;
 	default:
 		rc = -EINVAL;
 		goto out_put_pt;
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index e4d06ae6b0c5..34940596c2c2 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -237,12 +237,18 @@  struct iommufd_hw_pagetable {
 	void (*abort)(struct iommufd_object *obj);
 	void (*destroy)(struct iommufd_object *obj);
 
+	bool user_managed : 1;
 	union {
+		struct { /* user-managed */
+			struct iommufd_hw_pagetable *parent;
+		};
 		struct { /* kernel-managed */
 			struct iommufd_ioas *ioas;
+			struct mutex mutex;
 			bool auto_domain : 1;
 			bool enforce_cache_coherency : 1;
 			bool msi_cookie : 1;
+			bool nest_parent : 1;
 			/* Head at iommufd_ioas::hwpt_list */
 			struct list_head hwpt_item;
 		};