diff mbox

[Xen-devel,v3,10/13] xen/iommu: smmu: Check for duplicate stream IDs when registering master devices

Message ID 1422643768-23614-11-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall Jan. 30, 2015, 6:49 p.m. UTC 
From: Andreas Herrmann <andreas.herrmann@calxeda.com>

If DT information lists one stream ID twice for the master devices of
an SMMU this can cause a multi match when stream ID matching is used.
For stream ID indexing this might trigger an overwrite of an S2CR that
is already in use.

So better check for duplicates when DT information is parsed.

Taken from the linux ML:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html

Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch was sent on Linux ML back to January 2014. It has never
    been pushed upstream as the only use was for Calxeda.

    Note that SMMU bindings for Calxeda is not upstreamed.
---
 xen/drivers/passthrough/arm/smmu.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

Comments

Julien Grall Feb. 20, 2015, 1:34 p.m. UTC | #1 
On 20/02/15 12:35, Ian Campbell wrote:
> On Fri, 2015-01-30 at 18:49 +0000, Julien Grall wrote:
>> From: Andreas Herrmann <andreas.herrmann@calxeda.com>
>>
>> If DT information lists one stream ID twice for the master devices of
>> an SMMU this can cause a multi match when stream ID matching is used.
>> For stream ID indexing this might trigger an overwrite of an S2CR that
>> is already in use.
>>
>> So better check for duplicates when DT information is parsed.
>>
>> Taken from the linux ML:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html
>>
>> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com>
>> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> I think you were going to drop this one as it is not strictly required?
> 
> I'm still not entirely clear on the motivation for this patch, looking
> at the v2 thread I see "But, the developer made have written by mistake
> twice the streamid for the device." which I think you were saying that
> the DT might, hypothetically, be wrong and have duplicated IDs, is that
> right?
> 
> Is it a hypothetical problem or have we seen it in a real device-tree?

It's an hypothetical problem. The algo on the next patch won't work
correctly otherwise.

I may need to rework a bit the next patch if we drop it.

Regards,
diff mbox

Patch

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 6cd47b7..bfc1069 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -51,6 +51,9 @@ 
 /* Maximum number of stream IDs assigned to a single device */
 #define MAX_MASTER_STREAMIDS		MAX_PHANDLE_ARGS
 
+/* Maximum stream ID */
+#define ARM_SMMU_MAX_STREAMID		(SZ_64K - 1)
+
 /* Maximum number of context banks per SMMU */
 #define ARM_SMMU_MAX_CBS		128
 
@@ -519,7 +522,8 @@  static int insert_smmu_master(struct arm_smmu_device *smmu,
 
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
-				struct of_phandle_args *masterspec)
+				struct of_phandle_args *masterspec,
+				unsigned long *smmu_sids)
 {
 	int i;
 	struct arm_smmu_master *master;
@@ -556,6 +560,12 @@  static int register_smmu_master(struct arm_smmu_device *smmu,
 				masterspec->np->name, smmu->num_mapping_groups);
 			return -ERANGE;
 		}
+
+		if (test_and_set_bit(streamid, smmu_sids)) {
+			dev_err(dev, "duplicate stream ID (%d)\n", streamid);
+			return -EEXIST;
+		}
+
 		master->cfg.streamids[i] = streamid;
 	}
 	return insert_smmu_master(smmu, master);
@@ -1977,6 +1987,7 @@  static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rb_node *node;
 	struct of_phandle_args masterspec;
+	unsigned long *smmu_sids;
 	int num_irqs, i, err;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
@@ -2035,20 +2046,30 @@  static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	smmu_sids = kzalloc(BITS_TO_LONGS(ARM_SMMU_MAX_STREAMID) *
+			sizeof(long), GFP_KERNEL);
+	if (!smmu_sids) {
+		dev_err(dev,
+			"failed to allocate bitmap for stream ID tracking\n");
+		return -ENOMEM;
+	}
+
 	i = 0;
 	smmu->masters = RB_ROOT;
 	while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters",
 					   "#stream-id-cells", i,
 					   &masterspec)) {
-		err = register_smmu_master(smmu, dev, &masterspec);
+		err = register_smmu_master(smmu, dev, &masterspec, smmu_sids);
 		if (err) {
 			dev_err(dev, "failed to add master %s\n",
 				masterspec.np->name);
+			kfree(smmu_sids);
 			goto out_put_masters;
 		}
 
 		i++;
 	}
+	kfree(smmu_sids);
 	dev_notice(dev, "registered %d master devices\n", i);
 
 	parse_driver_options(smmu);