[v3,15/18] target/s390x: Implement SRSTU

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/s390x/helper.h      |  1 +
 target/s390x/insn-data.def |  2 ++
 target/s390x/mem_helper.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/translate.c   |  8 ++++++++
 4 files changed, 55 insertions(+)

> +uint64_t HELPER(srstu)(CPUS390XState *env, uint64_t end, uint64_t str)
> +{
> +    uintptr_t ra = GETPC();
> +    uint32_t len;
> +    uint16_t v, c = env->regs[0];
> +    uint64_t adj_end;
> +
> +    /* Bits 32-47 of R0 must be zero.  */
> +    if (env->regs[0] & 0xffff0000u) {
> +        cpu_restore_state(ENV_GET_CPU(env), ra);
> +        program_interrupt(env, PGM_SPECIFICATION, 6);
> +    }
> +
> +    str = wrap_address(env, str);
> +    end = wrap_address(env, end);
> +
> +    /* If the LSB of the two addresses differ, use one extra byte.  */
> +    adj_end = end + ((str ^ end) & 1);

This could theoretically wrap. Not sure how this is to be handled, do you?

> +
> +    /* Assume for now that R2 is unmodified.  */
> +    env->retxl = str;

If str was wrapped, r2 could be modified although it should not be touched.

> +
> +    /* 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 8k.  */
> +    for (len = 0; len < 0x2000; len += 2) {
> +        if (str + len == adj_end) {
> +            /* End of input found.  */
> +            env->cc_op = 2;
> +            return end;

If end was wrapped, r1 is modified here.

> +        }

Also str + len could wrap here. Not sure how this is to be handled.

> +        v = cpu_lduw_data_ra(env, str + len, ra);
> +        if (v == c) {
> +            /* Character found.  Set R1 to the location; R2 is unmodified.  */
> +            env->cc_op = 1;
> +            return str + len;
> +        }
> +    }
> +
> +    /* CPU-determined bytes processed.  Advance R2 to next byte to process.  */
> +    env->retxl = str + len;

Also wonder if r2 should be wrapped here. And if the "unused" bits
should be left unmodified here.

> +    env->cc_op = 3;
> +    return end;

Again, r1 could be modified here if end was wrapped.

> +}
> +
>  /* unsigned string compare (c is string terminator) */
>  uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2)
>  {
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 4a860f1..e594b91 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -4262,6 +4262,14 @@ static ExitStatus op_srst(DisasContext *s, DisasOps *o)
>      return NO_EXIT;
>  }
>  
> +static ExitStatus op_srstu(DisasContext *s, DisasOps *o)
> +{
> +    gen_helper_srstu(o->in1, cpu_env, o->in1, o->in2);
> +    set_cc_static(s);
> +    return_low128(o->in2);
> +    return NO_EXIT;
> +}
> +
>  static ExitStatus op_sub(DisasContext *s, DisasOps *o)
>  {
>      tcg_gen_sub_i64(o->out, o->in1, o->in2);
> 

Apart from special wrapping conditions, looks good to me!

(will scan the PoP how wrapping is to be handled in general during an
instruction. Some (like mvcos) mention it explicitly, others don't)

> Apart from special wrapping conditions, looks good to me!
> 
> (will scan the PoP how wrapping is to be handled in general during an
> instruction. Some (like mvcos) mention it explicitly, others don't)
> 

Answering my own questions:

1. We always have to wrap addresses that we generate except in some
special cases:

(PoP page 3-7)

"
The CPU performs address generation when it forms
an operand or instruction address or when it gener-
ates the address of a table entry from the appropriate
table origin and index. It also performs address gen-
eration when it increments an address to access suc-
cessive bytes of a field.

When, during the generation of the address, an
address is obtained that exceeds the value allowed
[...] one of the following two actions is taken:

 1. The carry out of the high-order bit position of the
    address is ignored. This handling of an address
    of excessive size is called wraparound.
 2. An interruption condition is recognized.

[...]

Addresses generated by the CPU that may be virtual
addresses always wrap."

... reading the following table, interrupts seem to get generated only
for some iplicit DAT translations/AR-mode tables and authority tables,
and only when Real or Absolute addresses are to be used.

So wrapping all addresses is done in general when working with virtual
addresses, whenever we generate an address.


2. We must not overwrite bit 0-31 in 24/31 bit mode:

(PoP page 3-6)
Unless specifically stated to the contrary, the follow-
ing definition applies in this publication: whenever the
machine generates and provides to the program a
24-bit or 31-bit address, the address is made avail-
able (placed in storage or loaded into a general regis-
ter) by being imbedded in a 32-bit field, with the
leftmost eight bits or one bit in the field, respectively,
set to zeros. When the address is loaded into a gen-
eral register, bits 0-31 of the register remain
unchanged.

On 06/20/2017 01:27 AM, David Hildenbrand wrote:
> 2. We must not overwrite bit 0-31 in 24/31 bit mode:
> 
> (PoP page 3-6)
> Unless specifically stated to the contrary, the follow-
> ing definition applies in this publication: whenever the
> machine generates and provides to the program a
> 24-bit or 31-bit address, the address is made avail-
> able (placed in storage or loaded into a general regis-
> ter) by being imbedded in a 32-bit field, with the
> leftmost eight bits or one bit in the field, respectively,
> set to zeros. When the address is loaded into a gen-
> eral register, bits 0-31 of the register remain
> unchanged.

Yes, Aurelien started down these lines when he added the set_address/set_length 
functions.  We just need to use them more often, I suppose.


r~

On 2017-06-19 17:04, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/s390x/helper.h      |  1 +
>  target/s390x/insn-data.def |  2 ++
>  target/s390x/mem_helper.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/translate.c   |  8 ++++++++
>  4 files changed, 55 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index cd51b89..58d7f5b 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -13,6 +13,7 @@ DEF_HELPER_FLAGS_3(divu32, TCG_CALL_NO_WG, i64, env, i64, i64)
>  DEF_HELPER_FLAGS_3(divs64, TCG_CALL_NO_WG, s64, env, s64, s64)
>  DEF_HELPER_FLAGS_4(divu64, TCG_CALL_NO_WG, i64, env, i64, i64, i64)
>  DEF_HELPER_3(srst, i64, env, i64, i64)
> +DEF_HELPER_3(srstu, i64, env, i64, i64)
>  DEF_HELPER_4(clst, i64, env, i64, i64, i64)
>  DEF_HELPER_FLAGS_4(mvn, TCG_CALL_NO_WG, void, env, i32, i64, i64)
>  DEF_HELPER_FLAGS_4(mvo, TCG_CALL_NO_WG, void, env, i32, i64, i64)
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index 634ef98..1bebcf2 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -736,6 +736,8 @@
>  
>  /* SEARCH STRING */
>      C(0xb25e, SRST,    RRE,   Z,   r1_o, r2_o, 0, 0, srst, 0)
> +/* SEARCH STRING UNICODE */
> +    C(0xb9be, SRSTU,   RRE,   ETF3, r1_o, r2_o, 0, 0, srstu, 0)
>  
>  /* SET ACCESS */
>      C(0xb24e, SAR,     RRE,   Z,   0, r2_o, 0, 0, sar, 0)
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 990858e..ce288d9 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -578,6 +578,50 @@ uint64_t HELPER(srst)(CPUS390XState *env, uint64_t end, uint64_t str)
>      return end;
>  }
>  
> +uint64_t HELPER(srstu)(CPUS390XState *env, uint64_t end, uint64_t str)
> +{
> +    uintptr_t ra = GETPC();
> +    uint32_t len;
> +    uint16_t v, c = env->regs[0];
> +    uint64_t adj_end;
> +
> +    /* Bits 32-47 of R0 must be zero.  */
> +    if (env->regs[0] & 0xffff0000u) {
> +        cpu_restore_state(ENV_GET_CPU(env), ra);
> +        program_interrupt(env, PGM_SPECIFICATION, 6);
> +    }
> +
> +    str = wrap_address(env, str);
> +    end = wrap_address(env, end);
> +
> +    /* If the LSB of the two addresses differ, use one extra byte.  */
> +    adj_end = end + ((str ^ end) & 1);
> +
> +    /* Assume for now that R2 is unmodified.  */
> +    env->retxl = str;
> +
> +    /* 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 8k.  */
> +    for (len = 0; len < 0x2000; len += 2) {
> +        if (str + len == adj_end) {
> +            /* End of input found.  */
> +            env->cc_op = 2;
> +            return end;
> +        }
> +        v = cpu_lduw_data_ra(env, str + len, ra);
> +        if (v == c) {
> +            /* Character found.  Set R1 to the location; R2 is unmodified.  */
> +            env->cc_op = 1;
> +            return str + len;
> +        }
> +    }
> +
> +    /* CPU-determined bytes processed.  Advance R2 to next byte to process.  */
> +    env->retxl = str + len;
> +    env->cc_op = 3;
> +    return end;
> +}
> +
>  /* unsigned string compare (c is string terminator) */
>  uint64_t HELPER(clst)(CPUS390XState *env, uint64_t c, uint64_t s1, uint64_t s2)
>  {

Overall that looks fine, but I think we should get the wrapping (almost)
correct, now that we have the get_address / set_address functions. As
all registers are saved on input, I guess the registers can be directly
written back in the helper using set_address. It should handle most of
the cases, except wrapping at the end of the address space, but anyway
I don't think it's handled somewhere.