[Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Jan Bobek]

This patch series adds support for i386 and x86_64 architectures to
RISU.  Notably, vector registers (SSE, AVX, AVX-512) are supported for
verification of the apprentice. This is V2 of the series posted in
[1].

I decided not to drop the register definitions from the second patch
as suggested by Alex Bennée [4], but replaced them in the fourth patch
instead. This keeps the second and third patches code-motion only.

I wasn't 100% sure how to acknowledge Richard's contributions in some
of the patches, and eventually decided to include a Suggested-by:
line. Let me know if that's (not) acceptable.

 -Jan

Changes in V2:
  - included Richard Henderson's fix-ups [2] and vector register
    support [3]
  - replaced the magic numbers for XSAVE feature sets with symbolic
    constants
  - symbolic names ("sse", "avx", "avx512") as well as numbers are
    accepted for the parameter --xfeatures

References:
  1. https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg01294.html
  2. https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg01338.html
  3. https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg01371.html
  4. https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg04307.html

Jan Bobek (10):
  Makefile: undefine the arch name symbol
  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
  test_i386: change syntax from nasm to gas
  configure: add i386/x86_64 architectures
  risu_reginfo_i386: replace xfeature constants with symbolic names
  risu_reginfo_i386: accept named feature sets for --xfeature

Richard Henderson (1):
  i386: Add avx512 state to reginfo_t

 configure           |  10 +-
 Makefile            |   5 +-
 risu_reginfo_i386.h |  49 ++++++
 risu_i386.c         | 142 ++--------------
 risu_reginfo_i386.c | 392 ++++++++++++++++++++++++++++++++++++++++++++
 test_i386.S         |  80 +++++++++
 test_i386.s         |  27 ---
 7 files changed, 548 insertions(+), 157 deletions(-)
 create mode 100644 risu_reginfo_i386.h
 create mode 100644 risu_reginfo_i386.c
 create mode 100644 test_i386.S
 delete mode 100644 test_i386.s

-- 
2.20.1

[Qemu-devel] [RISU v2 01/11] Makefile: undefine the arch name symbol

[Jan Bobek]

At least GCC defines the symbol "i386" to 1 to signal the target
platform. We need to use "i386" as an undefined symbol in order to
correctly include risu_reginfo_i386.h from risu.h. Add an -U option to
the build command to make sure the symbol remains undefined.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 4aad448..b362dbe 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
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 01/11] Makefile: undefine the arch name symbol

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> At least GCC defines the symbol "i386" to 1 to signal the target
> platform. We need to use "i386" as an undefined symbol in order to
> correctly include risu_reginfo_i386.h from risu.h. Add an -U option to
> the build command to make sure the symbol remains undefined.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 01/11] Makefile: undefine the arch name symbol

[Alex Bennée]

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

> At least GCC defines the symbol "i386" to 1 to signal the target
> platform. We need to use "i386" as an undefined symbol in order to
> correctly include risu_reginfo_i386.h from risu.h. Add an -U option to
> the build command to make sure the symbol remains undefined.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

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

> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 4aad448..b362dbe 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


--
Alex Bennée

[Qemu-devel] [RISU v2 02/11] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h

[Jan Bobek]

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.

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

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 */
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;
 
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 02/11] risu_i386: move reginfo_t and related defines to risu_reginfo_i386.h

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> 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.
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_reginfo_i386.h | 37 +++++++++++++++++++++++++++++++++++++
>  risu_i386.c         | 23 +----------------------
>  2 files changed, 38 insertions(+), 22 deletions(-)
>  create mode 100644 risu_reginfo_i386.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [RISU v2 03/11] risu_i386: move reginfo-related code to risu_reginfo_i386.c

[Jan Bobek]

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.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
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

Re: [Qemu-devel] [RISU v2 03/11] risu_i386: move reginfo-related code to risu_reginfo_i386.c

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> 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.
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 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

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [RISU v2 04/11] risu_reginfo_i386: implement arch-specific reginfo interface

[Jan Bobek]

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; and while we're at
it, expand the support to x86_64 as well.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_reginfo_i386.h |  24 ++++----
 risu_reginfo_i386.c | 147 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 127 insertions(+), 44 deletions(-)

diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index 5bba439..e350f01 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,17 +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_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 named 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_reginfo_i386.c b/risu_reginfo_i386.c
index e8d671f..c4dc14a 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -10,59 +10,144 @@
  ******************************************************************************/
 
 #include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
 #include <ucontext.h>
+#include <assert.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;
+
+    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)];
 }
 
-static char *regname[] = {
-    "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
-    "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
-    "CS", "EFL", "UESP", "SS", 0
+/* 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
 };
 
-static void dump_reginfo(struct reginfo *ri)
+#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(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] : "???",
-                ri->gregs[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);
 }
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 04/11] risu_reginfo_i386: implement arch-specific reginfo interface

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> 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; and while we're at
> it, expand the support to x86_64 as well.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_reginfo_i386.h |  24 ++++----
>  risu_reginfo_i386.c | 147 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 127 insertions(+), 44 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 04/11] risu_reginfo_i386: implement arch-specific reginfo interface

[Alex Bennée]

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; and while we're at
> it, expand the support to x86_64 as well.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

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

> ---
>  risu_reginfo_i386.h |  24 ++++----
>  risu_reginfo_i386.c | 147 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 127 insertions(+), 44 deletions(-)
>
> diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
> index 5bba439..e350f01 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,17 +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_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 named 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_reginfo_i386.c b/risu_reginfo_i386.c
> index e8d671f..c4dc14a 100644
> --- a/risu_reginfo_i386.c
> +++ b/risu_reginfo_i386.c
> @@ -10,59 +10,144 @@
>   ******************************************************************************/
>
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <ucontext.h>
> +#include <assert.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;
> +
> +    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)];
>  }
>
> -static char *regname[] = {
> -    "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
> -    "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
> -    "CS", "EFL", "UESP", "SS", 0
> +/* 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
>  };
>
> -static void dump_reginfo(struct reginfo *ri)
> +#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(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] : "???",
> -                ri->gregs[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);
>  }


--
Alex Bennée

[Qemu-devel] [RISU v2 05/11] risu_i386: implement missing CPU-specific functions

[Jan Bobek]

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 and opcode "UD1 %xxx,%eax" for triggering RISU actions.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_i386.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/risu_i386.c b/risu_i386.c
index 2d2f325..06d95e5 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -25,12 +25,37 @@ static int insn_is_ud2(uint32_t insn)
 
 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;
+}
+
+void set_ucontext_paramreg(void *vuc, uint64_t value)
+{
+    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;
+    }
+    return -1;
+}
+
+uintptr_t get_pc(struct reginfo *ri)
+{
+    return ri->gregs[REG_E(IP)];
 }
 
 int send_register_info(int sock, void *uc)
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 05/11] risu_i386: implement missing CPU-specific functions

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> 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 and opcode "UD1 %xxx,%eax" for triggering RISU actions.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_i386.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 05/11] risu_i386: implement missing CPU-specific functions

[Alex Bennée]

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

> 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 and opcode "UD1 %xxx,%eax" for triggering RISU actions.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

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

> ---
>  risu_i386.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/risu_i386.c b/risu_i386.c
> index 2d2f325..06d95e5 100644
> --- a/risu_i386.c
> +++ b/risu_i386.c
> @@ -25,12 +25,37 @@ static int insn_is_ud2(uint32_t insn)
>
>  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;
> +}
> +
> +void set_ucontext_paramreg(void *vuc, uint64_t value)
> +{
> +    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;
> +    }
> +    return -1;
> +}
> +
> +uintptr_t get_pc(struct reginfo *ri)
> +{
> +    return ri->gregs[REG_E(IP)];
>  }
>
>  int send_register_info(int sock, void *uc)


--
Alex Bennée

[Qemu-devel] [RISU v2 06/11] risu_i386: remove old unused code

[Jan Bobek]

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.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
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 06d95e5..9962b8f 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;
@@ -57,54 +50,3 @@ uintptr_t get_pc(struct reginfo *ri)
 {
     return ri->gregs[REG_E(IP)];
 }
-
-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

Re: [Qemu-devel] [RISU v2 06/11] risu_i386: remove old unused code

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> 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.
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_i386.c | 58 -----------------------------------------------------
>  1 file changed, 58 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Jan Bobek]

This allows us to drop dependency on NASM and build the test image
with GCC only. Adds support for x86_64, too.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 Makefile    |  3 +++
 test_i386.S | 41 +++++++++++++++++++++++++++++++++++++++++
 test_i386.s | 27 ---------------------------
 3 files changed, 44 insertions(+), 27 deletions(-)
 create mode 100644 test_i386.S
 delete mode 100644 test_i386.s

diff --git a/Makefile b/Makefile
index b362dbe..6ab014a 100644
--- a/Makefile
+++ b/Makefile
@@ -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/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
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> This allows us to drop dependency on NASM and build the test image
> with GCC only. Adds support for x86_64, too.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  Makefile    |  3 +++
>  test_i386.S | 41 +++++++++++++++++++++++++++++++++++++++++
>  test_i386.s | 27 ---------------------------
>  3 files changed, 44 insertions(+), 27 deletions(-)
>  create mode 100644 test_i386.S
>  delete mode 100644 test_i386.s

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Alex Bennée]

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

> This allows us to drop dependency on NASM and build the test image
> with GCC only. Adds support for x86_64, too.

  ./risu --master -t run.out test_i386.bin

and then:

  ./risu -t run.out test_i386.bin

Gives:

  loading test image test_i386.bin...
  starting apprentice image at 0xf7f07000
  starting image
  finished early after 1 checkpoints
  match status...
  mismatch on regs!
  this reginfo:
    faulting insn fc0b90f

Because:

  Mismatch (master v apprentice):
  xmm4  : fe76ea16f7d9c58c 000006fc00000000
       v: fe76ea16f7d1a58c 000006fc00000000

We probably need to zero or reset the xmm regs both in the test and when
risugen dumps it's preamble.

>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  Makefile    |  3 +++
>  test_i386.S | 41 +++++++++++++++++++++++++++++++++++++++++
>  test_i386.s | 27 ---------------------------
>  3 files changed, 44 insertions(+), 27 deletions(-)
>  create mode 100644 test_i386.S
>  delete mode 100644 test_i386.s
>
> diff --git a/Makefile b/Makefile
> index b362dbe..6ab014a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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/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


--
Alex Bennée

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Richard Henderson]

On Mon, May 20, 2019, 08:17 Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Jan Bobek <jan.bobek@gmail.com> writes:
>
> > This allows us to drop dependency on NASM and build the test image
> > with GCC only. Adds support for x86_64, too.
>
>   ./risu --master -t run.out test_i386.bin
>
> and then:
>
>   ./risu -t run.out test_i386.bin
>
> Gives:
>
>   loading test image test_i386.bin...
>   starting apprentice image at 0xf7f07000
>   starting image
>   finished early after 1 checkpoints
>   match status...
>   mismatch on regs!
>   this reginfo:
>     faulting insn fc0b90f
>
> Because:
>
>   Mismatch (master v apprentice):
>   xmm4  : fe76ea16f7d9c58c 000006fc00000000
>        v: fe76ea16f7d1a58c 000006fc00000000
>
> We probably need to zero or reset the xmm regs both in the test and when
> risugen dumps it's preamble.
>

That gets fixed later in the series.

r~

>

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On Mon, May 20, 2019, 08:17 Alex Bennée <alex.bennee@linaro.org> wrote:
>
>>
>> Jan Bobek <jan.bobek@gmail.com> writes:
>>
>> > This allows us to drop dependency on NASM and build the test image
>> > with GCC only. Adds support for x86_64, too.
>>
>>   ./risu --master -t run.out test_i386.bin
>>
>> and then:
>>
>>   ./risu -t run.out test_i386.bin
>>
>> Gives:
>>
>>   loading test image test_i386.bin...
>>   starting apprentice image at 0xf7f07000
>>   starting image
>>   finished early after 1 checkpoints
>>   match status...
>>   mismatch on regs!
>>   this reginfo:
>>     faulting insn fc0b90f
>>
>> Because:
>>
>>   Mismatch (master v apprentice):
>>   xmm4  : fe76ea16f7d9c58c 000006fc00000000
>>        v: fe76ea16f7d1a58c 000006fc00000000
>>
>> We probably need to zero or reset the xmm regs both in the test and when
>> risugen dumps it's preamble.
>>
>
> That gets fixed later in the series.

So it does, but I'm still seeing the test fail when played back :-/

--
Alex Bennée

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Richard Henderson]

On 5/21/19 5:08 AM, Alex Bennée wrote:
>>> We probably need to zero or reset the xmm regs both in the test and when
>>> risugen dumps it's preamble.
>>>
>>
>> That gets fixed later in the series.
> 
> So it does, but I'm still seeing the test fail when played back :-/

Um, no, I mean this test is extended in patch 9, exactly how you suggest.  Are
you trying to run the test as seen in patch 7 against the final series?


r~

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 5/21/19 5:08 AM, Alex Bennée wrote:
>>>> We probably need to zero or reset the xmm regs both in the test and when
>>>> risugen dumps it's preamble.
>>>>
>>>
>>> That gets fixed later in the series.
>>
>> So it does, but I'm still seeing the test fail when played back :-/
>
> Um, no, I mean this test is extended in patch 9, exactly how you suggest.  Are
> you trying to run the test as seen in patch 7 against the final
> series?

Running against:

  commit 555748b35849ad4d354a9a3cd7f8549994b2bea4 (HEAD -> review/i386-support-v2)
  Author: Jan Bobek <jan.bobek@gmail.com>
  Date:   Fri May 17 18:44:50 2019 -0400

      risu_reginfo_i386: accept named feature sets for --xfeature

fails for me.

--
Alex Bennée

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Jan Bobek]

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

On 5/21/19 11:30 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 5/21/19 5:08 AM, Alex Bennée wrote:
>>>>> We probably need to zero or reset the xmm regs both in the test and when
>>>>> risugen dumps it's preamble.
>>>>>
>>>>
>>>> That gets fixed later in the series.
>>>
>>> So it does, but I'm still seeing the test fail when played back :-/
>>
>> Um, no, I mean this test is extended in patch 9, exactly how you suggest.  Are
>> you trying to run the test as seen in patch 7 against the final
>> series?
> 
> Running against:
> 
>   commit 555748b35849ad4d354a9a3cd7f8549994b2bea4 (HEAD -> review/i386-support-v2)
>   Author: Jan Bobek <jan.bobek@gmail.com>
>   Date:   Fri May 17 18:44:50 2019 -0400
> 
>       risu_reginfo_i386: accept named feature sets for --xfeature
> 
> fails for me.

I get the same behavior, but it only occurs on 32bit builds of
RISU. Specifically, in risu_reginfo_i386.c, lines 172--178:

    for (i = 0; i < nvecregs; ++i) {
#ifdef __x86_64__
        memcpy(&ri->vregs[i], &fp->xmm_space[i], 16);
#else
        memcpy(&ri->vregs[i], &fp->_xmm[i * 4], 16);
#endif
    }

In the #else branch, fp->_xmm has type _libc_xmmreg[16], and
_libc_xmmreg itself is a struct with a 4-element array of uint32s. On
my box, this gets fixed by dropping the multiplication from the index,
i.e.

        memcpy(&ri->vregs[i], &fp->_xmm[i], 16);

I wonder why Richard wrote it like this in the first place; did
fp->_xmm use to be an array of uint32s in previous versions of this
API?

-Jan


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

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Richard Henderson]

On 5/21/19 12:48 PM, Jan Bobek wrote:
> I get the same behavior, but it only occurs on 32bit builds of
> RISU. Specifically, in risu_reginfo_i386.c, lines 172--178:
> 
>     for (i = 0; i < nvecregs; ++i) {
> #ifdef __x86_64__
>         memcpy(&ri->vregs[i], &fp->xmm_space[i], 16);
> #else
>         memcpy(&ri->vregs[i], &fp->_xmm[i * 4], 16);
> #endif
>     }
> 
> In the #else branch, fp->_xmm has type _libc_xmmreg[16], and
> _libc_xmmreg itself is a struct with a 4-element array of uint32s. On
> my box, this gets fixed by dropping the multiplication from the index,
> i.e.
> 
>         memcpy(&ri->vregs[i], &fp->_xmm[i], 16);
> 
> I wonder why Richard wrote it like this in the first place; did
> fp->_xmm use to be an array of uint32s in previous versions of this
> API?

I dunno what happened, but these indexes are backward.

>From <asm/sigcontext.h>:

struct _fpstate_32 {
    ...
        struct _xmmreg _xmm[8];


struct _fpstate_64 {
    ...
        __u32 xmm_space[64];  /* 16x XMM registers, 16 bytes each */



r~

Re: [Qemu-devel] [RISU v2 07/11] test_i386: change syntax from nasm to gas

[Jan Bobek]

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

On 5/21/19 12:56 PM, Richard Henderson wrote:
> On 5/21/19 12:48 PM, Jan Bobek wrote:
>> I get the same behavior, but it only occurs on 32bit builds of
>> RISU. Specifically, in risu_reginfo_i386.c, lines 172--178:
>>
>>     for (i = 0; i < nvecregs; ++i) {
>> #ifdef __x86_64__
>>         memcpy(&ri->vregs[i], &fp->xmm_space[i], 16);
>> #else
>>         memcpy(&ri->vregs[i], &fp->_xmm[i * 4], 16);
>> #endif
>>     }
>>
>> In the #else branch, fp->_xmm has type _libc_xmmreg[16], and
>> _libc_xmmreg itself is a struct with a 4-element array of uint32s. On
>> my box, this gets fixed by dropping the multiplication from the index,
>> i.e.
>>
>>         memcpy(&ri->vregs[i], &fp->_xmm[i], 16);
>>
>> I wonder why Richard wrote it like this in the first place; did
>> fp->_xmm use to be an array of uint32s in previous versions of this
>> API?
> 
> I dunno what happened, but these indexes are backward.
> 
>>From <asm/sigcontext.h>:
> 
> struct _fpstate_32 {
>     ...
>         struct _xmmreg _xmm[8];
> 
> 
> struct _fpstate_64 {
>     ...
>         __u32 xmm_space[64];  /* 16x XMM registers, 16 bytes each */

Indeed; that makes for one more fix in v3.

-Jan


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

[Qemu-devel] [RISU v2 08/11] configure: add i386/x86_64 architectures

[Jan Bobek]

Now that i386 and x86_64 architectures are supported by RISU, we want
to detect them and build RISU for them automatically.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 configure | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 08/11] configure: add i386/x86_64 architectures

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> Now that i386 and x86_64 architectures are supported by RISU, we want
> to detect them and build RISU for them automatically.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  configure | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 08/11] configure: add i386/x86_64 architectures

[Alex Bennée]

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

> Now that i386 and x86_64 architectures are supported by RISU, we want
> to detect them and build RISU for them automatically.
>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>

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

> ---
>  configure | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> 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


--
Alex Bennée

[Qemu-devel] [RISU v2 09/11] i386: Add avx512 state to reginfo_t

[Jan Bobek]

From: Richard Henderson <richard.henderson@linaro.org>

The state expected for a given test must be specifically requested
with the --xfeatures=mask command-line argument.  This is recorded
with the saved state so that it is obvious if the apprentice is given
a different argument.  Any features beyond what are present on the
running cpu will read as zero.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu_reginfo_i386.h |  14 +++
 risu_reginfo_i386.c | 228 ++++++++++++++++++++++++++++++++++++++++++--
 test_i386.S         |  39 ++++++++
 3 files changed, 273 insertions(+), 8 deletions(-)

diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index e350f01..b468f79 100644
--- a/risu_reginfo_i386.h
+++ b/risu_reginfo_i386.h
@@ -12,6 +12,10 @@
 #ifndef RISU_REGINFO_I386_H
 #define RISU_REGINFO_I386_H
 
+struct avx512_reg {
+    uint64_t q[8];
+};
+
 /*
  * This is the data structure we pass over the socket.
  * It is a simplified and reduced subset of what can
@@ -19,7 +23,17 @@
  */
 struct reginfo {
     uint32_t faulting_insn;
+    uint32_t mxcsr;
+    uint64_t xfeatures;
+
     gregset_t gregs;
+
+#ifdef __x86_64__
+    struct avx512_reg vregs[32];
+#else
+    struct avx512_reg vregs[8];
+#endif
+    uint64_t kregs[8];
 };
 
 /*
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index c4dc14a..35ff7c8 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -11,19 +11,32 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stddef.h>
 #include <string.h>
 #include <ucontext.h>
 #include <assert.h>
+#include <cpuid.h>
 
 #include "risu.h"
 #include "risu_reginfo_i386.h"
 
-const struct option * const arch_long_opts;
-const char * const arch_extra_help;
+#include <asm/sigcontext.h>
+
+static uint64_t xfeatures = 3;  /* SSE */
+
+static const struct option extra_ops[] = {
+    {"xfeatures", required_argument, NULL, FIRST_ARCH_OPT },
+    {0, 0, 0, 0}
+};
+
+const struct option * const arch_long_opts = extra_ops;
+const char * const arch_extra_help
+    = "  --xfeatures=<mask>  Use features in mask for XSAVE\n";
 
 void process_arch_opt(int opt, const char *arg)
 {
-    abort();
+    assert(opt == FIRST_ARCH_OPT);
+    xfeatures = strtoull(arg, 0, 0);
 }
 
 const int reginfo_size(void)
@@ -31,13 +44,37 @@ const int reginfo_size(void)
     return sizeof(struct reginfo);
 }
 
+static void *xsave_feature_buf(struct _xstate *xs, int feature)
+{
+    unsigned int eax, ebx, ecx, edx;
+    int ok;
+
+    /*
+     * Get the location of the XSAVE feature from the cpuid leaf.
+     * Given that we know the xfeature bit is set, this must succeed.
+     */
+    ok = __get_cpuid_count(0xd, feature, &eax, &ebx, &ecx, &edx);
+    assert(ok);
+
+    /* Sanity check that the frame stored by the kernel contains the data. */
+    assert(xs->fpstate.sw_reserved.extended_size >= eax + ebx);
+
+    return (void *)xs + ebx;
+}
+
 /* reginfo_init: initialize with a ucontext */
 void reginfo_init(struct reginfo *ri, ucontext_t *uc)
 {
-    int i;
+    int i, nvecregs;
+    struct _fpstate *fp;
+    struct _xstate *xs;
+    uint64_t features;
 
     memset(ri, 0, sizeof(*ri));
 
+    /* Require master and apprentice to be given the same arguments.  */
+    ri->xfeatures = xfeatures;
+
     for (i = 0; i < NGREG; i++) {
         switch (i) {
         case REG_E(IP):
@@ -79,12 +116,89 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
      * distinguish 'do compare' from 'stop'.
      */
     ri->faulting_insn = *(uint32_t *)uc->uc_mcontext.gregs[REG_E(IP)];
+
+    /*
+     * FP state is omitted if unused (aka in init state).
+     * Use the <asm/sigcontext.h> struct for access to AVX state.
+     */
+
+    fp = (struct _fpstate *)uc->uc_mcontext.fpregs;
+    if (fp == NULL) {
+        return;
+    }
+
+#ifdef __x86_64__
+    nvecregs = 16;
+#else
+    /* We don't (currently) care about the 80387 state, only SSE+.  */
+    if (fp->magic != X86_FXSR_MAGIC) {
+        return;
+    }
+    nvecregs = 8;
+#endif
+
+    /*
+     * Now we know that _fpstate contains FXSAVE data.
+     */
+    ri->mxcsr = fp->mxcsr;
+
+    for (i = 0; i < nvecregs; ++i) {
+#ifdef __x86_64__
+        memcpy(&ri->vregs[i], &fp->xmm_space[i], 16);
+#else
+        memcpy(&ri->vregs[i], &fp->_xmm[i * 4], 16);
+#endif
+    }
+
+    if (fp->sw_reserved.magic1 != FP_XSTATE_MAGIC1) {
+        return;
+    }
+    xs = (struct _xstate *)fp;
+    features = xfeatures & xs->xstate_hdr.xfeatures;
+
+    /*
+     * Now we know that _fpstate contains XSAVE data.
+     */
+
+    if (features & (1 << 2)) {
+        /* YMM_Hi128 state */
+        void *buf = xsave_feature_buf(xs, 2);
+        for (i = 0; i < nvecregs; ++i) {
+            memcpy(&ri->vregs[i].q[2], buf + 16 * i, 16);
+        }
+    }
+
+    if (features & (1 << 5)) {
+        /* Opmask state */
+        uint64_t *buf = xsave_feature_buf(xs, 5);
+        for (i = 0; i < 8; ++i) {
+            ri->kregs[i] = buf[i];
+        }
+    }
+
+    if (features & (1 << 6)) {
+        /* ZMM_Hi256 state */
+        void *buf = xsave_feature_buf(xs, 6);
+        for (i = 0; i < nvecregs; ++i) {
+            memcpy(&ri->vregs[i].q[4], buf + 32 * i, 32);
+        }
+    }
+
+#ifdef __x86_64__
+    if (features & (1 << 7)) {
+        /* Hi16_ZMM state */
+        void *buf = xsave_feature_buf(xs, 7);
+        for (i = 0; i < 16; ++i) {
+            memcpy(&ri->vregs[i + 16], buf + 64 * i, 64);
+        }
+    }
+#endif
 }
 
 /* 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));
+    return !memcmp(m, a, sizeof(*m));
 }
 
 static const char *const regname[NGREG] = {
@@ -126,28 +240,126 @@ static const char *const regname[NGREG] = {
 # define PRIxREG   "%08x"
 #endif
 
+static int get_nvecregs(uint64_t features)
+{
+#ifdef __x86_64__
+    return features & (1 << 7) ? 32 : 16;
+#else
+    return 8;
+#endif
+}
+
+static int get_nvecquads(uint64_t features)
+{
+    if (features & (1 << 6)) {
+        return 8;
+    } else if (features & (1 << 2)) {
+        return 4;
+    } else {
+        return 2;
+    }
+}
+
+static char get_vecletter(uint64_t features)
+{
+    if (features & (1 << 6 | 1 << 7)) {
+        return 'z';
+    } else if (features & (1 << 2)) {
+        return 'y';
+    } else {
+        return 'x';
+    }
+}
+
 /* reginfo_dump: print state to a stream, returns nonzero on success */
 int reginfo_dump(struct reginfo *ri, FILE *f)
 {
-    int i;
+    uint64_t features;
+    int i, j, n, w;
+    char r;
+
     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]);
         }
     }
+
+    fprintf(f, "  mxcsr : %x\n", ri->mxcsr);
+    fprintf(f, "  xfeat : %" PRIx64 "\n", ri->xfeatures);
+
+    features = ri->xfeatures;
+    n = get_nvecregs(features);
+    w = get_nvecquads(features);
+    r = get_vecletter(features);
+
+    for (i = 0; i < n; i++) {
+        fprintf(f, "  %cmm%-3d: ", r, i);
+        for (j = w - 1; j >= 0; j--) {
+            fprintf(f, "%016" PRIx64 "%c",
+                    ri->vregs[i].q[j], j == 0 ? '\n' : ' ');
+        }
+    }
+
+    if (features & (1 << 5)) {
+        for (i = 0; i < 8; i++) {
+            fprintf(f, "  k%-5d: %016" PRIx64 "\n", i, ri->kregs[i]);
+        }
+    }
+
     return !ferror(f);
 }
 
 int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f)
 {
-    int i;
+    int i, j, n, w;
+    uint64_t features;
+    char r;
+
+    fprintf(f, "Mismatch (master v apprentice):\n");
+
     for (i = 0; i < NGREG; i++) {
         if (m->gregs[i] != a->gregs[i]) {
             assert(regname[i]);
-            fprintf(f, "Mismatch: %s: " PRIxREG " v " PRIxREG "\n",
+            fprintf(f, "  %-6s: " PRIxREG " v " PRIxREG "\n",
                     regname[i], m->gregs[i], a->gregs[i]);
         }
     }
+
+    if (m->mxcsr != a->mxcsr) {
+        fprintf(f, "  mxcsr : %x v %x\n", m->mxcsr, a->mxcsr);
+    }
+    if (m->xfeatures != a->xfeatures) {
+        fprintf(f, "  xfeat : %" PRIx64 " v %" PRIx64 "\n",
+                m->xfeatures, a->xfeatures);
+    }
+
+    features = m->xfeatures;
+    n = get_nvecregs(features);
+    w = get_nvecquads(features);
+    r = get_vecletter(features);
+
+    for (i = 0; i < n; i++) {
+        if (memcmp(&m->vregs[i], &a->vregs[i], w * 8)) {
+            fprintf(f, "  %cmm%-3d: ", r, i);
+            for (j = w - 1; j >= 0; j--) {
+                fprintf(f, "%016" PRIx64 "%c",
+                        m->vregs[i].q[j], j == 0 ? '\n' : ' ');
+            }
+            fprintf(f, "       v: ");
+            for (j = w - 1; j >= 0; j--) {
+                fprintf(f, "%016" PRIx64 "%c",
+                        a->vregs[i].q[j], j == 0 ? '\n' : ' ');
+            }
+        }
+    }
+
+    for (i = 0; i < 8; i++) {
+        if (m->kregs[i] != a->kregs[i]) {
+            fprintf(f, "  k%-5d: %016" PRIx64 " v %016" PRIx64 "\n",
+                    i, m->kregs[i], a->kregs[i]);
+        }
+    }
+
     return !ferror(f);
 }
diff --git a/test_i386.S b/test_i386.S
index 456b99c..05344d7 100644
--- a/test_i386.S
+++ b/test_i386.S
@@ -12,6 +12,37 @@
 /* A trivial test image for x86 */
 
 /* Initialise the registers to avoid spurious mismatches */
+
+#ifdef __x86_64__
+#define BASE	%rax
+	lea	2f(%rip), BASE
+#else
+#define BASE	%eax
+	call	1f
+1:	pop	BASE
+	add	$2f-1b, BASE
+#endif
+
+	movdqa	0(BASE), %xmm0
+	movdqa	1*16(BASE), %xmm1
+	movdqa	2*16(BASE), %xmm2
+	movdqa	3*16(BASE), %xmm3
+	movdqa	4*16(BASE), %xmm4
+	movdqa	5*16(BASE), %xmm5
+	movdqa	6*16(BASE), %xmm6
+	movdqa	7*16(BASE), %xmm7
+
+#ifdef __x86_64__
+	movdqa	8*16(BASE), %xmm8
+	movdqa	9*16(BASE), %xmm9
+	movdqa	10*16(BASE), %xmm10
+	movdqa	11*16(BASE), %xmm11
+	movdqa	12*16(BASE), %xmm12
+	movdqa	13*16(BASE), %xmm13
+	movdqa	14*16(BASE), %xmm14
+	movdqa	15*16(BASE), %xmm15
+#endif
+
 	xor	%eax, %eax
 	sahf				/* init eflags */
 
@@ -39,3 +70,11 @@
 
 /* exit test */
 	ud1	%ecx, %eax
+
+	.p2align 16
+2:
+	.set	i, 0
+	.rept	256
+	.byte	i
+	.set	i, i + 1
+	.endr
-- 
2.20.1

[Qemu-devel] [RISU v2 10/11] risu_reginfo_i386: replace xfeature constants with symbolic names

[Jan Bobek]

The original code used "magic numbers", which made it unclear in
some places. Include a reference to the Intel manual where the
constants' meaning is discussed.

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

diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index 35ff7c8..aba5ae3 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -22,7 +22,25 @@
 
 #include <asm/sigcontext.h>
 
-static uint64_t xfeatures = 3;  /* SSE */
+/*
+ * Refer to "Intel(R) 64 and IA-32 Architectures Software Developer's
+ * Manual", Volume 1, Section 13.1 "XSAVE-Supported Features and
+ * State-Component Bitmaps" for detailed discussion of these constants
+ * and their meaning.
+ */
+enum {
+    XFEAT_X87              = 1 << 0,
+    XFEAT_SSE              = 1 << 1,
+    XFEAT_AVX              = 1 << 2,
+    XFEAT_AVX512_OPMASK    = 1 << 5,
+    XFEAT_AVX512_ZMM_HI256 = 1 << 6,
+    XFEAT_AVX512_HI16_ZMM  = 1 << 7,
+    XFEAT_AVX512           = XFEAT_AVX512_OPMASK
+                           | XFEAT_AVX512_ZMM_HI256
+                           | XFEAT_AVX512_HI16_ZMM
+};
+
+static uint64_t xfeatures = XFEAT_X87 | XFEAT_SSE;
 
 static const struct option extra_ops[] = {
     {"xfeatures", required_argument, NULL, FIRST_ARCH_OPT },
@@ -160,34 +178,34 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
      * Now we know that _fpstate contains XSAVE data.
      */
 
-    if (features & (1 << 2)) {
+    if (features & XFEAT_AVX) {
         /* YMM_Hi128 state */
-        void *buf = xsave_feature_buf(xs, 2);
+        void *buf = xsave_feature_buf(xs, XFEAT_AVX);
         for (i = 0; i < nvecregs; ++i) {
             memcpy(&ri->vregs[i].q[2], buf + 16 * i, 16);
         }
     }
 
-    if (features & (1 << 5)) {
+    if (features & XFEAT_AVX512_OPMASK) {
         /* Opmask state */
-        uint64_t *buf = xsave_feature_buf(xs, 5);
+        uint64_t *buf = xsave_feature_buf(xs, XFEAT_AVX512_OPMASK);
         for (i = 0; i < 8; ++i) {
             ri->kregs[i] = buf[i];
         }
     }
 
-    if (features & (1 << 6)) {
+    if (features & XFEAT_AVX512_ZMM_HI256) {
         /* ZMM_Hi256 state */
-        void *buf = xsave_feature_buf(xs, 6);
+        void *buf = xsave_feature_buf(xs, XFEAT_AVX512_ZMM_HI256);
         for (i = 0; i < nvecregs; ++i) {
             memcpy(&ri->vregs[i].q[4], buf + 32 * i, 32);
         }
     }
 
 #ifdef __x86_64__
-    if (features & (1 << 7)) {
+    if (features & XFEAT_AVX512_HI16_ZMM) {
         /* Hi16_ZMM state */
-        void *buf = xsave_feature_buf(xs, 7);
+        void *buf = xsave_feature_buf(xs, XFEAT_AVX512_HI16_ZMM);
         for (i = 0; i < 16; ++i) {
             memcpy(&ri->vregs[i + 16], buf + 64 * i, 64);
         }
@@ -243,7 +261,7 @@ static const char *const regname[NGREG] = {
 static int get_nvecregs(uint64_t features)
 {
 #ifdef __x86_64__
-    return features & (1 << 7) ? 32 : 16;
+    return features & XFEAT_AVX512_HI16_ZMM ? 32 : 16;
 #else
     return 8;
 #endif
@@ -251,9 +269,9 @@ static int get_nvecregs(uint64_t features)
 
 static int get_nvecquads(uint64_t features)
 {
-    if (features & (1 << 6)) {
+    if (features & XFEAT_AVX512_ZMM_HI256) {
         return 8;
-    } else if (features & (1 << 2)) {
+    } else if (features & XFEAT_AVX) {
         return 4;
     } else {
         return 2;
@@ -262,9 +280,9 @@ static int get_nvecquads(uint64_t features)
 
 static char get_vecletter(uint64_t features)
 {
-    if (features & (1 << 6 | 1 << 7)) {
+    if (features & (XFEAT_AVX512_ZMM_HI256 | XFEAT_AVX512_HI16_ZMM)) {
         return 'z';
-    } else if (features & (1 << 2)) {
+    } else if (features & XFEAT_AVX) {
         return 'y';
     } else {
         return 'x';
@@ -301,7 +319,7 @@ int reginfo_dump(struct reginfo *ri, FILE *f)
         }
     }
 
-    if (features & (1 << 5)) {
+    if (features & XFEAT_AVX512_OPMASK) {
         for (i = 0; i < 8; i++) {
             fprintf(f, "  k%-5d: %016" PRIx64 "\n", i, ri->kregs[i]);
         }
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 10/11] risu_reginfo_i386: replace xfeature constants with symbolic names

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> The original code used "magic numbers", which made it unclear in
> some places. Include a reference to the Intel manual where the
> constants' meaning is discussed.
> 
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_reginfo_i386.c | 48 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 15 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 10/11] risu_reginfo_i386: replace xfeature constants with symbolic names

[Alex Bennée]

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

> The original code used "magic numbers", which made it unclear in
> some places. Include a reference to the Intel manual where the
> constants' meaning is discussed.
>
> 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, 33 insertions(+), 15 deletions(-)
>
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> index 35ff7c8..aba5ae3 100644
> --- a/risu_reginfo_i386.c
> +++ b/risu_reginfo_i386.c
> @@ -22,7 +22,25 @@
>
>  #include <asm/sigcontext.h>
>
> -static uint64_t xfeatures = 3;  /* SSE */
> +/*
> + * Refer to "Intel(R) 64 and IA-32 Architectures Software Developer's
> + * Manual", Volume 1, Section 13.1 "XSAVE-Supported Features and
> + * State-Component Bitmaps" for detailed discussion of these constants
> + * and their meaning.
> + */
> +enum {
> +    XFEAT_X87              = 1 << 0,
> +    XFEAT_SSE              = 1 << 1,
> +    XFEAT_AVX              = 1 << 2,
> +    XFEAT_AVX512_OPMASK    = 1 << 5,
> +    XFEAT_AVX512_ZMM_HI256 = 1 << 6,
> +    XFEAT_AVX512_HI16_ZMM  = 1 << 7,
> +    XFEAT_AVX512           = XFEAT_AVX512_OPMASK
> +                           | XFEAT_AVX512_ZMM_HI256
> +                           | XFEAT_AVX512_HI16_ZMM
> +};
> +
> +static uint64_t xfeatures = XFEAT_X87 | XFEAT_SSE;
>
>  static const struct option extra_ops[] = {
>      {"xfeatures", required_argument, NULL, FIRST_ARCH_OPT },
> @@ -160,34 +178,34 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
>       * Now we know that _fpstate contains XSAVE data.
>       */
>
> -    if (features & (1 << 2)) {
> +    if (features & XFEAT_AVX) {
>          /* YMM_Hi128 state */
> -        void *buf = xsave_feature_buf(xs, 2);
> +        void *buf = xsave_feature_buf(xs, XFEAT_AVX);
>          for (i = 0; i < nvecregs; ++i) {
>              memcpy(&ri->vregs[i].q[2], buf + 16 * i, 16);
>          }
>      }
>
> -    if (features & (1 << 5)) {
> +    if (features & XFEAT_AVX512_OPMASK) {
>          /* Opmask state */
> -        uint64_t *buf = xsave_feature_buf(xs, 5);
> +        uint64_t *buf = xsave_feature_buf(xs, XFEAT_AVX512_OPMASK);
>          for (i = 0; i < 8; ++i) {
>              ri->kregs[i] = buf[i];
>          }
>      }
>
> -    if (features & (1 << 6)) {
> +    if (features & XFEAT_AVX512_ZMM_HI256) {
>          /* ZMM_Hi256 state */
> -        void *buf = xsave_feature_buf(xs, 6);
> +        void *buf = xsave_feature_buf(xs, XFEAT_AVX512_ZMM_HI256);
>          for (i = 0; i < nvecregs; ++i) {
>              memcpy(&ri->vregs[i].q[4], buf + 32 * i, 32);
>          }
>      }
>
>  #ifdef __x86_64__
> -    if (features & (1 << 7)) {
> +    if (features & XFEAT_AVX512_HI16_ZMM) {
>          /* Hi16_ZMM state */
> -        void *buf = xsave_feature_buf(xs, 7);
> +        void *buf = xsave_feature_buf(xs, XFEAT_AVX512_HI16_ZMM);
>          for (i = 0; i < 16; ++i) {
>              memcpy(&ri->vregs[i + 16], buf + 64 * i, 64);
>          }
> @@ -243,7 +261,7 @@ static const char *const regname[NGREG] = {
>  static int get_nvecregs(uint64_t features)
>  {
>  #ifdef __x86_64__
> -    return features & (1 << 7) ? 32 : 16;
> +    return features & XFEAT_AVX512_HI16_ZMM ? 32 : 16;
>  #else
>      return 8;
>  #endif
> @@ -251,9 +269,9 @@ static int get_nvecregs(uint64_t features)
>
>  static int get_nvecquads(uint64_t features)
>  {
> -    if (features & (1 << 6)) {
> +    if (features & XFEAT_AVX512_ZMM_HI256) {
>          return 8;
> -    } else if (features & (1 << 2)) {
> +    } else if (features & XFEAT_AVX) {
>          return 4;
>      } else {
>          return 2;
> @@ -262,9 +280,9 @@ static int get_nvecquads(uint64_t features)
>
>  static char get_vecletter(uint64_t features)
>  {
> -    if (features & (1 << 6 | 1 << 7)) {
> +    if (features & (XFEAT_AVX512_ZMM_HI256 | XFEAT_AVX512_HI16_ZMM)) {
>          return 'z';
> -    } else if (features & (1 << 2)) {
> +    } else if (features & XFEAT_AVX) {
>          return 'y';
>      } else {
>          return 'x';
> @@ -301,7 +319,7 @@ int reginfo_dump(struct reginfo *ri, FILE *f)
>          }
>      }
>
> -    if (features & (1 << 5)) {
> +    if (features & XFEAT_AVX512_OPMASK) {
>          for (i = 0; i < 8; i++) {
>              fprintf(f, "  k%-5d: %016" PRIx64 "\n", i, ri->kregs[i]);
>          }


--
Alex Bennée

[Qemu-devel] [RISU v2 11/11] risu_reginfo_i386: accept named feature sets for --xfeature

[Jan Bobek]

Have the --xfeature option accept "sse", "avx" and "avx512" in
addition to a plain numerical value, purely for users' convenience.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
---
 risu_reginfo_i386.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index aba5ae3..c15fe63 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -54,7 +54,16 @@ const char * const arch_extra_help
 void process_arch_opt(int opt, const char *arg)
 {
     assert(opt == FIRST_ARCH_OPT);
-    xfeatures = strtoull(arg, 0, 0);
+
+    if (!strcmp(arg, "sse")) {
+        xfeatures = XFEAT_X87 | XFEAT_SSE;
+    } else if (!strcmp(arg, "avx")) {
+        xfeatures = XFEAT_X87 | XFEAT_SSE | XFEAT_AVX;
+    } else if (!strcmp(arg, "avx512")) {
+        xfeatures = XFEAT_X87 | XFEAT_SSE | XFEAT_AVX | XFEAT_AVX512;
+    } else {
+        xfeatures = strtoull(arg, 0, 0);
+    }
 }
 
 const int reginfo_size(void)
-- 
2.20.1

Re: [Qemu-devel] [RISU v2 11/11] risu_reginfo_i386: accept named feature sets for --xfeature

[Richard Henderson]

On 5/17/19 3:44 PM, Jan Bobek wrote:
> Have the --xfeature option accept "sse", "avx" and "avx512" in
> addition to a plain numerical value, purely for users' convenience.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Jan Bobek <jan.bobek@gmail.com>
> ---
>  risu_reginfo_i386.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Alex Bennée]

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

> This patch series adds support for i386 and x86_64 architectures to
> RISU.  Notably, vector registers (SSE, AVX, AVX-512) are supported for
> verification of the apprentice. This is V2 of the series posted in
> [1].
>
> I decided not to drop the register definitions from the second patch
> as suggested by Alex Bennée [4], but replaced them in the fourth patch
> instead. This keeps the second and third patches code-motion only.
>
> I wasn't 100% sure how to acknowledge Richard's contributions in some
> of the patches, and eventually decided to include a Suggested-by:
> line. Let me know if that's (not) acceptable.

Suggested-by: is a common tag for this sort of thing ;-)

--
Alex Bennée

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Alex Bennée]

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

> This patch series adds support for i386 and x86_64 architectures to
> RISU.  Notably, vector registers (SSE, AVX, AVX-512) are supported for
> verification of the apprentice. This is V2 of the series posted in
> [1].
>
> I decided not to drop the register definitions from the second patch
> as suggested by Alex Bennée [4], but replaced them in the fourth patch
> instead. This keeps the second and third patches code-motion only.
>
> I wasn't 100% sure how to acknowledge Richard's contributions in some
> of the patches, and eventually decided to include a Suggested-by:
> line. Let me know if that's (not) acceptable.
>
>  -Jan
>
> Changes in V2:
>   - included Richard Henderson's fix-ups [2] and vector register
>     support [3]
>   - replaced the magic numbers for XSAVE feature sets with symbolic
>     constants
>   - symbolic names ("sse", "avx", "avx512") as well as numbers are
>     accepted for the parameter --xfeatures

I'm not sure where my test went wrong but I guess it's around xfeatures.
The code says required argument but risu doesn't seem to stop me not
specifying it. I suspect we should default to the most minimal x86_64 we
can and explicitly enable extra features.

Storing xfeat in the stream is a good idea so people don't mix up their
dumps but we probably need more validation when running in master mode
that the feature you have enabled is actually available on the
processor. Otherwise you'll potentially end up generating test streams
on HW with no support and just get a bunch of undef noise ;-)

However the series is looking pretty good so far. Looking forward to the
next iteration.

--
Alex Bennée

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Jan Bobek]

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

On 5/20/19 8:30 AM, Alex Bennée wrote:
> 
> I'm not sure where my test went wrong but I guess it's around xfeatures.
> The code says required argument but risu doesn't seem to stop me not
> specifying it. I suspect we should default to the most minimal x86_64 we
> can and explicitly enable extra features.

The argument is indeed required, that's taken care of by getopt: to
test that, one can simply specify --xfeatures as the last option on
the command-line. However, we don't check if the value successfully
parses into an integer; is it at all possible that --xfeatures
inadvertently swallowed the next part of your command-line? I shall
add this check in v3.

In any case, we currently default to SSE; this seems reasonable given
that it's an extension dating back some 20 years and pre-dates x86_64
by 4 years (1999 vs. 2003). Opinions?

> Storing xfeat in the stream is a good idea so people don't mix up their
> dumps but we probably need more validation when running in master mode
> that the feature you have enabled is actually available on the
> processor. Otherwise you'll potentially end up generating test streams
> on HW with no support and just get a bunch of undef noise ;-)

Correct me if I'm mistaken, but I believe this should be enforced by
xsave_feature_buf. There's a call to __get_cpuid_count to retrieve
location of a given XSAVE feature in memory, which is asserted to
complete successfully. I assume if the feature were not present, the
assertion would fail. I guess there's a point to be made about
release builds, in which the assert may have been optimized out; shall
I turn it into an error message instead?

> However the series is looking pretty good so far. Looking forward to the
> next iteration.

Once again, thanks a lot for the review, Alex!

-Jan


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

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Richard Henderson]

On 5/21/19 11:28 AM, Jan Bobek wrote:
> On 5/20/19 8:30 AM, Alex Bennée wrote:
>>
>> I'm not sure where my test went wrong but I guess it's around xfeatures.
>> The code says required argument but risu doesn't seem to stop me not
>> specifying it. I suspect we should default to the most minimal x86_64 we
>> can and explicitly enable extra features.
> 
> The argument is indeed required, that's taken care of by getopt: to
> test that, one can simply specify --xfeatures as the last option on
> the command-line. However, we don't check if the value successfully
> parses into an integer; is it at all possible that --xfeatures
> inadvertently swallowed the next part of your command-line? I shall
> add this check in v3.
> 
> In any case, we currently default to SSE; this seems reasonable given
> that it's an extension dating back some 20 years and pre-dates x86_64
> by 4 years (1999 vs. 2003). Opinions?

SSE2 is a mandatory part of the x86_64 ABI.

I sincerely doubt we care about testing 32-bit that does not have SSE, but even
then this patch set will not fail, as the kernel will not include the SSE
registers into the signal frame.  It would be the actual test cases for SSE
instructions that would SIGILL when run on a 32-bit guest w/o SSE.

>> Storing xfeat in the stream is a good idea so people don't mix up their
>> dumps but we probably need more validation when running in master mode
>> that the feature you have enabled is actually available on the
>> processor. Otherwise you'll potentially end up generating test streams
>> on HW with no support and just get a bunch of undef noise ;-)
> 
> Correct me if I'm mistaken, but I believe this should be enforced by
> xsave_feature_buf. There's a call to __get_cpuid_count to retrieve
> location of a given XSAVE feature in memory, which is asserted to
> complete successfully. I assume if the feature were not present, the
> assertion would fail. I guess there's a point to be made about
> release builds, in which the assert may have been optimized out; shall
> I turn it into an error message instead?

No, the assert is really an assert, because we have also masked the --xfeatures
value against the set of features stored in the signal frame.  If the kernel
reports a feature in the signal frame for which there is no cpuid leaf, then
something is very confused somewhere.

I am not sure that we can validate that the features in the signal frame match
the --xfeatures value, because I *think* that features are omitted from the
XSAVE data structure when they are in init state.  E.g. when we have not yet
exercised the feature.

This caveat is definitely true of ARM SVE, and I found that if I asserted that
all of the SVE state was in the signal frame that the generated RISU test which
uses memory would fail the 1st checkpoint, because no SVE instructions had yet
been executed.

A careful reading of the XSAVE documentation, plus some experimentation, is
definitely required.  Maybe hand-craft a test case using XRSTOR, giving it a
save area that specifies all features to be reset to init state.


r~

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Jan Bobek]

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

On 5/21/19 12:49 PM, Richard Henderson wrote:
> SSE2 is a mandatory part of the x86_64 ABI.
> 
> I sincerely doubt we care about testing 32-bit that does not have SSE, but even
> then this patch set will not fail, as the kernel will not include the SSE
> registers into the signal frame.  It would be the actual test cases for SSE
> instructions that would SIGILL when run on a 32-bit guest w/o SSE.

Fair enough. I had no idea that SSE2 is mandatory in x86_64, that's
good to know. Let's keep the SSE default then.

> No, the assert is really an assert, because we have also masked the --xfeatures
> value against the set of features stored in the signal frame.  If the kernel
> reports a feature in the signal frame for which there is no cpuid leaf, then
> something is very confused somewhere.

Ah, I see. Makes complete sense.

> I am not sure that we can validate that the features in the signal frame match
> the --xfeatures value, because I *think* that features are omitted from the
> XSAVE data structure when they are in init state.  E.g. when we have not yet
> exercised the feature.
> 
> This caveat is definitely true of ARM SVE, and I found that if I asserted that
> all of the SVE state was in the signal frame that the generated RISU test which
> uses memory would fail the 1st checkpoint, because no SVE instructions had yet
> been executed.
> 
> A careful reading of the XSAVE documentation, plus some experimentation, is
> definitely required.  Maybe hand-craft a test case using XRSTOR, giving it a
> save area that specifies all features to be reset to init state.

tl;dr Richard is exactly right; a component may be missing from the
XSAVE region if it's in the "initial configuration." I'd just leave it
as it is now: it appears everything more advanced than the SSE state
is in the initial configuration when the test image starts
executing. It won't show up until there's instructions which actually
touch it, but by then we get a SIGILL if the HW doesn't support it.

Long story: The Intel manual definitely has a notion of "init
optimization," in addition to "modified optimization." Vol. 1, Section
13.6 "Processor Tracking of XSAVE Managed State" says:

  The XSAVEOPT, XSAVEC, and XSAVES instructions use two optimizations
  to reduce the amount of data that they write to memory. They avoid
  writing data for any state component known to be in its initial
  configuration (the init optimization). In addition, if either
  XSAVEOPT or XSAVES is using the same XSAVE area as that used by the
  most recent execution of XRSTOR or XRSTORS, it may avoid writing
  data for any state component whose configuration is known not to
  have been modified since then (the modified optimization). (XSAVE
  does not use these optimizations, and XSAVEC does not use the
  modified optimization.)

So, XSAVE does not use any optimizations, whereas all other XSAVE
variants use at least the init optimization. Furthermore,

  The following notation describes the state of the init and modified
  optimizations:

  * XINUSE denotes the state-component bitmap corresponding to the
    init optimization. If XINUSE[i] = 0, state component i is known to
    be in its initial configuration; otherwise XINUSE[i] = 1. It is
    possible for XINUSE[i] to be 1 even when state component i is in
    its initial configuration. On a processor that does not support
    the init optimization, XINUSE[i] is always 1 for every value of i.

  [...]

The processor does not need to detect "return" to the initial
configuration; this makes more sense once it's clear what the initial
configuration is:

  The following items specify the initial configuration each state
  component (for the purposes of defining the XINUSE bitmap):

  * SSE state. In 64-bit mode, SSE state is in its initial
    configuration if each of XMM0–XMM15 is 0. Outside 64-bit mode, SSE
    state is in its initial configuration if each of XMM0–XMM7 is
    0. [...]

  * AVX state. In 64-bit mode, AVX state is in its initial
    configuration if each of YMM0_H–YMM15_H is 0. Outside 64-bit mode,
    AVX state is in its initial configuration if each of YMM0_H–YMM7_H
    is 0. [...]

  [...]

No surprise here; the initial configuration is just all zeros.

I ran some experiments on my laptop's Intel(R) Core(TM) i5-4210U to
find out what actually happens. This CPU supports AVX (but not
AVX-512) and doesn't support XSAVEC, so I only looked at XSAVE and
XSAVEOPT (XSAVES is kernel-mode only). I found out that:

1. both XSAVE and XSAVEOPT do not include the AVX state if it is in
   the initial configuration (not only XSAVEOPT),
2. return to initial configuration is not detected, i.e. the AVX state
   is included even though it has been set to all zeros via vxorps,
3. the AVX state can be brought back to the initial configuration via
   XRSTOR with the AVX component removed.

Cheers,
-Jan


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

Re: [Qemu-devel] [RISU v2 00/11] Support for i386/x86_64 with vector extensions

[Richard Henderson]

On 5/23/19 2:03 PM, Jan Bobek wrote:
> I ran some experiments on my laptop's Intel(R) Core(TM) i5-4210U to
> find out what actually happens. This CPU supports AVX (but not
> AVX-512) and doesn't support XSAVEC, so I only looked at XSAVE and
> XSAVEOPT (XSAVES is kernel-mode only). I found out that:
> 
> 1. both XSAVE and XSAVEOPT do not include the AVX state if it is in
>    the initial configuration (not only XSAVEOPT),
> 2. return to initial configuration is not detected, i.e. the AVX state
>    is included even though it has been set to all zeros via vxorps,
> 3. the AVX state can be brought back to the initial configuration via
>    XRSTOR with the AVX component removed.

Thanks, for doing the tests.  I'm glad what I understood from my reading
matches experimental results.  :-)


r~