Miscellaneous ARM SMMU patches

-----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 <[email protected]>

---
 drivers/iommu/arm-smmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/iommu/arm-smmu.c index renamed from drivers/iommu/arm-smmu.c +0 -0

59ee4b8..1f9093d 100644

drivers/iommu/arm-smmu.c +3 -0

@@ -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

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 <[email protected]>

---
 drivers/iommu/arm-smmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/iommu/arm-smmu.c +3 -0

index 59ee4b8..1f9093d 100644
@@ -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

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 <[email protected]>

---
 drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

drivers/iommu/arm-smmu.c +6 -0

index 1f9093d..d1b7dc1 100644
@@ -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,

Hi Robin,

Quoted 34 lines · 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 <[email protected]>

---
  drivers/iommu/arm-smmu.c | 32 +++++++++++++++++++++++---------
  1 file changed, 23 insertions(+), 9 deletions(-)

drivers/iommu/arm-smmu.c +6 -0

index 1f9093d..d1b7dc1 100644
@@ -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,

drivers/iommu/arm-smmu.c static irqreturn_t arm_smmu_global_fault(int irq, void *dev) · line 716

Quoted 9 lines

drivers/iommu/arm-smmu.c +4 -2

@@ -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.

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 <[email protected]>

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

drivers/iommu/arm-smmu.c +15 -5

index 18e0e10..bf1895c 100644
@@ -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,

Quoted 54 lines · 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 <[email protected]>

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

drivers/iommu/arm-smmu.c +15 -5

index 18e0e10..bf1895c 100644
@@ -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,

drivers/iommu/arm-smmu.c static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu) · line 584

Quoted 9 lines

drivers/iommu/arm-smmu.c +5 -1

@@ -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?

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.