[Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386

Jan Bobek posted 5 patches 4 years, 11 months ago
Failed in applying to current master (apply log)
risu_i386.c         | 140 ++++++--------------------------------------
risu_reginfo_i386.c | 104 ++++++++++++++++++++++++++++++++
risu_reginfo_i386.h |  38 ++++++++++++
3 files changed, 160 insertions(+), 122 deletions(-)
create mode 100644 risu_reginfo_i386.c
create mode 100644 risu_reginfo_i386.h
[Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
Posted by Jan Bobek 4 years, 11 months ago
Hi all,

here's a patch series that tries to fix the (currently broken) build
of RISU for i386. With the patches applied, I am able to successfully
cross-compile and run RISU for i386 on my x86_64 laptop running Debian
10 with:

$ CC='cc -m32 -std=c99' LD='ld -m32' AS='nasm -f elf32' ARCH=i386 ./configure
$ make
$ ./risu --master --trace test_i386.trace test_i386.bin
$ ./risu --trace test_i386.trace test_i386.bin

There's a couple of points that I'd like to mention/highlight:

1. Most of it is just moving stuff around, however I've implemented
   reginfo_dump_mismatch (based on reginfo_dump and code in other
   architectures) and defined EAX as the param register. There is no
   support for more registers yet, that will need to be added later.

2. Note the '-std=c99' switch in the command-line above; without it,
   GCC defines the symbol 'i386' to 1 and the preprocessor magic for
   including arch-specific headers in risu.h breaks. Does anyone have
   an idea how to fix this in a more robust way?

3. gas (the GNU assembler) chokes on the syntax of test_i386.s; that's
   why I'm using nasm as the assembler above. Is that intentional? I
   haven't found the nasm dependency mentioned anywhere.

   Also, nasm will happily emit the UD1 opcode (0F B9) with no
   operands (see test_i386.s). That's a bit surprising to me, since
   Intel's Software Developer's Manual says UD1 has two operands; I'd
   expect at least a follow-up ModR/M byte. gas refuses to assemble
   UD1 with no operands, and gdb's disassembler gets confused when I
   load up the nasm's binary into risu. Is there something obvious
   that I'm missing?

Thanks,
-Jan Bobek

P.S. This is my first time using git send-email, so please bear with
     me if something goes wrong and/or let me know how I can improve
     my future submissions. Thank you!

Jan Bobek (5):
  risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h
  risu_i386: move reginfo-related code to risu_reginfo_i386.c
  risu_reginfo_i386: implement arch-specific reginfo interface
  risu_i386: implement missing CPU-specific functions
  risu_i386: remove old unused code

 risu_i386.c         | 140 ++++++--------------------------------------
 risu_reginfo_i386.c | 104 ++++++++++++++++++++++++++++++++
 risu_reginfo_i386.h |  38 ++++++++++++
 3 files changed, 160 insertions(+), 122 deletions(-)
 create mode 100644 risu_reginfo_i386.c
 create mode 100644 risu_reginfo_i386.h

-- 
2.20.1


Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
Posted by Richard Henderson 4 years, 11 months ago
On 4/8/19 8:27 AM, Jan Bobek wrote:
> 1. Most of it is just moving stuff around, however I've implemented
>    reginfo_dump_mismatch (based on reginfo_dump and code in other
>    architectures) and defined EAX as the param register. There is no
>    support for more registers yet, that will need to be added later.

It's probably worthwhile to get x86_64 working at the same time.
This isn't too difficult -- see below.

> 2. Note the '-std=c99' switch in the command-line above; without it,
>    GCC defines the symbol 'i386' to 1 and the preprocessor magic for
>    including arch-specific headers in risu.h breaks. Does anyone have
>    an idea how to fix this in a more robust way?

Adding -U$(ARCH) to the command line is probably as good a fix as any.

> 3. gas (the GNU assembler) chokes on the syntax of test_i386.s; that's
>    why I'm using nasm as the assembler above. Is that intentional? I
>    haven't found the nasm dependency mentioned anywhere.

I think rewriting to not require nasm is better.

I'm not sure why it was done this way.  Perhaps Peter's unfamiliarity with x86
assembler, combined with the fact that the Intel manual is not in AT&T syntax?

In any case...

>    Also, nasm will happily emit the UD1 opcode (0F B9) with no
>    operands (see test_i386.s). That's a bit surprising to me, since
>    Intel's Software Developer's Manual says UD1 has two operands; I'd
>    expect at least a follow-up ModR/M byte. gas refuses to assemble
>    UD1 with no operands, and gdb's disassembler gets confused when I
>    load up the nasm's binary into risu. Is there something obvious
>    that I'm missing?

You are not missing anything -- ud1 should require a modrm byte.

My suggestion is to use only UD1 as the "break" insn, with the different OP_*
codes encoded into the modrm byte.  E.g.

void advance_pc(void *vuc)
{
    ucontext_t *uc = (ucontext_t *) vuc;

    /*
     * We assume that this is UD1 as per get_risuop below.
     * This would need tweaking if we want to test expected undefs.
     */
    uc->uc_mcontext.gregs[REG_E(IP)] += 3;
}

int get_risuop(struct reginfo *ri)
{
    if ((ri->faulting_insn & 0xf8ffff) == 0xc0b90f) { /* UD1 %xxx,%eax */
        return (ri->faulting_insn >> 16) & 7;
    }
    return -1;
}

This leads to:

  I    ENUM            INSN
  0    OP_COMPARE      ud1 %eax,%eax
  1    OP_TESTEND      ud1 %ecx,%eax
  2    OP_SETMEMBLOCK  ud1 %edx,%eax
  3    OP_GETMEMBLOCK  ud1 %ebx,%eax
  4    OP_COMPAREMEM   ud1 %esp,%eax


> P.S. This is my first time using git send-email, so please bear with
>      me if something goes wrong and/or let me know how I can improve
>      my future submissions. Thank you!

You've done well with git send-email.  ;-)

The following set of changes Works For Me for i386 and x86_64.  For clarity(?),
I've included diff from master and diff on top of your patch set.


r~
diff --git a/Makefile b/Makefile
index 4aad448..6ab014a 100644
--- a/Makefile
+++ b/Makefile
@@ -17,7 +17,7 @@ VPATH=$(SRCDIR)
 
 CFLAGS ?= -g
 
-ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) $(BUILD_INC) $(CFLAGS) $(EXTRA_CFLAGS)
+ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) -U$(ARCH) $(BUILD_INC) $(CFLAGS) $(EXTRA_CFLAGS)
 
 PROG=risu
 SRCS=risu.c comms.c reginfo.c risu_$(ARCH).c risu_reginfo_$(ARCH).c
@@ -49,6 +49,9 @@ $(PROG): $(OBJS)
 %_$(ARCH).elf: %_$(ARCH).s
 	$(AS) -o $@ $<
 
+%_$(ARCH).elf: %_$(ARCH).S
+	$(CC) $(CPPFLAGS) -o $@ -c $<
+
 clean:
 	rm -f $(PROG) $(OBJS) $(BINS)
 
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index 4ad90e1..755283a 100644
--- a/risu_reginfo_i386.h
+++ b/risu_reginfo_i386.h
@@ -12,7 +12,8 @@
 #ifndef RISU_REGINFO_I386_H
 #define RISU_REGINFO_I386_H
 
-/* This is the data structure we pass over the socket.
+/*
+ * This is the data structure we pass over the socket.
  * It is a simplified and reduced subset of what can
  * be obtained with a ucontext_t*
  */
@@ -21,18 +22,14 @@ struct reginfo {
     gregset_t gregs;
 };
 
-#ifndef REG_GS
-/* Assume that either we get all these defines or none */
-#   define REG_GS      0
-#   define REG_FS      1
-#   define REG_ES      2
-#   define REG_DS      3
-#   define REG_ESP     7
-#   define REG_EAX    11
-#   define REG_TRAPNO 12
-#   define REG_EIP    14
-#   define REG_EFL    16
-#   define REG_UESP   17
-#endif /* !defined(REG_GS) */
+/*
+ * For i386, the defines are named REG_EAX, etc.
+ * For x86_64, the defines are name REG_RAX, etc.
+ */
+#ifdef __x86_64__
+# define REG_E(X)   REG_R##X
+#else
+# define REG_E(X)   REG_E##X
+#endif
 
 #endif /* RISU_REGINFO_I386_H */
diff --git a/risu_i386.c b/risu_i386.c
index 14b0db3..9962b8f 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -20,37 +20,33 @@ void advance_pc(void *vuc)
 {
     ucontext_t *uc = (ucontext_t *) vuc;
 
-    /* We assume that this is either UD1 or UD2.
-     * This would need tweaking if we want to test
-     * expected undefs on x86.
+    /*
+     * We assume that this is UD1 as per get_risuop below.
+     * This would need tweaking if we want to test expected undefs.
      */
-    uc->uc_mcontext.gregs[REG_EIP] += 2;
+    uc->uc_mcontext.gregs[REG_E(IP)] += 3;
 }
 
 void set_ucontext_paramreg(void *vuc, uint64_t value)
 {
     ucontext_t *uc = (ucontext_t *) vuc;
-    uc->uc_mcontext.gregs[REG_EAX] = (uint32_t) value;
+    uc->uc_mcontext.gregs[REG_E(AX)] = value;
 }
 
 uint64_t get_reginfo_paramreg(struct reginfo *ri)
 {
-    return ri->gregs[REG_EAX];
+    return ri->gregs[REG_E(AX)];
 }
 
 int get_risuop(struct reginfo *ri)
 {
-    switch (ri->faulting_insn & 0xffff) {
-    case 0xb90f:                /* UD1 */
-        return OP_COMPARE;
-    case 0x0b0f:                /* UD2 */
-        return OP_TESTEND;
-    default:                    /* unexpected */
-        return -1;
+    if ((ri->faulting_insn & 0xf8ffff) == 0xc0b90f) { /* UD1 %xxx,%eax */
+        return (ri->faulting_insn >> 16) & 7;
     }
+    return -1;
 }
 
 uintptr_t get_pc(struct reginfo *ri)
 {
-    return ri->gregs[REG_EIP];
+    return ri->gregs[REG_E(IP)];
 }
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index 3882261..c4dc14a 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -13,6 +13,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ucontext.h>
+#include <assert.h>
 
 #include "risu.h"
 #include "risu_reginfo_i386.h"
@@ -34,37 +35,50 @@ const int reginfo_size(void)
 void reginfo_init(struct reginfo *ri, ucontext_t *uc)
 {
     int i;
+
+    memset(ri, 0, sizeof(*ri));
+
     for (i = 0; i < NGREG; i++) {
         switch (i) {
-        case REG_ESP:
-        case REG_UESP:
-        case REG_GS:
-        case REG_FS:
-        case REG_ES:
-        case REG_DS:
-        case REG_TRAPNO:
-        case REG_EFL:
-            /* Don't store these registers as it results in mismatches.
-             * In particular valgrind has different values for some
-             * segment registers, and they're boring anyway.
-             * We really shouldn't be ignoring EFL but valgrind doesn't
-             * seem to set it right and I don't care to investigate.
-             */
-            ri->gregs[i] = 0xDEADBEEF;
-            break;
-        case REG_EIP:
-            /* Store the offset from the start of the test image */
+        case REG_E(IP):
+            /* Store the offset from the start of the test image.  */
             ri->gregs[i] = uc->uc_mcontext.gregs[i] - image_start_address;
             break;
-        default:
+        case REG_EFL:
+            /* Store only the "flaggy" bits: SF, ZF, AF, PF, CF.  */
+            ri->gregs[i] = uc->uc_mcontext.gregs[i] & 0xd5;
+            break;
+        case REG_E(SP):
+            /* Ignore the stack.  */
+            ri->gregs[i] = 0xdeadbeef;
+            break;
+        case REG_E(AX):
+        case REG_E(BX):
+        case REG_E(CX):
+        case REG_E(DX):
+        case REG_E(DI):
+        case REG_E(SI):
+        case REG_E(BP):
+#ifdef __x86_64__
+        case REG_R8:
+        case REG_R9:
+        case REG_R10:
+        case REG_R11:
+        case REG_R12:
+        case REG_R13:
+        case REG_R14:
+        case REG_R15:
+#endif
             ri->gregs[i] = uc->uc_mcontext.gregs[i];
             break;
         }
     }
-    /* x86 insns aren't 32 bit but we're not really testing x86 so
-     * this is just to distinguish 'do compare' from 'stop'
+
+    /*
+     * x86 insns aren't 32 bit but 3 bytes are sufficient to
+     * distinguish 'do compare' from 'stop'.
      */
-    ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
+    ri->faulting_insn = *(uint32_t *)uc->uc_mcontext.gregs[REG_E(IP)];
 }
 
 /* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
@@ -74,19 +88,53 @@ int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
 }
 
 static const char *const regname[NGREG] = {
-    "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
-    "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
-    "CS", "EFL", "UESP", "SS"
+    [REG_EFL] = "eflags",
+#ifdef __x86_64__
+    [REG_RIP] = "rip",
+    [REG_RAX] = "rax",
+    [REG_RBX] = "rbx",
+    [REG_RCX] = "rcx",
+    [REG_RDX] = "rdx",
+    [REG_RDI] = "rdi",
+    [REG_RSI] = "rsi",
+    [REG_RBP] = "rbp",
+    [REG_RSP] = "rsp",
+    [REG_R8]  = "r8",
+    [REG_R9]  = "r9",
+    [REG_R10] = "r10",
+    [REG_R11] = "r11",
+    [REG_R12] = "r12",
+    [REG_R13] = "r13",
+    [REG_R14] = "r14",
+    [REG_R15] = "r15",
+#else
+    [REG_EIP] = "eip",
+    [REG_EAX] = "eax",
+    [REG_EBX] = "ebx",
+    [REG_ECX] = "ecx",
+    [REG_EDX] = "edx",
+    [REG_EDI] = "edi",
+    [REG_ESI] = "esi",
+    [REG_EBP] = "ebp",
+    [REG_ESP] = "esp",
+#endif
 };
 
+#ifdef __x86_64__
+# define PRIxREG   "%016llx"
+#else
+# define PRIxREG   "%08x"
+#endif
+
 /* reginfo_dump: print state to a stream, returns nonzero on success */
 int reginfo_dump(struct reginfo *ri, FILE *f)
 {
     int i;
     fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
     for (i = 0; i < NGREG; i++) {
-        fprintf(f, "  %s: %x\n", regname[i] ? regname[i] : "???",
-                ri->gregs[i]);
+        if (regname[i]) {
+            fprintf(f, "  %-6s: " PRIxREG "\n", regname[i], ri->gregs[i]);
+        }
     }
     return !ferror(f);
 }
@@ -96,8 +144,9 @@ int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f)
     int i;
     for (i = 0; i < NGREG; i++) {
         if (m->gregs[i] != a->gregs[i]) {
-            fprintf(f, "Mismatch: Register %s\n", regname[i] ? regname[i] : "???");
-            fprintf(f, "m: [%x] != a: [%x]\n", m->gregs[i], a->gregs[i]);
+            assert(regname[i]);
+            fprintf(f, "Mismatch: %s: " PRIxREG " v " PRIxREG "\n",
+                    regname[i], m->gregs[i], a->gregs[i]);
         }
     }
     return !ferror(f);
diff --git a/configure b/configure
index 65e1819..ca2d7db 100755
--- a/configure
+++ b/configure
@@ -48,12 +48,14 @@ EOF
 }
 
 guess_arch() {
-    if check_define __m68k__ ; then
-        ARCH="m68k"
+    if check_define __aarch64__ ; then
+        ARCH="aarch64"
     elif check_define __arm__ ; then
         ARCH="arm"
-    elif check_define __aarch64__ ; then
-        ARCH="aarch64"
+    elif check_define __i386__ || check_define __x86_64__ ; then
+        ARCH="i386"
+    elif check_define __m68k__ ; then
+        ARCH="m68k"
     elif check_define __powerpc64__ ; then
         ARCH="ppc64"
     else
diff --git a/test_i386.S b/test_i386.S
new file mode 100644
index 0000000..456b99c
--- /dev/null
+++ b/test_i386.S
@@ -0,0 +1,41 @@
+/*#############################################################################
+ * Copyright (c) 2010 Linaro Limited
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Peter Maydell (Linaro) - initial implementation
+ *###########################################################################*/
+
+/* A trivial test image for x86 */
+
+/* Initialise the registers to avoid spurious mismatches */
+	xor	%eax, %eax
+	sahf				/* init eflags */
+
+	mov	$0x12345678, %eax
+	mov	$0x9abcdef0, %ebx
+	mov	$0x97361234, %ecx
+	mov	$0x84310284, %edx
+	mov	$0x83624173, %edi
+	mov	$0xfaebfaeb, %esi
+	mov	$0x84610123, %ebp
+
+#ifdef __x86_64__
+	movq	$0x123456789abcdef0, %r8
+	movq	$0xaaaabbbbccccdddd, %r9
+	movq	$0x1010101010101010, %r10
+	movq	$0x1111111111111111, %r11
+	movq	$0x1212121212121212, %r12
+	movq	$0x1313131313131313, %r13
+	movq	$0x1414141414141414, %r14
+	movq	$0x1515151515151515, %r15
+#endif
+
+/* do compare */
+	ud1	%eax, %eax
+
+/* exit test */
+	ud1	%ecx, %eax
diff --git a/test_i386.s b/test_i386.s
deleted file mode 100644
index a2140a0..0000000
--- a/test_i386.s
+++ /dev/null
@@ -1,27 +0,0 @@
-;###############################################################################
-;# Copyright (c) 2010 Linaro Limited
-;# All rights reserved. This program and the accompanying materials
-;# are made available under the terms of the Eclipse Public License v1.0
-;# which accompanies this distribution, and is available at
-;# http://www.eclipse.org/legal/epl-v10.html
-;#
-;# Contributors:
-;#     Peter Maydell (Linaro) - initial implementation
-;###############################################################################
-
-; A trivial test image for x86
-
-BITS 32
-; Initialise the registers to avoid spurious mismatches
-mov eax, 0x12345678
-mov ebx, 0x9abcdef0
-mov ecx, 0x97361234
-mov edx, 0x84310284
-mov edi, 0x83624173
-mov esi, 0xfaebfaeb
-mov ebp, 0x84610123
-; UD1 : do compare
-UD1
-
-; UD2 : exit test
-UD2
diff --git a/Makefile b/Makefile
index 4aad448..6ab014a 100644
--- a/Makefile
+++ b/Makefile
@@ -17,7 +17,7 @@ VPATH=$(SRCDIR)
 
 CFLAGS ?= -g
 
-ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) $(BUILD_INC) $(CFLAGS) $(EXTRA_CFLAGS)
+ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) -U$(ARCH) $(BUILD_INC) $(CFLAGS) $(EXTRA_CFLAGS)
 
 PROG=risu
 SRCS=risu.c comms.c reginfo.c risu_$(ARCH).c risu_reginfo_$(ARCH).c
@@ -49,6 +49,9 @@ $(PROG): $(OBJS)
 %_$(ARCH).elf: %_$(ARCH).s
 	$(AS) -o $@ $<
 
+%_$(ARCH).elf: %_$(ARCH).S
+	$(CC) $(CPPFLAGS) -o $@ -c $<
+
 clean:
 	rm -f $(PROG) $(OBJS) $(BINS)
 
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
new file mode 100644
index 0000000..755283a
--- /dev/null
+++ b/risu_reginfo_i386.h
@@ -0,0 +1,35 @@
+/*******************************************************************************
+ * Copyright (c) 2010 Linaro Limited
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Peter Maydell (Linaro) - initial implementation
+ ******************************************************************************/
+
+#ifndef RISU_REGINFO_I386_H
+#define RISU_REGINFO_I386_H
+
+/*
+ * This is the data structure we pass over the socket.
+ * It is a simplified and reduced subset of what can
+ * be obtained with a ucontext_t*
+ */
+struct reginfo {
+    uint32_t faulting_insn;
+    gregset_t gregs;
+};
+
+/*
+ * For i386, the defines are named REG_EAX, etc.
+ * For x86_64, the defines are name REG_RAX, etc.
+ */
+#ifdef __x86_64__
+# define REG_E(X)   REG_R##X
+#else
+# define REG_E(X)   REG_E##X
+#endif
+
+#endif /* RISU_REGINFO_I386_H */
diff --git a/risu_i386.c b/risu_i386.c
index 5e7e01d..9962b8f 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -14,147 +14,39 @@
 #include <string.h>
 
 #include "risu.h"
-
-/* This is the data structure we pass over the socket.
- * It is a simplified and reduced subset of what can
- * be obtained with a ucontext_t*
- */
-struct reginfo {
-    uint32_t faulting_insn;
-    gregset_t gregs;
-};
-
-#ifndef REG_GS
-/* Assume that either we get all these defines or none */
-#define REG_GS 0
-#define REG_FS 1
-#define REG_ES 2
-#define REG_DS 3
-#define REG_ESP 7
-#define REG_TRAPNO 12
-#define REG_EIP 14
-#define REG_EFL 16
-#define REG_UESP 17
-#endif
-
-struct reginfo master_ri, apprentice_ri;
-
-static int insn_is_ud2(uint32_t insn)
-{
-    return ((insn & 0xffff) == 0x0b0f);
-}
+#include "risu_reginfo_i386.h"
 
 void advance_pc(void *vuc)
 {
-    /* We assume that this is either UD1 or UD2.
-     * This would need tweaking if we want to test
-     * expected undefs on x86.
+    ucontext_t *uc = (ucontext_t *) vuc;
+
+    /*
+     * We assume that this is UD1 as per get_risuop below.
+     * This would need tweaking if we want to test expected undefs.
      */
-    ucontext_t *uc = vuc;
-    uc->uc_mcontext.gregs[REG_EIP] += 2;
+    uc->uc_mcontext.gregs[REG_E(IP)] += 3;
 }
 
-static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
+void set_ucontext_paramreg(void *vuc, uint64_t value)
 {
-    int i;
-    for (i = 0; i < NGREG; i++) {
-        switch (i) {
-        case REG_ESP:
-        case REG_UESP:
-        case REG_GS:
-        case REG_FS:
-        case REG_ES:
-        case REG_DS:
-        case REG_TRAPNO:
-        case REG_EFL:
-            /* Don't store these registers as it results in mismatches.
-             * In particular valgrind has different values for some
-             * segment registers, and they're boring anyway.
-             * We really shouldn't be ignoring EFL but valgrind doesn't
-             * seem to set it right and I don't care to investigate.
-             */
-            ri->gregs[i] = 0xDEADBEEF;
-            break;
-        case REG_EIP:
-            /* Store the offset from the start of the test image */
-            ri->gregs[i] = uc->uc_mcontext.gregs[i] - image_start_address;
-            break;
-        default:
-            ri->gregs[i] = uc->uc_mcontext.gregs[i];
-            break;
-        }
+    ucontext_t *uc = (ucontext_t *) vuc;
+    uc->uc_mcontext.gregs[REG_E(AX)] = value;
+}
+
+uint64_t get_reginfo_paramreg(struct reginfo *ri)
+{
+    return ri->gregs[REG_E(AX)];
+}
+
+int get_risuop(struct reginfo *ri)
+{
+    if ((ri->faulting_insn & 0xf8ffff) == 0xc0b90f) { /* UD1 %xxx,%eax */
+        return (ri->faulting_insn >> 16) & 7;
     }
-    /* x86 insns aren't 32 bit but we're not really testing x86 so
-     * this is just to distinguish 'do compare' from 'stop'
-     */
-    ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
+    return -1;
 }
 
-
-int send_register_info(int sock, void *uc)
+uintptr_t get_pc(struct reginfo *ri)
 {
-    struct reginfo ri;
-    fill_reginfo(&ri, uc);
-    return send_data_pkt(sock, &ri, sizeof(ri));
-}
-
-/* Read register info from the socket and compare it with that from the
- * ucontext. Return 0 for match, 1 for end-of-test, 2 for mismatch.
- * NB: called from a signal handler.
- */
-int recv_and_compare_register_info(int sock, void *uc)
-{
-    int resp;
-    fill_reginfo(&master_ri, uc);
-    recv_data_pkt(sock, &apprentice_ri, sizeof(apprentice_ri));
-    if (memcmp(&master_ri, &apprentice_ri, sizeof(master_ri)) != 0) {
-        /* mismatch */
-        resp = 2;
-    } else if (insn_is_ud2(master_ri.faulting_insn)) {
-        /* end of test */
-        resp = 1;
-    } else {
-        /* either successful match or expected undef */
-        resp = 0;
-    }
-    send_response_byte(sock, resp);
-    return resp;
-}
-
-static char *regname[] = {
-    "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
-    "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
-    "CS", "EFL", "UESP", "SS", 0
-};
-
-static void dump_reginfo(struct reginfo *ri)
-{
-    int i;
-    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
-    for (i = 0; i < NGREG; i++) {
-        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
-                ri->gregs[i]);
-    }
-}
-
-
-/* Print a useful report on the status of the last comparison
- * done in recv_and_compare_register_info(). This is called on
- * exit, so need not restrict itself to signal-safe functions.
- * Should return 0 if it was a good match (ie end of test)
- * and 1 for a mismatch.
- */
-int report_match_status(void)
-{
-    fprintf(stderr, "match status...\n");
-    fprintf(stderr, "master reginfo:\n");
-    dump_reginfo(&master_ri);
-    fprintf(stderr, "apprentice reginfo:\n");
-    dump_reginfo(&apprentice_ri);
-    if (memcmp(&master_ri, &apprentice_ri, sizeof(master_ri)) == 0) {
-        fprintf(stderr, "match!\n");
-        return 0;
-    }
-    fprintf(stderr, "mismatch!\n");
-    return 1;
+    return ri->gregs[REG_E(IP)];
 }
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
new file mode 100644
index 0000000..c4dc14a
--- /dev/null
+++ b/risu_reginfo_i386.c
@@ -0,0 +1,153 @@
+/*******************************************************************************
+ * Copyright (c) 2010 Linaro Limited
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Peter Maydell (Linaro) - initial implementation
+ ******************************************************************************/
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ucontext.h>
+#include <assert.h>
+
+#include "risu.h"
+#include "risu_reginfo_i386.h"
+
+const struct option * const arch_long_opts;
+const char * const arch_extra_help;
+
+void process_arch_opt(int opt, const char *arg)
+{
+    abort();
+}
+
+const int reginfo_size(void)
+{
+    return sizeof(struct reginfo);
+}
+
+/* reginfo_init: initialize with a ucontext */
+void reginfo_init(struct reginfo *ri, ucontext_t *uc)
+{
+    int i;
+
+    memset(ri, 0, sizeof(*ri));
+
+    for (i = 0; i < NGREG; i++) {
+        switch (i) {
+        case REG_E(IP):
+            /* Store the offset from the start of the test image.  */
+            ri->gregs[i] = uc->uc_mcontext.gregs[i] - image_start_address;
+            break;
+        case REG_EFL:
+            /* Store only the "flaggy" bits: SF, ZF, AF, PF, CF.  */
+            ri->gregs[i] = uc->uc_mcontext.gregs[i] & 0xd5;
+            break;
+        case REG_E(SP):
+            /* Ignore the stack.  */
+            ri->gregs[i] = 0xdeadbeef;
+            break;
+        case REG_E(AX):
+        case REG_E(BX):
+        case REG_E(CX):
+        case REG_E(DX):
+        case REG_E(DI):
+        case REG_E(SI):
+        case REG_E(BP):
+#ifdef __x86_64__
+        case REG_R8:
+        case REG_R9:
+        case REG_R10:
+        case REG_R11:
+        case REG_R12:
+        case REG_R13:
+        case REG_R14:
+        case REG_R15:
+#endif
+            ri->gregs[i] = uc->uc_mcontext.gregs[i];
+            break;
+        }
+    }
+
+    /*
+     * x86 insns aren't 32 bit but 3 bytes are sufficient to
+     * distinguish 'do compare' from 'stop'.
+     */
+    ri->faulting_insn = *(uint32_t *)uc->uc_mcontext.gregs[REG_E(IP)];
+}
+
+/* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
+int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
+{
+    return 0 == memcmp(m, a, sizeof(*m));
+}
+
+static const char *const regname[NGREG] = {
+    [REG_EFL] = "eflags",
+#ifdef __x86_64__
+    [REG_RIP] = "rip",
+    [REG_RAX] = "rax",
+    [REG_RBX] = "rbx",
+    [REG_RCX] = "rcx",
+    [REG_RDX] = "rdx",
+    [REG_RDI] = "rdi",
+    [REG_RSI] = "rsi",
+    [REG_RBP] = "rbp",
+    [REG_RSP] = "rsp",
+    [REG_R8]  = "r8",
+    [REG_R9]  = "r9",
+    [REG_R10] = "r10",
+    [REG_R11] = "r11",
+    [REG_R12] = "r12",
+    [REG_R13] = "r13",
+    [REG_R14] = "r14",
+    [REG_R15] = "r15",
+#else
+    [REG_EIP] = "eip",
+    [REG_EAX] = "eax",
+    [REG_EBX] = "ebx",
+    [REG_ECX] = "ecx",
+    [REG_EDX] = "edx",
+    [REG_EDI] = "edi",
+    [REG_ESI] = "esi",
+    [REG_EBP] = "ebp",
+    [REG_ESP] = "esp",
+#endif
+};
+
+#ifdef __x86_64__
+# define PRIxREG   "%016llx"
+#else
+# define PRIxREG   "%08x"
+#endif
+
+/* reginfo_dump: print state to a stream, returns nonzero on success */
+int reginfo_dump(struct reginfo *ri, FILE *f)
+{
+    int i;
+    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
+    for (i = 0; i < NGREG; i++) {
+        if (regname[i]) {
+            fprintf(f, "  %-6s: " PRIxREG "\n", regname[i], ri->gregs[i]);
+        }
+    }
+    return !ferror(f);
+}
+
+int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f)
+{
+    int i;
+    for (i = 0; i < NGREG; i++) {
+        if (m->gregs[i] != a->gregs[i]) {
+            assert(regname[i]);
+            fprintf(f, "Mismatch: %s: " PRIxREG " v " PRIxREG "\n",
+                    regname[i], m->gregs[i], a->gregs[i]);
+        }
+    }
+    return !ferror(f);
+}
diff --git a/configure b/configure
index 65e1819..ca2d7db 100755
--- a/configure
+++ b/configure
@@ -48,12 +48,14 @@ EOF
 }
 
 guess_arch() {
-    if check_define __m68k__ ; then
-        ARCH="m68k"
+    if check_define __aarch64__ ; then
+        ARCH="aarch64"
     elif check_define __arm__ ; then
         ARCH="arm"
-    elif check_define __aarch64__ ; then
-        ARCH="aarch64"
+    elif check_define __i386__ || check_define __x86_64__ ; then
+        ARCH="i386"
+    elif check_define __m68k__ ; then
+        ARCH="m68k"
     elif check_define __powerpc64__ ; then
         ARCH="ppc64"
     else
diff --git a/test_i386.S b/test_i386.S
new file mode 100644
index 0000000..456b99c
--- /dev/null
+++ b/test_i386.S
@@ -0,0 +1,41 @@
+/*#############################################################################
+ * Copyright (c) 2010 Linaro Limited
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ *     Peter Maydell (Linaro) - initial implementation
+ *###########################################################################*/
+
+/* A trivial test image for x86 */
+
+/* Initialise the registers to avoid spurious mismatches */
+	xor	%eax, %eax
+	sahf				/* init eflags */
+
+	mov	$0x12345678, %eax
+	mov	$0x9abcdef0, %ebx
+	mov	$0x97361234, %ecx
+	mov	$0x84310284, %edx
+	mov	$0x83624173, %edi
+	mov	$0xfaebfaeb, %esi
+	mov	$0x84610123, %ebp
+
+#ifdef __x86_64__
+	movq	$0x123456789abcdef0, %r8
+	movq	$0xaaaabbbbccccdddd, %r9
+	movq	$0x1010101010101010, %r10
+	movq	$0x1111111111111111, %r11
+	movq	$0x1212121212121212, %r12
+	movq	$0x1313131313131313, %r13
+	movq	$0x1414141414141414, %r14
+	movq	$0x1515151515151515, %r15
+#endif
+
+/* do compare */
+	ud1	%eax, %eax
+
+/* exit test */
+	ud1	%ecx, %eax
diff --git a/test_i386.s b/test_i386.s
deleted file mode 100644
index a2140a0..0000000
--- a/test_i386.s
+++ /dev/null
@@ -1,27 +0,0 @@
-;###############################################################################
-;# Copyright (c) 2010 Linaro Limited
-;# All rights reserved. This program and the accompanying materials
-;# are made available under the terms of the Eclipse Public License v1.0
-;# which accompanies this distribution, and is available at
-;# http://www.eclipse.org/legal/epl-v10.html
-;#
-;# Contributors:
-;#     Peter Maydell (Linaro) - initial implementation
-;###############################################################################
-
-; A trivial test image for x86
-
-BITS 32
-; Initialise the registers to avoid spurious mismatches
-mov eax, 0x12345678
-mov ebx, 0x9abcdef0
-mov ecx, 0x97361234
-mov edx, 0x84310284
-mov edi, 0x83624173
-mov esi, 0xfaebfaeb
-mov ebp, 0x84610123
-; UD1 : do compare
-UD1
-
-; UD2 : exit test
-UD2
Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
Posted by Jan Bobek 4 years, 11 months ago
Sorry for the delayed reply, the U.S. tax deadline has caught up with
me, so I spent the last two evenings doing my taxes. (Yuck!)

Anyway...

On 4/8/19 6:18 PM, Richard Henderson wrote:
> On 4/8/19 8:27 AM, Jan Bobek wrote:
>> 2. Note the '-std=c99' switch in the command-line above; without it,
>>    GCC defines the symbol 'i386' to 1 and the preprocessor magic for
>>    including arch-specific headers in risu.h breaks. Does anyone have
>>    an idea how to fix this in a more robust way?
> 
> Adding -U$(ARCH) to the command line is probably as good a fix as any.

I didn't know about -U, nice!

>> 3. gas (the GNU assembler) chokes on the syntax of test_i386.s; that's
>>    why I'm using nasm as the assembler above. Is that intentional? I
>>    haven't found the nasm dependency mentioned anywhere.
> 
> I think rewriting to not require nasm is better.

Agreed.

>>    Also, nasm will happily emit the UD1 opcode (0F B9) with no
>>    operands (see test_i386.s). That's a bit surprising to me, since
>>    Intel's Software Developer's Manual says UD1 has two operands; I'd
>>    expect at least a follow-up ModR/M byte. gas refuses to assemble
>>    UD1 with no operands, and gdb's disassembler gets confused when I
>>    load up the nasm's binary into risu. Is there something obvious
>>    that I'm missing?
> 
> You are not missing anything -- ud1 should require a modrm byte.
> 
> My suggestion is to use only UD1 as the "break" insn, with the different OP_*
> codes encoded into the modrm byte.

I had to laugh when I read this; this is *exactly* what I had in mind,
but then I found out there was no ModR/M byte.

>> P.S. This is my first time using git send-email, so please bear with
>>      me if something goes wrong and/or let me know how I can improve
>>      my future submissions. Thank you!
> 
> You've done well with git send-email.  ;-)

Thanks a lot! :)

-Jan

Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
Posted by Alex Bennée 4 years, 11 months ago
Jan Bobek <jan.bobek@gmail.com> writes:

> Hi all,
>
<snip>
> Thanks,
> -Jan Bobek
>
> P.S. This is my first time using git send-email, so please bear with
>      me if something goes wrong and/or let me know how I can improve
>      my future submissions. Thank you!

Looks good to me. Excellent first patch series ;-)

I assume you'll post a v2 once you've integrated Richard's fixups to the
appropriate patches.

Have you had a chance to look at getting a .risu and risugen working yet?

--
Alex Bennée

Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
Posted by Jan Bobek 4 years, 10 months ago
Hi Alex,

I'm very sorry for the late reply, your emails got mixed up with
everything else in qemu-devel; I didn't setup my mail filters very
well (my bad).

On 4/25/19 9:45 AM, Alex Bennée wrote:
> 
> Jan Bobek <jan.bobek@gmail.com> writes:
> 
>> Hi all,
>>
> <snip>
>> Thanks,
>> -Jan Bobek
>>
>> P.S. This is my first time using git send-email, so please bear with
>>      me if something goes wrong and/or let me know how I can improve
>>      my future submissions. Thank you!
> 
> Looks good to me. Excellent first patch series ;-)

Thank you :) And thanks a lot for the review!

> I assume you'll post a v2 once you've integrated Richard's fixups to the
> appropriate patches.

Sounds reasonable, I'll get that done.

> Have you had a chance to look at getting a .risu and risugen working yet?

That's early work-in-progress as we speak (also, I'm picking up
Perl...).

Thanks again,
-Jan