Re: [PATCH 03/17] target/i386: add core of new i386 decoder

[Richard Henderson]

On 8/24/22 10:31, Paolo Bonzini wrote:
> diff --git a/target/i386/tcg/decode-old.c.inc 
> b/target/i386/tcg/decode-old.c.inc
> index 603642d6e1..fb86855501 100644
> --- a/target/i386/tcg/decode-old.c.inc
> +++ b/target/i386/tcg/decode-old.c.inc
> @@ -1808,10 +1808,24 @@ static target_ulong disas_insn(DisasContext *s, 
> CPUState *cpu)
>   
>       prefixes = 0;
>   
> +    if (first) first = false, limit = getenv("LIMIT") ? 
> atol(getenv("LIMIT")) : -1;
> +    bool use_new = true;
>    next_byte:
> +    s->prefix = prefixes;
>       b = x86_ldub_code(env, s);
>       /* Collect prefixes.  */
>       switch (b) {
> +    default:
> +#ifdef CONFIG_USER_ONLY
> +        use_new &= limit > 0;
> +#else
> +        use_new &= b <= limit;
> +#endif
> +        if (use_new && 0) {
> +            return disas_insn_new(s, cpu, b);
> +        }

Is this use_new/limit thing actually helpful?

I would have thought it would be helpful to block out adjusted opcodes
in the old decoder switch, and remove old code.  E.g. below.  That would
make it obvious what remains to be done.

>       case 0xc5: /* 2-byte VEX */
>       case 0xc4: /* 3-byte VEX */
> +        use_new = false;

Why?  You have support for By in the new decoder.

In the medium term, I wonder about passing in the vex.mmmmm argument,
so you can start someplace other than root.  Long term, that would go
away again when prefix processing is re-integrated with the new decoder.


r~


diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 9b925c7ec8..04626fa086 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2816,7 +2816,6 @@ static inline void gen_op_movq_env_0(DisasContext *s, int 
d_offset)
     tcg_gen_st_i64(s->tmp1_i64, cpu_env, d_offset);
 }
 
-static bool first = true; static unsigned long limit;
 #include "decode-new.c.inc"
 #include "decode-old.c.inc"
 
diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index b8726608bb..1195fea7c7 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -819,9 +819,6 @@ static target_ulong disas_insn_new(DisasContext *s, 
CPUState *cpu, int b)
     X86DecodedInsn decode;
     X86DecodeFunc decode_func = decode_root;
 
-#ifdef CONFIG_USER_ONLY
-    --limit;
-#endif
     s->has_modrm = false;
 #if 0
     s->pc_start = s->pc = s->base.pc_next;
diff --git a/target/i386/tcg/decode-old.c.inc b/target/i386/tcg/decode-old.c.inc
index c97289a3e4..b96c677915 100644
--- a/target/i386/tcg/decode-old.c.inc
+++ b/target/i386/tcg/decode-old.c.inc
@@ -1808,24 +1808,11 @@ static target_ulong disas_insn(DisasContext *s, 
CPUState *cpu)
 
     prefixes = 0;
 
-    if (first) first = false, limit = getenv("LIMIT") ? atol(getenv("LIMIT")) 
: -1;
-    bool use_new = true;
  next_byte:
     s->prefix = prefixes;
     b = x86_ldub_code(env, s);
     /* Collect prefixes.  */
     switch (b) {
-    default:
-#ifdef CONFIG_USER_ONLY
-        use_new &= limit > 0;
-#else
-        use_new &= b <= limit;
-#endif
-        if (use_new && b <= 0x1f) {
-            return disas_insn_new(s, cpu, b);
-        }
-    case 0x0f:
-        break;
     case 0xf3:
         prefixes |= PREFIX_REPZ;
         prefixes &= ~PREFIX_REPNZ;
@@ -1876,7 +1863,6 @@ static target_ulong disas_insn(DisasContext *s, CPUState 
*cpu)
 #endif
     case 0xc5: /* 2-byte VEX */
     case 0xc4: /* 3-byte VEX */
-        use_new = false;
         /* VEX prefixes cannot be used except in 32-bit mode.
            Otherwise the instruction is LES or LDS.  */
         if (CODE32(s) && !VM86(s)) {
@@ -1969,12 +1955,12 @@ static target_ulong disas_insn(DisasContext *s, 
CPUState *cpu)
         b = x86_ldub_code(env, s) | 0x100;
         goto reswitch;
 
+    case 0x00 ... 0x0e:
+    case 0x10 ... 0x1f:
+        return disas_insn_new(s, cpu, b);
+
         /**************************/
         /* arith & logic */
-    case 0x00 ... 0x05:
-    case 0x08 ... 0x0d:
-    case 0x10 ... 0x15:
-    case 0x18 ... 0x1d:
     case 0x20 ... 0x25:
     case 0x28 ... 0x2d:
     case 0x30 ... 0x35:
@@ -2764,40 +2750,11 @@ static target_ulong disas_insn(DisasContext *s, 
CPUState *cpu)
     case 0xc9: /* leave */
         gen_leave(s);
         break;
-    case 0x06: /* push es */
-    case 0x0e: /* push cs */
-    case 0x16: /* push ss */
-    case 0x1e: /* push ds */
-        if (CODE64(s))
-            goto illegal_op;
-        gen_op_movl_T0_seg(s, b >> 3);
-        gen_push_v(s, s->T0);
-        break;
     case 0x1a0: /* push fs */
     case 0x1a8: /* push gs */
         gen_op_movl_T0_seg(s, (b >> 3) & 7);
         gen_push_v(s, s->T0);
         break;
-    case 0x07: /* pop es */
-    case 0x17: /* pop ss */
-    case 0x1f: /* pop ds */
-        if (CODE64(s))
-            goto illegal_op;
-        reg = b >> 3;
-        ot = gen_pop_T0(s);
-        gen_movl_seg_T0(s, reg);
-        gen_pop_update(s, ot);
-        /* Note that reg == R_SS in gen_movl_seg_T0 always sets is_jmp.  */
-        if (s->base.is_jmp) {
-            gen_jmp_im(s, s->pc - s->cs_base);
-            if (reg == R_SS) {
-                s->flags &= ~HF_TF_MASK;
-                gen_eob_inhibit_irq(s, true);
-            } else {
-                gen_eob(s);
-            }
-        }
-        break;
     case 0x1a1: /* pop fs */
     case 0x1a9: /* pop gs */
         ot = gen_pop_T0(s);