[PATCH 0/7] x86emul: misc small adjustments

[PATCH 0/7] x86emul: misc small adjustments

[Jan Beulich]

This is mainly to reduce the amount of fetching of insn bytes past
the initial phase of decoding, as a little bit of preparation for the
intended splitting of decoding and execution stages. Having fully
read all insn bytes by that point would be specifically required if
the first stage alone should become usable to simply size an
instruction

There are, however, unrelated other improvements, with the
fundamental common attribute being the attempt to avoid
open coding in the handling of specific instructions what can be
done by generic code.

1: don't special case fetching the immediate of PUSH
2: don't special case fetching immediates of near and short branches
3: x86emul: all push flavors are data moves
4: fold SrcImmByte fetching
5: don't special case fetching unsigned 8-bit immediates
6: use DstEax where possible
7: introduce SrcImm16

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/7] x86emul: don't special case fetching the immediate of PUSH

[Jan Beulich]

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

These immediates follow the standard patterns in all modes, so they're
better fetched by the generic source operand handling code.

To facilitate testing, instead of adding yet another of these pretty
convoluted individual test cases, simply introduce another blowfish run
with -mno-accumulate-outgoing-args (the additional -Dstatic is to
keep the compiler from converting the calling convention to
"regparm(3)", which I did observe it does).

To make this introduction of a new blowfish pass (and potential further
ones later one) have less impact on the readability of the final code,
abstract all such "binary blob" executions via a table to iterate
through.

The resulting native code execution adjustment also uncovered a lack of
clobbers on the asm() in the 64-bit case, which is being fixed at once.

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

--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -11,21 +11,21 @@ all: $(TARGET)
 run: $(TARGET)
 	./$(TARGET)
 
-.PHONY: blowfish.h
-blowfish.h:
-	rm -f blowfish.bin
-	XEN_TARGET_ARCH=x86_32 make -f blowfish.mk all
-	(echo "static unsigned int blowfish32_code[] = {"; \
-	od -v -t x blowfish.bin | sed 's/^[0-9]* /0x/' | sed 's/ /, 0x/g' | sed 's/$$/,/';\
-	echo "};") >$@
-	rm -f blowfish.bin
-ifeq ($(XEN_COMPILE_ARCH),x86_64)
-	XEN_TARGET_ARCH=x86_64 make -f blowfish.mk all
-	(echo "static unsigned int blowfish64_code[] = {"; \
-	od -v -t x blowfish.bin | sed 's/^[0-9]* /0x/' | sed 's/ /, 0x/g' | sed 's/$$/,/';\
-	echo "};") >>$@
-	rm -f blowfish.bin
-endif
+cflags-x86_32 := "-mno-accumulate-outgoing-args -Dstatic="
+
+blowfish.h: blowfish.c blowfish.mk Makefile
+	rm -f $@.new blowfish.bin
+	$(foreach arch,$(filter-out $(XEN_COMPILE_ARCH),x86_32) $(XEN_COMPILE_ARCH), \
+	    for cflags in "" $(cflags-$(arch)); do \
+		$(MAKE) -f blowfish.mk XEN_TARGET_ARCH=$(arch) BLOWFISH_CFLAGS="$$cflags" all; \
+		flavor=$$(echo $${cflags} | sed -e 's, .*,,' -e 'y,-=,__,') ; \
+		(echo "static unsigned int blowfish_$(arch)$${flavor}[] = {"; \
+		 od -v -t x blowfish.bin | sed -e 's/^[0-9]* /0x/' -e 's/ /, 0x/g' -e 's/$$/,/'; \
+		 echo "};") >>$@.new; \
+		rm -f blowfish.bin; \
+	    done; \
+	)
+	mv $@.new $@
 
 $(TARGET): x86_emulate.o test_x86_emulator.o
 	$(HOSTCC) -o $@ $^
--- a/tools/tests/x86_emulator/blowfish.mk
+++ b/tools/tests/x86_emulator/blowfish.mk
@@ -5,7 +5,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
-CFLAGS += -fno-builtin -msoft-float
+CFLAGS += -fno-builtin -msoft-float $(BLOWFISH_CFLAGS)
 
 .PHONY: all
 all: blowfish.bin
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -1,4 +1,5 @@
 #include <errno.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -12,6 +13,21 @@
 #include "x86_emulate/x86_emulate.h"
 #include "blowfish.h"
 
+static const struct {
+    const void *code;
+    size_t size;
+    unsigned int bitness;
+    const char*name;
+} blobs[] = {
+    { blowfish_x86_32, sizeof(blowfish_x86_32), 32, "blowfish" },
+    { blowfish_x86_32_mno_accumulate_outgoing_args,
+      sizeof(blowfish_x86_32_mno_accumulate_outgoing_args),
+      32, "blowfish (push)" },
+#ifdef __x86_64__
+    { blowfish_x86_64, sizeof(blowfish_x86_64), 64, "blowfish" },
+#endif
+};
+
 #define MMAP_SZ 16384
 
 /* EFLAGS bit definitions. */
@@ -943,25 +959,19 @@ int main(int argc, char **argv)
     else
         printf("skipped\n");
 
-    for ( j = 1; j <= 2; j++ )
+    for ( j = 0; j < sizeof(blobs) / sizeof(*blobs); j++ )
     {
-#if defined(__i386__)
-        if ( j == 2 ) break;
-        memcpy(res, blowfish32_code, sizeof(blowfish32_code));
-#else
-        ctxt.addr_size = 16 << j;
-        ctxt.sp_size   = 16 << j;
-        memcpy(res, (j == 1) ? blowfish32_code : blowfish64_code,
-               (j == 1) ? sizeof(blowfish32_code) : sizeof(blowfish64_code));
-#endif
-        printf("Testing blowfish %u-bit code sequence", j*32);
+        memcpy(res, blobs[j].code, blobs[j].size);
+        ctxt.addr_size = ctxt.sp_size = blobs[j].bitness;
+
+        printf("Testing %s %u-bit code sequence",
+               blobs[j].name, ctxt.addr_size);
         regs.eax = 2;
         regs.edx = 1;
         regs.eip = (unsigned long)res;
         regs.esp = (unsigned long)res + MMAP_SZ - 4;
-        if ( j == 2 )
+        if ( ctxt.addr_size == 64 )
         {
-            ctxt.addr_size = ctxt.sp_size = 64;
             *(uint32_t *)(unsigned long)regs.esp = 0;
             regs.esp -= 4;
         }
@@ -983,20 +993,27 @@ int main(int argc, char **argv)
              (regs.eax != 2) || (regs.edx != 1) )
             goto fail;
         printf("okay\n");
-    }
 
-    printf("%-40s", "Testing blowfish native execution...");    
-    asm volatile (
+        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
+            continue;
+
+        i = printf("Testing %s native execution...", blobs[j].name);
+        asm volatile (
 #if defined(__i386__)
-        "movl $0x100000,%%ecx; call *%%ecx"
+            "movl $0x100000,%%ecx; call *%%ecx"
 #else
-        "movl $0x100000,%%ecx; call *%%rcx"
+            "movl $0x100000,%%ecx; call *%%rcx"
 #endif
-        : "=a" (regs.eax), "=d" (regs.edx)
-        : "0" (2), "1" (1) : "ecx" );
-    if ( (regs.eax != 2) || (regs.edx != 1) )
-        goto fail;
-    printf("okay\n");
+            : "=a" (regs.eax), "=d" (regs.edx)
+            : "0" (2), "1" (1) : "ecx"
+#ifdef __x86_64__
+              , "rsi", "rdi", "r8", "r9", "r10", "r11"
+#endif
+        );
+        if ( (regs.eax != 2) || (regs.edx != 1) )
+            goto fail;
+        printf("%*sokay\n", i < 40 ? 40 - i : 0, "");
+    }
 
     return 0;
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -94,8 +94,8 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, DstReg|SrcMem|ModRM, DstReg|SrcNone|ModRM|Mov,
     0, 0, 0, 0,
     /* 0x68 - 0x6F */
-    ImplicitOps|Mov, DstReg|SrcImm|ModRM|Mov,
-    ImplicitOps|Mov, DstReg|SrcImmByte|ModRM|Mov,
+    DstImplicit|SrcImm|Mov, DstReg|SrcImm|ModRM|Mov,
+    DstImplicit|SrcImmByte|Mov, DstReg|SrcImmByte|ModRM|Mov,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov,
     /* 0x70 - 0x77 */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
@@ -2335,10 +2335,15 @@ x86_emulate(
         break;
 
     case 0x68: /* push imm{16,32,64} */
-        src.val = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
-        goto push;
+    case 0x6a: /* push imm8 */
+    push:
+        d |= Mov; /* force writeback */
+        dst.type  = OP_MEM;
+        dst.bytes = mode_64bit() && (op_bytes == 4) ? 8 : op_bytes;
+        dst.val = src.val;
+        dst.mem.seg = x86_seg_ss;
+        dst.mem.off = sp_pre_dec(dst.bytes);
+        break;
 
     case 0x69: /* imul imm16/32 */
     case 0x6b: /* imul imm8 */
@@ -2349,19 +2354,6 @@ x86_emulate(
             goto done;
         goto imul;
 
-    case 0x6a: /* push imm8 */
-        src.val = insn_fetch_type(int8_t);
-    push:
-        d |= Mov; /* force writeback */
-        dst.type  = OP_MEM;
-        dst.bytes = op_bytes;
-        if ( mode_64bit() && (dst.bytes == 4) )
-            dst.bytes = 8;
-        dst.val = src.val;
-        dst.mem.seg = x86_seg_ss;
-        dst.mem.off = sp_pre_dec(dst.bytes);
-        break;
-
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
         unsigned long nr_reps = get_rep_prefix();
         unsigned int port = (uint16_t)_regs.edx;



[-- Attachment #2: x86emul-push-imm-generic.patch --]
[-- Type: text/plain, Size: 7920 bytes --]

x86emul: don't special case fetching the immediate of PUSH

These immediates follow the standard patterns in all modes, so they're
better fetched by the generic source operand handling code.

To facilitate testing, instead of adding yet another of these pretty
convoluted individual test cases, simply introduce another blowfish run
with -mno-accumulate-outgoing-args (the additional -Dstatic is to
keep the compiler from converting the calling convention to
"regparm(3)", which I did observe it does).

To make this introduction of a new blowfish pass (and potential further
ones later one) have less impact on the readability of the final code,
abstract all such "binary blob" executions via a table to iterate
through.

The resulting native code execution adjustment also uncovered a lack of
clobbers on the asm() in the 64-bit case, which is being fixed at once.

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

--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -11,21 +11,21 @@ all: $(TARGET)
 run: $(TARGET)
 	./$(TARGET)
 
-.PHONY: blowfish.h
-blowfish.h:
-	rm -f blowfish.bin
-	XEN_TARGET_ARCH=x86_32 make -f blowfish.mk all
-	(echo "static unsigned int blowfish32_code[] = {"; \
-	od -v -t x blowfish.bin | sed 's/^[0-9]* /0x/' | sed 's/ /, 0x/g' | sed 's/$$/,/';\
-	echo "};") >$@
-	rm -f blowfish.bin
-ifeq ($(XEN_COMPILE_ARCH),x86_64)
-	XEN_TARGET_ARCH=x86_64 make -f blowfish.mk all
-	(echo "static unsigned int blowfish64_code[] = {"; \
-	od -v -t x blowfish.bin | sed 's/^[0-9]* /0x/' | sed 's/ /, 0x/g' | sed 's/$$/,/';\
-	echo "};") >>$@
-	rm -f blowfish.bin
-endif
+cflags-x86_32 := "-mno-accumulate-outgoing-args -Dstatic="
+
+blowfish.h: blowfish.c blowfish.mk Makefile
+	rm -f $@.new blowfish.bin
+	$(foreach arch,$(filter-out $(XEN_COMPILE_ARCH),x86_32) $(XEN_COMPILE_ARCH), \
+	    for cflags in "" $(cflags-$(arch)); do \
+		$(MAKE) -f blowfish.mk XEN_TARGET_ARCH=$(arch) BLOWFISH_CFLAGS="$$cflags" all; \
+		flavor=$$(echo $${cflags} | sed -e 's, .*,,' -e 'y,-=,__,') ; \
+		(echo "static unsigned int blowfish_$(arch)$${flavor}[] = {"; \
+		 od -v -t x blowfish.bin | sed -e 's/^[0-9]* /0x/' -e 's/ /, 0x/g' -e 's/$$/,/'; \
+		 echo "};") >>$@.new; \
+		rm -f blowfish.bin; \
+	    done; \
+	)
+	mv $@.new $@
 
 $(TARGET): x86_emulate.o test_x86_emulator.o
 	$(HOSTCC) -o $@ $^
--- a/tools/tests/x86_emulator/blowfish.mk
+++ b/tools/tests/x86_emulator/blowfish.mk
@@ -5,7 +5,7 @@ include $(XEN_ROOT)/tools/Rules.mk
 
 $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
 
-CFLAGS += -fno-builtin -msoft-float
+CFLAGS += -fno-builtin -msoft-float $(BLOWFISH_CFLAGS)
 
 .PHONY: all
 all: blowfish.bin
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -1,4 +1,5 @@
 #include <errno.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -12,6 +13,21 @@
 #include "x86_emulate/x86_emulate.h"
 #include "blowfish.h"
 
+static const struct {
+    const void *code;
+    size_t size;
+    unsigned int bitness;
+    const char*name;
+} blobs[] = {
+    { blowfish_x86_32, sizeof(blowfish_x86_32), 32, "blowfish" },
+    { blowfish_x86_32_mno_accumulate_outgoing_args,
+      sizeof(blowfish_x86_32_mno_accumulate_outgoing_args),
+      32, "blowfish (push)" },
+#ifdef __x86_64__
+    { blowfish_x86_64, sizeof(blowfish_x86_64), 64, "blowfish" },
+#endif
+};
+
 #define MMAP_SZ 16384
 
 /* EFLAGS bit definitions. */
@@ -943,25 +959,19 @@ int main(int argc, char **argv)
     else
         printf("skipped\n");
 
-    for ( j = 1; j <= 2; j++ )
+    for ( j = 0; j < sizeof(blobs) / sizeof(*blobs); j++ )
     {
-#if defined(__i386__)
-        if ( j == 2 ) break;
-        memcpy(res, blowfish32_code, sizeof(blowfish32_code));
-#else
-        ctxt.addr_size = 16 << j;
-        ctxt.sp_size   = 16 << j;
-        memcpy(res, (j == 1) ? blowfish32_code : blowfish64_code,
-               (j == 1) ? sizeof(blowfish32_code) : sizeof(blowfish64_code));
-#endif
-        printf("Testing blowfish %u-bit code sequence", j*32);
+        memcpy(res, blobs[j].code, blobs[j].size);
+        ctxt.addr_size = ctxt.sp_size = blobs[j].bitness;
+
+        printf("Testing %s %u-bit code sequence",
+               blobs[j].name, ctxt.addr_size);
         regs.eax = 2;
         regs.edx = 1;
         regs.eip = (unsigned long)res;
         regs.esp = (unsigned long)res + MMAP_SZ - 4;
-        if ( j == 2 )
+        if ( ctxt.addr_size == 64 )
         {
-            ctxt.addr_size = ctxt.sp_size = 64;
             *(uint32_t *)(unsigned long)regs.esp = 0;
             regs.esp -= 4;
         }
@@ -983,20 +993,27 @@ int main(int argc, char **argv)
              (regs.eax != 2) || (regs.edx != 1) )
             goto fail;
         printf("okay\n");
-    }
 
-    printf("%-40s", "Testing blowfish native execution...");    
-    asm volatile (
+        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
+            continue;
+
+        i = printf("Testing %s native execution...", blobs[j].name);
+        asm volatile (
 #if defined(__i386__)
-        "movl $0x100000,%%ecx; call *%%ecx"
+            "movl $0x100000,%%ecx; call *%%ecx"
 #else
-        "movl $0x100000,%%ecx; call *%%rcx"
+            "movl $0x100000,%%ecx; call *%%rcx"
 #endif
-        : "=a" (regs.eax), "=d" (regs.edx)
-        : "0" (2), "1" (1) : "ecx" );
-    if ( (regs.eax != 2) || (regs.edx != 1) )
-        goto fail;
-    printf("okay\n");
+            : "=a" (regs.eax), "=d" (regs.edx)
+            : "0" (2), "1" (1) : "ecx"
+#ifdef __x86_64__
+              , "rsi", "rdi", "r8", "r9", "r10", "r11"
+#endif
+        );
+        if ( (regs.eax != 2) || (regs.edx != 1) )
+            goto fail;
+        printf("%*sokay\n", i < 40 ? 40 - i : 0, "");
+    }
 
     return 0;
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -94,8 +94,8 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, DstReg|SrcMem|ModRM, DstReg|SrcNone|ModRM|Mov,
     0, 0, 0, 0,
     /* 0x68 - 0x6F */
-    ImplicitOps|Mov, DstReg|SrcImm|ModRM|Mov,
-    ImplicitOps|Mov, DstReg|SrcImmByte|ModRM|Mov,
+    DstImplicit|SrcImm|Mov, DstReg|SrcImm|ModRM|Mov,
+    DstImplicit|SrcImmByte|Mov, DstReg|SrcImmByte|ModRM|Mov,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov,
     /* 0x70 - 0x77 */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
@@ -2335,10 +2335,15 @@ x86_emulate(
         break;
 
     case 0x68: /* push imm{16,32,64} */
-        src.val = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
-        goto push;
+    case 0x6a: /* push imm8 */
+    push:
+        d |= Mov; /* force writeback */
+        dst.type  = OP_MEM;
+        dst.bytes = mode_64bit() && (op_bytes == 4) ? 8 : op_bytes;
+        dst.val = src.val;
+        dst.mem.seg = x86_seg_ss;
+        dst.mem.off = sp_pre_dec(dst.bytes);
+        break;
 
     case 0x69: /* imul imm16/32 */
     case 0x6b: /* imul imm8 */
@@ -2349,19 +2354,6 @@ x86_emulate(
             goto done;
         goto imul;
 
-    case 0x6a: /* push imm8 */
-        src.val = insn_fetch_type(int8_t);
-    push:
-        d |= Mov; /* force writeback */
-        dst.type  = OP_MEM;
-        dst.bytes = op_bytes;
-        if ( mode_64bit() && (dst.bytes == 4) )
-            dst.bytes = 8;
-        dst.val = src.val;
-        dst.mem.seg = x86_seg_ss;
-        dst.mem.off = sp_pre_dec(dst.bytes);
-        break;
-
     case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
         unsigned long nr_reps = get_rep_prefix();
         unsigned int port = (uint16_t)_regs.edx;

[-- 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/7] x86emul: don't special case fetching the immediate of PUSH

[Andrew Cooper]

On 11/08/16 13:03, Jan Beulich wrote:
> These immediates follow the standard patterns in all modes, so they're
> better fetched by the generic source operand handling code.
>
> To facilitate testing, instead of adding yet another of these pretty
> convoluted individual test cases, simply introduce another blowfish run
> with -mno-accumulate-outgoing-args (the additional -Dstatic is to
> keep the compiler from converting the calling convention to
> "regparm(3)", which I did observe it does).
>
> To make this introduction of a new blowfish pass (and potential further
> ones later one) have less impact on the readability of the final code,
> abstract all such "binary blob" executions via a table to iterate
> through.
>
> The resulting native code execution adjustment also uncovered a lack of
> clobbers on the asm() in the 64-bit case, which is being fixed at once.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

It would be helpful if the blob refactoring was in a separate patch to
the emulator bugfix.

> @@ -983,20 +993,27 @@ int main(int argc, char **argv)
>               (regs.eax != 2) || (regs.edx != 1) )
>              goto fail;
>          printf("okay\n");
> -    }
>  
> -    printf("%-40s", "Testing blowfish native execution...");    
> -    asm volatile (
> +        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
> +            continue;

This wants to be at the top of the loop, or we will attempt to emulate
64bit code in a 32bit build of the test before hitting this condition.

Otherwise, LGTM.

~Andrew

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

Re: [PATCH 1/7] x86emul: don't special case fetching the immediate of PUSH

[Jan Beulich]

>>> On 11.08.16 at 14:58, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:03, Jan Beulich wrote:
>> These immediates follow the standard patterns in all modes, so they're
>> better fetched by the generic source operand handling code.
>>
>> To facilitate testing, instead of adding yet another of these pretty
>> convoluted individual test cases, simply introduce another blowfish run
>> with -mno-accumulate-outgoing-args (the additional -Dstatic is to
>> keep the compiler from converting the calling convention to
>> "regparm(3)", which I did observe it does).
>>
>> To make this introduction of a new blowfish pass (and potential further
>> ones later one) have less impact on the readability of the final code,
>> abstract all such "binary blob" executions via a table to iterate
>> through.
>>
>> The resulting native code execution adjustment also uncovered a lack of
>> clobbers on the asm() in the 64-bit case, which is being fixed at once.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> It would be helpful if the blob refactoring was in a separate patch to
> the emulator bugfix.

There's no bug fix here really, which is why I didn't think splitting
it was strictly necessary. Can be done, but if at all possible I'd
prefer not to spend extra time on disentangling the two, the more
that in this case I'm convinced the reviewing of the split changes
wouldn't be any easier than that of them being lumped together.

>> @@ -983,20 +993,27 @@ int main(int argc, char **argv)
>>               (regs.eax != 2) || (regs.edx != 1) )
>>              goto fail;
>>          printf("okay\n");
>> -    }
>>  
>> -    printf("%-40s", "Testing blowfish native execution...");    
>> -    asm volatile (
>> +        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
>> +            continue;
> 
> This wants to be at the top of the loop, or we will attempt to emulate
> 64bit code in a 32bit build of the test before hitting this condition.

Certainly not: We do want to test 32-bit code in a 64-bit build,
and 64-bit code simply mustn't be included in a 32-bit build (hence
the __x86_64__ conditionals inside the blobs[] initializer). Quite
the opposite - not running 32-bit code natively here on a 64-bit
system is solely because that would require extra setup (but not
much value), not because it's impossible.

And btw., I did run the changed test on both 64-bit and 32-bit
setups, so it is proven in practice that what you think needs to
be prevented can't happen.

Jan


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

Re: [PATCH 1/7] x86emul: don't special case fetching the immediate of PUSH

[Andrew Cooper]

On 11/08/16 14:26, Jan Beulich wrote:
>>>> On 11.08.16 at 14:58, <andrew.cooper3@citrix.com> wrote:
>> On 11/08/16 13:03, Jan Beulich wrote:
>>> These immediates follow the standard patterns in all modes, so they're
>>> better fetched by the generic source operand handling code.
>>>
>>> To facilitate testing, instead of adding yet another of these pretty
>>> convoluted individual test cases, simply introduce another blowfish run
>>> with -mno-accumulate-outgoing-args (the additional -Dstatic is to
>>> keep the compiler from converting the calling convention to
>>> "regparm(3)", which I did observe it does).
>>>
>>> To make this introduction of a new blowfish pass (and potential further
>>> ones later one) have less impact on the readability of the final code,
>>> abstract all such "binary blob" executions via a table to iterate
>>> through.
>>>
>>> The resulting native code execution adjustment also uncovered a lack of
>>> clobbers on the asm() in the 64-bit case, which is being fixed at once.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> It would be helpful if the blob refactoring was in a separate patch to
>> the emulator bugfix.
> There's no bug fix here really, which is why I didn't think splitting
> it was strictly necessary. Can be done, but if at all possible I'd
> prefer not to spend extra time on disentangling the two, the more
> that in this case I'm convinced the reviewing of the split changes
> wouldn't be any easier than that of them being lumped together.
>
>>> @@ -983,20 +993,27 @@ int main(int argc, char **argv)
>>>               (regs.eax != 2) || (regs.edx != 1) )
>>>              goto fail;
>>>          printf("okay\n");
>>> -    }
>>>  
>>> -    printf("%-40s", "Testing blowfish native execution...");    
>>> -    asm volatile (
>>> +        if ( ctxt.addr_size != sizeof(void *) * CHAR_BIT )
>>> +            continue;
>> This wants to be at the top of the loop, or we will attempt to emulate
>> 64bit code in a 32bit build of the test before hitting this condition.
> Certainly not: We do want to test 32-bit code in a 64-bit build,
> and 64-bit code simply mustn't be included in a 32-bit build (hence
> the __x86_64__ conditionals inside the blobs[] initializer). Quite
> the opposite - not running 32-bit code natively here on a 64-bit
> system is solely because that would require extra setup (but not
> much value), not because it's impossible.

Ah yes.

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

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

[PATCH 2/7] x86emul: don't special case fetching immediates of near and short branches

[Jan Beulich]

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

These immediates follow the standard patterns in all modes, so they're
better fetched by the generic source operand handling code.

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
@@ -98,11 +98,15 @@ static uint8_t opcode_table[256] = {
     DstImplicit|SrcImmByte|Mov, DstReg|SrcImmByte|ModRM|Mov,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov,
     /* 0x70 - 0x77 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0x78 - 0x7F */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0x80 - 0x87 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImm|ModRM,
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
@@ -155,10 +159,12 @@ static uint8_t opcode_table[256] = {
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     /* 0xE0 - 0xE7 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0xE8 - 0xEF */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
+    ImplicitOps, DstImplicit|SrcImmByte,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
@@ -206,11 +212,15 @@ static uint8_t twobyte_table[256] = {
     /* 0x70 - 0x7F */
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM,
     /* 0x80 - 0x87 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
     /* 0x88 - 0x8F */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
     /* 0x90 - 0x97 */
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
@@ -2415,12 +2425,10 @@ x86_emulate(
         break;
     }
 
-    case 0x70 ... 0x7f: /* jcc (short) */ {
-        int rel = insn_fetch_type(int8_t);
+    case 0x70 ... 0x7f: /* jcc (short) */
         if ( test_cc(b, _regs.eflags) )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0x82: /* Grp1 (x86/32 only) */
         generate_exception_if(mode_64bit(), EXC_UD, -1);
@@ -3461,8 +3469,8 @@ x86_emulate(
         break;
 
     case 0xe0 ... 0xe2: /* loop{,z,nz} */ {
-        int rel = insn_fetch_type(int8_t);
         int do_jmp = !(_regs.eflags & EFLG_ZF); /* loopnz */
+
         if ( b == 0xe1 )
             do_jmp = !do_jmp; /* loopz */
         else if ( b == 0xe2 )
@@ -3481,17 +3489,15 @@ x86_emulate(
             break;
         }
         if ( do_jmp )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
     }
 
-    case 0xe3: /* jcxz/jecxz (short) */ {
-        int rel = insn_fetch_type(int8_t);
+    case 0xe3: /* jcxz/jecxz (short) */
         if ( (ad_bytes == 2) ? !(uint16_t)_regs.ecx :
              (ad_bytes == 4) ? !(uint32_t)_regs.ecx : !_regs.ecx )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0xe4: /* in imm8,%al */
     case 0xe5: /* in imm8,%eax */
@@ -3528,22 +3534,18 @@ x86_emulate(
     }
 
     case 0xe8: /* call (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
+        int32_t rel = src.val;
+
         op_bytes = ((op_bytes == 4) && mode_64bit()) ? 8 : op_bytes;
         src.val = _regs.eip;
         jmp_rel(rel);
         goto push;
     }
 
-    case 0xe9: /* jmp (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
-        jmp_rel(rel);
+    case 0xe9: /* jmp (near) */
+    case 0xeb: /* jmp (short) */
+        jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0xea: /* jmp (far, absolute) */ {
         uint16_t sel;
@@ -3557,12 +3559,6 @@ x86_emulate(
         break;
     }
 
-    case 0xeb: /* jmp (short) */ {
-        int rel = insn_fetch_type(int8_t);
-        jmp_rel(rel);
-        break;
-    }
-
     case 0xf1: /* int1 (icebp) */
         src.val = EXC_DB;
         swint_type = x86_swint_icebp;
@@ -4503,14 +4499,10 @@ x86_emulate(
         break;
     }
 
-    case 0x80 ... 0x8f: /* jcc (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
+    case 0x80 ... 0x8f: /* jcc (near) */
         if ( test_cc(b, _regs.eflags) )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0x90 ... 0x9f: /* setcc */
         dst.val = test_cc(b, _regs.eflags);



[-- Attachment #2: x86emul-branch-generic.patch --]
[-- Type: text/plain, Size: 6096 bytes --]

x86emul: don't special case fetching immediates of near and short branches

These immediates follow the standard patterns in all modes, so they're
better fetched by the generic source operand handling code.

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
@@ -98,11 +98,15 @@ static uint8_t opcode_table[256] = {
     DstImplicit|SrcImmByte|Mov, DstReg|SrcImmByte|ModRM|Mov,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps|Mov,
     /* 0x70 - 0x77 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0x78 - 0x7F */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0x80 - 0x87 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImm|ModRM,
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
@@ -155,10 +159,12 @@ static uint8_t opcode_table[256] = {
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     /* 0xE0 - 0xE7 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0xE8 - 0xEF */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
+    ImplicitOps, DstImplicit|SrcImmByte,
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
@@ -206,11 +212,15 @@ static uint8_t twobyte_table[256] = {
     /* 0x70 - 0x7F */
     0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ImplicitOps|ModRM,
     /* 0x80 - 0x87 */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
     /* 0x88 - 0x8F */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
+    DstImplicit|SrcImm, DstImplicit|SrcImm,
     /* 0x90 - 0x97 */
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
@@ -2415,12 +2425,10 @@ x86_emulate(
         break;
     }
 
-    case 0x70 ... 0x7f: /* jcc (short) */ {
-        int rel = insn_fetch_type(int8_t);
+    case 0x70 ... 0x7f: /* jcc (short) */
         if ( test_cc(b, _regs.eflags) )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0x82: /* Grp1 (x86/32 only) */
         generate_exception_if(mode_64bit(), EXC_UD, -1);
@@ -3461,8 +3469,8 @@ x86_emulate(
         break;
 
     case 0xe0 ... 0xe2: /* loop{,z,nz} */ {
-        int rel = insn_fetch_type(int8_t);
         int do_jmp = !(_regs.eflags & EFLG_ZF); /* loopnz */
+
         if ( b == 0xe1 )
             do_jmp = !do_jmp; /* loopz */
         else if ( b == 0xe2 )
@@ -3481,17 +3489,15 @@ x86_emulate(
             break;
         }
         if ( do_jmp )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
     }
 
-    case 0xe3: /* jcxz/jecxz (short) */ {
-        int rel = insn_fetch_type(int8_t);
+    case 0xe3: /* jcxz/jecxz (short) */
         if ( (ad_bytes == 2) ? !(uint16_t)_regs.ecx :
              (ad_bytes == 4) ? !(uint32_t)_regs.ecx : !_regs.ecx )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0xe4: /* in imm8,%al */
     case 0xe5: /* in imm8,%eax */
@@ -3528,22 +3534,18 @@ x86_emulate(
     }
 
     case 0xe8: /* call (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
+        int32_t rel = src.val;
+
         op_bytes = ((op_bytes == 4) && mode_64bit()) ? 8 : op_bytes;
         src.val = _regs.eip;
         jmp_rel(rel);
         goto push;
     }
 
-    case 0xe9: /* jmp (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
-        jmp_rel(rel);
+    case 0xe9: /* jmp (near) */
+    case 0xeb: /* jmp (short) */
+        jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0xea: /* jmp (far, absolute) */ {
         uint16_t sel;
@@ -3557,12 +3559,6 @@ x86_emulate(
         break;
     }
 
-    case 0xeb: /* jmp (short) */ {
-        int rel = insn_fetch_type(int8_t);
-        jmp_rel(rel);
-        break;
-    }
-
     case 0xf1: /* int1 (icebp) */
         src.val = EXC_DB;
         swint_type = x86_swint_icebp;
@@ -4503,14 +4499,10 @@ x86_emulate(
         break;
     }
 
-    case 0x80 ... 0x8f: /* jcc (near) */ {
-        int rel = ((op_bytes == 2)
-                   ? (int32_t)insn_fetch_type(int16_t)
-                   : insn_fetch_type(int32_t));
+    case 0x80 ... 0x8f: /* jcc (near) */
         if ( test_cc(b, _regs.eflags) )
-            jmp_rel(rel);
+            jmp_rel((int32_t)src.val);
         break;
-    }
 
     case 0x90 ... 0x9f: /* setcc */
         dst.val = test_cc(b, _regs.eflags);

[-- 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 2/7] x86emul: don't special case fetching immediates of near and short branches

[Andrew Cooper]

On 11/08/16 13:04, Jan Beulich wrote:
> @@ -2415,12 +2425,10 @@ x86_emulate(
>          break;
>      }
>  
> -    case 0x70 ... 0x7f: /* jcc (short) */ {
> -        int rel = insn_fetch_type(int8_t);
> +    case 0x70 ... 0x7f: /* jcc (short) */
>          if ( test_cc(b, _regs.eflags) )
> -            jmp_rel(rel);
> +            jmp_rel((int32_t)src.val);

jmp_rel() already contains an explicit (int) cast of its parameter.  Is
the (int32_t) actually needed?

~Andrew

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

Re: [PATCH 2/7] x86emul: don't special case fetching immediates of near and short branches

[Jan Beulich]

>>> On 11.08.16 at 15:19, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:04, Jan Beulich wrote:
>> @@ -2415,12 +2425,10 @@ x86_emulate(
>>          break;
>>      }
>>  
>> -    case 0x70 ... 0x7f: /* jcc (short) */ {
>> -        int rel = insn_fetch_type(int8_t);
>> +    case 0x70 ... 0x7f: /* jcc (short) */
>>          if ( test_cc(b, _regs.eflags) )
>> -            jmp_rel(rel);
>> +            jmp_rel((int32_t)src.val);
> 
> jmp_rel() already contains an explicit (int) cast of its parameter.  Is
> the (int32_t) actually needed?

Good point - I admit I didn't inspect jmp_rel() when putting this together.

Jan


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

[PATCH 3/7] x86emul: all push flavors are data moves

[Jan Beulich]

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

Make all paths leading to the "push" label have the Mov flag set, and
ASSERT() that to be the case. For the opcode FF group the adjustment is
benign for the paths not leading to "push", as they all set dst.type to
OP_NONE

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
@@ -122,7 +122,7 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
     /* 0xA0 - 0xA7 */
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
@@ -1903,7 +1903,7 @@ x86_emulate(
                 /* fall through */
             case 3: /* call (far, absolute indirect) */
             case 5: /* jmp (far, absolute indirect) */
-                d = DstNone|SrcMem|ModRM;
+                d = DstNone | SrcMem | ModRM | Mov;
                 break;
             }
             break;
@@ -2347,7 +2347,7 @@ x86_emulate(
     case 0x68: /* push imm{16,32,64} */
     case 0x6a: /* push imm8 */
     push:
-        d |= Mov; /* force writeback */
+        ASSERT(d & Mov); /* writeback needed */
         dst.type  = OP_MEM;
         dst.bytes = mode_64bit() && (op_bytes == 4) ? 8 : op_bytes;
         dst.val = src.val;




[-- Attachment #2: x86emul-push-Mov.patch --]
[-- Type: text/plain, Size: 1546 bytes --]

x86emul: all push flavors are data moves

Make all paths leading to the "push" label have the Mov flag set, and
ASSERT() that to be the case. For the opcode FF group the adjustment is
benign for the paths not leading to "push", as they all set dst.type to
OP_NONE

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
@@ -122,7 +122,7 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     /* 0x98 - 0x9F */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
     /* 0xA0 - 0xA7 */
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
@@ -1903,7 +1903,7 @@ x86_emulate(
                 /* fall through */
             case 3: /* call (far, absolute indirect) */
             case 5: /* jmp (far, absolute indirect) */
-                d = DstNone|SrcMem|ModRM;
+                d = DstNone | SrcMem | ModRM | Mov;
                 break;
             }
             break;
@@ -2347,7 +2347,7 @@ x86_emulate(
     case 0x68: /* push imm{16,32,64} */
     case 0x6a: /* push imm8 */
     push:
-        d |= Mov; /* force writeback */
+        ASSERT(d & Mov); /* writeback needed */
         dst.type  = OP_MEM;
         dst.bytes = mode_64bit() && (op_bytes == 4) ? 8 : op_bytes;
         dst.val = src.val;

[-- 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 3/7] x86emul: all push flavors are data moves

[Andrew Cooper]

On 11/08/16 13:04, Jan Beulich wrote:
> Make all paths leading to the "push" label have the Mov flag set, and
> ASSERT() that to be the case. For the opcode FF group the adjustment is
> benign for the paths not leading to "push", as they all set dst.type to
> OP_NONE
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

[PATCH 4/7] x86emul: fold SrcImmByte fetching

[Jan Beulich]

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

There's no need for having identical code spelled out twice.

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
@@ -1979,9 +1979,12 @@ x86_emulate(
             goto done;
         break;
     case SrcImm:
+        if ( !(d & ByteOp) )
+            src.bytes = op_bytes != 8 ? op_bytes : 4;
+        else
+    case SrcImmByte:
+            src.bytes = 1;
         src.type  = OP_IMM;
-        src.bytes = (d & ByteOp) ? 1 : op_bytes;
-        if ( src.bytes == 8 ) src.bytes = 4;
         /* NB. Immediates are sign-extended as necessary. */
         switch ( src.bytes )
         {
@@ -1990,11 +1993,6 @@ x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
-    case SrcImmByte:
-        src.type  = OP_IMM;
-        src.bytes = 1;
-        src.val   = insn_fetch_type(int8_t);
-        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */




[-- Attachment #2: x86emul-fold-SrcImmByte-fetching.patch --]
[-- Type: text/plain, Size: 1083 bytes --]

x86emul: fold SrcImmByte fetching

There's no need for having identical code spelled out twice.

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
@@ -1979,9 +1979,12 @@ x86_emulate(
             goto done;
         break;
     case SrcImm:
+        if ( !(d & ByteOp) )
+            src.bytes = op_bytes != 8 ? op_bytes : 4;
+        else
+    case SrcImmByte:
+            src.bytes = 1;
         src.type  = OP_IMM;
-        src.bytes = (d & ByteOp) ? 1 : op_bytes;
-        if ( src.bytes == 8 ) src.bytes = 4;
         /* NB. Immediates are sign-extended as necessary. */
         switch ( src.bytes )
         {
@@ -1990,11 +1993,6 @@ x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
-    case SrcImmByte:
-        src.type  = OP_IMM;
-        src.bytes = 1;
-        src.val   = insn_fetch_type(int8_t);
-        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */

[-- 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 4/7] x86emul: fold SrcImmByte fetching

[Andrew Cooper]

On 11/08/16 13:05, Jan Beulich wrote:
> There's no need for having identical code spelled out twice.
>
> 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
> @@ -1979,9 +1979,12 @@ x86_emulate(
>              goto done;
>          break;
>      case SrcImm:
> +        if ( !(d & ByteOp) )
> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
> +        else

{

> +    case SrcImmByte:
> +            src.bytes = 1;

}

Otherwise, 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 4/7] x86emul: fold SrcImmByte fetching

[Jan Beulich]

>>> On 11.08.16 at 15:41, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:05, Jan Beulich wrote:
>> There's no need for having identical code spelled out twice.
>>
>> 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
>> @@ -1979,9 +1979,12 @@ x86_emulate(
>>              goto done;
>>          break;
>>      case SrcImm:
>> +        if ( !(d & ByteOp) )
>> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
>> +        else
> 
> {
> 
>> +    case SrcImmByte:
>> +            src.bytes = 1;
> 
> }

I don't understand: This is a single statement, which we don't require
to be surrounded by braces.

Jan


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

Re: [PATCH 4/7] x86emul: fold SrcImmByte fetching

[Andrew Cooper]

On 11/08/16 15:09, Jan Beulich wrote:
>>>> On 11.08.16 at 15:41, <andrew.cooper3@citrix.com> wrote:
>> On 11/08/16 13:05, Jan Beulich wrote:
>>> There's no need for having identical code spelled out twice.
>>>
>>> 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
>>> @@ -1979,9 +1979,12 @@ x86_emulate(
>>>              goto done;
>>>          break;
>>>      case SrcImm:
>>> +        if ( !(d & ByteOp) )
>>> +            src.bytes = op_bytes != 8 ? op_bytes : 4;
>>> +        else
>> {
>>
>>> +    case SrcImmByte:
>>> +            src.bytes = 1;
>> }
> I don't understand: This is a single statement, which we don't require
> to be surrounded by braces.

Yes it is syntactically correct, but it is very deceptive as the case
statement obscures the nature of the else scope.

~Andrew

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

[PATCH 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Jan Beulich]

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

These can be made work using SrcImmByte, making sure the low 8 bits of
src.val get suitably zero extended upon consumption. SHLD and SHRD
require a little more adjustment: Their source operands get changed
away from SrcReg, handling the register access "manually" instead of
the insn byte fetching.

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
@@ -148,11 +148,11 @@ static uint8_t opcode_table[256] = {
     ByteOp|DstMem|SrcImm|ModRM|Mov, DstMem|SrcImm|ModRM|Mov,
     /* 0xC8 - 0xCF */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    ImplicitOps, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD0 - 0xD7 */
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD8 - 0xDF */
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
@@ -161,7 +161,8 @@ static uint8_t opcode_table[256] = {
     /* 0xE0 - 0xE7 */
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0xE8 - 0xEF */
     DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
     ImplicitOps, DstImplicit|SrcImmByte,
@@ -233,10 +234,10 @@ static uint8_t twobyte_table[256] = {
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
     /* 0xA0 - 0xA7 */
     ImplicitOps, ImplicitOps, ImplicitOps, DstBitBase|SrcReg|ModRM,
-    DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM, 0, 0,
+    DstMem|SrcImmByte|ModRM, DstMem|SrcReg|ModRM, 0, 0,
     /* 0xA8 - 0xAF */
     ImplicitOps, ImplicitOps, 0, DstBitBase|SrcReg|ModRM,
-    DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
+    DstMem|SrcImmByte|ModRM, DstMem|SrcReg|ModRM,
     ImplicitOps|ModRM, DstReg|SrcMem|ModRM,
     /* 0xB0 - 0xB7 */
     ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
@@ -2893,7 +2894,6 @@ x86_emulate(
         goto swint;
 
     case 0xcd: /* int imm8 */
-        src.val = insn_fetch_type(uint8_t);
         swint_type = x86_swint_int;
     swint:
         rc = inject_swint(swint_type, src.val,
@@ -2942,7 +2942,7 @@ x86_emulate(
 
     case 0xd4: /* aam */
     case 0xd5: /* aad */ {
-        unsigned int base = insn_fetch_type(uint8_t);
+        unsigned int base = (uint8_t)src.val;
 
         generate_exception_if(mode_64bit(), EXC_UD, -1);
         if ( b & 0x01 )
@@ -3505,9 +3505,9 @@ x86_emulate(
     case 0xed: /* in %dx,%eax */
     case 0xee: /* out %al,%dx */
     case 0xef: /* out %eax,%dx */ {
-        unsigned int port = ((b < 0xe8)
-                             ? insn_fetch_type(uint8_t)
-                             : (uint16_t)_regs.edx);
+        unsigned int port = ((b < 0xe8) ? (uint8_t)src.val
+                                        : (uint16_t)_regs.edx);
+
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         if ( (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
             goto done;
@@ -4562,7 +4562,15 @@ x86_emulate(
     case 0xac: /* shrd imm8,r,r/m */
     case 0xad: /* shrd %%cl,r,r/m */ {
         uint8_t shift, width = dst.bytes << 3;
-        shift = (b & 1) ? (uint8_t)_regs.ecx : insn_fetch_type(uint8_t);
+
+        if ( b & 1 )
+            shift = _regs.ecx;
+        else
+        {
+            shift = src.val;
+            src.reg = decode_register(modrm_reg, &_regs, 0);
+            src.val = truncate_word(*src.reg, dst.bytes);
+        }
         if ( (shift &= width - 1) == 0 )
             break;
         dst.orig_val = truncate_word(dst.val, dst.bytes);




[-- Attachment #2: x86emul-u8-imm-generic.patch --]
[-- Type: text/plain, Size: 4128 bytes --]

x86emul: don't special case fetching unsigned 8-bit immediates

These can be made work using SrcImmByte, making sure the low 8 bits of
src.val get suitably zero extended upon consumption. SHLD and SHRD
require a little more adjustment: Their source operands get changed
away from SrcReg, handling the register access "manually" instead of
the insn byte fetching.

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
@@ -148,11 +148,11 @@ static uint8_t opcode_table[256] = {
     ByteOp|DstMem|SrcImm|ModRM|Mov, DstMem|SrcImm|ModRM|Mov,
     /* 0xC8 - 0xCF */
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    ImplicitOps, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD0 - 0xD7 */
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD8 - 0xDF */
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
     ImplicitOps|ModRM|Mov, ImplicitOps|ModRM|Mov,
@@ -161,7 +161,8 @@ static uint8_t opcode_table[256] = {
     /* 0xE0 - 0xE7 */
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0xE8 - 0xEF */
     DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
     ImplicitOps, DstImplicit|SrcImmByte,
@@ -233,10 +234,10 @@ static uint8_t twobyte_table[256] = {
     ByteOp|DstMem|SrcNone|ModRM|Mov, ByteOp|DstMem|SrcNone|ModRM|Mov,
     /* 0xA0 - 0xA7 */
     ImplicitOps, ImplicitOps, ImplicitOps, DstBitBase|SrcReg|ModRM,
-    DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM, 0, 0,
+    DstMem|SrcImmByte|ModRM, DstMem|SrcReg|ModRM, 0, 0,
     /* 0xA8 - 0xAF */
     ImplicitOps, ImplicitOps, 0, DstBitBase|SrcReg|ModRM,
-    DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
+    DstMem|SrcImmByte|ModRM, DstMem|SrcReg|ModRM,
     ImplicitOps|ModRM, DstReg|SrcMem|ModRM,
     /* 0xB0 - 0xB7 */
     ByteOp|DstMem|SrcReg|ModRM, DstMem|SrcReg|ModRM,
@@ -2893,7 +2894,6 @@ x86_emulate(
         goto swint;
 
     case 0xcd: /* int imm8 */
-        src.val = insn_fetch_type(uint8_t);
         swint_type = x86_swint_int;
     swint:
         rc = inject_swint(swint_type, src.val,
@@ -2942,7 +2942,7 @@ x86_emulate(
 
     case 0xd4: /* aam */
     case 0xd5: /* aad */ {
-        unsigned int base = insn_fetch_type(uint8_t);
+        unsigned int base = (uint8_t)src.val;
 
         generate_exception_if(mode_64bit(), EXC_UD, -1);
         if ( b & 0x01 )
@@ -3505,9 +3505,9 @@ x86_emulate(
     case 0xed: /* in %dx,%eax */
     case 0xee: /* out %al,%dx */
     case 0xef: /* out %eax,%dx */ {
-        unsigned int port = ((b < 0xe8)
-                             ? insn_fetch_type(uint8_t)
-                             : (uint16_t)_regs.edx);
+        unsigned int port = ((b < 0xe8) ? (uint8_t)src.val
+                                        : (uint16_t)_regs.edx);
+
         op_bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
         if ( (rc = ioport_access_check(port, op_bytes, ctxt, ops)) != 0 )
             goto done;
@@ -4562,7 +4562,15 @@ x86_emulate(
     case 0xac: /* shrd imm8,r,r/m */
     case 0xad: /* shrd %%cl,r,r/m */ {
         uint8_t shift, width = dst.bytes << 3;
-        shift = (b & 1) ? (uint8_t)_regs.ecx : insn_fetch_type(uint8_t);
+
+        if ( b & 1 )
+            shift = _regs.ecx;
+        else
+        {
+            shift = src.val;
+            src.reg = decode_register(modrm_reg, &_regs, 0);
+            src.val = truncate_word(*src.reg, dst.bytes);
+        }
         if ( (shift &= width - 1) == 0 )
             break;
         dst.orig_val = truncate_word(dst.val, dst.bytes);

[-- 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 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Andrew Cooper]

On 11/08/16 13:06, Jan Beulich wrote:
> @@ -2893,7 +2894,6 @@ x86_emulate(
>          goto swint;
>  
>      case 0xcd: /* int imm8 */
> -        src.val = insn_fetch_type(uint8_t);
>          swint_type = x86_swint_int;
>      swint:
>          rc = inject_swint(swint_type, src.val,

I would be tempted to and an explicit (uint8_t) here, so that injection
doesn't break if the prototype of inject_swint() changes.

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

~Andrew

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

Re: [PATCH 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Jan Beulich]

>>> On 11.08.16 at 18:32, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 13:06, Jan Beulich wrote:
>> @@ -2893,7 +2894,6 @@ x86_emulate(
>>          goto swint;
>>  
>>      case 0xcd: /* int imm8 */
>> -        src.val = insn_fetch_type(uint8_t);
>>          swint_type = x86_swint_int;
>>      swint:
>>          rc = inject_swint(swint_type, src.val,
> 
> I would be tempted to and an explicit (uint8_t) here, so that injection
> doesn't break if the prototype of inject_swint() changes.

I guess I'll leave it that way, for two reasons:
- One shouldn't change prototypes without checking whether callers
  cope.
- Here you basically suggest the opposite of what you wish done to
  the earlier patch for the jmp_rel() invocations.

Jan


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

Re: [PATCH 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Andrew Cooper]

On 11/08/16 17:44, Jan Beulich wrote:
>>>> On 11.08.16 at 18:32, <andrew.cooper3@citrix.com> wrote:
>> On 11/08/16 13:06, Jan Beulich wrote:
>>> @@ -2893,7 +2894,6 @@ x86_emulate(
>>>          goto swint;
>>>  
>>>      case 0xcd: /* int imm8 */
>>> -        src.val = insn_fetch_type(uint8_t);
>>>          swint_type = x86_swint_int;
>>>      swint:
>>>          rc = inject_swint(swint_type, src.val,
>> I would be tempted to and an explicit (uint8_t) here, so that injection
>> doesn't break if the prototype of inject_swint() changes.
> I guess I'll leave it that way, for two reasons:
> - One shouldn't change prototypes without checking whether callers
>   cope.

Indeed, but that doesn't alter the fact that you, I, and others we have
reviewed code from have managed to do precisely this, and break things.

> - Here you basically suggest the opposite of what you wish done to
>   the earlier patch for the jmp_rel() invocations.

jmp_rel() is a macro not a function, but in hindsight, I rescind that
request.

You specifically state in the commit message that this change overloads
the use of SrcImmByte, and that care needs to be taken dealing with src.val.

~Andrew

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

Re: [PATCH 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Jan Beulich]

>>> On 11.08.16 at 19:38, <andrew.cooper3@citrix.com> wrote:
> On 11/08/16 17:44, Jan Beulich wrote:
>>>>> On 11.08.16 at 18:32, <andrew.cooper3@citrix.com> wrote:
>>> On 11/08/16 13:06, Jan Beulich wrote:
>>>> @@ -2893,7 +2894,6 @@ x86_emulate(
>>>>          goto swint;
>>>>  
>>>>      case 0xcd: /* int imm8 */
>>>> -        src.val = insn_fetch_type(uint8_t);
>>>>          swint_type = x86_swint_int;
>>>>      swint:
>>>>          rc = inject_swint(swint_type, src.val,
>>> I would be tempted to and an explicit (uint8_t) here, so that injection
>>> doesn't break if the prototype of inject_swint() changes.
>> I guess I'll leave it that way, for two reasons:
>> - One shouldn't change prototypes without checking whether callers
>>   cope.
> 
> Indeed, but that doesn't alter the fact that you, I, and others we have
> reviewed code from have managed to do precisely this, and break things.

Well, okay, will do that then.

>> - Here you basically suggest the opposite of what you wish done to
>>   the earlier patch for the jmp_rel() invocations.
> 
> jmp_rel() is a macro not a function, but in hindsight, I rescind that
> request.

As that was the only change request to that other patch, may I
translate that to an ack / R-b for it then?

Jan


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

Re: [PATCH 5/7] x86emul: don't special case fetching unsigned 8-bit immediates

[Andrew Cooper]

On 12/08/16 11:50, Jan Beulich wrote:
>>>> On 11.08.16 at 19:38, <andrew.cooper3@citrix.com> wrote:
>> On 11/08/16 17:44, Jan Beulich wrote:
>>>>>> On 11.08.16 at 18:32, <andrew.cooper3@citrix.com> wrote:
>>>> On 11/08/16 13:06, Jan Beulich wrote:
>>>>> @@ -2893,7 +2894,6 @@ x86_emulate(
>>>>>          goto swint;
>>>>>  
>>>>>      case 0xcd: /* int imm8 */
>>>>> -        src.val = insn_fetch_type(uint8_t);
>>>>>          swint_type = x86_swint_int;
>>>>>      swint:
>>>>>          rc = inject_swint(swint_type, src.val,
>>>> I would be tempted to and an explicit (uint8_t) here, so that injection
>>>> doesn't break if the prototype of inject_swint() changes.
>>> I guess I'll leave it that way, for two reasons:
>>> - One shouldn't change prototypes without checking whether callers
>>>   cope.
>> Indeed, but that doesn't alter the fact that you, I, and others we have
>> reviewed code from have managed to do precisely this, and break things.
> Well, okay, will do that then.
>
>>> - Here you basically suggest the opposite of what you wish done to
>>>   the earlier patch for the jmp_rel() invocations.
>> jmp_rel() is a macro not a function, but in hindsight, I rescind that
>> request.
> As that was the only change request to that other patch, may I
> translate that to an ack / R-b for it then?

Yes.  Sorry for the confusion.

~Andrew

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

[PATCH 6/7] x86emul: use DstEax where possible

[Jan Beulich]

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

While it avoids just a few instructions, we should nevertheless make
use of generic code as much as possible.

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
@@ -124,7 +124,7 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
     /* 0xA0 - 0xA7 */
-    ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
+    ByteOp|DstEax|SrcImplicit|Mov, DstEax|SrcImplicit|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps, ImplicitOps,
@@ -161,12 +161,12 @@ static uint8_t opcode_table[256] = {
     /* 0xE0 - 0xE7 */
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
-    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstEax|SrcImmByte, DstEax|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0xE8 - 0xEF */
     DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
     ImplicitOps, DstImplicit|SrcImmByte,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstEax|SrcImplicit, DstEax|SrcImplicit, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
     ImplicitOps, ImplicitOps,
@@ -2617,8 +2617,6 @@ x86_emulate(
 
     case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
         /* Source EA is not encoded via ModRM. */
-        dst.type  = OP_REG;
-        dst.reg   = (unsigned long *)&_regs.eax;
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         if ( (rc = read_ulong(ea.mem.seg, insn_fetch_bytes(ad_bytes),
                               &dst.val, dst.bytes, ctxt, ops)) != 0 )
@@ -3520,9 +3518,7 @@ x86_emulate(
         else
         {
             /* in */
-            dst.type  = OP_REG;
             dst.bytes = op_bytes;
-            dst.reg   = (unsigned long *)&_regs.eax;
             fail_if(ops->read_io == NULL);
             rc = ops->read_io(port, dst.bytes, &dst.val, ctxt);
         }




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

x86emul: use DstEax where possible

While it avoids just a few instructions, we should nevertheless make
use of generic code as much as possible.

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
@@ -124,7 +124,7 @@ static uint8_t opcode_table[256] = {
     ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
     ImplicitOps|Mov, ImplicitOps|Mov, ImplicitOps, ImplicitOps,
     /* 0xA0 - 0xA7 */
-    ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
+    ByteOp|DstEax|SrcImplicit|Mov, DstEax|SrcImplicit|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps|Mov, ImplicitOps|Mov,
     ByteOp|ImplicitOps, ImplicitOps,
@@ -161,12 +161,12 @@ static uint8_t opcode_table[256] = {
     /* 0xE0 - 0xE7 */
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
-    DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
+    DstEax|SrcImmByte, DstEax|SrcImmByte,
     DstImplicit|SrcImmByte, DstImplicit|SrcImmByte,
     /* 0xE8 - 0xEF */
     DstImplicit|SrcImm|Mov, DstImplicit|SrcImm,
     ImplicitOps, DstImplicit|SrcImmByte,
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstEax|SrcImplicit, DstEax|SrcImplicit, ImplicitOps, ImplicitOps,
     /* 0xF0 - 0xF7 */
     0, ImplicitOps, 0, 0,
     ImplicitOps, ImplicitOps,
@@ -2617,8 +2617,6 @@ x86_emulate(
 
     case 0xa0 ... 0xa1: /* mov mem.offs,{%al,%ax,%eax,%rax} */
         /* Source EA is not encoded via ModRM. */
-        dst.type  = OP_REG;
-        dst.reg   = (unsigned long *)&_regs.eax;
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         if ( (rc = read_ulong(ea.mem.seg, insn_fetch_bytes(ad_bytes),
                               &dst.val, dst.bytes, ctxt, ops)) != 0 )
@@ -3520,9 +3518,7 @@ x86_emulate(
         else
         {
             /* in */
-            dst.type  = OP_REG;
             dst.bytes = op_bytes;
-            dst.reg   = (unsigned long *)&_regs.eax;
             fail_if(ops->read_io == NULL);
             rc = ops->read_io(port, dst.bytes, &dst.val, ctxt);
         }

[-- 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 6/7] x86emul: use DstEax where possible

[Andrew Cooper]

On 11/08/16 13:06, Jan Beulich wrote:
> While it avoids just a few instructions, we should nevertheless make
> use of generic code as much as possible.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

[PATCH 7/7] x86emul: introduce SrcImm16

[Jan Beulich]

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

... and use it for RET, LRET, and ENTER processing to limit the amount
of "manual" insn bytes fetching.

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
@@ -39,6 +39,7 @@
 #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 SrcMask     (7<<3)
 /* Generic ModRM decode. */
 #define ModRM       (1<<6)
@@ -143,11 +144,11 @@ static uint8_t opcode_table[256] = {
     DstReg|SrcImm|Mov, DstReg|SrcImm|Mov, DstReg|SrcImm|Mov, DstReg|SrcImm|Mov,
     /* 0xC0 - 0xC7 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
-    ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm16, ImplicitOps,
     DstReg|SrcMem|ModRM|Mov, DstReg|SrcMem|ModRM|Mov,
     ByteOp|DstMem|SrcImm|ModRM|Mov, DstMem|SrcImm|ModRM|Mov,
     /* 0xC8 - 0xCF */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm16, ImplicitOps, DstImplicit|SrcImm16, ImplicitOps,
     ImplicitOps, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD0 - 0xD7 */
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
@@ -1994,6 +1995,11 @@ x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
+    case SrcImm16:
+        src.type  = OP_IMM;
+        src.bytes = 2;
+        src.val   = insn_fetch_type(uint16_t);
+        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */
@@ -2786,16 +2792,14 @@ x86_emulate(
         break;
 
     case 0xc2: /* ret imm16 (near) */
-    case 0xc3: /* ret (near) */ {
-        int offset = (b == 0xc2) ? insn_fetch_type(uint16_t) : 0;
+    case 0xc3: /* ret (near) */
         op_bytes = ((op_bytes == 4) && mode_64bit()) ? 8 : op_bytes;
-        if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
+        if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
                               &dst.val, op_bytes, ctxt, ops)) != 0 ||
              (rc = ops->insn_fetch(x86_seg_cs, dst.val, NULL, 0, ctxt)) )
             goto done;
         _regs.eip = dst.val;
         break;
-    }
 
     case 0xc4: /* les */ {
         unsigned long sel;
@@ -2817,7 +2821,6 @@ x86_emulate(
         goto les;
 
     case 0xc8: /* enter imm16,imm8 */ {
-        uint16_t size = insn_fetch_type(uint16_t);
         uint8_t depth = insn_fetch_type(uint8_t) & 31;
         int i;
 
@@ -2846,7 +2849,7 @@ x86_emulate(
                 goto done;
         }
 
-        sp_pre_dec(size);
+        sp_pre_dec(src.val);
         break;
     }
 
@@ -2874,17 +2877,15 @@ x86_emulate(
         break;
 
     case 0xca: /* ret imm16 (far) */
-    case 0xcb: /* ret (far) */ {
-        int offset = (b == 0xca) ? insn_fetch_type(uint16_t) : 0;
+    case 0xcb: /* ret (far) */
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &dst.val, op_bytes, ctxt, ops)) ||
-             (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
+             (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
                               &src.val, op_bytes, ctxt, ops)) ||
              (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
              (rc = commit_far_branch(&cs, dst.val)) )
             goto done;
         break;
-    }
 
     case 0xcc: /* int3 */
         src.val = EXC_BP;




[-- Attachment #2: x86emul-SrcImm16.patch --]
[-- Type: text/plain, Size: 3712 bytes --]

x86emul: introduce SrcImm16

... and use it for RET, LRET, and ENTER processing to limit the amount
of "manual" insn bytes fetching.

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
@@ -39,6 +39,7 @@
 #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 SrcMask     (7<<3)
 /* Generic ModRM decode. */
 #define ModRM       (1<<6)
@@ -143,11 +144,11 @@ static uint8_t opcode_table[256] = {
     DstReg|SrcImm|Mov, DstReg|SrcImm|Mov, DstReg|SrcImm|Mov, DstReg|SrcImm|Mov,
     /* 0xC0 - 0xC7 */
     ByteOp|DstMem|SrcImm|ModRM, DstMem|SrcImmByte|ModRM,
-    ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm16, ImplicitOps,
     DstReg|SrcMem|ModRM|Mov, DstReg|SrcMem|ModRM|Mov,
     ByteOp|DstMem|SrcImm|ModRM|Mov, DstMem|SrcImm|ModRM|Mov,
     /* 0xC8 - 0xCF */
-    ImplicitOps, ImplicitOps, ImplicitOps, ImplicitOps,
+    DstImplicit|SrcImm16, ImplicitOps, DstImplicit|SrcImm16, ImplicitOps,
     ImplicitOps, DstImplicit|SrcImmByte, ImplicitOps, ImplicitOps,
     /* 0xD0 - 0xD7 */
     ByteOp|DstMem|SrcImplicit|ModRM, DstMem|SrcImplicit|ModRM,
@@ -1994,6 +1995,11 @@ x86_emulate(
         case 4: src.val = insn_fetch_type(int32_t); break;
         }
         break;
+    case SrcImm16:
+        src.type  = OP_IMM;
+        src.bytes = 2;
+        src.val   = insn_fetch_type(uint16_t);
+        break;
     }
 
     /* Decode and fetch the destination operand: register or memory. */
@@ -2786,16 +2792,14 @@ x86_emulate(
         break;
 
     case 0xc2: /* ret imm16 (near) */
-    case 0xc3: /* ret (near) */ {
-        int offset = (b == 0xc2) ? insn_fetch_type(uint16_t) : 0;
+    case 0xc3: /* ret (near) */
         op_bytes = ((op_bytes == 4) && mode_64bit()) ? 8 : op_bytes;
-        if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
+        if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
                               &dst.val, op_bytes, ctxt, ops)) != 0 ||
              (rc = ops->insn_fetch(x86_seg_cs, dst.val, NULL, 0, ctxt)) )
             goto done;
         _regs.eip = dst.val;
         break;
-    }
 
     case 0xc4: /* les */ {
         unsigned long sel;
@@ -2817,7 +2821,6 @@ x86_emulate(
         goto les;
 
     case 0xc8: /* enter imm16,imm8 */ {
-        uint16_t size = insn_fetch_type(uint16_t);
         uint8_t depth = insn_fetch_type(uint8_t) & 31;
         int i;
 
@@ -2846,7 +2849,7 @@ x86_emulate(
                 goto done;
         }
 
-        sp_pre_dec(size);
+        sp_pre_dec(src.val);
         break;
     }
 
@@ -2874,17 +2877,15 @@ x86_emulate(
         break;
 
     case 0xca: /* ret imm16 (far) */
-    case 0xcb: /* ret (far) */ {
-        int offset = (b == 0xca) ? insn_fetch_type(uint16_t) : 0;
+    case 0xcb: /* ret (far) */
         if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes),
                               &dst.val, op_bytes, ctxt, ops)) ||
-             (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + offset),
+             (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
                               &src.val, op_bytes, ctxt, ops)) ||
              (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
              (rc = commit_far_branch(&cs, dst.val)) )
             goto done;
         break;
-    }
 
     case 0xcc: /* int3 */
         src.val = EXC_BP;

[-- 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 7/7] x86emul: introduce SrcImm16

[Andrew Cooper]

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

On 11/08/16 13:07, Jan Beulich wrote:
> ... and use it for RET, LRET, and ENTER processing to limit the amount
> of "manual" insn bytes fetching.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

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

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

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