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

[David Cohen <david.cohen@nokia.com>]

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