[PATCH] target/i386: move prefetch and multi-byte UD/NOP to new decoder

[PATCH] target/i386: move prefetch and multi-byte UD/NOP to new decoder

[Paolo Bonzini]

These are trivial to add, and moving them to the new decoder fixes some
corner cases: raising #UD instead of an instruction fetch page fault for
the undefined opcodes, and incorrectly rejecting 0F 18 prefetches with
register operands (which are treated as reserved NOPs).

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/decode-new.h     |  1 +
 target/i386/tcg/translate.c      | 30 ------------------------------
 target/i386/tcg/decode-new.c.inc | 24 +++++++++++++++++++++---
 target/i386/tcg/emit.c.inc       |  5 +++++
 4 files changed, 27 insertions(+), 33 deletions(-)

diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index 2ea06b44787..51ef0e621b9 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -50,6 +50,7 @@ typedef enum X86OpType {
     X86_TYPE_EM, /* modrm byte selects an ALU memory operand */
     X86_TYPE_WM, /* modrm byte selects an XMM/YMM memory operand */
     X86_TYPE_I_unsigned, /* Immediate, zero-extended */
+    X86_TYPE_nop, /* modrm operand decoded but not loaded into s->T{0,1} */
     X86_TYPE_2op, /* 2-operand RMW instruction */
     X86_TYPE_LoBits, /* encoded in bits 0-2 of the operand + REX.B */
     X86_TYPE_0, /* Hard-coded GPRs (RAX..RDI) */
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 3da4fdf64cc..de87775016b 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -4019,25 +4019,6 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
             set_cc_op(s, CC_OP_EFLAGS);
         }
         break;
-    case 0x118:
-        modrm = x86_ldub_code(env, s);
-        mod = (modrm >> 6) & 3;
-        op = (modrm >> 3) & 7;
-        switch(op) {
-        case 0: /* prefetchnta */
-        case 1: /* prefetchnt0 */
-        case 2: /* prefetchnt0 */
-        case 3: /* prefetchnt0 */
-            if (mod == 3)
-                goto illegal_op;
-            gen_nop_modrm(env, s, modrm);
-            /* nothing more to do */
-            break;
-        default: /* nop (multi byte) */
-            gen_nop_modrm(env, s, modrm);
-            break;
-        }
-        break;
     case 0x11a:
         modrm = x86_ldub_code(env, s);
         if (s->flags & HF_MPX_EN_MASK) {
@@ -4229,10 +4210,6 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
         }
         gen_nop_modrm(env, s, modrm);
         break;
-    case 0x119: case 0x11c ... 0x11f: /* nop (multi byte) */
-        modrm = x86_ldub_code(env, s);
-        gen_nop_modrm(env, s, modrm);
-        break;
 
     case 0x120: /* mov reg, crN */
     case 0x122: /* mov crN, reg */
@@ -4506,13 +4483,6 @@ static void disas_insn_old(DisasContext *s, CPUState *cpu, int b)
         }
         break;
 
-    case 0x10d: /* 3DNow! prefetch(w) */
-        modrm = x86_ldub_code(env, s);
-        mod = (modrm >> 6) & 3;
-        if (mod == 3)
-            goto illegal_op;
-        gen_nop_modrm(env, s, modrm);
-        break;
     case 0x1aa: /* rsm */
         gen_svm_check_intercept(s, SVM_EXIT_RSM);
         if (!(s->flags & HF_SMM_MASK))
diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index 0e1811399f8..4baf7672158 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -55,6 +55,10 @@
  * mask could be applied (and the original sign-extended value would be
  * optimized away by TCG) in the emitter function.
  *
+ * Finally, a "nop" operand type is used for multi-byte NOPs.  It accepts
+ * any value of mod including 11b (unlike M) but it does not try to
+ * interpret the operand (like M).
+ *
  * Vector operands
  * ---------------
  *
@@ -1056,6 +1060,16 @@ static const X86OpEntry opcodes_0F[256] = {
     [0xa0] = X86_OP_ENTRYr(PUSH, FS, w),
     [0xa1] = X86_OP_ENTRYw(POP, FS, w),
 
+    [0x0b] = X86_OP_ENTRY0(UD),           /* UD2 */
+    [0x0d] = X86_OP_ENTRY1(NOP,  M,v),    /* 3DNow! prefetch */
+
+    [0x18] = X86_OP_ENTRY1(NOP,  nop,v),  /* prefetch/reserved NOP */
+    [0x19] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
+    [0x1c] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
+    [0x1d] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
+    [0x1e] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
+    [0x1f] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
+
     [0x28] = X86_OP_ENTRY3(MOVDQ,      V,x,  None,None, W,x, vex1 p_00_66), /* MOVAPS */
     [0x29] = X86_OP_ENTRY3(MOVDQ,      W,x,  None,None, V,x, vex1 p_00_66), /* MOVAPS */
     [0x2A] = X86_OP_GROUP0(0F2A),
@@ -1135,6 +1149,8 @@ static const X86OpEntry opcodes_0F[256] = {
     [0xb6] = X86_OP_ENTRY3(MOV,    G,v, E,b, None, None, zextT0), /* MOVZX */
     [0xb7] = X86_OP_ENTRY3(MOV,    G,v, E,w, None, None, zextT0), /* MOVZX */
 
+    /* decoded as modrm, which is visible as a difference between page fault and #UD */
+    [0xb9] = X86_OP_ENTRYr(UD,     nop,v),                        /* UD1 */
     [0xbe] = X86_OP_ENTRY3(MOV,    G,v, E,b, None, None, sextT0), /* MOVSX */
     [0xbf] = X86_OP_ENTRY3(MOV,    G,v, E,w, None, None, sextT0), /* MOVSX */
 
@@ -1206,7 +1222,7 @@ static const X86OpEntry opcodes_0F[256] = {
     [0xfc] = X86_OP_ENTRY3(PADDB,  V,x, H,x, W,x,  vex4 mmx avx2_256 p_00_66),
     [0xfd] = X86_OP_ENTRY3(PADDW,  V,x, H,x, W,x,  vex4 mmx avx2_256 p_00_66),
     [0xfe] = X86_OP_ENTRY3(PADDD,  V,x, H,x, W,x,  vex4 mmx avx2_256 p_00_66),
-    /* 0xff = UD0 */
+    [0xff] = X86_OP_ENTRYr(UD,     nop,v),                        /* UD0 */
 };
 
 static void do_decode_0F(DisasContext *s, CPUX86State *env, X86OpEntry *entry, uint8_t *b)
@@ -1852,6 +1868,8 @@ static bool decode_op(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode,
         if ((modrm >> 6) == 3) {
             return false;
         }
+        /* fall through */
+    case X86_TYPE_nop:  /* modrm operand decoded but not fetched */
     get_modrm:
         decode_modrm(s, env, decode, op, type);
         break;
@@ -2397,8 +2415,8 @@ static void disas_insn(DisasContext *s, CPUState *cpu)
             switch (b) {
             case 0x00 ... 0x03: /* mostly privileged instructions */
             case 0x05 ... 0x09:
-            case 0x0d:          /* 3DNow! prefetch */
-            case 0x18 ... 0x23: /* prefetch, MPX, mov from/to CR and DR */
+            case 0x1a ... 0x1b: /* MPX */
+            case 0x20 ... 0x23: /* mov from/to CR and DR */
             case 0x30 ... 0x35: /* more privileged instructions */
             case 0xa2 ... 0xa5: /* CPUID, BT, SHLD */
             case 0xaa ... 0xae: /* RSM, SHRD, grp15 */
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 58f255873ff..2dee33dd487 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -3517,6 +3517,11 @@ static void gen_SUB(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
     prepare_update2_cc(decode, s, CC_OP_SUBB + ot);
 }
 
+static void gen_UD(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
+{
+    gen_illegal_opcode(s);
+}
+
 static void gen_VAESIMC(DisasContext *s, CPUX86State *env, X86DecodedInsn *decode)
 {
     assert(!s->vex_l);
-- 
2.45.0

Re: [PATCH] target/i386: move prefetch and multi-byte UD/NOP to new decoder

[Zhao Liu]

On Thu, May 09, 2024 at 05:37:55PM +0200, Paolo Bonzini wrote:
> Date: Thu,  9 May 2024 17:37:55 +0200
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH] target/i386: move prefetch and multi-byte UD/NOP to new
>  decoder
> X-Mailer: git-send-email 2.45.0
> 
> These are trivial to add, and moving them to the new decoder fixes some
> corner cases: raising #UD instead of an instruction fetch page fault for
> the undefined opcodes, and incorrectly rejecting 0F 18 prefetches with
> register operands (which are treated as reserved NOPs).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  target/i386/tcg/decode-new.h     |  1 +
>  target/i386/tcg/translate.c      | 30 ------------------------------
>  target/i386/tcg/decode-new.c.inc | 24 +++++++++++++++++++++---
>  target/i386/tcg/emit.c.inc       |  5 +++++
>  4 files changed, 27 insertions(+), 33 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH] target/i386: move prefetch and multi-byte UD/NOP to new decoder

[Richard Henderson]

On 5/9/24 17:37, Paolo Bonzini wrote:
> +    [0x18] = X86_OP_ENTRY1(NOP,  nop,v),  /* prefetch/reserved NOP */
> +    [0x19] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
> +    [0x1c] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
> +    [0x1d] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
> +    [0x1e] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */
> +    [0x1f] = X86_OP_ENTRY1(NOP,  nop,v),  /* reserved NOP */

Maybe clearer as "NOP/reserved NOP" for 0x1f.

Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~