Re: [Qemu-devel] [PATCH v3 2/8] target/s390x: Implement CONVERT UNICODE insns

[Thomas Huth]

On 10.07.2017 22:45, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/s390x/helper.h      |   6 +
>  target/s390x/mem_helper.c  | 310 
> +++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/translate.c   |  43 +++++++
>  target/s390x/insn-data.def |  13 ++
>  4 files changed, 372 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 23e8d1d..2793cf3 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -107,6 +107,12 @@ DEF_HELPER_2(stfle, i32, env, i64)
>  DEF_HELPER_FLAGS_2(lpq, TCG_CALL_NO_WG, i64, env, i64)
>  DEF_HELPER_FLAGS_4(stpq, TCG_CALL_NO_WG, void, env, i64, i64, i64)
>  DEF_HELPER_4(mvcos, i32, env, i64, i64, i64)
> +DEF_HELPER_4(cu12, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu14, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu21, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu24, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu41, i32, env, i32, i32, i32)
> +DEF_HELPER_4(cu42, i32, env, i32, i32, i32)
>  
>  #ifndef CONFIG_USER_ONLY
>  DEF_HELPER_3(servc, i32, env, i64, i64)
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 513b402..0b18560 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
[...]
> +static inline uint32_t convert_unicode(CPUS390XState *env, uint32_t r1,
> +                                       uint32_t r2, uint32_t m3, uintptr_t 
> ra,
> +                                       decode_unicode_fn decode,
> +                                       encode_unicode_fn encode)
> +{
> +    uint64_t dst = get_address(env, r1);
> +    uint64_t dlen = get_length(env, r1 + 1);
> +    uint64_t src = get_address(env, r2);
> +    uint64_t slen = get_length(env, r2 + 1);
> +    bool enh_check = m3 & 1;
> +    int cc, i;

According to the PoP:

"The R1 and R2 fields [...] must designate an even-
 numbered register; otherwise, a specification excep-
 tion is recognized."

I think you should add such a check for even-numbered registers here.

> +    /* Lest we fail to service interrupts in a timely manner, limit the
> +       amount of work we're willing to do.  For now, let's cap at 256.  */
> +    for (i = 0; i < 256; ++i) {
> +        uint32_t c, ilen, olen;
> +
> +        cc = decode(env, src, slen, enh_check, ra, &c, &ilen);
> +        if (unlikely(cc >= 0)) {
> +            break;
> +        }
> +        cc = encode(env, dst, dlen, ra, c, &olen);
> +        if (unlikely(cc >= 0)) {
> +            break;
> +        }
> +
> +        src += ilen;
> +        slen -= ilen;
> +        dst += olen;
> +        dlen -= olen;
> +        cc = 3;
> +    }
> +
> +    set_address(env, r1, dst);
> +    set_length(env, r1 + 1, dlen);
> +    set_address(env, r2, src);
> +    set_length(env, r2 + 1, slen);
> +
> +    return cc;
> +}

 Thomas