[Qemu-devel] [PATCH v3] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

[Qemu-devel] [PATCH v3] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

[Petar Jovanovic]

From: Petar Jovanovic <petar.jovanovic@imgtec.com>

This change makes sure that modifications of pos field in the DSPControl
register do not trash other bits in the register. This bug can be triggered
with the additional test case in mips32-dsp/extpdp.c in this commit.

In addition to this, this change corrects incorrect calculation of the mask
for EXTPDP.

Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
---
 target-mips/dsp_helper.c           |   10 +++++-----
 tests/tcg/mips/mips32-dsp/extpdp.c |   18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index a55f866..655dc8a 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -19,6 +19,7 @@
 
 #include "cpu.h"
 #include "helper.h"
+#include "qemu/bitops.h"
 
 /* As the byte ordering doesn't matter, i.e. all columns are treated
    identically, these unions can be used directly.  */
@@ -90,10 +91,10 @@ static inline void set_DSPControl_pos(uint32_t pos, CPUMIPSState *env)
     dspc = env->active_tc.DSPControl;
 #ifndef TARGET_MIPS64
     dspc = dspc & 0xFFFFFFC0;
-    dspc |= pos;
+    dspc |= (pos & 0x3F);
 #else
     dspc = dspc & 0xFFFFFF80;
-    dspc |= pos;
+    dspc |= (pos & 0x7F);
 #endif
     env->active_tc.DSPControl = dspc;
 }
@@ -3439,10 +3440,9 @@ target_ulong helper_extpdp(target_ulong ac, target_ulong size,
     if (sub >= -1) {
         acc  = ((uint64_t)env->active_tc.HI[ac] << 32) |
                ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
-        temp = (acc >> (start_pos - size)) &
-               (((uint32_t)0x01 << (size + 1)) - 1);
+        temp = extract64(acc, start_pos - size, size + 1);
 
-        set_DSPControl_pos(start_pos - (size + 1), env);
+        set_DSPControl_pos(sub, env);
         set_DSPControl_efi(0, env);
     } else {
         set_DSPControl_efi(1, env);
diff --git a/tests/tcg/mips/mips32-dsp/extpdp.c b/tests/tcg/mips/mips32-dsp/extpdp.c
index 15ba082..79ee16e 100644
--- a/tests/tcg/mips/mips32-dsp/extpdp.c
+++ b/tests/tcg/mips/mips32-dsp/extpdp.c
@@ -42,5 +42,23 @@ int main()
     efi = (dsp >> 14) & 0x01;
     assert(efi == 1);
 
+
+    ach = 0;
+    acl = 0;
+    dsp = 0;
+    result = 0;
+
+    __asm
+        ("wrdsp %1\n\t"
+         "mthi %2, $ac1\n\t"
+         "mtlo %3, $ac1\n\t"
+         "extpdp %0, $ac1, 0x00\n\t"
+         "rddsp %1\n\t"
+         : "=r"(rt), "+r"(dsp)
+         : "r"(ach), "r"(acl)
+        );
+    assert(dsp == 0x3F);
+    assert(result == rt);
+
     return 0;
 }
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v3] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

[Aurelien Jarno]

On Sat, May 18, 2013 at 03:53:41AM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> This change makes sure that modifications of pos field in the DSPControl
> register do not trash other bits in the register. This bug can be triggered
> with the additional test case in mips32-dsp/extpdp.c in this commit.
> 
> In addition to this, this change corrects incorrect calculation of the mask
> for EXTPDP.
> 
> Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
> ---
>  target-mips/dsp_helper.c           |   10 +++++-----
>  tests/tcg/mips/mips32-dsp/extpdp.c |   18 ++++++++++++++++++
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index a55f866..655dc8a 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -19,6 +19,7 @@
>  
>  #include "cpu.h"
>  #include "helper.h"
> +#include "qemu/bitops.h"
>  
>  /* As the byte ordering doesn't matter, i.e. all columns are treated
>     identically, these unions can be used directly.  */
> @@ -90,10 +91,10 @@ static inline void set_DSPControl_pos(uint32_t pos, CPUMIPSState *env)
>      dspc = env->active_tc.DSPControl;
>  #ifndef TARGET_MIPS64
>      dspc = dspc & 0xFFFFFFC0;
> -    dspc |= pos;
> +    dspc |= (pos & 0x3F);
>  #else
>      dspc = dspc & 0xFFFFFF80;
> -    dspc |= pos;
> +    dspc |= (pos & 0x7F);
>  #endif
>      env->active_tc.DSPControl = dspc;
>  }
> @@ -3439,10 +3440,9 @@ target_ulong helper_extpdp(target_ulong ac, target_ulong size,
>      if (sub >= -1) {
>          acc  = ((uint64_t)env->active_tc.HI[ac] << 32) |
>                 ((uint64_t)env->active_tc.LO[ac] & MIPSDSP_LLO);
> -        temp = (acc >> (start_pos - size)) &
> -               (((uint32_t)0x01 << (size + 1)) - 1);
> +        temp = extract64(acc, start_pos - size, size + 1);
>  
> -        set_DSPControl_pos(start_pos - (size + 1), env);
> +        set_DSPControl_pos(sub, env);
>          set_DSPControl_efi(0, env);
>      } else {
>          set_DSPControl_efi(1, env);
> diff --git a/tests/tcg/mips/mips32-dsp/extpdp.c b/tests/tcg/mips/mips32-dsp/extpdp.c
> index 15ba082..79ee16e 100644
> --- a/tests/tcg/mips/mips32-dsp/extpdp.c
> +++ b/tests/tcg/mips/mips32-dsp/extpdp.c
> @@ -42,5 +42,23 @@ int main()
>      efi = (dsp >> 14) & 0x01;
>      assert(efi == 1);
>  
> +
> +    ach = 0;
> +    acl = 0;
> +    dsp = 0;
> +    result = 0;
> +
> +    __asm
> +        ("wrdsp %1\n\t"
> +         "mthi %2, $ac1\n\t"
> +         "mtlo %3, $ac1\n\t"
> +         "extpdp %0, $ac1, 0x00\n\t"
> +         "rddsp %1\n\t"
> +         : "=r"(rt), "+r"(dsp)
> +         : "r"(ach), "r"(acl)
> +        );
> +    assert(dsp == 0x3F);
> +    assert(result == rt);
> +
>      return 0;
>  }

Thanks, applied.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net