[PATCH 0/4] iovmm: fixes for iovmm module

[PATCH 0/4] iovmm: fixes for iovmm module

[Fernando Guzman Lugo]

This patches are needed in order to tidspbridge can
use iovmm with no issues.

Fernando Guzman Lugo (4):
  iommu: remove CONFIG_MPU_BRIDGE_IOMMU
  iovmm: fix roundup for next area and end check for the last area
  iovmm: add superpages support to fixed da address
  iovmm: replace __iounmap with omap_iounmap

 arch/arm/mach-omap2/omap-iommu.c |    2 -
 arch/arm/plat-omap/iovmm.c       |   69 ++++++++++++++++++++++---------------
 2 files changed, 41 insertions(+), 30 deletions(-)
 mode change 100644 => 100755 arch/arm/plat-omap/iovmm.c

[PATCH 1/4] iommu: remove CONFIG_MPU_BRIDGE_IOMMU

[Fernando Guzman Lugo]

remove CONFIG_MPU_BRIDGE_IOMMU in order to create iommu_device
for iva2.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/mach-omap2/omap-iommu.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index f5a1aad..bb8c01d 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -35,7 +35,6 @@ static struct iommu_device omap3_devices[] = {
 			.clk_name = "cam_ick",
 		},
 	},
-#if defined(CONFIG_MPU_BRIDGE_IOMMU)
 	{
 		.base = 0x5d000000,
 		.irq = 28,
@@ -45,7 +44,6 @@ static struct iommu_device omap3_devices[] = {
 			.clk_name = "iva2_ck",
 		},
 	},
-#endif
 };
 #define NR_OMAP3_IOMMU_DEVICES ARRAY_SIZE(omap3_devices)
 static struct platform_device *omap3_iommu_pdev[NR_OMAP3_IOMMU_DEVICES];
-- 
1.6.3.3

[PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[Fernando Guzman Lugo]

As da_end does not belongs to the area the roundup should
be done to da_end and not to da_end + 1.
Also the end check for the last area should be
ULONG_MAX - start + 1 >= bytes.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/plat-omap/iovmm.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index 24ca9c4..fc6b109 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -289,19 +289,19 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
 	prev_end = 0;
 	list_for_each_entry(tmp, &obj->mmap, list) {
 
-		if (prev_end >= start)
+		if (prev_end > start)
 			break;
 
 		if (start + bytes <= tmp->da_start)
 			goto found;
 
 		if (flags & IOVMF_DA_ANON)
-			start = roundup(tmp->da_end + 1, alignement);
+			start = roundup(tmp->da_end, alignement);
 
 		prev_end = tmp->da_end;
 	}
 
-	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
+	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
 		goto found;
 
 	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
-- 
1.6.3.3

[PATCH 3/4] iovmm: add superpages support to fixed da address

[Fernando Guzman Lugo]

This patch adds superpages support to fixed ad address
inside iommu_kmap function.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/plat-omap/iovmm.c |   61 ++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
index fc6b109..e7ffec8 100644
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -87,27 +87,37 @@ static size_t sgtable_len(const struct sg_table *sgt)
 }
 #define sgtable_ok(x)	(!!sgtable_len(x))
 
+
+static unsigned max_alignment(u32 addr)
+{
+	int i;
+	unsigned pagesize[] = { SZ_16M, SZ_1M, SZ_64K, SZ_4K, };
+	for (i = 0; i < ARRAY_SIZE(pagesize) && addr & (pagesize[i] - 1); i++)
+		;
+	return (i < ARRAY_SIZE(pagesize)) ? pagesize[i] : 0;
+}
+
+
 /*
  * calculate the optimal number sg elements from total bytes based on
  * iommu superpages
  */
-static unsigned int sgtable_nents(size_t bytes)
+static unsigned int sgtable_nents(size_t bytes, u32 da, u32 pa)
 {
-	int i;
-	unsigned int nr_entries;
-	const unsigned long pagesize[] = { SZ_16M, SZ_1M, SZ_64K, SZ_4K, };
+	unsigned int nr_entries = 0, ent_sz;
 
 	if (!IS_ALIGNED(bytes, PAGE_SIZE)) {
 		pr_err("%s: wrong size %08x\n", __func__, bytes);
 		return 0;
 	}
 
-	nr_entries = 0;
-	for (i = 0; i < ARRAY_SIZE(pagesize); i++) {
-		if (bytes >= pagesize[i]) {
-			nr_entries += (bytes / pagesize[i]);
-			bytes %= pagesize[i];
-		}
+	while (bytes) {
+		ent_sz = max_alignment(da | pa);
+		ent_sz = min(ent_sz, (unsigned)iopgsz_max(bytes));
+		nr_entries++;
+		da += ent_sz;
+		pa += ent_sz;
+		bytes -= ent_sz;
 	}
 	BUG_ON(bytes);
 
@@ -115,7 +125,8 @@ static unsigned int sgtable_nents(size_t bytes)
 }
 
 /* allocate and initialize sg_table header(a kind of 'superblock') */
-static struct sg_table *sgtable_alloc(const size_t bytes, u32 flags)
+static struct sg_table *sgtable_alloc(const size_t bytes, u32 flags,
+							u32 da, u32 pa)
 {
 	unsigned int nr_entries;
 	int err;
@@ -127,9 +138,8 @@ static struct sg_table *sgtable_alloc(const size_t bytes, u32 flags)
 	if (!IS_ALIGNED(bytes, PAGE_SIZE))
 		return ERR_PTR(-EINVAL);
 
-	/* FIXME: IOVMF_DA_FIXED should support 'superpages' */
-	if ((flags & IOVMF_LINEAR) && (flags & IOVMF_DA_ANON)) {
-		nr_entries = sgtable_nents(bytes);
+	if (flags & IOVMF_LINEAR) {
+		nr_entries = sgtable_nents(bytes, da, pa);
 		if (!nr_entries)
 			return ERR_PTR(-EINVAL);
 	} else
@@ -409,7 +419,8 @@ static inline void sgtable_drain_vmalloc(struct sg_table *sgt)
 	BUG_ON(!sgt);
 }
 
-static void sgtable_fill_kmalloc(struct sg_table *sgt, u32 pa, size_t len)
+static void sgtable_fill_kmalloc(struct sg_table *sgt, u32 pa, u32 da,
+								size_t len)
 {
 	unsigned int i;
 	struct scatterlist *sg;
@@ -420,7 +431,8 @@ static void sgtable_fill_kmalloc(struct sg_table *sgt, u32 pa, size_t len)
 	for_each_sg(sgt->sgl, sg, sgt->nents, i) {
 		size_t bytes;
 
-		bytes = iopgsz_max(len);
+		bytes = max_alignment(da | pa);
+		bytes = min(bytes, (size_t)iopgsz_max(len));
 
 		BUG_ON(!iopgsz_ok(bytes));
 
@@ -429,6 +441,7 @@ static void sgtable_fill_kmalloc(struct sg_table *sgt, u32 pa, size_t len)
 		 * 'pa' is cotinuous(linear).
 		 */
 		pa += bytes;
+		da += bytes;
 		len -= bytes;
 	}
 	BUG_ON(len);
@@ -695,18 +708,18 @@ u32 iommu_vmalloc(struct iommu *obj, u32 da, size_t bytes, u32 flags)
 	if (!va)
 		return -ENOMEM;
 
-	sgt = sgtable_alloc(bytes, 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)) {
 		da = PTR_ERR(sgt);
 		goto err_sgt_alloc;
 	}
 	sgtable_fill_vmalloc(sgt, va);
 
-	flags &= IOVMF_HW_MASK;
-	flags |= IOVMF_DISCONT;
-	flags |= IOVMF_ALLOC;
-	flags |= (da ? IOVMF_DA_FIXED : IOVMF_DA_ANON);
-
 	da = __iommu_vmap(obj, da, sgt, va, bytes, flags);
 	if (IS_ERR_VALUE(da))
 		goto err_iommu_vmap;
@@ -746,11 +759,11 @@ static u32 __iommu_kmap(struct iommu *obj, u32 da, u32 pa, void *va,
 {
 	struct sg_table *sgt;
 
-	sgt = sgtable_alloc(bytes, flags);
+	sgt = sgtable_alloc(bytes, flags, da, pa);
 	if (IS_ERR(sgt))
 		return PTR_ERR(sgt);
 
-	sgtable_fill_kmalloc(sgt, pa, bytes);
+	sgtable_fill_kmalloc(sgt, pa, da, bytes);
 
 	da = map_iommu_region(obj, da, sgt, va, bytes, flags);
 	if (IS_ERR_VALUE(da)) {
-- 
1.6.3.3

[PATCH 4/4] iovmm: replace __iounmap with omap_iounmap

[Fernando Guzman Lugo]

Omap platform is omap_iounmap function.

Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
---
 arch/arm/plat-omap/iovmm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
 mode change 100644 => 100755 arch/arm/plat-omap/iovmm.c

diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
old mode 100644
new mode 100755
index e7ffec8..326776d
--- a/arch/arm/plat-omap/iovmm.c
+++ b/arch/arm/plat-omap/iovmm.c
@@ -824,7 +824,7 @@ void iommu_kunmap(struct iommu *obj, u32 da)
 	struct sg_table *sgt;
 	typedef void (*func_t)(const void *);
 
-	sgt = unmap_vm_area(obj, da, (func_t)__iounmap,
+	sgt = unmap_vm_area(obj, da, (func_t)omap_iounmap,
 			    IOVMF_LINEAR | IOVMF_MMIO);
 	if (!sgt)
 		dev_dbg(obj->dev, "%s: No sgt\n", __func__);
-- 
1.6.3.3

RE: [PATCH 1/4] iommu: remove CONFIG_MPU_BRIDGE_IOMMU

[Marathe, Yogesh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Guzman Lugo, Fernando
> Sent: Friday, October 01, 2010 8:39 AM
> To: Hiroshi.DOYU@nokia.com
> Cc: felipe.contreras@nokia.com; ameya.palande@nokia.com; linux-
> kernel@vger.kernel.org; andy.shevchenko@gmail.com; linux-
> omap@vger.kernel.org; Guzman Lugo, Fernando
> Subject: [PATCH 1/4] iommu: remove
> CONFIG_MPU_BRIDGE_IOMMU
> 
> remove CONFIG_MPU_BRIDGE_IOMMU in order to create
> iommu_device
> for iva2.
> 
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> ---
>  arch/arm/mach-omap2/omap-iommu.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-
> omap2/omap-iommu.c
> index f5a1aad..bb8c01d 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -35,7 +35,6 @@ static struct iommu_device omap3_devices[] = {
>  			.clk_name = "cam_ick",
>  		},
>  	},
> -#if defined(CONFIG_MPU_BRIDGE_IOMMU)
>  	{
>  		.base = 0x5d000000,
>  		.irq = 28,
> @@ -45,7 +44,6 @@ static struct iommu_device omap3_devices[] = {
>  			.clk_name = "iva2_ck",
>  		},
>  	},
> -#endif
>  };
>  #define NR_OMAP3_IOMMU_DEVICES
> ARRAY_SIZE(omap3_devices)
>  static struct platform_device
> *omap3_iommu_pdev[NR_OMAP3_IOMMU_DEVICES];
> --
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fernando,

I have already submitted this patch and it is being discussed here.
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg35615.html

Regards,
Yogesh.

Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[David Cohen]

Hi,

On Fri, Oct 01, 2010 at 05:08:53AM +0200, ext Fernando Guzman Lugo wrote:
> As da_end does not belongs to the area the roundup should
> be done to da_end and not to da_end + 1.
> Also the end check for the last area should be
> ULONG_MAX - start + 1 >= bytes.
> 
> Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> ---
>  arch/arm/plat-omap/iovmm.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/iovmm.c b/arch/arm/plat-omap/iovmm.c
> index 24ca9c4..fc6b109 100644
> --- a/arch/arm/plat-omap/iovmm.c
> +++ b/arch/arm/plat-omap/iovmm.c
> @@ -289,19 +289,19 @@ static struct iovm_struct *alloc_iovm_area(struct iommu *obj, u32 da,
>  	prev_end = 0;
>  	list_for_each_entry(tmp, &obj->mmap, list) {
>  
> -		if (prev_end >= start)
> +		if (prev_end > start)
>  			break;
>  
>  		if (start + bytes <= tmp->da_start)
>  			goto found;
>  
>  		if (flags & IOVMF_DA_ANON)
> -			start = roundup(tmp->da_end + 1, alignement);
> +			start = roundup(tmp->da_end, alignement);

There's a lack of comment here, but the purpose of tmp->da_end + 1 is to
create a gap between iovm areas to force to trigger iommu faults when
some access exceeds a valid area. Without this gap, such situation
may produce data corruption which is much more difficult to track.

Br,

David

>  
>  		prev_end = tmp->da_end;
>  	}
>  
> -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> +	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
>  		goto found;
>  
>  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/4] iommu: remove CONFIG_MPU_BRIDGE_IOMMU

[Guzman Lugo, Fernando]

 

Hi Yogesh,

> -----Original Message-----
> From: Marathe, Yogesh 
> Sent: Friday, October 01, 2010 4:33 AM
> To: Guzman Lugo, Fernando; Hiroshi.DOYU@nokia.com
> Cc: felipe.contreras@nokia.com; ameya.palande@nokia.com; 
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; 
> linux-omap@vger.kernel.org
> Subject: RE: [PATCH 1/4] iommu: remove CONFIG_MPU_BRIDGE_IOMMU
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- 
> > owner@vger.kernel.org] On Behalf Of Guzman Lugo, Fernando
> > Sent: Friday, October 01, 2010 8:39 AM
> > To: Hiroshi.DOYU@nokia.com
> > Cc: felipe.contreras@nokia.com; ameya.palande@nokia.com; linux- 
> > kernel@vger.kernel.org; andy.shevchenko@gmail.com; linux- 
> > omap@vger.kernel.org; Guzman Lugo, Fernando
> > Subject: [PATCH 1/4] iommu: remove
> > CONFIG_MPU_BRIDGE_IOMMU
> > 
> > remove CONFIG_MPU_BRIDGE_IOMMU in order to create iommu_device for 
> > iva2.
> > 
> > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > ---
> >  arch/arm/mach-omap2/omap-iommu.c |    2 --
> >  1 files changed, 0 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach- 
> > omap2/omap-iommu.c index f5a1aad..bb8c01d 100644
> > --- a/arch/arm/mach-omap2/omap-iommu.c
> > +++ b/arch/arm/mach-omap2/omap-iommu.c
> > @@ -35,7 +35,6 @@ static struct iommu_device omap3_devices[] = {
> >  			.clk_name = "cam_ick",
> >  		},
> >  	},
> > -#if defined(CONFIG_MPU_BRIDGE_IOMMU)
> >  	{
> >  		.base = 0x5d000000,
> >  		.irq = 28,
> > @@ -45,7 +44,6 @@ static struct iommu_device omap3_devices[] = {
> >  			.clk_name = "iva2_ck",
> >  		},
> >  	},
> > -#endif
> >  };
> >  #define NR_OMAP3_IOMMU_DEVICES
> > ARRAY_SIZE(omap3_devices)
> >  static struct platform_device
> > *omap3_iommu_pdev[NR_OMAP3_IOMMU_DEVICES];
> > --
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" 
> > in the body of a message to majordomo@vger.kernel.org More 
> majordomo 
> > info at  http://vger.kernel.org/majordomo-info.html
> 
> Fernando,
> 
> I have already submitted this patch and it is being discussed here.
> http://www.mail-archive.com/linux-omap@vger.kernel.org/msg35615.html

Thanks Yogesh I was not aware of that. So please discard this patch.


Regards,
Fernando.

> 
> Regards,
> Yogesh.
> 
> 

RE: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[Guzman Lugo, Fernando]

 

> -----Original Message-----
> From: David Cohen [mailto:david.cohen@nokia.com] 
> Sent: Friday, October 01, 2010 5:57 AM
> To: Guzman Lugo, Fernando
> Cc: Doyu Hiroshi (Nokia-MS/Espoo); Contreras Felipe 
> (Nokia-MS/Helsinki); Palande Ameya (Nokia-MS/Helsinki); 
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; 
> linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and 
> end check for the last area
> 
> Hi,
> 
> On Fri, Oct 01, 2010 at 05:08:53AM +0200, ext Fernando Guzman 
> Lugo wrote:
> > As da_end does not belongs to the area the roundup should 
> be done to 
> > da_end and not to da_end + 1.
> > Also the end check for the last area should be ULONG_MAX - 
> start + 1 
> > >= bytes.
> > 
> > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com>
> > ---
> >  arch/arm/plat-omap/iovmm.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/plat-omap/iovmm.c 
> b/arch/arm/plat-omap/iovmm.c 
> > index 24ca9c4..fc6b109 100644
> > --- a/arch/arm/plat-omap/iovmm.c
> > +++ b/arch/arm/plat-omap/iovmm.c
> > @@ -289,19 +289,19 @@ static struct iovm_struct 
> *alloc_iovm_area(struct iommu *obj, u32 da,
> >  	prev_end = 0;
> >  	list_for_each_entry(tmp, &obj->mmap, list) {
> >  
> > -		if (prev_end >= start)
> > +		if (prev_end > start)
> >  			break;
> >  
> >  		if (start + bytes <= tmp->da_start)
> >  			goto found;
> >  
> >  		if (flags & IOVMF_DA_ANON)
> > -			start = roundup(tmp->da_end + 1, alignement);
> > +			start = roundup(tmp->da_end, alignement);
> 
> There's a lack of comment here, but the purpose of 
> tmp->da_end + 1 is to create a gap between iovm areas to 
> force to trigger iommu faults when some access exceeds a 
> valid area. Without this gap, such situation may produce data 
> corruption which is much more difficult to track.

That only works when you are accessing sequencially beyond the
End of the vm_area. However if you are accessing a random address
Which is in the mmu tables you still can corrupt memory which does
Not belong to you. That looks not very effective then why waste
Memory?

Maybe other mechanism should be implemente like in the process
Switching when if the process has DMM virtual memory area and if
So enablig only that area (all other process areas will be
Dissabled and it would get a mmufault in case of access). However
That increase the time of switching between process.

Regards,
Fernando.

> 
> Br,
> 
> David
> 
> >  
> >  		prev_end = tmp->da_end;
> >  	}
> >  
> > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > +	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> >  		goto found;
> >  
> >  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> > --
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> > linux-kernel" in the body of a message to majordomo@vger.kernel.org 
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[David Cohen]

On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, Fernando wrote:
>  

[snip]

> > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm/plat-omap/iovmm.c 
> > b/arch/arm/plat-omap/iovmm.c 
> > > index 24ca9c4..fc6b109 100644
> > > --- a/arch/arm/plat-omap/iovmm.c
> > > +++ b/arch/arm/plat-omap/iovmm.c
> > > @@ -289,19 +289,19 @@ static struct iovm_struct 
> > *alloc_iovm_area(struct iommu *obj, u32 da,
> > >  	prev_end = 0;
> > >  	list_for_each_entry(tmp, &obj->mmap, list) {
> > >  
> > > -		if (prev_end >= start)
> > > +		if (prev_end > start)
> > >  			break;
> > >  
> > >  		if (start + bytes <= tmp->da_start)
> > >  			goto found;
> > >  
> > >  		if (flags & IOVMF_DA_ANON)
> > > -			start = roundup(tmp->da_end + 1, alignement);
> > > +			start = roundup(tmp->da_end, alignement);
> > 
> > There's a lack of comment here, but the purpose of 
> > tmp->da_end + 1 is to create a gap between iovm areas to 
> > force to trigger iommu faults when some access exceeds a 
> > valid area. Without this gap, such situation may produce data 
> > corruption which is much more difficult to track.
> 
> That only works when you are accessing sequencially beyond the
> End of the vm_area. However if you are accessing a random address
> Which is in the mmu tables you still can corrupt memory which does
> Not belong to you. That looks not very effective then why waste
> Memory?

The main intention is to detect sequential access beyond the end of the
vm area and it is effective for that purpose.
i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, responsible
to produce statistics data for the captured image, to write more data than
it should. The workaround described in the errata wasn't enough to avoid
error conditions, so a different approach was implemented. This gap
did help me to make sure the new workaround is valid and no data
corruption was occurring anymore.
Anyway, I can't see why memory is being wasted.

> 
> Maybe other mechanism should be implemente like in the process
> Switching when if the process has DMM virtual memory area and if
> So enablig only that area (all other process areas will be
> Dissabled and it would get a mmufault in case of access). However
> That increase the time of switching between process.

Sure. We can have other mechanisms. But in the above scenario, H3A
submodule has sequential access and can corrupt its own data. It has
more than one buffer and they're likely to be mapped to sequential
memory areas without the mechanism you're about to remove.

If you're able to implement a better mechanism, please remove this one
just when a new is already there. :)

Regards,

David

> 
> Regards,
> Fernando.
> 
> > 
> > Br,
> > 
> > David
> > 
> > >  
> > >  		prev_end = tmp->da_end;
> > >  	}
> > >  
> > > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > +	if ((start >= prev_end) && (ULONG_MAX - start + 1 >= bytes))
> > >  		goto found;
> > >  
> > >  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) flags: %08x\n",
> > > --
> > > 1.6.3.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe 
> > > linux-kernel" in the body of a message to majordomo@vger.kernel.org 
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > 

RE: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[Guzman Lugo, Fernando]

 

> -----Original Message-----
> From: David Cohen [mailto:david.cohen@nokia.com] 
> Sent: Friday, October 01, 2010 12:53 PM
> To: Guzman Lugo, Fernando
> Cc: Doyu Hiroshi (Nokia-MS/Espoo); Contreras Felipe 
> (Nokia-MS/Helsinki); Palande Ameya (Nokia-MS/Helsinki); 
> linux-kernel@vger.kernel.org; andy.shevchenko@gmail.com; 
> linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and 
> end check for the last area
> 
> On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, 
> Fernando wrote:
> >  
> 
> [snip]
> 
> > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > > b/arch/arm/plat-omap/iovmm.c
> > > > index 24ca9c4..fc6b109 100644
> > > > --- a/arch/arm/plat-omap/iovmm.c
> > > > +++ b/arch/arm/plat-omap/iovmm.c
> > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > > >  	prev_end = 0;
> > > >  	list_for_each_entry(tmp, &obj->mmap, list) {
> > > >  
> > > > -		if (prev_end >= start)
> > > > +		if (prev_end > start)
> > > >  			break;
> > > >  
> > > >  		if (start + bytes <= tmp->da_start)
> > > >  			goto found;
> > > >  
> > > >  		if (flags & IOVMF_DA_ANON)
> > > > -			start = roundup(tmp->da_end + 
> 1, alignement);
> > > > +			start = roundup(tmp->da_end, 
> alignement);
> > > 
> > > There's a lack of comment here, but the purpose of
> > > tmp->da_end + 1 is to create a gap between iovm areas to
> > > force to trigger iommu faults when some access exceeds a 
> valid area. 
> > > Without this gap, such situation may produce data 
> corruption which 
> > > is much more difficult to track.
> > 
> > That only works when you are accessing sequencially beyond 
> the End of 
> > the vm_area. However if you are accessing a random address 
> Which is in 
> > the mmu tables you still can corrupt memory which does Not 
> belong to 
> > you. That looks not very effective then why waste Memory?
> 
> The main intention is to detect sequential access beyond the 
> end of the vm area and it is effective for that purpose.
> i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, 
> responsible to produce statistics data for the captured 
> image, to write more data than it should. The workaround 
> described in the errata wasn't enough to avoid error 
> conditions, so a different approach was implemented. This gap 
> did help me to make sure the new workaround is valid and no 
> data corruption was occurring anymore.
> Anyway, I can't see why memory is being wasted.
> 

I was taking about vitual memory waste (maybe not so important).
Is ok for me then keep the gap. Do other changes look good to
You?

Thnaks for the comments,
Fernando.

> > 
> > Maybe other mechanism should be implemente like in the process 
> > Switching when if the process has DMM virtual memory area and if So 
> > enablig only that area (all other process areas will be 
> Dissabled and 
> > it would get a mmufault in case of access). However That 
> increase the 
> > time of switching between process.
> 
> Sure. We can have other mechanisms. But in the above 
> scenario, H3A submodule has sequential access and can corrupt 
> its own data. It has more than one buffer and they're likely 
> to be mapped to sequential memory areas without the mechanism 
> you're about to remove.
> 
> If you're able to implement a better mechanism, please remove 
> this one just when a new is already there. :)
> 
> Regards,
> 
> David
> 
> > 
> > Regards,
> > Fernando.
> > 
> > > 
> > > Br,
> > > 
> > > David
> > > 
> > > >  
> > > >  		prev_end = tmp->da_end;
> > > >  	}
> > > >  
> > > > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > > +	if ((start >= prev_end) && (ULONG_MAX - start + 
> 1 >= bytes))
> > > >  		goto found;
> > > >  
> > > >  	dev_dbg(obj->dev, "%s: no space to fit %08x(%x) 
> flags: %08x\n",
> > > > --
> > > > 1.6.3.3
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe 
> > > > linux-kernel" in the body of a message to 
> > > > majordomo@vger.kernel.org More majordomo info at  
> > > > http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > 
> 

Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[David Cohen]

On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
>  
> > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, 
> > Fernando wrote:
> > >  
> > 
> > [snip]
> > 
> > > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > > > b/arch/arm/plat-omap/iovmm.c
> > > > > index 24ca9c4..fc6b109 100644
> > > > > --- a/arch/arm/plat-omap/iovmm.c
> > > > > +++ b/arch/arm/plat-omap/iovmm.c
> > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > > > >  	prev_end = 0;
> > > > >  	list_for_each_entry(tmp, &obj->mmap, list) {
> > > > >  
> > > > > -		if (prev_end >= start)
> > > > > +		if (prev_end > start)
> > > > >  			break;
> > > > >  
> > > > >  		if (start + bytes <= tmp->da_start)
> > > > >  			goto found;
> > > > >  
> > > > >  		if (flags & IOVMF_DA_ANON)
> > > > > -			start = roundup(tmp->da_end + 
> > 1, alignement);
> > > > > +			start = roundup(tmp->da_end, 
> > alignement);
> > > > 
> > > > There's a lack of comment here, but the purpose of
> > > > tmp->da_end + 1 is to create a gap between iovm areas to
> > > > force to trigger iommu faults when some access exceeds a 
> > valid area. 
> > > > Without this gap, such situation may produce data 
> > corruption which 
> > > > is much more difficult to track.
> > > 
> > > That only works when you are accessing sequencially beyond 
> > the End of 
> > > the vm_area. However if you are accessing a random address 
> > Which is in 
> > > the mmu tables you still can corrupt memory which does Not 
> > belong to 
> > > you. That looks not very effective then why waste Memory?
> > 
> > The main intention is to detect sequential access beyond the 
> > end of the vm area and it is effective for that purpose.
> > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, 
> > responsible to produce statistics data for the captured 
> > image, to write more data than it should. The workaround 
> > described in the errata wasn't enough to avoid error 
> > conditions, so a different approach was implemented. This gap 
> > did help me to make sure the new workaround is valid and no 
> > data corruption was occurring anymore.
> > Anyway, I can't see why memory is being wasted.
> > 
> 
> I was taking about vitual memory waste (maybe not so important).
> Is ok for me then keep the gap. Do other changes look good to
> You?

Do you mean in this patch?
All changes make sense only if you're removing the gap, except for the
fix below.

[snip]

> > > > >  
> > > > >  		prev_end = tmp->da_end;
> > > > >  	}
> > > > >  
> > > > > -	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > > > +	if ((start >= prev_end) && (ULONG_MAX - start + 
> > 1 >= bytes))

This fix is partially valid. The correct change must be only:
-	if ((start > prev_end) && (ULONG_MAX - start >= bytes))
+	if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))

Otherwise you wouldn't guarantee the gap for fixed da.

Br,

David

RE: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[Guzman Lugo, Fernando]

> ________________________________________
> From: David Cohen [david.cohen@nokia.com]
> Sent: Saturday, October 02, 2010 2:49 AM
> To: Guzman Lugo, Fernando
> Cc: Doyu Hiroshi (Nokia-MS/Espoo); Contreras Felipe (Nokia-MS/Helsinki);
>  Palande Ameya (Nokia-MS/Helsinki); linux-kernel@vger.kernel.org;
>  andy.shevchenko@gmail.com; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for
>  the last area
> 
> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
> >
> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo,
> > > Fernando wrote:
> > > >
> > >
> > > [snip]
> > >
> > > > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > > > > b/arch/arm/plat-omap/iovmm.c
> > > > > > index 24ca9c4..fc6b109 100644
> > > > > > --- a/arch/arm/plat-omap/iovmm.c
> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > > > > >       prev_end = 0;
> > > > > >       list_for_each_entry(tmp, &obj->mmap, list) {
> > > > > >
> > > > > > -             if (prev_end >= start)
> > > > > > +             if (prev_end > start)
> > > > > >                       break;
> > > > > >
> > > > > >               if (start + bytes <= tmp->da_start)
> > > > > >                       goto found;
> > > > > >
> > > > > >               if (flags & IOVMF_DA_ANON)
> > > > > > -                     start = roundup(tmp->da_end +
> > > 1, alignement);
> > > > > > +                     start = roundup(tmp->da_end,
> > > alignement);
> > > > >
> > > > > There's a lack of comment here, but the purpose of
> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
> > > > > force to trigger iommu faults when some access exceeds a
> > > valid area.
> > > > > Without this gap, such situation may produce data
> > > corruption which
> > > > > is much more difficult to track.
> > > >
> > > > That only works when you are accessing sequencially beyond
> > > the End of
> > > > the vm_area. However if you are accessing a random address
> > > Which is in
> > > > the mmu tables you still can corrupt memory which does Not
> > > belong to
> > > > you. That looks not very effective then why waste Memory?
> > >
> > > The main intention is to detect sequential access beyond the
> > > end of the vm area and it is effective for that purpose.
> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule,
> > > responsible to produce statistics data for the captured
> > > image, to write more data than it should. The workaround
> > > described in the errata wasn't enough to avoid error
> > > conditions, so a different approach was implemented. This gap
> > > did help me to make sure the new workaround is valid and no
> > > data corruption was occurring anymore.
> > > Anyway, I can't see why memory is being wasted.
> > >
> >
> > I was taking about vitual memory waste (maybe not so important).
> > Is ok for me then keep the gap. Do other changes look good to
> > You?
> 
> Do you mean in this patch?
> All changes make sense only if you're removing the gap, except for the
> fix below.

The thing is, the dspbridge needs to map some register in order to DSP 
can read and configure some of them. We need to map some pages
with fix addresses and to do that I use iommu_kmap. So when some
of that pages are contiguous I get his error:

"%s: no space to fit %08x(%x) flags: %08x\n"

Which is not true. The page to page perfectly fix, but the check with 1 byte
more avoid that it could be mapped and I am getting the error.

I am not agree with the gap, but I am ok when it is not fixed address as 
below code

if (flags & IOVMF_DA_ANON)
        start = roundup(tmp->da_end + 1, alignement);

But it is breaking the tidspbridge when the gap is used for fixed addresses.

It should not fail when we want to map a page what is freed just because of the gap.
Please let me know what you thing.

Thanks,
Fernando.

> 
> [snip]
> 
> > > > > >
> > > > > >               prev_end = tmp->da_end;
> > > > > >       }
> > > > > >
> > > > > > -     if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > > > > > +     if ((start >= prev_end) && (ULONG_MAX - start +
> > > 1 >= bytes))
> 
> This fix is partially valid. The correct change must be only:
> -       if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> +       if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
> 
> Otherwise you wouldn't guarantee the gap for fixed da.
> 
> Br,
> 
> David

Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[David Cohen]

Hi,

I have no access to my @nokia.com e-mail at this moment, so I'm
replying using my personal one.

On Mon, Oct 4, 2010 at 6:17 AM, Guzman Lugo, Fernando
<fernando.lugo@ti.com> wrote:
>
>> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, Fernando wrote:
>> >
>> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo,
>> > > Fernando wrote:
>> > > >
>> > >
>> > > [snip]
>> > >
>> > > > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
>> > > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
>> > > > > >
>> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
>> > > > > b/arch/arm/plat-omap/iovmm.c
>> > > > > > index 24ca9c4..fc6b109 100644
>> > > > > > --- a/arch/arm/plat-omap/iovmm.c
>> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
>> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
>> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
>> > > > > >       prev_end = 0;
>> > > > > >       list_for_each_entry(tmp, &obj->mmap, list) {
>> > > > > >
>> > > > > > -             if (prev_end >= start)
>> > > > > > +             if (prev_end > start)
>> > > > > >                       break;
>> > > > > >
>> > > > > >               if (start + bytes <= tmp->da_start)
>> > > > > >                       goto found;
>> > > > > >
>> > > > > >               if (flags & IOVMF_DA_ANON)
>> > > > > > -                     start = roundup(tmp->da_end +
>> > > 1, alignement);
>> > > > > > +                     start = roundup(tmp->da_end,
>> > > alignement);
>> > > > >
>> > > > > There's a lack of comment here, but the purpose of
>> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
>> > > > > force to trigger iommu faults when some access exceeds a
>> > > valid area.
>> > > > > Without this gap, such situation may produce data
>> > > corruption which
>> > > > > is much more difficult to track.
>> > > >
>> > > > That only works when you are accessing sequencially beyond
>> > > the End of
>> > > > the vm_area. However if you are accessing a random address
>> > > Which is in
>> > > > the mmu tables you still can corrupt memory which does Not
>> > > belong to
>> > > > you. That looks not very effective then why waste Memory?
>> > >
>> > > The main intention is to detect sequential access beyond the
>> > > end of the vm area and it is effective for that purpose.
>> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule,
>> > > responsible to produce statistics data for the captured
>> > > image, to write more data than it should. The workaround
>> > > described in the errata wasn't enough to avoid error
>> > > conditions, so a different approach was implemented. This gap
>> > > did help me to make sure the new workaround is valid and no
>> > > data corruption was occurring anymore.
>> > > Anyway, I can't see why memory is being wasted.
>> > >
>> >
>> > I was taking about vitual memory waste (maybe not so important).
>> > Is ok for me then keep the gap. Do other changes look good to
>> > You?
>>
>> Do you mean in this patch?
>> All changes make sense only if you're removing the gap, except for the
>> fix below.
>
> The thing is, the dspbridge needs to map some register in order to DSP
> can read and configure some of them. We need to map some pages
> with fix addresses and to do that I use iommu_kmap. So when some
> of that pages are contiguous I get his error:
>
> "%s: no space to fit %08x(%x) flags: %08x\n"
>
> Which is not true. The page to page perfectly fix, but the check with 1 byte
> more avoid that it could be mapped and I am getting the error.
>
> I am not agree with the gap, but I am ok when it is not fixed address as
> below code
>
> if (flags & IOVMF_DA_ANON)
>        start = roundup(tmp->da_end + 1, alignement);
>
> But it is breaking the tidspbridge when the gap is used for fixed addresses.
>
> It should not fail when we want to map a page what is freed just because of the gap.
> Please let me know what you thing.

I got your point. I agree the gap shouldn't be forced for fixed da.
IMO you can apply this change when !(flags & IOVMF_DA_ANON).

Regards,

David

>
> Thanks,
> Fernando.
>
>>
>> [snip]
>>
>> > > > > >
>> > > > > >               prev_end = tmp->da_end;
>> > > > > >       }
>> > > > > >
>> > > > > > -     if ((start > prev_end) && (ULONG_MAX - start >= bytes))
>> > > > > > +     if ((start >= prev_end) && (ULONG_MAX - start +
>> > > 1 >= bytes))
>>
>> This fix is partially valid. The correct change must be only:
>> -       if ((start > prev_end) && (ULONG_MAX - start >= bytes))
>> +       if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
>>
>> Otherwise you wouldn't guarantee the gap for fixed da.
>>
>> Br,
>>
>> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[David Cohen]

On Mon, Oct 04, 2010 at 05:37:04PM +0200, ext Guzman Lugo, Fernando wrote:
>  
> 
> > -----Original Message-----
> > From: David Cohen [mailto:dacohen@gmail.com] 
> > Sent: Monday, October 04, 2010 7:11 AM
> > To: Guzman Lugo, Fernando
> > Cc: David Cohen; Doyu Hiroshi (Nokia-MS/Espoo); Contreras 
> > Felipe (Nokia-MS/Helsinki); Palande Ameya 
> > (Nokia-MS/Helsinki); linux-kernel@vger.kernel.org; 
> > andy.shevchenko@gmail.com; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and 
> > end check for the last area
> > 
> > Hi,
> > 
> > I have no access to my @nokia.com e-mail at this moment, so 
> > I'm replying using my personal one.
> > 
> > On Mon, Oct 4, 2010 at 6:17 AM, Guzman Lugo, Fernando 
> > <fernando.lugo@ti.com> wrote:
> > >
> > >> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, 
> > Fernando wrote:
> > >> >
> > >> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, 
> > >> > > Fernando wrote:
> > >> > > >
> > >> > >
> > >> > > [snip]
> > >> > >
> > >> > > > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> > >> > > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >> > > > > >
> > >> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> > >> > > > > b/arch/arm/plat-omap/iovmm.c
> > >> > > > > > index 24ca9c4..fc6b109 100644
> > >> > > > > > --- a/arch/arm/plat-omap/iovmm.c
> > >> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
> > >> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> > >> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> > >> > > > > >       prev_end = 0;
> > >> > > > > >       list_for_each_entry(tmp, &obj->mmap, list) {
> > >> > > > > >
> > >> > > > > > -             if (prev_end >= start)
> > >> > > > > > +             if (prev_end > start)
> > >> > > > > >                       break;
> > >> > > > > >
> > >> > > > > >               if (start + bytes <= tmp->da_start)
> > >> > > > > >                       goto found;
> > >> > > > > >
> > >> > > > > >               if (flags & IOVMF_DA_ANON)
> > >> > > > > > -                     start = roundup(tmp->da_end +
> > >> > > 1, alignement);
> > >> > > > > > +                     start = roundup(tmp->da_end,
> > >> > > alignement);
> > >> > > > >
> > >> > > > > There's a lack of comment here, but the purpose of
> > >> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
> > >> > > > > force to trigger iommu faults when some access exceeds a
> > >> > > valid area.
> > >> > > > > Without this gap, such situation may produce data
> > >> > > corruption which
> > >> > > > > is much more difficult to track.
> > >> > > >
> > >> > > > That only works when you are accessing sequencially beyond
> > >> > > the End of
> > >> > > > the vm_area. However if you are accessing a random address
> > >> > > Which is in
> > >> > > > the mmu tables you still can corrupt memory which does Not
> > >> > > belong to
> > >> > > > you. That looks not very effective then why waste Memory?
> > >> > >
> > >> > > The main intention is to detect sequential access 
> > beyond the end 
> > >> > > of the vm area and it is effective for that purpose.
> > >> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, 
> > >> > > responsible to produce statistics data for the 
> > captured image, to 
> > >> > > write more data than it should. The workaround 
> > described in the 
> > >> > > errata wasn't enough to avoid error conditions, so a different 
> > >> > > approach was implemented. This gap did help me to make 
> > sure the 
> > >> > > new workaround is valid and no data corruption was occurring 
> > >> > > anymore.
> > >> > > Anyway, I can't see why memory is being wasted.
> > >> > >
> > >> >
> > >> > I was taking about vitual memory waste (maybe not so important).
> > >> > Is ok for me then keep the gap. Do other changes look 
> > good to You?
> > >>
> > >> Do you mean in this patch?
> > >> All changes make sense only if you're removing the gap, except for 
> > >> the fix below.
> > >
> > > The thing is, the dspbridge needs to map some register in 
> > order to DSP 
> > > can read and configure some of them. We need to map some pages with 
> > > fix addresses and to do that I use iommu_kmap. So when some of that 
> > > pages are contiguous I get his error:
> > >
> > > "%s: no space to fit %08x(%x) flags: %08x\n"
> > >
> > > Which is not true. The page to page perfectly fix, but the 
> > check with 
> > > 1 byte more avoid that it could be mapped and I am getting 
> > the error.
> > >
> > > I am not agree with the gap, but I am ok when it is not 
> > fixed address 
> > > as below code
> > >
> > > if (flags & IOVMF_DA_ANON)
> > >        start = roundup(tmp->da_end + 1, alignement);
> > >
> > > But it is breaking the tidspbridge when the gap is used for 
> > fixed addresses.
> > >
> > > It should not fail when we want to map a page what is freed 
> > just because of the gap.
> > > Please let me know what you thing.
> > 
> > I got your point. I agree the gap shouldn't be forced for fixed da.
> > IMO you can apply this change when !(flags & IOVMF_DA_ANON).
> 
> As for not fixed address it always travers the list from the
> Beginning. The only change need to revert in my patch is when
> We roundup. That means keeping:
> 
> if (flags & IOVMF_DA_ANON)
>         start = roundup(tmp->da_end + 1, alignement);
> 
> Is enough to create the gap for not fixed address. I will
> Update the patch and send them again.

Sounds fine for me. Please, update the patch and I'll ack it.

Br,

David

> 
> Thanks,
> Fernando.
> 
> > 
> > Regards,
> > 
> > David
> > 
> > >
> > > Thanks,
> > > Fernando.
> > >
> > >>
> > >> [snip]
> > >>
> > >> > > > > >
> > >> > > > > >               prev_end = tmp->da_end;
> > >> > > > > >       }
> > >> > > > > >
> > >> > > > > > -     if ((start > prev_end) && (ULONG_MAX - start >= 
> > >> > > > > > bytes))
> > >> > > > > > +     if ((start >= prev_end) && (ULONG_MAX - start +
> > >> > > 1 >= bytes))
> > >>
> > >> This fix is partially valid. The correct change must be only:
> > >> -       if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> > >> +       if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
> > >>
> > >> Otherwise you wouldn't guarantee the gap for fixed da.
> > >>
> > >> Br,
> > >>
> > >> David
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe 
> > linux-omap" 
> > > in the body of a message to majordomo@vger.kernel.org More 
> > majordomo 
> > > info at  http://vger.kernel.org/majordomo-info.html
> > >
> > 

RE: [PATCH 2/4] iovmm: fix roundup for next area and end check for the last area

[Guzman Lugo, Fernando]

 

> -----Original Message-----
> From: David Cohen [mailto:dacohen@gmail.com] 
> Sent: Monday, October 04, 2010 7:11 AM
> To: Guzman Lugo, Fernando
> Cc: David Cohen; Doyu Hiroshi (Nokia-MS/Espoo); Contreras 
> Felipe (Nokia-MS/Helsinki); Palande Ameya 
> (Nokia-MS/Helsinki); linux-kernel@vger.kernel.org; 
> andy.shevchenko@gmail.com; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/4] iovmm: fix roundup for next area and 
> end check for the last area
> 
> Hi,
> 
> I have no access to my @nokia.com e-mail at this moment, so 
> I'm replying using my personal one.
> 
> On Mon, Oct 4, 2010 at 6:17 AM, Guzman Lugo, Fernando 
> <fernando.lugo@ti.com> wrote:
> >
> >> On Fri, Oct 01, 2010 at 09:21:36PM +0200, ext Guzman Lugo, 
> Fernando wrote:
> >> >
> >> > > On Fri, Oct 01, 2010 at 06:10:30PM +0200, ext Guzman Lugo, 
> >> > > Fernando wrote:
> >> > > >
> >> > >
> >> > > [snip]
> >> > >
> >> > > > > >  arch/arm/plat-omap/iovmm.c |    6 +++---
> >> > > > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> >> > > > > >
> >> > > > > > diff --git a/arch/arm/plat-omap/iovmm.c
> >> > > > > b/arch/arm/plat-omap/iovmm.c
> >> > > > > > index 24ca9c4..fc6b109 100644
> >> > > > > > --- a/arch/arm/plat-omap/iovmm.c
> >> > > > > > +++ b/arch/arm/plat-omap/iovmm.c
> >> > > > > > @@ -289,19 +289,19 @@ static struct iovm_struct
> >> > > > > *alloc_iovm_area(struct iommu *obj, u32 da,
> >> > > > > >       prev_end = 0;
> >> > > > > >       list_for_each_entry(tmp, &obj->mmap, list) {
> >> > > > > >
> >> > > > > > -             if (prev_end >= start)
> >> > > > > > +             if (prev_end > start)
> >> > > > > >                       break;
> >> > > > > >
> >> > > > > >               if (start + bytes <= tmp->da_start)
> >> > > > > >                       goto found;
> >> > > > > >
> >> > > > > >               if (flags & IOVMF_DA_ANON)
> >> > > > > > -                     start = roundup(tmp->da_end +
> >> > > 1, alignement);
> >> > > > > > +                     start = roundup(tmp->da_end,
> >> > > alignement);
> >> > > > >
> >> > > > > There's a lack of comment here, but the purpose of
> >> > > > > tmp->da_end + 1 is to create a gap between iovm areas to
> >> > > > > force to trigger iommu faults when some access exceeds a
> >> > > valid area.
> >> > > > > Without this gap, such situation may produce data
> >> > > corruption which
> >> > > > > is much more difficult to track.
> >> > > >
> >> > > > That only works when you are accessing sequencially beyond
> >> > > the End of
> >> > > > the vm_area. However if you are accessing a random address
> >> > > Which is in
> >> > > > the mmu tables you still can corrupt memory which does Not
> >> > > belong to
> >> > > > you. That looks not very effective then why waste Memory?
> >> > >
> >> > > The main intention is to detect sequential access 
> beyond the end 
> >> > > of the vm area and it is effective for that purpose.
> >> > > i.e., OMAP3 ISP has a hw issue which makes its H3A submodule, 
> >> > > responsible to produce statistics data for the 
> captured image, to 
> >> > > write more data than it should. The workaround 
> described in the 
> >> > > errata wasn't enough to avoid error conditions, so a different 
> >> > > approach was implemented. This gap did help me to make 
> sure the 
> >> > > new workaround is valid and no data corruption was occurring 
> >> > > anymore.
> >> > > Anyway, I can't see why memory is being wasted.
> >> > >
> >> >
> >> > I was taking about vitual memory waste (maybe not so important).
> >> > Is ok for me then keep the gap. Do other changes look 
> good to You?
> >>
> >> Do you mean in this patch?
> >> All changes make sense only if you're removing the gap, except for 
> >> the fix below.
> >
> > The thing is, the dspbridge needs to map some register in 
> order to DSP 
> > can read and configure some of them. We need to map some pages with 
> > fix addresses and to do that I use iommu_kmap. So when some of that 
> > pages are contiguous I get his error:
> >
> > "%s: no space to fit %08x(%x) flags: %08x\n"
> >
> > Which is not true. The page to page perfectly fix, but the 
> check with 
> > 1 byte more avoid that it could be mapped and I am getting 
> the error.
> >
> > I am not agree with the gap, but I am ok when it is not 
> fixed address 
> > as below code
> >
> > if (flags & IOVMF_DA_ANON)
> >        start = roundup(tmp->da_end + 1, alignement);
> >
> > But it is breaking the tidspbridge when the gap is used for 
> fixed addresses.
> >
> > It should not fail when we want to map a page what is freed 
> just because of the gap.
> > Please let me know what you thing.
> 
> I got your point. I agree the gap shouldn't be forced for fixed da.
> IMO you can apply this change when !(flags & IOVMF_DA_ANON).

As for not fixed address it always travers the list from the
Beginning. The only change need to revert in my patch is when
We roundup. That means keeping:

if (flags & IOVMF_DA_ANON)
        start = roundup(tmp->da_end + 1, alignement);

Is enough to create the gap for not fixed address. I will
Update the patch and send them again.

Thanks,
Fernando.

> 
> Regards,
> 
> David
> 
> >
> > Thanks,
> > Fernando.
> >
> >>
> >> [snip]
> >>
> >> > > > > >
> >> > > > > >               prev_end = tmp->da_end;
> >> > > > > >       }
> >> > > > > >
> >> > > > > > -     if ((start > prev_end) && (ULONG_MAX - start >= 
> >> > > > > > bytes))
> >> > > > > > +     if ((start >= prev_end) && (ULONG_MAX - start +
> >> > > 1 >= bytes))
> >>
> >> This fix is partially valid. The correct change must be only:
> >> -       if ((start > prev_end) && (ULONG_MAX - start >= bytes))
> >> +       if ((start > prev_end) && (ULONG_MAX - start + 1 >= bytes))
> >>
> >> Otherwise you wouldn't guarantee the gap for fixed da.
> >>
> >> Br,
> >>
> >> David
> > --
> > To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" 
> > in the body of a message to majordomo@vger.kernel.org More 
> majordomo 
> > info at  http://vger.kernel.org/majordomo-info.html
> >
>