All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-08 18:27 ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

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

^ permalink raw reply	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-08 18:27 ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

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



^ permalink raw reply	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 1/5] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

In order to build risu successfully for i386, we need files
risu_reginfo_i386.{h,c}; this patch adds the former by extracting the
relevant code from risu_i386.c.

This patch is pure code motion; no functional changes were made.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 23 +----------------------
 risu_reginfo_i386.h | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 22 deletions(-)
 create mode 100644 risu_reginfo_i386.h

diff --git a/risu_i386.c b/risu_i386.c
index 5e7e01d..6798a78 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -14,28 +14,7 @@
 #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
+#include "risu_reginfo_i386.h"
 
 struct reginfo master_ri, apprentice_ri;
 
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
new file mode 100644
index 0000000..5bba439
--- /dev/null
+++ b/risu_reginfo_i386.h
@@ -0,0 +1,37 @@
+/*******************************************************************************
+ * 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;
+};
+
+#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 /* !defined(REG_GS) */
+
+#endif /* RISU_REGINFO_I386_H */
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 1/5] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

In order to build risu successfully for i386, we need files
risu_reginfo_i386.{h,c}; this patch adds the former by extracting the
relevant code from risu_i386.c.

This patch is pure code motion; no functional changes were made.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 23 +----------------------
 risu_reginfo_i386.h | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 22 deletions(-)
 create mode 100644 risu_reginfo_i386.h

diff --git a/risu_i386.c b/risu_i386.c
index 5e7e01d..6798a78 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -14,28 +14,7 @@
 #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
+#include "risu_reginfo_i386.h"
 
 struct reginfo master_ri, apprentice_ri;
 
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
new file mode 100644
index 0000000..5bba439
--- /dev/null
+++ b/risu_reginfo_i386.h
@@ -0,0 +1,37 @@
+/*******************************************************************************
+ * 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;
+};
+
+#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 /* !defined(REG_GS) */
+
+#endif /* RISU_REGINFO_I386_H */
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 2/5] risu_i386: move reginfo-related code to risu_reginfo_i386.c
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

In order to build risu successfully for i386, we need files
risu_reginfo_i386.{h,c}; this patch adds the latter by extracting the
relevant code from risu_i386.c.

This patch is pure code motion; no functional changes were made.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 54 -----------------------------------
 risu_reginfo_i386.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 54 deletions(-)
 create mode 100644 risu_reginfo_i386.c

diff --git a/risu_i386.c b/risu_i386.c
index 6798a78..2d2f325 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -33,43 +33,6 @@ void advance_pc(void *vuc)
     uc->uc_mcontext.gregs[REG_EIP] += 2;
 }
 
-static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
-{
-    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;
-        }
-    }
-    /* 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]);
-}
-
-
 int send_register_info(int sock, void *uc)
 {
     struct reginfo ri;
@@ -100,23 +63,6 @@ int recv_and_compare_register_info(int sock, void *uc)
     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.
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
new file mode 100644
index 0000000..e8d671f
--- /dev/null
+++ b/risu_reginfo_i386.c
@@ -0,0 +1,68 @@
+/*******************************************************************************
+ * 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 <ucontext.h>
+
+#include "risu.h"
+#include "risu_reginfo_i386.h"
+
+static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
+{
+    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;
+        }
+    }
+    /* 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]);
+}
+
+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]);
+    }
+}
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 2/5] risu_i386: move reginfo-related code to risu_reginfo_i386.c
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

In order to build risu successfully for i386, we need files
risu_reginfo_i386.{h,c}; this patch adds the latter by extracting the
relevant code from risu_i386.c.

This patch is pure code motion; no functional changes were made.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 54 -----------------------------------
 risu_reginfo_i386.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 54 deletions(-)
 create mode 100644 risu_reginfo_i386.c

diff --git a/risu_i386.c b/risu_i386.c
index 6798a78..2d2f325 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -33,43 +33,6 @@ void advance_pc(void *vuc)
     uc->uc_mcontext.gregs[REG_EIP] += 2;
 }
 
-static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
-{
-    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;
-        }
-    }
-    /* 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]);
-}
-
-
 int send_register_info(int sock, void *uc)
 {
     struct reginfo ri;
@@ -100,23 +63,6 @@ int recv_and_compare_register_info(int sock, void *uc)
     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.
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
new file mode 100644
index 0000000..e8d671f
--- /dev/null
+++ b/risu_reginfo_i386.c
@@ -0,0 +1,68 @@
+/*******************************************************************************
+ * 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 <ucontext.h>
+
+#include "risu.h"
+#include "risu_reginfo_i386.h"
+
+static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
+{
+    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;
+        }
+    }
+    /* 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]);
+}
+
+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]);
+    }
+}
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 3/5] risu_reginfo_i386: implement arch-specific reginfo interface
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

CPU-specific code in risu_reginfo_* is expected to define and export
the following symbols:

- arch_long_opts, arch_extra_help, process_arch_opt
- reginfo_size
- reginfo_init
- reginfo_is_eq
- reginfo_dump, reginfo_dump_mismatch

Make risu_reginfo_i386.c implement this interface.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_reginfo_i386.c | 48 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index e8d671f..3882261 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -10,12 +10,28 @@
  ******************************************************************************/
 
 #include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 #include <ucontext.h>
 
 #include "risu.h"
 #include "risu_reginfo_i386.h"
 
-static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
+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;
     for (i = 0; i < NGREG; i++) {
@@ -51,18 +67,38 @@ static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
     ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
 }
 
-static char *regname[] = {
+/* 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] = {
     "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
     "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
-    "CS", "EFL", "UESP", "SS", 0
+    "CS", "EFL", "UESP", "SS"
 };
 
-static void dump_reginfo(struct reginfo *ri)
+/* reginfo_dump: print state to a stream, returns nonzero on success */
+int reginfo_dump(struct reginfo *ri, FILE *f)
 {
     int i;
-    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
+    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
     for (i = 0; i < NGREG; i++) {
-        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
+        fprintf(f, "  %s: %x\n", regname[i] ? 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]) {
+            fprintf(f, "Mismatch: Register %s\n", regname[i] ? regname[i] : "???");
+            fprintf(f, "m: [%x] != a: [%x]\n", m->gregs[i], a->gregs[i]);
+        }
+    }
+    return !ferror(f);
 }
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 3/5] risu_reginfo_i386: implement arch-specific reginfo interface
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

CPU-specific code in risu_reginfo_* is expected to define and export
the following symbols:

- arch_long_opts, arch_extra_help, process_arch_opt
- reginfo_size
- reginfo_init
- reginfo_is_eq
- reginfo_dump, reginfo_dump_mismatch

Make risu_reginfo_i386.c implement this interface.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_reginfo_i386.c | 48 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index e8d671f..3882261 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -10,12 +10,28 @@
  ******************************************************************************/
 
 #include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 #include <ucontext.h>
 
 #include "risu.h"
 #include "risu_reginfo_i386.h"
 
-static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
+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;
     for (i = 0; i < NGREG; i++) {
@@ -51,18 +67,38 @@ static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
     ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
 }
 
-static char *regname[] = {
+/* 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] = {
     "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
     "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
-    "CS", "EFL", "UESP", "SS", 0
+    "CS", "EFL", "UESP", "SS"
 };
 
-static void dump_reginfo(struct reginfo *ri)
+/* reginfo_dump: print state to a stream, returns nonzero on success */
+int reginfo_dump(struct reginfo *ri, FILE *f)
 {
     int i;
-    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
+    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
     for (i = 0; i < NGREG; i++) {
-        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
+        fprintf(f, "  %s: %x\n", regname[i] ? 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]) {
+            fprintf(f, "Mismatch: Register %s\n", regname[i] ? regname[i] : "???");
+            fprintf(f, "m: [%x] != a: [%x]\n", m->gregs[i], a->gregs[i]);
+        }
+    }
+    return !ferror(f);
 }
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 4/5] risu_i386: implement missing CPU-specific functions
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

risu_i386.c is expected to implement the following functions:

- advance_pc
- get_reginfo_paramreg, set_ucontext_paramreg
- get_risuop
- get_pc

This patch adds the necessary code. (We use EAX as the parameter
register.)

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 31 ++++++++++++++++++++++++++++++-
 risu_reginfo_i386.h |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/risu_i386.c b/risu_i386.c
index 2d2f325..eb4dff4 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -25,14 +25,43 @@ static int insn_is_ud2(uint32_t insn)
 
 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.
      */
-    ucontext_t *uc = vuc;
     uc->uc_mcontext.gregs[REG_EIP] += 2;
 }
 
+void set_ucontext_paramreg(void *vuc, uint64_t value)
+{
+    ucontext_t *uc = (ucontext_t *) vuc;
+    uc->uc_mcontext.gregs[REG_EAX] = (uint32_t) value;
+}
+
+uint64_t get_reginfo_paramreg(struct reginfo *ri)
+{
+    return ri->gregs[REG_EAX];
+}
+
+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;
+    }
+}
+
+uintptr_t get_pc(struct reginfo *ri)
+{
+    return ri->gregs[REG_EIP];
+}
+
 int send_register_info(int sock, void *uc)
 {
     struct reginfo ri;
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index 5bba439..4ad90e1 100644
--- a/risu_reginfo_i386.h
+++ b/risu_reginfo_i386.h
@@ -28,6 +28,7 @@ struct reginfo {
 #   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
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 4/5] risu_i386: implement missing CPU-specific functions
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

risu_i386.c is expected to implement the following functions:

- advance_pc
- get_reginfo_paramreg, set_ucontext_paramreg
- get_risuop
- get_pc

This patch adds the necessary code. (We use EAX as the parameter
register.)

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c         | 31 ++++++++++++++++++++++++++++++-
 risu_reginfo_i386.h |  1 +
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/risu_i386.c b/risu_i386.c
index 2d2f325..eb4dff4 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -25,14 +25,43 @@ static int insn_is_ud2(uint32_t insn)
 
 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.
      */
-    ucontext_t *uc = vuc;
     uc->uc_mcontext.gregs[REG_EIP] += 2;
 }
 
+void set_ucontext_paramreg(void *vuc, uint64_t value)
+{
+    ucontext_t *uc = (ucontext_t *) vuc;
+    uc->uc_mcontext.gregs[REG_EAX] = (uint32_t) value;
+}
+
+uint64_t get_reginfo_paramreg(struct reginfo *ri)
+{
+    return ri->gregs[REG_EAX];
+}
+
+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;
+    }
+}
+
+uintptr_t get_pc(struct reginfo *ri)
+{
+    return ri->gregs[REG_EIP];
+}
+
 int send_register_info(int sock, void *uc)
 {
     struct reginfo ri;
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index 5bba439..4ad90e1 100644
--- a/risu_reginfo_i386.h
+++ b/risu_reginfo_i386.h
@@ -28,6 +28,7 @@ struct reginfo {
 #   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
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 5/5] risu_i386: remove old unused code
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Jan Bobek, Richard Henderson, Alex Bennée, Peter Maydell,
	Stefan Hajnoczi

The code being removed is a remnant of the past implementation; it has
since been replaced by its more powerful, architecture-independent
counterpart in reginfo.c.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c | 58 -----------------------------------------------------
 1 file changed, 58 deletions(-)

diff --git a/risu_i386.c b/risu_i386.c
index eb4dff4..14b0db3 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -16,13 +16,6 @@
 #include "risu.h"
 #include "risu_reginfo_i386.h"
 
-struct reginfo master_ri, apprentice_ri;
-
-static int insn_is_ud2(uint32_t insn)
-{
-    return ((insn & 0xffff) == 0x0b0f);
-}
-
 void advance_pc(void *vuc)
 {
     ucontext_t *uc = (ucontext_t *) vuc;
@@ -61,54 +54,3 @@ uintptr_t get_pc(struct reginfo *ri)
 {
     return ri->gregs[REG_EIP];
 }
-
-int send_register_info(int sock, void *uc)
-{
-    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;
-}
-
-/* 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;
-}
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* [Qemu-devel] [RISU PATCH 5/5] risu_i386: remove old unused code
@ 2019-04-08 18:27   ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-08 18:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alex Bennée, Richard Henderson, Jan Bobek, Stefan Hajnoczi,
	Peter Maydell

The code being removed is a remnant of the past implementation; it has
since been replaced by its more powerful, architecture-independent
counterpart in reginfo.c.

Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c | 58 -----------------------------------------------------
 1 file changed, 58 deletions(-)

diff --git a/risu_i386.c b/risu_i386.c
index eb4dff4..14b0db3 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -16,13 +16,6 @@
 #include "risu.h"
 #include "risu_reginfo_i386.h"
 
-struct reginfo master_ri, apprentice_ri;
-
-static int insn_is_ud2(uint32_t insn)
-{
-    return ((insn & 0xffff) == 0x0b0f);
-}
-
 void advance_pc(void *vuc)
 {
     ucontext_t *uc = (ucontext_t *) vuc;
@@ -61,54 +54,3 @@ uintptr_t get_pc(struct reginfo *ri)
 {
     return ri->gregs[REG_EIP];
 }
-
-int send_register_info(int sock, void *uc)
-{
-    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;
-}
-
-/* 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;
-}
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-08 22:18   ` Richard Henderson
  0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2019-04-08 22:18 UTC (permalink / raw)
  To: Jan Bobek, qemu-devel; +Cc: Alex Bennée, Peter Maydell, Stefan Hajnoczi

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

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~

[-- Attachment #2: z-diff-jb --]
[-- Type: text/plain, Size: 10817 bytes --]

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

[-- Attachment #3: z-diff-master --]
[-- Type: text/plain, Size: 14052 bytes --]

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-08 22:18   ` Richard Henderson
  0 siblings, 0 replies; 27+ messages in thread
From: Richard Henderson @ 2019-04-08 22:18 UTC (permalink / raw)
  To: Jan Bobek, qemu-devel; +Cc: Peter Maydell, Alex Bennée, Stefan Hajnoczi

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

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~

[-- Attachment #2: z-diff-jb --]
[-- Type: text/plain, Size: 10817 bytes --]

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

[-- Attachment #3: z-diff-master --]
[-- Type: text/plain, Size: 14052 bytes --]

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-12  1:43     ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-12  1:43 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Alex Bennée, Peter Maydell, Stefan Hajnoczi

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-12  1:43     ` Jan Bobek
  0 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-04-12  1:43 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel
  Cc: Peter Maydell, Alex Bennée, Stefan Hajnoczi

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 1/5] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h
@ 2019-04-25 13:39     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:39 UTC (permalink / raw)
  To: Jan Bobek; +Cc: qemu-devel, Richard Henderson, Peter Maydell, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> In order to build risu successfully for i386, we need files
> risu_reginfo_i386.{h,c}; this patch adds the former by extracting the
> relevant code from risu_i386.c.
>
> This patch is pure code motion; no functional changes were made.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_i386.c         | 23 +----------------------
>  risu_reginfo_i386.h | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 22 deletions(-)
>  create mode 100644 risu_reginfo_i386.h
>
> diff --git a/risu_i386.c b/risu_i386.c
> index 5e7e01d..6798a78 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -14,28 +14,7 @@
>  #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
> +#include "risu_reginfo_i386.h"
>
>  struct reginfo master_ri, apprentice_ri;
>
> diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
> new file mode 100644
> index 0000000..5bba439
> --- /dev/null
> +++ b/risu_reginfo_i386.h
> @@ -0,0 +1,37 @@
> +/*******************************************************************************
> + * 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;
> +};
> +

The actual reg definitions can be dropped here (as rth does in his fixup
series). We can get them from ucontext.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> +#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 /* !defined(REG_GS) */
> +
> +#endif /* RISU_REGINFO_I386_H */


--
Alex Bennée

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 1/5] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h
@ 2019-04-25 13:39     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:39 UTC (permalink / raw)
  To: Jan Bobek; +Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> In order to build risu successfully for i386, we need files
> risu_reginfo_i386.{h,c}; this patch adds the former by extracting the
> relevant code from risu_i386.c.
>
> This patch is pure code motion; no functional changes were made.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_i386.c         | 23 +----------------------
>  risu_reginfo_i386.h | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 22 deletions(-)
>  create mode 100644 risu_reginfo_i386.h
>
> diff --git a/risu_i386.c b/risu_i386.c
> index 5e7e01d..6798a78 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -14,28 +14,7 @@
>  #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
> +#include "risu_reginfo_i386.h"
>
>  struct reginfo master_ri, apprentice_ri;
>
> diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
> new file mode 100644
> index 0000000..5bba439
> --- /dev/null
> +++ b/risu_reginfo_i386.h
> @@ -0,0 +1,37 @@
> +/*******************************************************************************
> + * 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;
> +};
> +

The actual reg definitions can be dropped here (as rth does in his fixup
series). We can get them from ucontext.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> +#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 /* !defined(REG_GS) */
> +
> +#endif /* RISU_REGINFO_I386_H */


--
Alex Bennée


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 2/5] risu_i386: move reginfo-related code to risu_reginfo_i386.c
@ 2019-04-25 13:39     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:39 UTC (permalink / raw)
  To: Jan Bobek; +Cc: qemu-devel, Richard Henderson, Peter Maydell, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> In order to build risu successfully for i386, we need files
> risu_reginfo_i386.{h,c}; this patch adds the latter by extracting the
> relevant code from risu_i386.c.
>
> This patch is pure code motion; no functional changes were made.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_i386.c         | 54 -----------------------------------
>  risu_reginfo_i386.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 54 deletions(-)
>  create mode 100644 risu_reginfo_i386.c
>
> diff --git a/risu_i386.c b/risu_i386.c
> index 6798a78..2d2f325 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -33,43 +33,6 @@ void advance_pc(void *vuc)
>      uc->uc_mcontext.gregs[REG_EIP] += 2;
>  }
>
> -static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> -{
> -    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;
> -        }
> -    }
> -    /* 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]);
> -}
> -
> -
>  int send_register_info(int sock, void *uc)
>  {
>      struct reginfo ri;
> @@ -100,23 +63,6 @@ int recv_and_compare_register_info(int sock, void *uc)
>      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.
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> new file mode 100644
> index 0000000..e8d671f
> --- /dev/null
> +++ b/risu_reginfo_i386.c
> @@ -0,0 +1,68 @@
> +/*******************************************************************************
> + * 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 <ucontext.h>
> +
> +#include "risu.h"
> +#include "risu_reginfo_i386.h"
> +
> +static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> +{
> +    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;
> +        }
> +    }
> +    /* 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]);
> +}
> +
> +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]);
> +    }
> +}


--
Alex Bennée

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 2/5] risu_i386: move reginfo-related code to risu_reginfo_i386.c
@ 2019-04-25 13:39     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:39 UTC (permalink / raw)
  To: Jan Bobek; +Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> In order to build risu successfully for i386, we need files
> risu_reginfo_i386.{h,c}; this patch adds the latter by extracting the
> relevant code from risu_i386.c.
>
> This patch is pure code motion; no functional changes were made.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_i386.c         | 54 -----------------------------------
>  risu_reginfo_i386.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 54 deletions(-)
>  create mode 100644 risu_reginfo_i386.c
>
> diff --git a/risu_i386.c b/risu_i386.c
> index 6798a78..2d2f325 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -33,43 +33,6 @@ void advance_pc(void *vuc)
>      uc->uc_mcontext.gregs[REG_EIP] += 2;
>  }
>
> -static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> -{
> -    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;
> -        }
> -    }
> -    /* 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]);
> -}
> -
> -
>  int send_register_info(int sock, void *uc)
>  {
>      struct reginfo ri;
> @@ -100,23 +63,6 @@ int recv_and_compare_register_info(int sock, void *uc)
>      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.
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> new file mode 100644
> index 0000000..e8d671f
> --- /dev/null
> +++ b/risu_reginfo_i386.c
> @@ -0,0 +1,68 @@
> +/*******************************************************************************
> + * 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 <ucontext.h>
> +
> +#include "risu.h"
> +#include "risu_reginfo_i386.h"
> +
> +static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> +{
> +    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;
> +        }
> +    }
> +    /* 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]);
> +}
> +
> +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]);
> +    }
> +}


--
Alex Bennée


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 3/5] risu_reginfo_i386: implement arch-specific reginfo interface
@ 2019-04-25 13:42     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:42 UTC (permalink / raw)
  To: Jan Bobek; +Cc: qemu-devel, Richard Henderson, Peter Maydell, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> CPU-specific code in risu_reginfo_* is expected to define and export
> the following symbols:
>
> - arch_long_opts, arch_extra_help, process_arch_opt
> - reginfo_size
> - reginfo_init
> - reginfo_is_eq
> - reginfo_dump, reginfo_dump_mismatch
>
> Make risu_reginfo_i386.c implement this interface.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_reginfo_i386.c | 48 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> index e8d671f..3882261 100644
> --- a/risu_reginfo_i386.c
> +++ b/risu_reginfo_i386.c
> @@ -10,12 +10,28 @@
>   ******************************************************************************/
>
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <ucontext.h>
>
>  #include "risu.h"
>  #include "risu_reginfo_i386.h"
>
> -static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> +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;
>      for (i = 0; i < NGREG; i++) {
> @@ -51,18 +67,38 @@ static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
>      ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
>  }
>
> -static char *regname[] = {
> +/* 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] = {
>      "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
>      "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
> -    "CS", "EFL", "UESP", "SS", 0
> +    "CS", "EFL", "UESP", "SS"
>  };
>
> -static void dump_reginfo(struct reginfo *ri)
> +/* reginfo_dump: print state to a stream, returns nonzero on success */
> +int reginfo_dump(struct reginfo *ri, FILE *f)
>  {
>      int i;
> -    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
> +    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
>      for (i = 0; i < NGREG; i++) {
> -        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
> +        fprintf(f, "  %s: %x\n", regname[i] ? 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]) {
> +            fprintf(f, "Mismatch: Register %s\n", regname[i] ? regname[i] : "???");
> +            fprintf(f, "m: [%x] != a: [%x]\n", m->gregs[i], a->gregs[i]);
> +        }
> +    }
> +    return !ferror(f);
>  }


--
Alex Bennée

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 3/5] risu_reginfo_i386: implement arch-specific reginfo interface
@ 2019-04-25 13:42     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:42 UTC (permalink / raw)
  To: Jan Bobek; +Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> CPU-specific code in risu_reginfo_* is expected to define and export
> the following symbols:
>
> - arch_long_opts, arch_extra_help, process_arch_opt
> - reginfo_size
> - reginfo_init
> - reginfo_is_eq
> - reginfo_dump, reginfo_dump_mismatch
>
> Make risu_reginfo_i386.c implement this interface.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_reginfo_i386.c | 48 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> index e8d671f..3882261 100644
> --- a/risu_reginfo_i386.c
> +++ b/risu_reginfo_i386.c
> @@ -10,12 +10,28 @@
>   ******************************************************************************/
>
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <ucontext.h>
>
>  #include "risu.h"
>  #include "risu_reginfo_i386.h"
>
> -static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> +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;
>      for (i = 0; i < NGREG; i++) {
> @@ -51,18 +67,38 @@ static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
>      ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
>  }
>
> -static char *regname[] = {
> +/* 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] = {
>      "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
>      "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
> -    "CS", "EFL", "UESP", "SS", 0
> +    "CS", "EFL", "UESP", "SS"
>  };
>
> -static void dump_reginfo(struct reginfo *ri)
> +/* reginfo_dump: print state to a stream, returns nonzero on success */
> +int reginfo_dump(struct reginfo *ri, FILE *f)
>  {
>      int i;
> -    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
> +    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
>      for (i = 0; i < NGREG; i++) {
> -        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
> +        fprintf(f, "  %s: %x\n", regname[i] ? 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]) {
> +            fprintf(f, "Mismatch: Register %s\n", regname[i] ? regname[i] : "???");
> +            fprintf(f, "m: [%x] != a: [%x]\n", m->gregs[i], a->gregs[i]);
> +        }
> +    }
> +    return !ferror(f);
>  }


--
Alex Bennée


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 5/5] risu_i386: remove old unused code
@ 2019-04-25 13:43     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:43 UTC (permalink / raw)
  To: Jan Bobek; +Cc: qemu-devel, Richard Henderson, Peter Maydell, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> The code being removed is a remnant of the past implementation; it has
> since been replaced by its more powerful, architecture-independent
> counterpart in reginfo.c.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_i386.c | 58 -----------------------------------------------------
>  1 file changed, 58 deletions(-)
>
> diff --git a/risu_i386.c b/risu_i386.c
> index eb4dff4..14b0db3 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -16,13 +16,6 @@
>  #include "risu.h"
>  #include "risu_reginfo_i386.h"
>
> -struct reginfo master_ri, apprentice_ri;
> -
> -static int insn_is_ud2(uint32_t insn)
> -{
> -    return ((insn & 0xffff) == 0x0b0f);
> -}
> -
>  void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = (ucontext_t *) vuc;
> @@ -61,54 +54,3 @@ uintptr_t get_pc(struct reginfo *ri)
>  {
>      return ri->gregs[REG_EIP];
>  }
> -
> -int send_register_info(int sock, void *uc)
> -{
> -    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;
> -}
> -
> -/* 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;
> -}


--
Alex Bennée

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 5/5] risu_i386: remove old unused code
@ 2019-04-25 13:43     ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:43 UTC (permalink / raw)
  To: Jan Bobek; +Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi


Jan Bobek <jan.bobek@gmail.com> writes:

> The code being removed is a remnant of the past implementation; it has
> since been replaced by its more powerful, architecture-independent
> counterpart in reginfo.c.
>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  risu_i386.c | 58 -----------------------------------------------------
>  1 file changed, 58 deletions(-)
>
> diff --git a/risu_i386.c b/risu_i386.c
> index eb4dff4..14b0db3 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -16,13 +16,6 @@
>  #include "risu.h"
>  #include "risu_reginfo_i386.h"
>
> -struct reginfo master_ri, apprentice_ri;
> -
> -static int insn_is_ud2(uint32_t insn)
> -{
> -    return ((insn & 0xffff) == 0x0b0f);
> -}
> -
>  void advance_pc(void *vuc)
>  {
>      ucontext_t *uc = (ucontext_t *) vuc;
> @@ -61,54 +54,3 @@ uintptr_t get_pc(struct reginfo *ri)
>  {
>      return ri->gregs[REG_EIP];
>  }
> -
> -int send_register_info(int sock, void *uc)
> -{
> -    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;
> -}
> -
> -/* 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;
> -}


--
Alex Bennée


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-25 13:45   ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:45 UTC (permalink / raw)
  To: Jan Bobek; +Cc: qemu-devel, Richard Henderson, Peter Maydell, Stefan Hajnoczi


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

^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
@ 2019-04-25 13:45   ` Alex Bennée
  0 siblings, 0 replies; 27+ messages in thread
From: Alex Bennée @ 2019-04-25 13:45 UTC (permalink / raw)
  To: Jan Bobek; +Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi


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


^ permalink raw reply	[flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386
  2019-04-25 13:45   ` Alex Bennée
  (?)
@ 2019-05-15 14:32   ` Jan Bobek
  -1 siblings, 0 replies; 27+ messages in thread
From: Jan Bobek @ 2019-05-15 14:32 UTC (permalink / raw)
  To: Alex Bennée
  Cc: Peter Maydell, Richard Henderson, qemu-devel, Stefan Hajnoczi

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2019-05-15 14:33 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08 18:27 [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386 Jan Bobek
2019-04-08 18:27 ` Jan Bobek
2019-04-08 18:27 ` [Qemu-devel] [RISU PATCH 1/5] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h Jan Bobek
2019-04-08 18:27   ` Jan Bobek
2019-04-25 13:39   ` Alex Bennée
2019-04-25 13:39     ` Alex Bennée
2019-04-08 18:27 ` [Qemu-devel] [RISU PATCH 2/5] risu_i386: move reginfo-related code to risu_reginfo_i386.c Jan Bobek
2019-04-08 18:27   ` Jan Bobek
2019-04-25 13:39   ` Alex Bennée
2019-04-25 13:39     ` Alex Bennée
2019-04-08 18:27 ` [Qemu-devel] [RISU PATCH 3/5] risu_reginfo_i386: implement arch-specific reginfo interface Jan Bobek
2019-04-08 18:27   ` Jan Bobek
2019-04-25 13:42   ` Alex Bennée
2019-04-25 13:42     ` Alex Bennée
2019-04-08 18:27 ` [Qemu-devel] [RISU PATCH 4/5] risu_i386: implement missing CPU-specific functions Jan Bobek
2019-04-08 18:27   ` Jan Bobek
2019-04-08 18:27 ` [Qemu-devel] [RISU PATCH 5/5] risu_i386: remove old unused code Jan Bobek
2019-04-08 18:27   ` Jan Bobek
2019-04-25 13:43   ` Alex Bennée
2019-04-25 13:43     ` Alex Bennée
2019-04-08 22:18 ` [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386 Richard Henderson
2019-04-08 22:18   ` Richard Henderson
2019-04-12  1:43   ` Jan Bobek
2019-04-12  1:43     ` Jan Bobek
2019-04-25 13:45 ` Alex Bennée
2019-04-25 13:45   ` Alex Bennée
2019-05-15 14:32   ` Jan Bobek

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.