[Qemu-devel] [PATCH] target-mips: correct DERET instruction

[Qemu-devel] [PATCH] target-mips: correct DERET instruction

[Leon Alrae]

Fix Debug Mode flag clearing, and when DERET is placed between LL and SC
do not make SC fail.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 target-mips/op_helper.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index 2a9ddff..d461ad8 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -2154,10 +2154,9 @@ void helper_deret(CPUMIPSState *env)
     debug_pre_eret(env);
     set_pc(env, env->CP0_DEPC);
 
-    env->hflags &= MIPS_HFLAG_DM;
+    env->hflags &= ~MIPS_HFLAG_DM;
     compute_hflags(env);
     debug_post_eret(env);
-    env->lladdr = 1;
 }
 #endif /* !CONFIG_USER_ONLY */
 
-- 
2.1.0

[Qemu-devel] [PATCH] target-mips: fix logically dead code reported by Coverity

[Leon Alrae]

Make use of CMPOP in floating-point compare instructions.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 target-mips/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 7302857..4a1ffdb 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -9552,6 +9552,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
             gen_cmp_s(ctx, func-48, ft, fs, cc);
             opn = condnames[func-48];
         }
+        optype = CMPOP;
         break;
     case OPC_ADD_D:
         check_cp1_registers(ctx, fs | ft | fd);
@@ -10036,6 +10037,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
             gen_cmp_d(ctx, func-48, ft, fs, cc);
             opn = condnames[func-48];
         }
+        optype = CMPOP;
         break;
     case OPC_CVT_S_D:
         check_cp1_registers(ctx, fs);
@@ -10461,6 +10463,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
             gen_cmp_ps(ctx, func-48, ft, fs, cc);
             opn = condnames[func-48];
         }
+        optype = CMPOP;
         break;
     default:
         MIPS_INVAL(opn);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] target-mips: fix logically dead code reported by Coverity

[Aurelien Jarno]

On 2015-07-14 11:08, Leon Alrae wrote:
> Make use of CMPOP in floating-point compare instructions.
> 
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>  target-mips/translate.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 7302857..4a1ffdb 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -9552,6 +9552,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
>              gen_cmp_s(ctx, func-48, ft, fs, cc);
>              opn = condnames[func-48];
>          }
> +        optype = CMPOP;
>          break;
>      case OPC_ADD_D:
>          check_cp1_registers(ctx, fs | ft | fd);
> @@ -10036,6 +10037,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
>              gen_cmp_d(ctx, func-48, ft, fs, cc);
>              opn = condnames[func-48];
>          }
> +        optype = CMPOP;
>          break;
>      case OPC_CVT_S_D:
>          check_cp1_registers(ctx, fs);
> @@ -10461,6 +10463,7 @@ static void gen_farith (DisasContext *ctx, enum fopcode op1,
>              gen_cmp_ps(ctx, func-48, ft, fs, cc);
>              opn = condnames[func-48];
>          }
> +        optype = CMPOP;
>          break;
>      default:
>          MIPS_INVAL(opn);

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

By the way, is this debug code really useful? I think by looking at the
TCG code (-d in_asm,op), it's easy to determine if an instruction is
correctly disassembled or not.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] target-mips: fix logically dead code reported by Coverity

[Leon Alrae]

On 14/07/2015 16:45, Aurelien Jarno wrote:
> By the way, is this debug code really useful? I think by looking at the
> TCG code (-d in_asm,op), it's easy to determine if an instruction is
> correctly disassembled or not.
> 

For me this debug code doesn't seem to be useful at all and it only clutters
translate.c :) In this patch I just wanted to make Coverity happy so I
followed the existing code but generally would vote for removal all of MIPS_DEBUG.

Leon

Re: [Qemu-devel] [PATCH] target-mips: fix logically dead code reported by Coverity

[Aurelien Jarno]

On 2015-07-14 17:22, Leon Alrae wrote:
> On 14/07/2015 16:45, Aurelien Jarno wrote:
> > By the way, is this debug code really useful? I think by looking at the
> > TCG code (-d in_asm,op), it's easy to determine if an instruction is
> > correctly disassembled or not.
> > 
> 
> For me this debug code doesn't seem to be useful at all and it only clutters
> translate.c :) In this patch I just wanted to make Coverity happy so I
> followed the existing code but generally would vote for removal all of MIPS_DEBUG.

Ok, I'll work on a patch for 2.5. That said I think we should still
merge the patch for 2.4.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH] target-mips: fix resource leak reported by Coverity

[Leon Alrae]

UHI assert and link operations call lock_user_string() twice to obtain two
strings pointed by gpr[4] and gpr[5]. If the second lock_user_string()
fails, then the first one won't get freed. Fix this by introducing another
macro responsible for obtaining two strings and handling allocation
failure.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 target-mips/mips-semi.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/target-mips/mips-semi.c b/target-mips/mips-semi.c
index 1162c76..5050940 100644
--- a/target-mips/mips-semi.c
+++ b/target-mips/mips-semi.c
@@ -220,6 +220,23 @@ static int copy_argn_to_target(CPUMIPSState *env, int arg_num,
         }                                       \
     } while (0)
 
+#define GET_TARGET_STRINGS_2(p, addr, p2, addr2)        \
+    do {                                                \
+        p = lock_user_string(addr);                     \
+        if (!p) {                                       \
+            gpr[2] = -1;                                \
+            gpr[3] = EFAULT;                            \
+            goto uhi_done;                              \
+        }                                               \
+        p2 = lock_user_string(addr2);                   \
+        if (!p2) {                                      \
+            unlock_user(p, addr, 0);                    \
+            gpr[2] = -1;                                \
+            gpr[3] = EFAULT;                            \
+            goto uhi_done;                              \
+        }                                               \
+    } while (0)
+
 #define FREE_TARGET_STRING(p, gpr)              \
     do {                                        \
         unlock_user(p, gpr, 0);                 \
@@ -322,8 +339,7 @@ void helper_do_semihosting(CPUMIPSState *env)
         FREE_TARGET_STRING(p, gpr[4]);
         break;
     case UHI_assert:
-        GET_TARGET_STRING(p, gpr[4]);
-        GET_TARGET_STRING(p2, gpr[5]);
+        GET_TARGET_STRINGS_2(p, gpr[4], p2, gpr[5]);
         printf("assertion '");
         printf("\"%s\"", p);
         printf("': file \"%s\", line %d\n", p2, (int)gpr[6]);
@@ -341,8 +357,7 @@ void helper_do_semihosting(CPUMIPSState *env)
         break;
 #ifndef _WIN32
     case UHI_link:
-        GET_TARGET_STRING(p, gpr[4]);
-        GET_TARGET_STRING(p2, gpr[5]);
+        GET_TARGET_STRINGS_2(p, gpr[4], p2, gpr[5]);
         gpr[2] = link(p, p2);
         gpr[3] = errno_mips(errno);
         FREE_TARGET_STRING(p2, gpr[5]);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] target-mips: fix resource leak reported by Coverity

[Aurelien Jarno]

On 2015-07-14 11:08, Leon Alrae wrote:
> UHI assert and link operations call lock_user_string() twice to obtain two
> strings pointed by gpr[4] and gpr[5]. If the second lock_user_string()
> fails, then the first one won't get freed. Fix this by introducing another
> macro responsible for obtaining two strings and handling allocation
> failure.
> 
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>  target-mips/mips-semi.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/target-mips/mips-semi.c b/target-mips/mips-semi.c
> index 1162c76..5050940 100644
> --- a/target-mips/mips-semi.c
> +++ b/target-mips/mips-semi.c
> @@ -220,6 +220,23 @@ static int copy_argn_to_target(CPUMIPSState *env, int arg_num,
>          }                                       \
>      } while (0)
>  
> +#define GET_TARGET_STRINGS_2(p, addr, p2, addr2)        \
> +    do {                                                \
> +        p = lock_user_string(addr);                     \
> +        if (!p) {                                       \
> +            gpr[2] = -1;                                \
> +            gpr[3] = EFAULT;                            \
> +            goto uhi_done;                              \
> +        }                                               \
> +        p2 = lock_user_string(addr2);                   \
> +        if (!p2) {                                      \
> +            unlock_user(p, addr, 0);                    \
> +            gpr[2] = -1;                                \
> +            gpr[3] = EFAULT;                            \
> +            goto uhi_done;                              \
> +        }                                               \
> +    } while (0)
> +
>  #define FREE_TARGET_STRING(p, gpr)              \
>      do {                                        \
>          unlock_user(p, gpr, 0);                 \
> @@ -322,8 +339,7 @@ void helper_do_semihosting(CPUMIPSState *env)
>          FREE_TARGET_STRING(p, gpr[4]);
>          break;
>      case UHI_assert:
> -        GET_TARGET_STRING(p, gpr[4]);
> -        GET_TARGET_STRING(p2, gpr[5]);
> +        GET_TARGET_STRINGS_2(p, gpr[4], p2, gpr[5]);
>          printf("assertion '");
>          printf("\"%s\"", p);
>          printf("': file \"%s\", line %d\n", p2, (int)gpr[6]);
> @@ -341,8 +357,7 @@ void helper_do_semihosting(CPUMIPSState *env)
>          break;
>  #ifndef _WIN32
>      case UHI_link:
> -        GET_TARGET_STRING(p, gpr[4]);
> -        GET_TARGET_STRING(p2, gpr[5]);
> +        GET_TARGET_STRINGS_2(p, gpr[4], p2, gpr[5]);
>          gpr[2] = link(p, p2);
>          gpr[3] = errno_mips(errno);
>          FREE_TARGET_STRING(p2, gpr[5]);

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] target-mips: correct DERET instruction

[Aurelien Jarno]

On 2015-07-14 11:08, Leon Alrae wrote:
> Fix Debug Mode flag clearing, and when DERET is placed between LL and SC
> do not make SC fail.
> 
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> ---
>  target-mips/op_helper.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 2a9ddff..d461ad8 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -2154,10 +2154,9 @@ void helper_deret(CPUMIPSState *env)
>      debug_pre_eret(env);
>      set_pc(env, env->CP0_DEPC);
>  
> -    env->hflags &= MIPS_HFLAG_DM;
> +    env->hflags &= ~MIPS_HFLAG_DM;
>      compute_hflags(env);
>      debug_post_eret(env);
> -    env->lladdr = 1;
>  }
>  #endif /* !CONFIG_USER_ONLY */

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net