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. > 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. > 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()" > IMHO the only sensible way to approach this is to create a new ioctl > for a special restrictive mapping where user-space specifies if the > memory area is read-only or write-only and the size must be the real > size. This also has advantage that it does not break legacy > applications. While we are at it, this new mapping could do > PROC_ReserveMemory at the same time so that the user-space application > doesn't get a chance to modify the pa. > > -- > 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
