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