To summarize the discussion. 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. Also, another reason is the flush function in Bridge driver is going to be removed eventually. The open issue is the impact on the OMX components. Once if we finalize that there is no impact on the existing OMX components or we have a plan to take this into account we can enforce this check. Thank you, Best regards, Hari > -----Original Message----- > From: Felipe Contreras [mailto:felipe.contreras@xxxxxxxxx] > Sent: Monday, May 11, 2009 12:54 PM > To: Hiroshi DOYU > Cc: Kanigeri, Hari; Menon, Nishanth; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for > dsp cache line size > > 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
