[RFC][PATCH 0/2] DSPBRIDGE: 128 bytes alignment check

[RFC][PATCH 0/2] DSPBRIDGE: 128 bytes alignment check

[Omar Ramirez Luna]

Technical info:
https://omapzoom.org/gf/download/docmanfileversion/52/985/DSP_cache.pdf

This set of patches introduces the 128 byte alignment check,
needed to avoid corruption if the dsp is meant to write to
boundary portions of an unaligned chunk of memory.

The second patch uses a field composed of 2 bits to distinguish
if the mapped chunk is readable/writeable, so the check can be
performed on w/rw chunks of memory. 

Omar Ramirez Luna (2):
  DSPBRIDGE: add checking 128 byte alignment for dsp cache line size
  DSPBRIDGE: Distinguish between read or write buffers

 drivers/dsp/bridge/Kconfig     |   19 +++++++++++++++++++
 drivers/dsp/bridge/rmgr/proc.c |   20 ++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

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

[Omar Ramirez Luna]

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>
[orl: check into PROC_Map, created Kconfig option]
Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/dsp/bridge/Kconfig     |   19 +++++++++++++++++++
 drivers/dsp/bridge/rmgr/proc.c |   12 ++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/dsp/bridge/Kconfig b/drivers/dsp/bridge/Kconfig
index e494f02..646fdcb 100644
--- a/drivers/dsp/bridge/Kconfig
+++ b/drivers/dsp/bridge/Kconfig
@@ -35,6 +35,23 @@ config BRIDGE_DEBUG
 	help
 	  Say Y to enable Bridge debugging capabilities
 
+menu "Bridge Hacking"
+	depends on MPU_BRIDGE
+
+config BRIDGE_CACHE_LINE_CHECK
+	bool "Check buffers to be 128 byte aligned"
+	depends on MPU_BRIDGE
+	default n
+	help
+	  When the DSP processes data, the DSP cache controller loads 128-Byte
+	  chunks (lines) from SDRAM and writes the data back in 128-Byte chunks.
+	  If a DMM buffer does not start and end on a 128-Byte boundary, the data
+	  preceding the start address (SA) from the 128-Byte boundary to the SA
+	  and the data at addresses trailing the end address (EA) from the EA to
+	  the next 128-Byte boundary will be loaded and written back as well.
+	  This can lead to heap corruption. Say Y, to enforce the check for 128
+	  byte alignment, buffers failing this check will be rejected.
+
 comment "Bridge Notifications"
 	depends on MPU_BRIDGE
 
@@ -45,3 +62,5 @@ config BRIDGE_NTFY_PWRERR
 	  Enable notifications to registered clients on the event of power errror
 	  trying to suspend bridge driver. Say Y, to signal this event as a fatal
 	  error, this will require a bridge restart to recover.
+
+endmenu
diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 6c0173a..78a31ef 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -69,6 +69,8 @@
 #define PWR_TIMEOUT	 500	/* Sleep/wake timout in msec */
 #define EXTEND	      "_EXT_END"	/* Extmem end addr in DSP binary */
 
+#define DSP_CACHE_LINE 128
+
 extern char *iva_img;
 
 /*  ----------------------------------- Globals */
@@ -1293,6 +1295,16 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
 		 "hProcessor %x, pMpuAddr %x, ulSize %x, pReqAddr %x, "
 		 "ulMapAttr %x, ppMapAddr %x\n", hProcessor, pMpuAddr, ulSize,
 		 pReqAddr, ulMapAttr, ppMapAddr);
+
+#ifdef CONFIG_BRIDGE_CACHE_LINE_CHECK
+	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
+	    !IS_ALIGNED(size, DSP_CACHE_LINE)) {
+		pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
+						(u32)pMpuAddr, ulSize);
+		return -EFAULT;
+	}
+#endif
+
 	/* 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);
-- 
1.6.2.4

[RFC][PATCH 2/2] DSPBRIDGE: Distinguish between read or write buffers

[Omar Ramirez Luna]

This patch introduces the check to differentiate the buffers
coming to the dsp through bridgedriver. So far they can be
input (read) or output (write) or rw (which are treated the
same way as an output buffer), this distinctions are made from
dsp perspective.

Since this needs to be checked on map function, unused
bits (16, 15) of flags were used to check for this argument.

As 128 byte alignment limitation doesn't affect input buffers
only writable buffers are checked. Default value for read buffers
is set to be 1, this will enforce that users of bridge will fill
the flags with significant values otherwise (if enabled) check
will reject buffers not aligned to 128 bytes (even if they fall in
the input category).

Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
---
 drivers/dsp/bridge/rmgr/proc.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
index 78a31ef..4fe20ed 100644
--- a/drivers/dsp/bridge/rmgr/proc.c
+++ b/drivers/dsp/bridge/rmgr/proc.c
@@ -71,6 +71,12 @@
 
 #define DSP_CACHE_LINE 128
 
+#define BUFMODE_MASK	(3 << 14)
+
+/* Buffer modes from DSP perspective */
+#define RBUF		0x1	/* Input buffer */
+#define WBUF		0x2	/* Output Buffer */
+
 extern char *iva_img;
 
 /*  ----------------------------------- Globals */
@@ -1297,11 +1303,13 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
 		 pReqAddr, ulMapAttr, ppMapAddr);
 
 #ifdef CONFIG_BRIDGE_CACHE_LINE_CHECK
-	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
-	    !IS_ALIGNED(size, DSP_CACHE_LINE)) {
-		pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
+	if ((ulMapAttr & BUFMODE_MASK) != RBUF) {
+		if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
+		    !IS_ALIGNED(ulSize, DSP_CACHE_LINE)) {
+			pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
 						(u32)pMpuAddr, ulSize);
-		return -EFAULT;
+			return -EFAULT;
+		}
 	}
 #endif
 
-- 
1.6.2.4

Re: [RFC][PATCH 2/2] DSPBRIDGE: Distinguish between read or write buffers

[Deepak Chitriki]

Ramirez Luna, Omar wrote:
> This patch introduces the check to differentiate the buffers
> coming to the dsp through bridgedriver. So far they can be
> input (read) or output (write) or rw (which are treated the
> same way as an output buffer), this distinctions are made from
> dsp perspective.
>
> Since this needs to be checked on map function, unused
> bits (16, 15) of flags were used to check for this argument.
>
> As 128 byte alignment limitation doesn't affect input buffers
> only writable buffers are checked. Default value for read buffers
> is set to be 1, this will enforce that users of bridge will fill
> the flags with significant values otherwise (if enabled) check
> will reject buffers not aligned to 128 bytes (even if they fall in
> the input category).
>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@ti.com>
> ---
>  drivers/dsp/bridge/rmgr/proc.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
> index 78a31ef..4fe20ed 100644
> --- a/drivers/dsp/bridge/rmgr/proc.c
> +++ b/drivers/dsp/bridge/rmgr/proc.c
> @@ -71,6 +71,12 @@
>  
>  #define DSP_CACHE_LINE 128
>  
> +#define BUFMODE_MASK	(3 << 14)
> +
> +/* Buffer modes from DSP perspective */
> +#define RBUF		0x1	/* Input buffer */
> +#define WBUF		0x2	/* Output Buffer */
> +
>  extern char *iva_img;
>  
>  /*  ----------------------------------- Globals */
> @@ -1297,11 +1303,13 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
>  		 pReqAddr, ulMapAttr, ppMapAddr);
>  
>  #ifdef CONFIG_BRIDGE_CACHE_LINE_CHECK
> -	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
> -	    !IS_ALIGNED(size, DSP_CACHE_LINE)) {
> -		pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
> +	if ((ulMapAttr & BUFMODE_MASK) != RBUF) {
What if the result of (ulMapAttr & BUFMODE_MASK) is invalid,still it enters the loop.Since the condition is for writable buffers only,Isn't it better to use "if ((ulMapAttr & BUFMODE_MASK) == WBUF)" condition?

> +		if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
> +		    !IS_ALIGNED(ulSize, DSP_CACHE_LINE)) {
> +			pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
>  						(u32)pMpuAddr, ulSize);
> -		return -EFAULT;
> +			return -EFAULT;
> +		}
>  	}
>  #endif
Regards,
Deepak

>  
>   

Re: [RFC][PATCH 2/2] DSPBRIDGE: Distinguish between read or write buffers

[Omar Ramirez Luna]

On 2/19/2010 7:44 AM, Chitriki Rudramuni, Deepak wrote:
> Ramirez Luna, Omar wrote:
>> This patch introduces the check to differentiate the buffers
>> coming to the dsp through bridgedriver. So far they can be
>> input (read) or output (write) or rw (which are treated the
>> same way as an output buffer), this distinctions are made from
>> dsp perspective.
>>
>> Since this needs to be checked on map function, unused
>> bits (16, 15) of flags were used to check for this argument.
>>
>> As 128 byte alignment limitation doesn't affect input buffers
>> only writable buffers are checked. Default value for read buffers
>> is set to be 1, this will enforce that users of bridge will fill
>> the flags with significant values otherwise (if enabled) check
>> will reject buffers not aligned to 128 bytes (even if they fall in
>> the input category).
>>
>> Signed-off-by: Omar Ramirez Luna<omar.ramirez@ti.com>
>> ---
>>   drivers/dsp/bridge/rmgr/proc.c |   16 ++++++++++++----
>>   1 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c
>> index 78a31ef..4fe20ed 100644
>> --- a/drivers/dsp/bridge/rmgr/proc.c
>> +++ b/drivers/dsp/bridge/rmgr/proc.c
>> @@ -71,6 +71,12 @@
>>
>>   #define DSP_CACHE_LINE 128
>>
>> +#define BUFMODE_MASK	(3<<  14)
>> +
>> +/* Buffer modes from DSP perspective */
>> +#define RBUF		0x1	/* Input buffer */
>> +#define WBUF		0x2	/* Output Buffer */
>> +
>>   extern char *iva_img;
>>
>>   /*  ----------------------------------- Globals */
>> @@ -1297,11 +1303,13 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize,
>>   		 pReqAddr, ulMapAttr, ppMapAddr);
>>
>>   #ifdef CONFIG_BRIDGE_CACHE_LINE_CHECK
>> -	if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
>> -	    !IS_ALIGNED(size, DSP_CACHE_LINE)) {
>> -		pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
>> +	if ((ulMapAttr&  BUFMODE_MASK) != RBUF) {
> What if the result of (ulMapAttr&  BUFMODE_MASK) is invalid,still it enters the loop.Since the condition is for writable buffers only,Isn't it better to use "if ((ulMapAttr&  BUFMODE_MASK) == WBUF)" condition?
>

Description:
"this will enforce that users of bridge will fill
the flags with significant values otherwise (if enabled) check
will reject buffers not aligned to 128 bytes (even if they fall in
the input category)"

Let say you enable the check but bits are not used (meaning they are set 
to 0), bridge doesn't have a way to distinguish if this is expected or 
if you just forgot to fill them.

Anyway, if such a strict check is not required then I guess we can use 
only 1 bit (0 = READ, 1 = WRITE).

In short: if you are enabling the check then set the proper value for 
the bit.

>> +		if (!IS_ALIGNED((u32)pMpuAddr, DSP_CACHE_LINE) ||
>> +		    !IS_ALIGNED(ulSize, DSP_CACHE_LINE)) {
>> +			pr_err("%s: not aligned: 0x%x (%d)\n", __func__,
>>   						(u32)pMpuAddr, ulSize);
>> -		return -EFAULT;
>> +			return -EFAULT;
>> +		}
>>   	}
>>   #endif
> Regards,
> Deepak
>

- omar