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

Message ID b87d44e09fa18efbb9daf6258c1f8ecbedaac23c.1500637633.git.robin.murphy@arm.com
State Accepted
Headers show

Commit Message

Robin Murphy July 21, 2017, 12:12 p.m.
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(-)

Comments

Sricharan R July 24, 2017, 7:34 a.m. | #1
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
Robin Murphy July 24, 2017, 9:55 a.m. | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sricharan R July 26, 2017, 3:29 p.m. | #3
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@codeaurora.org

Regards,
 Sricharan

Patch

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,
 };