[RFC 0/3] iommu/arm-smmu: stalling support (v2)

[RFC 0/3] iommu/arm-smmu: stalling support (v2)

[Rob Clark]

Now, as was previously discussed[1], now we have a dt property on the
smmu, to indicate that the smmu supports stalling, as well as a flag
on the domain (set when fault-handler is registered) to indicate that
the domain supports stalling.

I ran into one snag, thanks to the auto-attached DMA domain, since
that doesn't support stalling.  I think when the driver is explicitly
managing the iommu, we should just get rid of the DMA domain.  (But
maybe there is a cleaner way to do this than what I came up with, so
suggestions welcome.)

[1] http://www.spinics.net/lists/linux-arm-msm/msg25357.html

Rob Clark (3):
  iommu: introduce stall/resume support
  iommu/arm-smmu: Add support to opt-in to stalling
  iommu/arm-smmu: detach DMA domain if driver is managing iommu

 .../devicetree/bindings/iommu/arm,smmu.txt         |   3 +
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c              |   2 +-
 drivers/gpu/drm/msm/msm_iommu.c                    |  12 ++-
 drivers/infiniband/hw/usnic/usnic_uiom.c           |   2 +-
 drivers/iommu/arm-smmu.c                           | 100 +++++++++++++++++++--
 drivers/iommu/iommu.c                              |  24 ++++-
 drivers/remoteproc/remoteproc_core.c               |   2 +-
 include/linux/iommu.h                              |   5 +-
 8 files changed, 136 insertions(+), 14 deletions(-)

-- 
2.9.3

[RFC 1/3] iommu: introduce stall/resume support

[Rob Clark]

A new flag when registering the fault handler indicates that the user
supports stalling, and will call iommu_domain_resume() at some point
later, potentially from a workqueue.  (This would allow the user to do
mm related operations that could not be done from irq context.)

Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c    |  2 +-
 drivers/gpu/drm/msm/msm_iommu.c          | 12 +++++++++---
 drivers/infiniband/hw/usnic/usnic_uiom.c |  2 +-
 drivers/iommu/iommu.c                    | 24 +++++++++++++++++++++++-
 drivers/remoteproc/remoteproc_core.c     |  2 +-
 include/linux/iommu.h                    |  5 ++++-
 6 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 169ac96..a8819bc 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -303,7 +303,7 @@ struct etnaviv_iommu *etnaviv_iommu_new(struct etnaviv_gpu *gpu)
 		    mmu->domain->geometry.aperture_end -
 		    mmu->domain->geometry.aperture_start + 1);
 
-	iommu_set_fault_handler(mmu->domain, etnaviv_fault_handler, gpu->dev);
+	iommu_set_fault_handler(mmu->domain, etnaviv_fault_handler, gpu->dev, false);
 
 	return mmu;
 }
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
index 48e79d0..7521582 100644
--- a/drivers/gpu/drm/msm/msm_iommu.c
+++ b/drivers/gpu/drm/msm/msm_iommu.c
@@ -29,9 +29,15 @@ static int msm_fault_handler(struct iommu_domain *domain, struct device *dev,
 		unsigned long iova, int flags, void *arg)
 {
 	struct msm_iommu *iommu = arg;
+	int ret = 0;
+
 	if (iommu->base.handler)
-		return iommu->base.handler(iommu->base.arg, iova, flags);
-	pr_warn_ratelimited("*** fault: iova=%08lx, flags=%d\n", iova, flags);
+		ret = iommu->base.handler(iommu->base.arg, iova, flags);
+	else
+		pr_warn_ratelimited("*** fault: iova=%08lx, flags=%d\n", iova, flags);
+
+	iommu_domain_resume(domain, false);
+
 	return 0;
 }
 
@@ -172,7 +178,7 @@ struct msm_mmu *msm_iommu_new(struct device *dev, struct iommu_domain *domain)
 
 	iommu->domain = domain;
 	msm_mmu_init(&iommu->base, dev, &funcs);
-	iommu_set_fault_handler(domain, msm_fault_handler, iommu);
+	iommu_set_fault_handler(domain, msm_fault_handler, iommu, true);
 
 	if (of_find_compatible_node(NULL, NULL, "qcom,msm-smmu-v2") ||
 			of_find_compatible_node(NULL, NULL, "qcom,msm-mmu-500"))
diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
index 1ccee6e..0613701 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.c
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
@@ -476,7 +476,7 @@ struct usnic_uiom_pd *usnic_uiom_alloc_pd(void)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL);
+	iommu_set_fault_handler(pd->domain, usnic_uiom_dma_fault, NULL, false);
 
 	spin_lock_init(&pd->lock);
 	INIT_LIST_HEAD(&pd->devs);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9a2f196..65257cc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1029,6 +1029,7 @@ EXPORT_SYMBOL_GPL(iommu_capable);
  * @domain: iommu domain
  * @handler: fault handler
  * @token: user data, will be passed back to the fault handler
+ * @can_stall: the user can support stalling on iommu fault
  *
  * This function should be used by IOMMU users which want to be notified
  * whenever an IOMMU fault happens.
@@ -1038,12 +1039,14 @@ EXPORT_SYMBOL_GPL(iommu_capable);
  */
 void iommu_set_fault_handler(struct iommu_domain *domain,
 					iommu_fault_handler_t handler,
-					void *token)
+					void *token,
+					bool can_stall)
 {
 	BUG_ON(!domain);
 
 	domain->handler = handler;
 	domain->handler_token = token;
+	domain->can_stall = can_stall;
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
@@ -1546,6 +1549,25 @@ int iommu_domain_set_attr(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_domain_set_attr);
 
+/**
+ * iommu_domain_resume() - resume a stalled transaction after fault
+ * @domain: iommu domain
+ * @resume: if true, resume the transaction, else abort it
+ *
+ * Users that pass can_stall=true to iommu_set_fault_handler() must
+ * call this function to resume (or terminate) the stalled iommu
+ * transaction.  It may either be called directly from the fault
+ * handler, or at some point later from a thread context (ie. if the
+ * fault handler needs to do anything that cannot be done from atomic
+ * context, ie. use any mm related API)
+ */
+void iommu_domain_resume(struct iommu_domain *domain, bool resume)
+{
+	if (domain->ops->domain_resume)
+		domain->ops->domain_resume(domain, resume);
+}
+EXPORT_SYMBOL_GPL(iommu_domain_resume);
+
 void iommu_get_dm_regions(struct device *dev, struct list_head *list)
 {
 	const struct iommu_ops *ops = dev->bus->iommu_ops;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c6bfb349..5f1aa4c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -110,7 +110,7 @@ static int rproc_enable_iommu(struct rproc *rproc)
 		return -ENOMEM;
 	}
 
-	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc);
+	iommu_set_fault_handler(domain, rproc_iommu_fault, rproc, false);
 
 	ret = iommu_attach_device(domain, dev);
 	if (ret) {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..c428a07 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -82,6 +82,7 @@ struct iommu_domain {
 	unsigned long pgsize_bitmap;	/* Bitmap of page sizes in use */
 	iommu_fault_handler_t handler;
 	void *handler_token;
+	bool can_stall;
 	struct iommu_domain_geometry geometry;
 	void *iova_cookie;
 };
@@ -183,6 +184,7 @@ struct iommu_ops {
 			       enum iommu_attr attr, void *data);
 	int (*domain_set_attr)(struct iommu_domain *domain,
 			       enum iommu_attr attr, void *data);
+	void (*domain_resume)(struct iommu_domain *domain, bool resume);
 
 	/* Request/Free a list of direct mapping requirements for a device */
 	void (*get_dm_regions)(struct device *dev, struct list_head *list);
@@ -231,7 +233,7 @@ extern size_t default_iommu_map_sg(struct iommu_domain *domain, unsigned long io
 				int prot);
 extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova);
 extern void iommu_set_fault_handler(struct iommu_domain *domain,
-			iommu_fault_handler_t handler, void *token);
+			iommu_fault_handler_t handler, void *token, bool can_stall);
 
 extern void iommu_get_dm_regions(struct device *dev, struct list_head *list);
 extern void iommu_put_dm_regions(struct device *dev, struct list_head *list);
@@ -266,6 +268,7 @@ extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
 extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
+extern void iommu_domain_resume(struct iommu_domain *domain, bool resume);
 struct device *iommu_device_create(struct device *parent, void *drvdata,
 				   const struct attribute_group **groups,
 				   const char *fmt, ...) __printf(4, 5);
-- 
2.9.3

[RFC 2/3] iommu/arm-smmu: Add support to opt-in to stalling

[Rob Clark]

TODO maybe some dev_dbg() or some other way to tell if stalling is
actually enabled?

Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/iommu/arm,smmu.txt         |  3 +
 drivers/iommu/arm-smmu.c                           | 85 ++++++++++++++++++++--
 2 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
index ef465b0..5f405a6 100644
--- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
+++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
@@ -68,6 +68,9 @@ conditions.
                   aliases of secure registers have to be used during
                   SMMU configuration.
 
+- arm,smmu-enable-stall : Enable stall mode to stall memory transactions
+                  and resume after fault is handled
+
 ** Deprecated properties:
 
 - mmu-masters (deprecated in favour of the generic "iommus" binding) :
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d505432..96a1be6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -350,6 +350,7 @@ struct arm_smmu_device {
 	u32				features;
 
 #define ARM_SMMU_OPT_SECURE_CFG_ACCESS (1 << 0)
+#define ARM_SMMU_OPT_ENABLE_STALL      (1 << 1)
 	u32				options;
 	enum arm_smmu_arch_version	version;
 	enum arm_smmu_implementation	model;
@@ -377,6 +378,8 @@ struct arm_smmu_device {
 	int                             num_clocks;
 	struct clk                      **clocks;
 
+	struct list_head		domain_list;
+
 	u32				cavium_id_base; /* Specific to Cavium */
 };
 
@@ -412,6 +415,7 @@ struct arm_smmu_domain {
 	enum arm_smmu_domain_stage	stage;
 	struct mutex			init_mutex; /* Protects smmu pointer */
 	struct iommu_domain		domain;
+	struct list_head		domain_node;
 };
 
 struct arm_smmu_option_prop {
@@ -425,6 +429,7 @@ static bool using_legacy_binding, using_generic_binding;
 
 static struct arm_smmu_option_prop arm_smmu_options[] = {
 	{ ARM_SMMU_OPT_SECURE_CFG_ACCESS, "calxeda,smmu-secure-config-access" },
+	{ ARM_SMMU_OPT_ENABLE_STALL,      "arm,smmu-enable-stall" },
 	{ 0, NULL},
 };
 
@@ -676,7 +681,8 @@ static struct iommu_gather_ops arm_smmu_gather_ops = {
 
 static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 {
-	u32 fsr, fsynr;
+	int flags, ret;
+	u32 fsr, fsynr, resume;
 	unsigned long iova;
 	struct iommu_domain *domain = dev;
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -690,15 +696,48 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 	if (!(fsr & FSR_FAULT))
 		return IRQ_NONE;
 
+	if (fsr & FSR_IGN)
+		dev_err_ratelimited(smmu->dev,
+				    "Unexpected context fault (fsr 0x%x)\n",
+				    fsr);
+
 	fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
-	iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
+	flags = fsynr & FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ;
 
-	dev_err_ratelimited(smmu->dev,
-	"Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x, cb=%d\n",
-			    fsr, iova, fsynr, cfg->cbndx);
+	iova = readq_relaxed(cb_base + ARM_SMMU_CB_FAR);
+	if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
+		ret = IRQ_HANDLED;
+		resume = RESUME_RETRY;
+	} else {
+		dev_err_ratelimited(smmu->dev,
+		    "Unhandled context fault: iova=0x%08lx, fsynr=0x%x, cb=%d\n",
+		    iova, fsynr, cfg->cbndx);
+		ret = IRQ_NONE;
+		resume = RESUME_TERMINATE;
+	}
 
+	/* Clear the faulting FSR */
 	writel(fsr, cb_base + ARM_SMMU_CB_FSR);
-	return IRQ_HANDLED;
+
+	if ((fsr & FSR_SS) && !domain->can_stall) {
+		/* Retry or terminate any stalled transactions */
+		writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
+	}
+
+	return ret;
+}
+
+static void arm_smmu_domain_resume(struct iommu_domain *domain, bool resume)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
+	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	void __iomem *cb_base;
+
+	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
+
+	writel_relaxed(resume ? RESUME_RETRY : RESUME_TERMINATE,
+			cb_base + ARM_SMMU_CB_RESUME);
 }
 
 static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
@@ -725,6 +764,20 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static bool arm_smmu_can_stall(struct arm_smmu_device *smmu)
+{
+	struct arm_smmu_domain *smmu_domain;
+
+	if (!(smmu->options & ARM_SMMU_OPT_ENABLE_STALL))
+		return false;
+
+	list_for_each_entry(smmu_domain, &smmu->domain_list, domain_node)
+		if (!smmu_domain->domain.can_stall)
+			return false;
+
+	return true;
+}
+
 static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 				       struct io_pgtable_cfg *pgtbl_cfg)
 {
@@ -824,6 +877,8 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain,
 
 	/* SCTLR */
 	reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_AFE | SCTLR_TRE | SCTLR_M;
+	if (arm_smmu_can_stall(smmu))
+		reg |= SCTLR_CFCFG;
 	if (stage1)
 		reg |= SCTLR_S1_ASIDPNE;
 #ifdef __BIG_ENDIAN
@@ -1049,6 +1104,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
+	INIT_LIST_HEAD(&smmu_domain->domain_node);
 
 	return &smmu_domain->domain;
 }
@@ -1246,6 +1302,13 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
+static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
+{
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+
+	list_del_init(&smmu_domain->domain_node);
+}
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret;
@@ -1259,6 +1322,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	smmu = fwspec_smmu(fwspec);
+
+	if (WARN_ON(!list_empty(&smmu_domain->domain_node)))
+		return -EINVAL;
+
+	list_add_tail(&smmu_domain->domain_node, &smmu->domain_list);
+
 	/* Ensure that the domain is finalised */
 	ret = arm_smmu_init_domain_context(domain, smmu);
 	if (ret < 0)
@@ -1578,6 +1647,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.domain_free		= arm_smmu_domain_free,
+	.detach_dev		= arm_smmu_detach_dev,
 	.attach_dev		= arm_smmu_attach_dev,
 	.map			= arm_smmu_map,
 	.unmap			= arm_smmu_unmap,
@@ -1588,6 +1658,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= arm_smmu_device_group,
 	.domain_get_attr	= arm_smmu_domain_get_attr,
 	.domain_set_attr	= arm_smmu_domain_set_attr,
+	.domain_resume		= arm_smmu_domain_resume,
 	.of_xlate		= arm_smmu_of_xlate,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
@@ -2001,6 +2072,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	}
 	smmu->dev = dev;
 
+	INIT_LIST_HEAD(&smmu->domain_list);
+
 	data = of_device_get_match_data(dev);
 	smmu->version = data->version;
 	smmu->model = data->model;
-- 
2.9.3

[RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Rob Clark]

Before the driver is probed, arm_smmu_add_device() helpfully attaches
an IOMMU_DOMAIN_DMA domain.  Which ofc does not support stalling, and
when the driver later attaches a domain that can_stall to an smmu that
can stall, the default _DMA domain prevents stalling from being enabled.
(And will cause further problems later)

One simple way to deal with this is simply toss the default _DMA domain
if the driver attaches it's own domain.

TODO maybe the tracking of list of attached domains should be done in
iommu core, so the detach can happen outside of group->mutex.

Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 drivers/iommu/arm-smmu.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 96a1be6..50bf135 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	smmu = fwspec_smmu(fwspec);
 
+	/*
+	 * If driver is explicitly managing the iommu, detatch any previously
+	 * attached _DMA domains.
+	 *
+	 * TODO maybe this logic should be in iommu_attach_device() so it can
+	 * happen outside of holding group->mutex??
+	 */
+	if (domain->type != IOMMU_DOMAIN_DMA) {
+		struct arm_smmu_domain *other_domain, *n;
+
+		list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node)
+			if (other_domain->domain.type == IOMMU_DOMAIN_DMA)
+				arm_smmu_detach_dev(&other_domain->domain, dev);
+	}
+
 	if (WARN_ON(!list_empty(&smmu_domain->domain_node)))
 		return -EINVAL;
 
-- 
2.9.3

Re: [RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Rob Clark]

On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Before the driver is probed, arm_smmu_add_device() helpfully attaches
> an IOMMU_DOMAIN_DMA domain.  Which ofc does not support stalling, and
> when the driver later attaches a domain that can_stall to an smmu that
> can stall, the default _DMA domain prevents stalling from being enabled.
> (And will cause further problems later)
>
> One simple way to deal with this is simply toss the default _DMA domain
> if the driver attaches it's own domain.
>
> TODO maybe the tracking of list of attached domains should be done in
> iommu core, so the detach can happen outside of group->mutex.
>
> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 96a1be6..50bf135 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>
>         smmu = fwspec_smmu(fwspec);
>
> +       /*
> +        * If driver is explicitly managing the iommu, detatch any previously
> +        * attached _DMA domains.
> +        *
> +        * TODO maybe this logic should be in iommu_attach_device() so it can
> +        * happen outside of holding group->mutex??
> +        */
> +       if (domain->type != IOMMU_DOMAIN_DMA) {
> +               struct arm_smmu_domain *other_domain, *n;
> +
> +               list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node)
> +                       if (other_domain->domain.type == IOMMU_DOMAIN_DMA)
> +                               arm_smmu_detach_dev(&other_domain->domain, dev);

hmm, we might want to unhook dev->archdata.dma_ops here too..

I'm thinking maybe on arm64 __generic_dma_ops() should fall back to
swiotlb ops instead of dummy_ops if archdata.dma_ops is NULL, so that
we could just set it to NULL here?

(Is there really any purpose for having the dummy-ops??)

BR,
-R

> +       }
> +
>         if (WARN_ON(!list_empty(&smmu_domain->domain_node)))
>                 return -EINVAL;
>
> --
> 2.9.3
>

RE: [RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Sricharan]

Hi Rob,

>On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Before the driver is probed, arm_smmu_add_device() helpfully attaches
>> an IOMMU_DOMAIN_DMA domain.  Which ofc does not support stalling, and
>> when the driver later attaches a domain that can_stall to an smmu that
>> can stall, the default _DMA domain prevents stalling from being enabled.
>> (And will cause further problems later)
>>
>> One simple way to deal with this is simply toss the default _DMA domain
>> if the driver attaches it's own domain.
>>
>> TODO maybe the tracking of list of attached domains should be done in
>> iommu core, so the detach can happen outside of group->mutex.
>>
>> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 96a1be6..50bf135 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>
>>         smmu = fwspec_smmu(fwspec);
>>
>> +       /*
>> +        * If driver is explicitly managing the iommu, detatch any previously
>> +        * attached _DMA domains.
>> +        *
>> +        * TODO maybe this logic should be in iommu_attach_device() so it can
>> +        * happen outside of holding group->mutex??
>> +        */
>> +       if (domain->type != IOMMU_DOMAIN_DMA) {
>> +               struct arm_smmu_domain *other_domain, *n;
>> +
>> +               list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node)
>> +                       if (other_domain->domain.type == IOMMU_DOMAIN_DMA)
>> +                               arm_smmu_detach_dev(&other_domain->domain, dev);
>

So the arm_smmu_detach_dev api is no more there and is removed now.
Also this will be a problem when multiple devices share the iommu, we end up
removing domains used by other devices. Should this not be done
per-device which does not want to use the DMA domain ?

>hmm, we might want to unhook dev->archdata.dma_ops here too..
>
>I'm thinking maybe on arm64 __generic_dma_ops() should fall back to
>swiotlb ops instead of dummy_ops if archdata.dma_ops is NULL, so that
>we could just set it to NULL here?
>

hmm, both not attaching the default dma domain and not setting the dma_ops
is tried in this series as well [1]

[1] https://www.spinics.net/lists/arm-kernel/msg556081.html

>(Is there really any purpose for having the dummy-ops??)
>

To enforce setting arch_setup_dma_ops for device so that the
devices can do cache coherent transactions, otherwise disable the dma
capability of the device. I see that this was introduced as a part of
making ACPI_CCA_REQUIRED to be set in arm64 and later
generalized.


Regards,
 Sricharan

>BR,
>-R
>
>> +       }
>> +
>>         if (WARN_ON(!list_empty(&smmu_domain->domain_node)))
>>                 return -EINVAL;
>>
>> --
>> 2.9.3
>>

Re: [RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Rob Clark]

On Wed, Feb 1, 2017 at 11:10 PM, Sricharan <sricharan@codeaurora.org> wrote:
> Hi Rob,
>
>>On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark@gmail.com> wrote:
>>> Before the driver is probed, arm_smmu_add_device() helpfully attaches
>>> an IOMMU_DOMAIN_DMA domain.  Which ofc does not support stalling, and
>>> when the driver later attaches a domain that can_stall to an smmu that
>>> can stall, the default _DMA domain prevents stalling from being enabled.
>>> (And will cause further problems later)
>>>
>>> One simple way to deal with this is simply toss the default _DMA domain
>>> if the driver attaches it's own domain.
>>>
>>> TODO maybe the tracking of list of attached domains should be done in
>>> iommu core, so the detach can happen outside of group->mutex.
>>>
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> ---
>>>  drivers/iommu/arm-smmu.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 96a1be6..50bf135 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>
>>>         smmu = fwspec_smmu(fwspec);
>>>
>>> +       /*
>>> +        * If driver is explicitly managing the iommu, detatch any previously
>>> +        * attached _DMA domains.
>>> +        *
>>> +        * TODO maybe this logic should be in iommu_attach_device() so it can
>>> +        * happen outside of holding group->mutex??
>>> +        */
>>> +       if (domain->type != IOMMU_DOMAIN_DMA) {
>>> +               struct arm_smmu_domain *other_domain, *n;
>>> +
>>> +               list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node)
>>> +                       if (other_domain->domain.type == IOMMU_DOMAIN_DMA)
>>> +                               arm_smmu_detach_dev(&other_domain->domain, dev);
>>
>
> So the arm_smmu_detach_dev api is no more there and is removed now.
> Also this will be a problem when multiple devices share the iommu, we end up
> removing domains used by other devices. Should this not be done
> per-device which does not want to use the DMA domain ?

I actually added the _detach fxn in the previous patch.

But I think managing the iommu explicitly is not going to work if
multiple devices share the same pagetable.  I think that is a scenario
that should never happen with the gpu.

>>hmm, we might want to unhook dev->archdata.dma_ops here too..
>>
>>I'm thinking maybe on arm64 __generic_dma_ops() should fall back to
>>swiotlb ops instead of dummy_ops if archdata.dma_ops is NULL, so that
>>we could just set it to NULL here?
>>
>
> hmm, both not attaching the default dma domain and not setting the dma_ops
> is tried in this series as well [1]
>
> [1] https://www.spinics.net/lists/arm-kernel/msg556081.html

I'll have a look at that series later this morning.

>>(Is there really any purpose for having the dummy-ops??)
>>
>
> To enforce setting arch_setup_dma_ops for device so that the
> devices can do cache coherent transactions, otherwise disable the dma
> capability of the device. I see that this was introduced as a part of
> making ACPI_CCA_REQUIRED to be set in arm64 and later
> generalized.

hmm, looks like all the dummy ops just fail.  I'm not entirely sure
how that can be useful.  But I'll poke around the git history.  Seems
like just always falling back to swiotlb op's would be the equivalent
to armv7 falling back to arm_dma_ops..

BR,
-R

>
> Regards,
>  Sricharan
>
>>BR,
>>-R
>>
>>> +       }
>>> +
>>>         if (WARN_ON(!list_empty(&smmu_domain->domain_node)))
>>>                 return -EINVAL;
>>>
>>> --
>>> 2.9.3
>>>
>

Re: [RFC 3/3] iommu/arm-smmu: detach DMA domain if driver is managing iommu

[Rob Clark]

On Wed, Feb 1, 2017 at 11:10 PM, Sricharan <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> Hi Rob,
>
>>On Wed, Feb 1, 2017 at 10:23 AM, Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Before the driver is probed, arm_smmu_add_device() helpfully attaches
>>> an IOMMU_DOMAIN_DMA domain.  Which ofc does not support stalling, and
>>> when the driver later attaches a domain that can_stall to an smmu that
>>> can stall, the default _DMA domain prevents stalling from being enabled.
>>> (And will cause further problems later)
>>>
>>> One simple way to deal with this is simply toss the default _DMA domain
>>> if the driver attaches it's own domain.
>>>
>>> TODO maybe the tracking of list of attached domains should be done in
>>> iommu core, so the detach can happen outside of group->mutex.
>>>
>>> Signed-off-by: Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/iommu/arm-smmu.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index 96a1be6..50bf135 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -1323,6 +1323,21 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>
>>>         smmu = fwspec_smmu(fwspec);
>>>
>>> +       /*
>>> +        * If driver is explicitly managing the iommu, detatch any previously
>>> +        * attached _DMA domains.
>>> +        *
>>> +        * TODO maybe this logic should be in iommu_attach_device() so it can
>>> +        * happen outside of holding group->mutex??
>>> +        */
>>> +       if (domain->type != IOMMU_DOMAIN_DMA) {
>>> +               struct arm_smmu_domain *other_domain, *n;
>>> +
>>> +               list_for_each_entry_safe(other_domain, n, &smmu->domain_list, domain_node)
>>> +                       if (other_domain->domain.type == IOMMU_DOMAIN_DMA)
>>> +                               arm_smmu_detach_dev(&other_domain->domain, dev);
>>
>
> So the arm_smmu_detach_dev api is no more there and is removed now.
> Also this will be a problem when multiple devices share the iommu, we end up
> removing domains used by other devices. Should this not be done
> per-device which does not want to use the DMA domain ?
>
>>hmm, we might want to unhook dev->archdata.dma_ops here too..
>>
>>I'm thinking maybe on arm64 __generic_dma_ops() should fall back to
>>swiotlb ops instead of dummy_ops if archdata.dma_ops is NULL, so that
>>we could just set it to NULL here?
>>
>
> hmm, both not attaching the default dma domain and not setting the dma_ops
> is tried in this series as well [1]
>
> [1] https://www.spinics.net/lists/arm-kernel/msg556081.html
>
>>(Is there really any purpose for having the dummy-ops??)
>>
>
> To enforce setting arch_setup_dma_ops for device so that the
> devices can do cache coherent transactions, otherwise disable the dma
> capability of the device. I see that this was introduced as a part of
> making ACPI_CCA_REQUIRED to be set in arm64 and later
> generalized.

hmm, maybe fallback to swiotlb ops vs dummy ops should depend on
whether it is an ACPI system or not..

BR,
-R