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

[Thomas Huth <thuth@redhat.com>]

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