RE: [PATCH 2/4] iovmm: fix roundup for next area and end check forthe last area
From: Guzman Lugo, Fernando
Date: Mon Oct 04 2010 - 11:37:22 EST
> -----Original Message-----
> From: David Cohen [mailto:dacohen@xxxxxxxxx]
> 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@xxxxxxxxxxxxxxx;
> andy.shevchenko@xxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx
> 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@xxxxxx> 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@xxxxxxxxxxxxxxx More
> majordomo
> > info at http://vger.kernel.org/majordomo-info.html
> >
> --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/