[PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[Rahul Singh]

This patch is the work to fix the stream match conflict issue when two devices
have the same stream-id.

Approach taken is to merge the below commit from Linux driver to fix the
issue.

1. "iommu/arm-smmu: Handle stream IDs more dynamically"
    commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
2. "iommu/arm-smmu: Consolidate stream map entry state"
    commit 1f3d5ca43019bff1105838712d55be087d93c0da
3. "iommu/arm-smmu: Keep track of S2CR state"
    commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
4. "iommu/arm-smmu: Add a stream map entry iterator"
    commit d3097e39302083d58922a3d1032d7d59a63d263d
5. "iommu/arm-smmu: Intelligent SMR allocation"
    commit 588888a7399db352d2b1a41c9d5b3bf0fd482390

Rahul Singh (5):
  xen/arm: smmuv1: Handle stream IDs more dynamically
  xen/arm: smmuv1: Consolidate stream map entry state
  xen/arm: smmuv1: Keep track of S2CR state
  xen/arm: smmuv1: Add a stream map entry iterator
  xen/arm: smmuv1: Intelligent SMR allocation

 xen/drivers/passthrough/arm/smmu.c | 423 ++++++++++++++++++-----------
 1 file changed, 262 insertions(+), 161 deletions(-)

-- 
2.17.1

[PATCH 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically

[Rahul Singh]

Backport commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
"iommu/arm-smmu: Handle stream IDs more dynamically" from the Linux
ernel.

This patch is the preparatory work to fix the stream match conflict
when two devices have the same stream-id.

Original commit message:

Rather than assuming fixed worst-case values for stream IDs and SMR
masks, keep track of whatever implemented bits the hardware actually
reports. This also obviates the slightly questionable validation of SMR
fields in isolation - rather than aborting the whole SMMU probe for a
hardware configuration which is still architecturally valid, we can
simply refuse masters later if they try to claim an unrepresentable ID
or mask (which almost certainly implies a DT error anyway).

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 43 +++++++++++++++---------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3e8aa37866..adfab8ee84 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -440,9 +440,7 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 #define ARM_SMMU_GR0_SMR(n)		(0x800 + ((n) << 2))
 #define SMR_VALID			(1U << 31)
 #define SMR_MASK_SHIFT			16
-#define SMR_MASK_MASK			0x7fff
 #define SMR_ID_SHIFT			0
-#define SMR_ID_MASK			0x7fff
 
 #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
 #define S2CR_CBNDX_SHIFT		0
@@ -632,6 +630,8 @@ struct arm_smmu_device {
 	atomic_t			irptndx;
 
 	u32				num_mapping_groups;
+	u16				streamid_mask;
+	u16				smr_mask_mask;
 	DECLARE_BITMAP(smr_map, ARM_SMMU_MAX_SMRS);
 
 	unsigned long			s1_input_size;
@@ -2140,39 +2140,40 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		dev_notice(smmu->dev, "\tcoherent table walk\n");
 	}
 
+	/* Max. number of entries we have for stream matching/indexing */
+	size = 1 << ((id >> ID0_NUMSIDB_SHIFT) & ID0_NUMSIDB_MASK);
+	smmu->streamid_mask = size - 1;
 	if (id & ID0_SMS) {
-		u32 smr, sid, mask;
+		u32 smr;
 
 		smmu->features |= ARM_SMMU_FEAT_STREAM_MATCH;
-		smmu->num_mapping_groups = (id >> ID0_NUMSMRG_SHIFT) &
-					   ID0_NUMSMRG_MASK;
-		if (smmu->num_mapping_groups == 0) {
+		size = (id >> ID0_NUMSMRG_SHIFT) & ID0_NUMSMRG_MASK;
+		if (size == 0) {
 			dev_err(smmu->dev,
 				"stream-matching supported, but no SMRs present!\n");
 			return -ENODEV;
 		}
 
-		smr = SMR_MASK_MASK << SMR_MASK_SHIFT;
-		smr |= (SMR_ID_MASK << SMR_ID_SHIFT);
+		/*
+		 * SMR.ID bits may not be preserved if the corresponding MASK
+		 * bits are set, so check each one separately. We can reject
+		 * masters later if they try to claim IDs outside these masks.
+		 */
+		smr = smmu->streamid_mask << SMR_ID_SHIFT;
 		writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
 		smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
+		smmu->streamid_mask = smr >> SMR_ID_SHIFT;
 
-		mask = (smr >> SMR_MASK_SHIFT) & SMR_MASK_MASK;
-		sid = (smr >> SMR_ID_SHIFT) & SMR_ID_MASK;
-		if ((mask & sid) != sid) {
-			dev_err(smmu->dev,
-				"SMR mask bits (0x%x) insufficient for ID field (0x%x)\n",
-				mask, sid);
-			return -ENODEV;
-		}
+		smr = smmu->streamid_mask << SMR_MASK_SHIFT;
+		writel_relaxed(smr, gr0_base + ARM_SMMU_GR0_SMR(0));
+		smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
+		smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
 
 		dev_notice(smmu->dev,
-			   "\tstream matching with %u register groups, mask 0x%x\n",
-			   smmu->num_mapping_groups, mask);
-	} else {
-		smmu->num_mapping_groups = (id >> ID0_NUMSIDB_SHIFT) &
-					   ID0_NUMSIDB_MASK;
+			   "\tstream matching with %lu register groups, mask 0x%x",
+			   size, smmu->smr_mask_mask);
 	}
+	smmu->num_mapping_groups = size;
 
 	/* ID1 */
 	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID1);
-- 
2.17.1

Re: [PATCH 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically

[Julien Grall]

Hi Rahul,

On 09/03/2021 18:19, Rahul Singh wrote:
> Backport commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
> "iommu/arm-smmu: Handle stream IDs more dynamically" from the Linux
> ernel.
> 
> This patch is the preparatory work to fix the stream match conflict
> when two devices have the same stream-id.
> 
> Original commit message:
> 
> Rather than assuming fixed worst-case values for stream IDs and SMR
> masks, keep track of whatever implemented bits the hardware actually
> reports. This also obviates the slightly questionable validation of SMR
> fields in isolation - rather than aborting the whole SMMU probe for a
> hardware configuration which is still architecturally valid, we can
> simply refuse masters later if they try to claim an unrepresentable ID
> or mask (which almost certainly implies a DT error anyway).

For single backported and verbatim commit, it is common to keep the 
origin tags (I usually indent them) to show who is the original author 
of the patch.

Since 7936671da9fbf645d6bb207608f7b81c27f992de from Wei Chen as an example.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically

[Rahul Singh]

Hello Julien

> On 16 Mar 2021, at 3:16 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 09/03/2021 18:19, Rahul Singh wrote:
>> Backport commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
>> "iommu/arm-smmu: Handle stream IDs more dynamically" from the Linux
>> ernel.
>> This patch is the preparatory work to fix the stream match conflict
>> when two devices have the same stream-id.
>> Original commit message:
>> Rather than assuming fixed worst-case values for stream IDs and SMR
>> masks, keep track of whatever implemented bits the hardware actually
>> reports. This also obviates the slightly questionable validation of SMR
>> fields in isolation - rather than aborting the whole SMMU probe for a
>> hardware configuration which is still architecturally valid, we can
>> simply refuse masters later if they try to claim an unrepresentable ID
>> or mask (which almost certainly implies a DT error anyway).
> 
> For single backported and verbatim commit, it is common to keep the origin tags (I usually indent them) to show who is the original author of the patch.
> 
> Since 7936671da9fbf645d6bb207608f7b81c27f992de from Wei Chen as an example.

Thanks for reviewing the series. I will add the origin tags in the next version.

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall

[PATCH 2/5] xen/arm: smmuv1: Consolidate stream map entry state

[Rahul Singh]

Backport commit 1f3d5ca43019bff1105838712d55be087d93c0da
"iommu/arm-smmu: Consolidate stream map entry state" from the Linux
kernel.

This patch is the preparatory work to fix the stream match conflict
when two devices have the same stream-id.

Original commit message:

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: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 138 +++++++++++++++++------------
 1 file changed, 79 insertions(+), 59 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index adfab8ee84..c41e94f836 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -308,9 +308,6 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
 
-/* Maximum number of mapping groups per SMMU */
-#define ARM_SMMU_MAX_SMRS		128
-
 /* SMMU global address space */
 #define ARM_SMMU_GR0(smmu)		((smmu)->base)
 #define ARM_SMMU_GR1(smmu)		((smmu)->base + (1 << (smmu)->pgshift))
@@ -589,16 +586,17 @@ enum arm_smmu_arch_version {
 };
 
 struct arm_smmu_smr {
-	u8				idx;
 	u16				mask;
 	u16				id;
+	bool				valid;
 };
 
 struct arm_smmu_master_cfg {
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
-	struct arm_smmu_smr		*smrs;
+	s16				smendx[MAX_MASTER_STREAMIDS];
 };
+#define INVALID_SMENDX			-1
 
 struct arm_smmu_master {
 	struct device_node		*of_node;
@@ -632,7 +630,7 @@ struct arm_smmu_device {
 	u32				num_mapping_groups;
 	u16				streamid_mask;
 	u16				smr_mask_mask;
-	DECLARE_BITMAP(smr_map, ARM_SMMU_MAX_SMRS);
+	struct arm_smmu_smr		*smrs;
 
 	unsigned long			s1_input_size;
 	unsigned long			s1_output_size;
@@ -818,6 +816,7 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 			return -ERANGE;
 		}
 		master->cfg.streamids[i] = streamid;
+		master->cfg.smendx[i] = INVALID_SMENDX;
 	}
 	return insert_smmu_master(smmu, master);
 }
@@ -1384,79 +1383,91 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
-static int arm_smmu_master_configure_smrs(struct arm_smmu_device *smmu,
-					  struct arm_smmu_master_cfg *cfg)
+static int arm_smmu_alloc_smr(struct arm_smmu_device *smmu)
 {
 	int i;
-	struct arm_smmu_smr *smrs;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
-	if (!(smmu->features & ARM_SMMU_FEAT_STREAM_MATCH))
-		return 0;
+	for (i = 0; i < smmu->num_mapping_groups; i++)
+		if (!cmpxchg(&smmu->smrs[i].valid, false, true))
+			return i;
 
-	if (cfg->smrs)
-		return -EEXIST;
+	return INVALID_SMENDX;
+}
 
-	smrs = kmalloc_array(cfg->num_streamids, sizeof(*smrs), GFP_KERNEL);
-	if (!smrs) {
-		dev_err(smmu->dev, "failed to allocate %d SMRs\n",
-			cfg->num_streamids);
-		return -ENOMEM;
-	}
+static void arm_smmu_free_smr(struct arm_smmu_device *smmu, int idx)
+{
+	writel_relaxed(~SMR_VALID, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(idx));
+	write_atomic(&smmu->smrs[idx].valid, false);
+}
+
+static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
+{
+	struct arm_smmu_smr *smr = smmu->smrs + idx;
+	u32 reg = (smr->id & smmu->streamid_mask) << SMR_ID_SHIFT |
+		  (smr->mask & smmu->smr_mask_mask) << SMR_MASK_SHIFT;
+
+	if (smr->valid)
+		reg |= SMR_VALID;
+	writel_relaxed(reg, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(idx));
+}
+
+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] != INVALID_SMENDX)
+			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 (IS_ERR_VALUE(idx)) {
 			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;
+	}
 	return -ENOSPC;
 }
 
-static void arm_smmu_master_free_smrs(struct arm_smmu_device *smmu,
+static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 				      struct arm_smmu_master_cfg *cfg)
 {
 	int i;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
-	struct arm_smmu_smr *smrs = cfg->smrs;
-
-	if (!smrs)
-		return;
 
 	/* Invalidate the SMRs before freeing back to the allocator */
 	for (i = 0; i < cfg->num_streamids; ++i) {
-		u8 idx = smrs[i].idx;
+		if (smmu->smrs)
+			arm_smmu_free_smr(smmu, cfg->smendx[i]);
 
-		writel_relaxed(~SMR_VALID, gr0_base + ARM_SMMU_GR0_SMR(idx));
-		__arm_smmu_free_bitmap(smmu->smr_map, idx);
+		cfg->smendx[i] = INVALID_SMENDX;
 	}
-
-	cfg->smrs = NULL;
-	kfree(smrs);
 }
 
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
@@ -1467,14 +1478,14 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
 	/* Devices in an IOMMU group may already be configured */
-	ret = arm_smmu_master_configure_smrs(smmu, cfg);
+	ret = arm_smmu_master_alloc_smes(smmu, cfg);
 	if (ret)
 		return ret == -EEXIST ? 0 : ret;
 
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		u32 idx, s2cr;
 
-		idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		idx = cfg->smendx[i];
 		s2cr = S2CR_TYPE_TRANS |
 		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
 		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
@@ -1490,23 +1501,23 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 
-	/* An IOMMU group is torn down by the first device to be removed */
-	if ((smmu->features & ARM_SMMU_FEAT_STREAM_MATCH) && !cfg->smrs)
-		return;
-
 	/*
 	 * We *must* clear the S2CR first, because freeing the SMR means
 	 * that it can be re-allocated immediately.
 	 * Xen: Unlike Linux, any access to non-configured stream will fault.
 	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
-		u32 idx = cfg->smrs ? cfg->smrs[i].idx : cfg->streamids[i];
+		int idx = cfg->smendx[i];
+
+		/* An IOMMU group is torn down by the first device to be removed */
+		if (idx == INVALID_SMENDX)
+			return;
 
 		writel_relaxed(S2CR_TYPE_FAULT,
 			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
 	}
 
-	arm_smmu_master_free_smrs(smmu, cfg);
+	arm_smmu_master_free_smes(smmu, cfg);
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -2017,16 +2028,20 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 {
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	void __iomem *cb_base;
-	int i = 0;
+	int i;
 	u32 reg;
 
 	/* clear global FSR */
 	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 */
+	/*
+	 * Reset stream mapping groups: Initial values mark all SMRn as
+	 * invalid and all S2CRn as bypass unless overridden.
+	 */
 	for (i = 0; i < smmu->num_mapping_groups; ++i) {
-		writel_relaxed(0, gr0_base + ARM_SMMU_GR0_SMR(i));
+		if (smmu->smrs)
+			arm_smmu_write_smr(smmu, i);
 		/*
 		 * Xen: Unlike Linux, any access to a non-configure stream
 		 * will fault by default.
@@ -2169,6 +2184,11 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		smr = readl_relaxed(gr0_base + ARM_SMMU_GR0_SMR(0));
 		smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
 
+		/* Zero-initialised to mark as invalid */
+		smmu->smrs = devm_kzalloc(smmu->dev, sizeof(*smmu->smrs), GFP_KERNEL);
+		if (!smmu->smrs)
+			return -ENOMEM;
+
 		dev_notice(smmu->dev,
 			   "\tstream matching with %lu register groups, mask 0x%x",
 			   size, smmu->smr_mask_mask);
-- 
2.17.1

[PATCH 3/5] xen/arm: smmuv1: Keep track of S2CR state

[Rahul Singh]

Backport commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
"iommu/arm-smmu: Keep track of S2CR state" from the Linux kernel.

This patch is the preparatory work to fix the stream match conflict
when two devices have the same stream-id.

Original commit message:

Making S2CRs first-class citizens within the driver with a high-level
representation of their state offers a neat solution to a few problems:

Firstly, the information about which context a device's stream IDs are
associated with is already present by necessity in the S2CR. With that
state easily accessible we can refer directly to it and obviate the need
to track an IOMMU domain in each device's archdata (its earlier purpose
of enforcing correct attachment of multi-device groups now being handled
by the IOMMU core itself).

Secondly, the core API now deprecates explicit domain detach and expects
domain attach to move devices smoothly from one domain to another; for
SMMUv2, this notion maps directly to simply rewriting the S2CRs assigned
to the device. By giving the driver a suitable abstraction of those
S2CRs to work with, we can massively reduce the overhead of the current
heavy-handed "detach, free resources, reallocate resources, attach"
approach.

Thirdly, making the software state hardware-shaped and attached to the
SMMU instance once again makes suspend/resume of this register group
that much simpler to implement in future.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 150 +++++++++++++++++------------
 1 file changed, 89 insertions(+), 61 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c41e94f836..e1b937bd4b 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -444,9 +444,20 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 #define S2CR_CBNDX_MASK			0xff
 #define S2CR_TYPE_SHIFT			16
 #define S2CR_TYPE_MASK			0x3
-#define S2CR_TYPE_TRANS			(0 << S2CR_TYPE_SHIFT)
-#define S2CR_TYPE_BYPASS		(1 << S2CR_TYPE_SHIFT)
-#define S2CR_TYPE_FAULT			(2 << S2CR_TYPE_SHIFT)
+enum arm_smmu_s2cr_type {
+	S2CR_TYPE_TRANS,
+	S2CR_TYPE_BYPASS,
+	S2CR_TYPE_FAULT,
+};
+
+#define S2CR_PRIVCFG_SHIFT		24
+#define S2CR_PRIVCFG_MASK		0x3
+enum arm_smmu_s2cr_privcfg {
+	S2CR_PRIVCFG_DEFAULT,
+	S2CR_PRIVCFG_DIPAN,
+	S2CR_PRIVCFG_UNPRIV,
+	S2CR_PRIVCFG_PRIV,
+};
 
 /* Context bank attribute registers */
 #define ARM_SMMU_GR1_CBAR(n)		(0x0 + ((n) << 2))
@@ -585,6 +596,16 @@ enum arm_smmu_arch_version {
 	ARM_SMMU_V2,
 };
 
+struct arm_smmu_s2cr {
+	enum arm_smmu_s2cr_type		type;
+	enum arm_smmu_s2cr_privcfg	privcfg;
+	u8				cbndx;
+};
+
+#define s2cr_init_val (struct arm_smmu_s2cr){				\
+	.type = S2CR_TYPE_FAULT                                 \
+}
+
 struct arm_smmu_smr {
 	u16				mask;
 	u16				id;
@@ -631,6 +652,7 @@ struct arm_smmu_device {
 	u16				streamid_mask;
 	u16				smr_mask_mask;
 	struct arm_smmu_smr		*smrs;
+	struct arm_smmu_s2cr		*s2crs;
 
 	unsigned long			s1_input_size;
 	unsigned long			s1_output_size;
@@ -1411,6 +1433,23 @@ static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
 	writel_relaxed(reg, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(idx));
 }
 
+static void arm_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
+{
+	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
+	u32 reg = (s2cr->type & S2CR_TYPE_MASK) << S2CR_TYPE_SHIFT |
+		  (s2cr->cbndx & S2CR_CBNDX_MASK) << S2CR_CBNDX_SHIFT |
+		  (s2cr->privcfg & S2CR_PRIVCFG_MASK) << S2CR_PRIVCFG_SHIFT;
+
+	writel_relaxed(reg, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_S2CR(idx));
+}
+
+static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int idx)
+{
+	arm_smmu_write_s2cr(smmu, idx);
+	if (smmu->smrs)
+		arm_smmu_write_smr(smmu, idx);
+}
+
 static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
 				      struct arm_smmu_master_cfg *cfg)
 {
@@ -1461,6 +1500,23 @@ static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 {
 	int i;
 
+	/*
+	 * We *must* clear the S2CR first, because freeing the SMR means
+	 * that it can be re-allocated immediately.
+	 */
+	for (i = 0; i < cfg->num_streamids; ++i) {
+		int idx = cfg->smendx[i];
+
+		/* An IOMMU group is torn down by the first device to be removed */
+		if (idx == INVALID_SMENDX)
+			return;
+
+		smmu->s2crs[idx] = s2cr_init_val;
+		arm_smmu_write_s2cr(smmu, idx);
+	}
+	/* Sync S2CR updates before touching anything else */
+	__iowmb();
+
 	/* Invalidate the SMRs before freeing back to the allocator */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		if (smmu->smrs)
@@ -1473,51 +1529,30 @@ static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 				      struct arm_smmu_master_cfg *cfg)
 {
-	int i, ret;
+	int i, ret = 0;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
+	enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
+	u8 cbndx = smmu_domain->cfg.cbndx;
 
-	/* Devices in an IOMMU group may already be configured */
-	ret = arm_smmu_master_alloc_smes(smmu, cfg);
+	if (cfg->smendx[0] == INVALID_SMENDX)
+		ret = arm_smmu_master_alloc_smes(smmu, cfg);
 	if (ret)
-		return ret == -EEXIST ? 0 : ret;
-
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		u32 idx, s2cr;
-
-		idx = cfg->smendx[i];
-		s2cr = S2CR_TYPE_TRANS |
-		       (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
-		writel_relaxed(s2cr, gr0_base + ARM_SMMU_GR0_S2CR(idx));
-	}
-
-	return 0;
-}
-
-static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
-					  struct arm_smmu_master_cfg *cfg)
-{
-	int i;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
+		return ret;
 
-	/*
-	 * We *must* clear the S2CR first, because freeing the SMR means
-	 * that it can be re-allocated immediately.
-	 * Xen: Unlike Linux, any access to non-configured stream will fault.
-	 */
 	for (i = 0; i < cfg->num_streamids; ++i) {
 		int idx = cfg->smendx[i];
 
-		/* An IOMMU group is torn down by the first device to be removed */
-		if (idx == INVALID_SMENDX)
-			return;
+		/* Devices in an IOMMU group may already be configured */
+		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
+			break;
 
-		writel_relaxed(S2CR_TYPE_FAULT,
-			       gr0_base + ARM_SMMU_GR0_S2CR(idx));
+		s2cr[idx].type = type ;
+		s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
+		s2cr[idx].cbndx = cbndx;
+		arm_smmu_write_s2cr(smmu, idx);
 	}
-
-	arm_smmu_master_free_smes(smmu, cfg);
+	return 0;
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1564,24 +1599,17 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (!cfg)
 		return -ENODEV;
 
-	ret = arm_smmu_domain_add_master(smmu_domain, cfg);
-
-	if (!ret)
-		dev_iommu_domain(dev) = domain;
-	return ret;
+	return arm_smmu_domain_add_master(smmu_domain, cfg);
 }
 
 static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
-	struct arm_smmu_master_cfg *cfg;
+	struct arm_smmu_device *smmu = find_smmu_for_device(dev);
+	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 
-	cfg = find_smmu_master_cfg(dev);
-	if (!cfg)
-		return;
+	if (smmu && cfg)
+		arm_smmu_master_free_smes(smmu, cfg);
 
-	dev_iommu_domain(dev) = NULL;
-	arm_smmu_domain_remove_master(smmu_domain, cfg);
 }
 
 #if 0 /*
@@ -2039,16 +2067,8 @@ static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
 	 * Reset stream mapping groups: Initial values mark all SMRn as
 	 * invalid and all S2CRn as bypass unless overridden.
 	 */
-	for (i = 0; i < smmu->num_mapping_groups; ++i) {
-		if (smmu->smrs)
-			arm_smmu_write_smr(smmu, i);
-		/*
-		 * Xen: Unlike Linux, any access to a non-configure stream
-		 * will fault by default.
-		 */
-		writel_relaxed(S2CR_TYPE_FAULT,
-			gr0_base + ARM_SMMU_GR0_S2CR(i));
-	}
+	for (i = 0; i < smmu->num_mapping_groups; ++i)
+		arm_smmu_write_sme(smmu, i);
 
 	/* Make sure all context banks are disabled and clear CB_FSR  */
 	for (i = 0; i < smmu->num_context_banks; ++i) {
@@ -2110,6 +2130,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	unsigned long size;
 	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
 	u32 id;
+	int i;
 
 	dev_notice(smmu->dev, "probing hardware configuration...\n");
 	dev_notice(smmu->dev, "SMMUv%d with:\n", smmu->version);
@@ -2193,6 +2214,13 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 			   "\tstream matching with %lu register groups, mask 0x%x",
 			   size, smmu->smr_mask_mask);
 	}
+	/* s2cr->type == 0 means translation, so initialise explicitly */
+	smmu->s2crs = kmalloc_array(size, sizeof(*smmu->s2crs), GFP_KERNEL);
+	if (!smmu->s2crs)
+		return -ENOMEM;
+	for (i = 0; i < size; i++)
+		smmu->s2crs[i] = s2cr_init_val;
+
 	smmu->num_mapping_groups = size;
 
 	/* ID1 */
-- 
2.17.1

[PATCH 4/5] xen/arm: smmuv1: Add a stream map entry iterator

[Rahul Singh]

Backport commit d3097e39302083d58922a3d1032d7d59a63d263d
"iommu/arm-smmu: Add a stream map entry iterator" from the Linux kernel.

This patch is the preparatory work to fix the stream match conflict
when two devices have the same stream-id.

Original commit message:

We iterate over the SMEs associated with a master config quite a lot in
various places, and are about to do so even more. Let's wrap the idiom
in a handy iterator macro before the repetition gets out of hand.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index e1b937bd4b..2c1ea8e6ff 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -618,6 +618,8 @@ struct arm_smmu_master_cfg {
 	s16				smendx[MAX_MASTER_STREAMIDS];
 };
 #define INVALID_SMENDX			-1
+#define for_each_cfg_sme(cfg, i, idx) \
+	for (i = 0; idx = cfg->smendx[i], i < cfg->num_streamids; ++i)
 
 struct arm_smmu_master {
 	struct device_node		*of_node;
@@ -1457,8 +1459,8 @@ static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
 	int i, idx;
 
 	/* Allocate the SMRs on the SMMU */
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		if (cfg->smendx[i] != INVALID_SMENDX)
+	for_each_cfg_sme(cfg, i, idx) {
+		if (idx != INVALID_SMENDX)
 			return -EEXIST;
 
 		/* ...except on stream indexing hardware, of course */
@@ -1482,8 +1484,8 @@ static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
 		return 0;
 
 	/* It worked! Now, poke the actual hardware */
-	for (i = 0; i < cfg->num_streamids; ++i)
-		arm_smmu_write_smr(smmu, cfg->smendx[i]);
+	for_each_cfg_sme(cfg, i, idx)
+		arm_smmu_write_smr(smmu, idx);
 
 	return 0;
 
@@ -1498,15 +1500,13 @@ err_free_smrs:
 static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 				      struct arm_smmu_master_cfg *cfg)
 {
-	int i;
+	int i, idx;
 
 	/*
 	 * We *must* clear the S2CR first, because freeing the SMR means
 	 * that it can be re-allocated immediately.
 	 */
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		int idx = cfg->smendx[i];
-
+	for_each_cfg_sme(cfg, i, idx) {
 		/* An IOMMU group is torn down by the first device to be removed */
 		if (idx == INVALID_SMENDX)
 			return;
@@ -1518,9 +1518,9 @@ static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 	__iowmb();
 
 	/* Invalidate the SMRs before freeing back to the allocator */
-	for (i = 0; i < cfg->num_streamids; ++i) {
+	for_each_cfg_sme(cfg, i, idx) {
 		if (smmu->smrs)
-			arm_smmu_free_smr(smmu, cfg->smendx[i]);
+			arm_smmu_free_smr(smmu, idx);
 
 		cfg->smendx[i] = INVALID_SMENDX;
 	}
@@ -1529,7 +1529,7 @@ static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 				      struct arm_smmu_master_cfg *cfg)
 {
-	int i, ret = 0;
+	int i, idx, ret = 0;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
 	enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
@@ -1540,9 +1540,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	if (ret)
 		return ret;
 
-	for (i = 0; i < cfg->num_streamids; ++i) {
-		int idx = cfg->smendx[i];
-
+	for_each_cfg_sme(cfg, i, idx) {
 		/* Devices in an IOMMU group may already be configured */
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			break;
-- 
2.17.1

[PATCH 5/5] xen/arm: smmuv1: Intelligent SMR allocation

[Rahul Singh]

Backport 588888a7399db352d2b1a41c9d5b3bf0fd482390
"iommu/arm-smmu: Intelligent SMR allocation" from the Linux kernel

This patch fix the stream match conflict issue when two devices have the
same stream-id.

Only difference while applying this patch is to use spinlock in place of
mutex and move iommu_group_alloc(..) function call in
arm_smmu_add_device(..) function from the start of the function
to the end.

Original commit message:

Stream Match Registers are one of the more awkward parts of the SMMUv2
architecture; there are typically never enough to assign one to each
stream ID in the system, and configuring them such that a single ID
matches multiple entries is catastrophically bad - at best, every
transaction raises a global fault; at worst, they go *somewhere*.

To address the former issue, we can mask ID bits such that a single
register may be used to match multiple IDs belonging to the same device
or group, but doing so also heightens the risk of the latter problem
(which can be nasty to debug).

Tackle both problems at once by replacing the simple bitmap allocator
with something much cleverer. Now that we have convenient in-memory
representations of the stream mapping table, it becomes straightforward
to properly validate new SMR entries against the current state, opening
the door to arbitrary masking and SMR sharing.

Another feature which falls out of this is that with IDs shared by
separate devices being automatically accounted for, simply associating a
group pointer with the S2CR offers appropriate group allocation almost
for free, so hook that up in the process.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 230 ++++++++++++++++++-----------
 1 file changed, 142 insertions(+), 88 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 2c1ea8e6ff..20ac672e91 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -597,6 +597,8 @@ enum arm_smmu_arch_version {
 };
 
 struct arm_smmu_s2cr {
+	struct iommu_group		*group;
+	int				count;
 	enum arm_smmu_s2cr_type		type;
 	enum arm_smmu_s2cr_privcfg	privcfg;
 	u8				cbndx;
@@ -613,6 +615,7 @@ struct arm_smmu_smr {
 };
 
 struct arm_smmu_master_cfg {
+	struct arm_smmu_device		*smmu;
 	int				num_streamids;
 	u16				streamids[MAX_MASTER_STREAMIDS];
 	s16				smendx[MAX_MASTER_STREAMIDS];
@@ -655,6 +658,7 @@ struct arm_smmu_device {
 	u16				smr_mask_mask;
 	struct arm_smmu_smr		*smrs;
 	struct arm_smmu_s2cr		*s2crs;
+	spinlock_t			stream_map_lock;
 
 	unsigned long			s1_input_size;
 	unsigned long			s1_output_size;
@@ -1407,23 +1411,6 @@ static void arm_smmu_domain_destroy(struct iommu_domain *domain)
 	kfree(smmu_domain);
 }
 
-static int arm_smmu_alloc_smr(struct arm_smmu_device *smmu)
-{
-	int i;
-
-	for (i = 0; i < smmu->num_mapping_groups; i++)
-		if (!cmpxchg(&smmu->smrs[i].valid, false, true))
-			return i;
-
-	return INVALID_SMENDX;
-}
-
-static void arm_smmu_free_smr(struct arm_smmu_device *smmu, int idx)
-{
-	writel_relaxed(~SMR_VALID, ARM_SMMU_GR0(smmu) + ARM_SMMU_GR0_SMR(idx));
-	write_atomic(&smmu->smrs[idx].valid, false);
-}
-
 static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
 {
 	struct arm_smmu_smr *smr = smmu->smrs + idx;
@@ -1452,98 +1439,143 @@ static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int idx)
 		arm_smmu_write_smr(smmu, idx);
 }
 
-static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
-				      struct arm_smmu_master_cfg *cfg)
+static int arm_smmu_find_sme(struct arm_smmu_device *smmu, u16 id, u16 mask)
 {
 	struct arm_smmu_smr *smrs = smmu->smrs;
-	int i, idx;
+	int i, free_idx = -ENOSPC;
 
-	/* Allocate the SMRs on the SMMU */
-	for_each_cfg_sme(cfg, i, idx) {
-		if (idx != INVALID_SMENDX)
-			return -EEXIST;
+	/* Stream indexing is blissfully easy */
+	if (!smrs)
+		return id;
 
-		/* ...except on stream indexing hardware, of course */
-		if (!smrs) {
-			cfg->smendx[i] = cfg->streamids[i];
+	/* Validating SMRs is... less so */
+	for (i = 0; i < smmu->num_mapping_groups; ++i) {
+		if (!smrs[i].valid) {
+			/*
+			 * Note the first free entry we come across, which
+			 * we'll claim in the end if nothing else matches.
+			 */
+			if (free_idx < 0)
+				free_idx = i;
 			continue;
 		}
+		/*
+		 * If the new entry is _entirely_ matched by an existing entry,
+		 * then reuse that, with the guarantee that there also cannot
+		 * be any subsequent conflicting entries. In normal use we'd
+		 * expect simply identical entries for this case, but there's
+		 * no harm in accommodating the generalisation.
+		 */
+		if ((mask & smrs[i].mask) == mask &&
+		    !((id ^ smrs[i].id) & ~smrs[i].mask))
+			return i;
+		/*
+		 * If the new entry has any other overlap with an existing one,
+		 * though, then there always exists at least one stream ID
+		 * which would cause a conflict, and we can't allow that risk.
+		 */
+		if (!((id ^ smrs[i].id) & ~(smrs[i].mask | mask)))
+			return -EINVAL;
+	}
 
-		idx = arm_smmu_alloc_smr(smmu);
-		if (IS_ERR_VALUE(idx)) {
-			dev_err(smmu->dev, "failed to allocate free SMR\n");
-			goto err_free_smrs;
+	return free_idx;
+}
+
+static bool arm_smmu_free_sme(struct arm_smmu_device *smmu, int idx)
+{
+	if (--smmu->s2crs[idx].count)
+		return false;
+
+	smmu->s2crs[idx] = s2cr_init_val;
+	if (smmu->smrs)
+		smmu->smrs[idx].valid = false;
+
+	return true;
+}
+
+static int arm_smmu_master_alloc_smes(struct device *dev)
+{
+	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
+	struct arm_smmu_device *smmu = cfg->smmu;
+	struct arm_smmu_smr *smrs = smmu->smrs;
+	struct iommu_group *group;
+	int i, idx, ret;
+
+	spin_lock(&smmu->stream_map_lock);
+	/* Figure out a viable stream map entry allocation */
+	for_each_cfg_sme(cfg, i, idx) {
+		if (idx != INVALID_SMENDX) {
+			ret = -EEXIST;
+			goto out_err;
 		}
-		cfg->smendx[i] = idx;
 
-		smrs[idx].id = cfg->streamids[i];
-		smrs[idx].mask = 0; /* We don't currently share SMRs */
+		ret = arm_smmu_find_sme(smmu, cfg->streamids[i], 0);
+		if (ret < 0)
+			goto out_err;
+
+		idx = ret;
+		if (smrs && smmu->s2crs[idx].count == 0) {
+			smrs[idx].id = cfg->streamids[i];
+			smrs[idx].mask = 0; /* We don't currently share SMRs */
+			smrs[idx].valid = true;
+		}
+		smmu->s2crs[idx].count++;
+		cfg->smendx[i] = (s16)idx;
 	}
 
-	if (!smrs)
-		return 0;
+	group = iommu_group_get(dev);
+	if (!group)
+		group = ERR_PTR(-ENOMEM);
+	if (IS_ERR(group)) {
+		ret = PTR_ERR(group);
+		goto out_err;
+	}
+	iommu_group_put(group);
 
 	/* It worked! Now, poke the actual hardware */
-	for_each_cfg_sme(cfg, i, idx)
-		arm_smmu_write_smr(smmu, idx);
+	for_each_cfg_sme(cfg, i, idx) {
+		arm_smmu_write_sme(smmu, idx);
+		smmu->s2crs[idx].group = group;
+	}
 
+	spin_unlock(&smmu->stream_map_lock);
 	return 0;
 
-err_free_smrs:
+out_err:
 	while (i--) {
-		arm_smmu_free_smr(smmu, cfg->smendx[i]);
+		arm_smmu_free_sme(smmu, cfg->smendx[i]);
 		cfg->smendx[i] = INVALID_SMENDX;
 	}
-	return -ENOSPC;
+	spin_unlock(&smmu->stream_map_lock);
+	return ret;
 }
 
-static void arm_smmu_master_free_smes(struct arm_smmu_device *smmu,
-				      struct arm_smmu_master_cfg *cfg)
+static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
 {
+    struct arm_smmu_device *smmu = cfg->smmu;
 	int i, idx;
 
-	/*
-	 * We *must* clear the S2CR first, because freeing the SMR means
-	 * that it can be re-allocated immediately.
-	 */
+	spin_lock(&smmu->stream_map_lock);
 	for_each_cfg_sme(cfg, i, idx) {
-		/* An IOMMU group is torn down by the first device to be removed */
-		if (idx == INVALID_SMENDX)
-			return;
-
-		smmu->s2crs[idx] = s2cr_init_val;
-		arm_smmu_write_s2cr(smmu, idx);
-	}
-	/* Sync S2CR updates before touching anything else */
-	__iowmb();
-
-	/* Invalidate the SMRs before freeing back to the allocator */
-	for_each_cfg_sme(cfg, i, idx) {
-		if (smmu->smrs)
-			arm_smmu_free_smr(smmu, idx);
-
+		if (arm_smmu_free_sme(smmu, idx))
+			arm_smmu_write_sme(smmu, idx);
 		cfg->smendx[i] = INVALID_SMENDX;
 	}
+	spin_unlock(&smmu->stream_map_lock);
 }
 
 static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 				      struct arm_smmu_master_cfg *cfg)
 {
-	int i, idx, ret = 0;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
 	enum arm_smmu_s2cr_type type = S2CR_TYPE_TRANS;
 	u8 cbndx = smmu_domain->cfg.cbndx;
-
-	if (cfg->smendx[0] == INVALID_SMENDX)
-		ret = arm_smmu_master_alloc_smes(smmu, cfg);
-	if (ret)
-		return ret;
+	int i, idx;
 
 	for_each_cfg_sme(cfg, i, idx) {
-		/* Devices in an IOMMU group may already be configured */
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
-			break;
+			continue;
 
 		s2cr[idx].type = type ;
 		s2cr[idx].privcfg = S2CR_PRIVCFG_UNPRIV;
@@ -1602,11 +1634,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
-	struct arm_smmu_device *smmu = find_smmu_for_device(dev);
 	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 
-	if (smmu && cfg)
-		arm_smmu_master_free_smes(smmu, cfg);
+	if (cfg)
+		arm_smmu_master_free_smes(cfg);
 
 }
 
@@ -1935,31 +1966,44 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }
 
+static struct iommu_group *arm_smmu_device_group(struct
+						arm_smmu_master_cfg *cfg)
+{
+	struct arm_smmu_device *smmu = cfg->smmu;
+	struct iommu_group *group = NULL;
+	int i, idx;
+
+	for_each_cfg_sme(cfg, i, idx) {
+		if (group && smmu->s2crs[idx].group &&
+		    group != smmu->s2crs[idx].group)
+			return ERR_PTR(-EINVAL);
+
+		group = smmu->s2crs[idx].group;
+	}
+
+	if (group)
+		return group;
+
+	return NULL;
+}
+
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
 	struct arm_smmu_master_cfg *cfg;
 	struct iommu_group *group;
 	void (*releasefn)(void *) = NULL;
-	int ret;
 
 	smmu = find_smmu_for_device(dev);
 	if (!smmu)
 		return -ENODEV;
 
-	group = iommu_group_alloc();
-	if (IS_ERR(group)) {
-		dev_err(dev, "Failed to allocate IOMMU group\n");
-		return PTR_ERR(group);
-	}
-
 	if (dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(dev);
 
 		cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
 		if (!cfg) {
-			ret = -ENOMEM;
-			goto out_put_group;
+			return -ENOMEM;
 		}
 
 		cfg->num_streamids = 1;
@@ -1970,24 +2014,33 @@ static int arm_smmu_add_device(struct device *dev)
 		pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid,
 				       &cfg->streamids[0]);
 		releasefn = __arm_smmu_release_pci_iommudata;
+		cfg->smmu = smmu;
 	} else {
 		struct arm_smmu_master *master;
 
 		master = find_smmu_master(smmu, dev->of_node);
 		if (!master) {
-			ret = -ENODEV;
-			goto out_put_group;
+			return -ENODEV;
 		}
 
 		cfg = &master->cfg;
+		cfg->smmu = smmu;
 	}
 
-	iommu_group_set_iommudata(group, cfg, releasefn);
-	ret = iommu_group_add_device(group, dev);
+	group = arm_smmu_device_group(cfg);
+	if (!group) {
+		group = iommu_group_alloc();
+		if (IS_ERR(group)) {
+			dev_err(dev, "Failed to allocate IOMMU group\n");
+			return PTR_ERR(group);
+		}
+	}
 
-out_put_group:
+	iommu_group_set_iommudata(group, cfg, releasefn);
+	iommu_group_add_device(group, dev);
 	iommu_group_put(group);
-	return ret;
+
+	return arm_smmu_master_alloc_smes(dev);
 }
 
 #if 0 /* Xen: We don't support remove device for now. Will be useful for PCI */
@@ -2220,6 +2273,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 		smmu->s2crs[i] = s2cr_init_val;
 
 	smmu->num_mapping_groups = size;
+	spin_lock_init(&smmu->stream_map_lock);
 
 	/* ID1 */
 	id = readl_relaxed(gr0_base + ARM_SMMU_GR0_ID1);
-- 
2.17.1

Re: [PATCH 5/5] xen/arm: smmuv1: Intelligent SMR allocation

[Julien Grall]

Hi Rahul,

On 09/03/2021 18:19, Rahul Singh wrote:
> Backport 588888a7399db352d2b1a41c9d5b3bf0fd482390
> "iommu/arm-smmu: Intelligent SMR allocation" from the Linux kernel
> 
> This patch fix the stream match conflict issue when two devices have the
> same stream-id.
> 
> Only difference while applying this patch is to use spinlock in place of
> mutex and move iommu_group_alloc(..) function call in
> arm_smmu_add_device(..) function from the start of the function
> to the end.

As you may remember the discussion on the SMMUv3 thread, replacing a 
spinlock by a mutex has consequences. Can you explain why this is fine?

Cheers,

-- 
Julien Grall

Re: [PATCH 5/5] xen/arm: smmuv1: Intelligent SMR allocation

[Rahul Singh]

Hello Julien,

> On 16 Mar 2021, at 3:08 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 09/03/2021 18:19, Rahul Singh wrote:
>> Backport 588888a7399db352d2b1a41c9d5b3bf0fd482390
>> "iommu/arm-smmu: Intelligent SMR allocation" from the Linux kernel
>> This patch fix the stream match conflict issue when two devices have the
>> same stream-id.
>> Only difference while applying this patch is to use spinlock in place of
>> mutex and move iommu_group_alloc(..) function call in
>> arm_smmu_add_device(..) function from the start of the function
>> to the end.
> 
> As you may remember the discussion on the SMMUv3 thread, replacing a spinlock by a mutex has consequences. Can you explain why this is fine?
Yes, I remember the discussion on the SMMUv3 thread, replacing a spinlock with a mutex has consequences. 

I replaced the mutex with spinlock in the SMMUv1 code when we are configuring the SMMUv1 hardware arm_smmu_master_alloc_smes(..).

I think it is fine to use the spinlock in place of mutex in SMMUv1 where we are configuring the hardware via registers as compared to SMMUv3 where we are configuring the SMMUv3 hardware with Memory-based circular buffer queues. Configuring the hardware via queues might take a long time that why mutex is a good choice but if we are configuring the hardware via registers it is very fast.

Configuring the SMMUv1 with the register is very fast and there are no side effects of this if we use spinlock. Let me know your view on this.

Regards,
Rahul

> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 5/5] xen/arm: smmuv1: Intelligent SMR allocation

[Julien Grall]

On 16/03/2021 17:54, Rahul Singh wrote:
> Hello Julien,

Hi Rahul,

>> On 16 Mar 2021, at 3:08 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 09/03/2021 18:19, Rahul Singh wrote:
>>> Backport 588888a7399db352d2b1a41c9d5b3bf0fd482390
>>> "iommu/arm-smmu: Intelligent SMR allocation" from the Linux kernel
>>> This patch fix the stream match conflict issue when two devices have the
>>> same stream-id.
>>> Only difference while applying this patch is to use spinlock in place of
>>> mutex and move iommu_group_alloc(..) function call in
>>> arm_smmu_add_device(..) function from the start of the function
>>> to the end.
>>
>> As you may remember the discussion on the SMMUv3 thread, replacing a spinlock by a mutex has consequences. Can you explain why this is fine?
> Yes, I remember the discussion on the SMMUv3 thread, replacing a spinlock with a mutex has consequences.
> 
> I replaced the mutex with spinlock in the SMMUv1 code when we are configuring the SMMUv1 hardware arm_smmu_master_alloc_smes(..).
> 
> I think it is fine to use the spinlock in place of mutex in SMMUv1 where we are configuring the hardware via registers as compared to SMMUv3 where we are configuring the SMMUv3 hardware with Memory-based circular buffer queues. Configuring the hardware via queues might take a long time that why mutex is a good choice but if we are configuring the hardware via registers it is very fast.
> 
> Configuring the SMMUv1 with the register is very fast and there are no side effects of this if we use spinlock. Let me know your view on this.

This looks fine. Can you explain it the commit message?

Cheers,

-- 
Julien Grall

Re: [PATCH 5/5] xen/arm: smmuv1: Intelligent SMR allocation

[Rahul Singh]

Hello Julien,

> On 20 Mar 2021, at 12:01 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/03/2021 17:54, Rahul Singh wrote:
>> Hello Julien,
> 
> Hi Rahul,
> 
>>> On 16 Mar 2021, at 3:08 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Rahul,
>>> 
>>> On 09/03/2021 18:19, Rahul Singh wrote:
>>>> Backport 588888a7399db352d2b1a41c9d5b3bf0fd482390
>>>> "iommu/arm-smmu: Intelligent SMR allocation" from the Linux kernel
>>>> This patch fix the stream match conflict issue when two devices have the
>>>> same stream-id.
>>>> Only difference while applying this patch is to use spinlock in place of
>>>> mutex and move iommu_group_alloc(..) function call in
>>>> arm_smmu_add_device(..) function from the start of the function
>>>> to the end.
>>> 
>>> As you may remember the discussion on the SMMUv3 thread, replacing a spinlock by a mutex has consequences. Can you explain why this is fine?
>> Yes, I remember the discussion on the SMMUv3 thread, replacing a spinlock with a mutex has consequences.
>> I replaced the mutex with spinlock in the SMMUv1 code when we are configuring the SMMUv1 hardware arm_smmu_master_alloc_smes(..).
>> I think it is fine to use the spinlock in place of mutex in SMMUv1 where we are configuring the hardware via registers as compared to SMMUv3 where we are configuring the SMMUv3 hardware with Memory-based circular buffer queues. Configuring the hardware via queues might take a long time that why mutex is a good choice but if we are configuring the hardware via registers it is very fast.
>> Configuring the SMMUv1 with the register is very fast and there are no side effects of this if we use spinlock. Let me know your view on this.
> 
> This looks fine. Can you explain it the commit message?

Yes, I will add the explanation in the commit message and will send the v2.

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On Tue, 9 Mar 2021, Rahul Singh wrote:
> This patch is the work to fix the stream match conflict issue when two devices
> have the same stream-id.
> 
> Approach taken is to merge the below commit from Linux driver to fix the
> issue.
> 
> 1. "iommu/arm-smmu: Handle stream IDs more dynamically"
>     commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 2. "iommu/arm-smmu: Consolidate stream map entry state"
>     commit 1f3d5ca43019bff1105838712d55be087d93c0da
> 3. "iommu/arm-smmu: Keep track of S2CR state"
>     commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
> 4. "iommu/arm-smmu: Add a stream map entry iterator"
>     commit d3097e39302083d58922a3d1032d7d59a63d263d
> 5. "iommu/arm-smmu: Intelligent SMR allocation"
>     commit 588888a7399db352d2b1a41c9d5b3bf0fd482390
> 
> Rahul Singh (5):
>   xen/arm: smmuv1: Handle stream IDs more dynamically
>   xen/arm: smmuv1: Consolidate stream map entry state
>   xen/arm: smmuv1: Keep track of S2CR state
>   xen/arm: smmuv1: Add a stream map entry iterator
>   xen/arm: smmuv1: Intelligent SMR allocation
> 
>  xen/drivers/passthrough/arm/smmu.c | 423 ++++++++++++++++++-----------
>  1 file changed, 262 insertions(+), 161 deletions(-)

I didn't closely review the changes but I made sure that all patches
build and I also compared each patch with the original commit in Linux:
all changes correspond correctly.

For the whole series:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[Bertrand Marquis]

Hi Rahul,

> On 9 Mar 2021, at 19:19, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> This patch is the work to fix the stream match conflict issue when two devices
> have the same stream-id.
> 
> Approach taken is to merge the below commit from Linux driver to fix the
> issue.
> 
> 1. "iommu/arm-smmu: Handle stream IDs more dynamically"
>    commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 2. "iommu/arm-smmu: Consolidate stream map entry state"
>    commit 1f3d5ca43019bff1105838712d55be087d93c0da
> 3. "iommu/arm-smmu: Keep track of S2CR state"
>    commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
> 4. "iommu/arm-smmu: Add a stream map entry iterator"
>    commit d3097e39302083d58922a3d1032d7d59a63d263d
> 5. "iommu/arm-smmu: Intelligent SMR allocation"
>    commit 588888a7399db352d2b1a41c9d5b3bf0fd482390
> 
> Rahul Singh (5):
>  xen/arm: smmuv1: Handle stream IDs more dynamically
>  xen/arm: smmuv1: Consolidate stream map entry state
>  xen/arm: smmuv1: Keep track of S2CR state
>  xen/arm: smmuv1: Add a stream map entry iterator
>  xen/arm: smmuv1: Intelligent SMR allocation
> 
> xen/drivers/passthrough/arm/smmu.c | 423 ++++++++++++++++++-----------
> 1 file changed, 262 insertions(+), 161 deletions(-)

For the whole serie:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> -- 
> 2.17.1
> 

Re: [PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[Julien Grall]

Hi Rahul,

On 09/03/2021 18:19, Rahul Singh wrote:
> This patch is the work to fix the stream match conflict issue when two devices
> have the same stream-id.
> 
> Approach taken is to merge the below commit from Linux driver to fix the
> issue.
> 
> 1. "iommu/arm-smmu: Handle stream IDs more dynamically"
>      commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
> 2. "iommu/arm-smmu: Consolidate stream map entry state"
>      commit 1f3d5ca43019bff1105838712d55be087d93c0da
> 3. "iommu/arm-smmu: Keep track of S2CR state"
>      commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
> 4. "iommu/arm-smmu: Add a stream map entry iterator"
>      commit d3097e39302083d58922a3d1032d7d59a63d263d
> 5. "iommu/arm-smmu: Intelligent SMR allocation"
>      commit 588888a7399db352d2b1a41c9d5b3bf0fd482390

A couple of questions:
  * Are they backported verbatim from Linux upstream?
  * Did you check there was no bug fix afterwards?

Cheers,

-- 
Julien Grall

Re: [PATCH 0/5] xen/arm: smmuv1: Fix stream match conflict issue

[Rahul Singh]

Hello Julien,

> On 16 Mar 2021, at 3:04 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 09/03/2021 18:19, Rahul Singh wrote:
>> This patch is the work to fix the stream match conflict issue when two devices
>> have the same stream-id.
>> Approach taken is to merge the below commit from Linux driver to fix the
>> issue.
>> 1. "iommu/arm-smmu: Handle stream IDs more dynamically"
>>     commit 21174240e4f4439bb8ed6c116cdbdc03eba2126e
>> 2. "iommu/arm-smmu: Consolidate stream map entry state"
>>     commit 1f3d5ca43019bff1105838712d55be087d93c0da
>> 3. "iommu/arm-smmu: Keep track of S2CR state"
>>     commit 8e8b203eabd8b9e96d02d6339e4abce3e5a7ea4b
>> 4. "iommu/arm-smmu: Add a stream map entry iterator"
>>     commit d3097e39302083d58922a3d1032d7d59a63d263d
>> 5. "iommu/arm-smmu: Intelligent SMR allocation"
>>     commit 588888a7399db352d2b1a41c9d5b3bf0fd482390
> 
> A couple of questions:
> * Are they backported verbatim from Linux upstream?

Yes all the patches are backported verbatim form Linux upstream.

> * Did you check there was no bug fix afterwards?

Yes I checked there is no bug afterwards related to the patches added.

Regards,
Rahul

> 
> Cheers,
> 
> -- 
> Julien Grall