[PATCH 0/4] IOMMU group cleanup

[PATCH 0/4] IOMMU group cleanup

[Robin Murphy]

Hi all,

Prompted by some discussion around the questionable usefulness of
iommu_present() for SoC topologies, I took a look at what was left to
finish off the intention of making groups mandatory. Turns out it
wasn't much, so here it is. As well as the general rationalisation
aspect, between this and the probe-deferral changes in 4.12, we should
now be able to rely on the presence of a group unambiguously indicating
whether a given device is associated with an IOMMU or not.

Robin.

Robin Murphy (4):
  iommu/msm: Add iommu_group support
  iommu/tegra-smmu: Add iommu_group support
  iommu/tegra-gart: Add iommu_group support
  iommu: Finish making iommu_group support mandatory

 drivers/iommu/iommu.c      | 19 ++++---------------
 drivers/iommu/msm_iommu.c  | 15 ++++++++++++++-
 drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
 drivers/iommu/tegra-smmu.c |  9 +++++++++
 4 files changed, 46 insertions(+), 16 deletions(-)

-- 
2.12.2.dirty

[PATCH 1/4] iommu/msm: Add iommu_group support

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index d0448353d501..04f4d51ffacb 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
 static int msm_iommu_add_device(struct device *dev)
 {
 	struct msm_iommu_dev *iommu;
+	struct iommu_group *group;
 	unsigned long flags;
 	int ret = 0;
 
@@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
 
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 
-	return ret;
+	if (ret)
+		return ret;
+
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	iommu_group_put(group);
+
+	return 0;
 }
 
 static void msm_iommu_remove_device(struct device *dev)
@@ -421,6 +431,8 @@ static void msm_iommu_remove_device(struct device *dev)
 		iommu_device_unlink(&iommu->iommu, dev);
 
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+
+	iommu_group_remove_device(dev);
 }
 
 static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -700,6 +712,7 @@ static struct iommu_ops msm_iommu_ops = {
 	.iova_to_phys = msm_iommu_iova_to_phys,
 	.add_device = msm_iommu_add_device,
 	.remove_device = msm_iommu_remove_device,
+	.device_group = generic_device_group,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 	.of_xlate = qcom_iommu_of_xlate,
 };
-- 
2.12.2.dirty

[PATCH 1/4] iommu/msm: Add iommu_group support

[Sricharan R]

Hi Robin,

> As the last step to making groups mandatory, clean up the remaining
> drivers by adding basic support. Whilst it may not perfectly reflect the
> isolation capabilities of the hardware, using generic_device_group()
> should at least maintain existing behaviour with respect to the API.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index d0448353d501..04f4d51ffacb 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>  static int msm_iommu_add_device(struct device *dev)
>  {
>  	struct msm_iommu_dev *iommu;
> +	struct iommu_group *group;
>  	unsigned long flags;
>  	int ret = 0;
>  
> @@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
>  
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	iommu_group_put(group);
> +
> +	return 0;
>  }
>  

 While this is correct for completing the group support, this adds the default domain and
 that might break in the driver while attaching a private domain. The msm_iomm_attach_dev
 needs to be fixed by calling msm_iommu_detach_dev while trying to attach a new domain when
 already connected to a default one. But let me test and confirm this.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[PATCH 1/4] iommu/msm: Add iommu_group support

[Robin Murphy]

On 24/07/17 08:34, Sricharan R wrote:
> Hi Robin,
> 
>> As the last step to making groups mandatory, clean up the remaining
>> drivers by adding basic support. Whilst it may not perfectly reflect the
>> isolation capabilities of the hardware, using generic_device_group()
>> should at least maintain existing behaviour with respect to the API.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
>> index d0448353d501..04f4d51ffacb 100644
>> --- a/drivers/iommu/msm_iommu.c
>> +++ b/drivers/iommu/msm_iommu.c
>> @@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>>  static int msm_iommu_add_device(struct device *dev)
>>  {
>>  	struct msm_iommu_dev *iommu;
>> +	struct iommu_group *group;
>>  	unsigned long flags;
>>  	int ret = 0;
>>  
>> @@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
>>  
>>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>>  
>> -	return ret;
>> +	if (ret)
>> +		return ret;
>> +
>> +	group = iommu_group_get_for_dev(dev);
>> +	if (IS_ERR(group))
>> +		return PTR_ERR(group);
>> +
>> +	iommu_group_put(group);
>> +
>> +	return 0;
>>  }
>>  
> 
>  While this is correct for completing the group support, this adds the default domain and
>  that might break in the driver while attaching a private domain. The msm_iomm_attach_dev
>  needs to be fixed by calling msm_iommu_detach_dev while trying to attach a new domain when
>  already connected to a default one. But let me test and confirm this.

The default domain shouldn't matter, since msm_iommu_domain_alloc() will
refuse to create DMA ops or identity domains in the first place. In the
absence of a default domain, __iommu_detach_group() will still end up
calling ops->detach_dev, so I don't think the ultimate behaviour is any
different with this change. Testing is of course welcome, though ;)

Robin.

[PATCH 1/4] iommu/msm: Add iommu_group support

[Sricharan R]

Hi Robin,

On 7/24/2017 3:25 PM, Robin Murphy wrote:
> On 24/07/17 08:34, Sricharan R wrote:
>> Hi Robin,
>>
>>> As the last step to making groups mandatory, clean up the remaining
>>> drivers by adding basic support. Whilst it may not perfectly reflect the
>>> isolation capabilities of the hardware, using generic_device_group()
>>> should at least maintain existing behaviour with respect to the API.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>  drivers/iommu/msm_iommu.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
>>> index d0448353d501..04f4d51ffacb 100644
>>> --- a/drivers/iommu/msm_iommu.c
>>> +++ b/drivers/iommu/msm_iommu.c
>>> @@ -393,6 +393,7 @@ static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
>>>  static int msm_iommu_add_device(struct device *dev)
>>>  {
>>>  	struct msm_iommu_dev *iommu;
>>> +	struct iommu_group *group;
>>>  	unsigned long flags;
>>>  	int ret = 0;
>>>  
>>> @@ -406,7 +407,16 @@ static int msm_iommu_add_device(struct device *dev)
>>>  
>>>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>>>  
>>> -	return ret;
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	group = iommu_group_get_for_dev(dev);
>>> +	if (IS_ERR(group))
>>> +		return PTR_ERR(group);
>>> +
>>> +	iommu_group_put(group);
>>> +
>>> +	return 0;
>>>  }
>>>  
>>
>>  While this is correct for completing the group support, this adds the default domain and
>>  that might break in the driver while attaching a private domain. The msm_iomm_attach_dev
>>  needs to be fixed by calling msm_iommu_detach_dev while trying to attach a new domain when
>>  already connected to a default one. But let me test and confirm this.
> 
> The default domain shouldn't matter, since msm_iommu_domain_alloc() will
> refuse to create DMA ops or identity domains in the first place. In the
> absence of a default domain, __iommu_detach_group() will still end up
> calling ops->detach_dev, so I don't think the ultimate behaviour is any
> different with this change. Testing is of course welcome, though ;)

 Ha, you are right. Sorry, overlooked that only DOMAIN_UNMANAGED is supported here.
 Meanwhile, lost access to the board. So would give test feedback once i get it.

 Otherwise, Reviewed-by: sricharan at codeaurora.org

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

[PATCH 2/4] iommu/tegra-smmu: Add iommu_group support

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect
the isolation capabilities of the hardware (tegra_smmu_swgroup sounds
suspiciously like something that might warrant representing at the
iommu_group level), using generic_device_group() should at least
maintain existing behaviour with respect to the API.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-smmu.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index eeb19f560a05..faa9c1e70482 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -704,6 +704,7 @@ static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
 static int tegra_smmu_add_device(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
+	struct iommu_group *group;
 	struct of_phandle_args args;
 	unsigned int index = 0;
 
@@ -725,12 +726,19 @@ static int tegra_smmu_add_device(struct device *dev)
 		index++;
 	}
 
+	group = iommu_group_get_for_dev(dev);
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	iommu_group_put(group);
+
 	return 0;
 }
 
 static void tegra_smmu_remove_device(struct device *dev)
 {
 	dev->archdata.iommu = NULL;
+	iommu_group_remove_device(dev);
 }
 
 static const struct iommu_ops tegra_smmu_ops = {
@@ -741,6 +749,7 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.detach_dev = tegra_smmu_detach_dev,
 	.add_device = tegra_smmu_add_device,
 	.remove_device = tegra_smmu_remove_device,
+	.device_group = generic_device_group,
 	.map = tegra_smmu_map,
 	.unmap = tegra_smmu_unmap,
 	.map_sg = default_iommu_map_sg,
-- 
2.12.2.dirty

[PATCH 2/4] iommu/tegra-smmu: Add iommu_group support

[Mikko Perttunen]

Tested-by: Mikko Perttunen <mperttunen@nvidia.com>

(Jetson TX1 / Tegra210 with Host1x/VIC behind IOMMU)

On 21.07.2017 15:12, Robin Murphy wrote:
> As the last step to making groups mandatory, clean up the remaining
> drivers by adding basic support. Whilst it may not perfectly reflect
> the isolation capabilities of the hardware (tegra_smmu_swgroup sounds
> suspiciously like something that might warrant representing at the
> iommu_group level), using generic_device_group() should at least
> maintain existing behaviour with respect to the API.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/tegra-smmu.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index eeb19f560a05..faa9c1e70482 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -704,6 +704,7 @@ static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
>  static int tegra_smmu_add_device(struct device *dev)
>  {
>  	struct device_node *np = dev->of_node;
> +	struct iommu_group *group;
>  	struct of_phandle_args args;
>  	unsigned int index = 0;
>
> @@ -725,12 +726,19 @@ static int tegra_smmu_add_device(struct device *dev)
>  		index++;
>  	}
>
> +	group = iommu_group_get_for_dev(dev);
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	iommu_group_put(group);
> +
>  	return 0;
>  }
>
>  static void tegra_smmu_remove_device(struct device *dev)
>  {
>  	dev->archdata.iommu = NULL;
> +	iommu_group_remove_device(dev);
>  }
>
>  static const struct iommu_ops tegra_smmu_ops = {
> @@ -741,6 +749,7 @@ static const struct iommu_ops tegra_smmu_ops = {
>  	.detach_dev = tegra_smmu_detach_dev,
>  	.add_device = tegra_smmu_add_device,
>  	.remove_device = tegra_smmu_remove_device,
> +	.device_group = generic_device_group,
>  	.map = tegra_smmu_map,
>  	.unmap = tegra_smmu_unmap,
>  	.map_sg = default_iommu_map_sg,
>

[PATCH 3/4] iommu/tegra-gart: Add iommu_group support

[Robin Murphy]

As the last step to making groups mandatory, clean up the remaining
drivers by adding basic support. Whilst it may not perfectly reflect the
isolation capabilities of the hardware, using generic_device_group()
should at least maintain existing behaviour with respect to the API.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
index 37e708fdbb5a..29bafc6e82ae 100644
--- a/drivers/iommu/tegra-gart.c
+++ b/drivers/iommu/tegra-gart.c
@@ -334,12 +334,31 @@ static bool gart_iommu_capable(enum iommu_cap cap)
 	return false;
 }
 
+static int gart_iommu_add_device(struct device *dev)
+{
+	struct iommu_group *group = iommu_group_get_for_dev(dev);
+
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	iommu_group_put(group);
+	return 0;
+}
+
+static void gart_iommu_remove_device(struct device *dev)
+{
+	iommu_group_remove_device(dev);
+}
+
 static const struct iommu_ops gart_iommu_ops = {
 	.capable	= gart_iommu_capable,
 	.domain_alloc	= gart_iommu_domain_alloc,
 	.domain_free	= gart_iommu_domain_free,
 	.attach_dev	= gart_iommu_attach_dev,
 	.detach_dev	= gart_iommu_detach_dev,
+	.add_device	= gart_iommu_add_device,
+	.remove_device	= gart_iommu_remove_device,
+	.device_group	= generic_device_group,
 	.map		= gart_iommu_map,
 	.map_sg		= default_iommu_map_sg,
 	.unmap		= gart_iommu_unmap,
-- 
2.12.2.dirty

[PATCH 3/4] iommu/tegra-gart: Add iommu_group support

[Dmitry Osipenko]

On 21.07.2017 15:12, Robin Murphy wrote:
> As the last step to making groups mandatory, clean up the remaining
> drivers by adding basic support. Whilst it may not perfectly reflect the
> isolation capabilities of the hardware, using generic_device_group()
> should at least maintain existing behaviour with respect to the API.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

Tested-by: Dmitry Osipenko <digetx@gmail.com>

-- 
Dmitry

[PATCH 4/4] iommu: Finish making iommu_group support mandatory

[Robin Murphy]

Now that all the drivers properly implementing the IOMMU API support
groups (I'm ignoring the etnaviv GPU MMUs which seemingly only do just
enough to convince the ARM DMA mapping ops), we can remove the FIXME
workarounds from the core code. In the process, it also seems logical to
make the .device_group callback non-optional for drivers calling
iommu_group_get_for_dev() - the current callers all implement it anyway,
and it doesn't make sense for any future callers not to either.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/iommu.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3f6ea160afed..af69bf7e035a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1005,11 +1005,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	if (group)
 		return group;
 
-	group = ERR_PTR(-EINVAL);
-
-	if (ops && ops->device_group)
-		group = ops->device_group(dev);
+	if (!ops)
+		return ERR_PTR(-EINVAL);
 
+	group = ops->device_group(dev);
 	if (WARN_ON_ONCE(group == NULL))
 		return ERR_PTR(-EINVAL);
 
@@ -1298,12 +1297,8 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 	int ret;
 
 	group = iommu_group_get(dev);
-	/* FIXME: Remove this when groups a mandatory for iommu drivers */
-	if (group == NULL)
-		return __iommu_attach_device(domain, dev);
-
 	/*
-	 * We have a group - lock it to make sure the device-count doesn't
+	 * Lock the group to make sure the device-count doesn't
 	 * change while we are attaching
 	 */
 	mutex_lock(&group->mutex);
@@ -1336,9 +1331,6 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 	struct iommu_group *group;
 
 	group = iommu_group_get(dev);
-	/* FIXME: Remove this when groups a mandatory for iommu drivers */
-	if (group == NULL)
-		return __iommu_detach_device(domain, dev);
 
 	mutex_lock(&group->mutex);
 	if (iommu_group_device_count(group) != 1) {
@@ -1360,9 +1352,6 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
 	struct iommu_group *group;
 
 	group = iommu_group_get(dev);
-	/* FIXME: Remove this when groups a mandatory for iommu drivers */
-	if (group == NULL)
-		return NULL;
 
 	domain = group->domain;
 
-- 
2.12.2.dirty

[PATCH 0/4] IOMMU group cleanup

[Joerg Roedel]

Hi Robin,

On Fri, Jul 21, 2017 at 01:12:34PM +0100, Robin Murphy wrote:
> Robin Murphy (4):
>   iommu/msm: Add iommu_group support
>   iommu/tegra-smmu: Add iommu_group support
>   iommu/tegra-gart: Add iommu_group support
>   iommu: Finish making iommu_group support mandatory
> 
>  drivers/iommu/iommu.c      | 19 ++++---------------
>  drivers/iommu/msm_iommu.c  | 15 ++++++++++++++-
>  drivers/iommu/tegra-gart.c | 19 +++++++++++++++++++
>  drivers/iommu/tegra-smmu.c |  9 +++++++++
>  4 files changed, 46 insertions(+), 16 deletions(-)

Thanks for this series, much appreciated! I'll wait for the testing
results and apply them when all looks good.


Thanks,

	Joerg

[PATCH 0/4] IOMMU group cleanup

[Joerg Roedel]

On Fri, Jul 21, 2017 at 01:12:34PM +0100, Robin Murphy wrote:
> Hi all,
> 
> Prompted by some discussion around the questionable usefulness of
> iommu_present() for SoC topologies, I took a look at what was left to
> finish off the intention of making groups mandatory. Turns out it
> wasn't much, so here it is. As well as the general rationalisation
> aspect, between this and the probe-deferral changes in 4.12, we should
> now be able to rely on the presence of a group unambiguously indicating
> whether a given device is associated with an IOMMU or not.
> 
> Robin.
> 
> Robin Murphy (4):
>   iommu/msm: Add iommu_group support
>   iommu/tegra-smmu: Add iommu_group support
>   iommu/tegra-gart: Add iommu_group support
>   iommu: Finish making iommu_group support mandatory

Applied, thanks Robin.