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).
1: https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf
--
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
