Hi Jean,

A few comments inline. I am also trying to converge to the common sva
APIs. I sent out the first step w/o iopage fault and the generic ops
you have here.

On Mon, 24 Feb 2020 19:23:37 +0100
Jean-Philippe Brucker [off-list ref] wrote:

From: Jean-Philippe Brucker <redacted>

Add a small library to help IOMMU drivers manage process address
spaces bound to their devices. Register an MMU notifier to track
modification on each address space bound to one or more devices.

IOMMU drivers must implement the io_mm_ops and can then use the
helpers provided by this library to easily implement the SVA API
introduced by commit 26b25a2b98e4. The io_mm_ops are:

void *alloc(struct mm_struct *)
  Allocate a PASID context private to the IOMMU driver. There is a
  single context per mm. IOMMU drivers may perform arch-specific
  operations in there, for example pinning down a CPU ASID (on Arm).

int attach(struct device *, int pasid, void *ctx, bool attach_domain)
  Attach a context to the device, by setting up the PASID table entry.

int invalidate(struct device *, int pasid, void *ctx,
               unsigned long vaddr, size_t size)
  Invalidate TLB entries for this address range.

int detach(struct device *, int pasid, void *ctx, bool detach_domain)
  Detach a context from the device, by clearing the PASID table entry
  and invalidating cached entries.

void free(void *ctx)

you meant release()?

Hmm, devices in domain A has access to both X & Y, isn't it
contradictory?

Device
+ * 00:00.0 accesses address spaces X and Y, each corresponding to an
mm_struct.
+ * Devices 00:01.* only access address space Y. In addition each
+ * IOMMU_DOMAIN_DMA domain has a private address space, io_pgtable,
that is
+ * managed with iommu_map()/iommu_unmap(), and isn't shared with the
CPU MMU.

So this would allow IOVA and SVA co-exist in the same address space?
I guess this is the PASID 0 for DMA request w/o PASID. If that is the
case, perhaps needs more explanation since the private address space
also has a private PASID within the domain.

+ *
+ * To obtain the above configuration, users would for instance issue
the
+ * following calls:
+ *
+ *     iommu_sva_bind_device(dev 00:00.0, mm X, ...) -> bond 1
+ *     iommu_sva_bind_device(dev 00:00.0, mm Y, ...) -> bond 2
+ *     iommu_sva_bind_device(dev 00:01.0, mm Y, ...) -> bond 3
+ *     iommu_sva_bind_device(dev 00:01.1, mm Y, ...) -> bond 4
+ *
+ * A single Process Address Space ID (PASID) is allocated for each
mm. In the
+ * example, devices use PASID 1 to read/write into address space X
and PASID 2
+ * to read/write into address space Y. Calling iommu_sva_get_pasid()
on bond 1
+ * returns 1, and calling it on bonds 2-4 returns 2.
+ *
+ * Hardware tables describing this configuration in the IOMMU would
typically
+ * look like this:
+ *
+ *                                PASID tables
+ *                                 of domain A
+ *                              .->+--------+
+ *                             / 0 |        |-------> io_pgtable
+ *                            /    +--------+
+ *            Device tables  /   1 |        |-------> pgd X
+ *              +--------+  /      +--------+
+ *      00:00.0 |      A |-'     2 |        |--.
+ *              +--------+         +--------+   \
+ *              :        :       3 |        |    \
+ *              +--------+         +--------+     --> pgd Y
+ *      00:01.0 |      B |--.                    /
+ *              +--------+   \                  |
+ *      00:01.1 |      B |----+   PASID tables  |
+ *              +--------+     \   of domain B  |
+ *                              '->+--------+   |
+ *                               0 |        |-- | --> io_pgtable
+ *                                 +--------+   |
+ *                               1 |        |   |
+ *                                 +--------+   |
+ *                               2 |        |---'
+ *                                 +--------+
+ *                               3 |        |
+ *                                 +--------+
+ *
+ * With this model, a single call binds all devices in a given
domain to an
+ * address space. Other devices in the domain will get the same bond
implicitly.
+ * However, users must issue one bind() for each device, because
IOMMUs may
+ * implement SVA differently. Furthermore, mandating one bind() per
device
+ * allows the driver to perform sanity-checks on device capabilities.
+ *
+ * In some IOMMUs, one entry of the PASID table (typically the first
one) can
+ * hold non-PASID translations. In this case PASID 0 is reserved and
the first
+ * entry points to the io_pgtable pointer. In other IOMMUs the
io_pgtable
+ * pointer is held in the device table and PASID 0 is available to
the
+ * allocator.
+ */
+
+struct io_mm {
+	struct list_head		devices;
+	struct mm_struct		*mm;
+	struct mmu_notifier		notifier;
+
+	/* Late initialization */
+	const struct io_mm_ops		*ops;
+	void				*ctx;
+	int				pasid;
+};
+
+#define to_io_mm(mmu_notifier)	container_of(mmu_notifier,
struct io_mm, notifier) +#define to_iommu_bond(handle)
container_of(handle, struct iommu_bond, sva) +
+struct iommu_bond {
+	struct iommu_sva		sva;
+	struct io_mm __rcu		*io_mm;
+
+	struct list_head		mm_head;
+	void				*drvdata;
+	struct rcu_head			rcu_head;
+	refcount_t			refs;
+};
+
+static DECLARE_IOASID_SET(shared_pasid);
+
+static struct mmu_notifier_ops iommu_mmu_notifier_ops;
+
+/*
+ * Serializes modifications of bonds.
+ * Lock order: Device SVA mutex; global SVA mutex; IOASID lock
+ */
+static DEFINE_MUTEX(iommu_sva_lock);
+
+struct io_mm_alloc_params {
+	const struct io_mm_ops *ops;
+	int min_pasid, max_pasid;
+};
+
+static struct mmu_notifier *io_mm_alloc(struct mm_struct *mm, void
*privdata) +{
+	int ret;
+	struct io_mm *io_mm;
+	struct io_mm_alloc_params *params = privdata;
+
+	io_mm = kzalloc(sizeof(*io_mm), GFP_KERNEL);
+	if (!io_mm)
+		return ERR_PTR(-ENOMEM);
+
+	io_mm->mm = mm;
+	io_mm->ops = params->ops;
+	INIT_LIST_HEAD(&io_mm->devices);
+
+	io_mm->pasid = ioasid_alloc(&shared_pasid, params->min_pasid,
+				    params->max_pasid, io_mm->mm);
+	if (io_mm->pasid == INVALID_IOASID) {
+		ret = -ENOSPC;
+		goto err_free_io_mm;
+	}
+
+	io_mm->ctx = params->ops->alloc(mm);
+	if (IS_ERR(io_mm->ctx)) {
+		ret = PTR_ERR(io_mm->ctx);
+		goto err_free_pasid;
+	}
+	return &io_mm->notifier;
+
+err_free_pasid:
+	ioasid_free(io_mm->pasid);
+err_free_io_mm:
+	kfree(io_mm);
+	return ERR_PTR(ret);
+}
+
+static void io_mm_free(struct mmu_notifier *mn)
+{
+	struct io_mm *io_mm = to_io_mm(mn);
+
+	WARN_ON(!list_empty(&io_mm->devices));
+
+	io_mm->ops->release(io_mm->ctx);
+	ioasid_free(io_mm->pasid);
+	kfree(io_mm);
+}
+
+/*
+ * io_mm_get - Allocate an io_mm or get the existing one for the
given mm
+ * @mm: the mm
+ * @ops: callbacks for the IOMMU driver
+ * @min_pasid: minimum PASID value (inclusive)
+ * @max_pasid: maximum PASID value (inclusive)
+ *
+ * Returns a valid io_mm or an error pointer.
+ */
+static struct io_mm *io_mm_get(struct mm_struct *mm,
+			       const struct io_mm_ops *ops,
+			       int min_pasid, int max_pasid)
+{
+	struct io_mm *io_mm;
+	struct mmu_notifier *mn;
+	struct io_mm_alloc_params params = {
+		.ops		= ops,
+		.min_pasid	= min_pasid,
+		.max_pasid	= max_pasid,
+	};
+
+	/*
+	 * A single notifier can exist for this (ops, mm) pair.
Allocate it if
+	 * necessary.
+	 */
+	mn = mmu_notifier_get(&iommu_mmu_notifier_ops, mm, &params);
+	if (IS_ERR(mn))
+		return ERR_CAST(mn);
+	io_mm = to_io_mm(mn);
+
+	if (WARN_ON(io_mm->ops != ops)) {
+		mmu_notifier_put(mn);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return io_mm;
+}
+
+static void io_mm_put(struct io_mm *io_mm)
+{
+	mmu_notifier_put(&io_mm->notifier);
+}
+
+static struct iommu_sva *
+io_mm_attach(struct device *dev, struct io_mm *io_mm, void *drvdata)
+{
+	int ret = 0;
+	bool attach_domain = true;
+	struct iommu_bond *bond, *tmp;
+	struct iommu_domain *domain, *other;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	domain = iommu_get_domain_for_dev(dev);
+
+	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
+	if (!bond)
+		return ERR_PTR(-ENOMEM);
+
+	bond->sva.dev	= dev;
+	bond->drvdata	= drvdata;
+	refcount_set(&bond->refs, 1);
+	RCU_INIT_POINTER(bond->io_mm, io_mm);
+
+	mutex_lock(&iommu_sva_lock);
+	/* Is it already bound to the device or domain? */
+	list_for_each_entry(tmp, &io_mm->devices, mm_head) {
+		if (tmp->sva.dev != dev) {
+			other =
iommu_get_domain_for_dev(tmp->sva.dev);
+			if (domain == other)
+				attach_domain = false;
+
+			continue;

At this point, we already know this is a new device trying to attach to
one of io_mm's existing domains. So there is no need to continue
checking, right? Perhaps check like this?
-               if (tmp->sva.dev != dev) {
+               if (tmp->sva.dev != dev && attach_domain) {

+		}
+
+		if (WARN_ON(tmp->drvdata != drvdata)) {
+			ret = -EINVAL;
+			goto err_free;
+		}
+
+		/*
+		 * Hold a single io_mm reference per bond. Note that
we can't
+		 * return an error after this, otherwise the caller
would drop
+		 * an additional reference to the io_mm.
+		 */
+		refcount_inc(&tmp->refs);
+		io_mm_put(io_mm);
+		kfree(bond);

Can bond be allocated after searching for existing bond or domain? If
so, we can avoid free bond here.

+		mutex_unlock(&iommu_sva_lock);
+		return &tmp->sva;
+	}
+
+	list_add_rcu(&bond->mm_head, &io_mm->devices);
+	param->nr_bonds++;
+	mutex_unlock(&iommu_sva_lock);
+
+	ret = io_mm->ops->attach(bond->sva.dev, io_mm->pasid,
io_mm->ctx,
+				 attach_domain);

For VT-d, if a device trying to do SVA bind, there would not be a DMA
domain. SVA should own the entire address space, no IOVA. So this
attach() call is for VT-d driver to setup the first PASID table entry
regardless attach_domain is true or false?

+	if (ret)
+		goto err_remove;
+
+	return &bond->sva;
+
+err_remove:
+	/*
+	 * At this point concurrent threads may have started to
access the
+	 * io_mm->devices list in order to invalidate address
ranges, which
+	 * requires to free the bond via kfree_rcu()
+	 */
+	mutex_lock(&iommu_sva_lock);
+	param->nr_bonds--;
+	list_del_rcu(&bond->mm_head);
+
+err_free:
+	mutex_unlock(&iommu_sva_lock);
+	kfree_rcu(bond, rcu_head);
+	return ERR_PTR(ret);
+}
+
+static void io_mm_detach_locked(struct iommu_bond *bond)
+{
+	struct io_mm *io_mm;
+	struct iommu_bond *tmp;
+	bool detach_domain = true;
+	struct iommu_domain *domain, *other;
+
+	io_mm = rcu_dereference_protected(bond->io_mm,
+
lockdep_is_held(&iommu_sva_lock));
+	if (!io_mm)
+		return;
+
+	domain = iommu_get_domain_for_dev(bond->sva.dev);
+
+	/* Are other devices in the same domain still attached to
this mm? */
+	list_for_each_entry(tmp, &io_mm->devices, mm_head) {
+		if (tmp == bond)
+			continue;
+		other = iommu_get_domain_for_dev(tmp->sva.dev);
+		if (domain == other) {
+			detach_domain = false;
+			break;
+		}
+	}
+
+	io_mm->ops->detach(bond->sva.dev, io_mm->pasid, io_mm->ctx,
+			   detach_domain);
+
+	list_del_rcu(&bond->mm_head);
+	RCU_INIT_POINTER(bond->io_mm, NULL);
+
+	/* Free after RCU grace period */
+	io_mm_put(io_mm);
+}
+
+/*
+ * io_mm_release - release MMU notifier
+ *
+ * Called when the mm exits. Some devices may still be bound to the
io_mm. A few
+ * things need to be done before it is safe to release:
+ *
+ * - Tell the device driver to stop using this PASID.
+ * - Clear the PASID table and invalidate TLBs.
+ * - Drop all references to this io_mm.
+ */
+static void io_mm_release(struct mmu_notifier *mn, struct mm_struct
*mm) +{
+	struct iommu_bond *bond, *next;
+	struct io_mm *io_mm = to_io_mm(mn);
+
+	mutex_lock(&iommu_sva_lock);
+	list_for_each_entry_safe(bond, next, &io_mm->devices,
mm_head) {
+		struct device *dev = bond->sva.dev;
+		struct iommu_sva *sva = &bond->sva;
+
+		if (sva->ops && sva->ops->mm_exit &&
+		    sva->ops->mm_exit(dev, sva, bond->drvdata))
+			dev_WARN(dev, "possible leak of PASID %u",
+				 io_mm->pasid);
+
+		/* unbind() frees the bond, we just detach it */
+		io_mm_detach_locked(bond);
+	}
+	mutex_unlock(&iommu_sva_lock);
+}
+
+static void io_mm_invalidate_range(struct mmu_notifier *mn,
+				   struct mm_struct *mm, unsigned
long start,
+				   unsigned long end)
+{
+	struct iommu_bond *bond;
+	struct io_mm *io_mm = to_io_mm(mn);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(bond, &io_mm->devices, mm_head)
+		io_mm->ops->invalidate(bond->sva.dev, io_mm->pasid,
io_mm->ctx,
+				       start, end - start);
+	rcu_read_unlock();
+}
+
+static struct mmu_notifier_ops iommu_mmu_notifier_ops = {
+	.alloc_notifier		= io_mm_alloc,
+	.free_notifier		= io_mm_free,
+	.release		= io_mm_release,
+	.invalidate_range	= io_mm_invalidate_range,
+};
+
+struct iommu_sva *
+iommu_sva_bind_generic(struct device *dev, struct mm_struct *mm,
+		       const struct io_mm_ops *ops, void *drvdata)
+{
+	struct io_mm *io_mm;
+	struct iommu_sva *handle;
+	struct iommu_param *param = dev->iommu_param;
+
+	if (!param)
+		return ERR_PTR(-ENODEV);
+
+	mutex_lock(&param->sva_lock);
+	if (!param->sva_param) {
+		handle = ERR_PTR(-ENODEV);
+		goto out_unlock;
+	}
+
+	io_mm = io_mm_get(mm, ops, param->sva_param->min_pasid,
+			  param->sva_param->max_pasid);
+	if (IS_ERR(io_mm)) {
+		handle = ERR_CAST(io_mm);
+		goto out_unlock;
+	}
+
+	handle = io_mm_attach(dev, io_mm, drvdata);
+	if (IS_ERR(handle))
+		io_mm_put(io_mm);
+
+out_unlock:
+	mutex_unlock(&param->sva_lock);
+	return handle;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_generic);
+
+static void iommu_sva_unbind_locked(struct iommu_bond *bond)
+{
+	struct device *dev = bond->sva.dev;
+	struct iommu_sva_param *param = dev->iommu_param->sva_param;
+
+	if (!refcount_dec_and_test(&bond->refs))
+		return;
+

dont you need to free bond here?

+	io_mm_detach_locked(bond);
+	param->nr_bonds--;
+	kfree_rcu(bond, rcu_head);
+}
+
+void iommu_sva_unbind_generic(struct iommu_sva *handle)
+{
+	struct iommu_param *param = handle->dev->iommu_param;
+
+	if (WARN_ON(!param))
+		return;
+
+	mutex_lock(&param->sva_lock);
+	mutex_lock(&iommu_sva_lock);
+	iommu_sva_unbind_locked(to_iommu_bond(handle));
+	mutex_unlock(&iommu_sva_lock);
+	mutex_unlock(&param->sva_lock);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_generic);
+
+/**
+ * iommu_sva_enable() - Enable Shared Virtual Addressing for a device
+ * @dev: the device
+ * @sva_param: the parameters.
+ *
+ * Called by an IOMMU driver to setup the SVA parameters
+ * @sva_param is duplicated and can be freed when this function
returns.
+ *
+ * Return 0 if initialization succeeded, or an error.
+ */

IOMMU vendor driver usually dont know when the device SVA feature will
be used until bind call. So we pretty much have to call this for every
device during init time?

If we have per device PASID table, then we need to set up PASID table
entry regardless of the domain sharing. What is confusing to me is that
domain is for DMA isolation on request w/o PASID, but with SVA we don't
really care about domains. Sorry, it has been a long time since we
discussed this. I think will work for VT-d but just wanted to make sure
I understand the intentions.

Thanks,

Jacob