[RISU 0/9] risu cleanups and improvements

[RISU 0/9] risu cleanups and improvements

[Richard Henderson]

This patch set does alter the format of the trace files, and thus
means we'll have to re-generate these.  However, the space saved
for sve trace files is significant, so I consider it worthwhile.

In addition, the new --dump option allows one to inspect the
contents of the trace file.


r~


Richard Henderson (9):
  Use bool for tracing variables
  Unify master_fd and apprentice_fd to comm_fd
  Hoist trace file opening
  Adjust tracefile open for write
  Use EXIT_FAILURE, EXIT_SUCCESS
  Add magic and size to the trace header
  Compute reginfo_size based on the reginfo
  aarch64: Reorg sve reginfo to save space
  Add --dump option to inspect trace files

 risu.h                 |  12 ++-
 risu_reginfo_aarch64.h |  16 +---
 comms.c                |  26 +++---
 reginfo.c              |  45 ++++++---
 risu.c                 | 208 ++++++++++++++++++++++++++++++-----------
 risu_reginfo_aarch64.c | 192 ++++++++++++++++++++-----------------
 risu_reginfo_arm.c     |   6 +-
 risu_reginfo_i386.c    |   8 +-
 risu_reginfo_m68k.c    |   6 +-
 risu_reginfo_ppc64.c   |   6 +-
 10 files changed, 342 insertions(+), 183 deletions(-)

-- 
2.20.1

[RISU 1/9] Use bool for tracing variables

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.h    | 3 ++-
 reginfo.c | 2 +-
 risu.c    | 8 ++++----
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/risu.h b/risu.h
index 8d2d646..e2b4508 100644
--- a/risu.h
+++ b/risu.h
@@ -17,6 +17,7 @@
 #include <ucontext.h>
 #include <stdio.h>
 #include <getopt.h>
+#include <stdbool.h>
 
 /* Extra option processing for architectures */
 extern const struct option * const arch_long_opts;
@@ -96,7 +97,7 @@ int recv_and_compare_register_info(read_fn read_fn,
  * Should return 0 if it was a good match (ie end of test)
  * and 1 for a mismatch.
  */
-int report_match_status(int trace);
+int report_match_status(bool trace);
 
 /* Interface provided by CPU-specific code: */
 
diff --git a/reginfo.c b/reginfo.c
index dd42ae2..1b2a821 100644
--- a/reginfo.c
+++ b/reginfo.c
@@ -141,7 +141,7 @@ int recv_and_compare_register_info(read_fn read_fn,
  * Should return 0 if it was a good match (ie end of test)
  * and 1 for a mismatch.
  */
-int report_match_status(int trace)
+int report_match_status(bool trace)
 {
     int resp = 0;
     fprintf(stderr, "match status...\n");
diff --git a/risu.c b/risu.c
index 01525d2..79b1092 100644
--- a/risu.c
+++ b/risu.c
@@ -31,7 +31,7 @@
 void *memblock;
 
 int apprentice_fd, master_fd;
-int trace;
+bool trace;
 size_t signal_count;
 
 #ifdef HAVE_ZLIB
@@ -228,7 +228,7 @@ int master(void)
                     signal_count);
             return 0;
         } else {
-            return report_match_status(0);
+            return report_match_status(false);
         }
     }
     set_sigill_handler(&master_sigill);
@@ -250,7 +250,7 @@ int apprentice(void)
 #endif
         close(apprentice_fd);
         fprintf(stderr, "finished early after %zd checkpoints\n", signal_count);
-        return report_match_status(1);
+        return report_match_status(true);
     }
     set_sigill_handler(&apprentice_sigill);
     fprintf(stderr, "starting apprentice image at 0x%"PRIxPTR"\n",
@@ -344,7 +344,7 @@ int main(int argc, char **argv)
             break;
         case 't':
             trace_fn = optarg;
-            trace = 1;
+            trace = true;
             break;
         case 'h':
             hostname = optarg;
-- 
2.20.1

Re: [RISU 1/9] Use bool for tracing variables

[Peter Maydell]

On Wed, 13 May 2020 at 19:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  risu.h    | 3 ++-
>  reginfo.c | 2 +-
>  risu.c    | 8 ++++----
>  3 files changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[RISU 2/9] Unify master_fd and apprentice_fd to comm_fd

[Richard Henderson]

Any one invocation cannot be both master and apprentice.
Let's use only one variable for the file descriptor.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/risu.c b/risu.c
index 79b1092..059348f 100644
--- a/risu.c
+++ b/risu.c
@@ -30,7 +30,7 @@
 
 void *memblock;
 
-int apprentice_fd, master_fd;
+static int comm_fd;
 bool trace;
 size_t signal_count;
 
@@ -50,7 +50,7 @@ sigjmp_buf jmpbuf;
 
 int read_sock(void *ptr, size_t bytes)
 {
-    return recv_data_pkt(master_fd, ptr, bytes);
+    return recv_data_pkt(comm_fd, ptr, bytes);
 }
 
 int write_trace(void *ptr, size_t bytes)
@@ -58,9 +58,9 @@ int write_trace(void *ptr, size_t bytes)
     size_t res;
 
 #ifdef HAVE_ZLIB
-    if (master_fd == STDOUT_FILENO) {
+    if (comm_fd == STDOUT_FILENO) {
 #endif
-        res = write(master_fd, ptr, bytes);
+        res = write(comm_fd, ptr, bytes);
 #ifdef HAVE_ZLIB
     } else {
         res = gzwrite(gz_trace_file, ptr, bytes);
@@ -71,14 +71,14 @@ int write_trace(void *ptr, size_t bytes)
 
 void respond_sock(int r)
 {
-    send_response_byte(master_fd, r);
+    send_response_byte(comm_fd, r);
 }
 
 /* Apprentice function */
 
 int write_sock(void *ptr, size_t bytes)
 {
-    return send_data_pkt(apprentice_fd, ptr, bytes);
+    return send_data_pkt(comm_fd, ptr, bytes);
 }
 
 int read_trace(void *ptr, size_t bytes)
@@ -86,9 +86,9 @@ int read_trace(void *ptr, size_t bytes)
     size_t res;
 
 #ifdef HAVE_ZLIB
-    if (apprentice_fd == STDIN_FILENO) {
+    if (comm_fd == STDIN_FILENO) {
 #endif
-        res = read(apprentice_fd, ptr, bytes);
+        res = read(comm_fd, ptr, bytes);
 #ifdef HAVE_ZLIB
     } else {
         res = gzread(gz_trace_file, ptr, bytes);
@@ -218,11 +218,11 @@ int master(void)
 {
     if (sigsetjmp(jmpbuf, 1)) {
 #ifdef HAVE_ZLIB
-        if (trace && master_fd != STDOUT_FILENO) {
+        if (trace && comm_fd != STDOUT_FILENO) {
             gzclose(gz_trace_file);
         }
 #endif
-        close(master_fd);
+        close(comm_fd);
         if (trace) {
             fprintf(stderr, "trace complete after %zd checkpoints\n",
                     signal_count);
@@ -244,11 +244,11 @@ int apprentice(void)
 {
     if (sigsetjmp(jmpbuf, 1)) {
 #ifdef HAVE_ZLIB
-        if (trace && apprentice_fd != STDIN_FILENO) {
+        if (trace && comm_fd != STDIN_FILENO) {
             gzclose(gz_trace_file);
         }
 #endif
-        close(apprentice_fd);
+        close(comm_fd);
         fprintf(stderr, "finished early after %zd checkpoints\n", signal_count);
         return report_match_status(true);
     }
@@ -375,31 +375,31 @@ int main(int argc, char **argv)
     if (ismaster) {
         if (trace) {
             if (strcmp(trace_fn, "-") == 0) {
-                master_fd = STDOUT_FILENO;
+                comm_fd = STDOUT_FILENO;
             } else {
-                master_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
+                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
 #ifdef HAVE_ZLIB
-                gz_trace_file = gzdopen(master_fd, "wb9");
+                gz_trace_file = gzdopen(comm_fd, "wb9");
 #endif
             }
         } else {
             fprintf(stderr, "master port %d\n", port);
-            master_fd = master_connect(port);
+            comm_fd = master_connect(port);
         }
         return master();
     } else {
         if (trace) {
             if (strcmp(trace_fn, "-") == 0) {
-                apprentice_fd = STDIN_FILENO;
+                comm_fd = STDIN_FILENO;
             } else {
-                apprentice_fd = open(trace_fn, O_RDONLY);
+                comm_fd = open(trace_fn, O_RDONLY);
 #ifdef HAVE_ZLIB
-                gz_trace_file = gzdopen(apprentice_fd, "rb");
+                gz_trace_file = gzdopen(comm_fd, "rb");
 #endif
             }
         } else {
             fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
-            apprentice_fd = apprentice_connect(hostname, port);
+            comm_fd = apprentice_connect(hostname, port);
         }
         return apprentice();
     }
-- 
2.20.1

Re: [RISU 2/9] Unify master_fd and apprentice_fd to comm_fd

[Peter Maydell]

On Wed, 13 May 2020 at 19:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Any one invocation cannot be both master and apprentice.
> Let's use only one variable for the file descriptor.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  risu.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[RISU 3/9] Hoist trace file opening

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/risu.c b/risu.c
index 059348f..1c66885 100644
--- a/risu.c
+++ b/risu.c
@@ -363,6 +363,21 @@ int main(int argc, char **argv)
         }
     }
 
+    if (trace) {
+        if (strcmp(trace_fn, "-") == 0) {
+            comm_fd = ismaster ? STDOUT_FILENO : STDIN_FILENO;
+        } else {
+            if (ismaster) {
+                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
+            } else {
+                comm_fd = open(trace_fn, O_RDONLY);
+            }
+#ifdef HAVE_ZLIB
+            gz_trace_file = gzdopen(comm_fd, ismaster ? "wb9" : "rb");
+#endif
+        }
+    }
+
     imgfile = argv[optind];
     if (!imgfile) {
         fprintf(stderr, "Error: must specify image file name\n\n");
@@ -373,31 +388,13 @@ int main(int argc, char **argv)
     load_image(imgfile);
 
     if (ismaster) {
-        if (trace) {
-            if (strcmp(trace_fn, "-") == 0) {
-                comm_fd = STDOUT_FILENO;
-            } else {
-                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
-#ifdef HAVE_ZLIB
-                gz_trace_file = gzdopen(comm_fd, "wb9");
-#endif
-            }
-        } else {
+        if (!trace) {
             fprintf(stderr, "master port %d\n", port);
             comm_fd = master_connect(port);
         }
         return master();
     } else {
-        if (trace) {
-            if (strcmp(trace_fn, "-") == 0) {
-                comm_fd = STDIN_FILENO;
-            } else {
-                comm_fd = open(trace_fn, O_RDONLY);
-#ifdef HAVE_ZLIB
-                gz_trace_file = gzdopen(comm_fd, "rb");
-#endif
-            }
-        } else {
+        if (!trace) {
             fprintf(stderr, "apprentice host %s port %d\n", hostname, port);
             comm_fd = apprentice_connect(hostname, port);
         }
-- 
2.20.1

Re: [RISU 3/9] Hoist trace file opening

[Peter Maydell]

On Wed, 13 May 2020 at 19:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  risu.c | 37 +++++++++++++++++--------------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
>
> diff --git a/risu.c b/risu.c
> index 059348f..1c66885 100644
> --- a/risu.c
> +++ b/risu.c
> @@ -363,6 +363,21 @@ int main(int argc, char **argv)
>          }
>      }
>
> +    if (trace) {
> +        if (strcmp(trace_fn, "-") == 0) {
> +            comm_fd = ismaster ? STDOUT_FILENO : STDIN_FILENO;
> +        } else {
> +            if (ismaster) {
> +                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
> +            } else {
> +                comm_fd = open(trace_fn, O_RDONLY);
> +            }
> +#ifdef HAVE_ZLIB
> +            gz_trace_file = gzdopen(comm_fd, ismaster ? "wb9" : "rb");
> +#endif
> +        }
> +    }

Looking at later patches in the series I see the rationale.
We should really be error-checking the opens here, but that's
a preexisting bug.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[RISU 4/9] Adjust tracefile open for write

[Richard Henderson]

Truncate the new output file.  Rely on umask to remove
group+other file permissions, if desired.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/risu.c b/risu.c
index 1c66885..f404d8f 100644
--- a/risu.c
+++ b/risu.c
@@ -368,7 +368,7 @@ int main(int argc, char **argv)
             comm_fd = ismaster ? STDOUT_FILENO : STDIN_FILENO;
         } else {
             if (ismaster) {
-                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
+                comm_fd = open(trace_fn, O_WRONLY | O_CREAT | O_TRUNC, 0666);
             } else {
                 comm_fd = open(trace_fn, O_RDONLY);
             }
-- 
2.20.1

Re: [RISU 4/9] Adjust tracefile open for write

[Peter Maydell]

On Wed, 13 May 2020 at 19:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Truncate the new output file.  Rely on umask to remove
> group+other file permissions, if desired.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  risu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/risu.c b/risu.c
> index 1c66885..f404d8f 100644
> --- a/risu.c
> +++ b/risu.c
> @@ -368,7 +368,7 @@ int main(int argc, char **argv)
>              comm_fd = ismaster ? STDOUT_FILENO : STDIN_FILENO;
>          } else {
>              if (ismaster) {
> -                comm_fd = open(trace_fn, O_WRONLY | O_CREAT, S_IRWXU);
> +                comm_fd = open(trace_fn, O_WRONLY | O_CREAT | O_TRUNC, 0666);
>              } else {
>                  comm_fd = open(trace_fn, O_RDONLY);
>              }

I dunno why we were giving it execute permissions...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[RISU 5/9] Use EXIT_FAILURE, EXIT_SUCCESS

[Richard Henderson]

Some of the time we exit via the return value from main.
This can make it easier to tell what it is we're returning.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 comms.c                | 26 +++++++++++++-------------
 risu.c                 | 22 +++++++++++-----------
 risu_reginfo_aarch64.c |  4 ++--
 risu_reginfo_i386.c    |  2 +-
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/comms.c b/comms.c
index 6946fd9..861e845 100644
--- a/comms.c
+++ b/comms.c
@@ -31,7 +31,7 @@ int apprentice_connect(const char *hostname, int port)
     sock = socket(PF_INET, SOCK_STREAM, 0);
     if (sock < 0) {
         perror("socket");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     struct hostent *hostinfo;
     sa.sin_family = AF_INET;
@@ -39,12 +39,12 @@ int apprentice_connect(const char *hostname, int port)
     hostinfo = gethostbyname(hostname);
     if (!hostinfo) {
         fprintf(stderr, "Unknown host %s\n", hostname);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     sa.sin_addr = *(struct in_addr *) hostinfo->h_addr;
     if (connect(sock, (struct sockaddr *) &sa, sizeof(sa)) < 0) {
         perror("connect");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     return sock;
 }
@@ -56,13 +56,13 @@ int master_connect(int port)
     sock = socket(PF_INET, SOCK_STREAM, 0);
     if (sock < 0) {
         perror("socket");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     int sora = 1;
     if (setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &sora, sizeof(sora)) !=
         0) {
         perror("setsockopt(SO_REUSEADDR)");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     sa.sin_family = AF_INET;
@@ -70,11 +70,11 @@ int master_connect(int port)
     sa.sin_addr.s_addr = htonl(INADDR_ANY);
     if (bind(sock, (struct sockaddr *) &sa, sizeof(sa)) < 0) {
         perror("bind");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     if (listen(sock, 1) < 0) {
         perror("listen");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     /* Just block until we get a connection */
     fprintf(stderr, "master: waiting for connection on port %d...\n",
@@ -84,7 +84,7 @@ int master_connect(int port)
     int nsock = accept(sock, (struct sockaddr *) &csa, &csasz);
     if (nsock < 0) {
         perror("accept");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     /* We're done with the server socket now */
     close(sock);
@@ -104,7 +104,7 @@ static void recv_bytes(int sock, void *pkt, int pktlen)
                 continue;
             }
             perror("read failed");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
         pktlen -= i;
         p += i;
@@ -127,7 +127,7 @@ static void recv_and_discard_bytes(int sock, int pktlen)
                 continue;
             }
             perror("read failed");
-            exit(1);
+            exit(EXIT_FAILURE);
         }
         pktlen -= i;
     }
@@ -186,12 +186,12 @@ int send_data_pkt(int sock, void *pkt, int pktlen)
 
     if (safe_writev(sock, iov, 2) == -1) {
         perror("writev failed");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 
     if (read(sock, &resp, 1) != 1) {
         perror("read failed");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     return resp;
 }
@@ -217,6 +217,6 @@ void send_response_byte(int sock, int resp)
     unsigned char r = resp;
     if (write(sock, &r, 1) != 1) {
         perror("write failed");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 }
diff --git a/risu.c b/risu.c
index f404d8f..979341c 100644
--- a/risu.c
+++ b/risu.c
@@ -153,13 +153,13 @@ void apprentice_sigill(int sig, siginfo_t *si, void *uc)
         return;
     case 1:
         /* end of test */
-        exit(0);
+        exit(EXIT_SUCCESS);
     default:
         /* mismatch */
         if (trace) {
             siglongjmp(jmpbuf, 1);
         }
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 }
 
@@ -173,7 +173,7 @@ static void set_sigill_handler(void (*fn) (int, siginfo_t *, void *))
     sigemptyset(&sa.sa_mask);
     if (sigaction(SIGILL, &sa, 0) != 0) {
         perror("sigaction");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 }
 
@@ -190,11 +190,11 @@ void load_image(const char *imgfile)
     int fd = open(imgfile, O_RDONLY);
     if (fd < 0) {
         fprintf(stderr, "failed to open image file %s\n", imgfile);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     if (fstat(fd, &st) != 0) {
         perror("fstat");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     size_t len = st.st_size;
     void *addr;
@@ -207,7 +207,7 @@ void load_image(const char *imgfile)
              0);
     if (!addr) {
         perror("mmap");
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     close(fd);
     image_start = addr;
@@ -226,7 +226,7 @@ int master(void)
         if (trace) {
             fprintf(stderr, "trace complete after %zd checkpoints\n",
                     signal_count);
-            return 0;
+            return EXIT_SUCCESS;
         } else {
             return report_match_status(false);
         }
@@ -237,7 +237,7 @@ int master(void)
     fprintf(stderr, "starting image\n");
     image_start();
     fprintf(stderr, "image returned unexpectedly\n");
-    exit(1);
+    return EXIT_FAILURE;
 }
 
 int apprentice(void)
@@ -258,7 +258,7 @@ int apprentice(void)
     fprintf(stderr, "starting image\n");
     image_start();
     fprintf(stderr, "image returned unexpectedly\n");
-    exit(1);
+    return EXIT_FAILURE;
 }
 
 int ismaster;
@@ -355,7 +355,7 @@ int main(int argc, char **argv)
             break;
         case '?':
             usage();
-            exit(1);
+            return EXIT_FAILURE;
         default:
             assert(c >= FIRST_ARCH_OPT);
             process_arch_opt(c, optarg);
@@ -382,7 +382,7 @@ int main(int argc, char **argv)
     if (!imgfile) {
         fprintf(stderr, "Error: must specify image file name\n\n");
         usage();
-        exit(1);
+        return EXIT_FAILURE;
     }
 
     load_image(imgfile);
diff --git a/risu_reginfo_aarch64.c b/risu_reginfo_aarch64.c
index 00d1c8b..028c690 100644
--- a/risu_reginfo_aarch64.c
+++ b/risu_reginfo_aarch64.c
@@ -51,7 +51,7 @@ void process_arch_opt(int opt, const char *arg)
 
     if (test_sve <= 0 || test_sve > SVE_VQ_MAX) {
         fprintf(stderr, "Invalid value for VQ (1-%d)\n", SVE_VQ_MAX);
-        exit(1);
+        exit(EXIT_FAILURE);
     }
     want = sve_vl_from_vq(test_sve);
     got = prctl(PR_SVE_SET_VL, want);
@@ -62,7 +62,7 @@ void process_arch_opt(int opt, const char *arg)
             fprintf(stderr, "Unsupported value for VQ (%d != %d)\n",
                     test_sve, (int)sve_vq_from_vl(got));
         }
-        exit(1);
+        exit(EXIT_FAILURE);
     }
 #else
     abort();
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index 194e0ad..60fc239 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -69,7 +69,7 @@ void process_arch_opt(int opt, const char *arg)
             fprintf(stderr,
                     "Unable to parse '%s' in '%s' into an xfeatures integer mask\n",
                     endptr, arg);
-            exit(1);
+            exit(EXIT_FAILURE);
         }
     }
 }
-- 
2.20.1

Re: [RISU 5/9] Use EXIT_FAILURE, EXIT_SUCCESS

[Peter Maydell]

On Wed, 13 May 2020 at 19:10, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Some of the time we exit via the return value from main.
> This can make it easier to tell what it is we're returning.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

I don't really see the benefit personally, but
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [RISU 5/9] Use EXIT_FAILURE, EXIT_SUCCESS

[Richard Henderson]

On 5/18/20 8:54 AM, Peter Maydell wrote:
> On Wed, 13 May 2020 at 19:10, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Some of the time we exit via the return value from main.
>> This can make it easier to tell what it is we're returning.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> I don't really see the benefit personally, but

It wasn't so much for the direct calls of exit, but when we return an "int"
that gets passed to exit later.


r~

[RISU 6/9] Add magic and size to the trace header

[Richard Henderson]

Sanity check that we're not getting out of sync with
the trace stream.  This will be especially bad with
the change in size of the sve save data.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.h    |  6 +++++-
 reginfo.c | 42 ++++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/risu.h b/risu.h
index e2b4508..3fc198f 100644
--- a/risu.h
+++ b/risu.h
@@ -62,10 +62,14 @@ extern void *memblock;
 struct reginfo;
 
 typedef struct {
-   uintptr_t pc;
+   uint32_t magic;
+   uint32_t size;
    uint32_t risu_op;
+   uintptr_t pc;
 } trace_header_t;
 
+#define RISU_MAGIC  (('R' << 24) | ('i' << 16) | ('S' << 8) | 'u')
+
 /* Functions operating on reginfo */
 
 /* Function prototypes for read/write helper functions.
diff --git a/reginfo.c b/reginfo.c
index 1b2a821..a4f7da6 100644
--- a/reginfo.c
+++ b/reginfo.c
@@ -26,20 +26,45 @@ int send_register_info(write_fn write_fn, void *uc)
     struct reginfo ri;
     trace_header_t header;
     int op;
+    void *extra;
 
     reginfo_init(&ri, uc);
     op = get_risuop(&ri);
 
     /* Write a header with PC/op to keep in sync */
+    header.magic = RISU_MAGIC;
     header.pc = get_pc(&ri);
     header.risu_op = op;
+
+    switch (op) {
+    case OP_TESTEND:
+    case OP_COMPARE:
+    default:
+        header.size = reginfo_size();
+        extra = &ri;
+        break;
+
+    case OP_SETMEMBLOCK:
+    case OP_GETMEMBLOCK:
+        header.size = 0;
+        extra = NULL;
+        break;
+
+    case OP_COMPAREMEM:
+        header.size = MEMBLOCKLEN;
+        extra = memblock;
+        break;
+    }
+
     if (write_fn(&header, sizeof(header)) != 0) {
         return -1;
     }
+    if (extra && write_fn(extra, header.size) != 0) {
+        return -1;
+    }
 
     switch (op) {
     case OP_TESTEND:
-        write_fn(&ri, reginfo_size());
         /* if we are tracing write_fn will return 0 unlike a remote
            end, hence we force return of 1 here */
         return 1;
@@ -51,14 +76,9 @@ int send_register_info(write_fn write_fn, void *uc)
                               get_reginfo_paramreg(&ri) + (uintptr_t)memblock);
         break;
     case OP_COMPAREMEM:
-        return write_fn(memblock, MEMBLOCKLEN);
-        break;
     case OP_COMPARE:
     default:
-        /* Do a simple register compare on (a) explicit request
-         * (b) end of test (c) a non-risuop UNDEF
-         */
-        return write_fn(&ri, reginfo_size());
+        break;
     }
     return 0;
 }
@@ -84,7 +104,7 @@ int recv_and_compare_register_info(read_fn read_fn,
         return -1;
     }
 
-    if (header.risu_op != op) {
+    if (header.magic != RISU_MAGIC || header.risu_op != op) {
         /* We are out of sync */
         resp = 2;
         resp_fn(resp);
@@ -101,7 +121,8 @@ int recv_and_compare_register_info(read_fn read_fn,
         /* Do a simple register compare on (a) explicit request
          * (b) end of test (c) a non-risuop UNDEF
          */
-        if (read_fn(&apprentice_ri, reginfo_size())) {
+        if (header.size != reginfo_size() ||
+            read_fn(&apprentice_ri, header.size)) {
             packet_mismatch = 1;
             resp = 2;
         } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {
@@ -121,7 +142,8 @@ int recv_and_compare_register_info(read_fn read_fn,
         break;
     case OP_COMPAREMEM:
         mem_used = 1;
-        if (read_fn(apprentice_memblock, MEMBLOCKLEN)) {
+        if (header.size != MEMBLOCKLEN ||
+            read_fn(apprentice_memblock, MEMBLOCKLEN)) {
             packet_mismatch = 1;
             resp = 2;
         } else if (memcmp(memblock, apprentice_memblock, MEMBLOCKLEN) != 0) {
-- 
2.20.1

[RISU 7/9] Compute reginfo_size based on the reginfo

[Richard Henderson]

This will allow dumping of SVE frames without having
to know the SVE vector length beforehand.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.h                 | 2 +-
 reginfo.c              | 7 ++++---
 risu_reginfo_aarch64.c | 4 ++--
 risu_reginfo_arm.c     | 2 +-
 risu_reginfo_i386.c    | 2 +-
 risu_reginfo_m68k.c    | 2 +-
 risu_reginfo_ppc64.c   | 2 +-
 7 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/risu.h b/risu.h
index 3fc198f..0ae7fa9 100644
--- a/risu.h
+++ b/risu.h
@@ -139,6 +139,6 @@ int reginfo_dump(struct reginfo *ri, FILE * f);
 int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f);
 
 /* return size of reginfo */
-const int reginfo_size(void);
+int reginfo_size(struct reginfo *ri);
 
 #endif /* RISU_H */
diff --git a/reginfo.c b/reginfo.c
index a4f7da6..1727867 100644
--- a/reginfo.c
+++ b/reginfo.c
@@ -40,7 +40,7 @@ int send_register_info(write_fn write_fn, void *uc)
     case OP_TESTEND:
     case OP_COMPARE:
     default:
-        header.size = reginfo_size();
+        header.size = reginfo_size(&ri);
         extra = &ri;
         break;
 
@@ -121,8 +121,9 @@ int recv_and_compare_register_info(read_fn read_fn,
         /* Do a simple register compare on (a) explicit request
          * (b) end of test (c) a non-risuop UNDEF
          */
-        if (header.size != reginfo_size() ||
-            read_fn(&apprentice_ri, header.size)) {
+        if (header.size > sizeof(struct reginfo) ||
+            read_fn(&apprentice_ri, header.size) ||
+            header.size != reginfo_size(&apprentice_ri)) {
             packet_mismatch = 1;
             resp = 2;
         } else if (!reginfo_is_eq(&master_ri, &apprentice_ri)) {
diff --git a/risu_reginfo_aarch64.c b/risu_reginfo_aarch64.c
index 028c690..7044648 100644
--- a/risu_reginfo_aarch64.c
+++ b/risu_reginfo_aarch64.c
@@ -69,7 +69,7 @@ void process_arch_opt(int opt, const char *arg)
 #endif
 }
 
-const int reginfo_size(void)
+int reginfo_size(struct reginfo *ri)
 {
     int size = offsetof(struct reginfo, simd.end);
 #ifdef SVE_MAGIC
@@ -194,7 +194,7 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
 /* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
 int reginfo_is_eq(struct reginfo *r1, struct reginfo *r2)
 {
-    return memcmp(r1, r2, reginfo_size()) == 0;
+    return memcmp(r1, r2, reginfo_size(r1)) == 0;
 }
 
 #ifdef SVE_MAGIC
diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
index 3662f12..3832e27 100644
--- a/risu_reginfo_arm.c
+++ b/risu_reginfo_arm.c
@@ -36,7 +36,7 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
-const int reginfo_size(void)
+int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
 }
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index 60fc239..902d33e 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -74,7 +74,7 @@ void process_arch_opt(int opt, const char *arg)
     }
 }
 
-const int reginfo_size(void)
+int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
 }
diff --git a/risu_reginfo_m68k.c b/risu_reginfo_m68k.c
index 32b28c8..361f172 100644
--- a/risu_reginfo_m68k.c
+++ b/risu_reginfo_m68k.c
@@ -23,7 +23,7 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
-const int reginfo_size(void)
+int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
 }
diff --git a/risu_reginfo_ppc64.c b/risu_reginfo_ppc64.c
index 071c951..c86313c 100644
--- a/risu_reginfo_ppc64.c
+++ b/risu_reginfo_ppc64.c
@@ -32,7 +32,7 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
-const int reginfo_size(void)
+int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
 }
-- 
2.20.1

[RISU 8/9] aarch64: Reorg sve reginfo to save space

[Richard Henderson]

Mirror the signal frame by storing all of the registers
as a lump.  Use the signal macros to pull out the values.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu_reginfo_aarch64.h |  16 +----
 risu_reginfo_aarch64.c | 135 +++++++++++++++++++++--------------------
 2 files changed, 73 insertions(+), 78 deletions(-)

diff --git a/risu_reginfo_aarch64.h b/risu_reginfo_aarch64.h
index c33b86f..01076b4 100644
--- a/risu_reginfo_aarch64.h
+++ b/risu_reginfo_aarch64.h
@@ -17,20 +17,8 @@
 
 struct simd_reginfo {
     __uint128_t vregs[32];
-    char end[0];
 };
 
-#ifdef SVE_MAGIC
-struct sve_reginfo {
-    /* SVE */
-    uint16_t    vl; /* current VL */
-    __uint128_t zregs[SVE_NUM_ZREGS][SVE_VQ_MAX];
-    uint16_t    pregs[SVE_NUM_PREGS][SVE_VQ_MAX];
-    uint16_t    ffr[SVE_VQ_MAX];
-    char end[0];
-};
-#endif
-
 /* The kernel headers set this based on future arch extensions.
    The current arch maximum is 16.  Save space below.  */
 #undef SVE_VQ_MAX
@@ -47,11 +35,13 @@ struct reginfo {
     /* FP/SIMD */
     uint32_t fpsr;
     uint32_t fpcr;
+    uint32_t sve_vl;
 
     union {
         struct simd_reginfo simd;
 #ifdef SVE_MAGIC
-        struct sve_reginfo sve;
+        char sve[SVE_SIG_CONTEXT_SIZE(16) - SVE_SIG_REGS_OFFSET]
+            __attribute__((aligned(16)));
 #endif
     };
 };
diff --git a/risu_reginfo_aarch64.c b/risu_reginfo_aarch64.c
index 7044648..a1020ac 100644
--- a/risu_reginfo_aarch64.c
+++ b/risu_reginfo_aarch64.c
@@ -71,15 +71,30 @@ void process_arch_opt(int opt, const char *arg)
 
 int reginfo_size(struct reginfo *ri)
 {
-    int size = offsetof(struct reginfo, simd.end);
 #ifdef SVE_MAGIC
-    if (test_sve) {
-        size = offsetof(struct reginfo, sve.end);
+    if (ri->sve_vl) {
+        int vq = sve_vq_from_vl(ri->sve_vl);
+        return (offsetof(struct reginfo, sve) +
+                SVE_SIG_CONTEXT_SIZE(vq) - SVE_SIG_REGS_OFFSET);
     }
 #endif
-    return size;
+    return offsetof(struct reginfo, simd) + sizeof(ri->simd);
 }
 
+#ifdef SVE_MAGIC
+static uint64_t *reginfo_zreg(struct reginfo *ri, int vq, int i)
+{
+    return (uint64_t *)(ri->sve + SVE_SIG_ZREG_OFFSET(vq, i) -
+                        SVE_SIG_REGS_OFFSET);
+}
+
+static uint16_t *reginfo_preg(struct reginfo *ri, int vq, int i)
+{
+    return (uint16_t *)(ri->sve + SVE_SIG_PREG_OFFSET(vq, i) -
+                        SVE_SIG_REGS_OFFSET);
+}
+#endif
+
 /* reginfo_init: initialize with a ucontext */
 void reginfo_init(struct reginfo *ri, ucontext_t *uc)
 {
@@ -152,8 +167,6 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
             return;
         }
 
-        ri->sve.vl = sve->vl;
-
         if (sve->head.size < SVE_SIG_CONTEXT_SIZE(vq)) {
             if (sve->head.size == sizeof(*sve)) {
                 /* SVE state is empty -- not an error.  */
@@ -164,24 +177,9 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
             return;
         }
 
-        /* Copy ZREG's one at a time */
-        for (i = 0; i < SVE_NUM_ZREGS; i++) {
-            memcpy(&ri->sve.zregs[i],
-                   (void *)sve + SVE_SIG_ZREG_OFFSET(vq, i),
-                   SVE_SIG_ZREG_SIZE(vq));
-        }
-
-        /* Copy PREG's one at a time */
-        for (i = 0; i < SVE_NUM_PREGS; i++) {
-            memcpy(&ri->sve.pregs[i],
-                   (void *)sve + SVE_SIG_PREG_OFFSET(vq, i),
-                   SVE_SIG_PREG_SIZE(vq));
-        }
-
-        /* Finally the FFR */
-        memcpy(&ri->sve.ffr, (void *)sve + SVE_SIG_FFR_OFFSET(vq),
-               SVE_SIG_FFR_SIZE(vq));
-
+        ri->sve_vl = sve->vl;
+        memcpy(ri->sve, (char *)sve + SVE_SIG_REGS_OFFSET,
+               SVE_SIG_CONTEXT_SIZE(vq) - SVE_SIG_REGS_OFFSET);
         return;
     }
 #endif /* SVE_MAGIC */
@@ -225,18 +223,20 @@ static void sve_dump_preg_diff(FILE *f, int vq, const uint16_t *p1,
     fprintf(f, "\n");
 }
 
-static void sve_dump_zreg_diff(FILE *f, int vq, const __uint128_t *z1,
-                               const __uint128_t *z2)
+static void sve_dump_zreg_diff(FILE *f, int vq, const uint64_t *za,
+                               const uint64_t *zb)
 {
     const char *pad = "";
     int q;
 
     for (q = 0; q < vq; ++q) {
-        if (z1[q] != z2[q]) {
+        uint64_t za0 = za[2 * q], za1 = za[2 * q + 1];
+        uint64_t zb0 = zb[2 * q], zb1 = zb[2 * q + 1];
+
+        if (za0 != zb0 || za1 != zb1) {
             fprintf(f, "%sq%-2d: %016" PRIx64 "%016" PRIx64
-                    " vs %016" PRIx64 "%016" PRIx64"\n", pad, q,
-                    (uint64_t)(z1[q] >> 64), (uint64_t)z1[q],
-                    (uint64_t)(z2[q] >> 64), (uint64_t)z2[q]);
+                    " vs %016" PRIx64 "%016" PRIx64"\n",
+                    pad, q, za1, za0, zb1, zb0);
             pad = "      ";
         }
     }
@@ -263,28 +263,30 @@ int reginfo_dump(struct reginfo *ri, FILE * f)
     if (test_sve) {
         int q, vq = test_sve;
 
-        fprintf(f, "  vl     : %d\n", ri->sve.vl);
+        fprintf(f, "  vl     : %d\n", ri->sve_vl);
 
-        for (i = 0; i < 32; i++) {
-            fprintf(f, "  Z%-2d q%-2d: %016" PRIx64 "%016" PRIx64 "\n", i, 0,
-                    (uint64_t)(ri->sve.zregs[i][0] >> 64),
-                    (uint64_t)ri->sve.zregs[i][0]);
+        for (i = 0; i < SVE_NUM_ZREGS; i++) {
+            uint64_t *z = reginfo_zreg(ri, vq, i);
+
+            fprintf(f, "  Z%-2d q%-2d: %016" PRIx64 "%016" PRIx64 "\n",
+                    i, 0, z[1], z[0]);
             for (q = 1; q < vq; ++q) {
-                fprintf(f, "      q%-2d: %016" PRIx64 "%016" PRIx64 "\n", q,
-                        (uint64_t)(ri->sve.zregs[i][q] >> 64),
-                        (uint64_t)ri->sve.zregs[i][q]);
+                fprintf(f, "      q%-2d: %016" PRIx64 "%016" PRIx64 "\n",
+                        q, z[q * 2 + 1], z[q * 2]);
             }
         }
 
-        for (i = 0; i < 16; i++) {
-            fprintf(f, "  P%-2d    : ", i);
-            sve_dump_preg(f, vq, &ri->sve.pregs[i][0]);
+        for (i = 0; i < SVE_NUM_PREGS + 1; i++) {
+            uint16_t *p = reginfo_preg(ri, vq, i);
+
+            if (i == SVE_NUM_PREGS) {
+                fprintf(f, "  FFR    : ");
+            } else {
+                fprintf(f, "  P%-2d    : ", i);
+            }
+            sve_dump_preg(f, vq, p);
             fprintf(f, "\n");
         }
-        fprintf(f, "  FFR    : ");
-        sve_dump_preg(f, vq, &ri->sve.ffr[0]);
-        fprintf(f, "\n");
-
         return !ferror(f);
     }
 #endif
@@ -338,31 +340,34 @@ int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE * f)
 
 #ifdef SVE_MAGIC
     if (test_sve) {
-        int vq = sve_vq_from_vl(m->sve.vl);
+        int vq = sve_vq_from_vl(m->sve_vl);
 
-        if (m->sve.vl != a->sve.vl) {
-            fprintf(f, "  vl    : %d vs %d\n", m->sve.vl, a->sve.vl);
+        if (m->sve_vl != a->sve_vl) {
+            fprintf(f, "  vl    : %d vs %d\n", m->sve_vl, a->sve_vl);
         }
 
         for (i = 0; i < SVE_NUM_ZREGS; i++) {
-            if (!sve_zreg_is_eq(vq, &m->sve.zregs[i], &a->sve.zregs[i])) {
-                fprintf(f, "  Z%-2d ", i);
-                sve_dump_zreg_diff(f, vq, &m->sve.zregs[i][0],
-                                   &a->sve.zregs[i][0]);
-            }
-        }
-        for (i = 0; i < SVE_NUM_PREGS; i++) {
-            if (!sve_preg_is_eq(vq, &m->sve.pregs[i], &a->sve.pregs[i])) {
-                fprintf(f, "  P%-2d    : ", i);
-                sve_dump_preg_diff(f, vq, &m->sve.pregs[i][0],
-                                   &a->sve.pregs[i][0]);
-            }
-        }
-        if (!sve_preg_is_eq(vq, &m->sve.ffr, &a->sve.ffr)) {
-            fprintf(f, "  FFR   : ");
-            sve_dump_preg_diff(f, vq, &m->sve.pregs[i][0], &a->sve.pregs[i][0]);
-        }
+            uint64_t *zm = reginfo_zreg(m, vq, i);
+            uint64_t *za = reginfo_zreg(a, vq, i);
 
+            if (!sve_zreg_is_eq(vq, zm, za)) {
+                fprintf(f, "  Z%-2d ", i);
+                sve_dump_zreg_diff(f, vq, zm, za);
+            }
+        }
+        for (i = 0; i < SVE_NUM_PREGS + 1; i++) {
+            uint16_t *pm = reginfo_preg(m, vq, i);
+            uint16_t *pa = reginfo_preg(a, vq, i);
+
+            if (!sve_preg_is_eq(vq, pm, pa)) {
+                if (i == SVE_NUM_PREGS) {
+                    fprintf(f, "  FFR   : ");
+                } else {
+                    fprintf(f, "  P%-2d    : ", i);
+                }
+                sve_dump_preg_diff(f, vq, pm, pa);
+            }
+        }
         return !ferror(f);
     }
 #endif
-- 
2.20.1

[RISU 9/9] Add --dump option to inspect trace files

[Richard Henderson]

Adjust some of the aarch64 code to look at the reginfo struct
instead of looking at test_sve, so that we do not need to pass
the --test-sve option in order to dump sve trace files.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 risu.h                 |   1 +
 risu.c                 | 111 ++++++++++++++++++++++++++++++++++++++++-
 risu_reginfo_aarch64.c |  55 ++++++++++++--------
 risu_reginfo_arm.c     |   4 ++
 risu_reginfo_i386.c    |   4 ++
 risu_reginfo_m68k.c    |   4 ++
 risu_reginfo_ppc64.c   |   4 ++
 7 files changed, 161 insertions(+), 22 deletions(-)

diff --git a/risu.h b/risu.h
index 0ae7fa9..515e5c2 100644
--- a/risu.h
+++ b/risu.h
@@ -23,6 +23,7 @@
 extern const struct option * const arch_long_opts;
 extern const char * const arch_extra_help;
 void process_arch_opt(int opt, const char *arg);
+void arch_init(void);
 #define FIRST_ARCH_OPT   0x100
 
 /* GCC computed include to pull in the correct risu_reginfo_*.h for
diff --git a/risu.c b/risu.c
index 979341c..6410b2f 100644
--- a/risu.c
+++ b/risu.c
@@ -261,6 +261,94 @@ int apprentice(void)
     return EXIT_FAILURE;
 }
 
+int dump_trace(void)
+{
+    trace_header_t header;
+    union {
+        struct reginfo ri;
+        unsigned char memblock[MEMBLOCKLEN];
+    } u;
+    const char *op_name;
+
+    while (1) {
+        if (read_trace(&header, sizeof(header))) {
+            fprintf(stderr, "Trace header read failed\n");
+            return EXIT_FAILURE;
+        }
+
+        if (header.magic != RISU_MAGIC) {
+            fprintf(stderr, "Unexpected header magic (%#x)\n", header.magic);
+            return EXIT_FAILURE;
+        }
+
+        switch (header.risu_op) {
+        case OP_COMPARE:
+           op_name = "COMPARE";
+           break;
+        case OP_TESTEND:
+           op_name = "TESTEND";
+           break;
+        case OP_SETMEMBLOCK:
+           op_name = "SETMEMBLOCK";
+           break;
+        case OP_GETMEMBLOCK:
+           op_name = "GETMEMBLOCK";
+           break;
+        case OP_COMPAREMEM:
+           op_name = "COMPAREMEM";
+           break;
+        case -1:
+           op_name = "FAULT";
+           break;
+        default:
+           op_name = "<unknown>";
+           break;
+        }
+
+        switch (header.risu_op) {
+        case OP_COMPARE:
+        case OP_TESTEND:
+        default:
+            if (header.size > sizeof(u.ri)) {
+                fprintf(stderr, "Unexpected trace size (%u)\n", header.size);
+                return EXIT_FAILURE;
+            }
+            if (read_trace(&u.ri, header.size)) {
+                fprintf(stderr, "Reginfo read failed\n");
+                return EXIT_FAILURE;
+            }
+            if (header.size != reginfo_size(&u.ri)) {
+                fprintf(stderr, "Unexpected trace size (%u)\n", header.size);
+                return EXIT_FAILURE;
+            }
+            printf("%s: (pc %#lx)\n", op_name, (unsigned long)header.pc);
+            reginfo_dump(&u.ri, stdout);
+            putchar('\n');
+            if (header.risu_op == OP_TESTEND) {
+                return EXIT_SUCCESS;
+            }
+            break;
+
+        case OP_COMPAREMEM:
+            if (header.size != MEMBLOCKLEN) {
+                fprintf(stderr, "Unexpected trace size (%u)\n", header.size);
+                return EXIT_FAILURE;
+            }
+            if (read_trace(&u.memblock, MEMBLOCKLEN)) {
+                fprintf(stderr, "Memblock read failed\n");
+                return EXIT_FAILURE;
+            }
+            /* TODO: Dump 8k of data? */
+            /* fall through */
+
+        case OP_SETMEMBLOCK:
+        case OP_GETMEMBLOCK:
+            printf("%s\n", op_name);
+            break;
+        }
+    }
+}
+
 int ismaster;
 
 void usage(void)
@@ -273,6 +361,7 @@ void usage(void)
     fprintf(stderr, "between master and apprentice risu processes.\n\n");
     fprintf(stderr, "Options:\n");
     fprintf(stderr, "  --master          Be the master (server)\n");
+    fprintf(stderr, "  -d, --dump=FILE   Dump " TRACE_TYPE " trace file\n");
     fprintf(stderr, "  -t, --trace=FILE  Record/playback " TRACE_TYPE " trace file\n");
     fprintf(stderr,
             "  -h, --host=HOST   Specify master host machine (apprentice only)"
@@ -293,11 +382,12 @@ static struct option * setup_options(char **short_opts)
         {"host", required_argument, 0, 'h'},
         {"port", required_argument, 0, 'p'},
         {"trace", required_argument, 0, 't'},
+        {"dump", required_argument, 0, 'd'},
         {0, 0, 0, 0}
     };
     struct option *lopts = &default_longopts[0];
 
-    *short_opts = "h:p:t:";
+    *short_opts = "d:h:p:t:";
 
     if (arch_long_opts) {
         const size_t osize = sizeof(struct option);
@@ -328,6 +418,7 @@ int main(int argc, char **argv)
     char *trace_fn = NULL;
     struct option *longopts;
     char *shortopts;
+    bool dump = false;
 
     longopts = setup_options(&shortopts);
 
@@ -342,6 +433,10 @@ int main(int argc, char **argv)
         case 0:
             /* flag set by getopt_long, do nothing */
             break;
+        case 'd':
+            trace_fn = optarg;
+            dump = true;
+            break;
         case 't':
             trace_fn = optarg;
             trace = true;
@@ -363,7 +458,12 @@ int main(int argc, char **argv)
         }
     }
 
-    if (trace) {
+    if (dump && ismaster) {
+        usage();
+        exit(1);
+    }
+
+    if (trace || dump) {
         if (strcmp(trace_fn, "-") == 0) {
             comm_fd = ismaster ? STDOUT_FILENO : STDIN_FILENO;
         } else {
@@ -378,6 +478,10 @@ int main(int argc, char **argv)
         }
     }
 
+    if (dump) {
+        return dump_trace();
+    }
+
     imgfile = argv[optind];
     if (!imgfile) {
         fprintf(stderr, "Error: must specify image file name\n\n");
@@ -387,6 +491,9 @@ int main(int argc, char **argv)
 
     load_image(imgfile);
 
+    /* Select requested SVE vector length. */
+    arch_init();
+
     if (ismaster) {
         if (!trace) {
             fprintf(stderr, "master port %d\n", port);
diff --git a/risu_reginfo_aarch64.c b/risu_reginfo_aarch64.c
index a1020ac..fb8e11a 100644
--- a/risu_reginfo_aarch64.c
+++ b/risu_reginfo_aarch64.c
@@ -44,8 +44,6 @@ const char * const arch_extra_help
 void process_arch_opt(int opt, const char *arg)
 {
 #ifdef SVE_MAGIC
-    long want, got;
-
     assert(opt == FIRST_ARCH_OPT);
     test_sve = strtol(arg, 0, 10);
 
@@ -53,22 +51,37 @@ void process_arch_opt(int opt, const char *arg)
         fprintf(stderr, "Invalid value for VQ (1-%d)\n", SVE_VQ_MAX);
         exit(EXIT_FAILURE);
     }
-    want = sve_vl_from_vq(test_sve);
-    got = prctl(PR_SVE_SET_VL, want);
-    if (want != got) {
-        if (got < 0) {
-            perror("prctl PR_SVE_SET_VL");
-        } else {
-            fprintf(stderr, "Unsupported value for VQ (%d != %d)\n",
-                    test_sve, (int)sve_vq_from_vl(got));
-        }
-        exit(EXIT_FAILURE);
-    }
 #else
     abort();
 #endif
 }
 
+void arch_init(void)
+{
+#ifdef SVE_MAGIC
+    long want, got1, got2;
+
+    if (test_sve == 0) {
+        return;
+    }
+
+    want = sve_vl_from_vq(test_sve);
+    asm(".arch_extension sve\n\trdvl %0, #1" : "=r"(got1));
+    if (want != got1) {
+        got2 = prctl(PR_SVE_SET_VL, want);
+        if (want != got2) {
+            if (got2 < 0) {
+                perror("prctl PR_SVE_SET_VL");
+                got2 = got1;
+            }
+            fprintf(stderr, "Unsupported value for VQ (%d != %d)\n",
+                    test_sve, (int)sve_vq_from_vl(got1));
+            exit(EXIT_FAILURE);
+        }
+    }
+#endif
+}
+
 int reginfo_size(struct reginfo *ri)
 {
 #ifdef SVE_MAGIC
@@ -170,6 +183,7 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
         if (sve->head.size < SVE_SIG_CONTEXT_SIZE(vq)) {
             if (sve->head.size == sizeof(*sve)) {
                 /* SVE state is empty -- not an error.  */
+                goto do_simd;
             } else {
                 fprintf(stderr, "risu_reginfo_aarch64: "
                         "failed to get complete SVE state\n");
@@ -182,6 +196,7 @@ void reginfo_init(struct reginfo *ri, ucontext_t *uc)
                SVE_SIG_CONTEXT_SIZE(vq) - SVE_SIG_REGS_OFFSET);
         return;
     }
+ do_simd:
 #endif /* SVE_MAGIC */
 
     for (i = 0; i < 32; i++) {
@@ -260,8 +275,9 @@ int reginfo_dump(struct reginfo *ri, FILE * f)
     fprintf(f, "  fpcr   : %08x\n", ri->fpcr);
 
 #ifdef SVE_MAGIC
-    if (test_sve) {
-        int q, vq = test_sve;
+    if (ri->sve_vl) {
+        int vq = sve_vq_from_vl(ri->sve_vl);
+        int q;
 
         fprintf(f, "  vl     : %d\n", ri->sve_vl);
 
@@ -339,13 +355,12 @@ int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE * f)
     }
 
 #ifdef SVE_MAGIC
-    if (test_sve) {
+    if (m->sve_vl != a->sve_vl) {
+        fprintf(f, "  vl    : %d vs %d\n", m->sve_vl, a->sve_vl);
+    }
+    if (m->sve_vl) {
         int vq = sve_vq_from_vl(m->sve_vl);
 
-        if (m->sve_vl != a->sve_vl) {
-            fprintf(f, "  vl    : %d vs %d\n", m->sve_vl, a->sve_vl);
-        }
-
         for (i = 0; i < SVE_NUM_ZREGS; i++) {
             uint64_t *zm = reginfo_zreg(m, vq, i);
             uint64_t *za = reginfo_zreg(a, vq, i);
diff --git a/risu_reginfo_arm.c b/risu_reginfo_arm.c
index 3832e27..2982435 100644
--- a/risu_reginfo_arm.c
+++ b/risu_reginfo_arm.c
@@ -36,6 +36,10 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
+void arch_init(void)
+{
+}
+
 int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
index 902d33e..68f2323 100644
--- a/risu_reginfo_i386.c
+++ b/risu_reginfo_i386.c
@@ -74,6 +74,10 @@ void process_arch_opt(int opt, const char *arg)
     }
 }
 
+void arch_init(void)
+{
+}
+
 int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
diff --git a/risu_reginfo_m68k.c b/risu_reginfo_m68k.c
index 361f172..499fdc4 100644
--- a/risu_reginfo_m68k.c
+++ b/risu_reginfo_m68k.c
@@ -23,6 +23,10 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
+void arch_init(void)
+{
+}
+
 int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
diff --git a/risu_reginfo_ppc64.c b/risu_reginfo_ppc64.c
index c86313c..3b04747 100644
--- a/risu_reginfo_ppc64.c
+++ b/risu_reginfo_ppc64.c
@@ -32,6 +32,10 @@ void process_arch_opt(int opt, const char *arg)
     abort();
 }
 
+void arch_init(void)
+{
+}
+
 int reginfo_size(struct reginfo *ri)
 {
     return sizeof(struct reginfo);
-- 
2.20.1

Re: [RISU 0/9] risu cleanups and improvements

[Peter Maydell]

On Wed, 13 May 2020 at 19:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This patch set does alter the format of the trace files, and thus
> means we'll have to re-generate these.  However, the space saved
> for sve trace files is significant, so I consider it worthwhile.
>
> In addition, the new --dump option allows one to inspect the
> contents of the trace file.

Alex, would you mind reviewing these risu patches, given that they're
mostly trace related? (Also you're the one who'll have to regenerate
a lot of trace files :-))

Richard: if you feed an old trace file to the new risu, what
error message does it give?

thanks
-- PMM

Re: [RISU 0/9] risu cleanups and improvements

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 13 May 2020 at 19:09, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This patch set does alter the format of the trace files, and thus
>> means we'll have to re-generate these.  However, the space saved
>> for sve trace files is significant, so I consider it worthwhile.
>>
>> In addition, the new --dump option allows one to inspect the
>> contents of the trace file.
>
> Alex, would you mind reviewing these risu patches, given that they're
> mostly trace related? (Also you're the one who'll have to regenerate
> a lot of trace files :-))

Sure. I'll have a look in the morning.

>
> Richard: if you feed an old trace file to the new risu, what
> error message does it give?
>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RISU 0/9] risu cleanups and improvements

[Richard Henderson]

On 5/18/20 11:39 AM, Peter Maydell wrote:
> On Wed, 13 May 2020 at 19:09, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This patch set does alter the format of the trace files, and thus
>> means we'll have to re-generate these.  However, the space saved
>> for sve trace files is significant, so I consider it worthwhile.
>>
>> In addition, the new --dump option allows one to inspect the
>> contents of the trace file.
> 
> Alex, would you mind reviewing these risu patches, given that they're
> mostly trace related? (Also you're the one who'll have to regenerate
> a lot of trace files :-))
> 
> Richard: if you feed an old trace file to the new risu, what
> error message does it give?

Bah, it should have generated an error vs the magic number, but doesn't -- it
silently exits with success.  Alex, expect a v2.


r~

Re: [RISU 0/9] risu cleanups and improvements

[Richard Henderson]

On 5/18/20 12:33 PM, Richard Henderson wrote:
> On 5/18/20 11:39 AM, Peter Maydell wrote:
>> Richard: if you feed an old trace file to the new risu, what
>> error message does it give?
> 
> Bah, it should have generated an error vs the magic number, but doesn't -- it
> silently exits with success.  Alex, expect a v2.

Double bah.  Typo in git checkout, so I didn't actually switch branches for the
test.  It does print an error, but not a useful one:

match status...
mismatch on regs!

I'll improve this.


r~