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

["Guzman Lugo, Fernando" <fernando.lugo@ti.com>]

> ________________________________________
> 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