* [PATCH v2 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
@ 2021-03-22 16:11 ` Rahul Singh
2021-03-22 16:11 ` [PATCH v2 2/5] xen/arm: smmuv1: Consolidate stream map entry state Rahul Singh
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-22 16:11 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
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:
iommu/arm-smmu: Handle stream IDs more dynamically
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).
Acked-by: Will Deacon <will.deacon@arm.com>
Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 2/5] xen/arm: smmuv1: Consolidate stream map entry state
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
2021-03-22 16:11 ` [PATCH v2 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically Rahul Singh
@ 2021-03-22 16:11 ` Rahul Singh
2021-03-22 16:11 ` [PATCH v2 3/5] xen/arm: smmuv1: Keep track of S2CR state Rahul Singh
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-22 16:11 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
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:
iommu/arm-smmu: Consolidate stream map entry state
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 <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 3/5] xen/arm: smmuv1: Keep track of S2CR state
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
2021-03-22 16:11 ` [PATCH v2 1/5] xen/arm: smmuv1: Handle stream IDs more dynamically Rahul Singh
2021-03-22 16:11 ` [PATCH v2 2/5] xen/arm: smmuv1: Consolidate stream map entry state Rahul Singh
@ 2021-03-22 16:11 ` Rahul Singh
2021-03-22 16:11 ` [PATCH v2 4/5] xen/arm: smmuv1: Add a stream map entry iterator Rahul Singh
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-22 16:11 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
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:
iommu/arm-smmu: Keep track of S2CR state
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: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 4/5] xen/arm: smmuv1: Add a stream map entry iterator
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
` (2 preceding siblings ...)
2021-03-22 16:11 ` [PATCH v2 3/5] xen/arm: smmuv1: Keep track of S2CR state Rahul Singh
@ 2021-03-22 16:11 ` Rahul Singh
2021-03-22 16:11 ` [PATCH v2 5/5] xen/arm: smmuv1: Intelligent SMR allocation Rahul Singh
2021-03-24 20:48 ` [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Julien Grall
5 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-22 16:11 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
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:
iommu/arm-smmu: Add a stream map entry iterator
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.
Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 5/5] xen/arm: smmuv1: Intelligent SMR allocation
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
` (3 preceding siblings ...)
2021-03-22 16:11 ` [PATCH v2 4/5] xen/arm: smmuv1: Add a stream map entry iterator Rahul Singh
@ 2021-03-22 16:11 ` Rahul Singh
2021-03-24 20:48 ` [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Julien Grall
5 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-22 16:11 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk
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 with regard to Linux patch are
as follows:
1. Spinlock is used in place of mutex when attaching a device to the
SMMU via arm_smmu_master_alloc_smes(..) function call.Replacing the
mutex with spinlock is fine here as we are configuring the hardware
via registers and it is very fast.
2. move iommu_group_alloc(..) function call in arm_smmu_add_device(..)
function from the start of the function to the end.
Original commit message:
iommu/arm-smmu: Intelligent SMR allocation
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: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Bertrand Marquis <bertrand.marquis@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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue
2021-03-22 16:11 [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Rahul Singh
` (4 preceding siblings ...)
2021-03-22 16:11 ` [PATCH v2 5/5] xen/arm: smmuv1: Intelligent SMR allocation Rahul Singh
@ 2021-03-24 20:48 ` Julien Grall
2021-03-25 8:43 ` Rahul Singh
5 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2021-03-24 20:48 UTC (permalink / raw)
To: Rahul Singh, xen-devel
Cc: bertrand.marquis, Stefano Stabellini, Volodymyr Babchuk
Hi Rahul,
On 22/03/2021 16:11, 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
>
> Changes since v2:
> - Updated commit message to show the original author of the patch.
> - Added Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> - Addded Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> 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
I went through the commit messages and they look good to me:
Acked-by: Julien Grall <jgrall@amazon.com>
I have committed the series to my for-next/4.16 branch. They will be
merged soon in staging.
Cheers,
>
> xen/drivers/passthrough/arm/smmu.c | 423 ++++++++++++++++++-----------
> 1 file changed, 262 insertions(+), 161 deletions(-)
>
--
Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue
2021-03-24 20:48 ` [PATCH v2 0/5] xen/arm: smmuv1: Fix stream match conflict issue Julien Grall
@ 2021-03-25 8:43 ` Rahul Singh
0 siblings, 0 replies; 8+ messages in thread
From: Rahul Singh @ 2021-03-25 8:43 UTC (permalink / raw)
To: Julien Grall
Cc: xen-devel, Bertrand Marquis, Stefano Stabellini,
Volodymyr Babchuk
Hi Julien,
> On 24 Mar 2021, at 8:48 pm, Julien Grall <julien@xen.org> wrote:
>
> Hi Rahul,
>
> On 22/03/2021 16:11, 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
>> Changes since v2:
>> - Updated commit message to show the original author of the patch.
>> - Added Acked-by: Stefano Stabellini <sstabellini@kernel.org>
>> - Addded Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>> 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
>
> I went through the commit messages and they look good to me:
>
> Acked-by: Julien Grall <jgrall@amazon.com>
>
> I have committed the series to my for-next/4.16 branch. They will be merged soon in staging.
Thanks.
Regards,
Rahul
>
> Cheers,
>
>> xen/drivers/passthrough/arm/smmu.c | 423 ++++++++++++++++++-----------
>> 1 file changed, 262 insertions(+), 161 deletions(-)
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 8+ messages in thread