On Wed, May 13, 2009 at 2:32 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
> From: ext Felipe Contreras <felipe.contreras@xxxxxxxxx>
> Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
> Date: Wed, 13 May 2009 13:13:35 +0200
>
>> On Wed, May 13, 2009 at 12:10 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
>> > From: ext Felipe Contreras <felipe.contreras@xxxxxxxxx>
>> > Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
>> > Date: Tue, 12 May 2009 23:41:04 +0200
>> >
>> >> On Tue, May 12, 2009 at 8:38 AM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
>> >> > From: "ext Kanigeri, Hari" <h-kanigeri2@xxxxxx>
>> >> > Subject: RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
>> >> > Date: Mon, 11 May 2009 20:26:11 +0200
>> >> >
>> >> >> To summarize the discussion.
>> >> >
>> >> > Thanks
>> >> >
>> >> >> We need to have the check for 128 bytes alignment (upper and
>> >> >> lower). The 2 places that this can be done are in flush function or
>> >> >> in Map function.
>> >> >>
>> >> >> - I prefer the check is done in Map function as Felipe
>> >> >> mentioned this function is bound to be called by MM components as
>> >> >> opposed to Flush function.
>> >> >
>> >> > Mapping is totally another thing from this problem.
>> >> >
>> >> > Any page mapping is being done, forcing PAGE_SIZE as below, because
>> >> > PAGE_SIZE(4KB) is the mininum unit from (io)mmu H/W perspective. This
>> >> > is same as ARM too. They've been aligned on 4KB already. So no need to
>> >> > worry about 128-byte alignement for mapping.
>> >>
>> >> It's the same address, you cannot flush an address that has not been
>> >> mapped. Look at the dsp-dummy code[1].
>> >>
>> >> DSPProcessor_ReserveMemory(b->node, to_reserve, &b->reserve);
>> >> DSPProcessor_Map(b->node, b->data, b->size, b->reserve, &b->map, 0);
>> >> DSPProcessor_FlushMemory(b->node, b->data, b->size, 0);
>> >>
>> >> See? b->data is used for *both* Map and FlushMemory. The only
>> >> difference is that you can skip FlushMemory, or flush half of the
>> >> memory area:
>> >> DSPProcessor_FlushMemory(b->node, b->data + (b->size / 2), b->size, 0);
>> >>
>> >> The only operation you cannot avoid is Map.
>> >>
>> >> > DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
>> >> > void *pReqAddr, void **ppMapAddr, u32 ulMapAttr)
>> >> > {
>> >> > .....
>> >> > /* Calculate the page-aligned PA, VA and size */
>> >> > vaAlign = PG_ALIGN_LOW((u32) pReqAddr, PG_SIZE_4K);
>> >> > paAlign = PG_ALIGN_LOW((u32) pMpuAddr, PG_SIZE_4K);
>> >> > sizeAlign = PG_ALIGN_HIGH(ulSize + (u32)pMpuAddr - paAlign,
>> >> > PG_SIZE_4K);
>> >> >
>> >> > The points which we should take care of are bridge cache
>> >> > operations("PROC_Flush/Invalidate..()") and we should/can handle this
>> >> > problem independently. It would cause another dependency and increase
>> >> > another complexity again if we have to assume something(ex:
>> >> > "read-only" or "write-only" buffer) on other modules or expecting
>> >> > something on its "protocol".
>> >>
>> >> If we are not going to have a read-only/write-only flag then I'm
>> >> against adding the alignment check. It will only force user-space to
>> >> do memory copies unnecessarily. That would kill performance
>> >> drastically. NAK!
>> >
>> > At least, better than allowing user to crash kernel.
>>
>> The kernel would crash even with that check because it's the software
>> running in the *DSP* side that is causing the corruption.
>>
>> It sounds you are still not convinced by that so I'll write a test
>> application to make sure my assumption is correct.
>
> You misunderstood.
>
> There are 2 patterns when kernel crashes from ARM cache flush and DSP
> cache flush(*1).
Let's suppose that's true, if you enforce ARM cache flushes to be
aligned (pattern 1) the DSP cache flushes (pattern 2) can still
corrupt the memory. Therefore even with your check the potential for
memory corruption is still there.
But it doesn't seem it's true; I modified my dsp-dummy test to detect
memory corruption by constantly modifying the data before and after
the buffer sent to the DSP. Map, Flush and Invalidate are unaligned in
the ARM side, and not surprisingly there's memory corruption. *But* if
I modify the DSP socket node to don't flush/invalidate the first/last
128 bytes of the buffer then there's no memory corruption.
So that's proof Flush/Invalidate on ARM side don't cause memory
corruption. Do you want to see the code?
--
Felipe Contreras
--
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
