[Qemu-devel] [PATCH 0/7] target-i386 fixes

[Qemu-devel] [PATCH 0/7] target-i386 fixes

[Richard Henderson]

This is primarily patches fixing Windows booting regressions
introduced by myself.  Many thanks to Herve for reporting them
and Paolo for fixing two of them.

Changes from patches previously seen on-list:
  * Tweak the bnd_jmp patch to test MPX enabled properly.
  * Dump illegal opcode data with -d unimp.
  * Use gen_nop_modrm for prefetch.
  * Fix Windows XP booting.

The illegal opcode dumping was useful for debugging.  The prefetch
patch I noticed while reviewing the addr16 patch wrt lea.


r~


Paolo Bonzini (3):
  target-i386: avoid repeated calls to the bnd_jmp helper
  target-i386: fix smsw and lmsw from/to register
  target-i386: fix addr16 prefix

Richard Henderson (4):
  target-i386: Fix SMSW for 64-bit mode
  target-i386: Dump illegal opcodes with -d unimp
  target-i386: Use gen_nop_modrm for prefetch instructions
  target-i386: Fix inhibit irq mask handling

 target-i386/translate.c | 179 +++++++++++++++++++++++++++---------------------
 1 file changed, 100 insertions(+), 79 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH 1/7] target-i386: avoid repeated calls to the bnd_jmp helper

[Richard Henderson]

From: Paolo Bonzini <pbonzini@redhat.com>

Two flags were tested the wrong way.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1456845145-18891-1-git-send-email-pbonzini@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
[rth: Fixed enable test as well.]
---
 target-i386/translate.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 53dee79..cd214a6 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2409,12 +2409,12 @@ static void gen_reset_hflag(DisasContext *s, uint32_t mask)
 /* Clear BND registers during legacy branches.  */
 static void gen_bnd_jmp(DisasContext *s)
 {
-    /* Do nothing if BND prefix present, MPX is disabled, or if the
-       BNDREGs are known to be in INIT state already.  The helper
-       itself will check BNDPRESERVE at runtime.  */
+    /* Clear the registers only if BND prefix is missing, MPX is enabled,
+       and if the BNDREGs are known to be in use (non-zero) already.
+       The helper itself will check BNDPRESERVE at runtime.  */
     if ((s->prefix & PREFIX_REPNZ) == 0
-        && (s->flags & HF_MPX_EN_MASK) == 0
-        && (s->flags & HF_MPX_IU_MASK) == 0) {
+        && (s->flags & HF_MPX_EN_MASK) != 0
+        && (s->flags & HF_MPX_IU_MASK) != 0) {
         gen_helper_bnd_jmp(cpu_env);
     }
 }
-- 
2.5.0

[Qemu-devel] [PATCH 2/7] target-i386: fix smsw and lmsw from/to register

[Richard Henderson]

From: Paolo Bonzini <pbonzini@redhat.com>

SMSW and LMSW accept register operands, but commit 1906b2a ("target-i386:
Rearrange processing of 0F 01", 2016-02-13) did not account for that.

Fixes: 1906b2af7c2345037d9b2fdf484b457b5acd09d1
Cc: rth@twiddle.net
Reported-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1456845134-18812-1-git-send-email-pbonzini@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index cd214a6..10cc2fa 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -57,11 +57,17 @@
 #endif
 
 /* For a switch indexed by MODRM, match all memory operands for a given OP.  */
-#define CASE_MEM_OP(OP) \
+#define CASE_MODRM_MEM_OP(OP) \
     case (0 << 6) | (OP << 3) | 0 ... (0 << 6) | (OP << 3) | 7: \
     case (1 << 6) | (OP << 3) | 0 ... (1 << 6) | (OP << 3) | 7: \
     case (2 << 6) | (OP << 3) | 0 ... (2 << 6) | (OP << 3) | 7
 
+#define CASE_MODRM_OP(OP) \
+    case (0 << 6) | (OP << 3) | 0 ... (0 << 6) | (OP << 3) | 7: \
+    case (1 << 6) | (OP << 3) | 0 ... (1 << 6) | (OP << 3) | 7: \
+    case (2 << 6) | (OP << 3) | 0 ... (2 << 6) | (OP << 3) | 7: \
+    case (3 << 6) | (OP << 3) | 0 ... (3 << 6) | (OP << 3) | 7
+
 //#define MACRO_TEST   1
 
 /* global register indexes */
@@ -7038,7 +7044,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
     case 0x101:
         modrm = cpu_ldub_code(env, s->pc++);
         switch (modrm) {
-        CASE_MEM_OP(0): /* sgdt */
+        CASE_MODRM_MEM_OP(0): /* sgdt */
             gen_svm_check_intercept(s, pc_start, SVM_EXIT_GDTR_READ);
             gen_lea_modrm(env, s, modrm);
             tcg_gen_ld32u_tl(cpu_T0,
@@ -7094,7 +7100,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_eob(s);
             break;
 
-        CASE_MEM_OP(1): /* sidt */
+        CASE_MODRM_MEM_OP(1): /* sidt */
             gen_svm_check_intercept(s, pc_start, SVM_EXIT_IDTR_READ);
             gen_lea_modrm(env, s, modrm);
             tcg_gen_ld32u_tl(cpu_T0, cpu_env, offsetof(CPUX86State, idt.limit));
@@ -7240,7 +7246,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_helper_invlpga(cpu_env, tcg_const_i32(s->aflag - 1));
             break;
 
-        CASE_MEM_OP(2): /* lgdt */
+        CASE_MODRM_MEM_OP(2): /* lgdt */
             if (s->cpl != 0) {
                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
                 break;
@@ -7257,7 +7263,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             tcg_gen_st32_tl(cpu_T1, cpu_env, offsetof(CPUX86State, gdt.limit));
             break;
 
-        CASE_MEM_OP(3): /* lidt */
+        CASE_MODRM_MEM_OP(3): /* lidt */
             if (s->cpl != 0) {
                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
                 break;
@@ -7274,7 +7280,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             tcg_gen_st32_tl(cpu_T1, cpu_env, offsetof(CPUX86State, idt.limit));
             break;
 
-        CASE_MEM_OP(4): /* smsw */
+        CASE_MODRM_OP(4): /* smsw */
             gen_svm_check_intercept(s, pc_start, SVM_EXIT_READ_CR0);
 #if defined TARGET_X86_64 && defined HOST_WORDS_BIGENDIAN
             tcg_gen_ld32u_tl(cpu_T0, cpu_env, offsetof(CPUX86State, cr[0]) + 4);
@@ -7284,7 +7290,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 1);
             break;
 
-        CASE_MEM_OP(6): /* lmsw */
+        CASE_MODRM_OP(6): /* lmsw */
             if (s->cpl != 0) {
                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
                 break;
@@ -7296,7 +7302,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_eob(s);
             break;
 
-        CASE_MEM_OP(7): /* invlpg */
+        CASE_MODRM_MEM_OP(7): /* invlpg */
             if (s->cpl != 0) {
                 gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
                 break;
@@ -7778,7 +7784,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
     case 0x1ae:
         modrm = cpu_ldub_code(env, s->pc++);
         switch (modrm) {
-        CASE_MEM_OP(0): /* fxsave */
+        CASE_MODRM_MEM_OP(0): /* fxsave */
             if (!(s->cpuid_features & CPUID_FXSR)
                 || (prefixes & PREFIX_LOCK)) {
                 goto illegal_op;
@@ -7791,7 +7797,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_helper_fxsave(cpu_env, cpu_A0);
             break;
 
-        CASE_MEM_OP(1): /* fxrstor */
+        CASE_MODRM_MEM_OP(1): /* fxrstor */
             if (!(s->cpuid_features & CPUID_FXSR)
                 || (prefixes & PREFIX_LOCK)) {
                 goto illegal_op;
@@ -7804,7 +7810,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_helper_fxrstor(cpu_env, cpu_A0);
             break;
 
-        CASE_MEM_OP(2): /* ldmxcsr */
+        CASE_MODRM_MEM_OP(2): /* ldmxcsr */
             if ((s->flags & HF_EM_MASK) || !(s->flags & HF_OSFXSR_MASK)) {
                 goto illegal_op;
             }
@@ -7817,7 +7823,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_helper_ldmxcsr(cpu_env, cpu_tmp2_i32);
             break;
 
-        CASE_MEM_OP(3): /* stmxcsr */
+        CASE_MODRM_MEM_OP(3): /* stmxcsr */
             if ((s->flags & HF_EM_MASK) || !(s->flags & HF_OSFXSR_MASK)) {
                 goto illegal_op;
             }
@@ -7830,7 +7836,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_op_st_v(s, MO_32, cpu_T0, cpu_A0);
             break;
 
-        CASE_MEM_OP(4): /* xsave */
+        CASE_MODRM_MEM_OP(4): /* xsave */
             if ((s->cpuid_ext_features & CPUID_EXT_XSAVE) == 0
                 || (prefixes & (PREFIX_LOCK | PREFIX_DATA
                                 | PREFIX_REPZ | PREFIX_REPNZ))) {
@@ -7842,7 +7848,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_helper_xsave(cpu_env, cpu_A0, cpu_tmp1_i64);
             break;
 
-        CASE_MEM_OP(5): /* xrstor */
+        CASE_MODRM_MEM_OP(5): /* xrstor */
             if ((s->cpuid_ext_features & CPUID_EXT_XSAVE) == 0
                 || (prefixes & (PREFIX_LOCK | PREFIX_DATA
                                 | PREFIX_REPZ | PREFIX_REPNZ))) {
@@ -7859,7 +7865,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             gen_eob(s);
             break;
 
-        CASE_MEM_OP(6): /* xsaveopt / clwb */
+        CASE_MODRM_MEM_OP(6): /* xsaveopt / clwb */
             if (prefixes & PREFIX_LOCK) {
                 goto illegal_op;
             }
@@ -7883,7 +7889,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             }
             break;
 
-        CASE_MEM_OP(7): /* clflush / clflushopt */
+        CASE_MODRM_MEM_OP(7): /* clflush / clflushopt */
             if (prefixes & PREFIX_LOCK) {
                 goto illegal_op;
             }
-- 
2.5.0

[Qemu-devel] [PATCH 3/7] target-i386: Fix SMSW for 64-bit mode

[Richard Henderson]

In non-64-bit modes, the instruction always stores 16 bits.
But in 64-bit mode, when the destination is a register, the
instruction can write 32 or 64 bits.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 10cc2fa..b73c237 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7282,12 +7282,14 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
 
         CASE_MODRM_OP(4): /* smsw */
             gen_svm_check_intercept(s, pc_start, SVM_EXIT_READ_CR0);
-#if defined TARGET_X86_64 && defined HOST_WORDS_BIGENDIAN
-            tcg_gen_ld32u_tl(cpu_T0, cpu_env, offsetof(CPUX86State, cr[0]) + 4);
-#else
-            tcg_gen_ld32u_tl(cpu_T0, cpu_env, offsetof(CPUX86State, cr[0]));
-#endif
-            gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 1);
+            tcg_gen_ld_tl(cpu_T0, cpu_env, offsetof(CPUX86State, cr[0]));
+            if (CODE64(s)) {
+                mod = (modrm >> 6) & 3;
+                ot = (mod != 3 ? MO_16 : s->dflag);
+            } else {
+                ot = MO_16;
+            }
+            gen_ldst_modrm(env, s, modrm, ot, OR_TMP0, 1);
             break;
 
         CASE_MODRM_OP(6): /* lmsw */
-- 
2.5.0

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

[Richard Henderson]

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)) {
+        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;
 }
 
-- 
2.5.0

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

[Hervé Poussineau]

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é

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

[Paolo Bonzini]

> Do you want LOG_UNIMP or LOG_GUEST_ERROR? 

I would actually use LOG_IN_ASM.  As you noticed, guests sometimes use
illegal opcodes; another example is Xen's hypercall interface.

On 03/03/2016 07:57, Hervé Poussineau wrote:
> 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
> 
> Is it normal?

Yes, it is.  As usual, Raymond Chen explains what's going on:

https://blogs.msdn.microsoft.com/oldnewthing/20041215-00/?p=37003

Alternatively, you can buy Unauthorized Windows 95 on Amazon for about
10 euros.  Which of course I just did. :)

Paolo

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

[Richard Henderson]

On 03/03/2016 02:08 AM, Paolo Bonzini wrote:
>> Do you want LOG_UNIMP or LOG_GUEST_ERROR?
>
> I would actually use LOG_IN_ASM.  As you noticed, guests sometimes use
> illegal opcodes; another example is Xen's hypercall interface.
>
> On 03/03/2016 07:57, Hervé Poussineau wrote:
>> 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
>>
>> Is it normal?
>
> Yes, it is.  As usual, Raymond Chen explains what's going on:
>
> https://blogs.msdn.microsoft.com/oldnewthing/20041215-00/?p=37003

Wow.  That's... interesting.

I think maybe I'll re-do the patch to distinguish between those opcodes that 
are completely unrecognized (which is what I was expecting to find) and those 
that raise #UD due to cpu state (e.g. this arpl in vm86 mode).


r~

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

[Paolo Bonzini]

On 03/03/2016 20:06, Richard Henderson wrote:
> On 03/03/2016 02:08 AM, Paolo Bonzini wrote:
>>> Do you want LOG_UNIMP or LOG_GUEST_ERROR?
>>
>> I would actually use LOG_IN_ASM.  As you noticed, guests sometimes use
>> illegal opcodes; another example is Xen's hypercall interface.
>>
>> On 03/03/2016 07:57, Hervé Poussineau wrote:
>>> 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
>>>
>>> Is it normal?
>>
>> Yes, it is.  As usual, Raymond Chen explains what's going on:
>>
>> https://blogs.msdn.microsoft.com/oldnewthing/20041215-00/?p=37003
> 
> Wow.  That's... interesting.
> 
> I think maybe I'll re-do the patch to distinguish between those opcodes
> that are completely unrecognized (which is what I was expecting to find)
> and those that raise #UD due to cpu state (e.g. this arpl in vm86 mode).

Good idea.  UD2 should not warn too, and also VEX prefixes outside
64-bit mode.

Any thoughts about patch 7?

Paolo

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

[Richard Henderson]

[-- Attachment #1: Type: text/html, Size: 382 bytes --]

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

[Paolo Bonzini]

On 03/03/2016 20:06, Richard Henderson wrote:
> On 03/03/2016 02:08 AM, Paolo Bonzini wrote:
>>> Do you want LOG_UNIMP or LOG_GUEST_ERROR?
>>
>> I would actually use LOG_IN_ASM.  As you noticed, guests sometimes use
>> illegal opcodes; another example is Xen's hypercall interface.
>>
>> On 03/03/2016 07:57, Hervé Poussineau wrote:
>>> 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
>>>
>>> Is it normal?
>>
>> Yes, it is.  As usual, Raymond Chen explains what's going on:
>>
>> https://blogs.msdn.microsoft.com/oldnewthing/20041215-00/?p=37003
> 
> Wow.  That's... interesting.

It's actually even more interesting (the explanation is in the book) if
you notice that 0xffb17 is in the middle of the BIOS.  Indeed Windows 95
first locates a single 0x63 in the BIOS (so that it's ROM and no one can
write a different byte).  Then the 32-bit code can use a system service
that allocates a callback from 16-bit MS-DOS.  That service gets a
32-bit address for the 32-bit code and returns a real-mode address to be
used in 16-bit code.

The kick is that all the real-mode addresses point to that single 0x63
that was found in ROM.  For example in the case above the real-mode
addresses could be FFB1:07, FFB0:17, FFAF:27, etc.  The illegal opcode
exception handler looks at the segment to figure out which 32-bit
address to jump to.

There are also cases where the ARPL is patched into existing code (like
a breakpoint) to trap that code to 32-bit.  But this one using the ROM
is much cooler.

Paolo

[Qemu-devel] [PATCH 5/7] target-i386: fix addr16 prefix

[Richard Henderson]

From: Paolo Bonzini <pbonzini@redhat.com>

While ADDSEG will only be false in 16-bit mode for LEA, it can be
false even in other cases when 16-bit addresses are obtained via
the 67h prefix in 32-bit mode.  In this case, gen_lea_v_seg forgets
to add a nonzero FS or GS base if CS/DS/ES/SS are all zero.  This
case is pretty rare but happens when booting Windows 95/98, and
this patch fixes it.

The bug is visible since commit d6a291498, but it was introduced
together with gen_lea_v_seg and it probably could be reproduced
with a "addr16 gs movsb" instruction as early as in commit
ca2f29f555805d07fb0b9ebfbbfc4e3656530977.

Reported-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <1456931078-21635-1-git-send-email-pbonzini@redhat.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index aa423cb..ed54381 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -467,15 +467,15 @@ static void gen_lea_v_seg(DisasContext *s, TCGMemOp aflag, TCGv a0,
         break;
     case MO_16:
         /* 16 bit address */
-        if (ovr_seg < 0) {
-            ovr_seg = def_seg;
-        }
         tcg_gen_ext16u_tl(cpu_A0, a0);
-        /* ADDSEG will only be false in 16-bit mode for LEA.  */
-        if (!s->addseg) {
-            return;
-        }
         a0 = cpu_A0;
+        if (ovr_seg < 0) {
+            if (s->addseg) {
+                ovr_seg = def_seg;
+            } else {
+                return;
+            }
+        }
         break;
     default:
         tcg_abort();
-- 
2.5.0

[Qemu-devel] [PATCH 6/7] target-i386: Use gen_nop_modrm for prefetch instructions

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index ed54381..7455a18 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7491,7 +7491,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         case 3: /* prefetchnt0 */
             if (mod == 3)
                 goto illegal_op;
-            gen_lea_modrm(env, s, modrm);
+            gen_nop_modrm(env, s, modrm);
             /* nothing more to do */
             break;
         default: /* nop (multi byte) */
@@ -7989,8 +7989,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         mod = (modrm >> 6) & 3;
         if (mod == 3)
             goto illegal_op;
-        gen_lea_modrm(env, s, modrm);
-        /* ignore for now */
+        gen_nop_modrm(env, s, modrm);
         break;
     case 0x1aa: /* rsm */
         gen_svm_check_intercept(s, pc_start, SVM_EXIT_RSM);
-- 
2.5.0

[Qemu-devel] [PATCH 7/7] target-i386: Fix inhibit irq mask handling

[Richard Henderson]

The patch in 7f0b714 was too simplistic, in that we wound up setting
the flag and then resetting it immediately in gen_eob.

Fixes the reported boot problem with Windows XP.

Reported-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 76 ++++++++++++++++++++++++-------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 7455a18..513b53a 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2441,12 +2441,19 @@ static void gen_bnd_jmp(DisasContext *s)
     }
 }
 
-/* generate a generic end of block. Trace exception is also generated
-   if needed */
-static void gen_eob(DisasContext *s)
+/* Generate an end of block. Trace exception is also generated if needed.
+   If IIM, set HF_INHIBIT_IRQ_MASK if it isn't already set.  */
+static void gen_eob_inhibit_irq(DisasContext *s, bool inhibit)
 {
     gen_update_cc_op(s);
-    gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK);
+
+    /* If several instructions disable interrupts, only the first does it.  */
+    if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) {
+        gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
+    } else {
+        gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK);
+    }
+
     if (s->tb->flags & HF_RF_MASK) {
         gen_helper_reset_rf(cpu_env);
     }
@@ -2460,6 +2467,12 @@ static void gen_eob(DisasContext *s)
     s->is_jmp = DISAS_TB_JUMP;
 }
 
+/* End of block, resetting the inhibit irq flag.  */
+static void gen_eob(DisasContext *s)
+{
+    gen_eob_inhibit_irq(s, false);
+}
+
 /* generate a jump to eip. No segment change must happen before as a
    direct call to the next block may occur */
 static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num)
@@ -5193,16 +5206,15 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         ot = gen_pop_T0(s);
         gen_movl_seg_T0(s, reg);
         gen_pop_update(s, ot);
-        if (reg == R_SS) {
-            /* if reg == SS, inhibit interrupts/trace. */
-            /* If several instructions disable interrupts, only the
-               _first_ does it */
-            gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
-            s->tf = 0;
-        }
+        /* Note that reg == R_SS in gen_movl_seg_T0 always sets is_jmp.  */
         if (s->is_jmp) {
             gen_jmp_im(s->pc - s->cs_base);
-            gen_eob(s);
+            if (reg == R_SS) {
+                s->tf = 0;
+                gen_eob_inhibit_irq(s, true);
+            } else {
+                gen_eob(s);
+            }
         }
         break;
     case 0x1a1: /* pop fs */
@@ -5260,16 +5272,15 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
             goto illegal_op;
         gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 0);
         gen_movl_seg_T0(s, reg);
-        if (reg == R_SS) {
-            /* if reg == SS, inhibit interrupts/trace */
-            /* If several instructions disable interrupts, only the
-               _first_ does it */
-            gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
-            s->tf = 0;
-        }
+        /* Note that reg == R_SS in gen_movl_seg_T0 always sets is_jmp.  */
         if (s->is_jmp) {
             gen_jmp_im(s->pc - s->cs_base);
-            gen_eob(s);
+            if (reg == R_SS) {
+                s->tf = 0;
+                gen_eob_inhibit_irq(s, true);
+            } else {
+                gen_eob(s);
+            }
         }
         break;
     case 0x8c: /* mov Gv, seg */
@@ -6795,26 +6806,13 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         }
         break;
     case 0xfb: /* sti */
-        if (!s->vm86) {
-            if (s->cpl <= s->iopl) {
-            gen_sti:
-                gen_helper_sti(cpu_env);
-                /* interruptions are enabled only the first insn after sti */
-                /* If several instructions disable interrupts, only the
-                   _first_ does it */
-                gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
-                /* give a chance to handle pending irqs */
-                gen_jmp_im(s->pc - s->cs_base);
-                gen_eob(s);
-            } else {
-                gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
-            }
+        if (s->vm86 ? s->iopl == 3 : s->cpl <= s->iopl) {
+            gen_helper_sti(cpu_env);
+            /* interruptions are enabled only the first insn after sti */
+            gen_jmp_im(s->pc - s->cs_base);
+            gen_eob_inhibit_irq(s, true);
         } else {
-            if (s->iopl == 3) {
-                goto gen_sti;
-            } else {
-                gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
-            }
+            gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
         }
         break;
     case 0x62: /* bound */
-- 
2.5.0

Re: [Qemu-devel] [PATCH 7/7] target-i386: Fix inhibit irq mask handling

[Paolo Bonzini]

On 03/03/2016 06:30, Richard Henderson wrote:
> The patch in 7f0b714 was too simplistic, in that we wound up setting
> the flag and then resetting it immediately in gen_eob.
> 
> Fixes the reported boot problem with Windows XP.
> 
> Reported-by: Hervé Poussineau <hpoussin@reactos.org>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-i386/translate.c | 76 ++++++++++++++++++++++++-------------------------
>  1 file changed, 37 insertions(+), 39 deletions(-)

Oops, I forgot to Cc you on my fix for this:

http://permalink.gmane.org/gmane.comp.emulators.qemu/397645

Feel free to pick the one you prefer and send a pull request!

Thanks,

Paolo

> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 7455a18..513b53a 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -2441,12 +2441,19 @@ static void gen_bnd_jmp(DisasContext *s)
>      }
>  }
>  
> -/* generate a generic end of block. Trace exception is also generated
> -   if needed */
> -static void gen_eob(DisasContext *s)
> +/* Generate an end of block. Trace exception is also generated if needed.
> +   If IIM, set HF_INHIBIT_IRQ_MASK if it isn't already set.  */
> +static void gen_eob_inhibit_irq(DisasContext *s, bool inhibit)
>  {
>      gen_update_cc_op(s);
> -    gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK);
> +
> +    /* If several instructions disable interrupts, only the first does it.  */
> +    if (inhibit && !(s->flags & HF_INHIBIT_IRQ_MASK)) {
> +        gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
> +    } else {
> +        gen_reset_hflag(s, HF_INHIBIT_IRQ_MASK);
> +    }
> +
>      if (s->tb->flags & HF_RF_MASK) {
>          gen_helper_reset_rf(cpu_env);
>      }
> @@ -2460,6 +2467,12 @@ static void gen_eob(DisasContext *s)
>      s->is_jmp = DISAS_TB_JUMP;
>  }
>  
> +/* End of block, resetting the inhibit irq flag.  */
> +static void gen_eob(DisasContext *s)
> +{
> +    gen_eob_inhibit_irq(s, false);
> +}
> +
>  /* generate a jump to eip. No segment change must happen before as a
>     direct call to the next block may occur */
>  static void gen_jmp_tb(DisasContext *s, target_ulong eip, int tb_num)
> @@ -5193,16 +5206,15 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          ot = gen_pop_T0(s);
>          gen_movl_seg_T0(s, reg);
>          gen_pop_update(s, ot);
> -        if (reg == R_SS) {
> -            /* if reg == SS, inhibit interrupts/trace. */
> -            /* If several instructions disable interrupts, only the
> -               _first_ does it */
> -            gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
> -            s->tf = 0;
> -        }
> +        /* Note that reg == R_SS in gen_movl_seg_T0 always sets is_jmp.  */
>          if (s->is_jmp) {
>              gen_jmp_im(s->pc - s->cs_base);
> -            gen_eob(s);
> +            if (reg == R_SS) {
> +                s->tf = 0;
> +                gen_eob_inhibit_irq(s, true);
> +            } else {
> +                gen_eob(s);
> +            }
>          }
>          break;
>      case 0x1a1: /* pop fs */
> @@ -5260,16 +5272,15 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>              goto illegal_op;
>          gen_ldst_modrm(env, s, modrm, MO_16, OR_TMP0, 0);
>          gen_movl_seg_T0(s, reg);
> -        if (reg == R_SS) {
> -            /* if reg == SS, inhibit interrupts/trace */
> -            /* If several instructions disable interrupts, only the
> -               _first_ does it */
> -            gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
> -            s->tf = 0;
> -        }
> +        /* Note that reg == R_SS in gen_movl_seg_T0 always sets is_jmp.  */
>          if (s->is_jmp) {
>              gen_jmp_im(s->pc - s->cs_base);
> -            gen_eob(s);
> +            if (reg == R_SS) {
> +                s->tf = 0;
> +                gen_eob_inhibit_irq(s, true);
> +            } else {
> +                gen_eob(s);
> +            }
>          }
>          break;
>      case 0x8c: /* mov Gv, seg */
> @@ -6795,26 +6806,13 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>          }
>          break;
>      case 0xfb: /* sti */
> -        if (!s->vm86) {
> -            if (s->cpl <= s->iopl) {
> -            gen_sti:
> -                gen_helper_sti(cpu_env);
> -                /* interruptions are enabled only the first insn after sti */
> -                /* If several instructions disable interrupts, only the
> -                   _first_ does it */
> -                gen_set_hflag(s, HF_INHIBIT_IRQ_MASK);
> -                /* give a chance to handle pending irqs */
> -                gen_jmp_im(s->pc - s->cs_base);
> -                gen_eob(s);
> -            } else {
> -                gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> -            }
> +        if (s->vm86 ? s->iopl == 3 : s->cpl <= s->iopl) {
> +            gen_helper_sti(cpu_env);
> +            /* interruptions are enabled only the first insn after sti */
> +            gen_jmp_im(s->pc - s->cs_base);
> +            gen_eob_inhibit_irq(s, true);
>          } else {
> -            if (s->iopl == 3) {
> -                goto gen_sti;
> -            } else {
> -                gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
> -            }
> +            gen_exception(s, EXCP0D_GPF, pc_start - s->cs_base);
>          }
>          break;
>      case 0x62: /* bound */
> 

Re: [Qemu-devel] [PATCH 0/7] target-i386 fixes

[Hervé Poussineau]

Le 03/03/2016 06:30, Richard Henderson a écrit :
> This is primarily patches fixing Windows booting regressions
> introduced by myself.  Many thanks to Herve for reporting them
> and Paolo for fixing two of them.
>
> Changes from patches previously seen on-list:
>    * Tweak the bnd_jmp patch to test MPX enabled properly.
>    * Dump illegal opcode data with -d unimp.
>    * Use gen_nop_modrm for prefetch.
>    * Fix Windows XP booting.
>
> The illegal opcode dumping was useful for debugging.  The prefetch
> patch I noticed while reviewing the addr16 patch wrt lea.
>
>
> r~
>
>
> Paolo Bonzini (3):
>    target-i386: avoid repeated calls to the bnd_jmp helper
>    target-i386: fix smsw and lmsw from/to register
>    target-i386: fix addr16 prefix
>
> Richard Henderson (4):
>    target-i386: Fix SMSW for 64-bit mode
>    target-i386: Dump illegal opcodes with -d unimp
>    target-i386: Use gen_nop_modrm for prefetch instructions
>    target-i386: Fix inhibit irq mask handling
>
>   target-i386/translate.c | 179 +++++++++++++++++++++++++++---------------------
>   1 file changed, 100 insertions(+), 79 deletions(-)
>

Tested-by: Hervé Poussineau <hpoussin@reactos.org>

Regressions in MS-DOS 6 / Windows 9x / Windows XP are fixed.

Hervé