[PATCH 1/2] DSPBRIDGE: fix L2 pagetable alignment for DSP MMU

[PATCH 1/2] DSPBRIDGE: fix L2 pagetable alignment for DSP MMU

[Hiroshi DOYU]

From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>


The L2 pagetable should align on 1KB since the lower 10 bits of L2
pagetable address are not used by DSP H/W.

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 drivers/dsp/bridge/wmd/tiomap3430.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/dsp/bridge/wmd/tiomap3430.c b/drivers/dsp/bridge/wmd/tiomap3430.c
index 2ab585d..5a858bd 100644
--- a/drivers/dsp/bridge/wmd/tiomap3430.c
+++ b/drivers/dsp/bridge/wmd/tiomap3430.c
@@ -1080,8 +1080,7 @@ static DSP_STATUS WMD_DEV_Create(OUT struct WMD_DEV_CONTEXT **ppDevContext,
 		pPtAttrs->L2NumPages = ((DMMPOOLSIZE >> 20) + 6);
 		pPtAttrs->L2size = HW_MMU_COARSE_PAGE_SIZE *
 				   pPtAttrs->L2NumPages;
-		align_size = 4; /* Make it u32 aligned  */
-		/* we like to get aligned on L1 table size */
+		align_size = 1 << 10; /* L2 pagetable alignement */
 		pg_tbl_va = (u32)MEM_AllocPhysMem(pPtAttrs->L2size,
 			    align_size, &pg_tbl_pa);
 	pPtAttrs->L2TblAllocPa = pg_tbl_pa;
-- 
1.6.0.4

[PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>

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.

Please refer to:
https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf

Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
---
 drivers/dsp/bridge/rmgr/proc.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 59073dd..437c3fe 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -159,6 +159,8 @@
 #define PWR_TIMEOUT	 500	/* Sleep/wake timout in msec */
 #define EXTEND	      "_EXT_END"	/* Extmem end addr in DSP binary */
 
+#define DSP_CACHE_SIZE 128 /* DSP cacheline size */
+
 extern char *iva_img;
 /* The PROC_OBJECT structure.   */
 struct PROC_OBJECT {
@@ -722,6 +724,13 @@ DSP_STATUS PROC_FlushMemory(DSP_HPROCESSOR hProcessor, void *pMpuAddr,
 	enum DSP_FLUSHTYPE FlushMemType = PROC_WRITEBACK_INVALIDATE_MEM;
 	DBC_Require(cRefs > 0);
 
+	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
+	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
+		pr_err("%s: Invalid alignment %p %x\n",
+		       __func__, pMpuAddr, ulSize);
+		return DSP_EALIGNMENT;
+	}
+
 	GT_4trace(PROC_DebugMask, GT_ENTER,
 		 "Entered PROC_FlushMemory, args:\n\t"
 		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x, ulFlags 0x%x\n",
@@ -753,6 +762,14 @@ DSP_STATUS PROC_InvalidateMemory(DSP_HPROCESSOR hProcessor, void *pMpuAddr,
 		 "Entered PROC_InvalidateMemory, args:\n\t"
 		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x\n", hProcessor,
 		 pMpuAddr, ulSize);
+
+	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
+	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
+		pr_err("%s: Invalid alignment %p %x\n",
+		       __func__, pMpuAddr, ulSize);
+		return DSP_EALIGNMENT;
+	}
+
 	(void)SYNC_EnterCS(hProcLock);
 	MEM_FlushCache(pMpuAddr, ulSize, FlushMemType);
 	(void)SYNC_LeaveCS(hProcLock);
-- 
1.6.0.4

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

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

[Hari K] - Even with the above change, this might still be a hack for time being until the flushing of the user space buffers is moved from the Bridge driver to User-mode.


Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> Sent: Sunday, May 10, 2009 11:55 PM
> To: Kanigeri, Hari
> Cc: Menon, Nishanth; linux-omap@vger.kernel.org; Hiroshi DOYU
> Subject: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp
> cache line size
> 
> From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> 
> 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.
> 
> Please refer to:
> https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf
> 
> Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> ---
>  drivers/dsp/bridge/rmgr/proc.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/dsp/bridge/rmgr/proc.c
> b/drivers/dsp/bridge/rmgr/proc.c
> index 59073dd..437c3fe 100644
> --- a/drivers/dsp/bridge/rmgr/proc.c
> +++ b/drivers/dsp/bridge/rmgr/proc.c
> @@ -159,6 +159,8 @@
>  #define PWR_TIMEOUT	 500	/* Sleep/wake timout in msec */
>  #define EXTEND	      "_EXT_END"	/* Extmem end addr in DSP binary */
> 
> +#define DSP_CACHE_SIZE 128 /* DSP cacheline size */
> +
>  extern char *iva_img;
>  /* The PROC_OBJECT structure.   */
>  struct PROC_OBJECT {
> @@ -722,6 +724,13 @@ DSP_STATUS PROC_FlushMemory(DSP_HPROCESSOR
> hProcessor, void *pMpuAddr,
>  	enum DSP_FLUSHTYPE FlushMemType = PROC_WRITEBACK_INVALIDATE_MEM;
>  	DBC_Require(cRefs > 0);
> 
> +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> +		pr_err("%s: Invalid alignment %p %x\n",
> +		       __func__, pMpuAddr, ulSize);
> +		return DSP_EALIGNMENT;
> +	}
> +
>  	GT_4trace(PROC_DebugMask, GT_ENTER,
>  		 "Entered PROC_FlushMemory, args:\n\t"
>  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x, ulFlags
> 0x%x\n",
> @@ -753,6 +762,14 @@ DSP_STATUS PROC_InvalidateMemory(DSP_HPROCESSOR
> hProcessor, void *pMpuAddr,
>  		 "Entered PROC_InvalidateMemory, args:\n\t"
>  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x\n", hProcessor,
>  		 pMpuAddr, ulSize);
> +
> +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> +		pr_err("%s: Invalid alignment %p %x\n",
> +		       __func__, pMpuAddr, ulSize);
> +		return DSP_EALIGNMENT;
> +	}
> +
>  	(void)SYNC_EnterCS(hProcLock);
>  	MEM_FlushCache(pMpuAddr, ulSize, FlushMemType);
>  	(void)SYNC_LeaveCS(hProcLock);
> --
> 1.6.0.4
> 

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

Hi Hari,

From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
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.


> 
> [Hari K] - Even with the above change, this might still be a hack for time being until the flushing of the user space buffers is moved from the Bridge driver to User-mode.
> 
> 
> Thank you,
> Best regards,
> Hari
> 
> > -----Original Message-----
> > From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> > Sent: Sunday, May 10, 2009 11:55 PM
> > To: Kanigeri, Hari
> > Cc: Menon, Nishanth; linux-omap@vger.kernel.org; Hiroshi DOYU
> > Subject: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp
> > cache line size
> > 
> > From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> > 
> > 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.
> > 
> > Please refer to:
> > https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf
> > 
> > Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> > ---
> >  drivers/dsp/bridge/rmgr/proc.c |   17 +++++++++++++++++
> >  1 files changed, 17 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/dsp/bridge/rmgr/proc.c
> > b/drivers/dsp/bridge/rmgr/proc.c
> > index 59073dd..437c3fe 100644
> > --- a/drivers/dsp/bridge/rmgr/proc.c
> > +++ b/drivers/dsp/bridge/rmgr/proc.c
> > @@ -159,6 +159,8 @@
> >  #define PWR_TIMEOUT	 500	/* Sleep/wake timout in msec */
> >  #define EXTEND	      "_EXT_END"	/* Extmem end addr in DSP binary */
> > 
> > +#define DSP_CACHE_SIZE 128 /* DSP cacheline size */
> > +
> >  extern char *iva_img;
> >  /* The PROC_OBJECT structure.   */
> >  struct PROC_OBJECT {
> > @@ -722,6 +724,13 @@ DSP_STATUS PROC_FlushMemory(DSP_HPROCESSOR
> > hProcessor, void *pMpuAddr,
> >  	enum DSP_FLUSHTYPE FlushMemType = PROC_WRITEBACK_INVALIDATE_MEM;
> >  	DBC_Require(cRefs > 0);
> > 
> > +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> > +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> > +		pr_err("%s: Invalid alignment %p %x\n",
> > +		       __func__, pMpuAddr, ulSize);
> > +		return DSP_EALIGNMENT;
> > +	}
> > +
> >  	GT_4trace(PROC_DebugMask, GT_ENTER,
> >  		 "Entered PROC_FlushMemory, args:\n\t"
> >  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x, ulFlags
> > 0x%x\n",
> > @@ -753,6 +762,14 @@ DSP_STATUS PROC_InvalidateMemory(DSP_HPROCESSOR
> > hProcessor, void *pMpuAddr,
> >  		 "Entered PROC_InvalidateMemory, args:\n\t"
> >  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x\n", hProcessor,
> >  		 pMpuAddr, ulSize);
> > +
> > +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> > +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> > +		pr_err("%s: Invalid alignment %p %x\n",
> > +		       __func__, pMpuAddr, ulSize);
> > +		return DSP_EALIGNMENT;
> > +	}
> > +
> >  	(void)SYNC_EnterCS(hProcLock);
> >  	MEM_FlushCache(pMpuAddr, ulSize, FlushMemType);
> >  	(void)SYNC_LeaveCS(hProcLock);
> > --
> > 1.6.0.4
> > 
> 

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

Hi Doyu-san,

> 
> Hi Hari,
> 
> From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> 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.  

-- Why would there be memory corruption if the OMX components did the 128 bytes padding? The padding is dummy region which is not used by any other process.

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

-- I agree with you. I am just cautious to push this change by ensuring it doesn't break any other MM component functionality. 
> 
> 
> >
> > [Hari K] - Even with the above change, this might still be a hack for
> time being until the flushing of the user space buffers is moved from the
> Bridge driver to User-mode.
> >
> >
> > Thank you,
> > Best regards,
> > Hari
> >
> > > -----Original Message-----
> > > From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> > > Sent: Sunday, May 10, 2009 11:55 PM
> > > To: Kanigeri, Hari
> > > Cc: Menon, Nishanth; linux-omap@vger.kernel.org; Hiroshi DOYU
> > > Subject: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for
> dsp
> > > cache line size
> > >
> > > From: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> > >
> > > 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.
> > >
> > > Please refer to:
> > >
> https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf
> > >
> > > Signed-off-by: Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
> > > ---
> > >  drivers/dsp/bridge/rmgr/proc.c |   17 +++++++++++++++++
> > >  1 files changed, 17 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/dsp/bridge/rmgr/proc.c
> > > b/drivers/dsp/bridge/rmgr/proc.c
> > > index 59073dd..437c3fe 100644
> > > --- a/drivers/dsp/bridge/rmgr/proc.c
> > > +++ b/drivers/dsp/bridge/rmgr/proc.c
> > > @@ -159,6 +159,8 @@
> > >  #define PWR_TIMEOUT	 500	/* Sleep/wake timout in msec */
> > >  #define EXTEND	      "_EXT_END"	/* Extmem end addr in DSP binary
> */
> > >
> > > +#define DSP_CACHE_SIZE 128 /* DSP cacheline size */
> > > +
> > >  extern char *iva_img;
> > >  /* The PROC_OBJECT structure.   */
> > >  struct PROC_OBJECT {
> > > @@ -722,6 +724,13 @@ DSP_STATUS PROC_FlushMemory(DSP_HPROCESSOR
> > > hProcessor, void *pMpuAddr,
> > >  	enum DSP_FLUSHTYPE FlushMemType = PROC_WRITEBACK_INVALIDATE_MEM;
> > >  	DBC_Require(cRefs > 0);
> > >
> > > +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> > > +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> > > +		pr_err("%s: Invalid alignment %p %x\n",
> > > +		       __func__, pMpuAddr, ulSize);
> > > +		return DSP_EALIGNMENT;
> > > +	}
> > > +
> > >  	GT_4trace(PROC_DebugMask, GT_ENTER,
> > >  		 "Entered PROC_FlushMemory, args:\n\t"
> > >  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x, ulFlags
> > > 0x%x\n",
> > > @@ -753,6 +762,14 @@ DSP_STATUS PROC_InvalidateMemory(DSP_HPROCESSOR
> > > hProcessor, void *pMpuAddr,
> > >  		 "Entered PROC_InvalidateMemory, args:\n\t"
> > >  		 "hProcessor: 0x%x pMpuAddr: 0x%x ulSize 0x%x\n", hProcessor,
> > >  		 pMpuAddr, ulSize);
> > > +
> > > +	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_SIZE) ||
> > > +	    !IS_ALIGNED(ulSize, DSP_CACHE_SIZE)) {
> > > +		pr_err("%s: Invalid alignment %p %x\n",
> > > +		       __func__, pMpuAddr, ulSize);
> > > +		return DSP_EALIGNMENT;
> > > +	}
> > > +
> > >  	(void)SYNC_EnterCS(hProcLock);
> > >  	MEM_FlushCache(pMpuAddr, ulSize, FlushMemType);
> > >  	(void)SYNC_LeaveCS(hProcLock);
> > > --
> > > 1.6.0.4
> > >
> >

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Mon, May 11, 2009 at 6:47 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> Hi Hari,
>
> From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> 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.

Also, you are not checking for the upper limit, which also should be
128-byte aligned.

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Mon, May 11, 2009 at 7:26 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Hi Doyu-san,
>
>>
>> Hi Hari,
>>
>> From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
>> 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.
>
> -- Why would there be memory corruption if the OMX components did the 128 bytes padding? The padding is dummy region which is not used by any other process.

It's the kernel's job to enforce proper usage of memory, it should not
blindly trust that user-space is doing the right thing.

-- 
Felipe Contreras

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
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@nokia.com> wrote:
> > Hi Hari,
> >
> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Mon, May 11, 2009 at 8:35 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> 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@nokia.com> wrote:
>> > Hi Hari,
>> >
>> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
>> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

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@gmail.com]
> Sent: Monday, May 11, 2009 12:54 PM
> To: Hiroshi DOYU
> Cc: Kanigeri, Hari; Menon, Nishanth; linux-omap@vger.kernel.org
> 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@nokia.com>
> wrote:
> > From: ext Felipe Contreras <felipe.contreras@gmail.com>
> > 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@nokia.com>
> wrote:
> >> > Hi Hari,
> >> >
> >> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> >> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Mon, May 11, 2009 at 9:26 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> 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.

First of all the check is not required on input buffers, enforcing it
will make memory copy unavoidable on certain situations. That is not
good.

Also, it will break OMX components. The fix is trivial (use
posix_memalign for example) but code will need to be changed.

The easiest solution IMHO is to add a new ioctl which has other advantages.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

Felipe,

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

-- How are you planning to inform LCML about choosing this new ioctl as opposed to the regular one? Any ways, I think the ulMapAttr flag in DSPProcessor_Map function can be extended to provide the information about read-only or write-only. The only issue is with this we can enforce the checks on only the applications that set this check and it will not cover the buffer alignment issues with the applications that don't enforce this check.

>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.
>
-- What does PROC_ReserveMemory has to do with pa ?

Thank you,
Best regards,
Hari

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
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.

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

> Also, another reason is the flush
> function in Bridge driver is going to be removed eventually.

Maybe this should be discussed separately from the current topic.

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

This modification impact is a compatibility issue with the existing
S/W. Fixing it is the way to go rather than working around the
potential risk that userland can cause kernel crash / memory
corruption.

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com] 
> Sent: Tuesday, May 12, 2009 12:39 AM
> To: Kanigeri, Hari
> Cc: felipe.contreras@gmail.com; Menon, Nishanth; 
> linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte 
> alignment for dsp cache line size
> 
> From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> 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.
> 

-- pMpuAddr that is passed to the PROC_Map is not aligned on 4KB boundary.
The alignemnt is taken care within the function. If this function is passed
the information of the buffer is read-only (from MPU point of view) then we
can enforce the cache alignment check. 


> 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".
> 
> > Also, another reason is the flush
> > function in Bridge driver is going to be removed eventually.
> 
> Maybe this should be discussed separately from the current topic.
> 
> > 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.
> 
> This modification impact is a compatibility issue with the 
> existing S/W. Fixing it is the way to go rather than working 
> around the potential risk that userland can cause kernel 
> crash / memory corruption.
> 
> 

-- I agree with you that fixing is the way to go, but I think we should
give some transition time for the applications to adapt to this change.

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Tue, May 12, 2009 at 2:21 AM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
> Felipe,
>
>>
>> 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.
>
> -- How are you planning to inform LCML about choosing this new ioctl as opposed to the regular one? Any ways, I think the ulMapAttr flag in DSPProcessor_Map function can be extended to provide the information about read-only or write-only. The only issue is with this we can enforce the checks on only the applications that set this check and it will not cover the buffer alignment issues with the applications that don't enforce this check.

Inform?

LCML can use the new ioctl by simply using it:
ioctl(handle, <new id>, <new arg>)

>>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.
>>
> -- What does PROC_ReserveMemory has to do with pa ?

Sorry, I don't know what kind of address PROC_ReserveMemory returns
(va?), but I don't see any point in having PROC_ReserveMemory and
PROC_Map as separate ioctls


-- 
Felipe Contreras

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Tue, May 12, 2009 at 8:38 AM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> 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!

Even mmap has PROT_READ and PROT_WRITE, is that unnecessary complexity?

[1] http://github.com/felipec/dsp-dummy/blob/9806fd01c3bf8129d27a673086d286011565fc2c/dmm_buffer.h

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
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@nokia.com> wrote:
> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Wed, May 13, 2009 at 12:10 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> 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@nokia.com> wrote:
>> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
>> > 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.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
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@nokia.com> wrote:
> > From: ext Felipe Contreras <felipe.contreras@gmail.com>
> > 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@nokia.com> wrote:
> >> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> >> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Wed, May 13, 2009 at 2:32 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> 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@nokia.com> wrote:
>> > From: ext Felipe Contreras <felipe.contreras@gmail.com>
>> > 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@nokia.com> wrote:
>> >> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
>> >> > 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).

Let's suppose that's true, if you enforce ARM cache flushes to be
aligned (pattern 1) the DSP cache flushes (pattern 2) can still
corrupt the memory. Therefore even with your check the potential for
memory corruption is still there.

But it doesn't seem it's true; I modified my dsp-dummy test to detect
memory corruption by constantly modifying the data before and after
the buffer sent to the DSP. Map, Flush and Invalidate are unaligned in
the ARM side, and not surprisingly there's memory corruption. *But* if
I modify the DSP socket node to don't flush/invalidate the first/last
128 bytes of the buffer then there's no memory corruption.

So that's proof Flush/Invalidate on ARM side don't cause memory
corruption. Do you want to see the code?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Hiroshi DOYU]

From: ext Felipe Contreras <felipe.contreras@gmail.com>
Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
Date: Wed, 13 May 2009 14:50:26 +0200

[...]

> >> >> 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).
> 
> Let's suppose that's true, if you enforce ARM cache flushes to be
> aligned (pattern 1) the DSP cache flushes (pattern 2) can still
> corrupt the memory. Therefore even with your check the potential for
> memory corruption is still there.
> 
> But it doesn't seem it's true; I modified my dsp-dummy test to detect

We are not talking about the specific application program/case, but
the general risk that kernel can be crashed by userland with incorrect
parameters in dspbridge.

> memory corruption by constantly modifying the data before and after
> the buffer sent to the DSP. Map, Flush and Invalidate are unaligned in
> the ARM side, and not surprisingly there's memory corruption. *But* if
> I modify the DSP socket node to don't flush/invalidate the first/last
> 128 bytes of the buffer then there's no memory corruption.
> 
> So that's proof Flush/Invalidate on ARM side don't cause memory
> corruption.

No, the above doesn't prove that ARM side doesn't cause memory
corruption(pattern 1).

Currently dspbridge doesn't have any parameter checking/validation
from "bridge_ioctl()" to "v7_dma_*_range()". *Any* value/address can
be passed to "v7_dma_*_range()" from userland. It means that any data
which resides on cache can be , for exmaple, invalidated by userland
spontaneously. That is why I mentioned the necessity of "VMA" checking
to prevent this.

For pattern 2, if a buffer is aligned on DSP cache size, then just
adding VMA checking can prevent userland from corrupting other
process/kernel address space. Also same kind of VMA checking for DSP
mmaping can prevent other cases too. So I think that, at least, we can
protect kernel address from crash with having VMA and alignment
checking. I'll work on that if we can agree on this.

> Do you want to see the code?

Yes, I want!! it would be quite helpful to test something easily;)

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Wed, May 13, 2009 at 5:18 PM, Hiroshi DOYU <Hiroshi.DOYU@nokia.com> wrote:
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
> Date: Wed, 13 May 2009 14:50:26 +0200
>
> [...]
>
>> >> >> 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).
>>
>> Let's suppose that's true, if you enforce ARM cache flushes to be
>> aligned (pattern 1) the DSP cache flushes (pattern 2) can still
>> corrupt the memory. Therefore even with your check the potential for
>> memory corruption is still there.
>>
>> But it doesn't seem it's true; I modified my dsp-dummy test to detect
>
> We are not talking about the specific application program/case, but
> the general risk that kernel can be crashed by userland with incorrect
> parameters in dspbridge.

The purpose of the test application is to quickly test different
scenarios... tell me which scenario will trigger the corruption you
are talking about, and I'll modify the test app to reproduce it.

>> memory corruption by constantly modifying the data before and after
>> the buffer sent to the DSP. Map, Flush and Invalidate are unaligned in
>> the ARM side, and not surprisingly there's memory corruption. *But* if
>> I modify the DSP socket node to don't flush/invalidate the first/last
>> 128 bytes of the buffer then there's no memory corruption.
>>
>> So that's proof Flush/Invalidate on ARM side don't cause memory
>> corruption.
>
> No, the above doesn't prove that ARM side doesn't cause memory
> corruption(pattern 1).
>
> Currently dspbridge doesn't have any parameter checking/validation
> from "bridge_ioctl()" to "v7_dma_*_range()". *Any* value/address can
> be passed to "v7_dma_*_range()" from userland. It means that any data
> which resides on cache can be , for exmaple, invalidated by userland
> spontaneously. That is why I mentioned the necessity of "VMA" checking
> to prevent this.

If user-space manually invalidates some random memory areas
spontaneously what's the worst that could happen? Bad performance?

The problem with corruption is not the cache invalidation, it's the
contents of the main memory.

> For pattern 2, if a buffer is aligned on DSP cache size, then just
> adding VMA checking can prevent userland from corrupting other
> process/kernel address space. Also same kind of VMA checking for DSP
> mmaping can prevent other cases too. So I think that, at least, we can
> protect kernel address from crash with having VMA and alignment
> checking. I'll work on that if we can agree on this.

pattern 2 is triggered by the software running on the DSP. Why are you
talking about VMA stuff that happens in the linux kernel on *ARM*
side?

>> Do you want to see the code?
>
> Yes, I want!! it would be quite helpful to test something easily;)

Ok, I pushed a branch:
http://github.com/felipec/dsp-dummy/tree/corruption-test

Now, I ran a few more tests and it turns out 'pattern 2' is not enough
to trigger the corruption, but neither is 'pattern 1'. The corruption
happens only when both happen at the same time.

So, the DSP writes on a wrong memory area, then flushes the cache, so
it goes the main memory, but the ARM side already updated the memory
on it's cache and it's different from the main memory. Then ARM side
invalidates it's cache and *bang* the corruption happens.

Now is ARM cache invalidation the problem? No, it's the DSP cache
flush. Even if there isn't a manual ARM cache invalidation the cache
will receive the main memory update at some point, wouldn't it? Maybe
a cache miss.

-- 
Felipe Contreras

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

> >>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.
> >>
> > -- What does PROC_ReserveMemory has to do with pa ?
> 
> Sorry, I don't know what kind of address PROC_ReserveMemory returns
> (va?), but I don't see any point in having PROC_ReserveMemory and
> PROC_Map as separate ioctls
>

-- The purpose of PROC_ReserveMemory is for the Bridge clients to grab DSP VA memory one time and use the PROC_MapMemory to map the address within this
reserved memory. So, in a way with proper usage of PROC_ReserveMemory, it should be called only one time and subsequent Map/Unmaps from a client should happen within this memory region. By following this, you can eliminate the overhead of calling redundant reserve/unreserve calls that go into the driver.

Thank you,
Best regards,
Hari

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

Few reasons as why the check shouldn't be in DSP flush functions.

	- Clients might forget to call flush call
	
	- In some cases you are not required to call the flush calls as you might map to a non-cacheable memory for optimization purposes.

	- DSP Bridge flush functions will be removed from Driver.

If we go with a check, the check should be in the Map function as this is a required call for all Bridge clients.

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Hiroshi DOYU [mailto:Hiroshi.DOYU@nokia.com]
> Sent: Wednesday, May 13, 2009 6:33 AM
> To: felipe.contreras@gmail.com
> Cc: Kanigeri, Hari; Menon, Nishanth; linux-omap@vger.kernel.org; Menon,
> Nishanth
> Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for
> dsp cache line size
> 
> From: ext Felipe Contreras <felipe.contreras@gmail.com>
> 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@nokia.com>
> wrote:
> > > From: ext Felipe Contreras <felipe.contreras@gmail.com>
> > > 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@nokia.com> wrote:
> > >> > From: "ext Kanigeri, Hari" <h-kanigeri2@ti.com>
> > >> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Fri, May 15, 2009 at 5:54 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
>> >>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.
>> >>
>> > -- What does PROC_ReserveMemory has to do with pa ?
>>
>> Sorry, I don't know what kind of address PROC_ReserveMemory returns
>> (va?), but I don't see any point in having PROC_ReserveMemory and
>> PROC_Map as separate ioctls
>>
>
> -- The purpose of PROC_ReserveMemory is for the Bridge clients to grab DSP VA memory one time and use the PROC_MapMemory to map the address within this
> reserved memory. So, in a way with proper usage of PROC_ReserveMemory, it should be called only one time and subsequent Map/Unmaps from a client should happen within this memory region. By following this, you can eliminate the overhead of calling redundant reserve/unreserve calls that go into the driver.

Do you know of any client that doing ReseveMemory and Map independently?
How much overhead is there in ReserveMemory?
What happens if the Map size is different than the ReserveMemory? What
happens if the size is bigger?

In fact I don't even understand what is DSP VA memory. Is that virtual
memory? What meaning is there on virtual memory that has no physical
memory?

-- 
Felipe Contreras

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

> Do you know of any client that doing ReseveMemory and Map independently?
> How much overhead is there in ReserveMemory?
> What happens if the Map size is different than the ReserveMemory? What
> happens if the size is bigger?

-- I cannot disclose the name, but it is some major Mobile Company that is following this approach. One big chunk of DSP Virtual address region is grabbed during boot time, and after that all the maps/unmaps are managed from this memory region.

> How much overhead is there in ReserveMemory?
-- The overhead is the ioctls to the Bridge driver and the search that is involved in searching for the DSP virtual address region that meets the size requirement.

> What happens if the Map size is different than the ReserveMemory? What
> happens if the size is bigger?

-- The onus will be on the Client that reserved the memory to make sure that the size to map is within the reserved memory block.

> In fact I don't even understand what is DSP VA memory. Is that virtual
> memory? 
-- Yes, that is the DSP Virtual address.  

> What meaning is there on virtual memory that has no physical
> memory?

-- The mapping to the physical address is done with the DSPProcessor_Map function call. 

Thank you,
Best regards,
Hari

> -----Original Message-----
> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Friday, May 15, 2009 11:59 AM
> To: Kanigeri, Hari
> Cc: Hiroshi DOYU; Menon, Nishanth; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for
> dsp cache line size
> 
> On Fri, May 15, 2009 at 5:54 PM, Kanigeri, Hari <h-kanigeri2@ti.com>
> wrote:
> >> >>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.
> >> >>
> >> > -- What does PROC_ReserveMemory has to do with pa ?
> >>
> >> Sorry, I don't know what kind of address PROC_ReserveMemory returns
> >> (va?), but I don't see any point in having PROC_ReserveMemory and
> >> PROC_Map as separate ioctls
> >>
> >
> > -- The purpose of PROC_ReserveMemory is for the Bridge clients to grab
> DSP VA memory one time and use the PROC_MapMemory to map the address
> within this
> > reserved memory. So, in a way with proper usage of PROC_ReserveMemory,
> it should be called only one time and subsequent Map/Unmaps from a client
> should happen within this memory region. By following this, you can
> eliminate the overhead of calling redundant reserve/unreserve calls that
> go into the driver.
> 
> Do you know of any client that doing ReseveMemory and Map independently?
> How much overhead is there in ReserveMemory?
> What happens if the Map size is different than the ReserveMemory? What
> happens if the size is bigger?
> 
> In fact I don't even understand what is DSP VA memory. Is that virtual
> memory? What meaning is there on virtual memory that has no physical
> memory?
> 
> --
> Felipe Contreras

Re: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Felipe Contreras]

On Fri, May 15, 2009 at 8:32 PM, Kanigeri, Hari <h-kanigeri2@ti.com> wrote:
>> Do you know of any client that doing ReseveMemory and Map independently?
>> How much overhead is there in ReserveMemory?
>> What happens if the Map size is different than the ReserveMemory? What
>> happens if the size is bigger?
>
> -- I cannot disclose the name, but it is some major Mobile Company that is following this approach. One big chunk of DSP Virtual address region is grabbed during boot time, and after that all the maps/unmaps are managed from this memory region.

On Linux?

So the DSP VA passed to Map would be v + chunk_size * i?

>> How much overhead is there in ReserveMemory?
> -- The overhead is the ioctls to the Bridge driver and the search that is involved in searching for the DSP virtual address region that meets the size requirement.

Search a DSP virtual memory region from where?

>> What happens if the Map size is different than the ReserveMemory? What
>> happens if the size is bigger?
>
> -- The onus will be on the Client that reserved the memory to make sure that the size to map is within the reserved memory block.

Aha! Yet another place where the kernel is letting user-space screw up
the system.

>> In fact I don't even understand what is DSP VA memory. Is that virtual
>> memory?
> -- Yes, that is the DSP Virtual address.
>
>> What meaning is there on virtual memory that has no physical
>> memory?
>
> -- The mapping to the physical address is done with the DSPProcessor_Map function call.

Yes, but before the Map function the virtual memory has no meaning.

-- 
Felipe Contreras

RE: [PATCH 2/2] DSPBRIDGE: add checking 128 byte alignment for dsp cache line size

[Kanigeri, Hari]

Hi Felipe,

> > -- I cannot disclose the name, but it is some major Mobile Company that
> is following this approach. One big chunk of DSP Virtual address region is
> grabbed during boot time, and after that all the maps/unmaps are managed
> from this memory region.
> 
> On Linux?

-- Yes.

> So the DSP VA passed to Map would be v + chunk_size * i?

-- Yes. 

> 
> Search a DSP virtual memory region from where?

-- Bridge internally does the management of this virtual memory pool. It might get fragmented over time, so based on the size that is requested it will loop through the pool to find the requested continguous DSP VA region.

> > -- The onus will be on the Client that reserved the memory to make sure
> that the size to map is within the reserved memory block.
> 
> Aha! Yet another place where the kernel is letting user-space screw up
> the system.

-- :) :). It is a choice between either
	a. Provide too much configurable options to the Users that they can optimize according to their needs and requirements.
	b. Don't provide any configurable options to Users and acting like a Black box. 
With option a. at the benefit of getting configurable option the Users can also use the configurable options to screw up the System :). Examples of (a) are TI's MMU and Mailbox modules and (b) is the IOMMU module and Mailbox module in LO tree. 
But any ways, coming to Map question, if the User provide size to map greater than reserved memory, the map function fails as this was never reserved. Or it might screw up other Client's reserved memory as this Map request might fall in the other reserved memory region.

> > -- The mapping to the physical address is done with the DSPProcessor_Map
> function call.
> 
> Yes, but before the Map function the virtual memory has no meaning.
>
-- Isn't to similar to ARM's virtual memory that doesn't has any meaning until the Physical memory is assigned to it ?


Thank you,
Best regards,
Hari