Re: [PATCH v5 14/19] iommu/arm-smmu: Intelligent SMR allocation

[Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>]

On Tue, Aug 23, 2016 at 08:05:25PM +0100, Robin Murphy wrote:
> 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-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 192 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 114 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 17bf871030c6..88f82eb8d1fe 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -302,6 +302,8 @@ enum arm_smmu_implementation {
>  };
>  
>  struct arm_smmu_s2cr {
> +	struct iommu_group		*group;
> +	int				count;
>  	enum arm_smmu_s2cr_type		type;
>  	enum arm_smmu_s2cr_privcfg	privcfg;
>  	u8				cbndx;
> @@ -363,6 +365,7 @@ struct arm_smmu_device {
>  	u16				smr_mask_mask;
>  	struct arm_smmu_smr		*smrs;
>  	struct arm_smmu_s2cr		*s2crs;
> +	struct mutex			stream_map_mutex;
>  
>  	unsigned long			va_size;
>  	unsigned long			ipa_size;
> @@ -1016,23 +1019,6 @@ static void arm_smmu_domain_free(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_ONCE(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;
> @@ -1060,49 +1046,110 @@ 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, idx = -ENOSPC;
>  
> -	/* Allocate the SMRs on the SMMU */
> -	for_each_cfg_sme(cfg, i, idx) {
> -		if (idx >= 0)
> -			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) {
> +			if (idx < 0)
> +				idx = i;

This is to stash an "empty" entry index in case none matches, right ?
Let me say it is not that obvious, that deserves a comment since it
is hard to follow.

>  			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;
> +		/* Exact matches are good */
> +		if (mask == smrs[i].mask && id == smrs[i].id)
> +			return i;
>  
> -		smrs[idx].id = cfg->streamids[i];
> -		smrs[idx].mask = 0; /* We don't currently share SMRs */
> +		/* New unmasked IDs matching existing masks we can cope with */
> +		if (!mask && !((smrs[i].id ^ id) & ~smrs[i].mask))
> +			return i;
> +
> +		/* Overlapping masks are right out */
> +		if (mask & smrs[i].mask)
> +			return -EINVAL;
> +
> +		/* Distinct masks must match unambiguous ranges */
> +		if (mask && !((smrs[i].id ^ id) & ~(smrs[i].mask | mask)))
> +			return -EINVAL;

And this is to _prevent_ an entry to match the input streamid
range (masked streamid) because it would create an ambiguous match ?

Basically here you keep looping because this function is at the
same time allocating and validating the SMRS (ie you want to make
sure there are no valid entries that clash with the streamid you
are currently allocating).

It is a tad complicated and a bit hard to parse. I wonder if it would
not be better to split into two SMRS validation and allocation
functions, I think I fully grasped what you want to achieve but it is
not that trivial.

Thanks,
Lorenzo

>  	}
>  
> -	if (!smrs)
> -		return 0;
> +	return 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 = dev->archdata.iommu;
> +	struct arm_smmu_device *smmu = cfg->smmu;
> +	struct arm_smmu_smr *smrs = smmu->smrs;
> +	struct iommu_group *group;
> +	int i, idx, ret;
> +
> +	mutex_lock(&smmu->stream_map_mutex);
> +	/* Figure out a viable stream map entry allocation */
> +	for_each_cfg_sme(cfg, i, idx) {
> +		if (idx >= 0) {
> +			ret = -EEXIST;
> +			goto out_err;
> +		}
> +
> +		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;
> +	}
> +
> +	group = iommu_group_get_for_dev(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;
> +	}
>  
> +	mutex_unlock(&smmu->stream_map_mutex);
>  	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;
> +	mutex_unlock(&smmu->stream_map_mutex);
> +	return ret;
>  }
>  
>  static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg)
> @@ -1110,43 +1157,23 @@ 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.
> -	 */
> +	mutex_lock(&smmu->stream_map_mutex);
>  	for_each_cfg_sme(cfg, i, idx) {
> -		/* An IOMMU group is torn down by the first device to be removed */
> -		if (idx < 0)
> -			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;
>  	}
> +	mutex_unlock(&smmu->stream_map_mutex);
>  }
>  
>  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] < 0)
> -		ret = arm_smmu_master_alloc_smes(smmu, cfg);
> -	if (ret)
> -		return ret;
> +	int i, idx;
>  
>  	/*
>  	 * FIXME: This won't be needed once we have IOMMU-backed DMA ops
> @@ -1158,9 +1185,8 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>  		type = S2CR_TYPE_BYPASS;
>  
>  	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;
> @@ -1320,7 +1346,6 @@ static bool arm_smmu_capable(enum iommu_cap cap)
>  static int arm_smmu_add_device(struct device *dev)
>  {
>  	struct arm_smmu_master_cfg *cfg;
> -	struct iommu_group *group;
>  	int i, ret;
>  
>  	ret = arm_smmu_register_legacy_master(dev);
> @@ -1340,13 +1365,9 @@ static int arm_smmu_add_device(struct device *dev)
>  		cfg->smendx[i] = INVALID_SMENDX;
>  	}
>  
> -	group = iommu_group_get_for_dev(dev);
> -	if (IS_ERR(group)) {
> -		ret = PTR_ERR(group);
> -		goto out_free;
> -	}
> -	iommu_group_put(group);
> -	return 0;
> +	ret = arm_smmu_master_alloc_smes(dev);
> +	if (!ret)
> +		return ret;
>  
>  out_free:
>  	kfree(cfg);
> @@ -1369,7 +1390,21 @@ static void arm_smmu_remove_device(struct device *dev)
>  
>  static struct iommu_group *arm_smmu_device_group(struct device *dev)
>  {
> -	struct iommu_group *group;
> +	struct arm_smmu_master_cfg *cfg = dev->archdata.iommu;
> +	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;
>  
>  	if (dev_is_pci(dev))
>  		group = pci_device_group(dev);
> @@ -1652,6 +1687,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>  		smmu->s2crs[i] = s2cr_init_val;
>  
>  	smmu->num_mapping_groups = size;
> +	mutex_init(&smmu->stream_map_mutex);
>  
>  	if (smmu->version < ARM_SMMU_V2 || !(id & ID0_PTFS_NO_AARCH32)) {
>  		smmu->features |= ARM_SMMU_FEAT_FMT_AARCH32_L;
> -- 
> 2.8.1.dirty
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html