[1/2] x86emul: simplify DstBitBase handling code

Commit Message

Jan Beulich Nov. 22, 2016, 2:20 p.m. UTC

..., at once making it more obvious that even in the negative bit
offset case the resulting bit offset to be used by the inlined
instructions will always be constrained to the operand size of the
original instruction.

Also add a test case which would have failed without the XSA-195 fix.

Signed-off-by: Jan Beulich <[e-mail]>
x86emul: simplify DstBitBase handling code

..., at once making it more obvious that even in the negative bit
offset case the resulting bit offset to be used by the inlined
instructions will always be constrained to the operand size of the
original instruction.

Also add a test case which would have failed without the XSA-195 fix.

Signed-off-by: Jan Beulich <[e-mail]>

--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -431,6 +431,22 @@ int main(int argc, char **argv)
         goto fail;
     printf("okay\n");
 
+#ifdef __x86_64__
+    printf("%-40s", "Testing btcq %r8,(%r11)...");
+    instr[0] = 0x4d; instr[1] = 0x0f; instr[2] = 0xbb; instr[3] = 0x03;
+    regs.eflags = 0x200;
+    regs.rip    = (unsigned long)&instr[0];
+    regs.r8     = (-1L << 40) + 1;
+    regs.r11    = (unsigned long)(res + (1L << 35));
+    rc = x86_emulate(&ctxt, &emulops);
+    if ( (rc != X86EMUL_OKAY) ||
+         (*res != 0x2233445C) ||
+         (regs.eflags != 0x201) ||
+         (regs.rip != (unsigned long)&instr[4]) )
+        goto fail;
+    printf("okay\n");
+#endif
+
     res[0] = 0x12345678;
     res[1] = 0x87654321;
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2560,18 +2560,11 @@ x86_emulate(
             else if ( op_bytes == 4 )
                 src.val = (int32_t)src.val;
             if ( (long)src.val < 0 )
-            {
-                unsigned long byte_offset =
+                ea.mem.off -=
                     op_bytes + (((-src.val - 1) >> 3) & ~(op_bytes - 1L));
-
-                ea.mem.off -= byte_offset;
-                src.val = (byte_offset << 3) + src.val;
-            }
             else
-            {
                 ea.mem.off += (src.val >> 3) & ~(op_bytes - 1L);
-                src.val &= (op_bytes << 3) - 1;
-            }
+            src.val &= (op_bytes << 3) - 1;
         }
         /* Becomes a normal DstMem operation from here on. */
         d = (d & ~DstMask) | DstMem;

Comments

Andrew Cooper Nov. 22, 2016, 2:23 p.m. UTC | #1

On 22/11/16 14:20, Jan Beulich wrote:
> ..., at once making it more obvious that even in the negative bit
> offset case the resulting bit offset to be used by the inlined
> instructions will always be constrained to the operand size of the
> original instruction.
>
> Also add a test case which would have failed without the XSA-195 fix.
>
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Andrew Cooper <[e-mail]>

Patch

--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -431,6 +431,22 @@  int main(int argc, char **argv)
         goto fail;
     printf("okay\n");
 
+#ifdef __x86_64__
+    printf("%-40s", "Testing btcq %r8,(%r11)...");
+    instr[0] = 0x4d; instr[1] = 0x0f; instr[2] = 0xbb; instr[3] = 0x03;
+    regs.eflags = 0x200;
+    regs.rip    = (unsigned long)&instr[0];
+    regs.r8     = (-1L << 40) + 1;
+    regs.r11    = (unsigned long)(res + (1L << 35));
+    rc = x86_emulate(&ctxt, &emulops);
+    if ( (rc != X86EMUL_OKAY) ||
+         (*res != 0x2233445C) ||
+         (regs.eflags != 0x201) ||
+         (regs.rip != (unsigned long)&instr[4]) )
+        goto fail;
+    printf("okay\n");
+#endif
+
     res[0] = 0x12345678;
     res[1] = 0x87654321;
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2560,18 +2560,11 @@  x86_emulate(
             else if ( op_bytes == 4 )
                 src.val = (int32_t)src.val;
             if ( (long)src.val < 0 )
-            {
-                unsigned long byte_offset =
+                ea.mem.off -=
                     op_bytes + (((-src.val - 1) >> 3) & ~(op_bytes - 1L));
-
-                ea.mem.off -= byte_offset;
-                src.val = (byte_offset << 3) + src.val;
-            }
             else
-            {
                 ea.mem.off += (src.val >> 3) & ~(op_bytes - 1L);
-                src.val &= (op_bytes << 3) - 1;
-            }
+            src.val &= (op_bytes << 3) - 1;
         }
         /* Becomes a normal DstMem operation from here on. */
         d = (d & ~DstMask) | DstMem;