Re: [Qemu-devel] [PATCH 1/6] target/s390x: add BAL and BALR instructions

[David Hildenbrand <david@redhat.com>]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> These instructions are provided for compatibility purposes and are
> used only by old software, in the new code BAS and BASR are preferred.
> The difference between the old and new instruction exists only in the
> 24-bit mode.
> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/insn-data.def |  3 +++
>  target/s390x/translate.c   | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index 5c6f33ed9c..9c7b434fca 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -102,6 +102,9 @@
>      D(0x9400, NI,      SI,    Z,   la1, i2_8u, new, 0, ni, nz64, MO_UB)
>      D(0xeb54, NIY,     SIY,   LD,  la1, i2_8u, new, 0, ni, nz64, MO_UB)
>  
> +/* BRANCH AND LINK */
> +    C(0x0500, BALR,    RR_a,  Z,   0, r2_nz, r1, 0, bal, 0)
> +    C(0x4500, BAL,     RX_a,  Z,   0, a2, r1, 0, bal, 0)
>  /* BRANCH AND SAVE */
>      C(0x0d00, BASR,    RR_a,  Z,   0, r2_nz, r1, 0, bas, 0)
>      C(0x4d00, BAS,     RX_a,  Z,   0, a2, r1, 0, bas, 0)
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 57c03cbf58..efdc88e227 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -1463,6 +1463,38 @@ static DisasJumpType op_bas(DisasContext *s, DisasOps *o)
>      }
>  }
>  
> +static void save_link_info(TCGv_i64 out, uint64_t pc, uint64_t ilc)
> +{
> +    TCGv_i64 t;
> +

This is !FLAG_MASK_64 && !FLAG_MASK_32 (24 bit) if I am not wrong?

FLAG_MASK_64 should simply be pc_to_link_info() just as for BAS.

"The link information in the 64-bit addressing mode
consists of the updated instruction address, placed in
bit positions 0-63 of the first-operand location."


> +    tcg_gen_andi_i64(out, out, 0xffffffff00000000);
> +    tcg_gen_ori_i64(out, out, (ilc << 30) | pc);
> +    t = tcg_temp_new_i64();
> +    tcg_gen_shri_i64(t, psw_mask, 16);
> +    tcg_gen_andi_i64(t, t, 0x0f000000);
> +    tcg_gen_or_i64(out, out, t);
> +    tcg_gen_extu_i32_i64(t, cc_op);
> +    tcg_gen_shli_i64(t, t, 28);
> +    tcg_gen_or_i64(out, out, t);
> +    tcg_temp_free_i64(t);
> +}
> +
> +static DisasJumpType op_bal(DisasContext *s, DisasOps *o)
> +{
> +    if (s->base.tb->flags & FLAG_MASK_32) {
> +        return op_bas(s, o);
> +    }

Personally, I prefer using pc_to_link_info() instead of calling op_bas.
(handled in save_link_info() completely then).

So 24/31/64 bit mode should be handled in save_link_info.

> +    gen_op_calc_cc(s);
> +    save_link_info(o->out, s->pc_tmp, s->ilen / 2);
> +    if (o->in2) {
> +        tcg_gen_mov_i64(psw_addr, o->in2);
> +        per_branch(s, false);
> +        return DISAS_PC_UPDATED;
> +    } else {
> +        return DISAS_NEXT;
> +    }

This looks just like BAS and should be fine.

> +}
> +
>  static DisasJumpType op_basi(DisasContext *s, DisasOps *o)
>  {
>      tcg_gen_movi_i64(o->out, pc_to_link_info(s, s->pc_tmp));
> 


-- 

Thanks,

David / dhildenb