[Qemu-devel] [PATCH 1/2] target-mips: fix incorrect behaviour for EXTP

[Qemu-devel] [PATCH 1/2] target-mips: fix incorrect behaviour for EXTP

[Petar Jovanovic]

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

The mask for EXTP instruction when size=31 has not been correctly
calculated.

The test (mips32-dsp/extp.c) has been extended to include the case that
triggers the issue.

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

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index 9212789..a55f866 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -3415,8 +3415,7 @@ target_ulong helper_extp(target_ulong ac, target_ulong size, CPUMIPSState *env)
     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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));
         set_DSPControl_efi(0, env);
     } else {
         set_DSPControl_efi(1, env);
diff --git a/tests/tcg/mips/mips32-dsp/extp.c b/tests/tcg/mips/mips32-dsp/extp.c
index 21a67af..b18bdb3 100644
--- a/tests/tcg/mips/mips32-dsp/extp.c
+++ b/tests/tcg/mips/mips32-dsp/extp.c
@@ -40,5 +40,23 @@ int main()
     dsp = (dsp >> 14) & 0x01;
     assert(dsp == 1);
 
+    ach = 0;
+    acl = 0x80000001;
+    dsp = 0x1F;
+    result = 0x80000001;
+
+    __asm
+        ("wrdsp %1\n\t"
+         "mthi %2, $ac2\n\t"
+         "mtlo %3, $ac2\n\t"
+         "extp %0, $ac2, 0x1F\n\t"
+         "rddsp %1\n\t"
+         : "=r"(rt), "+r"(dsp)
+         : "r"(ach), "r"(acl)
+        );
+    dsp = (dsp >> 14) & 0x01;
+    assert(dsp == 0);
+    assert(result == rt);
+
     return 0;
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] 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           |    9 ++++-----
 tests/tcg/mips/mips32-dsp/extpdp.c |   18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
index a55f866..27283ed 100644
--- a/target-mips/dsp_helper.c
+++ b/target-mips/dsp_helper.c
@@ -90,10 +90,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 +3439,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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));
 
-        set_DSPControl_pos(start_pos - (size + 1), env);
+        set_DSPControl_pos(sub & 0x3F, 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 2/2] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

[Aurelien Jarno]

On Mon, May 13, 2013 at 03:20:27PM +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           |    9 ++++-----
>  tests/tcg/mips/mips32-dsp/extpdp.c |   18 ++++++++++++++++++
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index a55f866..27283ed 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -90,10 +90,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 +3439,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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));
>  
> -        set_DSPControl_pos(start_pos - (size + 1), env);
> +        set_DSPControl_pos(sub & 0x3F, env);

I am not sure it is correct there for the 64-bit mode.

The value that should be written should be DSPcontrolpos6..0 - (size
+ 1), therefore keeeping only last 6 bits doesn't seems correct to me.
Given the change above, I think it should simply be 
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
> 
> 

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

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

[Petar Jovanovic]

________________________________________
From: Aurelien Jarno [aurelien@aurel32.net]
Sent: Friday, May 17, 2013 7:34 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic
Subject: Re: [PATCH 2/2] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

I am not sure it is correct there for the 64-bit mode.

The value that should be written should be DSPcontrolpos6..0 - (size
+ 1), therefore keeeping only last 6 bits doesn't seems correct to me.
Given the change above, I think it should simply be
set_DSPControl_pos(sub, env);

----------

You are correct. I have submitted a second version of the patch.

Petar

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

[Peter Maydell]

On 13 May 2013 14:20, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote:
> @@ -3439,10 +3439,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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));

    temp = extract64(acc, start_pos - size, size + 1);

I think?

thanks
-- PMM

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

[Petar Jovanovic]

________________________________________
From: Peter Maydell [peter.maydell@linaro.org]
Sent: Friday, May 17, 2013 8:01 PM
To: Petar Jovanovic
Cc: qemu-devel@nongnu.org; Petar Jovanovic; aurelien@aurel32.net
Subject: Re: [Qemu-devel] [PATCH 2/2] target-mips: fix EXTPDP and setting up pos field in the DSPControl reg

On 13 May 2013 14:20, Petar Jovanovic <petar.jovanovic@rt-rk.com> wrote:
> @@ -3439,10 +3439,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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));

    temp = extract64(acc, start_pos - size, size + 1);

I think?

thanks
-- PMM

________________________________________

Thanks Peter, this is indeed more readable. I have just uploaded another
patch set with the use of extract64 helper.

http://patchwork.ozlabs.org/patch/244721/

Note to Aurelien -
the #include "qemu/bitops.h" will cause a conflict if the patch 
"target-mips: clean-up in BIT_INSV" is applied first, since both patches
add that line at the top of the file.

Petar

Re: [Qemu-devel] [PATCH 1/2] target-mips: fix incorrect behaviour for EXTP

[Aurelien Jarno]

On Mon, May 13, 2013 at 03:20:26PM +0200, Petar Jovanovic wrote:
> From: Petar Jovanovic <petar.jovanovic@imgtec.com>
> 
> The mask for EXTP instruction when size=31 has not been correctly
> calculated.
> 
> The test (mips32-dsp/extp.c) has been extended to include the case that
> triggers the issue.
> 
> Signed-off-by: Petar Jovanovic <petar.jovanovic@imgtec.com>
> ---
>  target-mips/dsp_helper.c         |    3 +--
>  tests/tcg/mips/mips32-dsp/extp.c |   18 ++++++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/target-mips/dsp_helper.c b/target-mips/dsp_helper.c
> index 9212789..a55f866 100644
> --- a/target-mips/dsp_helper.c
> +++ b/target-mips/dsp_helper.c
> @@ -3415,8 +3415,7 @@ target_ulong helper_extp(target_ulong ac, target_ulong size, CPUMIPSState *env)
>      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 = (acc >> (start_pos - size)) & (~0U >> (31 - size));
>          set_DSPControl_efi(0, env);
>      } else {
>          set_DSPControl_efi(1, env);
> diff --git a/tests/tcg/mips/mips32-dsp/extp.c b/tests/tcg/mips/mips32-dsp/extp.c
> index 21a67af..b18bdb3 100644
> --- a/tests/tcg/mips/mips32-dsp/extp.c
> +++ b/tests/tcg/mips/mips32-dsp/extp.c
> @@ -40,5 +40,23 @@ int main()
>      dsp = (dsp >> 14) & 0x01;
>      assert(dsp == 1);
>  
> +    ach = 0;
> +    acl = 0x80000001;
> +    dsp = 0x1F;
> +    result = 0x80000001;
> +
> +    __asm
> +        ("wrdsp %1\n\t"
> +         "mthi %2, $ac2\n\t"
> +         "mtlo %3, $ac2\n\t"
> +         "extp %0, $ac2, 0x1F\n\t"
> +         "rddsp %1\n\t"
> +         : "=r"(rt), "+r"(dsp)
> +         : "r"(ach), "r"(acl)
> +        );
> +    dsp = (dsp >> 14) & 0x01;
> +    assert(dsp == 0);
> +    assert(result == rt);
> +
>      return 0;
>  }

Thanks, applied.

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