Re: [Qemu-devel] [PATCH 4/7] target-i386: Dump illegal opcodes with -d unimp

["Hervé Poussineau" <hpoussin@reactos.org>]

Le 03/03/2016 06:30, Richard Henderson a écrit :
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>   target-i386/translate.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index b73c237..aa423cb 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -99,6 +99,7 @@ typedef struct DisasContext {
>       int prefix;
>       TCGMemOp aflag;
>       TCGMemOp dflag;
> +    target_ulong pc_start;
>       target_ulong pc; /* pc = eip + cs_base */
>       int is_jmp; /* 1 = means jump (stop translation), 2 means CPU
>                      static state change (stop translation) */
> @@ -2368,6 +2369,21 @@ static void gen_exception(DisasContext *s, int trapno, target_ulong cur_eip)
>       s->is_jmp = DISAS_TB_JUMP;
>   }
>
> +static void gen_illop(CPUX86State *env, DisasContext *s)
> +{
> +    target_ulong pc = s->pc_start;
> +    gen_exception(s, EXCP06_ILLOP, pc - s->cs_base);
> +
> +    if (qemu_loglevel_mask(LOG_UNIMP)) {

Do you want LOG_UNIMP or LOG_GUEST_ERROR?
Both are possible. Either you decide that guest works well, and an unknown instruction is a valid instruction unimplemented in QEMU side,
you decide that guest can do invalid things, and LOG_GUEST_ERROR is probably better.

> +        target_ulong end = s->pc;
> +        qemu_log("ILLOPC: " TARGET_FMT_lx ":", pc);
> +        for (; pc < end; ++pc) {
> +            qemu_log(" %02x", cpu_ldub_code(env, pc));
> +        }
> +        qemu_log("\n");
> +    }
> +}
> +
>   /* an interrupt is different from an exception because of the
>      privilege checks */
>   static void gen_interrupt(DisasContext *s, int intno,
> @@ -2893,7 +2909,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
>       }
>       if (s->flags & HF_EM_MASK) {
>       illegal_op:
> -        gen_exception(s, EXCP06_ILLOP, pc_start - s->cs_base);
> +        gen_illop(env, s);
>           return;
>       }
>       if (is_xmm && !(s->flags & HF_OSFXSR_MASK))
> @@ -4293,7 +4309,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>       target_ulong next_eip, tval;
>       int rex_w, rex_r;
>
> -    s->pc = pc_start;
> +    s->pc_start = s->pc = pc_start;
>       prefixes = 0;
>       s->override = -1;
>       rex_w = -1;
> @@ -8031,7 +8047,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>       if (s->prefix & PREFIX_LOCK)
>           gen_helper_unlock();
>       /* XXX: ensure that no lock was generated */
> -    gen_exception(s, EXCP06_ILLOP, pc_start - s->cs_base);
> +    gen_illop(env, s);
>       return s->pc;
>   }
>
>


This patch is not quiet on some operating systems:
OS/2:
ILLOPC: 000172e1: 0f a6

Windows XP:
ILLOPC: 00020d1a: c4 c4

And very verbose in Windows 3.11, Windows 9x:
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000027fe: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 00011b36: 63
ILLOPC: 000ffb17: 63
ILLOPC: 00011b3d: 63
ILLOPC: 00011b36: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 0001e3b9: 0f ff
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 00011b36: 63
ILLOPC: 00011b3d: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 00014d8a: 0f ff
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000ffb17: 63
ILLOPC: 000118ca: 63
ILLOPC: 000ffb17: 63

Is it normal?

Regards,

Hervé