[PATCH 0/4] Miscellaneous ARM SMMU patches

[PATCH 0/4] Miscellaneous ARM SMMU patches

[Robin Murphy]

Hi Will,

Here's my current "miscellaneous SMMU enhancements" branch for your
consideration. Patch #1 solves a subtle corner case that cropped up
while exercising stage 1 context formats on the DMA330-MMU500 model;
#3 will be wanted fairly soon for DMA ops integration so may as well
get some exposure now; #2 and #4 are more nice-to-haves.

Thanks,
Robin.

Robin Murphy (4):
  iommu/arm-smmu: Treat all device transactions as unprivileged
  iommu/arm-smmu: Allow disabling unmatched stream bypass
  iommu/arm-smmu: Support DMA-API domains
  iommu/arm-smmu: Use per-context TLB sync as appropriate

 drivers/iommu/arm-smmu-v3.c | 10 +++++-
 drivers/iommu/arm-smmu.c    | 79 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 69 insertions(+), 20 deletions(-)

-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 0/4] Miscellaneous ARM SMMU patches

[Robin Murphy]

Hi Will,

Here's my current "miscellaneous SMMU enhancements" branch for your
consideration. Patch #1 solves a subtle corner case that cropped up
while exercising stage 1 context formats on the DMA330-MMU500 model;
#3 will be wanted fairly soon for DMA ops integration so may as well
get some exposure now; #2 and #4 are more nice-to-haves.

Thanks,
Robin.

Robin Murphy (4):
  iommu/arm-smmu: Treat all device transactions as unprivileged
  iommu/arm-smmu: Allow disabling unmatched stream bypass
  iommu/arm-smmu: Support DMA-API domains
  iommu/arm-smmu: Use per-context TLB sync as appropriate

 drivers/iommu/arm-smmu-v3.c | 10 +++++-
 drivers/iommu/arm-smmu.c    | 79 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 69 insertions(+), 20 deletions(-)

-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Robin Murphy]

The IOMMU API has no concept of privilege so assumes all devices and
mappings are equal, and indeed most non-CPU master devices on an AMBA
interconnect make little use of the attribute bits on the bus thus by
default perform unprivileged data accesses.

Some devices, however, believe themselves more equal than others, such
as programmable DMA controllers whose 'master' thread issues bus
transactions marked as privileged instruction fetches, while the data
accesses of its channel threads (under the control of Linux, at least)
are marked as unprivileged. This poses a problem for implementing the
DMA API on an IOMMU conforming to ARM VMSAv8, under which a page that is
unprivileged-writeable is also implicitly privileged-execute-never.
Given that, there is no one set of attributes with which iommu_map() can
implement, say, dma_alloc_coherent() that will allow every possible type
of access without something running into unexecepted permission faults.

Fortunately the SMMU architecture provides a means to mitigate such
issues by overriding the incoming attributes of a transaction; make use
of that to strip the privileged/unprivileged status off incoming
transactions, leaving just the instruction/data dichotomy which the
IOMMU API does at least understand; Four states good, two states better.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..1f9093d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -167,6 +167,9 @@
 #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
 #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
 
+#define S2CR_PRIVCFG_SHIFT		24
+#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
+
 /* Context bank attribute registers */
 #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
 #define CBAR_VMID_SHIFT			0
@@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		u32 idx, s2cr;
 
 		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
-		s2cr = S2CR_TYPE_TRANS |
+		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Robin Murphy]

The IOMMU API has no concept of privilege so assumes all devices and
mappings are equal, and indeed most non-CPU master devices on an AMBA
interconnect make little use of the attribute bits on the bus thus by
default perform unprivileged data accesses.

Some devices, however, believe themselves more equal than others, such
as programmable DMA controllers whose 'master' thread issues bus
transactions marked as privileged instruction fetches, while the data
accesses of its channel threads (under the control of Linux, at least)
are marked as unprivileged. This poses a problem for implementing the
DMA API on an IOMMU conforming to ARM VMSAv8, under which a page that is
unprivileged-writeable is also implicitly privileged-execute-never.
Given that, there is no one set of attributes with which iommu_map() can
implement, say, dma_alloc_coherent() that will allow every possible type
of access without something running into unexecepted permission faults.

Fortunately the SMMU architecture provides a means to mitigate such
issues by overriding the incoming attributes of a transaction; make use
of that to strip the privileged/unprivileged status off incoming
transactions, leaving just the instruction/data dichotomy which the
IOMMU API does at least understand; Four states good, two states better.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 59ee4b8..1f9093d 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -167,6 +167,9 @@
 #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
 #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
 
+#define S2CR_PRIVCFG_SHIFT		24
+#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
+
 /* Context bank attribute registers */
 #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
 #define CBAR_VMID_SHIFT			0
@@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		u32 idx, s2cr;
 
 		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
-		s2cr = S2CR_TYPE_TRANS |
+		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
debugging/security feature so that unmatched stream IDs (i.e. devices
not attached to an IOMMU domain) may be configured to fault.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1f9093d..d1b7dc1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -141,6 +141,8 @@
 #define ID2_PTFS_16K			(1 << 13)
 #define ID2_PTFS_64K			(1 << 14)
 
+#define sGFSR_USF			(1 << 1)
+
 /* Global TLB invalidation */
 #define ARM_SMMU_GR0_TLBIVMID		0x64
 #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
@@ -263,6 +265,10 @@ static int force_stage;
 module_param_named(force_stage, force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
+static bool disable_bypass;
+module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_bypass,
+	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
 
 enum arm_smmu_arch_version {
 	ARM_SMMU_V1 = 1,
@@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 	if (!gfsr)
 		return IRQ_NONE;
 
-	dev_err_ratelimited(smmu->dev,
-		"Unexpected global fault, this could be serious\n");
+	if (gfsr & sGFSR_USF)
+		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
+				    gfsynr1 & SMR_ID_MASK);
+	else
+		dev_err_ratelimited(smmu->dev,
+			"Unexpected global fault, this could be serious\n");
 	dev_err_ratelimited(smmu->dev,
 		"\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 0x%08x\n",
 		gfsr, gfsynr0, gfsynr1, gfsynr2);
@@ -1111,9 +1121,9 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		u32 reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
 	arm_smmu_master_free_smrs(smmu, cfg);
@@ -1476,11 +1486,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
-	/* Mark all SMRn as invalid and all S2CRn as bypass */
+	/* Mark all SMRn as invalid and all S2CRn as bypass unless overridden */
+	reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
 		writel_relaxed(0, gr0_base + ARM_SMMU_GR0_SMR(i));
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			gr0_base + ARM_SMMU_GR0_S2CR(i));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(i));
 	}
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
@@ -1502,8 +1512,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	/* Disable TLB broadcasting. */
 	reg |= (sCR0_VMIDPNE | sCR0_PTM);
 
-	/* Enable client access, but bypass when no mapping is found */
-	reg &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	/* Enable client access, handling unmatched streams as appropriate */
+	reg &= ~sCR0_CLIENTPD;
+	if (disable_bypass)
+		reg |= sCR0_USFCFG;
+	else
+		reg &= ~sCR0_USFCFG;
 
 	/* Disable forced broadcasting */
 	reg &= ~sCR0_FB;
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
debugging/security feature so that unmatched stream IDs (i.e. devices
not attached to an IOMMU domain) may be configured to fault.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1f9093d..d1b7dc1 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -141,6 +141,8 @@
 #define ID2_PTFS_16K			(1 << 13)
 #define ID2_PTFS_64K			(1 << 14)
 
+#define sGFSR_USF			(1 << 1)
+
 /* Global TLB invalidation */
 #define ARM_SMMU_GR0_TLBIVMID		0x64
 #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
@@ -263,6 +265,10 @@ static int force_stage;
 module_param_named(force_stage, force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed@a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
+static bool disable_bypass;
+module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_bypass,
+	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
 
 enum arm_smmu_arch_version {
 	ARM_SMMU_V1 = 1,
@@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
 	if (!gfsr)
 		return IRQ_NONE;
 
-	dev_err_ratelimited(smmu->dev,
-		"Unexpected global fault, this could be serious\n");
+	if (gfsr & sGFSR_USF)
+		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
+				    gfsynr1 & SMR_ID_MASK);
+	else
+		dev_err_ratelimited(smmu->dev,
+			"Unexpected global fault, this could be serious\n");
 	dev_err_ratelimited(smmu->dev,
 		"\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 0x%08x\n",
 		gfsr, gfsynr0, gfsynr1, gfsynr2);
@@ -1111,9 +1121,9 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		u32 reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
 	arm_smmu_master_free_smrs(smmu, cfg);
@@ -1476,11 +1486,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
-	/* Mark all SMRn as invalid and all S2CRn as bypass */
+	/* Mark all SMRn as invalid and all S2CRn as bypass unless overridden */
+	reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
 		writel_relaxed(0, gr0_base + ARM_SMMU_GR0_SMR(i));
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			gr0_base + ARM_SMMU_GR0_S2CR(i));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(i));
 	}
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
@@ -1502,8 +1512,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	/* Disable TLB broadcasting. */
 	reg |= (sCR0_VMIDPNE | sCR0_PTM);
 
-	/* Enable client access, but bypass when no mapping is found */
-	reg &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	/* Enable client access, handling unmatched streams as appropriate */
+	reg &= ~sCR0_CLIENTPD;
+	if (disable_bypass)
+		reg |= sCR0_USFCFG;
+	else
+		reg &= ~sCR0_USFCFG;
 
 	/* Disable forced broadcasting */
 	reg &= ~sCR0_FB;
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 3/4] iommu/arm-smmu: Support DMA-API domains

[Robin Murphy]

With DMA mapping ops provided by the iommu-dma code, only a minimal
contribution from the IOMMU driver is needed to create a suitable
DMA-API domain for them to use. Implement this for the ARM SMMUs.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu-v3.c | 10 +++++++++-
 drivers/iommu/arm-smmu.c    | 10 +++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2087534..f003b3a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
@@ -1396,7 +1397,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	/*
@@ -1408,6 +1409,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	if (!smmu_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
+		kfree(smmu_domain);
+		return NULL;
+	}
+
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
 	return &smmu_domain->domain;
@@ -1436,6 +1443,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
+	iommu_put_dma_cookie(domain);
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 
 	/* Free the CD and ASID, if we allocated them */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d1b7dc1..18e0e10 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -29,6 +29,7 @@
 #define pr_fmt(fmt) "arm-smmu: " fmt
 
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -976,7 +977,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
@@ -987,6 +988,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	if (!smmu_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
+		kfree(smmu_domain);
+		return NULL;
+	}
+
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
 
@@ -1001,6 +1008,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
+	iommu_put_dma_cookie(domain);
 	arm_smmu_destroy_domain_context(domain);
 	kfree(smmu_domain);
 }
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 3/4] iommu/arm-smmu: Support DMA-API domains

[Robin Murphy]

With DMA mapping ops provided by the iommu-dma code, only a minimal
contribution from the IOMMU driver is needed to create a suitable
DMA-API domain for them to use. Implement this for the ARM SMMUs.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu-v3.c | 10 +++++++++-
 drivers/iommu/arm-smmu.c    | 10 +++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 2087534..f003b3a 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -21,6 +21,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/iommu.h>
@@ -1396,7 +1397,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	/*
@@ -1408,6 +1409,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	if (!smmu_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
+		kfree(smmu_domain);
+		return NULL;
+	}
+
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
 	return &smmu_domain->domain;
@@ -1436,6 +1443,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 
+	iommu_put_dma_cookie(domain);
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
 
 	/* Free the CD and ASID, if we allocated them */
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d1b7dc1..18e0e10 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -29,6 +29,7 @@
 #define pr_fmt(fmt) "arm-smmu: " fmt
 
 #include <linux/delay.h>
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -976,7 +977,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 	/*
 	 * Allocate the domain and initialise some of its data structures.
@@ -987,6 +988,12 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	if (!smmu_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&smmu_domain->domain)) {
+		kfree(smmu_domain);
+		return NULL;
+	}
+
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->pgtbl_lock);
 
@@ -1001,6 +1008,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
+	iommu_put_dma_cookie(domain);
 	arm_smmu_destroy_domain_context(domain);
 	kfree(smmu_domain);
 }
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Robin Murphy]

TLB synchronisation is a mighty big hammmer to bring down on the
transaction stream, typically stalling all in-flight transactions until
the sync completes. Since in most cases (except at stage 2 on SMMUv1)
a per-context sync operation is available, prefer that over the global
operation when performing TLB maintenance for a single domain, to avoid
unecessarily disrupting ongoing traffic in other contexts.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 18e0e10..bf1895c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -219,6 +219,8 @@
 #define ARM_SMMU_CB_S1_TLBIVAL		0x620
 #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
 #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
+#define ARM_SMMU_CB_TLBSYNC		0x7f0
+#define ARM_SMMU_CB_TLBSTATUS		0x7f4
 #define ARM_SMMU_CB_ATS1PR		0x800
 #define ARM_SMMU_CB_ATSR		0x8f0
 
@@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
 }
 
 /* Wait for any pending TLB invalidations to complete */
-static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
+static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
 {
 	int count = 0;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	void __iomem *base, __iomem *status;
 
-	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
-	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
-	       & sTLBGSTATUS_GSACTIVE) {
+	if (cbndx < 0) {
+		base = ARM_SMMU_GR0(smmu);
+		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
+		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
+	} else {
+		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
+		status = base + ARM_SMMU_CB_TLBSTATUS;
+		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
+	}
+
+	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
 		cpu_relax();
 		if (++count == TLB_LOOP_TIMEOUT) {
 			dev_err_ratelimited(smmu->dev,
@@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
 static void arm_smmu_tlb_sync(void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
-	__arm_smmu_tlb_sync(smmu_domain->smmu);
+	int cbndx = smmu_domain->cfg.cbndx;
+
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
+	    smmu_domain->smmu->version < ARM_SMMU_V2)
+		cbndx = -1;
+
+	__arm_smmu_tlb_sync(smmu_domain->smmu, cbndx);
 }
 
 static void arm_smmu_tlb_inv_context(void *cookie)
@@ -588,7 +604,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 			       base + ARM_SMMU_GR0_TLBIVMID);
 	}
 
-	__arm_smmu_tlb_sync(smmu);
+	__arm_smmu_tlb_sync(smmu, cfg->cbndx);
 }
 
 static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
@@ -1534,7 +1550,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
-	__arm_smmu_tlb_sync(smmu);
+	__arm_smmu_tlb_sync(smmu, -1);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 }
 
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Robin Murphy]

TLB synchronisation is a mighty big hammmer to bring down on the
transaction stream, typically stalling all in-flight transactions until
the sync completes. Since in most cases (except at stage 2 on SMMUv1)
a per-context sync operation is available, prefer that over the global
operation when performing TLB maintenance for a single domain, to avoid
unecessarily disrupting ongoing traffic in other contexts.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 18e0e10..bf1895c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -219,6 +219,8 @@
 #define ARM_SMMU_CB_S1_TLBIVAL		0x620
 #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
 #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
+#define ARM_SMMU_CB_TLBSYNC		0x7f0
+#define ARM_SMMU_CB_TLBSTATUS		0x7f4
 #define ARM_SMMU_CB_ATS1PR		0x800
 #define ARM_SMMU_CB_ATSR		0x8f0
 
@@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
 }
 
 /* Wait for any pending TLB invalidations to complete */
-static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
+static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
 {
 	int count = 0;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	void __iomem *base, __iomem *status;
 
-	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
-	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
-	       & sTLBGSTATUS_GSACTIVE) {
+	if (cbndx < 0) {
+		base = ARM_SMMU_GR0(smmu);
+		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
+		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
+	} else {
+		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
+		status = base + ARM_SMMU_CB_TLBSTATUS;
+		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
+	}
+
+	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
 		cpu_relax();
 		if (++count == TLB_LOOP_TIMEOUT) {
 			dev_err_ratelimited(smmu->dev,
@@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
 static void arm_smmu_tlb_sync(void *cookie)
 {
 	struct arm_smmu_domain *smmu_domain = cookie;
-	__arm_smmu_tlb_sync(smmu_domain->smmu);
+	int cbndx = smmu_domain->cfg.cbndx;
+
+	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
+	    smmu_domain->smmu->version < ARM_SMMU_V2)
+		cbndx = -1;
+
+	__arm_smmu_tlb_sync(smmu_domain->smmu, cbndx);
 }
 
 static void arm_smmu_tlb_inv_context(void *cookie)
@@ -588,7 +604,7 @@ static void arm_smmu_tlb_inv_context(void *cookie)
 			       base + ARM_SMMU_GR0_TLBIVMID);
 	}
 
-	__arm_smmu_tlb_sync(smmu);
+	__arm_smmu_tlb_sync(smmu, cfg->cbndx);
 }
 
 static void arm_smmu_tlb_inv_range_nosync(unsigned long iova, size_t size,
@@ -1534,7 +1550,7 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg &= ~(sCR0_BSU_MASK << sCR0_BSU_SHIFT);
 
 	/* Push the button */
-	__arm_smmu_tlb_sync(smmu);
+	__arm_smmu_tlb_sync(smmu, -1);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
 }
 
-- 
2.7.0.25.gfc10eb5.dirty

RE: [PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Anup Patel]

Hi Robin,

> -----Original Message-----
> From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-
> bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] On Behalf Of Robin Murphy
> Sent: 26 January 2016 23:37
> To: will.deacon-5wv7dgnIgG8@public.gmane.org
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org;
> tchalamarla-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org
> Subject: [PATCH 1/4] iommu/arm-smmu: Treat all device transactions as
> unprivileged
> 
> The IOMMU API has no concept of privilege so assumes all devices and
> mappings are equal, and indeed most non-CPU master devices on an AMBA
> interconnect make little use of the attribute bits on the bus thus by default
> perform unprivileged data accesses.
> 
> Some devices, however, believe themselves more equal than others, such as
> programmable DMA controllers whose 'master' thread issues bus transactions
> marked as privileged instruction fetches, while the data accesses of its channel
> threads (under the control of Linux, at least) are marked as unprivileged. This
> poses a problem for implementing the DMA API on an IOMMU conforming to
> ARM VMSAv8, under which a page that is unprivileged-writeable is also implicitly
> privileged-execute-never.
> Given that, there is no one set of attributes with which iommu_map() can
> implement, say, dma_alloc_coherent() that will allow every possible type of
> access without something running into unexecepted permission faults.
> 
> Fortunately the SMMU architecture provides a means to mitigate such issues by
> overriding the incoming attributes of a transaction; make use of that to strip the
> privileged/unprivileged status off incoming transactions, leaving just the
> instruction/data dichotomy which the IOMMU API does at least understand;
> Four states good, two states better.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> 59ee4b8..1f9093d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -167,6 +167,9 @@
>  #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>  #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
> 
> +#define S2CR_PRIVCFG_SHIFT		24
> +#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
> +
>  /* Context bank attribute registers */
>  #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
>  #define CBAR_VMID_SHIFT			0
> @@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct
> arm_smmu_domain *smmu_domain,
>  		u32 idx, s2cr;
> 
>  		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> -		s2cr = S2CR_TYPE_TRANS |
> +		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
>  		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);

Treating all MMU master access as unprivileged sounds more
conservative. Alternate approach would be to treat instruction
fetches as data reads.

Regards,
Anup

[PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Anup Patel]

Hi Robin,

> -----Original Message-----
> From: iommu-bounces at lists.linux-foundation.org [mailto:iommu-
> bounces at lists.linux-foundation.org] On Behalf Of Robin Murphy
> Sent: 26 January 2016 23:37
> To: will.deacon at arm.com
> Cc: iommu at lists.linux-foundation.org; linux-arm-kernel at lists.infradead.org;
> tchalamarla at caviumnetworks.com
> Subject: [PATCH 1/4] iommu/arm-smmu: Treat all device transactions as
> unprivileged
> 
> The IOMMU API has no concept of privilege so assumes all devices and
> mappings are equal, and indeed most non-CPU master devices on an AMBA
> interconnect make little use of the attribute bits on the bus thus by default
> perform unprivileged data accesses.
> 
> Some devices, however, believe themselves more equal than others, such as
> programmable DMA controllers whose 'master' thread issues bus transactions
> marked as privileged instruction fetches, while the data accesses of its channel
> threads (under the control of Linux, at least) are marked as unprivileged. This
> poses a problem for implementing the DMA API on an IOMMU conforming to
> ARM VMSAv8, under which a page that is unprivileged-writeable is also implicitly
> privileged-execute-never.
> Given that, there is no one set of attributes with which iommu_map() can
> implement, say, dma_alloc_coherent() that will allow every possible type of
> access without something running into unexecepted permission faults.
> 
> Fortunately the SMMU architecture provides a means to mitigate such issues by
> overriding the incoming attributes of a transaction; make use of that to strip the
> privileged/unprivileged status off incoming transactions, leaving just the
> instruction/data dichotomy which the IOMMU API does at least understand;
> Four states good, two states better.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> 59ee4b8..1f9093d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -167,6 +167,9 @@
>  #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>  #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
> 
> +#define S2CR_PRIVCFG_SHIFT		24
> +#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
> +
>  /* Context bank attribute registers */
>  #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
>  #define CBAR_VMID_SHIFT			0
> @@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct
> arm_smmu_domain *smmu_domain,
>  		u32 idx, s2cr;
> 
>  		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> -		s2cr = S2CR_TYPE_TRANS |
> +		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
>  		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);

Treating all MMU master access as unprivileged sounds more
conservative. Alternate approach would be to treat instruction
fetches as data reads.

Regards,
Anup

Re: [PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Will Deacon]

Hi Robin,

On Tue, Jan 26, 2016 at 06:06:35PM +0000, Robin Murphy wrote:
> Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
> debugging/security feature so that unmatched stream IDs (i.e. devices
> not attached to an IOMMU domain) may be configured to fault.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 1f9093d..d1b7dc1 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -141,6 +141,8 @@
>  #define ID2_PTFS_16K			(1 << 13)
>  #define ID2_PTFS_64K			(1 << 14)
>  
> +#define sGFSR_USF			(1 << 1)
> +
>  /* Global TLB invalidation */
>  #define ARM_SMMU_GR0_TLBIVMID		0x64
>  #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
> @@ -263,6 +265,10 @@ static int force_stage;
>  module_param_named(force_stage, force_stage, int, S_IRUGO);
>  MODULE_PARM_DESC(force_stage,
>  	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
> +static bool disable_bypass;
> +module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
> +MODULE_PARM_DESC(disable_bypass,
> +	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
>  
>  enum arm_smmu_arch_version {
>  	ARM_SMMU_V1 = 1,
> @@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>  	if (!gfsr)
>  		return IRQ_NONE;
>  
> -	dev_err_ratelimited(smmu->dev,
> -		"Unexpected global fault, this could be serious\n");
> +	if (gfsr & sGFSR_USF)
> +		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
> +				    gfsynr1 & SMR_ID_MASK);
> +	else

I think I'd rather drop this message -- a global error is still indicative
that something has gone horriby wrong, and we already print the gsynr
registers below. Furthermore, if we take multiple faults, it might look
like they're all exclusively due to unmatched streams.

On top of that, I'm not convinced that USF will be set for an SMMU that
doesn't implement stream-matching (because it will match an S2CR of type
FAULT).

Other than that, looks good.

Will

[PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Will Deacon]

Hi Robin,

On Tue, Jan 26, 2016 at 06:06:35PM +0000, Robin Murphy wrote:
> Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
> debugging/security feature so that unmatched stream IDs (i.e. devices
> not attached to an IOMMU domain) may be configured to fault.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 1f9093d..d1b7dc1 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -141,6 +141,8 @@
>  #define ID2_PTFS_16K			(1 << 13)
>  #define ID2_PTFS_64K			(1 << 14)
>  
> +#define sGFSR_USF			(1 << 1)
> +
>  /* Global TLB invalidation */
>  #define ARM_SMMU_GR0_TLBIVMID		0x64
>  #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
> @@ -263,6 +265,10 @@ static int force_stage;
>  module_param_named(force_stage, force_stage, int, S_IRUGO);
>  MODULE_PARM_DESC(force_stage,
>  	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
> +static bool disable_bypass;
> +module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
> +MODULE_PARM_DESC(disable_bypass,
> +	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
>  
>  enum arm_smmu_arch_version {
>  	ARM_SMMU_V1 = 1,
> @@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>  	if (!gfsr)
>  		return IRQ_NONE;
>  
> -	dev_err_ratelimited(smmu->dev,
> -		"Unexpected global fault, this could be serious\n");
> +	if (gfsr & sGFSR_USF)
> +		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
> +				    gfsynr1 & SMR_ID_MASK);
> +	else

I think I'd rather drop this message -- a global error is still indicative
that something has gone horriby wrong, and we already print the gsynr
registers below. Furthermore, if we take multiple faults, it might look
like they're all exclusively due to unmatched streams.

On top of that, I'm not convinced that USF will be set for an SMMU that
doesn't implement stream-matching (because it will match an S2CR of type
FAULT).

Other than that, looks good.

Will

Re: [PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:34PM +0000, Robin Murphy wrote:
> The IOMMU API has no concept of privilege so assumes all devices and
> mappings are equal, and indeed most non-CPU master devices on an AMBA
> interconnect make little use of the attribute bits on the bus thus by
> default perform unprivileged data accesses.
> 
> Some devices, however, believe themselves more equal than others, such
> as programmable DMA controllers whose 'master' thread issues bus
> transactions marked as privileged instruction fetches, while the data
> accesses of its channel threads (under the control of Linux, at least)
> are marked as unprivileged. This poses a problem for implementing the
> DMA API on an IOMMU conforming to ARM VMSAv8, under which a page that is
> unprivileged-writeable is also implicitly privileged-execute-never.
> Given that, there is no one set of attributes with which iommu_map() can
> implement, say, dma_alloc_coherent() that will allow every possible type
> of access without something running into unexecepted permission faults.
> 
> Fortunately the SMMU architecture provides a means to mitigate such
> issues by overriding the incoming attributes of a transaction; make use
> of that to strip the privileged/unprivileged status off incoming
> transactions, leaving just the instruction/data dichotomy which the
> IOMMU API does at least understand; Four states good, two states better.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..1f9093d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -167,6 +167,9 @@
>  #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>  #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
>  
> +#define S2CR_PRIVCFG_SHIFT		24
> +#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
> +
>  /* Context bank attribute registers */
>  #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
>  #define CBAR_VMID_SHIFT			0
> @@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  		u32 idx, s2cr;
>  
>  		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> -		s2cr = S2CR_TYPE_TRANS |
> +		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
>  		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
>  		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));

Hmm, do we also need to worry about the bypass case? I guess not for the
moment, but I anticipate horrible things.

Will

[PATCH 1/4] iommu/arm-smmu: Treat all device transactions as unprivileged

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:34PM +0000, Robin Murphy wrote:
> The IOMMU API has no concept of privilege so assumes all devices and
> mappings are equal, and indeed most non-CPU master devices on an AMBA
> interconnect make little use of the attribute bits on the bus thus by
> default perform unprivileged data accesses.
> 
> Some devices, however, believe themselves more equal than others, such
> as programmable DMA controllers whose 'master' thread issues bus
> transactions marked as privileged instruction fetches, while the data
> accesses of its channel threads (under the control of Linux, at least)
> are marked as unprivileged. This poses a problem for implementing the
> DMA API on an IOMMU conforming to ARM VMSAv8, under which a page that is
> unprivileged-writeable is also implicitly privileged-execute-never.
> Given that, there is no one set of attributes with which iommu_map() can
> implement, say, dma_alloc_coherent() that will allow every possible type
> of access without something running into unexecepted permission faults.
> 
> Fortunately the SMMU architecture provides a means to mitigate such
> issues by overriding the incoming attributes of a transaction; make use
> of that to strip the privileged/unprivileged status off incoming
> transactions, leaving just the instruction/data dichotomy which the
> IOMMU API does at least understand; Four states good, two states better.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 59ee4b8..1f9093d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -167,6 +167,9 @@
>  #define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
>  #define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
>  
> +#define S2CR_PRIVCFG_SHIFT		24
> +#define S2CR_PRIVCFG_UNPRIV		(2 << S2CR_PRIVCFG_SHIFT)
> +
>  /* Context bank attribute registers */
>  #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
>  #define CBAR_VMID_SHIFT			0
> @@ -1083,7 +1086,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  		u32 idx, s2cr;
>  
>  		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
> -		s2cr = S2CR_TYPE_TRANS |
> +		s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
>  		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
>  		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));

Hmm, do we also need to worry about the bypass case? I guess not for the
moment, but I anticipate horrible things.

Will

Re: [PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:37PM +0000, Robin Murphy wrote:
> TLB synchronisation is a mighty big hammmer to bring down on the
> transaction stream, typically stalling all in-flight transactions until
> the sync completes. Since in most cases (except at stage 2 on SMMUv1)
> a per-context sync operation is available, prefer that over the global
> operation when performing TLB maintenance for a single domain, to avoid
> unecessarily disrupting ongoing traffic in other contexts.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 18e0e10..bf1895c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -219,6 +219,8 @@
>  #define ARM_SMMU_CB_S1_TLBIVAL		0x620
>  #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
>  #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
> +#define ARM_SMMU_CB_TLBSYNC		0x7f0
> +#define ARM_SMMU_CB_TLBSTATUS		0x7f4
>  #define ARM_SMMU_CB_ATS1PR		0x800
>  #define ARM_SMMU_CB_ATSR		0x8f0
>  
> @@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>  }
>  
>  /* Wait for any pending TLB invalidations to complete */
> -static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
>  {
>  	int count = 0;
> -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	void __iomem *base, __iomem *status;
>  
> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
> -	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
> -	       & sTLBGSTATUS_GSACTIVE) {
> +	if (cbndx < 0) {
> +		base = ARM_SMMU_GR0(smmu);
> +		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
> +		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
> +	} else {
> +		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> +		status = base + ARM_SMMU_CB_TLBSTATUS;
> +		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
> +	}
> +
> +	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
>  		cpu_relax();
>  		if (++count == TLB_LOOP_TIMEOUT) {
>  			dev_err_ratelimited(smmu->dev,
> @@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>  static void arm_smmu_tlb_sync(void *cookie)
>  {
>  	struct arm_smmu_domain *smmu_domain = cookie;
> -	__arm_smmu_tlb_sync(smmu_domain->smmu);
> +	int cbndx = smmu_domain->cfg.cbndx;
> +
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
> +	    smmu_domain->smmu->version < ARM_SMMU_V2)
> +		cbndx = -1;

I think it would be cleaner just to override the sync function pointer
when we initialise a stage-2 page table for an SMMUv1 implementation.

Any reason not to go that way?

Will

[PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:37PM +0000, Robin Murphy wrote:
> TLB synchronisation is a mighty big hammmer to bring down on the
> transaction stream, typically stalling all in-flight transactions until
> the sync completes. Since in most cases (except at stage 2 on SMMUv1)
> a per-context sync operation is available, prefer that over the global
> operation when performing TLB maintenance for a single domain, to avoid
> unecessarily disrupting ongoing traffic in other contexts.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 18e0e10..bf1895c 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -219,6 +219,8 @@
>  #define ARM_SMMU_CB_S1_TLBIVAL		0x620
>  #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
>  #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
> +#define ARM_SMMU_CB_TLBSYNC		0x7f0
> +#define ARM_SMMU_CB_TLBSTATUS		0x7f4
>  #define ARM_SMMU_CB_ATS1PR		0x800
>  #define ARM_SMMU_CB_ATSR		0x8f0
>  
> @@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>  }
>  
>  /* Wait for any pending TLB invalidations to complete */
> -static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
>  {
>  	int count = 0;
> -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
> +	void __iomem *base, __iomem *status;
>  
> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
> -	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
> -	       & sTLBGSTATUS_GSACTIVE) {
> +	if (cbndx < 0) {
> +		base = ARM_SMMU_GR0(smmu);
> +		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
> +		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
> +	} else {
> +		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
> +		status = base + ARM_SMMU_CB_TLBSTATUS;
> +		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
> +	}
> +
> +	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
>  		cpu_relax();
>  		if (++count == TLB_LOOP_TIMEOUT) {
>  			dev_err_ratelimited(smmu->dev,
> @@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>  static void arm_smmu_tlb_sync(void *cookie)
>  {
>  	struct arm_smmu_domain *smmu_domain = cookie;
> -	__arm_smmu_tlb_sync(smmu_domain->smmu);
> +	int cbndx = smmu_domain->cfg.cbndx;
> +
> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
> +	    smmu_domain->smmu->version < ARM_SMMU_V2)
> +		cbndx = -1;

I think it would be cleaner just to override the sync function pointer
when we initialise a stage-2 page table for an SMMUv1 implementation.

Any reason not to go that way?

Will

Re: [PATCH 0/4] Miscellaneous ARM SMMU patches

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:33PM +0000, Robin Murphy wrote:
> Here's my current "miscellaneous SMMU enhancements" branch for your
> consideration. Patch #1 solves a subtle corner case that cropped up
> while exercising stage 1 context formats on the DMA330-MMU500 model;
> #3 will be wanted fairly soon for DMA ops integration so may as well
> get some exposure now; #2 and #4 are more nice-to-haves.

Cheers, Robin. I've queued #1 and #3 for 4.6. If you address the comments
on the other two, I can queue those as well.

Will

[PATCH 0/4] Miscellaneous ARM SMMU patches

[Will Deacon]

On Tue, Jan 26, 2016 at 06:06:33PM +0000, Robin Murphy wrote:
> Here's my current "miscellaneous SMMU enhancements" branch for your
> consideration. Patch #1 solves a subtle corner case that cropped up
> while exercising stage 1 context formats on the DMA330-MMU500 model;
> #3 will be wanted fairly soon for DMA ops integration so may as well
> get some exposure now; #2 and #4 are more nice-to-haves.

Cheers, Robin. I've queued #1 and #3 for 4.6. If you address the comments
on the other two, I can queue those as well.

Will

Re: [PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Robin Murphy]

On 09/02/16 14:15, Will Deacon wrote:
> On Tue, Jan 26, 2016 at 06:06:37PM +0000, Robin Murphy wrote:
>> TLB synchronisation is a mighty big hammmer to bring down on the
>> transaction stream, typically stalling all in-flight transactions until
>> the sync completes. Since in most cases (except at stage 2 on SMMUv1)
>> a per-context sync operation is available, prefer that over the global
>> operation when performing TLB maintenance for a single domain, to avoid
>> unecessarily disrupting ongoing traffic in other contexts.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 18e0e10..bf1895c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -219,6 +219,8 @@
>>   #define ARM_SMMU_CB_S1_TLBIVAL		0x620
>>   #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
>>   #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
>> +#define ARM_SMMU_CB_TLBSYNC		0x7f0
>> +#define ARM_SMMU_CB_TLBSTATUS		0x7f4
>>   #define ARM_SMMU_CB_ATS1PR		0x800
>>   #define ARM_SMMU_CB_ATSR		0x8f0
>>
>> @@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>>   }
>>
>>   /* Wait for any pending TLB invalidations to complete */
>> -static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
>>   {
>>   	int count = 0;
>> -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>> +	void __iomem *base, __iomem *status;
>>
>> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
>> -	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
>> -	       & sTLBGSTATUS_GSACTIVE) {
>> +	if (cbndx < 0) {
>> +		base = ARM_SMMU_GR0(smmu);
>> +		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
>> +		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
>> +	} else {
>> +		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
>> +		status = base + ARM_SMMU_CB_TLBSTATUS;
>> +		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
>> +	}
>> +
>> +	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
>>   		cpu_relax();
>>   		if (++count == TLB_LOOP_TIMEOUT) {
>>   			dev_err_ratelimited(smmu->dev,
>> @@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>>   static void arm_smmu_tlb_sync(void *cookie)
>>   {
>>   	struct arm_smmu_domain *smmu_domain = cookie;
>> -	__arm_smmu_tlb_sync(smmu_domain->smmu);
>> +	int cbndx = smmu_domain->cfg.cbndx;
>> +
>> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
>> +	    smmu_domain->smmu->version < ARM_SMMU_V2)
>> +		cbndx = -1;
>
> I think it would be cleaner just to override the sync function pointer
> when we initialise a stage-2 page table for an SMMUv1 implementation.
>
> Any reason not to go that way?

Frankly, the idea just didn't occur to me. Looking more closely, if we 
were to simply put a set of iommu_gather_ops pointers in each domain 
based on the format, we could then also break up the confusing mess of 
arm_smmu_tlb_inv_range_nosync(), which would be nice.

I'll give that a go on top of the context format series I'm preparing 
(with which it would otherwise conflict horribly) and see how it looks.

Robin.

[PATCH 4/4] iommu/arm-smmu: Use per-context TLB sync as appropriate

[Robin Murphy]

On 09/02/16 14:15, Will Deacon wrote:
> On Tue, Jan 26, 2016 at 06:06:37PM +0000, Robin Murphy wrote:
>> TLB synchronisation is a mighty big hammmer to bring down on the
>> transaction stream, typically stalling all in-flight transactions until
>> the sync completes. Since in most cases (except at stage 2 on SMMUv1)
>> a per-context sync operation is available, prefer that over the global
>> operation when performing TLB maintenance for a single domain, to avoid
>> unecessarily disrupting ongoing traffic in other contexts.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm-smmu.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 18e0e10..bf1895c 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -219,6 +219,8 @@
>>   #define ARM_SMMU_CB_S1_TLBIVAL		0x620
>>   #define ARM_SMMU_CB_S2_TLBIIPAS2	0x630
>>   #define ARM_SMMU_CB_S2_TLBIIPAS2L	0x638
>> +#define ARM_SMMU_CB_TLBSYNC		0x7f0
>> +#define ARM_SMMU_CB_TLBSTATUS		0x7f4
>>   #define ARM_SMMU_CB_ATS1PR		0x800
>>   #define ARM_SMMU_CB_ATSR		0x8f0
>>
>> @@ -546,14 +548,22 @@ static void __arm_smmu_free_bitmap(unsigned long *map, int idx)
>>   }
>>
>>   /* Wait for any pending TLB invalidations to complete */
>> -static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>> +static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu, int cbndx)
>>   {
>>   	int count = 0;
>> -	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>> +	void __iomem *base, __iomem *status;
>>
>> -	writel_relaxed(0, gr0_base + ARM_SMMU_GR0_sTLBGSYNC);
>> -	while (readl_relaxed(gr0_base + ARM_SMMU_GR0_sTLBGSTATUS)
>> -	       & sTLBGSTATUS_GSACTIVE) {
>> +	if (cbndx < 0) {
>> +		base = ARM_SMMU_GR0(smmu);
>> +		status = base + ARM_SMMU_GR0_sTLBGSTATUS;
>> +		writel_relaxed(0, base + ARM_SMMU_GR0_sTLBGSYNC);
>> +	} else {
>> +		base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cbndx);
>> +		status = base + ARM_SMMU_CB_TLBSTATUS;
>> +		writel_relaxed(0, base + ARM_SMMU_CB_TLBSYNC);
>> +	}
>> +
>> +	while (readl_relaxed(status) & sTLBGSTATUS_GSACTIVE) {
>>   		cpu_relax();
>>   		if (++count == TLB_LOOP_TIMEOUT) {
>>   			dev_err_ratelimited(smmu->dev,
>> @@ -567,7 +577,13 @@ static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>>   static void arm_smmu_tlb_sync(void *cookie)
>>   {
>>   	struct arm_smmu_domain *smmu_domain = cookie;
>> -	__arm_smmu_tlb_sync(smmu_domain->smmu);
>> +	int cbndx = smmu_domain->cfg.cbndx;
>> +
>> +	if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 &&
>> +	    smmu_domain->smmu->version < ARM_SMMU_V2)
>> +		cbndx = -1;
>
> I think it would be cleaner just to override the sync function pointer
> when we initialise a stage-2 page table for an SMMUv1 implementation.
>
> Any reason not to go that way?

Frankly, the idea just didn't occur to me. Looking more closely, if we 
were to simply put a set of iommu_gather_ops pointers in each domain 
based on the format, we could then also break up the confusing mess of 
arm_smmu_tlb_inv_range_nosync(), which would be nice.

I'll give that a go on top of the context format series I'm preparing 
(with which it would otherwise conflict horribly) and see how it looks.

Robin.

Re: [PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

On 09/02/16 14:06, Will Deacon wrote:
> Hi Robin,
>
> On Tue, Jan 26, 2016 at 06:06:35PM +0000, Robin Murphy wrote:
>> Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
>> debugging/security feature so that unmatched stream IDs (i.e. devices
>> not attached to an IOMMU domain) may be configured to fault.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
>>   1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 1f9093d..d1b7dc1 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -141,6 +141,8 @@
>>   #define ID2_PTFS_16K			(1 << 13)
>>   #define ID2_PTFS_64K			(1 << 14)
>>
>> +#define sGFSR_USF			(1 << 1)
>> +
>>   /* Global TLB invalidation */
>>   #define ARM_SMMU_GR0_TLBIVMID		0x64
>>   #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
>> @@ -263,6 +265,10 @@ static int force_stage;
>>   module_param_named(force_stage, force_stage, int, S_IRUGO);
>>   MODULE_PARM_DESC(force_stage,
>>   	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
>> +static bool disable_bypass;
>> +module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
>> +MODULE_PARM_DESC(disable_bypass,
>> +	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
>>
>>   enum arm_smmu_arch_version {
>>   	ARM_SMMU_V1 = 1,
>> @@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>>   	if (!gfsr)
>>   		return IRQ_NONE;
>>
>> -	dev_err_ratelimited(smmu->dev,
>> -		"Unexpected global fault, this could be serious\n");
>> +	if (gfsr & sGFSR_USF)
>> +		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
>> +				    gfsynr1 & SMR_ID_MASK);
>> +	else
>
> I think I'd rather drop this message -- a global error is still indicative
> that something has gone horriby wrong, and we already print the gsynr
> registers below. Furthermore, if we take multiple faults, it might look
> like they're all exclusively due to unmatched streams.
>
> On top of that, I'm not convinced that USF will be set for an SMMU that
> doesn't implement stream-matching (because it will match an S2CR of type
> FAULT).

You're quite right, by the look of it the stream indexing case should 
report ICF rather than USF. My rationale for special-casing the message 
was that it's less of an "unexpected" fault when it's something you've 
specifically asked the driver for, but it also seems reasonable that 
someone turning on such an "I know what I'm doing" feature might be able 
to decode GFSR themselves. Considering the machinations of matching 
!MULTI and USF or ICF as appropriate just for the sake of printing a 
slightly reworded message, I'll drop this hunk.

> Other than that, looks good.

Thanks,
Robin.

[PATCH 2/4] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

On 09/02/16 14:06, Will Deacon wrote:
> Hi Robin,
>
> On Tue, Jan 26, 2016 at 06:06:35PM +0000, Robin Murphy wrote:
>> Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
>> debugging/security feature so that unmatched stream IDs (i.e. devices
>> not attached to an IOMMU domain) may be configured to fault.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
>>   1 file changed, 23 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 1f9093d..d1b7dc1 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -141,6 +141,8 @@
>>   #define ID2_PTFS_16K			(1 << 13)
>>   #define ID2_PTFS_64K			(1 << 14)
>>
>> +#define sGFSR_USF			(1 << 1)
>> +
>>   /* Global TLB invalidation */
>>   #define ARM_SMMU_GR0_TLBIVMID		0x64
>>   #define ARM_SMMU_GR0_TLBIALLNSNH	0x68
>> @@ -263,6 +265,10 @@ static int force_stage;
>>   module_param_named(force_stage, force_stage, int, S_IRUGO);
>>   MODULE_PARM_DESC(force_stage,
>>   	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
>> +static bool disable_bypass;
>> +module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
>> +MODULE_PARM_DESC(disable_bypass,
>> +	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
>>
>>   enum arm_smmu_arch_version {
>>   	ARM_SMMU_V1 = 1,
>> @@ -704,8 +710,12 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
>>   	if (!gfsr)
>>   		return IRQ_NONE;
>>
>> -	dev_err_ratelimited(smmu->dev,
>> -		"Unexpected global fault, this could be serious\n");
>> +	if (gfsr & sGFSR_USF)
>> +		dev_err_ratelimited(smmu->dev, "Unmatched stream fault: ID 0x%x\n",
>> +				    gfsynr1 & SMR_ID_MASK);
>> +	else
>
> I think I'd rather drop this message -- a global error is still indicative
> that something has gone horriby wrong, and we already print the gsynr
> registers below. Furthermore, if we take multiple faults, it might look
> like they're all exclusively due to unmatched streams.
>
> On top of that, I'm not convinced that USF will be set for an SMMU that
> doesn't implement stream-matching (because it will match an S2CR of type
> FAULT).

You're quite right, by the look of it the stream indexing case should 
report ICF rather than USF. My rationale for special-casing the message 
was that it's less of an "unexpected" fault when it's something you've 
specifically asked the driver for, but it also seems reasonable that 
someone turning on such an "I know what I'm doing" feature might be able 
to decode GFSR themselves. Considering the machinations of matching 
!MULTI and USF or ICF as appropriate just for the sake of printing a 
slightly reworded message, I'll drop this hunk.

> Other than that, looks good.

Thanks,
Robin.

[PATCH v2] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
debugging/security feature so that unmatched stream IDs (i.e. devices
not attached to an IOMMU domain) may be configured to fault.

Rather than introduce unsightly inconsistency, or repeat the existing
unnecessary use of module_param_named(), fix that as well in passing.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

v2:
- Don't bother trying to report the fault specially.
- Realise that calling module_param_named() with identical variable
  and parameter names is silly.

 drivers/iommu/arm-smmu.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1f9093d..cf1e330 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -260,9 +260,13 @@
 #define FSYNR0_WNR			(1 << 4)
 
 static int force_stage;
-module_param_named(force_stage, force_stage, int, S_IRUGO);
+module_param(force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
+static bool disable_bypass;
+module_param(disable_bypass, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_bypass,
+	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
 
 enum arm_smmu_arch_version {
 	ARM_SMMU_V1 = 1,
@@ -1111,9 +1115,9 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		u32 reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
 	arm_smmu_master_free_smrs(smmu, cfg);
@@ -1476,11 +1480,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
-	/* Mark all SMRn as invalid and all S2CRn as bypass */
+	/* Mark all SMRn as invalid and all S2CRn as bypass unless overridden */
+	reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
 		writel_relaxed(0, gr0_base + ARM_SMMU_GR0_SMR(i));
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			gr0_base + ARM_SMMU_GR0_S2CR(i));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(i));
 	}
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
@@ -1502,8 +1506,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	/* Disable TLB broadcasting. */
 	reg |= (sCR0_VMIDPNE | sCR0_PTM);
 
-	/* Enable client access, but bypass when no mapping is found */
-	reg &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	/* Enable client access, handling unmatched streams as appropriate */
+	reg &= ~sCR0_CLIENTPD;
+	if (disable_bypass)
+		reg |= sCR0_USFCFG;
+	else
+		reg &= ~sCR0_USFCFG;
 
 	/* Disable forced broadcasting */
 	reg &= ~sCR0_FB;
-- 
2.7.0.25.gfc10eb5.dirty

[PATCH v2] iommu/arm-smmu: Allow disabling unmatched stream bypass

[Robin Murphy]

Borrow the disable_bypass parameter from the SMMUv3 driver as a handy
debugging/security feature so that unmatched stream IDs (i.e. devices
not attached to an IOMMU domain) may be configured to fault.

Rather than introduce unsightly inconsistency, or repeat the existing
unnecessary use of module_param_named(), fix that as well in passing.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2:
- Don't bother trying to report the fault specially.
- Realise that calling module_param_named() with identical variable
  and parameter names is silly.

 drivers/iommu/arm-smmu.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 1f9093d..cf1e330 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -260,9 +260,13 @@
 #define FSYNR0_WNR			(1 << 4)
 
 static int force_stage;
-module_param_named(force_stage, force_stage, int, S_IRUGO);
+module_param(force_stage, int, S_IRUGO);
 MODULE_PARM_DESC(force_stage,
 	"Force SMMU mappings to be installed@a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation.");
+static bool disable_bypass;
+module_param(disable_bypass, bool, S_IRUGO);
+MODULE_PARM_DESC(disable_bypass,
+	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
 
 enum arm_smmu_arch_version {
 	ARM_SMMU_V1 = 1,
@@ -1111,9 +1115,9 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		u32 reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
 	arm_smmu_master_free_smrs(smmu, cfg);
@@ -1476,11 +1480,11 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	reg = readl_relaxed(ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 	writel(reg, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sGFSR);
 
-	/* Mark all SMRn as invalid and all S2CRn as bypass */
+	/* Mark all SMRn as invalid and all S2CRn as bypass unless overridden */
+	reg = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS;
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
 		writel_relaxed(0, gr0_base + ARM_SMMU_GR0_SMR(i));
-		writel_relaxed(S2CR_TYPE_BYPASS,
-			gr0_base + ARM_SMMU_GR0_S2CR(i));
+		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_S2CR(i));
 	}
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
@@ -1502,8 +1506,12 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	/* Disable TLB broadcasting. */
 	reg |= (sCR0_VMIDPNE | sCR0_PTM);
 
-	/* Enable client access, but bypass when no mapping is found */
-	reg &= ~(sCR0_CLIENTPD | sCR0_USFCFG);
+	/* Enable client access, handling unmatched streams as appropriate */
+	reg &= ~sCR0_CLIENTPD;
+	if (disable_bypass)
+		reg |= sCR0_USFCFG;
+	else
+		reg &= ~sCR0_USFCFG;
 
 	/* Disable forced broadcasting */
 	reg &= ~sCR0_FB;
-- 
2.7.0.25.gfc10eb5.dirty