Re: [PATCH v5 09/19] iommu/arm-smmu: Consolidate stream map entry state

On Tue, Aug 23, 2016 at 08:05:20PM +0100, Robin Murphy wrote:

In order to consider SMR masking, we really want to be able to validate
ID/mask pairs against existing SMR contents to prevent stream match
conflicts, which at best would cause transactions to fault unexpectedly,
and at worst lead to silent unpredictable behaviour. With our SMMU
instance data holding only an allocator bitmap, and the SMR values
themselves scattered across master configs hanging off devices which we
may have no way of finding, there's essentially no way short of digging
everything back out of the hardware. Similarly, the thought of power
management ops to support suspend/resume faces the exact same problem.

By massaging the software state into a closer shape to the underlying
hardware, everything comes together quite nicely; the allocator and the
high-level view of the data become a single centralised state which we
can easily keep track of, and to which any updates can be validated in
full before being synchronised to the hardware itself.

Signed-off-by: Robin Murphy <redacted>
---
 drivers/iommu/arm-smmu.c | 147 +++++++++++++++++++++++++++--------------------
 1 file changed, 86 insertions(+), 61 deletions(-)

[...]

+static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
+				      struct arm_smmu_master_cfg *cfg)
+{
+	struct arm_smmu_smr *smrs = smmu->smrs;
+	int i, idx;
 
 	/* Allocate the SMRs on the SMMU */
 	for (i = 0; i < cfg->num_streamids; ++i) {
-		int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
-						  smmu->num_mapping_groups);
+		if (cfg->smendx[i] >= 0)
+			return -EEXIST;
+
+		/* ...except on stream indexing hardware, of course */
+		if (!smrs) {
+			cfg->smendx[i] = cfg->streamids[i];
+			continue;
+		}
+
+		idx = arm_smmu_alloc_smr(smmu);
 		if (idx < 0) {
 			dev_err(smmu->dev, "failed to allocate free SMR\n");
 			goto err_free_smrs;
 		}
+		cfg->smendx[i] = idx;
 
-		smrs[i] = (struct arm_smmu_smr) {
-			.idx	= idx,
-			.mask	= 0, /* We don't currently share SMRs */
-			.id	= cfg->streamids[i],
-		};
+		smrs[idx].id = cfg->streamids[i];
+		smrs[idx].mask = 0; /* We don't currently share SMRs */
 	}
 
+	if (!smrs)
+		return 0;
+
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
-			  smrs[i].mask << SMR_MASK_SHIFT;
-		writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
-	}
+	for (i = 0; i < cfg->num_streamids; ++i)
+		arm_smmu_write_smr(smmu, cfg->smendx[i]);
 
-	cfg->smrs = smrs;
 	return 0;
 
 err_free_smrs:
-	while (--i >= 0)
-		__arm_smmu_free_bitmap(smmu->smr_map, smrs[i].idx);
-	kfree(smrs);
+	while (i--) {
+		arm_smmu_free_smr(smmu, cfg->smendx[i]);
+		cfg->smendx[i] = INVALID_SMENDX;
+	}

Hmm, don't you have an off-by-one here? At least, looking at the final
code, we branch to out_err from within the for_each_cfg_sme loop, but
before we've incremented smmu->s2crs[idx].count, so the arm_smmu_free_smr
will erroneously decrement that.

Will