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