Hi folks,
The former part of this series refactors the IOMMU SVA code by assigning
an SVA type of iommu_domain to a shared virtual address and replacing
sva_bind/unbind iommu ops with attach/detach_dev_pasid domain ops.
The latter part changes the existing I/O page fault handling framework
from only serving SVA to a generic one. Any driver or component could
handle the I/O page faults for its domain in its own way by installing
an I/O page fault handler.
This series has been functionally tested on an x86 machine and compile
tested for other architectures.
This series is also available on github:
[2] https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v6
Please review and suggest.
Best regards,
baolu
Change log:
v6:
- Refine the SVA basic data structures.
Link: https://lore.kernel.org/linux-iommu/YnFv0ps0Ad8v+7uH@myrica/
- Refine arm smmuv3 sva domain allocation.
- Fix a possible lock issue.
Link: https://lore.kernel.org/linux-iommu/YnFydE8j8l7Q4m+b@myrica/
v5:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Address review comments from Jean-Philippe Brucker. Very appreciated!
- Remove redundant pci aliases check in
device_group_immutable_singleton().
- Treat all buses exept PCI as static in immutable singleton check.
- As the sva_bind/unbind() have already guaranteed sva domain free only
after iopf_queue_flush_dev(), remove the unnecessary domain refcount.
- Move domain get() out of the list iteration in iopf_handle_group().
v4:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Solve the overlap with another series and make this series
self-contained.
- No objection to the abstraction of data structure during v3 review.
Hence remove the RFC subject prefix.
- Refine the immutable singleton group code according to Kevin's
comments.
v3:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Rework iommu_group_singleton_lockdown() by adding a flag to the group
that positively indicates the group can never have more than one
member, even after hot plug.
- Abstract the data structs used for iommu sva in a separated patches to
make it easier for review.
- I still keep the RFC prefix in this series as above two significant
changes need at least another round review to be finalized.
- Several misc refinements.
v2:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Add sva domain life cycle management to avoid race between unbind and
page fault handling.
- Use a single domain for each mm.
- Return a single sva handler for the same binding.
- Add a new helper to meet singleton group requirement.
- Rework the SVA domain allocation for arm smmu v3 driver and move the
pasid_bit initialization to device probe.
- Drop the patch "iommu: Handle IO page faults directly".
- Add mmget_not_zero(mm) in SVA page fault handler.
v1:
- https://lore.kernel.org/linux-iommu/[email protected]/
- Initial post.
Dave Jiang (1):
dmaengine: idxd: Separate user and kernel pasid enabling
Lu Baolu (11):
iommu: Add pasid_bits field in struct dev_iommu
iommu: Add attach/detach_dev_pasid domain ops
iommu/sva: Basic data structures for SVA
iommu/vt-d: Remove SVM_FLAG_SUPERVISOR_MODE support
iommu/vt-d: Add SVA domain support
arm-smmu-v3/sva: Add SVA domain support
iommu/sva: Use attach/detach_pasid_dev in SVA interfaces
iommu: Remove SVA related callbacks from iommu ops
iommu: Prepare IOMMU domain for IOPF
iommu: Per-domain I/O page fault handling
iommu: Rename iommu-sva-lib.{c,h}
include/linux/intel-iommu.h | 9 +-
include/linux/iommu.h | 99 +++++--
drivers/dma/idxd/idxd.h | 6 +
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 19 +-
.../iommu/{iommu-sva-lib.h => iommu-sva.h} | 3 -
drivers/dma/idxd/cdev.c | 4 +-
drivers/dma/idxd/init.c | 30 +-
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 111 +++++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 10 +-
drivers/iommu/intel/iommu.c | 12 +-
drivers/iommu/intel/svm.c | 146 ++++------
drivers/iommu/io-pgfault.c | 73 +----
drivers/iommu/iommu-sva-lib.c | 71 -----
drivers/iommu/iommu-sva.c | 261 ++++++++++++++++++
drivers/iommu/iommu.c | 193 +++++++------
drivers/iommu/Makefile | 2 +-
16 files changed, 623 insertions(+), 426 deletions(-)
rename drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} (91%)
delete mode 100644 drivers/iommu/iommu-sva-lib.c
create mode 100644 drivers/iommu/iommu-sva.c
--
2.25.1
From: Dave Jiang <[email protected]>
The idxd driver always gated the pasid enabling under a single knob and
this assumption is incorrect. The pasid used for kernel operation can be
independently toggled and has no dependency on the user pasid (and vice
versa). Split the two so they are independent "enabled" flags.
Cc: Vinod Koul <[email protected]>
Signed-off-by: Dave Jiang <[email protected]>
Signed-off-by: Jacob Pan <[email protected]>
Link: https://lore.kernel.org/linux-iommu/[email protected]/
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/dma/idxd/idxd.h | 6 ++++++
drivers/dma/idxd/cdev.c | 4 ++--
drivers/dma/idxd/init.c | 30 ++++++++++++++++++------------
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index da72eb15f610..ccbefd0be617 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -239,6 +239,7 @@ enum idxd_device_flag {
IDXD_FLAG_CONFIGURABLE = 0,
IDXD_FLAG_CMD_RUNNING,
IDXD_FLAG_PASID_ENABLED,
+ IDXD_FLAG_USER_PASID_ENABLED,
};
struct idxd_dma_dev {
@@ -469,6 +470,11 @@ static inline bool device_pasid_enabled(struct idxd_device *idxd)
return test_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
}
+static inline bool device_user_pasid_enabled(struct idxd_device *idxd)
+{
+ return test_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
+}
+
static inline bool device_swq_supported(struct idxd_device *idxd)
{
return (support_enqcmd && device_pasid_enabled(idxd));
diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index b9b2b4a4124e..7df996deffbe 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -99,7 +99,7 @@ static int idxd_cdev_open(struct inode *inode, struct file *filp)
ctx->wq = wq;
filp->private_data = ctx;
- if (device_pasid_enabled(idxd)) {
+ if (device_user_pasid_enabled(idxd)) {
sva = iommu_sva_bind_device(dev, current->mm, NULL);
if (IS_ERR(sva)) {
rc = PTR_ERR(sva);
@@ -152,7 +152,7 @@ static int idxd_cdev_release(struct inode *node, struct file *filep)
if (wq_shared(wq)) {
idxd_device_drain_pasid(idxd, ctx->pasid);
} else {
- if (device_pasid_enabled(idxd)) {
+ if (device_user_pasid_enabled(idxd)) {
/* The wq disable in the disable pasid function will drain the wq */
rc = idxd_wq_disable_pasid(wq);
if (rc < 0)
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 993a5dcca24f..e1b5d1e4a949 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -513,16 +513,19 @@ static int idxd_probe(struct idxd_device *idxd)
if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && sva) {
rc = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA);
- if (rc == 0) {
- rc = idxd_enable_system_pasid(idxd);
- if (rc < 0) {
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
- dev_warn(dev, "Failed to enable PASID. No SVA support: %d\n", rc);
- } else {
- set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
- }
- } else {
+ if (rc) {
+ /*
+ * Do not bail here since legacy DMA is still
+ * supported, both user and in-kernel DMA with
+ * PASID rely on SVA feature.
+ */
dev_warn(dev, "Unable to turn on SVA feature.\n");
+ } else {
+ set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
+ if (idxd_enable_system_pasid(idxd))
+ dev_warn(dev, "No in-kernel DMA with PASID.\n");
+ else
+ set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
}
} else if (!sva) {
dev_warn(dev, "User forced SVA off via module param.\n");
@@ -561,7 +564,8 @@ static int idxd_probe(struct idxd_device *idxd)
err:
if (device_pasid_enabled(idxd))
idxd_disable_system_pasid(idxd);
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
return rc;
}
@@ -574,7 +578,8 @@ static void idxd_cleanup(struct idxd_device *idxd)
idxd_cleanup_internals(idxd);
if (device_pasid_enabled(idxd))
idxd_disable_system_pasid(idxd);
- iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
}
static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -691,7 +696,8 @@ static void idxd_remove(struct pci_dev *pdev)
free_irq(irq_entry->vector, irq_entry);
pci_free_irq_vectors(pdev);
pci_iounmap(pdev, idxd->reg_base);
- iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
+ if (device_user_pasid_enabled(idxd) || device_pasid_enabled(idxd))
+ iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
pci_disable_device(pdev);
destroy_workqueue(idxd->wq);
perfmon_pmu_remove(idxd);
--
2.25.1
The existing iommu SVA interfaces are implemented by calling the SVA
specific iommu ops provided by the IOMMU drivers. There's no need for
any SVA specific ops in iommu_ops vector anymore as we can achieve
this through the generic attach/detach_dev_pasid domain ops.
This refactors the IOMMU SVA interfaces implementation by using the
attach/detach_pasid_dev ops and align them with the concept of the
iommu domain. Put the new SVA code in the sva related file in order
to make it self-contained.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 44 ++++++-----
drivers/iommu/iommu-sva-lib.c | 145 ++++++++++++++++++++++++++++++++++
drivers/iommu/iommu.c | 92 ---------------------
3 files changed, 168 insertions(+), 113 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2921e634491e..5a3ef4d58b1f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -684,12 +684,6 @@ int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features f);
-struct iommu_sva *iommu_sva_bind_device(struct device *dev,
- struct mm_struct *mm,
- void *drvdata);
-void iommu_sva_unbind_device(struct iommu_sva *handle);
-u32 iommu_sva_get_pasid(struct iommu_sva *handle);
-
int iommu_device_use_default_domain(struct device *dev);
void iommu_device_unuse_default_domain(struct device *dev);
@@ -1031,21 +1025,6 @@ iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
return -ENODEV;
}
-static inline struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- return NULL;
-}
-
-static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
-{
-}
-
-static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- return IOMMU_PASID_INVALID;
-}
-
static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
{
return NULL;
@@ -1087,6 +1066,29 @@ static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
}
#endif /* CONFIG_IOMMU_API */
+#ifdef CONFIG_IOMMU_SVA
+struct iommu_sva *iommu_sva_bind_device(struct device *dev,
+ struct mm_struct *mm,
+ void *drvdata);
+void iommu_sva_unbind_device(struct iommu_sva *handle);
+u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+#else /* CONFIG_IOMMU_SVA */
+static inline struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ return NULL;
+}
+
+static inline void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+}
+
+static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ return IOMMU_PASID_INVALID;
+}
+#endif /* CONFIG_IOMMU_SVA */
+
/**
* iommu_map_sgtable - Map the given buffer to the IOMMU domain
* @domain: The IOMMU domain to perform the mapping
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 106506143896..e7301514f286 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -3,6 +3,8 @@
* Helpers for IOMMU drivers implementing SVA
*/
#include <linux/mutex.h>
+#include <linux/iommu.h>
+#include <linux/slab.h>
#include <linux/sched/mm.h>
#include "iommu-sva-lib.h"
@@ -69,3 +71,146 @@ struct mm_struct *iommu_sva_find(ioasid_t pasid)
return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
}
EXPORT_SYMBOL_GPL(iommu_sva_find);
+
+/*
+ * IOMMU SVA driver-oriented interfaces
+ */
+static struct iommu_domain *
+iommu_sva_alloc_domain(struct device *dev, struct mm_struct *mm)
+{
+ struct bus_type *bus = dev->bus;
+ struct iommu_domain *domain;
+
+ if (!bus || !bus->iommu_ops)
+ return NULL;
+
+ domain = bus->iommu_ops->domain_alloc(IOMMU_DOMAIN_SVA);
+ if (!domain)
+ return NULL;
+
+ mmgrab(mm);
+ domain->mm = mm;
+ domain->type = IOMMU_DOMAIN_SVA;
+
+ return domain;
+}
+
+static void iommu_sva_free_domain(struct iommu_domain *domain)
+{
+ mmdrop(domain->mm);
+ iommu_domain_free(domain);
+}
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to mm_users
+ * @drvdata: opaque data pointer to pass to bind callback
+ *
+ * Create a bond between device and address space, allowing the device to access
+ * the mm using the returned PASID. If a bond already exists between @device and
+ * @mm, it is returned and an additional reference is taken. Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ int ret = -EINVAL;
+ struct iommu_sva *handle;
+ struct iommu_domain *domain;
+
+ /*
+ * TODO: Remove the drvdata parameter after kernel PASID support is
+ * enabled for the idxd driver.
+ */
+ if (drvdata)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ /* Allocate mm->pasid if necessary. */
+ ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
+ if (ret)
+ return ERR_PTR(ret);
+
+ mutex_lock(&iommu_sva_lock);
+ /* Search for an existing bond. */
+ handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
+ if (handle) {
+ refcount_inc(&handle->users);
+ goto out_success;
+ }
+
+ handle = kzalloc(sizeof(*handle), GFP_KERNEL);
+ if (!handle) {
+ ret = -ENOMEM;
+ goto out_unlock;
+ }
+
+ domain = iommu_sva_alloc_domain(dev, mm);
+ if (!domain) {
+ ret = -ENODEV;
+ goto out_free_bond;
+ }
+
+ ret = iommu_attach_device_pasid(domain, dev, mm->pasid);
+ if (ret)
+ goto out_free_domain;
+
+ handle->dev = dev;
+ handle->domain = domain;
+ refcount_set(&handle->users, 1);
+ ret = xa_err(xa_store(&dev->iommu->sva_bonds, mm->pasid,
+ handle, GFP_KERNEL));
+ if (ret)
+ goto out_detach_domain;
+
+out_success:
+ mutex_unlock(&iommu_sva_lock);
+ return handle;
+
+out_detach_domain:
+ iommu_detach_device_pasid(domain, dev, mm->pasid);
+out_free_domain:
+ iommu_sva_free_domain(domain);
+out_free_bond:
+ kfree(handle);
+out_unlock:
+ mutex_unlock(&iommu_sva_lock);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @handle: the handle returned by iommu_sva_bind_device()
+ *
+ * Put reference to a bond between device and address space. The device should
+ * not be issuing any more transaction for this PASID. All outstanding page
+ * requests for this PASID must have been flushed to the IOMMU.
+ */
+void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+ struct device *dev = handle->dev;
+ struct iommu_domain *domain = handle->domain;
+ ioasid_t pasid = iommu_sva_get_pasid(handle);
+
+ mutex_lock(&iommu_sva_lock);
+ if (refcount_dec_and_test(&handle->users)) {
+ xa_erase(&dev->iommu->sva_bonds, pasid);
+ iommu_detach_device_pasid(domain, dev, pasid);
+ iommu_sva_free_domain(domain);
+ kfree(handle);
+ }
+ mutex_unlock(&iommu_sva_lock);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
+
+u32 iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ return handle->domain->mm->pasid;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 1abff5fc9554..367d0ecf6e12 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2734,98 +2734,6 @@ bool iommu_dev_feature_enabled(struct device *dev, enum iommu_dev_features feat)
}
EXPORT_SYMBOL_GPL(iommu_dev_feature_enabled);
-/**
- * iommu_sva_bind_device() - Bind a process address space to a device
- * @dev: the device
- * @mm: the mm to bind, caller must hold a reference to it
- * @drvdata: opaque data pointer to pass to bind callback
- *
- * Create a bond between device and address space, allowing the device to access
- * the mm using the returned PASID. If a bond already exists between @device and
- * @mm, it is returned and an additional reference is taken. Caller must call
- * iommu_sva_unbind_device() to release each reference.
- *
- * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
- * initialize the required SVA features.
- *
- * On error, returns an ERR_PTR value.
- */
-struct iommu_sva *
-iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct iommu_group *group;
- struct iommu_sva *handle = ERR_PTR(-EINVAL);
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_bind)
- return ERR_PTR(-ENODEV);
-
- group = iommu_group_get(dev);
- if (!group)
- return ERR_PTR(-ENODEV);
-
- /* Ensure device count and domain don't change while we're binding */
- mutex_lock(&group->mutex);
-
- /*
- * To keep things simple, SVA currently doesn't support IOMMU groups
- * with more than one device. Existing SVA-capable systems are not
- * affected by the problems that required IOMMU groups (lack of ACS
- * isolation, device ID aliasing and other hardware issues).
- */
- if (iommu_group_device_count(group) != 1)
- goto out_unlock;
-
- handle = ops->sva_bind(dev, mm, drvdata);
-
-out_unlock:
- mutex_unlock(&group->mutex);
- iommu_group_put(group);
-
- return handle;
-}
-EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
-
-/**
- * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
- * @handle: the handle returned by iommu_sva_bind_device()
- *
- * Put reference to a bond between device and address space. The device should
- * not be issuing any more transaction for this PASID. All outstanding page
- * requests for this PASID must have been flushed to the IOMMU.
- */
-void iommu_sva_unbind_device(struct iommu_sva *handle)
-{
- struct iommu_group *group;
- struct device *dev = handle->dev;
- const struct iommu_ops *ops = dev_iommu_ops(dev);
-
- if (!ops->sva_unbind)
- return;
-
- group = iommu_group_get(dev);
- if (!group)
- return;
-
- mutex_lock(&group->mutex);
- ops->sva_unbind(handle);
- mutex_unlock(&group->mutex);
-
- iommu_group_put(group);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
-
-u32 iommu_sva_get_pasid(struct iommu_sva *handle)
-{
- const struct iommu_ops *ops = dev_iommu_ops(handle->dev);
-
- if (!ops->sva_get_pasid)
- return IOMMU_PASID_INVALID;
-
- return ops->sva_get_pasid(handle);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
-
/*
* Changes the default domain of an iommu group that has *only* one device
*
--
2.25.1
Tweak the I/O page fault handling framework to route the page faults to
the domain and call the page fault handler retrieved from the domain.
This makes the I/O page fault handling framework possible to serve more
usage scenarios as long as they have an IOMMU domain and install a page
fault handler in it. Some unused functions are also removed to avoid
dead code.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
---
drivers/iommu/iommu-sva-lib.h | 2 --
drivers/iommu/io-pgfault.c | 64 ++++-------------------------------
drivers/iommu/iommu-sva-lib.c | 20 -----------
3 files changed, 7 insertions(+), 79 deletions(-)
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
index 3420654c6e2f..74ce2e76321b 100644
--- a/drivers/iommu/iommu-sva-lib.h
+++ b/drivers/iommu/iommu-sva-lib.h
@@ -8,8 +8,6 @@
#include <linux/ioasid.h>
#include <linux/mm_types.h>
-struct mm_struct *iommu_sva_find(ioasid_t pasid);
-
/* I/O Page fault */
struct device;
struct iommu_fault;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index aee9e033012f..9efe5259402b 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -69,69 +69,18 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
return iommu_page_response(dev, &resp);
}
-static enum iommu_page_response_code
-iopf_handle_single(struct iopf_fault *iopf)
-{
- vm_fault_t ret;
- struct mm_struct *mm;
- struct vm_area_struct *vma;
- unsigned int access_flags = 0;
- unsigned int fault_flags = FAULT_FLAG_REMOTE;
- struct iommu_fault_page_request *prm = &iopf->fault.prm;
- enum iommu_page_response_code status = IOMMU_PAGE_RESP_INVALID;
-
- if (!(prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID))
- return status;
-
- mm = iommu_sva_find(prm->pasid);
- if (IS_ERR_OR_NULL(mm))
- return status;
-
- mmap_read_lock(mm);
-
- vma = find_extend_vma(mm, prm->addr);
- if (!vma)
- /* Unmapped area */
- goto out_put_mm;
-
- if (prm->perm & IOMMU_FAULT_PERM_READ)
- access_flags |= VM_READ;
-
- if (prm->perm & IOMMU_FAULT_PERM_WRITE) {
- access_flags |= VM_WRITE;
- fault_flags |= FAULT_FLAG_WRITE;
- }
-
- if (prm->perm & IOMMU_FAULT_PERM_EXEC) {
- access_flags |= VM_EXEC;
- fault_flags |= FAULT_FLAG_INSTRUCTION;
- }
-
- if (!(prm->perm & IOMMU_FAULT_PERM_PRIV))
- fault_flags |= FAULT_FLAG_USER;
-
- if (access_flags & ~vma->vm_flags)
- /* Access fault */
- goto out_put_mm;
-
- ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
- status = ret & VM_FAULT_ERROR ? IOMMU_PAGE_RESP_INVALID :
- IOMMU_PAGE_RESP_SUCCESS;
-
-out_put_mm:
- mmap_read_unlock(mm);
- mmput(mm);
-
- return status;
-}
-
static void iopf_handle_group(struct work_struct *work)
{
struct iopf_group *group;
+ struct iommu_domain *domain;
struct iopf_fault *iopf, *next;
enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;
group = container_of(work, struct iopf_group, work);
+ domain = iommu_get_domain_for_iopf(group->dev,
+ group->last_fault.fault.prm.pasid);
+ if (!domain || !domain->iopf_handler)
+ status = IOMMU_PAGE_RESP_INVALID;
list_for_each_entry_safe(iopf, next, &group->faults, list) {
/*
@@ -139,7 +88,8 @@ static void iopf_handle_group(struct work_struct *work)
* faults in the group if there is an error.
*/
if (status == IOMMU_PAGE_RESP_SUCCESS)
- status = iopf_handle_single(iopf);
+ status = domain->iopf_handler(&iopf->fault,
+ domain->fault_data);
if (!(iopf->fault.prm.flags &
IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index 32c836e4a60e..ea12504a9e12 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -52,26 +52,6 @@ static int iommu_sva_alloc_pasid(struct mm_struct *mm,
return ret;
}
-/* ioasid_find getter() requires a void * argument */
-static bool __mmget_not_zero(void *mm)
-{
- return mmget_not_zero(mm);
-}
-
-/**
- * iommu_sva_find() - Find mm associated to the given PASID
- * @pasid: Process Address Space ID assigned to the mm
- *
- * On success a reference to the mm is taken, and must be released with mmput().
- *
- * Returns the mm corresponding to this PASID, or an error if not found.
- */
-struct mm_struct *iommu_sva_find(ioasid_t pasid)
-{
- return ioasid_find(&iommu_sva_pasid, pasid, __mmget_not_zero);
-}
-EXPORT_SYMBOL_GPL(iommu_sva_find);
-
/*
* I/O page fault handler for SVA
*
--
2.25.1
Use below data structures for SVA implementation in the IOMMU core:
- struct iommu_domain (IOMMU_DOMAIN_SVA type)
Represent a hardware pagetable that the IOMMU hardware could use for
SVA translation. Multiple iommu domains could be bound with an SVA mm
and each grabs a mm_count of the mm in order to make sure mm could
only be freed after all domains have been unbound. A new mm field is
added to struct iommu_domain and a helper is added to retrieve mm from
a domain pointer.
- struct iommu_sva (existing)
Represent a bond relationship between an SVA ioas and an iommu domain.
If a bond already exists, it's reused and a reference is taken.
- struct dev_iommu::sva_bonds
A pasid-indexed xarray to track the bonds happened on the device.
Suggested-by: Jean-Philippe Brucker <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 13 +++++++++++++
drivers/iommu/iommu.c | 3 +++
2 files changed, 16 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ab36244d4e94..2921e634491e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -64,6 +64,9 @@ struct iommu_domain_geometry {
#define __IOMMU_DOMAIN_PT (1U << 2) /* Domain is identity mapped */
#define __IOMMU_DOMAIN_DMA_FQ (1U << 3) /* DMA-API uses flush queue */
+#define __IOMMU_DOMAIN_SHARED (1U << 4) /* Page table shared from CPU */
+#define __IOMMU_DOMAIN_HOST_VA (1U << 5) /* Host CPU virtual address */
+
/*
* This are the possible domain-types
*
@@ -86,6 +89,8 @@ struct iommu_domain_geometry {
#define IOMMU_DOMAIN_DMA_FQ (__IOMMU_DOMAIN_PAGING | \
__IOMMU_DOMAIN_DMA_API | \
__IOMMU_DOMAIN_DMA_FQ)
+#define IOMMU_DOMAIN_SVA (__IOMMU_DOMAIN_SHARED | \
+ __IOMMU_DOMAIN_HOST_VA)
struct iommu_domain {
unsigned type;
@@ -95,6 +100,9 @@ struct iommu_domain {
void *handler_token;
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
+#ifdef CONFIG_IOMMU_SVA
+ struct mm_struct *mm;
+#endif
};
static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
@@ -380,6 +388,9 @@ struct dev_iommu {
struct iommu_device *iommu_dev;
void *priv;
unsigned int pasid_bits;
+#ifdef CONFIG_IOMMU_SVA
+ struct xarray sva_bonds;
+#endif
};
int iommu_device_register(struct iommu_device *iommu,
@@ -629,6 +640,8 @@ struct iommu_fwspec {
*/
struct iommu_sva {
struct device *dev;
+ struct iommu_domain *domain;
+ refcount_t users;
};
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 16e8db2d86fc..1abff5fc9554 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -202,6 +202,9 @@ static struct dev_iommu *dev_iommu_get(struct device *dev)
return NULL;
mutex_init(¶m->lock);
+#ifdef CONFIG_IOMMU_SVA
+ xa_init(¶m->sva_bonds);
+#endif
dev->iommu = param;
return param;
}
--
2.25.1
Attaching an IOMMU domain to a PASID of a device is a generic operation
for modern IOMMU drivers which support PASID-granular DMA address
translation. Currently visible usage scenarios include (but not limited):
- SVA (Shared Virtual Address)
- kernel DMA with PASID
- hardware-assist mediated device
This adds a pair of common domain ops for this purpose and adds helpers
to attach/detach a domain to/from a {device, PASID}. Some buses, like
PCI, route packets without considering the PASID value. Thus a DMA target
address with PASID might be treated as P2P if the address falls into the
MMIO BAR of other devices in the group. To make things simple, these
interfaces only apply to devices belonging to the singleton groups, and
the singleton is immutable in fabric i.e. not affected by hotplug.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
---
include/linux/iommu.h | 21 +++++++++++++
drivers/iommu/iommu.c | 71 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 92 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b8ffaf2cb1d0..ab36244d4e94 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -263,6 +263,8 @@ struct iommu_ops {
* struct iommu_domain_ops - domain specific operations
* @attach_dev: attach an iommu domain to a device
* @detach_dev: detach an iommu domain from a device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
* @map: map a physically contiguous memory region to an iommu domain
* @map_pages: map a physically contiguous set of pages of the same size to
* an iommu domain.
@@ -283,6 +285,10 @@ struct iommu_ops {
struct iommu_domain_ops {
int (*attach_dev)(struct iommu_domain *domain, struct device *dev);
void (*detach_dev)(struct iommu_domain *domain, struct device *dev);
+ int (*attach_dev_pasid)(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+ void (*detach_dev_pasid)(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
int (*map)(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t size, int prot, gfp_t gfp);
@@ -678,6 +684,10 @@ int iommu_group_claim_dma_owner(struct iommu_group *group, void *owner);
void iommu_group_release_dma_owner(struct iommu_group *group);
bool iommu_group_dma_owner_claimed(struct iommu_group *group);
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
+void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid);
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
@@ -1051,6 +1061,17 @@ static inline bool iommu_group_dma_owner_claimed(struct iommu_group *group)
{
return false;
}
+
+static inline int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ return -ENODEV;
+}
+
+static inline void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+}
#endif /* CONFIG_IOMMU_API */
/**
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 29906bc16371..16e8db2d86fc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -38,6 +38,7 @@ struct iommu_group {
struct kobject kobj;
struct kobject *devices_kobj;
struct list_head devices;
+ struct xarray pasid_array;
struct mutex mutex;
void *iommu_data;
void (*iommu_data_release)(void *iommu_data);
@@ -630,6 +631,7 @@ struct iommu_group *iommu_group_alloc(void)
mutex_init(&group->mutex);
INIT_LIST_HEAD(&group->devices);
INIT_LIST_HEAD(&group->entry);
+ xa_init(&group->pasid_array);
ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
if (ret < 0) {
@@ -3190,3 +3192,72 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
return user;
}
EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
+
+static bool device_group_immutable_singleton(struct device *dev)
+{
+ struct iommu_group *group = iommu_group_get(dev);
+ int count;
+
+ if (!group)
+ return false;
+
+ mutex_lock(&group->mutex);
+ count = iommu_group_device_count(group);
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ if (count != 1)
+ return false;
+
+ /*
+ * The PCI device could be considered to be fully isolated if all
+ * devices on the path from the device to the host-PCI bridge are
+ * protected from peer-to-peer DMA by ACS.
+ */
+ if (dev_is_pci(dev))
+ return pci_acs_path_enabled(to_pci_dev(dev), NULL,
+ REQ_ACS_FLAGS);
+
+ return true;
+}
+
+int iommu_attach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group;
+ int ret = -EINVAL;
+ void *curr;
+
+ if (!domain->ops->attach_dev_pasid)
+ return -EOPNOTSUPP;
+
+ if (!device_group_immutable_singleton(dev))
+ return -EINVAL;
+
+ group = iommu_group_get(dev);
+ mutex_lock(&group->mutex);
+ curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
+ if (curr)
+ goto out_unlock;
+ ret = domain->ops->attach_dev_pasid(domain, dev, pasid);
+ if (ret)
+ xa_erase(&group->pasid_array, pasid);
+out_unlock:
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return ret;
+}
+
+void iommu_detach_device_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct iommu_group *group = iommu_group_get(dev);
+
+ mutex_lock(&group->mutex);
+ domain->ops->detach_dev_pasid(domain, dev, pasid);
+ xa_erase(&group->pasid_array, pasid);
+ mutex_unlock(&group->mutex);
+
+ iommu_group_put(group);
+}
--
2.25.1
Rename iommu-sva-lib.c[h] to iommu-sva.c[h] as it contains all code
for SVA implementation in iommu core.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
---
drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} | 0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 2 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/svm.c | 2 +-
drivers/iommu/io-pgfault.c | 2 +-
drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} | 2 +-
drivers/iommu/Makefile | 2 +-
8 files changed, 7 insertions(+), 7 deletions(-)
rename drivers/iommu/{iommu-sva-lib.h => iommu-sva.h} (100%)
rename drivers/iommu/{iommu-sva-lib.c => iommu-sva.c} (99%)
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva.h
similarity index 100%
rename from drivers/iommu/iommu-sva-lib.h
rename to drivers/iommu/iommu-sva.h
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 56644e553c42..265b125d7dc4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -9,7 +9,7 @@
#include <linux/slab.h>
#include "arm-smmu-v3.h"
-#include "../../iommu-sva-lib.h"
+#include "../../iommu-sva.h"
#include "../../io-pgtable-arm.h"
struct arm_smmu_mmu_notifier {
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6a10fa181827..de3b6fbf8766 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -31,7 +31,7 @@
#include <linux/amba/bus.h>
#include "arm-smmu-v3.h"
-#include "../../iommu-sva-lib.h"
+#include "../../iommu-sva.h"
static bool disable_bypass = true;
module_param(disable_bypass, bool, 0444);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a5728f743c6d..1c2c92b657c7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -27,7 +27,7 @@
#include <linux/tboot.h>
#include "../irq_remapping.h"
-#include "../iommu-sva-lib.h"
+#include "../iommu-sva.h"
#include "pasid.h"
#include "cap_audit.h"
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ca83ebc708a8..44331db060e4 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -25,7 +25,7 @@
#include "pasid.h"
#include "perf.h"
-#include "../iommu-sva-lib.h"
+#include "../iommu-sva.h"
static irqreturn_t prq_event_thread(int irq, void *d);
static void intel_svm_drain_prq(struct device *dev, u32 pasid);
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 9efe5259402b..2a8604013b7e 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -11,7 +11,7 @@
#include <linux/slab.h>
#include <linux/workqueue.h>
-#include "iommu-sva-lib.h"
+#include "iommu-sva.h"
/**
* struct iopf_queue - IO Page Fault queue
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva.c
similarity index 99%
rename from drivers/iommu/iommu-sva-lib.c
rename to drivers/iommu/iommu-sva.c
index ea12504a9e12..1791ac1e3d34 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva.c
@@ -7,7 +7,7 @@
#include <linux/slab.h>
#include <linux/sched/mm.h>
-#include "iommu-sva-lib.h"
+#include "iommu-sva.h"
static DEFINE_MUTEX(iommu_sva_lock);
static DECLARE_IOASID_SET(iommu_sva_pasid);
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 44475a9b3eea..c1763476162b 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -27,6 +27,6 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
-obj-$(CONFIG_IOMMU_SVA) += iommu-sva-lib.o io-pgfault.o
+obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o io-pgfault.o
obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
obj-$(CONFIG_APPLE_DART) += apple-dart.o
--
2.25.1
These ops'es have been replaced with the dev_attach/detach_pasid domain
ops'es. There's no need for them anymore. Remove them to avoid dead
code.
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jean-Philippe Brucker <[email protected]>
---
include/linux/intel-iommu.h | 4 --
include/linux/iommu.h | 8 ---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 17 -------
drivers/iommu/iommu-sva-lib.h | 1 -
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 41 ----------------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 --
drivers/iommu/intel/iommu.c | 3 --
drivers/iommu/intel/svm.c | 49 -------------------
drivers/iommu/iommu-sva-lib.c | 4 +-
9 files changed, 2 insertions(+), 128 deletions(-)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2397c2007cda..4a5cc796f917 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -738,10 +738,6 @@ struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
extern void intel_svm_check(struct intel_iommu *iommu);
extern int intel_svm_enable_prq(struct intel_iommu *iommu);
extern int intel_svm_finish_prq(struct intel_iommu *iommu);
-struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm,
- void *drvdata);
-void intel_svm_unbind(struct iommu_sva *handle);
-u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
struct iommu_page_response *msg);
struct iommu_domain *intel_svm_domain_alloc(void);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5a3ef4d58b1f..392b8adc3495 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -215,9 +215,6 @@ struct iommu_iotlb_gather {
* @dev_has/enable/disable_feat: per device entries to check/enable/disable
* iommu specific features.
* @dev_feat_enabled: check enabled feature
- * @sva_bind: Bind process address space to device
- * @sva_unbind: Unbind process address space from device
- * @sva_get_pasid: Get PASID associated to a SVA handle
* @page_response: handle page request response
* @def_domain_type: device default domain type, return value:
* - IOMMU_DOMAIN_IDENTITY: must use an identity domain
@@ -251,11 +248,6 @@ struct iommu_ops {
int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
- struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
- void *drvdata);
- void (*sva_unbind)(struct iommu_sva *handle);
- u32 (*sva_get_pasid)(struct iommu_sva *handle);
-
int (*page_response)(struct device *dev,
struct iommu_fault_event *evt,
struct iommu_page_response *msg);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index e077f21e2528..15dd4c7e6d3a 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -754,10 +754,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master);
int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
-struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
- void *drvdata);
-void arm_smmu_sva_unbind(struct iommu_sva *handle);
-u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
void arm_smmu_sva_notifier_synchronize(void);
struct iommu_domain *arm_smmu_sva_domain_alloc(void);
#else /* CONFIG_ARM_SMMU_V3_SVA */
@@ -791,19 +787,6 @@ static inline bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master
return false;
}
-static inline struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- return ERR_PTR(-ENODEV);
-}
-
-static inline void arm_smmu_sva_unbind(struct iommu_sva *handle) {}
-
-static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
-{
- return IOMMU_PASID_INVALID;
-}
-
static inline void arm_smmu_sva_notifier_synchronize(void) {}
static inline struct iommu_domain *arm_smmu_sva_domain_alloc(void)
diff --git a/drivers/iommu/iommu-sva-lib.h b/drivers/iommu/iommu-sva-lib.h
index 8909ea1094e3..3420654c6e2f 100644
--- a/drivers/iommu/iommu-sva-lib.h
+++ b/drivers/iommu/iommu-sva-lib.h
@@ -8,7 +8,6 @@
#include <linux/ioasid.h>
#include <linux/mm_types.h>
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max);
struct mm_struct *iommu_sva_find(ioasid_t pasid);
/* I/O Page fault */
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 9d176e836d6b..56644e553c42 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -335,11 +335,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
if (!bond)
return ERR_PTR(-ENOMEM);
- /* Allocate a PASID for this mm if necessary */
- ret = iommu_sva_alloc_pasid(mm, 1, (1U << master->ssid_bits) - 1);
- if (ret)
- goto err_free_bond;
-
bond->mm = mm;
bond->sva.dev = dev;
refcount_set(&bond->refs, 1);
@@ -358,42 +353,6 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
return ERR_PTR(ret);
}
-struct iommu_sva *
-arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct iommu_sva *handle;
- struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
- struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
-
- if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
- return ERR_PTR(-EINVAL);
-
- mutex_lock(&sva_lock);
- handle = __arm_smmu_sva_bind(dev, mm);
- mutex_unlock(&sva_lock);
- return handle;
-}
-
-void arm_smmu_sva_unbind(struct iommu_sva *handle)
-{
- struct arm_smmu_bond *bond = sva_to_bond(handle);
-
- mutex_lock(&sva_lock);
- if (refcount_dec_and_test(&bond->refs)) {
- list_del(&bond->list);
- arm_smmu_mmu_notifier_put(bond->smmu_mn);
- kfree(bond);
- }
- mutex_unlock(&sva_lock);
-}
-
-u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
-{
- struct arm_smmu_bond *bond = sva_to_bond(handle);
-
- return bond->mm->pasid;
-}
-
bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
{
unsigned long reg, fld;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 9daf3de7e539..6a10fa181827 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2857,9 +2857,6 @@ static struct iommu_ops arm_smmu_ops = {
.dev_feat_enabled = arm_smmu_dev_feature_enabled,
.dev_enable_feat = arm_smmu_dev_enable_feature,
.dev_disable_feat = arm_smmu_dev_disable_feature,
- .sva_bind = arm_smmu_sva_bind,
- .sva_unbind = arm_smmu_sva_unbind,
- .sva_get_pasid = arm_smmu_sva_get_pasid,
.page_response = arm_smmu_page_response,
.pgsize_bitmap = -1UL, /* Restricted during device attach */
.owner = THIS_MODULE,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 10b1e9dcbd98..a5728f743c6d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4919,9 +4919,6 @@ const struct iommu_ops intel_iommu_ops = {
.def_domain_type = device_def_domain_type,
.pgsize_bitmap = SZ_4K,
#ifdef CONFIG_INTEL_IOMMU_SVM
- .sva_bind = intel_svm_bind,
- .sva_unbind = intel_svm_unbind,
- .sva_get_pasid = intel_svm_get_pasid,
.page_response = intel_svm_page_response,
#endif
.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 641ab0491ada..ca83ebc708a8 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -313,14 +313,6 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
return 0;
}
-static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm)
-{
- ioasid_t max_pasid = dev_is_pci(dev) ?
- pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
-
- return iommu_sva_alloc_pasid(mm, PASID_MIN, max_pasid - 1);
-}
-
static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
struct device *dev,
struct mm_struct *mm)
@@ -809,47 +801,6 @@ static irqreturn_t prq_event_thread(int irq, void *d)
return IRQ_RETVAL(handled);
}
-struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
-{
- struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
- struct iommu_sva *sva;
- int ret;
-
- mutex_lock(&pasid_mutex);
- ret = intel_svm_alloc_pasid(dev, mm);
- if (ret) {
- mutex_unlock(&pasid_mutex);
- return ERR_PTR(ret);
- }
-
- sva = intel_svm_bind_mm(iommu, dev, mm);
- mutex_unlock(&pasid_mutex);
-
- return sva;
-}
-
-void intel_svm_unbind(struct iommu_sva *sva)
-{
- struct intel_svm_dev *sdev = to_intel_svm_dev(sva);
-
- mutex_lock(&pasid_mutex);
- intel_svm_unbind_mm(sdev->dev, sdev->pasid);
- mutex_unlock(&pasid_mutex);
-}
-
-u32 intel_svm_get_pasid(struct iommu_sva *sva)
-{
- struct intel_svm_dev *sdev;
- u32 pasid;
-
- mutex_lock(&pasid_mutex);
- sdev = to_intel_svm_dev(sva);
- pasid = sdev->pasid;
- mutex_unlock(&pasid_mutex);
-
- return pasid;
-}
-
int intel_svm_page_response(struct device *dev,
struct iommu_fault_event *evt,
struct iommu_page_response *msg)
diff --git a/drivers/iommu/iommu-sva-lib.c b/drivers/iommu/iommu-sva-lib.c
index e7301514f286..ef6ed87d04ba 100644
--- a/drivers/iommu/iommu-sva-lib.c
+++ b/drivers/iommu/iommu-sva-lib.c
@@ -24,7 +24,8 @@ static DECLARE_IOASID_SET(iommu_sva_pasid);
*
* Returns 0 on success and < 0 on error.
*/
-int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+static int iommu_sva_alloc_pasid(struct mm_struct *mm,
+ ioasid_t min, ioasid_t max)
{
int ret = 0;
ioasid_t pasid;
@@ -50,7 +51,6 @@ int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
mutex_unlock(&iommu_sva_lock);
return ret;
}
-EXPORT_SYMBOL_GPL(iommu_sva_alloc_pasid);
/* ioasid_find getter() requires a void * argument */
static bool __mmget_not_zero(void *mm)
--
2.25.1
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 6 ++
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 68 +++++++++++++++++++
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +
3 files changed, 77 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index cd48590ada30..e077f21e2528 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -759,6 +759,7 @@ struct iommu_sva *arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
void arm_smmu_sva_unbind(struct iommu_sva *handle);
u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle);
void arm_smmu_sva_notifier_synchronize(void);
+struct iommu_domain *arm_smmu_sva_domain_alloc(void);
#else /* CONFIG_ARM_SMMU_V3_SVA */
static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
{
@@ -804,5 +805,10 @@ static inline u32 arm_smmu_sva_get_pasid(struct iommu_sva *handle)
}
static inline void arm_smmu_sva_notifier_synchronize(void) {}
+
+static inline struct iommu_domain *arm_smmu_sva_domain_alloc(void)
+{
+ return NULL;
+}
#endif /* CONFIG_ARM_SMMU_V3_SVA */
#endif /* _ARM_SMMU_V3_H */
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index c623dae1e115..9d176e836d6b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -541,3 +541,71 @@ void arm_smmu_sva_notifier_synchronize(void)
*/
mmu_notifier_synchronize();
}
+
+static int arm_smmu_sva_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id)
+{
+ int ret = 0;
+ struct mm_struct *mm;
+ struct iommu_sva *handle;
+
+ if (domain->type != IOMMU_DOMAIN_SVA)
+ return -EINVAL;
+
+ mm = domain->mm;
+ if (WARN_ON(!mm))
+ return -ENODEV;
+
+ mutex_lock(&sva_lock);
+ handle = __arm_smmu_sva_bind(dev, mm);
+ if (IS_ERR(handle))
+ ret = PTR_ERR(handle);
+ mutex_unlock(&sva_lock);
+
+ return ret;
+}
+
+static void arm_smmu_sva_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t id)
+{
+ struct mm_struct *mm = domain->mm;
+ struct arm_smmu_bond *bond = NULL, *t;
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+ mutex_lock(&sva_lock);
+ list_for_each_entry(t, &master->bonds, list) {
+ if (t->mm == mm) {
+ bond = t;
+ break;
+ }
+ }
+
+ if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
+ list_del(&bond->list);
+ arm_smmu_mmu_notifier_put(bond->smmu_mn);
+ kfree(bond);
+ }
+ mutex_unlock(&sva_lock);
+}
+
+static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
+{
+ kfree(domain);
+}
+
+static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
+ .attach_dev_pasid = arm_smmu_sva_attach_dev_pasid,
+ .detach_dev_pasid = arm_smmu_sva_detach_dev_pasid,
+ .free = arm_smmu_sva_domain_free,
+};
+
+struct iommu_domain *arm_smmu_sva_domain_alloc(void)
+{
+ struct iommu_domain *domain;
+
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (domain)
+ domain->ops = &arm_smmu_sva_domain_ops;
+
+ return domain;
+}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index afc63fce6107..9daf3de7e539 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1999,6 +1999,9 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
{
struct arm_smmu_domain *smmu_domain;
+ if (type == IOMMU_DOMAIN_SVA)
+ return arm_smmu_sva_domain_alloc();
+
if (type != IOMMU_DOMAIN_UNMANAGED &&
type != IOMMU_DOMAIN_DMA &&
type != IOMMU_DOMAIN_DMA_FQ &&
--
2.25.1
Add support for SVA domain allocation and provide an SVA-specific
iommu_domain_ops.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/intel-iommu.h | 5 ++++
drivers/iommu/intel/iommu.c | 2 ++
drivers/iommu/intel/svm.c | 48 +++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 72e5d7900e71..2397c2007cda 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -744,6 +744,7 @@ void intel_svm_unbind(struct iommu_sva *handle);
u32 intel_svm_get_pasid(struct iommu_sva *handle);
int intel_svm_page_response(struct device *dev, struct iommu_fault_event *evt,
struct iommu_page_response *msg);
+struct iommu_domain *intel_svm_domain_alloc(void);
struct intel_svm_dev {
struct list_head list;
@@ -769,6 +770,10 @@ struct intel_svm {
};
#else
static inline void intel_svm_check(struct intel_iommu *iommu) {}
+static inline struct iommu_domain *intel_svm_domain_alloc(void)
+{
+ return NULL;
+}
#endif
#ifdef CONFIG_INTEL_IOMMU_DEBUGFS
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 99643f897f26..10b1e9dcbd98 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4343,6 +4343,8 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type)
return domain;
case IOMMU_DOMAIN_IDENTITY:
return &si_domain->domain;
+ case IOMMU_DOMAIN_SVA:
+ return intel_svm_domain_alloc();
default:
return NULL;
}
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 574ddddaa33a..641ab0491ada 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -931,3 +931,51 @@ int intel_svm_page_response(struct device *dev,
mutex_unlock(&pasid_mutex);
return ret;
}
+
+static int intel_svm_attach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ struct device_domain_info *info = dev_iommu_priv_get(dev);
+ struct intel_iommu *iommu = info->iommu;
+ struct mm_struct *mm = domain->mm;
+ struct iommu_sva *sva;
+ int ret = 0;
+
+ mutex_lock(&pasid_mutex);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
+ if (IS_ERR(sva))
+ ret = PTR_ERR(sva);
+ mutex_unlock(&pasid_mutex);
+
+ return ret;
+}
+
+static void intel_svm_detach_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ mutex_lock(&pasid_mutex);
+ intel_svm_unbind_mm(dev, pasid);
+ mutex_unlock(&pasid_mutex);
+}
+
+static void intel_svm_domain_free(struct iommu_domain *domain)
+{
+ kfree(domain);
+}
+
+static const struct iommu_domain_ops intel_svm_domain_ops = {
+ .attach_dev_pasid = intel_svm_attach_dev_pasid,
+ .detach_dev_pasid = intel_svm_detach_dev_pasid,
+ .free = intel_svm_domain_free,
+};
+
+struct iommu_domain *intel_svm_domain_alloc(void)
+{
+ struct iommu_domain *domain;
+
+ domain = kzalloc(sizeof(*domain), GFP_KERNEL);
+ if (domain)
+ domain->ops = &intel_svm_domain_ops;
+
+ return domain;
+}
--
2.25.1
On Tue, May 10, 2022 at 02:17:29PM +0800, Lu Baolu wrote:
> This adds a pair of common domain ops for this purpose and adds helpers
> to attach/detach a domain to/from a {device, PASID}.
I wonder if this should not have a detach op - after discussing with
Robin we can see that detach_dev is not used in updated
drivers. Instead attach_dev acts as 'set_domain'
So, it would be more symmetrical if attaching a blocking_domain to the
PASID was the way to 'detach'.
This could be made straightforward by following the sketch I showed to
have a static, global blocing_domain and providing a pointer to it in
struct iommu_ops
Then 'detach pasid' is:
iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev, pasid);
And we move away from the notion of 'detach' and in the direction that
everything continuously has a domain set. PASID would logically
default to blocking_domain, though we wouldn't track this anywhere.
Jason
On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
> +/**
> + * iommu_sva_bind_device() - Bind a process address space to a device
> + * @dev: the device
> + * @mm: the mm to bind, caller must hold a reference to mm_users
> + * @drvdata: opaque data pointer to pass to bind callback
> + *
> + * Create a bond between device and address space, allowing the device to access
> + * the mm using the returned PASID. If a bond already exists between @device and
> + * @mm, it is returned and an additional reference is taken. Caller must call
> + * iommu_sva_unbind_device() to release each reference.
> + *
> + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
> + * initialize the required SVA features.
> + *
> + * On error, returns an ERR_PTR value.
> + */
> +struct iommu_sva *
> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
> +{
> + int ret = -EINVAL;
> + struct iommu_sva *handle;
> + struct iommu_domain *domain;
> +
> + /*
> + * TODO: Remove the drvdata parameter after kernel PASID support is
> + * enabled for the idxd driver.
> + */
> + if (drvdata)
> + return ERR_PTR(-EOPNOTSUPP);
Why is this being left behind? Clean up the callers too please.
> + /* Allocate mm->pasid if necessary. */
> + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + mutex_lock(&iommu_sva_lock);
> + /* Search for an existing bond. */
> + handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
> + if (handle) {
> + refcount_inc(&handle->users);
> + goto out_success;
> + }
How can there be an existing bond?
dev->iommu is per-device
The device_group_immutable_singleton() insists on a single device
group
Basically 'sva_bonds' is the same thing as the group->pasid_array.
Assuming we leave room for multi-device groups this logic should just
be
group = iommu_group_get(dev);
if (!group)
return -ENODEV;
mutex_lock(&group->mutex);
domain = xa_load(&group->pasid_array, mm->pasid);
if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
domain = iommu_sva_alloc_domain(dev, mm);
?
And stick the refcount in the sva_domain
Also, given the current arrangement it might make sense to have a
struct iommu_domain_sva given that no driver is wrappering this in
something else.
Jason
The current kernel DMA with PASID support is based on the SVA with a flag
SVM_FLAG_SUPERVISOR_MODE. The IOMMU driver binds the kernel memory address
space to a PASID of the device. The device driver programs the device with
kernel virtual address (KVA) for DMA access. There have been security and
functional issues with this approach:
- The lack of IOTLB synchronization upon kernel page table updates.
(vmalloc, module/BPF loading, CONFIG_DEBUG_PAGEALLOC etc.)
- Other than slight more protection, using kernel virtual address (KVA)
has little advantage over physical address. There are also no use
cases yet where DMA engines need kernel virtual addresses for in-kernel
DMA.
This removes SVM_FLAG_SUPERVISOR_MODE support in the Intel IOMMU driver.
The device driver is suggested to handle kernel DMA with PASID through
the kernel DMA APIs.
Link: https://lore.kernel.org/linux-iommu/[email protected]/
Signed-off-by: Jacob Pan <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/svm.c | 53 +++++++++------------------------------
1 file changed, 12 insertions(+), 41 deletions(-)
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 7ee37d996e15..574ddddaa33a 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -313,8 +313,7 @@ static int pasid_to_svm_sdev(struct device *dev, unsigned int pasid,
return 0;
}
-static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm,
- unsigned int flags)
+static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm)
{
ioasid_t max_pasid = dev_is_pci(dev) ?
pci_max_pasids(to_pci_dev(dev)) : intel_pasid_max_id;
@@ -324,8 +323,7 @@ static int intel_svm_alloc_pasid(struct device *dev, struct mm_struct *mm,
static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
struct device *dev,
- struct mm_struct *mm,
- unsigned int flags)
+ struct mm_struct *mm)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
unsigned long iflags, sflags;
@@ -341,22 +339,18 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
svm->pasid = mm->pasid;
svm->mm = mm;
- svm->flags = flags;
INIT_LIST_HEAD_RCU(&svm->devs);
- if (!(flags & SVM_FLAG_SUPERVISOR_MODE)) {
- svm->notifier.ops = &intel_mmuops;
- ret = mmu_notifier_register(&svm->notifier, mm);
- if (ret) {
- kfree(svm);
- return ERR_PTR(ret);
- }
+ svm->notifier.ops = &intel_mmuops;
+ ret = mmu_notifier_register(&svm->notifier, mm);
+ if (ret) {
+ kfree(svm);
+ return ERR_PTR(ret);
}
ret = pasid_private_add(svm->pasid, svm);
if (ret) {
- if (svm->notifier.ops)
- mmu_notifier_unregister(&svm->notifier, mm);
+ mmu_notifier_unregister(&svm->notifier, mm);
kfree(svm);
return ERR_PTR(ret);
}
@@ -391,9 +385,7 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
}
/* Setup the pasid table: */
- sflags = (flags & SVM_FLAG_SUPERVISOR_MODE) ?
- PASID_FLAG_SUPERVISOR_MODE : 0;
- sflags |= cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
+ sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
spin_lock_irqsave(&iommu->lock, iflags);
ret = intel_pasid_setup_first_level(iommu, dev, mm->pgd, mm->pasid,
FLPT_DEFAULT_DID, sflags);
@@ -410,8 +402,7 @@ static struct iommu_sva *intel_svm_bind_mm(struct intel_iommu *iommu,
kfree(sdev);
free_svm:
if (list_empty(&svm->devs)) {
- if (svm->notifier.ops)
- mmu_notifier_unregister(&svm->notifier, mm);
+ mmu_notifier_unregister(&svm->notifier, mm);
pasid_private_remove(mm->pasid);
kfree(svm);
}
@@ -821,37 +812,17 @@ static irqreturn_t prq_event_thread(int irq, void *d)
struct iommu_sva *intel_svm_bind(struct device *dev, struct mm_struct *mm, void *drvdata)
{
struct intel_iommu *iommu = device_to_iommu(dev, NULL, NULL);
- unsigned int flags = 0;
struct iommu_sva *sva;
int ret;
- if (drvdata)
- flags = *(unsigned int *)drvdata;
-
- if (flags & SVM_FLAG_SUPERVISOR_MODE) {
- if (!ecap_srs(iommu->ecap)) {
- dev_err(dev, "%s: Supervisor PASID not supported\n",
- iommu->name);
- return ERR_PTR(-EOPNOTSUPP);
- }
-
- if (mm) {
- dev_err(dev, "%s: Supervisor PASID with user provided mm\n",
- iommu->name);
- return ERR_PTR(-EINVAL);
- }
-
- mm = &init_mm;
- }
-
mutex_lock(&pasid_mutex);
- ret = intel_svm_alloc_pasid(dev, mm, flags);
+ ret = intel_svm_alloc_pasid(dev, mm);
if (ret) {
mutex_unlock(&pasid_mutex);
return ERR_PTR(ret);
}
- sva = intel_svm_bind_mm(iommu, dev, mm, flags);
+ sva = intel_svm_bind_mm(iommu, dev, mm);
mutex_unlock(&pasid_mutex);
return sva;
--
2.25.1
On 2022/5/10 22:02, Jason Gunthorpe wrote:
> On Tue, May 10, 2022 at 02:17:29PM +0800, Lu Baolu wrote:
>
>> This adds a pair of common domain ops for this purpose and adds helpers
>> to attach/detach a domain to/from a {device, PASID}.
>
> I wonder if this should not have a detach op - after discussing with
> Robin we can see that detach_dev is not used in updated
> drivers. Instead attach_dev acts as 'set_domain'
>
> So, it would be more symmetrical if attaching a blocking_domain to the
> PASID was the way to 'detach'.
>
> This could be made straightforward by following the sketch I showed to
> have a static, global blocing_domain and providing a pointer to it in
> struct iommu_ops
>
> Then 'detach pasid' is:
>
> iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev, pasid);
>
> And we move away from the notion of 'detach' and in the direction that
> everything continuously has a domain set. PASID would logically
> default to blocking_domain, though we wouldn't track this anywhere.
I am not sure whether we still need to keep the blocking domain concept
when we are entering the new PASID world. Please allow me to wait and
listen to more opinions.
Best regards,
baolu
On Wed, May 11, 2022 at 04:09:14AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <[email protected]>
> > Sent: Wednesday, May 11, 2022 10:32 AM
> >
> > On 2022/5/10 22:02, Jason Gunthorpe wrote:
> > > On Tue, May 10, 2022 at 02:17:29PM +0800, Lu Baolu wrote:
> > >
> > >> This adds a pair of common domain ops for this purpose and adds
> > helpers
> > >> to attach/detach a domain to/from a {device, PASID}.
> > >
> > > I wonder if this should not have a detach op - after discussing with
> > > Robin we can see that detach_dev is not used in updated
> > > drivers. Instead attach_dev acts as 'set_domain'
> > >
> > > So, it would be more symmetrical if attaching a blocking_domain to the
> > > PASID was the way to 'detach'.
> > >
> > > This could be made straightforward by following the sketch I showed to
> > > have a static, global blocing_domain and providing a pointer to it in
> > > struct iommu_ops
> > >
> > > Then 'detach pasid' is:
> > >
> > > iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev,
> > pasid);
> > >
> > > And we move away from the notion of 'detach' and in the direction that
> > > everything continuously has a domain set. PASID would logically
> > > default to blocking_domain, though we wouldn't track this anywhere.
> >
> > I am not sure whether we still need to keep the blocking domain concept
> > when we are entering the new PASID world. Please allow me to wait and
> > listen to more opinions.
> >
>
> I'm with Jason on this direction. In concept after a PASID is detached it's
> essentially blocked. Implementation-wise it doesn't prevent the iommu
> driver from marking the PASID entry as non-present as doing in this
> series instead of actually pointing to the empty page table of the block
> domain. But api-wise it does make the entire semantics more consistent.
This is all internal to IOMMU so I don't think we should be concerned
about API consistency. I prefer a straighforward detach() operation
because that way IOMMU drivers don't have to keep track of which domain is
attached to which PASID. That code can be factored into the IOMMU core.
In addition to clearing contexts, detach() also needs to invalidate TLBs,
and for that the SMMU driver needs to know the old ASID (!= PASID) that
was used by the context descriptor. We can certainly work around a missing
detach() to implement this, but it will be convoluted.
Thanks,
Jean
> From: Baolu Lu <[email protected]>
> Sent: Wednesday, May 11, 2022 10:32 AM
>
> On 2022/5/10 22:02, Jason Gunthorpe wrote:
> > On Tue, May 10, 2022 at 02:17:29PM +0800, Lu Baolu wrote:
> >
> >> This adds a pair of common domain ops for this purpose and adds
> helpers
> >> to attach/detach a domain to/from a {device, PASID}.
> >
> > I wonder if this should not have a detach op - after discussing with
> > Robin we can see that detach_dev is not used in updated
> > drivers. Instead attach_dev acts as 'set_domain'
> >
> > So, it would be more symmetrical if attaching a blocking_domain to the
> > PASID was the way to 'detach'.
> >
> > This could be made straightforward by following the sketch I showed to
> > have a static, global blocing_domain and providing a pointer to it in
> > struct iommu_ops
> >
> > Then 'detach pasid' is:
> >
> > iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev,
> pasid);
> >
> > And we move away from the notion of 'detach' and in the direction that
> > everything continuously has a domain set. PASID would logically
> > default to blocking_domain, though we wouldn't track this anywhere.
>
> I am not sure whether we still need to keep the blocking domain concept
> when we are entering the new PASID world. Please allow me to wait and
> listen to more opinions.
>
I'm with Jason on this direction. In concept after a PASID is detached it's
essentially blocked. Implementation-wise it doesn't prevent the iommu
driver from marking the PASID entry as non-present as doing in this
series instead of actually pointing to the empty page table of the block
domain. But api-wise it does make the entire semantics more consistent.
Thanks
Kevin
On 2022/5/10 23:23, Jason Gunthorpe wrote:
> On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
>
>> +/**
>> + * iommu_sva_bind_device() - Bind a process address space to a device
>> + * @dev: the device
>> + * @mm: the mm to bind, caller must hold a reference to mm_users
>> + * @drvdata: opaque data pointer to pass to bind callback
>> + *
>> + * Create a bond between device and address space, allowing the device to access
>> + * the mm using the returned PASID. If a bond already exists between @device and
>> + * @mm, it is returned and an additional reference is taken. Caller must call
>> + * iommu_sva_unbind_device() to release each reference.
>> + *
>> + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
>> + * initialize the required SVA features.
>> + *
>> + * On error, returns an ERR_PTR value.
>> + */
>> +struct iommu_sva *
>> +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
>> +{
>> + int ret = -EINVAL;
>> + struct iommu_sva *handle;
>> + struct iommu_domain *domain;
>> +
>> + /*
>> + * TODO: Remove the drvdata parameter after kernel PASID support is
>> + * enabled for the idxd driver.
>> + */
>> + if (drvdata)
>> + return ERR_PTR(-EOPNOTSUPP);
>
> Why is this being left behind? Clean up the callers too please.
Okay, let me try to.
>
>> + /* Allocate mm->pasid if necessary. */
>> + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + mutex_lock(&iommu_sva_lock);
>> + /* Search for an existing bond. */
>> + handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
>> + if (handle) {
>> + refcount_inc(&handle->users);
>> + goto out_success;
>> + }
>
> How can there be an existing bond?
>
> dev->iommu is per-device
>
> The device_group_immutable_singleton() insists on a single device
> group
>
> Basically 'sva_bonds' is the same thing as the group->pasid_array.
Yes, really.
>
> Assuming we leave room for multi-device groups this logic should just
> be
>
> group = iommu_group_get(dev);
> if (!group)
> return -ENODEV;
>
> mutex_lock(&group->mutex);
> domain = xa_load(&group->pasid_array, mm->pasid);
> if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
> domain = iommu_sva_alloc_domain(dev, mm);
>
> ?
Agreed. As a helper in iommu core, how about making it more generic like
below?
+struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
+ iosid_t pasid,
+ unsigned int type)
+{
+ struct iommu_domain *domain;
+ struct iommu_group *group;
+
+ if (!pasid_valid(pasid))
+ return NULL;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return NULL;
+
+ mutex_lock(&group->mutex);
+ domain = xa_load(&group->pasid_array, pasid);
+ if (domain && domain->type != type)
+ domain = NULL;
+ mutex_unlock(&group->mutex);
+ iommu_group_put(group);
+
+ return domain;
+}
>
> And stick the refcount in the sva_domain
>
> Also, given the current arrangement it might make sense to have a
> struct iommu_domain_sva given that no driver is wrappering this in
> something else.
Fair enough. How about below wrapper?
+struct iommu_sva_domain {
+ /*
+ * Common iommu domain header, *must* be put at the top
+ * of the structure.
+ */
+ struct iommu_domain domain;
+ struct mm_struct *mm;
+ struct iommu_sva bond;
+}
The refcount is wrapped in bond.
Best regards,
baolu
On Wed, May 11, 2022 at 08:54:39AM +0100, Jean-Philippe Brucker wrote:
> > > > Then 'detach pasid' is:
> > > >
> > > > iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev,
> > > pasid);
> > > >
> > > > And we move away from the notion of 'detach' and in the direction that
> > > > everything continuously has a domain set. PASID would logically
> > > > default to blocking_domain, though we wouldn't track this anywhere.
> > >
> > > I am not sure whether we still need to keep the blocking domain concept
> > > when we are entering the new PASID world. Please allow me to wait and
> > > listen to more opinions.
> > >
> >
> > I'm with Jason on this direction. In concept after a PASID is detached it's
> > essentially blocked. Implementation-wise it doesn't prevent the iommu
> > driver from marking the PASID entry as non-present as doing in this
> > series instead of actually pointing to the empty page table of the block
> > domain. But api-wise it does make the entire semantics more consistent.
>
> This is all internal to IOMMU so I don't think we should be concerned
> about API consistency. I prefer a straighforward detach() operation
> because that way IOMMU drivers don't have to keep track of which domain is
> attached to which PASID. That code can be factored into the IOMMU core.
Why would a driver need to keep additional tracking?
> In addition to clearing contexts, detach() also needs to invalidate TLBs,
> and for that the SMMU driver needs to know the old ASID (!= PASID) that
> was used by the context descriptor. We can certainly work around a missing
> detach() to implement this, but it will be convoluted.
It is not "missing" it is just renamed to blocking_domain->ops->set_dev_pasid()
The implementation of that function would be identical to
detach_dev_pasid.
Jason
On Wed, May 11, 2022 at 03:21:31PM +0800, Baolu Lu wrote:
> On 2022/5/10 23:23, Jason Gunthorpe wrote:
> > On Tue, May 10, 2022 at 02:17:34PM +0800, Lu Baolu wrote:
> >
> > > +/**
> > > + * iommu_sva_bind_device() - Bind a process address space to a device
> > > + * @dev: the device
> > > + * @mm: the mm to bind, caller must hold a reference to mm_users
> > > + * @drvdata: opaque data pointer to pass to bind callback
> > > + *
> > > + * Create a bond between device and address space, allowing the device to access
> > > + * the mm using the returned PASID. If a bond already exists between @device and
> > > + * @mm, it is returned and an additional reference is taken. Caller must call
> > > + * iommu_sva_unbind_device() to release each reference.
> > > + *
> > > + * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
> > > + * initialize the required SVA features.
> > > + *
> > > + * On error, returns an ERR_PTR value.
> > > + */
> > > +struct iommu_sva *
> > > +iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
> > > +{
> > > + int ret = -EINVAL;
> > > + struct iommu_sva *handle;
> > > + struct iommu_domain *domain;
> > > +
> > > + /*
> > > + * TODO: Remove the drvdata parameter after kernel PASID support is
> > > + * enabled for the idxd driver.
> > > + */
> > > + if (drvdata)
> > > + return ERR_PTR(-EOPNOTSUPP);
> >
> > Why is this being left behind? Clean up the callers too please.
>
> Okay, let me try to.
>
> >
> > > + /* Allocate mm->pasid if necessary. */
> > > + ret = iommu_sva_alloc_pasid(mm, 1, (1U << dev->iommu->pasid_bits) - 1);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > +
> > > + mutex_lock(&iommu_sva_lock);
> > > + /* Search for an existing bond. */
> > > + handle = xa_load(&dev->iommu->sva_bonds, mm->pasid);
> > > + if (handle) {
> > > + refcount_inc(&handle->users);
> > > + goto out_success;
> > > + }
> >
> > How can there be an existing bond?
> >
> > dev->iommu is per-device
> >
> > The device_group_immutable_singleton() insists on a single device
> > group
> >
> > Basically 'sva_bonds' is the same thing as the group->pasid_array.
>
> Yes, really.
>
> >
> > Assuming we leave room for multi-device groups this logic should just
> > be
> >
> > group = iommu_group_get(dev);
> > if (!group)
> > return -ENODEV;
> >
> > mutex_lock(&group->mutex);
> > domain = xa_load(&group->pasid_array, mm->pasid);
> > if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
> > domain = iommu_sva_alloc_domain(dev, mm);
> >
> > ?
>
> Agreed. As a helper in iommu core, how about making it more generic like
> below?
IDK, is there more users of this? AFAIK SVA is the only place that
will be auto-sharing?
> + mutex_lock(&group->mutex);
> + domain = xa_load(&group->pasid_array, pasid);
> + if (domain && domain->type != type)
> + domain = NULL;
> + mutex_unlock(&group->mutex);
> + iommu_group_put(group);
> +
> + return domain;
This is bad locking, group->pasid_array values cannot be taken outside
the lock.
> > And stick the refcount in the sva_domain
> >
> > Also, given the current arrangement it might make sense to have a
> > struct iommu_domain_sva given that no driver is wrappering this in
> > something else.
>
> Fair enough. How about below wrapper?
>
> +struct iommu_sva_domain {
> + /*
> + * Common iommu domain header, *must* be put at the top
> + * of the structure.
> + */
> + struct iommu_domain domain;
> + struct mm_struct *mm;
> + struct iommu_sva bond;
> +}
>
> The refcount is wrapped in bond.
I'm still not sure that bond is necessary
But yes, something like that
Jason
On 2022/5/12 13:44, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Thursday, May 12, 2022 1:17 PM
>>
>> On 2022/5/12 13:01, Tian, Kevin wrote:
>>>> From: Baolu Lu <[email protected]>
>>>> Sent: Thursday, May 12, 2022 11:03 AM
>>>>
>>>> On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>>>>>> Also, given the current arrangement it might make sense to have a
>>>>>>> struct iommu_domain_sva given that no driver is wrappering this in
>>>>>>> something else.
>>>>>> Fair enough. How about below wrapper?
>>>>>>
>>>>>> +struct iommu_sva_domain {
>>>>>> + /*
>>>>>> + * Common iommu domain header,*must* be put at the top
>>>>>> + * of the structure.
>>>>>> + */
>>>>>> + struct iommu_domain domain;
>>>>>> + struct mm_struct *mm;
>>>>>> + struct iommu_sva bond;
>>>>>> +}
>>>>>>
>>>>>> The refcount is wrapped in bond.
>>>>> I'm still not sure that bond is necessary
>>>>
>>>> "bond" is the sva handle that the device drivers get through calling
>>>> iommu_sva_bind().
>>>>
>>>
>>> 'bond' was required before because we didn't have a domain to wrap
>>> the page table at that time.
>>>
>>> Now we have a domain and it is 1:1 associated to bond. Probably
>>> make sense now by just returning the domain as the sva handle
>>> instead?
>>
>> It also includes the device information that the domain has been
>> attached. So the sva_unbind() looks like this:
>>
>> /**
>> * iommu_sva_unbind_device() - Remove a bond created with
>> iommu_sva_bind_device
>> * @handle: the handle returned by iommu_sva_bind_device()
>> *
>> * Put reference to a bond between device and address space. The device
>> should
>> * not be issuing any more transaction for this PASID. All outstanding page
>> * requests for this PASID must have been flushed to the IOMMU.
>> */
>> void iommu_sva_unbind_device(struct iommu_sva *handle)
>>
>> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
>> if we can include the device in the unbind() interface.
>
> can we just have unbind(domain, device)?
Yes. With this, we can remove bond.
This could be done in below phase 2.
>
>>
>> Anyway, I'd expect to achieve all these in two steps:
>>
>> - sva and iopf refactoring, only iommu internal changes;
>> - sva interface refactoring, only interface changes.
>>
>> Does above work?
>>
>> Best regards,
>> baolu
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Thursday, May 12, 2022 1:17 PM
>
> On 2022/5/12 13:01, Tian, Kevin wrote:
> >> From: Baolu Lu <[email protected]>
> >> Sent: Thursday, May 12, 2022 11:03 AM
> >>
> >> On 2022/5/11 22:53, Jason Gunthorpe wrote:
> >>>>> Also, given the current arrangement it might make sense to have a
> >>>>> struct iommu_domain_sva given that no driver is wrappering this in
> >>>>> something else.
> >>>> Fair enough. How about below wrapper?
> >>>>
> >>>> +struct iommu_sva_domain {
> >>>> + /*
> >>>> + * Common iommu domain header,*must* be put at the top
> >>>> + * of the structure.
> >>>> + */
> >>>> + struct iommu_domain domain;
> >>>> + struct mm_struct *mm;
> >>>> + struct iommu_sva bond;
> >>>> +}
> >>>>
> >>>> The refcount is wrapped in bond.
> >>> I'm still not sure that bond is necessary
> >>
> >> "bond" is the sva handle that the device drivers get through calling
> >> iommu_sva_bind().
> >>
> >
> > 'bond' was required before because we didn't have a domain to wrap
> > the page table at that time.
> >
> > Now we have a domain and it is 1:1 associated to bond. Probably
> > make sense now by just returning the domain as the sva handle
> > instead?
>
> It also includes the device information that the domain has been
> attached. So the sva_unbind() looks like this:
>
> /**
> * iommu_sva_unbind_device() - Remove a bond created with
> iommu_sva_bind_device
> * @handle: the handle returned by iommu_sva_bind_device()
> *
> * Put reference to a bond between device and address space. The device
> should
> * not be issuing any more transaction for this PASID. All outstanding page
> * requests for this PASID must have been flushed to the IOMMU.
> */
> void iommu_sva_unbind_device(struct iommu_sva *handle)
>
> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> if we can include the device in the unbind() interface.
can we just have unbind(domain, device)?
>
> Anyway, I'd expect to achieve all these in two steps:
>
> - sva and iopf refactoring, only iommu internal changes;
> - sva interface refactoring, only interface changes.
>
> Does above work?
>
> Best regards,
> baolu
On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>> Assuming we leave room for multi-device groups this logic should just
>>> be
>>>
>>> group = iommu_group_get(dev);
>>> if (!group)
>>> return -ENODEV;
>>>
>>> mutex_lock(&group->mutex);
>>> domain = xa_load(&group->pasid_array, mm->pasid);
>>> if (!domain || domain->type != IOMMU_DOMAIN_SVA || domain->mm != mm)
>>> domain = iommu_sva_alloc_domain(dev, mm);
>>>
>>> ?
>> Agreed. As a helper in iommu core, how about making it more generic like
>> below?
> IDK, is there more users of this? AFAIK SVA is the only place that
> will be auto-sharing?
The generic thing is that components, like SVA, want to fetch the
attached domain from the iommu core.
>
>> + mutex_lock(&group->mutex);
>> + domain = xa_load(&group->pasid_array, pasid);
>> + if (domain && domain->type != type)
>> + domain = NULL;
>> + mutex_unlock(&group->mutex);
>> + iommu_group_put(group);
>> +
>> + return domain;
> This is bad locking, group->pasid_array values cannot be taken outside
> the lock.
It's not iommu core, but SVA (or other feature components) that manage
the life cycle of a domain. The iommu core only provides a place to
store the domain pointer. The feature components are free to fetch their
domain pointers from iommu core as long as they are sure that the domain
is alive during use.
>
>>> And stick the refcount in the sva_domain
>>>
>>> Also, given the current arrangement it might make sense to have a
>>> struct iommu_domain_sva given that no driver is wrappering this in
>>> something else.
>> Fair enough. How about below wrapper?
>>
>> +struct iommu_sva_domain {
>> + /*
>> + * Common iommu domain header,*must* be put at the top
>> + * of the structure.
>> + */
>> + struct iommu_domain domain;
>> + struct mm_struct *mm;
>> + struct iommu_sva bond;
>> +}
>>
>> The refcount is wrapped in bond.
> I'm still not sure that bond is necessary
"bond" is the sva handle that the device drivers get through calling
iommu_sva_bind().
>
> But yes, something like that
Best regards,
baolu
On 2022/5/12 13:01, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Thursday, May 12, 2022 11:03 AM
>>
>> On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>>>> Also, given the current arrangement it might make sense to have a
>>>>> struct iommu_domain_sva given that no driver is wrappering this in
>>>>> something else.
>>>> Fair enough. How about below wrapper?
>>>>
>>>> +struct iommu_sva_domain {
>>>> + /*
>>>> + * Common iommu domain header,*must* be put at the top
>>>> + * of the structure.
>>>> + */
>>>> + struct iommu_domain domain;
>>>> + struct mm_struct *mm;
>>>> + struct iommu_sva bond;
>>>> +}
>>>>
>>>> The refcount is wrapped in bond.
>>> I'm still not sure that bond is necessary
>>
>> "bond" is the sva handle that the device drivers get through calling
>> iommu_sva_bind().
>>
>
> 'bond' was required before because we didn't have a domain to wrap
> the page table at that time.
>
> Now we have a domain and it is 1:1 associated to bond. Probably
> make sense now by just returning the domain as the sva handle
> instead?
It also includes the device information that the domain has been
attached. So the sva_unbind() looks like this:
/**
* iommu_sva_unbind_device() - Remove a bond created with
iommu_sva_bind_device
* @handle: the handle returned by iommu_sva_bind_device()
*
* Put reference to a bond between device and address space. The device
should
* not be issuing any more transaction for this PASID. All outstanding page
* requests for this PASID must have been flushed to the IOMMU.
*/
void iommu_sva_unbind_device(struct iommu_sva *handle)
It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
if we can include the device in the unbind() interface.
Anyway, I'd expect to achieve all these in two steps:
- sva and iopf refactoring, only iommu internal changes;
- sva interface refactoring, only interface changes.
Does above work?
Best regards,
baolu
On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
> On 2022/5/12 13:01, Tian, Kevin wrote:
> > > From: Baolu Lu <[email protected]>
> > > Sent: Thursday, May 12, 2022 11:03 AM
> > >
> > > On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > > > > > Also, given the current arrangement it might make sense to have a
> > > > > > struct iommu_domain_sva given that no driver is wrappering this in
> > > > > > something else.
> > > > > Fair enough. How about below wrapper?
> > > > >
> > > > > +struct iommu_sva_domain {
> > > > > + /*
> > > > > + * Common iommu domain header,*must* be put at the top
> > > > > + * of the structure.
> > > > > + */
> > > > > + struct iommu_domain domain;
> > > > > + struct mm_struct *mm;
> > > > > + struct iommu_sva bond;
> > > > > +}
> > > > >
> > > > > The refcount is wrapped in bond.
> > > > I'm still not sure that bond is necessary
> > >
> > > "bond" is the sva handle that the device drivers get through calling
> > > iommu_sva_bind().
> > >
> >
> > 'bond' was required before because we didn't have a domain to wrap
> > the page table at that time.
> >
> > Now we have a domain and it is 1:1 associated to bond. Probably
> > make sense now by just returning the domain as the sva handle
> > instead?
>
> It also includes the device information that the domain has been
> attached. So the sva_unbind() looks like this:
>
> /**
> * iommu_sva_unbind_device() - Remove a bond created with
> iommu_sva_bind_device
> * @handle: the handle returned by iommu_sva_bind_device()
> *
> * Put reference to a bond between device and address space. The device
> should
> * not be issuing any more transaction for this PASID. All outstanding page
> * requests for this PASID must have been flushed to the IOMMU.
> */
> void iommu_sva_unbind_device(struct iommu_sva *handle)
>
> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> if we can include the device in the unbind() interface.
Why would we have a special unbind for SVA?
SVA should not different from normal domains it should use the normal
detach flow too.
Jason
On Wed, May 11, 2022 at 09:02:40AM -0300, Jason Gunthorpe wrote:
> On Wed, May 11, 2022 at 08:54:39AM +0100, Jean-Philippe Brucker wrote:
> > > > > Then 'detach pasid' is:
> > > > >
> > > > > iommu_ops->blocking_domain->ops->attach_dev_pasid(domain, dev,
> > > > pasid);
> > > > >
> > > > > And we move away from the notion of 'detach' and in the direction that
> > > > > everything continuously has a domain set. PASID would logically
> > > > > default to blocking_domain, though we wouldn't track this anywhere.
> > > >
> > > > I am not sure whether we still need to keep the blocking domain concept
> > > > when we are entering the new PASID world. Please allow me to wait and
> > > > listen to more opinions.
> > > >
> > >
> > > I'm with Jason on this direction. In concept after a PASID is detached it's
> > > essentially blocked. Implementation-wise it doesn't prevent the iommu
> > > driver from marking the PASID entry as non-present as doing in this
> > > series instead of actually pointing to the empty page table of the block
> > > domain. But api-wise it does make the entire semantics more consistent.
> >
> > This is all internal to IOMMU so I don't think we should be concerned
> > about API consistency. I prefer a straighforward detach() operation
> > because that way IOMMU drivers don't have to keep track of which domain is
> > attached to which PASID. That code can be factored into the IOMMU core.
>
> Why would a driver need to keep additional tracking?
>
> > In addition to clearing contexts, detach() also needs to invalidate TLBs,
> > and for that the SMMU driver needs to know the old ASID (!= PASID) that
> > was used by the context descriptor. We can certainly work around a missing
> > detach() to implement this, but it will be convoluted.
>
> It is not "missing" it is just renamed to blocking_domain->ops->set_dev_pasid()
>
> The implementation of that function would be identical to
> detach_dev_pasid.
attach(dev, pasid, sva_domain)
detach(dev, pasid, sva_domain)
versus
set_dev_pasid(dev, pasid, sva_domain)
set_dev_pasid(dev, pasid, blocking)
we loose the information of the domain previously attached, and the SMMU
driver has to retrieve it to find the ASID corresponding to the mm.
Thanks,
Jean
> From: Baolu Lu <[email protected]>
> Sent: Thursday, May 12, 2022 11:03 AM
>
> On 2022/5/11 22:53, Jason Gunthorpe wrote:
> >>> Also, given the current arrangement it might make sense to have a
> >>> struct iommu_domain_sva given that no driver is wrappering this in
> >>> something else.
> >> Fair enough. How about below wrapper?
> >>
> >> +struct iommu_sva_domain {
> >> + /*
> >> + * Common iommu domain header,*must* be put at the top
> >> + * of the structure.
> >> + */
> >> + struct iommu_domain domain;
> >> + struct mm_struct *mm;
> >> + struct iommu_sva bond;
> >> +}
> >>
> >> The refcount is wrapped in bond.
> > I'm still not sure that bond is necessary
>
> "bond" is the sva handle that the device drivers get through calling
> iommu_sva_bind().
>
'bond' was required before because we didn't have a domain to wrap
the page table at that time.
Now we have a domain and it is 1:1 associated to bond. Probably
make sense now by just returning the domain as the sva handle
instead?
On Thu, May 12, 2022 at 08:00:59AM +0100, Jean-Philippe Brucker wrote:
> > It is not "missing" it is just renamed to blocking_domain->ops->set_dev_pasid()
> >
> > The implementation of that function would be identical to
> > detach_dev_pasid.
>
> attach(dev, pasid, sva_domain)
> detach(dev, pasid, sva_domain)
>
> versus
>
> set_dev_pasid(dev, pasid, sva_domain)
> set_dev_pasid(dev, pasid, blocking)
>
> we loose the information of the domain previously attached, and the SMMU
> driver has to retrieve it to find the ASID corresponding to the mm.
It would be easy to have the old domain be an input as well - the core
code knows it.
Jason
On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote:
> > > + mutex_lock(&group->mutex);
> > > + domain = xa_load(&group->pasid_array, pasid);
> > > + if (domain && domain->type != type)
> > > + domain = NULL;
> > > + mutex_unlock(&group->mutex);
> > > + iommu_group_put(group);
> > > +
> > > + return domain;
> > This is bad locking, group->pasid_array values cannot be taken outside
> > the lock.
>
> It's not iommu core, but SVA (or other feature components) that manage
> the life cycle of a domain. The iommu core only provides a place to
> store the domain pointer. The feature components are free to fetch their
> domain pointers from iommu core as long as they are sure that the domain
> is alive during use.
I'm not convinced.
Jason
On 2022/5/12 20:03, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 07:59:41PM +0800, Baolu Lu wrote:
>> On 2022/5/12 19:48, Jason Gunthorpe wrote:
>>> On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
>>>> On 2022/5/12 13:01, Tian, Kevin wrote:
>>>>>> From: Baolu Lu <[email protected]>
>>>>>> Sent: Thursday, May 12, 2022 11:03 AM
>>>>>>
>>>>>> On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>>>>>>>> Also, given the current arrangement it might make sense to have a
>>>>>>>>> struct iommu_domain_sva given that no driver is wrappering this in
>>>>>>>>> something else.
>>>>>>>> Fair enough. How about below wrapper?
>>>>>>>>
>>>>>>>> +struct iommu_sva_domain {
>>>>>>>> + /*
>>>>>>>> + * Common iommu domain header,*must* be put at the top
>>>>>>>> + * of the structure.
>>>>>>>> + */
>>>>>>>> + struct iommu_domain domain;
>>>>>>>> + struct mm_struct *mm;
>>>>>>>> + struct iommu_sva bond;
>>>>>>>> +}
>>>>>>>>
>>>>>>>> The refcount is wrapped in bond.
>>>>>>> I'm still not sure that bond is necessary
>>>>>>
>>>>>> "bond" is the sva handle that the device drivers get through calling
>>>>>> iommu_sva_bind().
>>>>>>
>>>>>
>>>>> 'bond' was required before because we didn't have a domain to wrap
>>>>> the page table at that time.
>>>>>
>>>>> Now we have a domain and it is 1:1 associated to bond. Probably
>>>>> make sense now by just returning the domain as the sva handle
>>>>> instead?
>>>>
>>>> It also includes the device information that the domain has been
>>>> attached. So the sva_unbind() looks like this:
>>>>
>>>> /**
>>>> * iommu_sva_unbind_device() - Remove a bond created with
>>>> iommu_sva_bind_device
>>>> * @handle: the handle returned by iommu_sva_bind_device()
>>>> *
>>>> * Put reference to a bond between device and address space. The device
>>>> should
>>>> * not be issuing any more transaction for this PASID. All outstanding page
>>>> * requests for this PASID must have been flushed to the IOMMU.
>>>> */
>>>> void iommu_sva_unbind_device(struct iommu_sva *handle)
>>>>
>>>> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
>>>> if we can include the device in the unbind() interface.
>>>
>>> Why would we have a special unbind for SVA?
>>
>> It's about SVA kAPI for device drivers. The existing kAPIs include:
>>
>> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
>> struct mm_struct *mm,
>> void *drvdata);
>> void iommu_sva_unbind_device(struct iommu_sva *handle);
>> u32 iommu_sva_get_pasid(struct iommu_sva *handle);
>
> This is not what we agreed the API should be. We agreed:
>
> iommu_sva_domain_alloc()
> iommu_attach_device_pasid()
> iommu_detach_device_pasid()
>
> Again, SVA should not be different from normal domain stuff.
Yes, agreed.
I am trying to achieve this in two steps. This first step focuses on
internal iommu implementation and keep the driver kAPI untouched. Then,
the second step focus on the driver APIs.
Best regards,
baolu
On 2022/5/12 19:51, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 11:02:39AM +0800, Baolu Lu wrote:
>>>> + mutex_lock(&group->mutex);
>>>> + domain = xa_load(&group->pasid_array, pasid);
>>>> + if (domain && domain->type != type)
>>>> + domain = NULL;
>>>> + mutex_unlock(&group->mutex);
>>>> + iommu_group_put(group);
>>>> +
>>>> + return domain;
>>> This is bad locking, group->pasid_array values cannot be taken outside
>>> the lock.
>>
>> It's not iommu core, but SVA (or other feature components) that manage
>> the life cycle of a domain. The iommu core only provides a place to
>> store the domain pointer. The feature components are free to fetch their
>> domain pointers from iommu core as long as they are sure that the domain
>> is alive during use.
>
> I'm not convinced.
I'm sorry, I may not have explained it clearly. :-)
This helper is safe for uses inside the IOMMU subsystem. We could trust
ourselves that nobody will abuse this helper to obtain domains belonging
to others as the pasid has been allocated for SVA code. No other code
should be able to setup another type of domain for this pasid. The SVA
code has its own lock mechanism to avoid using after free.
Please correct me if I missed anything. :-) By the way, I can see some
similar helpers in current IOMMU core. For example,
struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
{
struct iommu_domain *domain;
struct iommu_group *group;
group = iommu_group_get(dev);
if (!group)
return NULL;
domain = group->domain;
iommu_group_put(group);
return domain;
}
EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
Best regards,
baolu
On Thu, May 12, 2022 at 07:59:41PM +0800, Baolu Lu wrote:
> On 2022/5/12 19:48, Jason Gunthorpe wrote:
> > On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
> > > On 2022/5/12 13:01, Tian, Kevin wrote:
> > > > > From: Baolu Lu <[email protected]>
> > > > > Sent: Thursday, May 12, 2022 11:03 AM
> > > > >
> > > > > On 2022/5/11 22:53, Jason Gunthorpe wrote:
> > > > > > > > Also, given the current arrangement it might make sense to have a
> > > > > > > > struct iommu_domain_sva given that no driver is wrappering this in
> > > > > > > > something else.
> > > > > > > Fair enough. How about below wrapper?
> > > > > > >
> > > > > > > +struct iommu_sva_domain {
> > > > > > > + /*
> > > > > > > + * Common iommu domain header,*must* be put at the top
> > > > > > > + * of the structure.
> > > > > > > + */
> > > > > > > + struct iommu_domain domain;
> > > > > > > + struct mm_struct *mm;
> > > > > > > + struct iommu_sva bond;
> > > > > > > +}
> > > > > > >
> > > > > > > The refcount is wrapped in bond.
> > > > > > I'm still not sure that bond is necessary
> > > > >
> > > > > "bond" is the sva handle that the device drivers get through calling
> > > > > iommu_sva_bind().
> > > > >
> > > >
> > > > 'bond' was required before because we didn't have a domain to wrap
> > > > the page table at that time.
> > > >
> > > > Now we have a domain and it is 1:1 associated to bond. Probably
> > > > make sense now by just returning the domain as the sva handle
> > > > instead?
> > >
> > > It also includes the device information that the domain has been
> > > attached. So the sva_unbind() looks like this:
> > >
> > > /**
> > > * iommu_sva_unbind_device() - Remove a bond created with
> > > iommu_sva_bind_device
> > > * @handle: the handle returned by iommu_sva_bind_device()
> > > *
> > > * Put reference to a bond between device and address space. The device
> > > should
> > > * not be issuing any more transaction for this PASID. All outstanding page
> > > * requests for this PASID must have been flushed to the IOMMU.
> > > */
> > > void iommu_sva_unbind_device(struct iommu_sva *handle)
> > >
> > > It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
> > > if we can include the device in the unbind() interface.
> >
> > Why would we have a special unbind for SVA?
>
> It's about SVA kAPI for device drivers. The existing kAPIs include:
>
> struct iommu_sva *iommu_sva_bind_device(struct device *dev,
> struct mm_struct *mm,
> void *drvdata);
> void iommu_sva_unbind_device(struct iommu_sva *handle);
> u32 iommu_sva_get_pasid(struct iommu_sva *handle);
This is not what we agreed the API should be. We agreed:
iommu_sva_domain_alloc()
iommu_attach_device_pasid()
iommu_detach_device_pasid()
Again, SVA should not be different from normal domain stuff.
Jason
On 2022/5/12 19:48, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 01:17:08PM +0800, Baolu Lu wrote:
>> On 2022/5/12 13:01, Tian, Kevin wrote:
>>>> From: Baolu Lu <[email protected]>
>>>> Sent: Thursday, May 12, 2022 11:03 AM
>>>>
>>>> On 2022/5/11 22:53, Jason Gunthorpe wrote:
>>>>>>> Also, given the current arrangement it might make sense to have a
>>>>>>> struct iommu_domain_sva given that no driver is wrappering this in
>>>>>>> something else.
>>>>>> Fair enough. How about below wrapper?
>>>>>>
>>>>>> +struct iommu_sva_domain {
>>>>>> + /*
>>>>>> + * Common iommu domain header,*must* be put at the top
>>>>>> + * of the structure.
>>>>>> + */
>>>>>> + struct iommu_domain domain;
>>>>>> + struct mm_struct *mm;
>>>>>> + struct iommu_sva bond;
>>>>>> +}
>>>>>>
>>>>>> The refcount is wrapped in bond.
>>>>> I'm still not sure that bond is necessary
>>>>
>>>> "bond" is the sva handle that the device drivers get through calling
>>>> iommu_sva_bind().
>>>>
>>>
>>> 'bond' was required before because we didn't have a domain to wrap
>>> the page table at that time.
>>>
>>> Now we have a domain and it is 1:1 associated to bond. Probably
>>> make sense now by just returning the domain as the sva handle
>>> instead?
>>
>> It also includes the device information that the domain has been
>> attached. So the sva_unbind() looks like this:
>>
>> /**
>> * iommu_sva_unbind_device() - Remove a bond created with
>> iommu_sva_bind_device
>> * @handle: the handle returned by iommu_sva_bind_device()
>> *
>> * Put reference to a bond between device and address space. The device
>> should
>> * not be issuing any more transaction for this PASID. All outstanding page
>> * requests for this PASID must have been flushed to the IOMMU.
>> */
>> void iommu_sva_unbind_device(struct iommu_sva *handle)
>>
>> It's fine to replace the iommu_sva with iommu_sva_domain for sva handle,
>> if we can include the device in the unbind() interface.
>
> Why would we have a special unbind for SVA?
It's about SVA kAPI for device drivers. The existing kAPIs include:
struct iommu_sva *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm,
void *drvdata);
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
> SVA should not different from normal domains it should use the normal
> detach flow too.
Best regards,
baolu
On 2022/5/12 19:51, Jason Gunthorpe wrote:
> On Thu, May 12, 2022 at 08:00:59AM +0100, Jean-Philippe Brucker wrote:
>
>>> It is not "missing" it is just renamed to blocking_domain->ops->set_dev_pasid()
>>>
>>> The implementation of that function would be identical to
>>> detach_dev_pasid.
>>
>> attach(dev, pasid, sva_domain)
>> detach(dev, pasid, sva_domain)
>>
>> versus
>>
>> set_dev_pasid(dev, pasid, sva_domain)
>> set_dev_pasid(dev, pasid, blocking)
>>
>> we loose the information of the domain previously attached, and the SMMU
>> driver has to retrieve it to find the ASID corresponding to the mm.
>
> It would be easy to have the old domain be an input as well - the core
> code knows it.
Thanks a lot for all suggestions. I have posted a follow-up series for
this:
https://lore.kernel.org/linux-iommu/[email protected]/
Let's discuss this there.
Best regards,
baolu