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

[Paolo Bonzini <pbonzini@redhat.com>]

On 8/25/22 03:47, Richard Henderson wrote:
> 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?

Extremely so. :)  When debugging, it is very fast and satisfying to run 
a simple bisection script (see below) that tells you exactly which 
instruction is mistranslated.

In an actual series for commit I would add use_new/limit as a PATCH 
18/17, but for the RFC "how to debug this thing" seemed like an 
interesting topic of its own.

> 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.

Because it doesn't pass down VEX.mmmmm as you point out below.  But 
really it's simpler to move all the VEX opcodes to the new decoder and 
remove all traces of it from the decoder.  I have already done that but 
I want to write a testcase first.

Paolo

#! /bin/bash
run() {
   "$@" > /dev/null
   if test $? = 0; then
     x=ok     # or "read x" sometimes
   else
     x=bad
   fi
}

high=1
while :; do
   low=$high
   high=$(($high * 2))
   echo $low..?, trying $high
   LIMIT=$high run "$@"
   if test "$x" != ok; then
       break
   fi
done
while test $low != $high; do
   mid=$(( ( $low + $high ) / 2 ))
   echo $low..$high, trying $mid
   LIMIT=$mid run "$@"
   if test "$x" = ok; then
       low=$(( $mid + 1 ))
   else
       high=$mid
   fi
done
echo reproducer: LIMIT=$low "$@"

> 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);
> 
>