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

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

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-5wv7dgnIgG8@public.gmane.org>
> ---
>  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