x86_emulate: fix flag setting for 8-bit signed multiplication

We really need to check for a signed overflow of 8 bits, while the
previous check compared the sign-extended 8-bit result with the
zero-extended 16-bit one (which was wrong for all negative results).

Once at it
- also adjust the 16-bit comparison for symmetry
- improve the 8-bit multiplication (no need to zero-extend to 32-bits
  the sign-extended to 16 bits original 8-bit value)
- fold both signed multiplication variants

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Keir Fraser <keir@xen.org>

diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
index f18f61520940146f5162c67cf13fe88a3ca3cb94..b157adef51dbfd8f91a133d30e5e7c9d836b3c00 100644 (file)
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -1,3 +1,4 @@
+#include <assert.h>
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
@@ -8,6 +9,7 @@
 typedef bool bool_t;
 
 #define BUG() abort()
+#define ASSERT assert
 
 #define cpu_has_amd_erratum(nr) 0
 #define mark_regs_dirty(r) ((void)(r))

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index b4a24c8cb1b898d635b6159af7bc70a0ca93d4bb..d3023de0eb19832685dd23c2ffbfa3e1d5cad30a 100644 (file)
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2096,40 +2096,13 @@ x86_emulate(
         goto push;
 
     case 0x69: /* imul imm16/32 */
-    case 0x6b: /* imul imm8 */ {
-        unsigned long src1; /* ModR/M source operand */
+    case 0x6b: /* imul imm8 */
         if ( ea.type == OP_REG )
-            src1 = *ea.reg;
+            dst.val = *ea.reg;
         else if ( (rc = read_ulong(ea.mem.seg, ea.mem.off,
-                                   &src1, op_bytes, ctxt, ops)) )
+                                   &dst.val, op_bytes, ctxt, ops)) )
             goto done;
-        _regs.eflags &= ~(EFLG_OF|EFLG_CF);
-        switch ( dst.bytes )
-        {
-        case 2:
-            dst.val = ((uint32_t)(int16_t)src.val *
-                       (uint32_t)(int16_t)src1);
-            if ( (int16_t)dst.val != (uint32_t)dst.val )
-                _regs.eflags |= EFLG_OF|EFLG_CF;
-            break;
-#ifdef __x86_64__
-        case 4:
-            dst.val = ((uint64_t)(int32_t)src.val *
-                       (uint64_t)(int32_t)src1);
-            if ( (int32_t)dst.val != dst.val )
-                _regs.eflags |= EFLG_OF|EFLG_CF;
-            break;
-#endif
-        default: {
-            unsigned long m[2] = { src.val, src1 };
-            if ( imul_dbl(m) )
-                _regs.eflags |= EFLG_OF|EFLG_CF;
-            dst.val = m[0];
-            break;
-        }
-        }
-        break;
-    }
+        goto imul;
 
     case 0x6a: /* push imm8 */
         src.val = insn_fetch_type(int8_t);
@@ -3404,22 +3377,25 @@ x86_emulate(
             dst.type = OP_REG;
             dst.reg  = (unsigned long *)&_regs.eax;
             dst.val  = *dst.reg;
+            dst.bytes = src.bytes;
+        imul:
             _regs.eflags &= ~(EFLG_OF|EFLG_CF);
-            switch ( dst.bytes = src.bytes )
+            switch ( dst.bytes )
             {
             case 1:
-                dst.val = ((uint16_t)(int8_t)src.val *
-                           (uint16_t)(int8_t)dst.val);
-                if ( (int8_t)dst.val != (uint16_t)dst.val )
+                dst.val = (int8_t)src.val * (int8_t)dst.val;
+                if ( (int8_t)dst.val != (int16_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
+                ASSERT(b > 0x6b);
                 dst.bytes = 2;
                 break;
             case 2:
                 dst.val = ((uint32_t)(int16_t)src.val *
                            (uint32_t)(int16_t)dst.val);
-                if ( (int16_t)dst.val != (uint32_t)dst.val )
+                if ( (int16_t)dst.val != (int32_t)dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
-                *(uint16_t *)&_regs.edx = dst.val >> 16;
+                if ( b > 0x6b )
+                    *(uint16_t *)&_regs.edx = dst.val >> 16;
                 break;
 #ifdef __x86_64__
             case 4:
@@ -3427,14 +3403,16 @@ x86_emulate(
                            (uint64_t)(int32_t)dst.val);
                 if ( (int32_t)dst.val != dst.val )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
-                _regs.edx = (uint32_t)(dst.val >> 32);
+                if ( b > 0x6b )
+                    _regs.edx = (uint32_t)(dst.val >> 32);
                 break;
 #endif
             default: {
                 unsigned long m[2] = { src.val, dst.val };
                 if ( imul_dbl(m) )
                     _regs.eflags |= EFLG_OF|EFLG_CF;
-                _regs.edx = m[1];
+                if ( b > 0x6b )
+                    _regs.edx = m[1];
                 dst.val  = m[0];
                 break;
             }