Re: [PATCH 2/4] iovmm: fix roundup for next area and end check forthe 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@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).

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/