[risu PATCH 0/4] Add support for s390x to RISU

[risu PATCH 0/4] Add support for s390x to RISU

[Thomas Huth]

 Hi Peter!

Here are some patches that add basic support for s390x to RISU.
It's still quite limited, e.g. no support for load/store memory
operations yet, but the basics with simple 16-bit or 32-bit
instructions work already fine.

(In the long run, we'd need to support instructions with 48-bit
length on s390x, too, since most newer "interesting" instructions
like e.g. vector SIMD instructions are encoded with 48 bit. This
will require modifications to the generic code, too, so I limited
my initial implementation to 16-bit and 32-bit instruction length
support to keep the code self-contained in the s390x architecture
specific files)

Thomas Huth (4):
  s390x: Add basic s390x support to the C code
  s390x: Add simple s390x.risu file
  s390x: Add basic risugen perl module for s390x
  s390x: Update the configure script for s390x support

 configure            |   4 +-
 risu_reginfo_s390x.c | 142 +++++++++++++++++++++++++++++++
 risu_reginfo_s390x.h |  23 +++++
 risu_s390x.c         |  48 +++++++++++
 risugen_s390x.pm     | 194 +++++++++++++++++++++++++++++++++++++++++++
 s390x.risu           |  48 +++++++++++
 test_s390x.S         |  32 +++++++
 7 files changed, 490 insertions(+), 1 deletion(-)
 create mode 100644 risu_reginfo_s390x.c
 create mode 100644 risu_reginfo_s390x.h
 create mode 100644 risu_s390x.c
 create mode 100644 risugen_s390x.pm
 create mode 100644 s390x.risu
 create mode 100644 test_s390x.S

-- 
2.39.3

[risu PATCH 1/4] s390x: Add basic s390x support to the C code

[Thomas Huth]

With these changes, it is now possible to compile the "risu" binary
for s390x hosts.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 risu_reginfo_s390x.c | 142 +++++++++++++++++++++++++++++++++++++++++++
 risu_reginfo_s390x.h |  23 +++++++
 risu_s390x.c         |  48 +++++++++++++++
 test_s390x.S         |  32 ++++++++++
 4 files changed, 245 insertions(+)
 create mode 100644 risu_reginfo_s390x.c
 create mode 100644 risu_reginfo_s390x.h
 create mode 100644 risu_s390x.c
 create mode 100644 test_s390x.S

diff --git a/risu_reginfo_s390x.c b/risu_reginfo_s390x.c
new file mode 100644
index 0000000..9e118a2
--- /dev/null
+++ b/risu_reginfo_s390x.c
@@ -0,0 +1,142 @@
+/******************************************************************************
+ * Copyright 2023 Red Hat Inc.
+ * 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:
+ *     Thomas Huth - initial implementation
+ *****************************************************************************/
+
+#include <stdio.h>
+#include <ucontext.h>
+#include <string.h>
+#include <math.h>
+#include <stdlib.h>
+#include <sys/user.h>
+
+#include "risu.h"
+#include "risu_reginfo_s390x.h"
+
+
+const struct option * const arch_long_opts;
+const char * const arch_extra_help;
+
+void process_arch_opt(int opt, const char *arg)
+{
+    abort();
+}
+
+void arch_init(void)
+{
+}
+
+int reginfo_size(struct reginfo *ri)
+{
+    return sizeof(*ri);
+}
+
+/* reginfo_init: initialize with a ucontext */
+void reginfo_init(struct reginfo *ri, ucontext_t *uc)
+{
+    int i;
+
+    memset(ri, 0, sizeof(*ri));
+
+    ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.psw.addr);
+    ri->psw_mask = uc->uc_mcontext.psw.mask;
+    ri->psw_addr = uc->uc_mcontext.psw.addr - image_start_address;
+
+    for (i = 0; i < 16; i++) {
+        ri->gregs[i] = uc->uc_mcontext.gregs[i];
+    }
+
+    memcpy(&ri->fpregs, &uc->uc_mcontext.fpregs, sizeof(fpregset_t));
+}
+
+/* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
+int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
+{
+    int i;
+
+    if (m->psw_mask != a->psw_mask || m->psw_addr != a->psw_addr) {
+        return 0;
+    }
+
+    /* Skip return address register and stack register for comparison */
+    for (i = 0; i < 14; i++) {
+        if (m->gregs[i] != a->gregs[i]) {
+            return 0;
+        }
+    }
+
+    if (memcmp(&m->fpregs, &a->fpregs, sizeof(fpregset_t))) {
+        return 0;
+    }
+
+    return 1;
+}
+
+/* reginfo_dump: print state to a stream, returns nonzero on success */
+int reginfo_dump(struct reginfo *ri, FILE * f)
+{
+    int i;
+
+    fprintf(f, "  faulting insn 0x%x\n", ri->faulting_insn);
+    fprintf(f, "  PSW mask      0x%" PRIx64 "\n\n", ri->psw_mask);
+    fprintf(f, "  PSW addr offs 0x%" PRIx64 "\n\n", ri->psw_addr);
+
+    for (i = 0; i < 16/2; i++) {
+        fprintf(f, "\tr%d: %16lx\tr%02d: %16lx\n", i, ri->gregs[i],
+                i + 8, ri->gregs[i + 8]);
+    }
+    fprintf(f, "\n");
+
+    for (i = 0; i < 16/2; i++) {
+        fprintf(f, "\tf%d: %16lx\tf%02d: %16lx\n",
+                i, *(uint64_t *)&ri->fpregs.fprs[i],
+                i + 8, *(uint64_t *)&ri->fpregs.fprs[i + 8]);
+    }
+    fprintf(f, "\tFPC: %8x\n\n", ri->fpregs.fpc);
+
+    return !ferror(f);
+}
+
+int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f)
+{
+    int i;
+
+    if (m->psw_mask != a->psw_mask) {
+        fprintf(f, "Mismatch: PSW mask master: [%016lx] - PSW mask apprentice: [%016lx]\n",
+                m->psw_mask, a->psw_mask);
+    }
+
+    if (m->psw_addr != a->psw_addr) {
+        fprintf(f, "Mismatch: PSW addr offset master: [%016lx] - PSW addr offset apprentice: [%016lx]\n",
+                m->psw_addr, a->psw_addr);
+    }
+
+    /* Skip return address register and stack register for comparison */
+    for (i = 0; i < 14; i++) {
+        if (m->gregs[i] != a->gregs[i]) {
+            fprintf(f, "Mismatch: r%d master: [%016lx] - r%d apprentice: [%016lx]\n",
+                    i, m->gregs[i], i, a->gregs[i]);
+        }
+    }
+
+    for (i = 0; i < 16; i++) {
+        if (*(uint64_t *)&m->fpregs.fprs[i] != *(uint64_t *)&a->fpregs.fprs[i]) {
+            fprintf(f, "Mismatch: f%d master: [%016lx] - f%d apprentice: [%016lx]\n",
+                    i, *(uint64_t *)&m->fpregs.fprs[i],
+                    i, *(uint64_t *)&a->fpregs.fprs[i]);
+        }
+    }
+
+    if (m->fpregs.fpc != a->fpregs.fpc) {
+        fprintf(f, "Mismatch: FPC master: [%08x] - FPC apprentice: [%08x]\n",
+                m->fpregs.fpc, a->fpregs.fpc);
+    }
+
+    return !ferror(f);
+}
diff --git a/risu_reginfo_s390x.h b/risu_reginfo_s390x.h
new file mode 100644
index 0000000..b55a11d
--- /dev/null
+++ b/risu_reginfo_s390x.h
@@ -0,0 +1,23 @@
+/******************************************************************************
+ * Copyright 2023 Red Hat Inc.
+ * 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:
+ *     Thomas Huth - initial implementation
+ *****************************************************************************/
+
+#ifndef RISU_REGINFO_S390X_H
+#define RISU_REGINFO_S390X_H
+
+struct reginfo {
+    uint32_t faulting_insn;
+    uint64_t psw_mask;
+    uint64_t psw_addr;
+    gregset_t gregs;
+    fpregset_t fpregs;
+};
+
+#endif /* RISU_REGINFO_S390X_H */
diff --git a/risu_s390x.c b/risu_s390x.c
new file mode 100644
index 0000000..4a83869
--- /dev/null
+++ b/risu_s390x.c
@@ -0,0 +1,48 @@
+/******************************************************************************
+ * Copyright 2023 Red Hat Inc.
+ * 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:
+ *     Thomas Huth - initial implementation
+ *****************************************************************************/
+
+#include <sys/user.h>
+
+#include "risu.h"
+
+void advance_pc(void *vuc)
+{
+    /*
+     * Note: The PSW address already points to the next instruction
+     * after we get a SIGILL, so we must not advance it here!
+     */
+    // ucontext_t *uc = (ucontext_t *) vuc;
+    // uc->uc_mcontext.psw.addr += 4;
+}
+
+void set_ucontext_paramreg(void *vuc, uint64_t value)
+{
+    ucontext_t *uc = vuc;
+    uc->uc_mcontext.gregs[0] = value;
+}
+
+uint64_t get_reginfo_paramreg(struct reginfo *ri)
+{
+    return ri->gregs[0];
+}
+
+RisuOp get_risuop(struct reginfo *ri)
+{
+    uint32_t insn = ri->faulting_insn;
+    uint32_t op = insn & 0xff;
+    uint32_t key = insn & ~0xff;
+    return (key != 0x835a0f00) ? OP_SIGILL : op;
+}
+
+uintptr_t get_pc(struct reginfo *ri)
+{
+   return ri->psw_addr;
+}
diff --git a/test_s390x.S b/test_s390x.S
new file mode 100644
index 0000000..b67594f
--- /dev/null
+++ b/test_s390x.S
@@ -0,0 +1,32 @@
+/*****************************************************************************
+ * Copyright 2023 Red Hat Inc.
+ * 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:
+ *     Thomas Huth - initial implementation
+ *****************************************************************************/
+
+    /* Initialise the general purpose registers */
+    lgfi %r0, 0
+    lgfi %r1, 0x1111111
+    lgfi %r2, 0x2222222
+    lgfi %r3, 0x3333333
+    lgfi %r4, 0x4444444
+    lgfi %r5, 0x5555555
+    lgfi %r6, 0x6666666
+    lgfi %r7, 0x7777777
+    lgfi %r8, 0x8888888
+    lgfi %r9, 0x9999999
+    lgfi %r10, 0xaaaaaaa
+    lgfi %r11, 0xbbbbbbb
+    lgfi %r12, 0xccccccc
+    lgfi %r13, 0xddddddd
+    lgfi %r14, 0xeeeeeee
+
+    /* do compare */
+    .int 0x835a0f00
+    /* exit test */
+    .int 0x835a0f01
-- 
2.39.3

[risu PATCH 2/4] s390x: Add simple s390x.risu file

[Thomas Huth]

This only adds a limited set of s390x instructions for initial testing.
More instructions will be added later.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 s390x.risu | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 s390x.risu

diff --git a/s390x.risu b/s390x.risu
new file mode 100644
index 0000000..3ad7015
--- /dev/null
+++ b/s390x.risu
@@ -0,0 +1,48 @@
+###############################################################################
+# Copyright 2023 Red Hat Inc.
+# 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:
+#     Thomas Huth - initial implementation
+###############################################################################
+
+.mode s390x
+
+# format:RR Add (register + register, 32 bit)
+AR Z 00011010 r1:4 r2:4
+
+# format:RRE Add (register + register, 64 bit)
+AGR Z 10111001 00001000 00000000 r1:4 r2:4
+
+# format:RRE Add (register + register, 32 bit to 64 bit)
+AGFR Z 10111001 00011000 00000000 r1:4 r2:4
+
+# format:RRF-a Add (three registers, 32 bit)
+ARK STFLE45 10111001 11111000 r3:4 0000 r1:4 r2:4
+
+# format:RRF-a Add (three registers, 64 bit)
+AGRK STFLE45 10111001 11101000 r3:4 0000 r1:4 r2:4
+
+
+# format:RRE Add Halfword Immediate (32 bit)
+AHI Z 10100111 r1:4 1010 i2:16
+
+# format:RI Add Halfword Immediate (64 bit)
+AGHI Z 10100111 r1:4 1011 i2:16
+
+
+# format:RR Add Logical (32 bit)
+ALR Z 00011110 r1:4 r2:4
+
+# format:RRE Add Logical (64 bit)
+ALGR Z 10111001 00001010 00000000 r1:4 r2:4
+
+# format:RRE Add Logical (32 bit to 64 bit)
+ALGFR Z 10111001 00011010 00000000 r1:4 r2:4
+
+
+# format:RRF-c Population Count
+POPCNT STFLE45 10111001 11100001 m3:4 0000 r1:4 r2:4
-- 
2.39.3

[risu PATCH 3/4] s390x: Add basic risugen perl module for s390x

[Thomas Huth]

This implements support for simple 16-bit and 32-bit instructions.
Support for 48-bit instructions and support for load/store memory
instructions is not implemented yet.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 risugen_s390x.pm | 194 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 194 insertions(+)
 create mode 100644 risugen_s390x.pm

diff --git a/risugen_s390x.pm b/risugen_s390x.pm
new file mode 100644
index 0000000..c58d3c1
--- /dev/null
+++ b/risugen_s390x.pm
@@ -0,0 +1,194 @@
+#!/usr/bin/perl -w
+###############################################################################
+# Copyright 2023 Red Hat Inc.
+# 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:
+#     Thomas Huth - initial implementation (based on risugen_ppc64.pm etc.)
+###############################################################################
+
+# risugen -- generate a test binary file for use with risu
+# See 'risugen --help' for usage information.
+package risugen_s390x;
+
+use strict;
+use warnings;
+
+use risugen_common;
+
+require Exporter;
+
+our @ISA    = qw(Exporter);
+our @EXPORT = qw(write_test_code);
+
+my $periodic_reg_random = 1;
+
+# Maximum alignment restriction permitted for a memory op.
+my $MAXALIGN = 64;
+
+sub write_mov_ri($$$)
+{
+    my ($r, $imm_h, $imm_l) = @_;
+
+    # LGFI
+    insn16(0xc0 << 8 | $r << 4 | 0x1);
+    insn32($imm_l);
+    # IIHF r,imm_high
+    insn16(0xc0 << 8 | $r << 4 | 0x8);
+    insn32($imm_h);
+}
+
+sub write_mov_fp($$)
+{
+    my ($r, $imm) = @_;
+
+    write_mov_ri(0, ~$imm, $imm);
+    # LDGR
+    insn32(0xb3c1 << 16 | $r << 4);
+}
+
+sub write_random_regdata()
+{
+    # Floating point registers
+    for (my $i = 0; $i < 16; $i++) {
+        write_mov_fp($i, rand(0xffffffff));
+    }
+
+    # Load FPC (via r0)
+    write_mov_ri(0, 0, (rand(0xffffffff) & 0xfcfcff77));
+    insn32(0xb3840000);
+
+    # general purpose registers (except return addr in r14 and the stack in r15)
+    for (my $i = 0; $i < 14; $i++) {
+        write_mov_ri($i, rand(0xffffffff), rand(0xffffffff));
+    }
+}
+
+my $OP_COMPARE = 0;        # compare registers
+my $OP_TESTEND = 1;        # end of test, stop
+
+sub write_random_register_data()
+{
+    write_random_regdata();
+    write_risuop($OP_COMPARE);
+}
+
+sub gen_one_insn($$)
+{
+    # Given an instruction-details array, generate an instruction
+    my $constraintfailures = 0;
+
+    INSN: while(1) {
+        my ($forcecond, $rec) = @_;
+        my $insn = int(rand(0xffffffff));
+        my $insnname = $rec->{name};
+        my $insnwidth = $rec->{width};
+        my $fixedbits = $rec->{fixedbits};
+        my $fixedbitmask = $rec->{fixedbitmask};
+        my $constraint = $rec->{blocks}{"constraints"};
+        my $memblock = $rec->{blocks}{"memory"};
+
+        $insn &= ~$fixedbitmask;
+        $insn |= $fixedbits;
+
+        for my $tuple (@{ $rec->{fields} }) {
+            my ($var, $pos, $mask) = @$tuple;
+            my $val = ($insn >> $pos) & $mask;
+            # Check constraints here: Do not allow to use or modify
+            # the return address (r14) or stack pointer (r15)
+            next INSN if ($var =~ /^r/ && (($val == 14) || ($val == 15)));
+        }
+
+        if (defined $constraint) {
+            # user-specified constraint: evaluate in an environment
+            # with variables set corresponding to the variable fields.
+            my $v = eval_with_fields($insnname, $insn, $rec, "constraints", $constraint);
+            if (!$v) {
+                $constraintfailures++;
+                if ($constraintfailures > 10000) {
+                    print "10000 consecutive constraint failures for $insnname constraints string:\n$constraint\n";
+                    exit (1);
+                }
+                next INSN;
+            }
+        }
+
+        # OK, we got a good one
+        $constraintfailures = 0;
+
+        my $basereg;
+
+        if (defined $memblock) {
+            die "memblock handling has not been implemented yet."
+        }
+
+        if ($insnwidth == 16) {
+            insn16(($insn >> 16) & 0xffff);
+        } else {
+            insn32($insn);
+        }
+
+        return;
+    }
+}
+
+sub write_risuop($)
+{
+    my ($op) = @_;
+    insn32(0x835a0f00 | $op);
+}
+
+sub write_test_code($)
+{
+    my ($params) = @_;
+
+    my $condprob = $params->{ 'condprob' };
+    my $numinsns = $params->{ 'numinsns' };
+    my $outfile = $params->{ 'outfile' };
+
+    my %insn_details = %{ $params->{ 'details' } };
+    my @keys = @{ $params->{ 'keys' } };
+
+    set_endian(1);
+
+    open_bin($outfile);
+
+    # convert from probability that insn will be conditional to
+    # probability of forcing insn to unconditional
+    $condprob = 1 - $condprob;
+
+    # TODO better random number generator?
+    srand(0);
+
+    print "Generating code using patterns: @keys...\n";
+    progress_start(78, $numinsns);
+
+    if (grep { defined($insn_details{$_}->{blocks}->{"memory"}) } @keys) {
+        write_memblock_setup();
+    }
+
+    # memblock setup doesn't clean its registers, so this must come afterwards.
+    write_random_register_data();
+
+    for my $i (1..$numinsns) {
+        my $insn_enc = $keys[int rand (@keys)];
+        #dump_insn_details($insn_enc, $insn_details{$insn_enc});
+        my $forcecond = (rand() < $condprob) ? 1 : 0;
+        gen_one_insn($forcecond, $insn_details{$insn_enc});
+        write_risuop($OP_COMPARE);
+        # Rewrite the registers periodically. This avoids the tendency
+        # for the VFP registers to decay to NaNs and zeroes.
+        if ($periodic_reg_random && ($i % 100) == 0) {
+            write_random_register_data();
+        }
+        progress_update($i);
+    }
+    write_risuop($OP_TESTEND);
+    progress_end();
+    close_bin();
+}
+
+1;
-- 
2.39.3

[risu PATCH 4/4] s390x: Update the configure script for s390x support

[Thomas Huth]

Auto-detect s390x hosts and add s390x information to the help text.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index ca2d7db..2f7c580 100755
--- a/configure
+++ b/configure
@@ -58,6 +58,8 @@ guess_arch() {
         ARCH="m68k"
     elif check_define __powerpc64__ ; then
         ARCH="ppc64"
+    elif check_define __s390x__ ; then
+        ARCH="s390x"
     else
         echo "This cpu is not supported by risu. Try -h. " >&2
         exit 1
@@ -139,7 +141,7 @@ Some influential environment variables:
                prefixed with the given string.
 
   ARCH         force target architecture instead of trying to detect it.
-               Valid values=[arm|aarch64|ppc64|ppc64le|m68k]
+               Valid values=[arm|aarch64|m68k|ppc64|ppc64le|s390x]
 
   CC           C compiler command
   CFLAGS       C compiler flags
-- 
2.39.3

Re: [risu PATCH 1/4] s390x: Add basic s390x support to the C code

[Ilya Leoshkevich]

On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
> With these changes, it is now possible to compile the "risu" binary
> for s390x hosts.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  risu_reginfo_s390x.c | 142
> +++++++++++++++++++++++++++++++++++++++++++
>  risu_reginfo_s390x.h |  23 +++++++
>  risu_s390x.c         |  48 +++++++++++++++
>  test_s390x.S         |  32 ++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 risu_reginfo_s390x.c
>  create mode 100644 risu_reginfo_s390x.h
>  create mode 100644 risu_s390x.c
>  create mode 100644 test_s390x.S

Looks really interesting! I was doing similar qemu-system-s390x testing
with a bunch of ad-hoc scripts, and there are quite a few unresolved
problems still, especially in the error handling area.

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

I have one small question (see below).

[...]

> +/* reginfo_is_eq: compare the reginfo structs, returns nonzero if
> equal */
> +int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
> +{
> +    int i;
> +
> +    if (m->psw_mask != a->psw_mask || m->psw_addr != a->psw_addr) {
> +        return 0;
> +    }
> +
> +    /* Skip return address register and stack register for
> comparison */

Is this because of ASLR? In this case, would it be possible to build a
non-PIE binary and switch to a private stack at the beginning? This
could be useful for the other architectures as well.

[...]

Re: [risu PATCH 2/4] s390x: Add simple s390x.risu file

[Ilya Leoshkevich]

On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
> This only adds a limited set of s390x instructions for initial
> testing.
> More instructions will be added later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  s390x.risu | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 s390x.risu

Can this be somehow automatically derived from
target/s390x/tcg/insn-data.h.inc?

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

Re: [risu PATCH 4/4] s390x: Update the configure script for s390x support

[Philippe Mathieu-Daudé]

On 4/9/23 16:00, Thomas Huth wrote:
> Auto-detect s390x hosts and add s390x information to the help text.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   configure | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [risu PATCH 1/4] s390x: Add basic s390x support to the C code

[Thomas Huth]

On 04/09/2023 16.19, Ilya Leoshkevich wrote:
> On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
>> With these changes, it is now possible to compile the "risu" binary
>> for s390x hosts.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   risu_reginfo_s390x.c | 142
>> +++++++++++++++++++++++++++++++++++++++++++
>>   risu_reginfo_s390x.h |  23 +++++++
>>   risu_s390x.c         |  48 +++++++++++++++
>>   test_s390x.S         |  32 ++++++++++
>>   4 files changed, 245 insertions(+)
>>   create mode 100644 risu_reginfo_s390x.c
>>   create mode 100644 risu_reginfo_s390x.h
>>   create mode 100644 risu_s390x.c
>>   create mode 100644 test_s390x.S
> 
> Looks really interesting! I was doing similar qemu-system-s390x testing
> with a bunch of ad-hoc scripts, and there are quite a few unresolved
> problems still, especially in the error handling area.
> 
> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> I have one small question (see below).
> 
> [...]
> 
>> +/* reginfo_is_eq: compare the reginfo structs, returns nonzero if
>> equal */
>> +int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
>> +{
>> +    int i;
>> +
>> +    if (m->psw_mask != a->psw_mask || m->psw_addr != a->psw_addr) {
>> +        return 0;
>> +    }
>> +
>> +    /* Skip return address register and stack register for
>> comparison */
> 
> Is this because of ASLR? In this case, would it be possible to build a
> non-PIE binary and switch to a private stack at the beginning? This
> could be useful for the other architectures as well.

Ah, no, it's not due to ASLR ... I just experienced some crashes during 
development and saw that other targets like m68k skip the stack pointer 
here, too, so I did it the same way on s390x.
(But I finally discovered that the crashes were due to other reasons that I 
then fixed)

Looking through the code, it seems like there is already support for an 
alternate signal handler stack:

 
https://git.linaro.org/people/peter.maydell/risu.git/commit/?id=ad82a069e8d6a21842bbb265

... maybe it's working on s390x, too ... I'll give it a try.

  Thomas

Re: [risu PATCH 0/4] Add support for s390x to RISU

[Ilya Leoshkevich]

On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
>  Hi Peter!
> 
> Here are some patches that add basic support for s390x to RISU.
> It's still quite limited, e.g. no support for load/store memory
> operations yet, but the basics with simple 16-bit or 32-bit
> instructions work already fine.
> 
> (In the long run, we'd need to support instructions with 48-bit
> length on s390x, too, since most newer "interesting" instructions
> like e.g. vector SIMD instructions are encoded with 48 bit. This
> will require modifications to the generic code, too, so I limited
> my initial implementation to 16-bit and 32-bit instruction length
> support to keep the code self-contained in the s390x architecture
> specific files)

What's also interesting about SIMD, is that floating-point instructions
clobber the upper parts of vector registers. I wonder if there is a way
to systematically solve this? In my scripts the solution isn't pretty:

            insn = gdb.execute("x/i $pc", to_string=True)
            print(insn)
            gdb.execute("stepi")
            if "%f" in insn:
               [ Skip comparison ]

I think there are also a few cases in non-SIMD areas, where PoP
basically says "if conditions X and Y hold, the output is
unpredictable".


One other thing - for not-so-near future - is it possible to integrate
this with coverage-based fuzzers? I.e., somehow generate the
instructions based on the coverage signal. Maybe even make sure that
the signal comes from JITed code too. I wanted to try AFLplusplus in
QEMU mode for this purpose (which would ultimately run QEMU in QEMU),
but never found the time.

[...]

Re: [risu PATCH 2/4] s390x: Add simple s390x.risu file

[Thomas Huth]

On 04/09/2023 16.20, Ilya Leoshkevich wrote:
> On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
>> This only adds a limited set of s390x instructions for initial
>> testing.
>> More instructions will be added later.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   s390x.risu | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100644 s390x.risu
> 
> Can this be somehow automatically derived from
> target/s390x/tcg/insn-data.h.inc?

Hmm, maybe ... OTOH, if something is wrong in that file, you won't find the 
bug with RISU is you used the same source, I guess...

> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

Thanks!

  Thomas

Re: [risu PATCH 0/4] Add support for s390x to RISU

[Thomas Huth]

On 04/09/2023 16.30, Ilya Leoshkevich wrote:
> On Mon, 2023-09-04 at 16:00 +0200, Thomas Huth wrote:
>>   Hi Peter!
>>
>> Here are some patches that add basic support for s390x to RISU.
>> It's still quite limited, e.g. no support for load/store memory
>> operations yet, but the basics with simple 16-bit or 32-bit
>> instructions work already fine.
>>
>> (In the long run, we'd need to support instructions with 48-bit
>> length on s390x, too, since most newer "interesting" instructions
>> like e.g. vector SIMD instructions are encoded with 48 bit. This
>> will require modifications to the generic code, too, so I limited
>> my initial implementation to 16-bit and 32-bit instruction length
>> support to keep the code self-contained in the s390x architecture
>> specific files)
> 
> What's also interesting about SIMD, is that floating-point instructions
> clobber the upper parts of vector registers. I wonder if there is a way
> to systematically solve this?#

No clue yet, so far the code does not support the extended vector registers 
yet (since the weren't part of the information that is provided by the 
ucontext.h header file).

I guess it should be OK to check only the floating point part for the 
registers where it overlaps, and only check the full vector register if the 
register does not overlap ... I don't expect much difference for a vector 
instruction when it executes with register 0 - 15 compared to when it 
executes with register 16 - 31, so skipping half of the check for register 0 
- 15 shouldn't be too bad.

> One other thing - for not-so-near future - is it possible to integrate
> this with coverage-based fuzzers? I.e., somehow generate the
> instructions based on the coverage signal. Maybe even make sure that
> the signal comes from JITed code too. I wanted to try AFLplusplus in
> QEMU mode for this purpose (which would ultimately run QEMU in QEMU),
> but never found the time.

I don't think this is possible yet, but maybe it's be possible to write a 
TCG plugin for QEMU to dump the executed instructions into an input file for 
risu?

  Thomas