diff mbox series

[v3,04/40] target/mips: Add decode_nanomips_opc() function

Message ID 1532004912-13899-5-git-send-email-stefan.markovic@rt-rk.com
State New
Headers show
Series Add nanoMIPS support to QEMU | expand

Commit Message

Stefan Markovic July 19, 2018, 12:54 p.m. UTC 
From: Yongbok Kim <yongbok.kim@mips.com>

Add empty body and invocation of decode_nanomips_opc() if the bit
ISA_NANOMIPS32 is set in env->insn_flags.

Signed-off-by: Yongbok Kim <yongbok.kim@mips.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 target/mips/translate.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Richard Henderson July 19, 2018, 4:39 p.m. UTC | #1 
On 07/19/2018 05:54 AM, Stefan Markovic wrote:
>          decode_opc(env, ctx);
>      } else if (ctx->insn_flags & ASE_MICROMIPS) {
> -        ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> -        insn_bytes = decode_micromips_opc(env, ctx);
> +        if (env->insn_flags & ISA_NANOMIPS32) {

Be consistent and use ctx->insn_flags.

> +            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> +            insn_bytes = decode_nanomips_opc(env, ctx);
> +        } else {
> +            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> +            insn_bytes = decode_micromips_opc(env, ctx);
> +        }
>      } else if (ctx->insn_flags & ASE_MIPS16) {
>          ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);

Do you really want to nest nanoMIPS within microMIPS?

I would have thought a better structure was

  } else if (ctx->insn_flags & ISA_NANOMIPS32) {
      ...
  } else if (ctx->insn_flags & ASE_MICROMIPS) {
      ...
  } else if (ctx->insn_flags & ASE_MIPS16) {


r~
Aleksandar Markovic July 24, 2018, 10:56 a.m. UTC | #2 
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, July 19, 2018 6:39 PM
> On 07/19/2018 05:54 AM, Stefan Markovic wrote:
> >          decode_opc(env, ctx);
> >      } else if (ctx->insn_flags & ASE_MICROMIPS) {
> > -        ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> > -        insn_bytes = decode_micromips_opc(env, ctx);
> > +        if (env->insn_flags & ISA_NANOMIPS32) {
>
> Be consistent and use ctx->insn_flags.
>
> > +            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> > +            insn_bytes = decode_nanomips_opc(env, ctx);
> > +        } else {
> > +            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
> > +            insn_bytes = decode_micromips_opc(env, ctx);
> > +        }
> >      } else if (ctx->insn_flags & ASE_MIPS16) {
> >          ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
>
> Do you really want to nest nanoMIPS within microMIPS?
>
> I would have thought a better structure was
>
>   } else if (ctx->insn_flags & ISA_NANOMIPS32) {
>       ...
>   } else if (ctx->insn_flags & ASE_MICROMIPS) {
>       ...
>   } else if (ctx->insn_flags & ASE_MIPS16) {
>
>
> r~

Hi, Richard,

This will be fixed in the way you described in v4.

Aleksandar
diff mbox series

Patch

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 227b2c0..67a0f70 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -16458,6 +16458,19 @@  enum {
     NM_EVP      = 0x01,
 };
 
+
+/*
+ *
+ * nanoMIPS decoding engine
+ *
+ */
+
+static int decode_nanomips_opc(CPUMIPSState *env, DisasContext *ctx)
+{
+    return 2;
+}
+
+
 /* SmartMIPS extension to MIPS32 */
 
 #if defined(TARGET_MIPS64)
@@ -21263,8 +21276,13 @@  static void mips_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
         insn_bytes = 4;
         decode_opc(env, ctx);
     } else if (ctx->insn_flags & ASE_MICROMIPS) {
-        ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
-        insn_bytes = decode_micromips_opc(env, ctx);
+        if (env->insn_flags & ISA_NANOMIPS32) {
+            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
+            insn_bytes = decode_nanomips_opc(env, ctx);
+        } else {
+            ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
+            insn_bytes = decode_micromips_opc(env, ctx);
+        }
     } else if (ctx->insn_flags & ASE_MIPS16) {
         ctx->opcode = cpu_lduw_code(env, ctx->base.pc_next);
         insn_bytes = decode_mips16_opc(env, ctx);