On Mon, May 11, 2009 at 8:35 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: Mon, 11 May 2009 18:31:28 +0200 > >> On Mon, May 11, 2009 at 6:47 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote: >> > Hi Hari, >> > >> > 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 17:09:03 +0200 >> > >> >> Hi Doyu-san, >> >> >> >> > A buffer shared with MPU and DSP has to be aligned on both cache line >> >> > size to avoid memory corrupton with some DSP cache operations. Since >> >> > there's no way for dspbridge to know how the shared buffer will be >> >> > used like: "read-only", "write-only", "rw" through its life span, any >> >> > shared buffer passed to DSP should be on this alignment. This patch >> >> > adds checking those shared buffer alignement in bridgedriver cache >> >> > operations and prevents userland applications from causing the above >> >> > memory corruption. >> >> > >> >> >> >> -- It looks like the buffer that are passed to the Bridge are not necessarily 128 byte aligned. >> >> >> >> This is the comment I received from Nikhil Mande (MM Engineer). >> >> >> >> [Nikhil Mande] "They are not necessarily "aligned" all the time. >> >> Sometimes they just have 128 byte padding at the start & end of the buffer and the buffer pointer passed to bridge is not guaranteed to be 128 aligned. >> >> So if you are adding such a check, it will require modifications OMX >> >> components & test apps." >> > >> > The above means that passing unaligned address to dsp can cause memory >> > corruption in kernel and this problem can be avoided only in the case >> > where OMX(userland) is always supposed to pass aligned address. The >> > above assumption cannot be accepted because kernel should always be >> > robust against any incorrect behaviour of userland application. For >> > example, even if an userland application passes any incorrect >> > parameters to kernel through ioctl(), kernel shouldn't crash, but just >> > returns -EINVAL. >> >> Yes, but flush and invalidate are not the right places to do that. An >> application in user-space might be buggy and decide not to do any >> flush or invalidation, therefore the checks won't happen. A more >> proper use-case could be to flush only parts of a big buffer. > > Without cache flushing/invalidating from ARM, memory won't be > corrupted. The problem here is cache operation with incorrect address. >From ARM point of view flushing is done after ARM side writes to the buffer and before DSP *reads*, right? So invalidate happens after DSP *writes* to the buffer and before ARM reads it, right? If that's the case then the check should happen only on invalidate, not flush. But oh wait a second, omx is not invalidating any memory. So can you explain to me what is happening? But the DSP side also does the same; before reading a buffer from ARM it invalidates it, and after writing it flushes it. I thought the last operation (DSP flush) is the one that generated the memory corruption. >> Also, you are not checking for the upper limit, which also should be >> 128-byte aligned. > > The further sanity checking should be done with VMA. That's the unix way. The VMA would check the flush from DSP side is not touching wrong ARM memory? >> A more correct place to do this would be on PROC_Map, which user-space >> cannot avoid in any way, and also you have the upper limit too. >> Unfortunately TI's OpenMAX IL is sending a fake upper limit (page >> aligned). > > Mapping is done with page aligned at least. no sense to do it in "PROC_Map()" With page aligned what? MPU address or memory area size? The omx code is page aligning the size but so is the the PROC_Map code, so that's redundant. In any case the ARM address (pMpuAddr) the one that should be 128-byte aligned: DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize, void *pReqAddr, void **ppMapAddr, u32 ulMapAttr) -- 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
