[PATCHv2 1/2] iommu/exynos: add devices attached to the System MMU to an IOMMU group

[PATCHv2 1/2] iommu/exynos: add devices attached to the System MMU to an IOMMU group

[Antonios Motakis]

IOMMU groups are expected by certain users of the IOMMU API,
e.g. VFIO. Since each device is behind its own System MMU, we
can allocate a new IOMMU group for each device.

This patch depends on Cho KyongHo's patch series titled "[PATCH v7 00/12]
iommu/exynos: Fixes and Enhancements of System MMU driver with DT",
applied on a Linux 3.10.1 kernel. It has been tested on the Arndale board.

Changes since in v2:
- Removed possibility for minor memory leak in case of
  misbehaving platform drivers

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/iommu/exynos-iommu.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 51d43bb..c7dd4b5 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1134,6 +1134,32 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain,
 	return phys;
 }
 
+static int exynos_iommu_add_device(struct device *dev)
+{
+	struct iommu_group *group;
+	int ret;
+
+	group = iommu_group_get(dev);
+
+	if (!group) {
+		group = iommu_group_alloc();
+		if (IS_ERR(group)) {
+			dev_err(dev, "Failed to allocate IOMMU group\n");
+			return PTR_ERR(group);
+		}
+	}
+
+	ret = iommu_group_add_device(group, dev);
+	iommu_group_put(group);
+
+	return ret;
+}
+
+static void exynos_iommu_remove_device(struct device *dev)
+{
+	iommu_group_remove_device(dev);
+}
+
 static struct iommu_ops exynos_iommu_ops = {
 	.domain_init = &exynos_iommu_domain_init,
 	.domain_destroy = &exynos_iommu_domain_destroy,
@@ -1142,6 +1168,8 @@ static struct iommu_ops exynos_iommu_ops = {
 	.map = &exynos_iommu_map,
 	.unmap = &exynos_iommu_unmap,
 	.iova_to_phys = &exynos_iommu_iova_to_phys,
+	.add_device	= exynos_iommu_add_device,
+	.remove_device	= exynos_iommu_remove_device,
 	.pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
 };
 
-- 
1.8.1.2

[PATCH 2/2] iommu/exynos: Follow kernel coding style for __sysmmu_enable return type

[Antonios Motakis]

On success, the __sysmmu_enable returns 1 instead of 0, which does not
respect the convention described in Chapter 16 of the Linux kernel coding
style.

In fact, this return value is propagated all the way up to
iommu_attach_device() and iommu_attach_device() in drivers/iommu.c,
which results into inconsistent behavior of the IOMMU API with Exynos
systems, compared to other IOMMUs.

This patch replaces the return value with 0, which makes the Exynos'
IOMMU driver behavior consistent with that of other IOMMUs.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
---
 drivers/iommu/exynos-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index c7dd4b5..4ea3abb 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -504,7 +504,7 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data,
 
 		dev_dbg(data->sysmmu, "Enabled\n");
 	} else {
-		ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
+		ret = (pgtable == data->pgtable) ? 0 : -EBUSY;
 
 		dev_dbg(data->sysmmu, "already enabled\n");
 	}
-- 
1.8.1.2

RE: [PATCH 2/2] iommu/exynos: Follow kernel coding style for __sysmmu_enable return type

[Cho KyongHo]

> -----Original Message-----
> From: Antonios Motakis [mailto:a.motakis@virtualopensystems.com]
> Sent: Friday, July 26, 2013 12:35 AM
> 
> On success, the __sysmmu_enable returns 1 instead of 0, which does not
> respect the convention described in Chapter 16 of the Linux kernel coding
> style.
> 
> In fact, this return value is propagated all the way up to
> iommu_attach_device() and iommu_attach_device() in drivers/iommu.c,
> which results into inconsistent behavior of the IOMMU API with Exynos
> systems, compared to other IOMMUs.
> 
> This patch replaces the return value with 0, which makes the Exynos'
> IOMMU driver behavior consistent with that of other IOMMUs.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/iommu/exynos-iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index c7dd4b5..4ea3abb 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -504,7 +504,7 @@ static int __sysmmu_enable(struct sysmmu_drvdata *data,
> 
>  		dev_dbg(data->sysmmu, "Enabled\n");
>  	} else {
> -		ret = (pgtable == data->pgtable) ? 1 : -EBUSY;
> +		ret = (pgtable == data->pgtable) ? 0 : -EBUSY;
> 
Ok.

__sysmmu_enable() must return 1 if it is called with the same page table.
I have fixed it exynos_iommu_attach_device() to always return zero on success
in the next patchset which I will post today.

Thank you.

>  		dev_dbg(data->sysmmu, "already enabled\n");
>  	}
> --
> 1.8.1.2

Re: [PATCH 2/2] iommu/exynos: Follow kernel coding style for __sysmmu_enable return type

['Joerg Roedel']

KyongHo,

On Fri, Jul 26, 2013 at 07:46:01PM +0900, Cho KyongHo wrote:
> __sysmmu_enable() must return 1 if it is called with the same page table.
> I have fixed it exynos_iommu_attach_device() to always return zero on success
> in the next patchset which I will post today.
> 
> Thank you.

When you are fine with these patches please put them on-top of your
patch-set when you re-submit. I will take them from there then.


	Joerg

Re: [PATCH 2/2] iommu/exynos: Follow kernel coding style for __sysmmu_enable return type

[Cho KyongHo]

On Wed, 14 Aug 2013 15:15:49 +0200, 'Joerg Roedel' wrote:
> KyongHo,
> 
> On Fri, Jul 26, 2013 at 07:46:01PM +0900, Cho KyongHo wrote:
> > __sysmmu_enable() must return 1 if it is called with the same page table.
> > I have fixed it exynos_iommu_attach_device() to always return zero on success
> > in the next patchset which I will post today.
> > 
> > Thank you.
> 
> When you are fine with these patches please put them on-top of your
> patch-set when you re-submit. I will take them from there then.
> 

Ok.

Antonios,

Would you mind
if I submit your 2 patches on top of my patches with your signed-off?

KyongHo.

> 
> 	Joerg
> 
> 

Re: [PATCH 2/2] iommu/exynos: Follow kernel coding style for __sysmmu_enable return type

[Antonios Motakis]

Yes, of course, I have no objections.

On Fri, Aug 16, 2013 at 1:21 PM, Cho KyongHo <pullip.cho@samsung.com> wrote:
> On Wed, 14 Aug 2013 15:15:49 +0200, 'Joerg Roedel' wrote:
>> KyongHo,
>>
>> On Fri, Jul 26, 2013 at 07:46:01PM +0900, Cho KyongHo wrote:
>> > __sysmmu_enable() must return 1 if it is called with the same page table.
>> > I have fixed it exynos_iommu_attach_device() to always return zero on success
>> > in the next patchset which I will post today.
>> >
>> > Thank you.
>>
>> When you are fine with these patches please put them on-top of your
>> patch-set when you re-submit. I will take them from there then.
>>
>
> Ok.
>
> Antonios,
>
> Would you mind
> if I submit your 2 patches on top of my patches with your signed-off?
>
> KyongHo.
>
>>
>>       Joerg
>>
>>

Re: [PATCHv2 1/2] iommu/exynos: add devices attached to the System MMU to an IOMMU group

[Sachin Kamat]

Hi Antonios,

On 25 July 2013 21:04, Antonios Motakis
<a.motakis@virtualopensystems.com> wrote:
> IOMMU groups are expected by certain users of the IOMMU API,
> e.g. VFIO. Since each device is behind its own System MMU, we
> can allocate a new IOMMU group for each device.
>
> This patch depends on Cho KyongHo's patch series titled "[PATCH v7 00/12]
> iommu/exynos: Fixes and Enhancements of System MMU driver with DT",
> applied on a Linux 3.10.1 kernel.

This kind of meta information should go after the "---" line below.

It has been tested on the Arndale board.
>
> Changes since in v2:
> - Removed possibility for minor memory leak in case of
>   misbehaving platform drivers
>
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> ---
>  drivers/iommu/exynos-iommu.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 51d43bb..c7dd4b5 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -1134,6 +1134,32 @@ static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain,
>         return phys;
>  }
>
> +static int exynos_iommu_add_device(struct device *dev)
> +{
> +       struct iommu_group *group;
> +       int ret;
> +
> +       group = iommu_group_get(dev);
> +
> +       if (!group) {
> +               group = iommu_group_alloc();
> +               if (IS_ERR(group)) {
> +                       dev_err(dev, "Failed to allocate IOMMU group\n");
> +                       return PTR_ERR(group);
> +               }
> +       }
> +
> +       ret = iommu_group_add_device(group, dev);
> +       iommu_group_put(group);
> +
> +       return ret;
> +}
> +
> +static void exynos_iommu_remove_device(struct device *dev)
> +{
> +       iommu_group_remove_device(dev);
> +}
> +
>  static struct iommu_ops exynos_iommu_ops = {
>         .domain_init = &exynos_iommu_domain_init,
>         .domain_destroy = &exynos_iommu_domain_destroy,
> @@ -1142,6 +1168,8 @@ static struct iommu_ops exynos_iommu_ops = {
>         .map = &exynos_iommu_map,
>         .unmap = &exynos_iommu_unmap,
>         .iova_to_phys = &exynos_iommu_iova_to_phys,
> +       .add_device     = exynos_iommu_add_device,
> +       .remove_device  = exynos_iommu_remove_device,
>         .pgsize_bitmap = SECT_SIZE | LPAGE_SIZE | SPAGE_SIZE,
>  };
>
> --
> 1.8.1.2
>



-- 
With warm regards,
Sachin