On Tue, Aug 23, 2016 at 08:05:20PM +0100, Robin Murphy wrote:
> In order to consider SMR masking, we really want to be able to validate
> ID/mask pairs against existing SMR contents to prevent stream match
> conflicts, which at best would cause transactions to fault unexpectedly,
> and at worst lead to silent unpredictable behaviour. With our SMMU
> instance data holding only an allocator bitmap, and the SMR values
> themselves scattered across master configs hanging off devices which we
> may have no way of finding, there's essentially no way short of digging
> everything back out of the hardware. Similarly, the thought of power
> management ops to support suspend/resume faces the exact same problem.
>
> By massaging the software state into a closer shape to the underlying
> hardware, everything comes together quite nicely; the allocator and the
> high-level view of the data become a single centralised state which we
> can easily keep track of, and to which any updates can be validated in
> full before being synchronised to the hardware itself.
>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> drivers/iommu/arm-smmu.c | 147 +++++++++++++++++++++++++++--------------------
> 1 file changed, 86 insertions(+), 61 deletions(-)
[...]
> +static int arm_smmu_master_alloc_smes(struct arm_smmu_device *smmu,
> + struct arm_smmu_master_cfg *cfg)
> +{
> + struct arm_smmu_smr *smrs = smmu->smrs;
> + int i, idx;
>
> /* Allocate the SMRs on the SMMU */
> for (i = 0; i < cfg->num_streamids; ++i) {
> - int idx = __arm_smmu_alloc_bitmap(smmu->smr_map, 0,
> - smmu->num_mapping_groups);
> + if (cfg->smendx[i] >= 0)
> + return -EEXIST;
> +
> + /* ...except on stream indexing hardware, of course */
> + if (!smrs) {
> + cfg->smendx[i] = cfg->streamids[i];
> + continue;
> + }
> +
> + idx = arm_smmu_alloc_smr(smmu);
> if (idx < 0) {
> dev_err(smmu->dev, "failed to allocate free SMR\n");
> goto err_free_smrs;
> }
> + cfg->smendx[i] = idx;
>
> - smrs[i] = (struct arm_smmu_smr) {
> - .idx = idx,
> - .mask = 0, /* We don't currently share SMRs */
> - .id = cfg->streamids[i],
> - };
> + smrs[idx].id = cfg->streamids[i];
> + smrs[idx].mask = 0; /* We don't currently share SMRs */
> }
>
> + if (!smrs)
> + return 0;
> +
> /* It worked! Now, poke the actual hardware */
> - for (i = 0; i < cfg->num_streamids; ++i) {
> - u32 reg = SMR_VALID | smrs[i].id << SMR_ID_SHIFT |
> - smrs[i].mask << SMR_MASK_SHIFT;
> - writel_relaxed(reg, gr0_base + ARM_SMMU_GR0_SMR(smrs[i].idx));
> - }
> + for (i = 0; i < cfg->num_streamids; ++i)
> + arm_smmu_write_smr(smmu, cfg->smendx[i]);
>
> - cfg->smrs = smrs;
> return 0;
>
> err_free_smrs:
> - while (--i >= 0)
> - __arm_smmu_free_bitmap(smmu->smr_map, smrs[i].idx);
> - kfree(smrs);
> + while (i--) {
> + arm_smmu_free_smr(smmu, cfg->smendx[i]);
> + cfg->smendx[i] = INVALID_SMENDX;
> + }
Hmm, don't you have an off-by-one here? At least, looking at the final
code, we branch to out_err from within the for_each_cfg_sme loop, but
before we've incremented smmu->s2crs[idx].count, so the arm_smmu_free_smr
will erroneously decrement that.
Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
