[Qemu-devel] [PATCH 0/6] Some improvements in z/Arch instructions support

[Qemu-devel] [PATCH 0/6] Some improvements in z/Arch instructions support

[Pavel Zbitskiy]

Add BAL, BALR, CVB instructions
Fix few bugs in PACK, CSST

Pavel Zbitskiy (6):
  target/s390x: add BAL and BALR instructions
  target/s390x: fix CSST decoding and runtime alignment check
  target/s390x: fix ipm polluting irrelevant bits
  target/s390x: add EX support for TRT and TRTR
  target/s390x: fix PACK reading 1 byte less and writing 1 byte more
  target/s390x: implement CVB, CVBY and CVBG

 target/s390x/helper.h      |  2 ++
 target/s390x/insn-data.def |  7 ++++++
 target/s390x/int_helper.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/mem_helper.c  | 24 +++++++++++++++----
 target/s390x/translate.c   | 52 +++++++++++++++++++++++++++++++++++++----
 5 files changed, 135 insertions(+), 8 deletions(-)

-- 
2.16.2.windows.1

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

[Pavel Zbitskiy]

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;
+
+    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);
+    }
+    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;
+    }
+}
+
 static DisasJumpType op_basi(DisasContext *s, DisasOps *o)
 {
     tcg_gen_movi_i64(o->out, pc_to_link_info(s, s->pc_tmp));
-- 
2.16.2.windows.1

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

[David Hildenbrand]

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

[Qemu-devel] [PATCH 2/6] target/s390x: fix CSST decoding and runtime alignment check

[Pavel Zbitskiy]

CSST is defined as:

    C(0xc802, CSST,    SSF,   CASS, la1, a2, 0, 0, csst, 0)

It means that the first parameter is handled by in1_la1().
in1_la1() fills addr1 field, and not in1.

Furthermore, when extract32() is used for the alignment check, the
third parameter should specify the number of trailing bits that must
be 0. For FC these numbers are:

    FC=0: 2
    FC=1: 3
    FC=2: 4

For SC these numbers are:

    SC=0: 0
    SC=1: 1
    SC=2: 2
    SC=3: 3
    SC=4: 4

Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
---
 target/s390x/mem_helper.c | 2 +-
 target/s390x/translate.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index e21a47fb4d..c94dbf3fcb 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1442,7 +1442,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,
     }
 
     /* Sanity check the alignments.  */
-    if (extract32(a1, 0, 4 << fc) || extract32(a2, 0, 1 << sc)) {
+    if (extract32(a1, 0, fc + 2) || extract32(a2, 0, sc)) {
         goto spec_exception;
     }
 
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index efdc88e227..f318fb6e4e 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -2050,9 +2050,9 @@ static DisasJumpType op_csst(DisasContext *s, DisasOps *o)
     TCGv_i32 t_r3 = tcg_const_i32(r3);
 
     if (tb_cflags(s->base.tb) & CF_PARALLEL) {
-        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->in1, o->in2);
+        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->addr1, o->in2);
     } else {
-        gen_helper_csst(cc_op, cpu_env, t_r3, o->in1, o->in2);
+        gen_helper_csst(cc_op, cpu_env, t_r3, o->addr1, o->in2);
     }
     tcg_temp_free_i32(t_r3);
 
-- 
2.16.2.windows.1

Re: [Qemu-devel] [PATCH 2/6] target/s390x: fix CSST decoding and runtime alignment check

[David Hildenbrand]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> CSST is defined as:
> 
>     C(0xc802, CSST,    SSF,   CASS, la1, a2, 0, 0, csst, 0)
> 
> It means that the first parameter is handled by in1_la1().
> in1_la1() fills addr1 field, and not in1.
> 
> Furthermore, when extract32() is used for the alignment check, the
> third parameter should specify the number of trailing bits that must
> be 0. For FC these numbers are:
> 
>     FC=0: 2

-> word, 4 bytes -> 2 bit

>     FC=1: 3

-> double word, 8 bytes -> 3 bit

>     FC=2: 4

-> quad word, 16 bytes -> 4 bit

> 
> For SC these numbers are:
> 
>     SC=0: 0
>     SC=1: 1
>     SC=2: 2
>     SC=3: 3
>     SC=4: 4

Right, corresponds to the size.

> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/mem_helper.c | 2 +-
>  target/s390x/translate.c  | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index e21a47fb4d..c94dbf3fcb 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1442,7 +1442,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t r3, uint64_t a1,
>      }
>  
>      /* Sanity check the alignments.  */
> -    if (extract32(a1, 0, 4 << fc) || extract32(a2, 0, 1 << sc)) {
> +    if (extract32(a1, 0, fc + 2) || extract32(a2, 0, sc)) {
>          goto spec_exception;
>      }
>  
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index efdc88e227..f318fb6e4e 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2050,9 +2050,9 @@ static DisasJumpType op_csst(DisasContext *s, DisasOps *o)
>      TCGv_i32 t_r3 = tcg_const_i32(r3);
>  
>      if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> -        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->in1, o->in2);
> +        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->addr1, o->in2);
>      } else {
> -        gen_helper_csst(cc_op, cpu_env, t_r3, o->in1, o->in2);
> +        gen_helper_csst(cc_op, cpu_env, t_r3, o->addr1, o->in2);

Indeed, only addr1 is filled.

(did this ever work?)

>      }
>      tcg_temp_free_i32(t_r3);
>  
> 

Are you running some test case or how did you find this? (PoP review?)


Haven't tested it yet, but looks sane to me

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH 3/6] target/s390x: fix ipm polluting irrelevant bits

[Pavel Zbitskiy]

Suppose psw.mask=0x0000000080000000, cc=2, r1=0 and we do "ipm 1".
This command must touch only bits 32-39, so the expected output
is r1=0x20000000. However, currently qemu yields r1=0x20008000,
because irrelevant parts of PSW leak into r1 during program mask
transfer.

Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
---
 target/s390x/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index f318fb6e4e..05442dff36 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -2442,8 +2442,8 @@ static DisasJumpType op_ipm(DisasContext *s, DisasOps *o)
     tcg_gen_andi_i64(o->out, o->out, ~0xff000000ull);
 
     t1 = tcg_temp_new_i64();
-    tcg_gen_shli_i64(t1, psw_mask, 20);
-    tcg_gen_shri_i64(t1, t1, 36);
+    tcg_gen_andi_i64(t1, psw_mask, 0x00000f0000000000);
+    tcg_gen_shri_i64(t1, t1, 16);
     tcg_gen_or_i64(o->out, o->out, t1);
 
     tcg_gen_extu_i32_i64(t1, cc_op);
-- 
2.16.2.windows.1

Re: [Qemu-devel] [PATCH 3/6] target/s390x: fix ipm polluting irrelevant bits

[David Hildenbrand]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> Suppose psw.mask=0x0000000080000000, cc=2, r1=0 and we do "ipm 1".
> This command must touch only bits 32-39, so the expected output
> is r1=0x20000000. However, currently qemu yields r1=0x20008000,
> because irrelevant parts of PSW leak into r1 during program mask
> transfer.
> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index f318fb6e4e..05442dff36 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2442,8 +2442,8 @@ static DisasJumpType op_ipm(DisasContext *s, DisasOps *o)
>      tcg_gen_andi_i64(o->out, o->out, ~0xff000000ull);
>  
>      t1 = tcg_temp_new_i64();
> -    tcg_gen_shli_i64(t1, psw_mask, 20);
> -    tcg_gen_shri_i64(t1, t1, 36);
> +    tcg_gen_andi_i64(t1, psw_mask, 0x00000f0000000000);

ull?

> +    tcg_gen_shri_i64(t1, t1, 16);
>      tcg_gen_or_i64(o->out, o->out, t1);
>  
>      tcg_gen_extu_i32_i64(t1, cc_op);
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 3/6] target/s390x: fix ipm polluting irrelevant bits

[Richard Henderson]

On 08/05/2018 11:28 AM, Pavel Zbitskiy wrote:
> +    tcg_gen_andi_i64(t1, psw_mask, 0x00000f0000000000);
> +    tcg_gen_shri_i64(t1, t1, 16);

It would be better to swap these two operations, so that a 64-bit constant
isn't needed for the mask, e.g:

    tcg_gen_shri_i64(t1, psw_mask, 16);
    tcg_gen_andi_i64(t1, t1, 0xf000000);

Or maybe rewrite this whole function with extract/deposit:

    tcg_gen_extract_i64(t1, psw_mask, 40, 4);
    tcg_gen_extu_i32_i64(t2, cc_op);
    tcg_gen_deposit_i64(t1, t1, t2, 4, 60);
    tcg_gen_deposit_i64(o->out, o->out, t1, 24, 8);

But what you have is not wrong so,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 4/6] target/s390x: add EX support for TRT and TRTR

[Pavel Zbitskiy]

Improves "b213c9f5: target/s390x: Implement TRTR" by introducing the
intermediate functions, which are compatible with dx_helper type.

Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
---
 target/s390x/mem_helper.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index c94dbf3fcb..704d0193b5 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1299,12 +1299,26 @@ static inline uint32_t do_helper_trt(CPUS390XState *env, int len,
     return 0;
 }
 
+static uint32_t do_helper_trt_fwd(CPUS390XState *env, uint32_t len,
+                                  uint64_t array, uint64_t trans,
+                                  uintptr_t ra)
+{
+    return do_helper_trt(env, len, array, trans, 1, ra);
+}
+
 uint32_t HELPER(trt)(CPUS390XState *env, uint32_t len, uint64_t array,
                      uint64_t trans)
 {
     return do_helper_trt(env, len, array, trans, 1, GETPC());
 }
 
+static uint32_t do_helper_trt_bkwd(CPUS390XState *env, uint32_t len,
+                                   uint64_t array, uint64_t trans,
+                                   uintptr_t ra)
+{
+    return do_helper_trt(env, len, array, trans, -1, ra);
+}
+
 uint32_t HELPER(trtr)(CPUS390XState *env, uint32_t len, uint64_t array,
                       uint64_t trans)
 {
@@ -2193,12 +2207,14 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr)
         typedef uint32_t (*dx_helper)(CPUS390XState *, uint32_t, uint64_t,
                                       uint64_t, uintptr_t);
         static const dx_helper dx[16] = {
+            [0x0] = do_helper_trt_bkwd,
             [0x2] = do_helper_mvc,
             [0x4] = do_helper_nc,
             [0x5] = do_helper_clc,
             [0x6] = do_helper_oc,
             [0x7] = do_helper_xc,
             [0xc] = do_helper_tr,
+            [0xd] = do_helper_trt_fwd,
         };
         dx_helper helper = dx[opc & 0xf];
 
-- 
2.16.2.windows.1

[Qemu-devel] [PATCH 5/6] target/s390x: fix PACK reading 1 byte less and writing 1 byte more

[Pavel Zbitskiy]

PACK fails on the test from the Principles of Operation: F1F2F3F4
becomes 0000234C instead of 0001234C due to an off-by-one error.
Furthermore, it overwrites one extra byte to the left of F1.

Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
---
 target/s390x/mem_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 704d0193b5..bacae4f503 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -1019,15 +1019,15 @@ void HELPER(pack)(CPUS390XState *env, uint32_t len, uint64_t dest, uint64_t src)
     len_src--;
 
     /* now pack every value */
-    while (len_dest >= 0) {
+    while (len_dest > 0) {
         b = 0;
 
-        if (len_src > 0) {
+        if (len_src >= 0) {
             b = cpu_ldub_data_ra(env, src, ra) & 0x0f;
             src--;
             len_src--;
         }
-        if (len_src > 0) {
+        if (len_src >= 0) {
             b |= cpu_ldub_data_ra(env, src, ra) << 4;
             src--;
             len_src--;
-- 
2.16.2.windows.1

Re: [Qemu-devel] [PATCH 5/6] target/s390x: fix PACK reading 1 byte less and writing 1 byte more

[David Hildenbrand]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> PACK fails on the test from the Principles of Operation: F1F2F3F4
> becomes 0000234C instead of 0001234C due to an off-by-one error.
> Furthermore, it overwrites one extra byte to the left of F1.
> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/mem_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index 704d0193b5..bacae4f503 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1019,15 +1019,15 @@ void HELPER(pack)(CPUS390XState *env, uint32_t len, uint64_t dest, uint64_t src)
>      len_src--;
>  
>      /* now pack every value */
> -    while (len_dest >= 0) {
> +    while (len_dest > 0) {
>          b = 0;
>  
> -        if (len_src > 0) {
> +        if (len_src >= 0) {
>              b = cpu_ldub_data_ra(env, src, ra) & 0x0f;
>              src--;
>              len_src--;
>          }
> -        if (len_src > 0) {
> +        if (len_src >= 0) {
>              b |= cpu_ldub_data_ra(env, src, ra) << 4;
>              src--;
>              len_src--;
> 

In the SS format with two length fields, and in the
RSL format, L1 specifies the number of additional
operand bytes to the right of the byte designated by
the first-operand address. Therefore, the length in
bytes of the first operand is 1-16, corresponding to a
length code in L1 of 0-15. Similarly, L2. specifies the
number of additional operand bytes to the right of the
location designated by the second-operand address.
Results replace the first operand and are never
stored outside the field specified by the address and
length. If the first operand is longer than the second,
the second operand is extended on the left with zeros
up to the length of the first operand. This extension
does not modify the second operand in storage.

Doesn't this imply that we have always length + 1, and therefore the
current code is fine? ("length code vs length")

(e.g. len_dest = 0 implies one byte?)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH 5/6] target/s390x: fix PACK reading 1 byte less and writing 1 byte more

[Thomas Huth]

On 08/05/2018 08:28 PM, Pavel Zbitskiy wrote:
> PACK fails on the test from the Principles of Operation: F1F2F3F4
> becomes 0000234C instead of 0001234C due to an off-by-one error.
> Furthermore, it overwrites one extra byte to the left of F1.

 Hi Pavel,

thanks for fixing these bugs in the s390x instructions! If you've got
some more spare minutes, could you maybe also add regression tests for
these (user space) instruction to the tests/tcg/s390x folder? ... we
really need some tests there ...

 Thomas

[Qemu-devel] [PATCH 6/6] target/s390x: implement CVB, CVBY and CVBG

[Pavel Zbitskiy]

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

Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
---
 target/s390x/helper.h      |  2 ++
 target/s390x/insn-data.def |  4 ++++
 target/s390x/int_helper.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 target/s390x/translate.c   | 12 ++++++++++
 4 files changed, 76 insertions(+)

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 97c60ca7bc..20e0c424f9 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -88,6 +88,8 @@ DEF_HELPER_FLAGS_4(tcxb, TCG_CALL_NO_RWG_SE, i32, env, i64, i64, 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_3(sqxb, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_2(cvb, TCG_CALL_NO_WG, i64, env, i64)
+DEF_HELPER_FLAGS_2(cvbg, TCG_CALL_NO_WG, i64, env, i64)
 DEF_HELPER_FLAGS_1(cvd, TCG_CALL_NO_RWG_SE, i64, s32)
 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)
diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
index 9c7b434fca..f0b1cbc4b2 100644
--- a/target/s390x/insn-data.def
+++ b/target/s390x/insn-data.def
@@ -284,6 +284,10 @@
     D(0xec73, CLFIT,   RIE_a, GIE, r1_32u, i2_32u, 0, 0, ct, 0, 1)
     D(0xec71, CLGIT,   RIE_a, GIE, r1_o, i2_32u, 0, 0, ct, 0, 1)
 
+/* CONVERT TO BINARY */
+    C(0x4f00, CVB,     RX_a,  Z,   0, a2, new, r1_32, cvb, 0)
+    C(0xe306, CVBY,    RXY_a, LD,  0, a2, new, r1_32, cvb, 0)
+    C(0xe30e, CVBG,    RXY_a, Z,   0, a2, 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/int_helper.c b/target/s390x/int_helper.c
index abf77a94e6..2d67347d08 100644
--- a/target/s390x/int_helper.c
+++ b/target/s390x/int_helper.c
@@ -24,6 +24,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
@@ -118,6 +119,63 @@ uint64_t HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al,
     return ret;
 }
 
+static void general_operand_exception(CPUS390XState *env, uintptr_t ra)
+{
+    LowCore *lowcore;
+
+    lowcore = cpu_map_lowcore(env);
+    lowcore->data_exc_code = 0;
+    cpu_unmap_lowcore(lowcore);
+    s390_program_interrupt(env, PGM_DATA, ILEN_AUTO, ra);
+}
+
+static int64_t do_cvb(CPUS390XState *env, uint64_t src, int n)
+{
+    int i, j;
+    uintptr_t ra = GETPC();
+    int64_t dec, sign, digit, val, pow10;
+
+    for (i = 0; i < n; i++) {
+        dec = cpu_ldq_data_ra(env, src + (n - i - 1) * 8, ra);
+        for (j = 0; j < 16; j++, dec >>= 4) {
+            if (i == 0 && j == 0) {
+                sign = dec & 0xf;
+                if (sign < 0xa) {
+                    general_operand_exception(env, ra);
+                }
+                continue;
+            }
+            digit = dec & 0xf;
+            if (digit > 0x9) {
+                general_operand_exception(env, ra);
+            }
+            if (i == 0 && j == 1) {
+                if (sign == 0xb || sign == 0xd) {
+                    val = -digit;
+                    pow10 = -10;
+                } else {
+                    val = digit;
+                    pow10 = 10;
+                }
+            } else {
+                val += digit * pow10;
+                pow10 *= 10;
+            }
+        }
+    }
+    return val;
+}
+
+uint64_t HELPER(cvb)(CPUS390XState *env, uint64_t src)
+{
+    return do_cvb(env, src, 1);
+}
+
+uint64_t HELPER(cvbg)(CPUS390XState *env, uint64_t src)
+{
+    return do_cvb(env, src, 2);
+}
+
 uint64_t HELPER(cvd)(int32_t reg)
 {
     /* positive 0 */
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 05442dff36..83d71815d4 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -2106,6 +2106,18 @@ static DisasJumpType op_csp(DisasContext *s, DisasOps *o)
 }
 #endif
 
+static DisasJumpType op_cvb(DisasContext *s, DisasOps *o)
+{
+    gen_helper_cvb(o->out, cpu_env, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_cvbg(DisasContext *s, DisasOps *o)
+{
+    gen_helper_cvbg(o->out, cpu_env, o->in2);
+    return DISAS_NEXT;
+}
+
 static DisasJumpType op_cvd(DisasContext *s, DisasOps *o)
 {
     TCGv_i64 t1 = tcg_temp_new_i64();
-- 
2.16.2.windows.1

Re: [Qemu-devel] [PATCH 6/6] target/s390x: implement CVB, CVBY and CVBG

[David Hildenbrand]

On 05.08.2018 20:28, Pavel Zbitskiy wrote:
> Convert to Binary - counterparts of the already implemented Convert
> to Decimal (CVD*) instructions.
> Example from the Principles of Operation: 25594C becomes 63FA.
> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/helper.h      |  2 ++
>  target/s390x/insn-data.def |  4 ++++
>  target/s390x/int_helper.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/translate.c   | 12 ++++++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 97c60ca7bc..20e0c424f9 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -88,6 +88,8 @@ DEF_HELPER_FLAGS_4(tcxb, TCG_CALL_NO_RWG_SE, i32, env, i64, i64, 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_3(sqxb, TCG_CALL_NO_WG, i64, env, i64, i64)
> +DEF_HELPER_FLAGS_2(cvb, TCG_CALL_NO_WG, i64, env, i64)
> +DEF_HELPER_FLAGS_2(cvbg, TCG_CALL_NO_WG, i64, env, i64)
>  DEF_HELPER_FLAGS_1(cvd, TCG_CALL_NO_RWG_SE, i64, s32)
>  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)
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index 9c7b434fca..f0b1cbc4b2 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -284,6 +284,10 @@
>      D(0xec73, CLFIT,   RIE_a, GIE, r1_32u, i2_32u, 0, 0, ct, 0, 1)
>      D(0xec71, CLGIT,   RIE_a, GIE, r1_o, i2_32u, 0, 0, ct, 0, 1)
>  
> +/* CONVERT TO BINARY */
> +    C(0x4f00, CVB,     RX_a,  Z,   0, a2, new, r1_32, cvb, 0)
> +    C(0xe306, CVBY,    RXY_a, LD,  0, a2, new, r1_32, cvb, 0)
> +    C(0xe30e, CVBG,    RXY_a, Z,   0, a2, r1, 0, cvbg, 0)

Instead, use the "D" variant and pass e.g. 1/2 (or 4/8) as data ("length").

OR (better) access and test "s->fields->op" in op_cvb to detect the length.

You can then forward (e.g. the length) to the helper and avoid
op/cvbg/cvbg and inline do_cvb.

>  /* 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)

Time to also implement CVDG in a similar fashion ? :)

> diff --git a/target/s390x/int_helper.c b/target/s390x/int_helper.c
> index abf77a94e6..2d67347d08 100644
> --- a/target/s390x/int_helper.c
> +++ b/target/s390x/int_helper.c
> @@ -24,6 +24,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
> @@ -118,6 +119,63 @@ uint64_t HELPER(divu64)(CPUS390XState *env, uint64_t ah, uint64_t al,
>      return ret;
>  }
>  
> +static void general_operand_exception(CPUS390XState *env, uintptr_t ra)
> +{
> +    LowCore *lowcore;
> +
> +    lowcore = cpu_map_lowcore(env);
> +    lowcore->data_exc_code = 0;
> +    cpu_unmap_lowcore(lowcore);
> +    s390_program_interrupt(env, PGM_DATA, ILEN_AUTO, ra);
> +}
> +
> +static int64_t do_cvb(CPUS390XState *env, uint64_t src, int n)
> +{
> +    int i, j;
> +    uintptr_t ra = GETPC();
> +    int64_t dec, sign, digit, val, pow10;
> +
> +    for (i = 0; i < n; i++) {
> +        dec = cpu_ldq_data_ra(env, src + (n - i - 1) * 8, ra);
> +        for (j = 0; j < 16; j++, dec >>= 4) {
> +            if (i == 0 && j == 0) {
> +                sign = dec & 0xf;
> +                if (sign < 0xa) {
> +                    general_operand_exception(env, ra);
> +                }
> +                continue;
> +            }
> +            digit = dec & 0xf;
> +            if (digit > 0x9) {
> +                general_operand_exception(env, ra);
> +            }
> +            if (i == 0 && j == 1) {
> +                if (sign == 0xb || sign == 0xd) {
> +                    val = -digit;
> +                    pow10 = -10;
> +                } else {
> +                    val = digit;
> +                    pow10 = 10;
> +                }
> +            } else {
> +                val += digit * pow10;
> +                pow10 *= 10;
> +            }
> +        }
> +    }
> +    return val;
> +}
> +
> +uint64_t HELPER(cvb)(CPUS390XState *env, uint64_t src)
> +{
> +    return do_cvb(env, src, 1);
> +}
> +
> +uint64_t HELPER(cvbg)(CPUS390XState *env, uint64_t src)
> +{
> +    return do_cvb(env, src, 2);
> +}
> +
>  uint64_t HELPER(cvd)(int32_t reg)
>  {
>      /* positive 0 */
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 05442dff36..83d71815d4 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2106,6 +2106,18 @@ static DisasJumpType op_csp(DisasContext *s, DisasOps *o)
>  }
>  #endif
>  
> +static DisasJumpType op_cvb(DisasContext *s, DisasOps *o)
> +{
> +    gen_helper_cvb(o->out, cpu_env, o->in2);
> +    return DISAS_NEXT;
> +}
> +
> +static DisasJumpType op_cvbg(DisasContext *s, DisasOps *o)
> +{
> +    gen_helper_cvbg(o->out, cpu_env, o->in2);
> +    return DISAS_NEXT;
> +}
> +
>  static DisasJumpType op_cvd(DisasContext *s, DisasOps *o)
>  {
>      TCGv_i64 t1 = tcg_temp_new_i64();
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 0/6] Some improvements in z/Arch instructions support

[Cornelia Huck]

On Sun,  5 Aug 2018 14:28:25 -0400
Pavel Zbitskiy <pavel.zbitskiy@gmail.com> wrote:

Meta note #1: please add the s390x maintainers on cc: for the cover
              letter as well; that makes tracking easier
Meta note #2: I don't think this really fits trivial? I'll be happy to
              take the end result via the s390x tree, anyway.

> Add BAL, BALR, CVB instructions
> Fix few bugs in PACK, CSST
> 
> Pavel Zbitskiy (6):
>   target/s390x: add BAL and BALR instructions
>   target/s390x: fix CSST decoding and runtime alignment check
>   target/s390x: fix ipm polluting irrelevant bits
>   target/s390x: add EX support for TRT and TRTR
>   target/s390x: fix PACK reading 1 byte less and writing 1 byte more
>   target/s390x: implement CVB, CVBY and CVBG
> 
>  target/s390x/helper.h      |  2 ++
>  target/s390x/insn-data.def |  7 ++++++
>  target/s390x/int_helper.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/mem_helper.c  | 24 +++++++++++++++----
>  target/s390x/translate.c   | 52 +++++++++++++++++++++++++++++++++++++----
>  5 files changed, 135 insertions(+), 8 deletions(-)
> 

Out of curiousity: Do you have some code that actually makes use of
these instructions (especially BAL/BALR), or have you found this by
looking at the documentation?