[PATCH v2 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'

[PATCH v2 0/3] omap: iovmm: Fix IOVMM check for fixed 'da'

[David Cohen]

IOVMM driver checks input 'da == 0' when mapping address to determine whether
user wants fixed 'da' or not. At the same time, it doesn't disallow address
0x0 to be used, what creates an ambiguous situation. This patch set moves
fixed 'da' check to the input flags.
It also fixes da_start value for ISP IOMMU instance.

Br,

David Cohen
---

David Cohen (2):
  omap3: change ISP's IOMMU da_start address
  omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag

Michael Jones (1):
  omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set

 arch/arm/mach-omap2/omap-iommu.c        |    2 +-
 arch/arm/plat-omap/include/plat/iovmm.h |    2 --
 arch/arm/plat-omap/iovmm.c              |   30 +++++++++++++++---------------
 3 files changed, 16 insertions(+), 18 deletions(-)

[PATCH v2 1/3] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set

[David Cohen]

From: Michael Jones <michael.jones@matrix-vision.de>

commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
address if da_start==0, which would then not get unmapped. Disallow
this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
correctly.

Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
---
 arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 6dc1296..e5f8341 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 					   size_t bytes, u32 flags)
 {
 	struct iovm_struct *new, *tmp;
-	u32 start, prev_end, alignement;
+	u32 start, prev_end, alignment;
 
 	if (!obj || !bytes)
 		return ERR_PTR(-EINVAL);
 
 	start = da;
-	alignement = PAGE_SIZE;
+	alignment = PAGE_SIZE;
 
 	if (flags & IOVMF_DA_ANON) {
-		start = obj->da_start;
+		/* Don't map address 0 */
+		if (obj->da_start)
+			start = obj->da_start;
+		else
+			start = alignment;
 
 		if (flags & IOVMF_LINEAR)
-			alignement = iopgsz_max(bytes);
-		start = roundup(start, alignement);
+			alignment = iopgsz_max(bytes);
+		start = roundup(start, alignment);
 	} else if (start < obj->da_start || start > obj->da_end ||
 					obj->da_end - start < bytes) {
 		return ERR_PTR(-EINVAL);
@@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 			goto found;
 
 		if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
-			start = roundup(tmp->da_end + 1, alignement);
+			start = roundup(tmp->da_end + 1, alignment);
 
 		prev_end = tmp->da_end;
 	}
-- 
1.7.0.4

[PATCH v2 2/3] omap3: change ISP's IOMMU da_start address

[David Cohen]

ISP doesn't consider 0x0 as a valid address, so it should explicitly
exclude first page from allowed 'da' range.

Signed-off-by: David Cohen <dacohen@gmail.com>
---
 arch/arm/mach-omap2/omap-iommu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index 3fc5dc7..3bea489 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
 			.name = "isp",
 			.nr_tlb_entries = 8,
 			.clk_name = "cam_ick",
-			.da_start = 0x0,
+			.da_start = 0x1000,
 			.da_end = 0xFFFFF000,
 		},
 	},
-- 
1.7.0.4

[PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag

[David Cohen]

Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
to input 'da' address when mapping memory:
da == 0: IOVMF_DA_ANON
da != 0: IOVMF_DA_FIXED

It prevents IOMMU to map first page with fixed 'da'. To avoid such
issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
come from the user throught 'flags' parameter when mapping memory.
As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
can be removed. The driver will now check internally if IOVMF_DA_FIXED
is set or not.

Signed-off-by: David Cohen <dacohen@gmail.com>
---
 arch/arm/plat-omap/include/plat/iovmm.h |    2 --
 arch/arm/plat-omap/iovmm.c              |   14 +++++---------
 2 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/iovmm.h b/arch/arm/plat-omap/include/plat/iovmm.h
index bdc7ce5..32a2f6c 100644
--- a/arch/arm/plat-omap/include/plat/iovmm.h
+++ b/arch/arm/plat-omap/include/plat/iovmm.h
@@ -71,8 +71,6 @@ struct iovm_struct {
 #define IOVMF_LINEAR_MASK	(3 << (2 + IOVMF_SW_SHIFT))
 
 #define IOVMF_DA_FIXED		(1 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_ANON		(2 << (4 + IOVMF_SW_SHIFT))
-#define IOVMF_DA_MASK		(3 << (4 + IOVMF_SW_SHIFT))
 
 
 extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index e5f8341..894489c 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 	start = da;
 	alignment = PAGE_SIZE;
 
-	if (flags & IOVMF_DA_ANON) {
+	if (~flags & IOVMF_DA_FIXED) {
 		/* Don't map address 0 */
 		if (obj->da_start)
 			start = obj->da_start;
@@ -307,7 +307,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 		if (tmp->da_start > start && (tmp->da_start - start) >= bytes)
 			goto found;
 
-		if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
+		if (tmp->da_end >= start && ~flags & IOVMF_DA_FIXED)
 			start = roundup(tmp->da_end + 1, alignment);
 
 		prev_end = tmp->da_end;
@@ -654,7 +654,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_DISCONT;
 	flags |= IOVMF_MMIO;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
 
 	da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
@@ -694,7 +693,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
  * @flags:	iovma and page property
  *
  * Allocate @bytes linearly and creates 1-n-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
  */
 u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 {
@@ -713,7 +712,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_DISCONT;
 	flags |= IOVMF_ALLOC;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
 
 	sgt = sgtable_alloc(bytes, flags, da, 0);
 	if (IS_ERR(sgt)) {
@@ -784,7 +782,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
  * @flags:	iovma and page property
  *
  * Creates 1-1-1 mapping and returns @da again, which can be
- * adjusted if 'IOVMF_DA_ANON' is set.
+ * adjusted if 'IOVMF_DA_FIXED' is not set.
  */
 u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
 		 u32 flags)
@@ -803,7 +801,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_LINEAR;
 	flags |= IOVMF_MMIO;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
 
 	da = __iommu_kmap(obj, da, pa, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
@@ -842,7 +839,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
  * @flags:	iovma and page property
  *
  * Allocate @bytes linearly and creates 1-1-1 mapping and returns
- * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
+ * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
  */
 u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 {
@@ -862,7 +859,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 	flags &= IOVMF_HW_MASK;
 	flags |= IOVMF_LINEAR;
 	flags |= IOVMF_ALLOC;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
 
 	da = __iommu_kmap(obj, da, pa, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
-- 
1.7.0.4

Re: [PATCH v2 1/3] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set

[Guzman Lugo, Fernando]

On Tue, Mar 8, 2011 at 2:15 PM, David Cohen <dacohen@gmail.com> wrote:
> From: Michael Jones <michael.jones@matrix-vision.de>
>
> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
> address if da_start==0, which would then not get unmapped. Disallow
> this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
> correctly.
>
> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
> ---
>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 6dc1296..e5f8341 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>                                           size_t bytes, u32 flags)
>  {
>        struct iovm_struct *new, *tmp;
> -       u32 start, prev_end, alignement;
> +       u32 start, prev_end, alignment;
>
>        if (!obj || !bytes)
>                return ERR_PTR(-EINVAL);
>
>        start = da;
> -       alignement = PAGE_SIZE;
> +       alignment = PAGE_SIZE;
>
>        if (flags & IOVMF_DA_ANON) {
> -               start = obj->da_start;
> +               /* Don't map address 0 */
> +               if (obj->da_start)
> +                       start = obj->da_start;
> +               else
> +                       start = alignment;

looks good to me, just a nitpick comment, that would look better

                  start = (obj->da_start) ? obj->da_start : alignment;

Regards,
Fernando.

>
>                if (flags & IOVMF_LINEAR)
> -                       alignement = iopgsz_max(bytes);
> -               start = roundup(start, alignement);
> +                       alignment = iopgsz_max(bytes);
> +               start = roundup(start, alignment);
>        } else if (start < obj->da_start || start > obj->da_end ||
>                                        obj->da_end - start < bytes) {
>                return ERR_PTR(-EINVAL);
> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>                        goto found;
>
>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
> -                       start = roundup(tmp->da_end + 1, alignement);
> +                       start = roundup(tmp->da_end + 1, alignment);
>
>                prev_end = tmp->da_end;
>        }
> --
> 1.7.0.4
>
>

Re: [PATCH v2 3/3] omap: iovmm: don't check 'da' to set IOVMF_DA_FIXED flag

[Guzman Lugo, Fernando]

On Tue, Mar 8, 2011 at 2:15 PM, David Cohen <dacohen@gmail.com> wrote:
> Currently IOVMM driver sets IOVMF_DA_FIXED/IOVMF_DA_ANON flags according
> to input 'da' address when mapping memory:
> da == 0: IOVMF_DA_ANON
> da != 0: IOVMF_DA_FIXED
>
> It prevents IOMMU to map first page with fixed 'da'. To avoid such
> issue, IOVMM will not automatically set IOVMF_DA_FIXED. It should now
> come from the user throught 'flags' parameter when mapping memory.
> As IOVMF_DA_ANON and IOVMF_DA_FIXED are mutually exclusive, IOVMF_DA_ANON
> can be removed. The driver will now check internally if IOVMF_DA_FIXED
> is set or not.

Looks good to me.

Regards,
Fernando.

>
> Signed-off-by: David Cohen <dacohen@gmail.com>
> ---
>  arch/arm/plat-omap/include/plat/iovmm.h |    2 --
>  arch/arm/plat-omap/iovmm.c              |   14 +++++---------
>  2 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/iovmm.h b/arch/arm/plat-omap/include/plat/iovmm.h
> index bdc7ce5..32a2f6c 100644
> --- a/arch/arm/plat-omap/include/plat/iovmm.h
> +++ b/arch/arm/plat-omap/include/plat/iovmm.h
> @@ -71,8 +71,6 @@ struct iovm_struct {
>  #define IOVMF_LINEAR_MASK      (3 << (2 + IOVMF_SW_SHIFT))
>
>  #define IOVMF_DA_FIXED         (1 << (4 + IOVMF_SW_SHIFT))
> -#define IOVMF_DA_ANON          (2 << (4 + IOVMF_SW_SHIFT))
> -#define IOVMF_DA_MASK          (3 << (4 + IOVMF_SW_SHIFT))
>
>
>  extern struct iovm_struct *find_iovm_area(struct iommu *obj, u32 da);
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index e5f8341..894489c 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -279,7 +279,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>        start = da;
>        alignment = PAGE_SIZE;
>
> -       if (flags & IOVMF_DA_ANON) {
> +       if (~flags & IOVMF_DA_FIXED) {
>                /* Don't map address 0 */
>                if (obj->da_start)
>                        start = obj->da_start;
> @@ -307,7 +307,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>                if (tmp->da_start > start && (tmp->da_start - start) >= bytes)
>                        goto found;
>
> -               if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
> +               if (tmp->da_end >= start && ~flags & IOVMF_DA_FIXED)
>                        start = roundup(tmp->da_end + 1, alignment);
>
>                prev_end = tmp->da_end;
> @@ -654,7 +654,6 @@ u32 iommu_vmap(struct iommu *obj, u32 da, const struct sg_table *sgt,
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_DISCONT;
>        flags |= IOVMF_MMIO;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>
>        da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> @@ -694,7 +693,7 @@ EXPORT_SYMBOL_GPL(iommu_vunmap);
>  * @flags:     iovma and page property
>  *
>  * Allocate @bytes linearly and creates 1-n-1 mapping and returns
> - * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
> + * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
>  */
>  u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>  {
> @@ -713,7 +712,6 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_DISCONT;
>        flags |= IOVMF_ALLOC;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>
>        sgt = sgtable_alloc(bytes, flags, da, 0);
>        if (IS_ERR(sgt)) {
> @@ -784,7 +782,7 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
>  * @flags:     iovma and page property
>  *
>  * Creates 1-1-1 mapping and returns @da again, which can be
> - * adjusted if 'IOVMF_DA_ANON' is set.
> + * adjusted if 'IOVMF_DA_FIXED' is not set.
>  */
>  u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
>                 u32 flags)
> @@ -803,7 +801,6 @@ u32 iommu_kmap(struct iommu *obj, u32 da, u32 pa, size_t bytes,
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_LINEAR;
>        flags |= IOVMF_MMIO;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>
>        da = __iommu_kmap(obj, da, pa, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> @@ -842,7 +839,7 @@ EXPORT_SYMBOL_GPL(iommu_kunmap);
>  * @flags:     iovma and page property
>  *
>  * Allocate @bytes linearly and creates 1-1-1 mapping and returns
> - * @da again, which might be adjusted if 'IOVMF_DA_ANON' is set.
> + * @da again, which might be adjusted if 'IOVMF_DA_FIXED' is not set.
>  */
>  u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>  {
> @@ -862,7 +859,6 @@ u32 iommu_kmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
>        flags &= IOVMF_HW_MASK;
>        flags |= IOVMF_LINEAR;
>        flags |= IOVMF_ALLOC;
> -       flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
>
>        da = __iommu_kmap(obj, da, pa, va, bytes, flags);
>        if (IS_ERR_VALUE(da))
> --
> 1.7.0.4
>
>

Re: [PATCH v2 2/3] omap3: change ISP's IOMMU da_start address

[Sakari Ailus]

David Cohen wrote:
> ISP doesn't consider 0x0 as a valid address, so it should explicitly
> exclude first page from allowed 'da' range.
> 
> Signed-off-by: David Cohen <dacohen@gmail.com>
> ---
>  arch/arm/mach-omap2/omap-iommu.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index 3fc5dc7..3bea489 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>  			.name = "isp",
>  			.nr_tlb_entries = 8,
>  			.clk_name = "cam_ick",
> -			.da_start = 0x0,
> +			.da_start = 0x1000,
>  			.da_end = 0xFFFFF000,
>  		},
>  	},

Hi David!

Thanks for the patch.

My question is once again: is this necessary? My understanding is that
the IOMMU allows mapping the NULL address if the user wishes to map it
explicitly. da_end specifies the real hardware limit for the mapped top
address, da_start should do the same for bottom.

I think that the IOMMU users should be either able to rely that they get
no NULL allocated automatically for them. Do we want or not want it to
be part of the API? I don't think the ISP driver is a special case of
all the possible drivers using the IOMMU.

On the other hand, probably there will be an API change at some point
for the IOMMU since as far as I remember, there are somewhat
established APIs for IOMMUs in existence.

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v2 1/3] omap: iovmm: disallow mapping NULL address when IOVMF_DA_ANON is set

[Michael Jones]

On 03/08/2011 09:31 PM, Guzman Lugo, Fernando wrote:
> On Tue, Mar 8, 2011 at 2:15 PM, David Cohen <dacohen@gmail.com> wrote:
>> From: Michael Jones <michael.jones@matrix-vision.de>
>>
>> commit c7f4ab26e3bcdaeb3e19ec658e3ad9092f1a6ceb allowed mapping the NULL
>> address if da_start==0, which would then not get unmapped. Disallow
>> this again if IOVMF_DA_ANON is set. And spell variable 'alignment'
>> correctly.
>>
>> Signed-off-by: Michael Jones <michael.jones@matrix-vision.de>
>> ---
>>  arch/arm/plat-omap/iovmm.c |   16 ++++++++++------
>>  1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
>> index 6dc1296..e5f8341 100644
>> --- a/arch/arm/plat-omap/iovmm.c
>> +++ b/arch/arm/plat-omap/iovmm.c
>> @@ -271,20 +271,24 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>                                           size_t bytes, u32 flags)
>>  {
>>        struct iovm_struct *new, *tmp;
>> -       u32 start, prev_end, alignement;
>> +       u32 start, prev_end, alignment;
>>
>>        if (!obj || !bytes)
>>                return ERR_PTR(-EINVAL);
>>
>>        start = da;
>> -       alignement = PAGE_SIZE;
>> +       alignment = PAGE_SIZE;
>>
>>        if (flags & IOVMF_DA_ANON) {
>> -               start = obj->da_start;
>> +               /* Don't map address 0 */
>> +               if (obj->da_start)
>> +                       start = obj->da_start;
>> +               else
>> +                       start = alignment;
> 
> looks good to me, just a nitpick comment, that would look better
> 
>                   start = (obj->da_start) ? obj->da_start : alignment;
> 
> Regards,
> Fernando.
> 
>>
>>                if (flags & IOVMF_LINEAR)
>> -                       alignement = iopgsz_max(bytes);
>> -               start = roundup(start, alignement);
>> +                       alignment = iopgsz_max(bytes);
>> +               start = roundup(start, alignment);
>>        } else if (start < obj->da_start || start > obj->da_end ||
>>                                        obj->da_end - start < bytes) {
>>                return ERR_PTR(-EINVAL);
>> @@ -304,7 +308,7 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>>                        goto found;
>>
>>                if (tmp->da_end >= start && flags & IOVMF_DA_ANON)
>> -                       start = roundup(tmp->da_end + 1, alignement);
>> +                       start = roundup(tmp->da_end + 1, alignment);
>>
>>                prev_end = tmp->da_end;
>>        }
>> --
>> 1.7.0.4
>>

Hi David,

These changes to my patch are fine with me.  If you want to incorporate
Fernando's recommendation above, too, go ahead.

-Michael

MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [PATCH v2 2/3] omap3: change ISP's IOMMU da_start address

[David Cohen]

On Wed, Mar 9, 2011 at 9:43 AM, Sakari Ailus
<sakari.ailus@maxwell.research.nokia.com> wrote:
> David Cohen wrote:
>> ISP doesn't consider 0x0 as a valid address, so it should explicitly
>> exclude first page from allowed 'da' range.
>>
>> Signed-off-by: David Cohen <dacohen@gmail.com>
>> ---
>>  arch/arm/mach-omap2/omap-iommu.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
>> index 3fc5dc7..3bea489 100644
>> --- a/arch/arm/mach-omap2/omap-iommu.c
>> +++ b/arch/arm/mach-omap2/omap-iommu.c
>> @@ -33,7 +33,7 @@ static struct iommu_device omap3_devices[] = {
>>                       .name = "isp",
>>                       .nr_tlb_entries = 8,
>>                       .clk_name = "cam_ick",
>> -                     .da_start = 0x0,
>> +                     .da_start = 0x1000,
>>                       .da_end = 0xFFFFF000,
>>               },
>>       },
>
> Hi David!

Hi Sakari,

>
> Thanks for the patch.

And thanks for the comments. :)

>
> My question is once again: is this necessary? My understanding is that
> the IOMMU allows mapping the NULL address if the user wishes to map it
> explicitly. da_end specifies the real hardware limit for the mapped top
> address, da_start should do the same for bottom.

Hm. da_start/da_end in this case belong to OMAP3 IOMMU ISP VM. It
means they're related to OMAP3 ISP only. But they do not reflect the
hw limit, as the range limit is anything which fits in u32. They say
what's the range OMAP3 ISP is expecting to have mapped. And the answer
to this question is the first page is not expected. Then, why say the
opposite in da_start?

>
> I think that the IOMMU users should be either able to rely that they get
> no NULL allocated automatically for them. Do we want or not want it to
> be part of the API? I don't think the ISP driver is a special case of
> all the possible drivers using the IOMMU.

My understanding after this discussion is address 0x0 should be
allowed (what is done by patch 3/3 of this set). But as a safer
choice, it won't be returned without explicitly request (what is done
in path 1/3). Because of that, I'm OK in drop this patch 2/3. But then
there's one question which is the motivation for this change:
If the current OMAP3 ISP driver doesn't want the first page, (1)
should we pass a generic information and expect IOVMM driver to
correct it to ISP need or (2) should we pass the information which
reflects the real ISP driver's need?
My understanding is (2). But I'm fine with (1) as it will lead to the
same result.

>
> On the other hand, probably there will be an API change at some point
> for the IOMMU since as far as I remember, there are somewhat
> established APIs for IOMMUs in existence.

At some point we need to find a standard for many IOMMU drivers. But
for now we need to stick with what we have. :)

Regards,

David Cohen