[PATCH v4 0/4] target/s390x: Emulate CVDG and CVB*

[PATCH v4 0/4] target/s390x: Emulate CVDG and CVB*

[Ilya Leoshkevich]

v3: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg06664.html
v3 -> v4: Implement CVB error handling (David/Thomas).

v2: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg05048.html
v2 -> v3: Resurrect an old CVB* patch (Thomas).
          Add Richard's R-b.

v1: https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02865.html
v1 -> v2: Fix !CONFIG_INT128 builds (Richard).

Hi,

Ido reported that we are missing the CVDG emulation (which is very
similar to the existing CVD emulation). This series adds it along with
a test.

Best regards,
Ilya

Ilya Leoshkevich (4):
  target/s390x: Emulate CVDG
  target/s390x: Emulate CVB, CVBY and CVBG
  tests/tcg/s390x: Test CONVERT TO DECIMAL
  tests/tcg/s390x: Test CONVERT TO BINARY

 target/s390x/helper.h            |   3 +
 target/s390x/tcg/insn-data.h.inc |   5 ++
 target/s390x/tcg/int_helper.c    |  93 ++++++++++++++++++++++++++++
 target/s390x/tcg/translate.c     |  24 ++++++++
 tests/tcg/s390x/Makefile.target  |   2 +
 tests/tcg/s390x/cvb.c            | 102 +++++++++++++++++++++++++++++++
 tests/tcg/s390x/cvd.c            |  63 +++++++++++++++++++
 7 files changed, 292 insertions(+)
 create mode 100644 tests/tcg/s390x/cvb.c
 create mode 100644 tests/tcg/s390x/cvd.c

-- 
2.43.0

[PATCH v4 1/4] target/s390x: Emulate CVDG

[Ilya Leoshkevich]

CVDG is the same as CVD, except that it converts 64 bits into 128,
rather than 32 into 64. Create a new helper, which uses Int128
wrappers.

Reported-by: Ido Plat <Ido.Plat@ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/helper.h            |  1 +
 target/s390x/tcg/insn-data.h.inc |  1 +
 target/s390x/tcg/int_helper.c    | 21 +++++++++++++++++++++
 target/s390x/tcg/translate.c     |  8 ++++++++
 4 files changed, 31 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 05102578fc9..332a9a9c632 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -89,6 +89,7 @@ DEF_HELPER_FLAGS_2(sqeb, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_2(sqdb, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_2(sqxb, TCG_CALL_NO_WG, i128, env, i128)
 DEF_HELPER_FLAGS_1(cvd, TCG_CALL_NO_RWG_SE, i64, s32)
+DEF_HELPER_FLAGS_1(cvdg, TCG_CALL_NO_RWG_SE, i128, s64)
 DEF_HELPER_FLAGS_4(pack, TCG_CALL_NO_WG, void, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(pka, TCG_CALL_NO_WG, void, env, i64, i64, i32)
 DEF_HELPER_FLAGS_4(pku, TCG_CALL_NO_WG, void, env, i64, i64, i32)
diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 2f07f39d9cb..388dcb8dbbc 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -296,6 +296,7 @@
 /* CONVERT TO DECIMAL */
     C(0x4e00, CVD,     RX_a,  Z,   r1_o, a2, 0, 0, cvd, 0)
     C(0xe326, CVDY,    RXY_a, LD,  r1_o, a2, 0, 0, cvd, 0)
+    C(0xe32e, CVDG,    RXY_a, Z,   r1_o, a2, 0, 0, cvdg, 0)
 /* CONVERT TO FIXED */
     F(0xb398, CFEBR,   RRF_e, Z,   0, e2, new, r1_32, cfeb, 0, IF_BFP)
     F(0xb399, CFDBR,   RRF_e, Z,   0, f2, new, r1_32, cfdb, 0, IF_BFP)
diff --git a/target/s390x/tcg/int_helper.c b/target/s390x/tcg/int_helper.c
index eb8e6dd1b57..121e3006a65 100644
--- a/target/s390x/tcg/int_helper.c
+++ b/target/s390x/tcg/int_helper.c
@@ -118,6 +118,27 @@ uint64_t HELPER(cvd)(int32_t reg)
     return dec;
 }
 
+Int128 HELPER(cvdg)(int64_t reg)
+{
+    /* positive 0 */
+    Int128 dec = int128_make64(0x0c);
+    Int128 bin = int128_makes64(reg);
+    Int128 base = int128_make64(10);
+    int shift;
+
+    if (!int128_nonneg(bin)) {
+        bin = int128_neg(bin);
+        dec = int128_make64(0x0d);
+    }
+
+    for (shift = 4; (shift < 128) && int128_nz(bin); shift += 4) {
+        dec = int128_or(dec, int128_lshift(int128_remu(bin, base), shift));
+        bin = int128_divu(bin, base);
+    }
+
+    return dec;
+}
+
 uint64_t HELPER(popcnt)(uint64_t val)
 {
     /* Note that we don't fold past bytes. */
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index a5fd9cccaa5..c2fdc920a50 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2233,6 +2233,14 @@ static DisasJumpType op_cvd(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
+static DisasJumpType op_cvdg(DisasContext *s, DisasOps *o)
+{
+    TCGv_i128 t = tcg_temp_new_i128();
+    gen_helper_cvdg(t, o->in1);
+    tcg_gen_qemu_st_i128(t, o->in2, get_mem_index(s), MO_TE | MO_128);
+    return DISAS_NEXT;
+}
+
 static DisasJumpType op_ct(DisasContext *s, DisasOps *o)
 {
     int m3 = get_field(s, m3);
-- 
2.43.0

[PATCH v4 2/4] target/s390x: Emulate CVB, CVBY and CVBG

[Ilya Leoshkevich]

Convert to Binary - counterparts of the already implemented Convert
to Decimal (CVD*) instructions.
Example from the Principles of Operation: 25594C becomes 63FA.

Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/helper.h            |  2 +
 target/s390x/tcg/insn-data.h.inc |  4 ++
 target/s390x/tcg/int_helper.c    | 72 ++++++++++++++++++++++++++++++++
 target/s390x/tcg/translate.c     | 16 +++++++
 4 files changed, 94 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 332a9a9c632..cc1c20e9e3f 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -88,6 +88,8 @@ DEF_HELPER_FLAGS_3(tcxb, TCG_CALL_NO_RWG_SE, i32, env, i128, i64)
 DEF_HELPER_FLAGS_2(sqeb, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_2(sqdb, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_2(sqxb, TCG_CALL_NO_WG, i128, env, i128)
+DEF_HELPER_3(cvb, void, env, i32, i64)
+DEF_HELPER_FLAGS_2(cvbg, TCG_CALL_NO_WG, i64, env, i128)
 DEF_HELPER_FLAGS_1(cvd, TCG_CALL_NO_RWG_SE, i64, s32)
 DEF_HELPER_FLAGS_1(cvdg, TCG_CALL_NO_RWG_SE, i128, s64)
 DEF_HELPER_FLAGS_4(pack, TCG_CALL_NO_WG, void, env, i32, i64, i64)
diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 388dcb8dbbc..e7d61cdec28 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -293,6 +293,10 @@
     D(0xec73, CLFIT,   RIE_a, GIE, r1_32u, i2_16u, 0, 0, ct, 0, 1)
     D(0xec71, CLGIT,   RIE_a, GIE, r1_o, i2_16u, 0, 0, ct, 0, 1)
 
+/* CONVERT TO BINARY */
+    C(0x4f00, CVB,     RX_a,  Z,   la2, 0, 0, 0, cvb, 0)
+    C(0xe306, CVBY,    RXY_a, LD,  la2, 0, 0, 0, cvb, 0)
+    C(0xe30e, CVBG,    RXY_a, Z,   la2, 0, r1, 0, cvbg, 0)
 /* CONVERT TO DECIMAL */
     C(0x4e00, CVD,     RX_a,  Z,   r1_o, a2, 0, 0, cvd, 0)
     C(0xe326, CVDY,    RXY_a, LD,  r1_o, a2, 0, 0, cvd, 0)
diff --git a/target/s390x/tcg/int_helper.c b/target/s390x/tcg/int_helper.c
index 121e3006a65..17974375e98 100644
--- a/target/s390x/tcg/int_helper.c
+++ b/target/s390x/tcg/int_helper.c
@@ -25,6 +25,7 @@
 #include "exec/exec-all.h"
 #include "qemu/host-utils.h"
 #include "exec/helper-proto.h"
+#include "exec/cpu_ldst.h"
 
 /* #define DEBUG_HELPER */
 #ifdef DEBUG_HELPER
@@ -98,6 +99,77 @@ Int128 HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al, uint64_t b)
     tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
 }
 
+void HELPER(cvb)(CPUS390XState *env, uint32_t r1, uint64_t dec)
+{
+    int64_t pow10 = 1, bin = 0;
+    int digit, sign;
+
+    sign = dec & 0xf;
+    if (sign < 0xa) {
+        tcg_s390_data_exception(env, 0, GETPC());
+    }
+    dec >>= 4;
+
+    while (dec) {
+        digit = dec & 0xf;
+        if (digit > 0x9) {
+            tcg_s390_data_exception(env, 0, GETPC());
+        }
+        dec >>= 4;
+        bin += digit * pow10;
+        pow10 *= 10;
+    }
+
+    if (sign == 0xb || sign == 0xd) {
+        bin = -bin;
+    }
+
+    /* R1 is updated even on fixed-point-divide exception. */
+    env->regs[r1] = (env->regs[r1] & 0xffffffff00000000ULL) | (uint32_t)bin;
+    if (bin != (int32_t)bin) {
+        tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
+    }
+}
+
+uint64_t HELPER(cvbg)(CPUS390XState *env, Int128 dec)
+{
+    uint64_t dec64[] = {int128_getlo(dec), int128_gethi(dec)};
+    int64_t bin = 0, pow10, tmp;
+    int digit, i, sign;
+
+    sign = dec64[0] & 0xf;
+    if (sign < 0xa) {
+        tcg_s390_data_exception(env, 0, GETPC());
+    }
+    dec64[0] >>= 4;
+    pow10 = (sign == 0xb || sign == 0xd) ? -1 : 1;
+
+    for (i = 1; i < 20; i++) {
+        digit = dec64[i >> 4] & 0xf;
+        if (digit > 0x9) {
+            tcg_s390_data_exception(env, 0, GETPC());
+        }
+        dec64[i >> 4] >>= 4;
+        tmp = pow10 * digit;
+        if (digit && ((tmp ^ pow10) < 0)) {
+            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
+        }
+        tmp = bin + tmp;
+        if (bin && ((tmp ^ bin) < 0)) {
+            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
+        }
+        bin = tmp;
+        pow10 *= 10;
+    }
+
+    g_assert(!dec64[0]);
+    if (dec64[1]) {
+        tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
+    }
+
+    return bin;
+}
+
 uint64_t HELPER(cvd)(int32_t reg)
 {
     /* positive 0 */
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index c2fdc920a50..325b25959d3 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -2223,6 +2223,22 @@ static DisasJumpType op_csp(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static DisasJumpType op_cvb(DisasContext *s, DisasOps *o)
+{
+    TCGv_i64 t = tcg_temp_new_i64();
+    tcg_gen_qemu_ld_i64(t, o->addr1, get_mem_index(s), MO_TEUQ);
+    gen_helper_cvb(tcg_env, tcg_constant_i32(get_field(s, r1)), t);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_cvbg(DisasContext *s, DisasOps *o)
+{
+    TCGv_i128 t = tcg_temp_new_i128();
+    tcg_gen_qemu_ld_i128(t, o->addr1, get_mem_index(s), MO_TE | MO_128);
+    gen_helper_cvbg(o->out, tcg_env, t);
+    return DISAS_NEXT;
+}
+
 static DisasJumpType op_cvd(DisasContext *s, DisasOps *o)
 {
     TCGv_i64 t1 = tcg_temp_new_i64();
-- 
2.43.0

[PATCH v4 3/4] tests/tcg/s390x: Test CONVERT TO DECIMAL

[Ilya Leoshkevich]

Check the CVD's, CVDY's, and CVDG's corner cases.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target |  1 +
 tests/tcg/s390x/cvd.c           | 63 +++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tests/tcg/s390x/cvd.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 30994dcf9c2..04e4bddd83d 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -45,6 +45,7 @@ TESTS+=clc
 TESTS+=laalg
 TESTS+=add-logical-with-carry
 TESTS+=lae
+TESTS+=cvd
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/cvd.c b/tests/tcg/s390x/cvd.c
new file mode 100644
index 00000000000..d776688985e
--- /dev/null
+++ b/tests/tcg/s390x/cvd.c
@@ -0,0 +1,63 @@
+/*
+ * Test the CONVERT TO DECIMAL instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+static uint64_t cvd(int32_t x)
+{
+    uint64_t ret;
+
+    asm("cvd %[x],%[ret]" : [ret] "=R" (ret) : [x] "r" (x));
+
+    return ret;
+}
+
+static uint64_t cvdy(int32_t x)
+{
+    uint64_t ret;
+
+    asm("cvdy %[x],%[ret]" : [ret] "=T" (ret) : [x] "r" (x));
+
+    return ret;
+}
+
+static __uint128_t cvdg(int64_t x)
+{
+    __uint128_t ret;
+
+    asm("cvdg %[x],%[ret]" : [ret] "=T" (ret) : [x] "r" (x));
+
+    return ret;
+}
+
+int main(void)
+{
+    __uint128_t m = (((__uint128_t)0x9223372036854775) << 16) | 0x8070;
+
+    assert(cvd(0) == 0xc);
+    assert(cvd(1) == 0x1c);
+    assert(cvd(25594) == 0x25594c);
+    assert(cvd(-1) == 0x1d);
+    assert(cvd(0x7fffffff) == 0x2147483647c);
+    assert(cvd(-0x80000000) == 0x2147483648d);
+
+    assert(cvdy(0) == 0xc);
+    assert(cvdy(1) == 0x1c);
+    assert(cvdy(25594) == 0x25594c);
+    assert(cvdy(-1) == 0x1d);
+    assert(cvdy(0x7fffffff) == 0x2147483647c);
+    assert(cvdy(-0x80000000) == 0x2147483648d);
+
+    assert(cvdg(0) == 0xc);
+    assert(cvdg(1) == 0x1c);
+    assert(cvdg(25594) == 0x25594c);
+    assert(cvdg(-1) == 0x1d);
+    assert(cvdg(0x7fffffffffffffff) == (m + 0xc));
+    assert(cvdg(-0x8000000000000000) == (m + 0x1d));
+
+    return EXIT_SUCCESS;
+}
-- 
2.43.0

[PATCH v4 4/4] tests/tcg/s390x: Test CONVERT TO BINARY

[Ilya Leoshkevich]

Check the CVB's, CVBY's, and CVBG's corner cases.

Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target |   1 +
 tests/tcg/s390x/cvb.c           | 102 ++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 tests/tcg/s390x/cvb.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 04e4bddd83d..e2aba2ec274 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -46,6 +46,7 @@ TESTS+=laalg
 TESTS+=add-logical-with-carry
 TESTS+=lae
 TESTS+=cvd
+TESTS+=cvb
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/cvb.c b/tests/tcg/s390x/cvb.c
new file mode 100644
index 00000000000..e1735f6b81c
--- /dev/null
+++ b/tests/tcg/s390x/cvb.c
@@ -0,0 +1,102 @@
+/*
+ * Test the CONVERT TO BINARY instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+static int signum;
+
+static void signal_handler(int n)
+{
+    signum = n;
+}
+
+#define FAIL 0x1234567887654321
+#define OK32(x) (0x1234567800000000 | (uint32_t)(x))
+
+static int64_t cvb(uint64_t x)
+{
+    int64_t ret = FAIL;
+
+    signum = -1;
+    asm("cvb %[ret],%[x]" : [ret] "+r" (ret) : [x] "R" (x));
+
+    return ret;
+}
+
+static int64_t cvby(uint64_t x)
+{
+    int64_t ret = FAIL;
+
+    signum = -1;
+    asm("cvby %[ret],%[x]" : [ret] "+r" (ret) : [x] "T" (x));
+
+    return ret;
+}
+
+static int64_t cvbg(__uint128_t x)
+{
+    int64_t ret = FAIL;
+
+    signum = -1;
+    asm("cvbg %[ret],%[x]" : [ret] "+r" (ret) : [x] "T" (x));
+
+    return ret;
+}
+
+int main(void)
+{
+    __uint128_t m = (((__uint128_t)0x9223372036854775) << 16) | 0x8070;
+    struct sigaction act;
+    int err;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_handler = signal_handler;
+    err = sigaction(SIGFPE, &act, NULL);
+    assert(err == 0);
+    err = sigaction(SIGILL, &act, NULL);
+    assert(err == 0);
+
+    assert(cvb(0xc) == OK32(0) && signum == -1);
+    assert(cvb(0x1c) == OK32(1) && signum == -1);
+    assert(cvb(0x25594c) == OK32(25594) && signum == -1);
+    assert(cvb(0x1d) == OK32(-1) && signum == -1);
+    assert(cvb(0x2147483647c) == OK32(0x7fffffff) && signum == -1);
+    assert(cvb(0x2147483648d) == OK32(-0x80000000) && signum == -1);
+    assert(cvb(0x7) == FAIL && signum == SIGILL);
+    assert(cvb(0x2147483648c) == OK32(0x80000000) && signum == SIGFPE);
+    assert(cvb(0x3000000000c) == OK32(0xb2d05e00) && signum == SIGFPE);
+    assert(cvb(0x2147483649d) == OK32(0x7fffffff) && signum == SIGFPE);
+    assert(cvb(0x3000000000d) == OK32(0x4d2fa200) && signum == SIGFPE);
+
+    assert(cvby(0xc) == OK32(0));
+    assert(cvby(0x1c) == OK32(1));
+    assert(cvby(0x25594c) == OK32(25594));
+    assert(cvby(0x1d) == OK32(-1));
+    assert(cvby(0x2147483647c) == OK32(0x7fffffff));
+    assert(cvby(0x2147483648d) == OK32(-0x80000000));
+    assert(cvby(0x7) == FAIL && signum == SIGILL);
+    assert(cvby(0x2147483648c) == OK32(0x80000000) && signum == SIGFPE);
+    assert(cvby(0x3000000000c) == OK32(0xb2d05e00) && signum == SIGFPE);
+    assert(cvby(0x2147483649d) == OK32(0x7fffffff) && signum == SIGFPE);
+    assert(cvby(0x3000000000d) == OK32(0x4d2fa200) && signum == SIGFPE);
+
+    assert(cvbg(0xc) == 0);
+    assert(cvbg(0x1c) == 1);
+    assert(cvbg(0x25594c) == 25594);
+    assert(cvbg(0x1d) == -1);
+    assert(cvbg(m + 0xc) == 0x7fffffffffffffff);
+    assert(cvbg(m + 0x1d) == -0x8000000000000000);
+    assert(cvbg(0x7) == FAIL && signum == SIGILL);
+    assert(cvbg(m + 0x1c) == FAIL && signum == SIGFPE);
+    assert(cvbg(m + 0x2d) == FAIL && signum == SIGFPE);
+    assert(cvbg(((__uint128_t)1 << 80) + 0xc) == FAIL && signum == SIGFPE);
+    assert(cvbg(((__uint128_t)1 << 80) + 0xd) == FAIL && signum == SIGFPE);
+
+    return EXIT_SUCCESS;
+}
-- 
2.43.0

Re: [PATCH v4 3/4] tests/tcg/s390x: Test CONVERT TO DECIMAL

[Thomas Huth]

On 02/02/2024 15.11, Ilya Leoshkevich wrote:
> Check the CVD's, CVDY's, and CVDG's corner cases.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/tcg/s390x/Makefile.target |  1 +
>   tests/tcg/s390x/cvd.c           | 63 +++++++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+)
>   create mode 100644 tests/tcg/s390x/cvd.c

Thanks for adding CVDY here, too!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 4/4] tests/tcg/s390x: Test CONVERT TO BINARY

[Thomas Huth]

On 02/02/2024 15.12, Ilya Leoshkevich wrote:
> Check the CVB's, CVBY's, and CVBG's corner cases.
> 
> Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   tests/tcg/s390x/Makefile.target |   1 +
>   tests/tcg/s390x/cvb.c           | 102 ++++++++++++++++++++++++++++++++
>   2 files changed, 103 insertions(+)
>   create mode 100644 tests/tcg/s390x/cvb.c


Reviewed-by: Thomas Huth <thuth@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 2/4] target/s390x: Emulate CVB, CVBY and CVBG

[Thomas Huth]

On 02/02/2024 15.11, Ilya Leoshkevich wrote:
> Convert to Binary - counterparts of the already implemented Convert
> to Decimal (CVD*) instructions.
> Example from the Principles of Operation: 25594C becomes 63FA.
> 
> Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/helper.h            |  2 +
>   target/s390x/tcg/insn-data.h.inc |  4 ++
>   target/s390x/tcg/int_helper.c    | 72 ++++++++++++++++++++++++++++++++
>   target/s390x/tcg/translate.c     | 16 +++++++
>   4 files changed, 94 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 332a9a9c632..cc1c20e9e3f 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -88,6 +88,8 @@ DEF_HELPER_FLAGS_3(tcxb, TCG_CALL_NO_RWG_SE, i32, env, i128, i64)
>   DEF_HELPER_FLAGS_2(sqeb, TCG_CALL_NO_WG, i64, env, i64)
>   DEF_HELPER_FLAGS_2(sqdb, TCG_CALL_NO_WG, i64, env, i64)
>   DEF_HELPER_FLAGS_2(sqxb, TCG_CALL_NO_WG, i128, env, i128)
> +DEF_HELPER_3(cvb, void, env, i32, i64)
> +DEF_HELPER_FLAGS_2(cvbg, TCG_CALL_NO_WG, i64, env, i128)
>   DEF_HELPER_FLAGS_1(cvd, TCG_CALL_NO_RWG_SE, i64, s32)
>   DEF_HELPER_FLAGS_1(cvdg, TCG_CALL_NO_RWG_SE, i128, s64)
>   DEF_HELPER_FLAGS_4(pack, TCG_CALL_NO_WG, void, env, i32, i64, i64)
> diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
> index 388dcb8dbbc..e7d61cdec28 100644
> --- a/target/s390x/tcg/insn-data.h.inc
> +++ b/target/s390x/tcg/insn-data.h.inc
> @@ -293,6 +293,10 @@
>       D(0xec73, CLFIT,   RIE_a, GIE, r1_32u, i2_16u, 0, 0, ct, 0, 1)
>       D(0xec71, CLGIT,   RIE_a, GIE, r1_o, i2_16u, 0, 0, ct, 0, 1)
>   
> +/* CONVERT TO BINARY */
> +    C(0x4f00, CVB,     RX_a,  Z,   la2, 0, 0, 0, cvb, 0)
> +    C(0xe306, CVBY,    RXY_a, LD,  la2, 0, 0, 0, cvb, 0)
> +    C(0xe30e, CVBG,    RXY_a, Z,   la2, 0, r1, 0, cvbg, 0)
>   /* CONVERT TO DECIMAL */
>       C(0x4e00, CVD,     RX_a,  Z,   r1_o, a2, 0, 0, cvd, 0)
>       C(0xe326, CVDY,    RXY_a, LD,  r1_o, a2, 0, 0, cvd, 0)
> diff --git a/target/s390x/tcg/int_helper.c b/target/s390x/tcg/int_helper.c
> index 121e3006a65..17974375e98 100644
> --- a/target/s390x/tcg/int_helper.c
> +++ b/target/s390x/tcg/int_helper.c
> @@ -25,6 +25,7 @@
>   #include "exec/exec-all.h"
>   #include "qemu/host-utils.h"
>   #include "exec/helper-proto.h"
> +#include "exec/cpu_ldst.h"
>   
>   /* #define DEBUG_HELPER */
>   #ifdef DEBUG_HELPER
> @@ -98,6 +99,77 @@ Int128 HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al, uint64_t b)
>       tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
>   }
>   
> +void HELPER(cvb)(CPUS390XState *env, uint32_t r1, uint64_t dec)
> +{
> +    int64_t pow10 = 1, bin = 0;
> +    int digit, sign;
> +
> +    sign = dec & 0xf;
> +    if (sign < 0xa) {
> +        tcg_s390_data_exception(env, 0, GETPC());
> +    }
> +    dec >>= 4;
> +
> +    while (dec) {
> +        digit = dec & 0xf;
> +        if (digit > 0x9) {
> +            tcg_s390_data_exception(env, 0, GETPC());
> +        }
> +        dec >>= 4;
> +        bin += digit * pow10;
> +        pow10 *= 10;
> +    }
> +
> +    if (sign == 0xb || sign == 0xd) {
> +        bin = -bin;
> +    }
> +
> +    /* R1 is updated even on fixed-point-divide exception. */
> +    env->regs[r1] = (env->regs[r1] & 0xffffffff00000000ULL) | (uint32_t)bin;
> +    if (bin != (int32_t)bin) {
> +        tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> +    }
> +}
> +
> +uint64_t HELPER(cvbg)(CPUS390XState *env, Int128 dec)
> +{
> +    uint64_t dec64[] = {int128_getlo(dec), int128_gethi(dec)};
> +    int64_t bin = 0, pow10, tmp;
> +    int digit, i, sign;
> +
> +    sign = dec64[0] & 0xf;
> +    if (sign < 0xa) {
> +        tcg_s390_data_exception(env, 0, GETPC());
> +    }
> +    dec64[0] >>= 4;
> +    pow10 = (sign == 0xb || sign == 0xd) ? -1 : 1;
> +
> +    for (i = 1; i < 20; i++) {
> +        digit = dec64[i >> 4] & 0xf;
> +        if (digit > 0x9) {
> +            tcg_s390_data_exception(env, 0, GETPC());
> +        }
> +        dec64[i >> 4] >>= 4;
> +        tmp = pow10 * digit;
> +        if (digit && ((tmp ^ pow10) < 0)) {

That tmp ^ pow10 caused some frowning for me first, but it's just a check 
whether the sign changed, right? ... a comment in front of the line might be 
helpful.

> +            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> +        }
> +        tmp = bin + tmp;
> +        if (bin && ((tmp ^ bin) < 0)) {
> +            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> +        }
> +        bin = tmp;
> +        pow10 *= 10;
> +    }
> +
> +    g_assert(!dec64[0]);
> +    if (dec64[1]) {
> +        tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> +    }
> +
> +    return bin;
> +}

Patch looks sane to me now, but I'd appreciate if Richard and/or David could 
have a look at this, too!

  Thomas

Re: Re: [PATCH v4 2/4] target/s390x: Emulate CVB, CVBY and CVBG

[Ilya Leoshkevich]

On Mon, Feb 05, 2024 at 06:04:27PM +0100, Thomas Huth wrote:
> On 02/02/2024 15.11, Ilya Leoshkevich wrote:
> > Convert to Binary - counterparts of the already implemented Convert
> > to Decimal (CVD*) instructions.
> > Example from the Principles of Operation: 25594C becomes 63FA.
> > 
> > Co-developed-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   target/s390x/helper.h            |  2 +
> >   target/s390x/tcg/insn-data.h.inc |  4 ++
> >   target/s390x/tcg/int_helper.c    | 72 ++++++++++++++++++++++++++++++++
> >   target/s390x/tcg/translate.c     | 16 +++++++
> >   4 files changed, 94 insertions(+)

[...]

> > +uint64_t HELPER(cvbg)(CPUS390XState *env, Int128 dec)
> > +{
> > +    uint64_t dec64[] = {int128_getlo(dec), int128_gethi(dec)};
> > +    int64_t bin = 0, pow10, tmp;
> > +    int digit, i, sign;
> > +
> > +    sign = dec64[0] & 0xf;
> > +    if (sign < 0xa) {
> > +        tcg_s390_data_exception(env, 0, GETPC());
> > +    }
> > +    dec64[0] >>= 4;
> > +    pow10 = (sign == 0xb || sign == 0xd) ? -1 : 1;
> > +
> > +    for (i = 1; i < 20; i++) {
> > +        digit = dec64[i >> 4] & 0xf;
> > +        if (digit > 0x9) {
> > +            tcg_s390_data_exception(env, 0, GETPC());
> > +        }
> > +        dec64[i >> 4] >>= 4;
> > +        tmp = pow10 * digit;
> > +        if (digit && ((tmp ^ pow10) < 0)) {
> 
> That tmp ^ pow10 caused some frowning for me first, but it's just a check
> whether the sign changed, right? ... a comment in front of the line might be
> helpful.

Good point about writing a comment, I tried to elaborate why checking
the sign is justified, and realized that it's actually redundant.
The int64_t bounds are roughly +-9.2E+18, and the last pow10 value in
this loop is +-1E+18. The multiplication cannot overflow.

> > +            tcg_s390_program_interrupt(env, PGM_FIXPT_DIVIDE, GETPC());
> > +        }
> > +        tmp = bin + tmp;
> > +        if (bin && ((tmp ^ bin) < 0)) {

The addition, however, can, e.g., bin=9E+17 and tmp=9E+18.
So I'll send a v5 without the first check and with a comment.

[...]