[PATCH 0/4] x86emul: more misc small adjustments

[PATCH 0/4] x86emul: more misc small adjustments

[Jan Beulich]

Along the lines of and on top of
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01744.html
here are a few more.

1: remove dead code
2: drop RIP-relative special case for TEST
3: drop SrcInvalid
4: use DstEax also for XCHG

Signed-off-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/4] x86emul: remove dead code

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

As of commit 989cdfa9b4 ("x86emul: don't special case fetching unsigned
8-bit immediates") the conditional being removed has been always false.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1856,9 +1856,6 @@ x86_emulate(
                     /* Special case in Grp3: test has immediate operand. */
                     ea.mem.off += (d & ByteOp) ? 1
                         : ((op_bytes == 8) ? 4 : op_bytes);
-                else if ( ext == ext_0f && ((b & 0xf7) == 0xa4) )
-                    /* SHLD/SHRD with immediate byte third operand. */
-                    ea.mem.off++;
                 break;
             case 1:
                 ea.mem.off += insn_fetch_type(int8_t);




[-- Attachment #2: x86emul-SHxD-dead-code.patch --]
[-- Type: text/plain, Size: 829 bytes --]

x86emul: remove dead code

As of commit 989cdfa9b4 ("x86emul: don't special case fetching unsigned
8-bit immediates") the conditional being removed has been always false.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1856,9 +1856,6 @@ x86_emulate(
                     /* Special case in Grp3: test has immediate operand. */
                     ea.mem.off += (d & ByteOp) ? 1
                         : ((op_bytes == 8) ? 4 : op_bytes);
-                else if ( ext == ext_0f && ((b & 0xf7) == 0xa4) )
-                    /* SHLD/SHRD with immediate byte third operand. */
-                    ea.mem.off++;
                 break;
             case 1:
                 ea.mem.off += insn_fetch_type(int8_t);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/4] x86emul: drop RIP-relative special case for TEST

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 5141 bytes --]

Moving ahead the "early operand adjustments" logic, the "test $imm,r/m"
special logic in the determination of the instruction boundary is no
longer necessary.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1742,6 +1742,66 @@ x86_emulate(
         modrm_reg = ((rex_prefix & 4) << 1) | ((modrm & 0x38) >> 3);
         modrm_rm  = modrm & 0x07;
 
+        /* Early operand adjustments. */
+        switch ( ext )
+        {
+        case ext_none:
+            switch ( b )
+            {
+            case 0xf6 ... 0xf7: /* Grp3 */
+                switch ( modrm_reg & 7 )
+                {
+                case 0 ... 1: /* test */
+                    d = (d & ~SrcMask) | SrcImm;
+                    break;
+                case 4: /* mul */
+                case 5: /* imul */
+                case 6: /* div */
+                case 7: /* idiv */
+                    d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem;
+                    break;
+                }
+                break;
+            case 0xff: /* Grp5 */
+                switch ( modrm_reg & 7 )
+                {
+                case 2: /* call (near) */
+                case 4: /* jmp (near) */
+                case 6: /* push */
+                    if ( mode_64bit() && op_bytes == 4 )
+                        op_bytes = 8;
+                    /* fall through */
+                case 3: /* call (far, absolute indirect) */
+                case 5: /* jmp (far, absolute indirect) */
+                    d = DstNone | SrcMem | ModRM | Mov;
+                    break;
+                }
+                break;
+            }
+            break;
+
+        case ext_0f:
+            break;
+
+        case ext_0f38:
+            switch ( b )
+            {
+            case 0xf0: /* movbe / crc32 */
+                d |= repne_prefix() ? ByteOp : Mov;
+                break;
+            case 0xf1: /* movbe / crc32 */
+                if ( !repne_prefix() )
+                    d = (d & ~(DstMask | SrcMask)) | DstMem | SrcReg | Mov;
+                break;
+            default: /* Until it is worth making this table based ... */
+                goto cannot_emulate;
+            }
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+        }
+
         if ( modrm_mod == 3 )
         {
             modrm_rm |= (rex_prefix & 1) << 3;
@@ -1851,11 +1911,6 @@ x86_emulate(
                         ((op_bytes == 8) ? 4 : op_bytes);
                 else if ( (d & SrcMask) == SrcImmByte )
                     ea.mem.off += 1;
-                else if ( !ext && ((b & 0xfe) == 0xf6) &&
-                          ((modrm_reg & 7) <= 1) )
-                    /* Special case in Grp3: test has immediate operand. */
-                    ea.mem.off += (d & ByteOp) ? 1
-                        : ((op_bytes == 8) ? 4 : op_bytes);
                 break;
             case 1:
                 ea.mem.off += insn_fetch_type(int8_t);
@@ -1871,66 +1926,6 @@ x86_emulate(
     if ( override_seg != -1 && ea.type == OP_MEM )
         ea.mem.seg = override_seg;
 
-    /* Early operand adjustments. */
-    switch ( ext )
-    {
-    case ext_none:
-        switch ( b )
-        {
-        case 0xf6 ... 0xf7: /* Grp3 */
-            switch ( modrm_reg & 7 )
-            {
-            case 0 ... 1: /* test */
-                d = (d & ~SrcMask) | SrcImm;
-                break;
-            case 4: /* mul */
-            case 5: /* imul */
-            case 6: /* div */
-            case 7: /* idiv */
-                d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem;
-                break;
-            }
-            break;
-        case 0xff: /* Grp5 */
-            switch ( modrm_reg & 7 )
-            {
-            case 2: /* call (near) */
-            case 4: /* jmp (near) */
-            case 6: /* push */
-                if ( mode_64bit() && op_bytes == 4 )
-                    op_bytes = 8;
-                /* fall through */
-            case 3: /* call (far, absolute indirect) */
-            case 5: /* jmp (far, absolute indirect) */
-                d = DstNone | SrcMem | ModRM | Mov;
-                break;
-            }
-            break;
-        }
-        break;
-
-    case ext_0f:
-        break;
-
-    case ext_0f38:
-        switch ( b )
-        {
-        case 0xf0: /* movbe / crc32 */
-            d |= repne_prefix() ? ByteOp : Mov;
-            break;
-        case 0xf1: /* movbe / crc32 */
-            if ( !repne_prefix() )
-                d = (d & ~(DstMask | SrcMask)) | DstMem | SrcReg | Mov;
-            break;
-        default: /* Until it is worth making this table based ... */
-            goto cannot_emulate;
-        }
-        break;
-
-    default:
-        ASSERT_UNREACHABLE();
-    }
-
     /* Decode and fetch the source operand: register, memory or immediate. */
     switch ( d & SrcMask )
     {



[-- Attachment #2: x86emul-RIPrel-drop-special-case.patch --]
[-- Type: text/plain, Size: 5189 bytes --]

x86emul: drop RIP-relative special case for TEST

Moving ahead the "early operand adjustments" logic, the "test $imm,r/m"
special logic in the determination of the instruction boundary is no
longer necessary.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1742,6 +1742,66 @@ x86_emulate(
         modrm_reg = ((rex_prefix & 4) << 1) | ((modrm & 0x38) >> 3);
         modrm_rm  = modrm & 0x07;
 
+        /* Early operand adjustments. */
+        switch ( ext )
+        {
+        case ext_none:
+            switch ( b )
+            {
+            case 0xf6 ... 0xf7: /* Grp3 */
+                switch ( modrm_reg & 7 )
+                {
+                case 0 ... 1: /* test */
+                    d = (d & ~SrcMask) | SrcImm;
+                    break;
+                case 4: /* mul */
+                case 5: /* imul */
+                case 6: /* div */
+                case 7: /* idiv */
+                    d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem;
+                    break;
+                }
+                break;
+            case 0xff: /* Grp5 */
+                switch ( modrm_reg & 7 )
+                {
+                case 2: /* call (near) */
+                case 4: /* jmp (near) */
+                case 6: /* push */
+                    if ( mode_64bit() && op_bytes == 4 )
+                        op_bytes = 8;
+                    /* fall through */
+                case 3: /* call (far, absolute indirect) */
+                case 5: /* jmp (far, absolute indirect) */
+                    d = DstNone | SrcMem | ModRM | Mov;
+                    break;
+                }
+                break;
+            }
+            break;
+
+        case ext_0f:
+            break;
+
+        case ext_0f38:
+            switch ( b )
+            {
+            case 0xf0: /* movbe / crc32 */
+                d |= repne_prefix() ? ByteOp : Mov;
+                break;
+            case 0xf1: /* movbe / crc32 */
+                if ( !repne_prefix() )
+                    d = (d & ~(DstMask | SrcMask)) | DstMem | SrcReg | Mov;
+                break;
+            default: /* Until it is worth making this table based ... */
+                goto cannot_emulate;
+            }
+            break;
+
+        default:
+            ASSERT_UNREACHABLE();
+        }
+
         if ( modrm_mod == 3 )
         {
             modrm_rm |= (rex_prefix & 1) << 3;
@@ -1851,11 +1911,6 @@ x86_emulate(
                         ((op_bytes == 8) ? 4 : op_bytes);
                 else if ( (d & SrcMask) == SrcImmByte )
                     ea.mem.off += 1;
-                else if ( !ext && ((b & 0xfe) == 0xf6) &&
-                          ((modrm_reg & 7) <= 1) )
-                    /* Special case in Grp3: test has immediate operand. */
-                    ea.mem.off += (d & ByteOp) ? 1
-                        : ((op_bytes == 8) ? 4 : op_bytes);
                 break;
             case 1:
                 ea.mem.off += insn_fetch_type(int8_t);
@@ -1871,66 +1926,6 @@ x86_emulate(
     if ( override_seg != -1 && ea.type == OP_MEM )
         ea.mem.seg = override_seg;
 
-    /* Early operand adjustments. */
-    switch ( ext )
-    {
-    case ext_none:
-        switch ( b )
-        {
-        case 0xf6 ... 0xf7: /* Grp3 */
-            switch ( modrm_reg & 7 )
-            {
-            case 0 ... 1: /* test */
-                d = (d & ~SrcMask) | SrcImm;
-                break;
-            case 4: /* mul */
-            case 5: /* imul */
-            case 6: /* div */
-            case 7: /* idiv */
-                d = (d & (ByteOp | ModRM)) | DstImplicit | SrcMem;
-                break;
-            }
-            break;
-        case 0xff: /* Grp5 */
-            switch ( modrm_reg & 7 )
-            {
-            case 2: /* call (near) */
-            case 4: /* jmp (near) */
-            case 6: /* push */
-                if ( mode_64bit() && op_bytes == 4 )
-                    op_bytes = 8;
-                /* fall through */
-            case 3: /* call (far, absolute indirect) */
-            case 5: /* jmp (far, absolute indirect) */
-                d = DstNone | SrcMem | ModRM | Mov;
-                break;
-            }
-            break;
-        }
-        break;
-
-    case ext_0f:
-        break;
-
-    case ext_0f38:
-        switch ( b )
-        {
-        case 0xf0: /* movbe / crc32 */
-            d |= repne_prefix() ? ByteOp : Mov;
-            break;
-        case 0xf1: /* movbe / crc32 */
-            if ( !repne_prefix() )
-                d = (d & ~(DstMask | SrcMask)) | DstMem | SrcReg | Mov;
-            break;
-        default: /* Until it is worth making this table based ... */
-            goto cannot_emulate;
-        }
-        break;
-
-    default:
-        ASSERT_UNREACHABLE();
-    }
-
     /* Decode and fetch the source operand: register, memory or immediate. */
     switch ( d & SrcMask )
     {

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/4] x86emul: drop SrcInvalid

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

As of commit a800e4f611 ("x86emul: drop pointless and add useful
default cases") we no longer need the early bailing when "d == 0" (the
default cases in the main switch() statements take care of that),
removal of which renders internal_error() wrong and SrcInvalid useless.
Drop them, as they're going to get in the way of completing the decoder
to cover all known insns (to allow it to be used by more callers)
without at the same time completing the actual emulation logic.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -31,15 +31,14 @@
 #define DstMem      (3<<1) /* Memory operand. */
 #define DstMask     (3<<1)
 /* Source operand type. */
-#define SrcInvalid  (0<<3) /* Unimplemented opcode. */
-#define SrcNone     (1<<3) /* No source operand. */
-#define SrcImplicit (1<<3) /* Source operand is implicit in the opcode. */
-#define SrcReg      (2<<3) /* Register operand. */
-#define SrcMem      (3<<3) /* Memory operand. */
-#define SrcMem16    (4<<3) /* Memory operand (16-bit). */
-#define SrcImm      (5<<3) /* Immediate operand. */
-#define SrcImmByte  (6<<3) /* 8-bit sign-extended immediate operand. */
-#define SrcImm16    (7<<3) /* 16-bit zero-extended immediate operand. */
+#define SrcNone     (0<<3) /* No source operand. */
+#define SrcImplicit (0<<3) /* Source operand is implicit in the opcode. */
+#define SrcReg      (1<<3) /* Register operand. */
+#define SrcMem      (2<<3) /* Memory operand. */
+#define SrcMem16    (3<<3) /* Memory operand (16-bit). */
+#define SrcImm      (4<<3) /* Immediate operand. */
+#define SrcImmByte  (5<<3) /* 8-bit sign-extended immediate operand. */
+#define SrcImm16    (6<<3) /* 16-bit zero-extended immediate operand. */
 #define SrcMask     (7<<3)
 /* Generic ModRM decode. */
 #define ModRM       (1<<6)
@@ -1532,19 +1531,6 @@ int x86emul_unhandleable_rw(
     return X86EMUL_UNHANDLEABLE;
 }
 
-static void internal_error(const char *which, uint8_t byte,
-                           const struct cpu_user_regs *regs)
-{
-#ifdef __XEN__
-    static bool_t logged;
-
-    if ( !test_and_set_bool(logged) )
-        gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
-                which, byte, regs->cs, regs->eip);
-#endif
-    ASSERT_UNREACHABLE();
-}
-
 int
 x86_emulate(
     struct x86_emulate_ctxt *ctxt,
@@ -1664,10 +1650,6 @@ x86_emulate(
                 break;
             }
         }
-
-        /* Unrecognised? */
-        if ( d == 0 )
-            goto cannot_emulate;
     }
 
     /* Lock prefix is allowed only on RMW instructions. */
@@ -1729,10 +1711,6 @@ x86_emulate(
                 b = insn_fetch_type(uint8_t);
                 d = twobyte_table[b];
 
-                /* Unrecognised? */
-                if ( d == 0 || b == 0x38 )
-                    goto cannot_emulate;
-
                 modrm = insn_fetch_type(uint8_t);
                 modrm_mod = (modrm & 0xc0) >> 6;
 
@@ -3851,7 +3829,6 @@ x86_emulate(
         break;
 
     default:
-        internal_error("primary", b, ctxt->regs);
         goto cannot_emulate;
     }
 
@@ -4837,7 +4814,6 @@ x86_emulate(
         break;
 
     default:
-        internal_error("secondary", b, ctxt->regs);
         goto cannot_emulate;
     }
     goto writeback;




[-- Attachment #2: x86emul-drop-SrcInvalid.patch --]
[-- Type: text/plain, Size: 3433 bytes --]

x86emul: drop SrcInvalid

As of commit a800e4f611 ("x86emul: drop pointless and add useful
default cases") we no longer need the early bailing when "d == 0" (the
default cases in the main switch() statements take care of that),
removal of which renders internal_error() wrong and SrcInvalid useless.
Drop them, as they're going to get in the way of completing the decoder
to cover all known insns (to allow it to be used by more callers)
without at the same time completing the actual emulation logic.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -31,15 +31,14 @@
 #define DstMem      (3<<1) /* Memory operand. */
 #define DstMask     (3<<1)
 /* Source operand type. */
-#define SrcInvalid  (0<<3) /* Unimplemented opcode. */
-#define SrcNone     (1<<3) /* No source operand. */
-#define SrcImplicit (1<<3) /* Source operand is implicit in the opcode. */
-#define SrcReg      (2<<3) /* Register operand. */
-#define SrcMem      (3<<3) /* Memory operand. */
-#define SrcMem16    (4<<3) /* Memory operand (16-bit). */
-#define SrcImm      (5<<3) /* Immediate operand. */
-#define SrcImmByte  (6<<3) /* 8-bit sign-extended immediate operand. */
-#define SrcImm16    (7<<3) /* 16-bit zero-extended immediate operand. */
+#define SrcNone     (0<<3) /* No source operand. */
+#define SrcImplicit (0<<3) /* Source operand is implicit in the opcode. */
+#define SrcReg      (1<<3) /* Register operand. */
+#define SrcMem      (2<<3) /* Memory operand. */
+#define SrcMem16    (3<<3) /* Memory operand (16-bit). */
+#define SrcImm      (4<<3) /* Immediate operand. */
+#define SrcImmByte  (5<<3) /* 8-bit sign-extended immediate operand. */
+#define SrcImm16    (6<<3) /* 16-bit zero-extended immediate operand. */
 #define SrcMask     (7<<3)
 /* Generic ModRM decode. */
 #define ModRM       (1<<6)
@@ -1532,19 +1531,6 @@ int x86emul_unhandleable_rw(
     return X86EMUL_UNHANDLEABLE;
 }
 
-static void internal_error(const char *which, uint8_t byte,
-                           const struct cpu_user_regs *regs)
-{
-#ifdef __XEN__
-    static bool_t logged;
-
-    if ( !test_and_set_bool(logged) )
-        gprintk(XENLOG_ERR, "Internal error: %s/%02x [%04x:%08lx]\n",
-                which, byte, regs->cs, regs->eip);
-#endif
-    ASSERT_UNREACHABLE();
-}
-
 int
 x86_emulate(
     struct x86_emulate_ctxt *ctxt,
@@ -1664,10 +1650,6 @@ x86_emulate(
                 break;
             }
         }
-
-        /* Unrecognised? */
-        if ( d == 0 )
-            goto cannot_emulate;
     }
 
     /* Lock prefix is allowed only on RMW instructions. */
@@ -1729,10 +1711,6 @@ x86_emulate(
                 b = insn_fetch_type(uint8_t);
                 d = twobyte_table[b];
 
-                /* Unrecognised? */
-                if ( d == 0 || b == 0x38 )
-                    goto cannot_emulate;
-
                 modrm = insn_fetch_type(uint8_t);
                 modrm_mod = (modrm & 0xc0) >> 6;
 
@@ -3851,7 +3829,6 @@ x86_emulate(
         break;
 
     default:
-        internal_error("primary", b, ctxt->regs);
         goto cannot_emulate;
     }
 
@@ -4837,7 +4814,6 @@ x86_emulate(
         break;
 
     default:
-        internal_error("secondary", b, ctxt->regs);
         goto cannot_emulate;
     }
     goto writeback;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/4] x86emul: use DstEax also for XCHG

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
possible"): While it avoids just a few instructions, we should
nevertheless make use of generic code as much as possible. Here we can
arrange for that by simply swapping source and destination (as they're
interchangeable).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
     DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
     DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
     /* 0x90 - 0x97 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
@@ -2491,16 +2493,14 @@ x86_emulate(
 
     case 0x90: /* nop / xchg %%r8,%%rax */
         if ( !(rex_prefix & 1) )
-            break; /* nop */
+            goto no_writeback; /* nop */
 
     case 0x91 ... 0x97: /* xchg reg,%%rax */
-        src.type = dst.type = OP_REG;
-        src.bytes = dst.bytes = op_bytes;
-        src.reg  = (unsigned long *)&_regs.eax;
-        src.val  = *src.reg;
-        dst.reg  = decode_register(
+        src.type = OP_REG;
+        src.bytes = op_bytes;
+        src.reg  = decode_register(
             (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
-        dst.val  = *dst.reg;
+        src.val  = *src.reg;
         goto xchg;
 
     case 0x98: /* cbw/cwde/cdqe */




[-- Attachment #2: x86emul-use-DstEax-xchg.patch --]
[-- Type: text/plain, Size: 1847 bytes --]

x86emul: use DstEax also for XCHG

Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
possible"): While it avoids just a few instructions, we should
nevertheless make use of generic code as much as possible. Here we can
arrange for that by simply swapping source and destination (as they're
interchangeable).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
     DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
     DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
     /* 0x90 - 0x97 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
+    DstEax|SrcImplicit, DstEax|SrcImplicit,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
@@ -2491,16 +2493,14 @@ x86_emulate(
 
     case 0x90: /* nop / xchg %%r8,%%rax */
         if ( !(rex_prefix & 1) )
-            break; /* nop */
+            goto no_writeback; /* nop */
 
     case 0x91 ... 0x97: /* xchg reg,%%rax */
-        src.type = dst.type = OP_REG;
-        src.bytes = dst.bytes = op_bytes;
-        src.reg  = (unsigned long *)&_regs.eax;
-        src.val  = *src.reg;
-        dst.reg  = decode_register(
+        src.type = OP_REG;
+        src.bytes = op_bytes;
+        src.reg  = decode_register(
             (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
-        dst.val  = *dst.reg;
+        src.val  = *src.reg;
         goto xchg;
 
     case 0x98: /* cbw/cwde/cdqe */

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] x86emul: remove dead code

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 293 bytes --]

On 15/08/16 09:34, Jan Beulich wrote:
> As of commit 989cdfa9b4 ("x86emul: don't special case fetching unsigned
> 8-bit immediates") the conditional being removed has been always false.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[-- Attachment #1.2: Type: text/html, Size: 846 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] x86emul: drop RIP-relative special case for TEST

[Andrew Cooper]

On 15/08/16 09:34, Jan Beulich wrote:
> @@ -1851,11 +1911,6 @@ x86_emulate(
>                          ((op_bytes == 8) ? 4 : op_bytes);
>                  else if ( (d & SrcMask) == SrcImmByte )
>                      ea.mem.off += 1;
> -                else if ( !ext && ((b & 0xfe) == 0xf6) &&
> -                          ((modrm_reg & 7) <= 1) )

Do we actually handle these cases correctly?  0xf6 /0 (imm8) and 0xf7 /0
(imm) look to work as expected

However, 0xf6 /1, 0xf7 /1 are harder to pin down.  We have an
implementation of it, but the only other reference I can find to them
are in the AMD grp3 opcode map, where they appear equal to their /0
variants.  The /1 variants do not appear in the AMD description of the
TEST instruction, and do not appear anywhere in the Intel manuals.

Suravee: Can you confirm whether the /1 variants are expected to be
implemented and copies of the /0 variants?

~Andrew

> -                    /* Special case in Grp3: test has immediate operand. */
> -                    ea.mem.off += (d & ByteOp) ? 1
> -                        : ((op_bytes == 8) ? 4 : op_bytes);
>                  break;
>              case 1:
>                  ea.mem.off += insn_fetch_type(int8_t);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] x86emul: drop RIP-relative special case for TEST

[Jan Beulich]

>>> On 15.08.16 at 16:25, <andrew.cooper3@citrix.com> wrote:
> On 15/08/16 09:34, Jan Beulich wrote:
>> @@ -1851,11 +1911,6 @@ x86_emulate(
>>                          ((op_bytes == 8) ? 4 : op_bytes);
>>                  else if ( (d & SrcMask) == SrcImmByte )
>>                      ea.mem.off += 1;
>> -                else if ( !ext && ((b & 0xfe) == 0xf6) &&
>> -                          ((modrm_reg & 7) <= 1) )
> 
> Do we actually handle these cases correctly?  0xf6 /0 (imm8) and 0xf7 /0
> (imm) look to work as expected

I think we do; what makes you think we might not?

> However, 0xf6 /1, 0xf7 /1 are harder to pin down.  We have an
> implementation of it, but the only other reference I can find to them
> are in the AMD grp3 opcode map, where they appear equal to their /0
> variants.  The /1 variants do not appear in the AMD description of the
> TEST instruction, and do not appear anywhere in the Intel manuals.
> 
> Suravee: Can you confirm whether the /1 variants are expected to be
> implemented and copies of the /0 variants?

I've check on Intel systems that they are aliases (Intel doesn't
document them), and AMD halfway documenting them as aliases
makes me assume they indeed are. Other than opcode 82 they're
also aliases regardless of whether in 64-bit mode (on Intel again,
that is, I can't get to my AMD boxes right now).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] x86emul: drop RIP-relative special case for TEST

[Jan Beulich]

>>> On 15.08.16 at 16:25, <andrew.cooper3@citrix.com> wrote:
> On 15/08/16 09:34, Jan Beulich wrote:
>> @@ -1851,11 +1911,6 @@ x86_emulate(
>>                          ((op_bytes == 8) ? 4 : op_bytes);
>>                  else if ( (d & SrcMask) == SrcImmByte )
>>                      ea.mem.off += 1;
>> -                else if ( !ext && ((b & 0xfe) == 0xf6) &&
>> -                          ((modrm_reg & 7) <= 1) )
> 
> Do we actually handle these cases correctly?  0xf6 /0 (imm8) and 0xf7 /0
> (imm) look to work as expected
> 
> However, 0xf6 /1, 0xf7 /1 are harder to pin down.  We have an
> implementation of it, but the only other reference I can find to them
> are in the AMD grp3 opcode map, where they appear equal to their /0
> variants.  The /1 variants do not appear in the AMD description of the
> TEST instruction, and do not appear anywhere in the Intel manuals.

And btw., your questions are kind of orthogonal to the purpose of
this patch, which doesn't change which opcodes we do or do not
emulate. It solely slightly re-orders how we do so.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] x86emul: drop RIP-relative special case for TEST

[Andrew Cooper]

On 15/08/16 16:08, Jan Beulich wrote:
>>>> On 15.08.16 at 16:25, <andrew.cooper3@citrix.com> wrote:
>> On 15/08/16 09:34, Jan Beulich wrote:
>>> @@ -1851,11 +1911,6 @@ x86_emulate(
>>>                          ((op_bytes == 8) ? 4 : op_bytes);
>>>                  else if ( (d & SrcMask) == SrcImmByte )
>>>                      ea.mem.off += 1;
>>> -                else if ( !ext && ((b & 0xfe) == 0xf6) &&
>>> -                          ((modrm_reg & 7) <= 1) )
>> Do we actually handle these cases correctly?  0xf6 /0 (imm8) and 0xf7 /0
>> (imm) look to work as expected
> I think we do; what makes you think we might not?
>
>> However, 0xf6 /1, 0xf7 /1 are harder to pin down.  We have an
>> implementation of it, but the only other reference I can find to them
>> are in the AMD grp3 opcode map, where they appear equal to their /0
>> variants.  The /1 variants do not appear in the AMD description of the
>> TEST instruction, and do not appear anywhere in the Intel manuals.
>>
>> Suravee: Can you confirm whether the /1 variants are expected to be
>> implemented and copies of the /0 variants?
> I've check on Intel systems that they are aliases (Intel doesn't
> document them), and AMD halfway documenting them as aliases
> makes me assume they indeed are. Other than opcode 82 they're
> also aliases regardless of whether in 64-bit mode (on Intel again,
> that is, I can't get to my AMD boxes right now).

How helpful.  It would be lovely if the documentation matched reality.

Anyway, the change isn't really related to this question, so
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] x86emul: drop SrcInvalid

[Andrew Cooper]

On 15/08/16 09:35, Jan Beulich wrote:
> As of commit a800e4f611 ("x86emul: drop pointless and add useful
> default cases") we no longer need the early bailing when "d == 0" (the
> default cases in the main switch() statements take care of that),
> removal of which renders internal_error() wrong and SrcInvalid useless.

"the removal of which".

However, SrcInvalid is already unused, irrespective of internal_error().

I don't however see how this renders internal_error() incorrect.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86emul: use DstEax also for XCHG

[Andrew Cooper]

On 15/08/16 09:35, Jan Beulich wrote:
> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
> possible"): While it avoids just a few instructions, we should
> nevertheless make use of generic code as much as possible. Here we can
> arrange for that by simply swapping source and destination (as they're
> interchangeable).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
>      DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
>      DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
>      /* 0x90 - 0x97 */
> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> +    DstEax|SrcImplicit, DstEax|SrcImplicit,

Please add a comment explaining that DstEax is interchangeable with
SrcEax in the xchg case.  Otherwise, the decode table reads incorrectly.

>      /* 0x98 - 0x9F */
>      ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>      ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
> @@ -2491,16 +2493,14 @@ x86_emulate(
>  
>      case 0x90: /* nop / xchg %%r8,%%rax */
>          if ( !(rex_prefix & 1) )
> -            break; /* nop */
> +            goto no_writeback; /* nop */

Could you add an explicit /* fallthrough */ here?  The only reason it
isn't currently a coverity defect is because of the /* nop */ comment.

With these two, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>  
>      case 0x91 ... 0x97: /* xchg reg,%%rax */
> -        src.type = dst.type = OP_REG;
> -        src.bytes = dst.bytes = op_bytes;
> -        src.reg  = (unsigned long *)&_regs.eax;
> -        src.val  = *src.reg;
> -        dst.reg  = decode_register(
> +        src.type = OP_REG;
> +        src.bytes = op_bytes;
> +        src.reg  = decode_register(
>              (b & 7) | ((rex_prefix & 1) << 3), &_regs, 0);
> -        dst.val  = *dst.reg;
> +        src.val  = *src.reg;
>          goto xchg;
>  
>      case 0x98: /* cbw/cwde/cdqe */
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] x86emul: drop SrcInvalid

[Jan Beulich]

>>> On 16.08.16 at 12:12, <andrew.cooper3@citrix.com> wrote:
> On 15/08/16 09:35, Jan Beulich wrote:
>> As of commit a800e4f611 ("x86emul: drop pointless and add useful
>> default cases") we no longer need the early bailing when "d == 0" (the
>> default cases in the main switch() statements take care of that),
>> removal of which renders internal_error() wrong and SrcInvalid useless.
> 
> "the removal of which".

Is the article really necessary in that case? So far I thought I had
learned it's optional in such situations.

> However, SrcInvalid is already unused, irrespective of internal_error().

Well, it's not explicitly referenced, but it having been zero and
the zero checks now getting dropped ...

> I don't however see how this renders internal_error() incorrect.

... both callers of internal_error() need to go away (perhaps I
simply used unclear wording, which obviously I could improve:
"renders both callers of internal_error() wrong"). IOW it is now
no longer an internal error to reach these default labels.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86emul: use DstEax also for XCHG

[Jan Beulich]

>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> wrote:
> On 15/08/16 09:35, Jan Beulich wrote:
>> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
>> possible"): While it avoids just a few instructions, we should
>> nevertheless make use of generic code as much as possible. Here we can
>> arrange for that by simply swapping source and destination (as they're
>> interchangeable).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
>>      DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
>>      DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
>>      /* 0x90 - 0x97 */
>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
> 
> Please add a comment explaining that DstEax is interchangeable with
> SrcEax in the xchg case.  Otherwise, the decode table reads incorrectly.

Do you mean me to do so even considering there's no SrcEax
(yet, it'll come with the not yet posted patch finally doing the
split off of the decode part)? (Nor can I see why the decode
table reads incorrectly the way it is above.)

>> @@ -2491,16 +2493,14 @@ x86_emulate(
>>  
>>      case 0x90: /* nop / xchg %%r8,%%rax */
>>          if ( !(rex_prefix & 1) )
>> -            break; /* nop */
>> +            goto no_writeback; /* nop */
> 
> Could you add an explicit /* fallthrough */ here?  The only reason it
> isn't currently a coverity defect is because of the /* nop */ comment.

Will do; I actually had considered that, but then thought the
present comment is enough to silence Coverity.

> With these two, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll wait with using this until the first point above got clarified.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86emul: use DstEax also for XCHG

[Andrew Cooper]

On 16/08/16 12:31, Jan Beulich wrote:
>>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> wrote:
>> On 15/08/16 09:35, Jan Beulich wrote:
>>> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
>>> possible"): While it avoids just a few instructions, we should
>>> nevertheless make use of generic code as much as possible. Here we can
>>> arrange for that by simply swapping source and destination (as they're
>>> interchangeable).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
>>>      DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
>>>      DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
>>>      /* 0x90 - 0x97 */
>>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>> Please add a comment explaining that DstEax is interchangeable with
>> SrcEax in the xchg case.  Otherwise, the decode table reads incorrectly.
> Do you mean me to do so even considering there's no SrcEax
> (yet, it'll come with the not yet posted patch finally doing the
> split off of the decode part)? (Nor can I see why the decode
> table reads incorrectly the way it is above.)

xchg is explicitly specified to have SrcEax, so people comparing the
instruction manuals to our implementation can be forgiven for thinking
that our code is wrong if it has DstEax instead.

If SrcEax is imminent then it perhaps it doesn't matter too much.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] x86emul: use DstEax also for XCHG

[Jan Beulich]

>>> On 16.08.16 at 14:46, <andrew.cooper3@citrix.com> wrote:
> On 16/08/16 12:31, Jan Beulich wrote:
>>>>> On 16.08.16 at 12:59, <andrew.cooper3@citrix.com> wrote:
>>> On 15/08/16 09:35, Jan Beulich wrote:
>>>> Just like said in commit c0bc0adf24 ("x86emul: use DstEax where
>>>> possible"): While it avoids just a few instructions, we should
>>>> nevertheless make use of generic code as much as possible. Here we can
>>>> arrange for that by simply swapping source and destination (as they're
>>>> interchangeable).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -118,8 +118,10 @@ static uint8_t opcode_table[256] = {
>>>>      DstMem|SrcReg|ModRM|Mov, DstReg|SrcNone|ModRM,
>>>>      DstReg|SrcMem16|ModRM|Mov, DstMem|SrcNone|ModRM|Mov,
>>>>      /* 0x90 - 0x97 */
>>>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>>>> -    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
>>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>>> +    DstEax|SrcImplicit, DstEax|SrcImplicit,
>>> Please add a comment explaining that DstEax is interchangeable with
>>> SrcEax in the xchg case.  Otherwise, the decode table reads incorrectly.
>> Do you mean me to do so even considering there's no SrcEax
>> (yet, it'll come with the not yet posted patch finally doing the
>> split off of the decode part)? (Nor can I see why the decode
>> table reads incorrectly the way it is above.)
> 
> xchg is explicitly specified to have SrcEax, so people comparing the
> instruction manuals to our implementation can be forgiven for thinking
> that our code is wrong if it has DstEax instead.
> 
> If SrcEax is imminent then it perhaps it doesn't matter too much.

Otoh I could obviously re-order this with the other one and
use SrcEax here, making for a slightly smaller overall change.
Or introduce SrcEax right here. Maybe that's the most natural
route to go.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] x86emul: drop SrcInvalid

[Andrew Cooper]

On 16/08/16 12:27, Jan Beulich wrote:
>>>> On 16.08.16 at 12:12, <andrew.cooper3@citrix.com> wrote:
>> On 15/08/16 09:35, Jan Beulich wrote:
>>> As of commit a800e4f611 ("x86emul: drop pointless and add useful
>>> default cases") we no longer need the early bailing when "d == 0" (the
>>> default cases in the main switch() statements take care of that),
>>> removal of which renders internal_error() wrong and SrcInvalid useless.
>> "the removal of which".
> Is the article really necessary in that case? So far I thought I had
> learned it's optional in such situations.

The sentence sounds wrong without it.

>
>> However, SrcInvalid is already unused, irrespective of internal_error().
> Well, it's not explicitly referenced, but it having been zero and
> the zero checks now getting dropped ...
>
>> I don't however see how this renders internal_error() incorrect.
> ... both callers of internal_error() need to go away (perhaps I
> simply used unclear wording, which obviously I could improve:
> "renders both callers of internal_error() wrong"). IOW it is now
> no longer an internal error to reach these default labels.

Ah - that makes more sense.  With suitable wording adjustments,
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] x86emul: drop SrcInvalid

[George Dunlap]

On Tue, Aug 16, 2016 at 2:34 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 16/08/16 12:27, Jan Beulich wrote:
>>>>> On 16.08.16 at 12:12, <andrew.cooper3@citrix.com> wrote:
>>> On 15/08/16 09:35, Jan Beulich wrote:
>>>> As of commit a800e4f611 ("x86emul: drop pointless and add useful
>>>> default cases") we no longer need the early bailing when "d == 0" (the
>>>> default cases in the main switch() statements take care of that),
>>>> removal of which renders internal_error() wrong and SrcInvalid useless.
>>> "the removal of which".
>> Is the article really necessary in that case? So far I thought I had
>> learned it's optional in such situations.
>
> The sentence sounds wrong without it.

It doesn't sound *wrong* to me without it, but it sounds better to me
with it.  (Or maybe this is a US / UK thing -- I have a hard time
telling anymore.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel