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

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 01/09/16 19:32, Will Deacon wrote:
> 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@arm.com>
>> ---
>>  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.

Given that s2crs only doesn't exist until patch 10, and s2crs->count
doesn't exist until patch 14, I'd have to say pick one of:

a) no

b) ¯\_(ツ)_/¯

Robin.

> 
> Will
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu