All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x
@ 2022-02-15 20:26 David Miller
  2022-02-16  9:08 ` David Hildenbrand
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: David Miller @ 2022-02-15 20:26 UTC (permalink / raw)
  To: qemu-s390x, qemu-devel
  Cc: thuth, david, cohuck, richard.henderson, farman, pasic, borntraeger

resolves: https://gitlab.com/qemu-project/qemu/-/issues/737
implements:
AND WITH COMPLEMENT   (NCRK, NCGRK)
NAND                  (NNRK, NNGRK)
NOT EXCLUSIVE OR      (NXRK, NXGRK)
NOR                   (NORK, NOGRK)
OR WITH COMPLEMENT    (OCRK, OCGRK)
SELECT                (SELR, SELGR)
SELECT HIGH           (SELFHR)
MOVE RIGHT TO LEFT    (MVCRL)
POPULATION COUNT      (POPCNT)

Signed-off-by: David Miller <dmiller423@gmail.com>
---
  target/s390x/gen-features.c    |  1 +
  target/s390x/helper.h          |  1 +
  target/s390x/tcg/insn-data.def | 30 ++++++++++++++++--
  target/s390x/tcg/mem_helper.c  | 20 ++++++++++++
  target/s390x/tcg/translate.c   | 56 +++++++++++++++++++++++++++++++++-
  5 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 7cb1a6ec10..a3f30f69d9 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -740,6 +740,7 @@ static uint16_t qemu_LATEST[] = {
   /* add all new definitions before this point */
  static uint16_t qemu_MAX[] = {
+    S390_FEAT_MISC_INSTRUCTION_EXT3,
      /* generates a dependency warning, leave it out for now */
      S390_FEAT_MSA_EXT_5,
  };
diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 271b081e8c..69f69cf718 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -4,6 +4,7 @@ DEF_HELPER_FLAGS_4(nc, TCG_CALL_NO_WG, i32, env, i32, 
i64, i64)
  DEF_HELPER_FLAGS_4(oc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
  DEF_HELPER_FLAGS_4(xc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
  DEF_HELPER_FLAGS_4(mvc, TCG_CALL_NO_WG, void, env, i32, i64, i64)
+DEF_HELPER_FLAGS_4(mvcrl, TCG_CALL_NO_WG, void, env, i64, i64, i64)
  DEF_HELPER_FLAGS_4(mvcin, TCG_CALL_NO_WG, void, env, i32, i64, i64)
  DEF_HELPER_FLAGS_4(clc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
  DEF_HELPER_3(mvcl, i32, env, i32, i32)
diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def
index 1c3e115712..a64555f824 100644
--- a/target/s390x/tcg/insn-data.def
+++ b/target/s390x/tcg/insn-data.def
@@ -105,6 +105,9 @@
      D(0xa507, NILL,    RI_a,  Z,   r1_o, i2_16u, r1, 0, andi, 0, 0x1000)
      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)
+/* AND WITH COMPLEMENT */
+    C(0xb9f5, NCRK,    RRF_a, MIE3, r2, r3, new, r1_32, andc, nz32)
+    C(0xb9e5, NCGRK,   RRF_a, MIE3, r2, r3, r1, 0, andc, nz64)
   /* BRANCH AND LINK */
      C(0x0500, BALR,    RR_a,  Z,   0, r2_nz, r1, 0, bal, 0)
@@ -640,6 +643,8 @@
      C(0xeb8e, MVCLU,   RSY_a, E2,  0, a2, 0, 0, mvclu, 0)
  /* MOVE NUMERICS */
      C(0xd100, MVN,     SS_a,  Z,   la1, a2, 0, 0, mvn, 0)
+/* MOVE RIGHT TO LEFT */
+    C(0xe50a, MVCRL,   SSE,  MIE3, la1, a2, 0, 0, mvcrl, 0)
  /* MOVE PAGE */
      C(0xb254, MVPG,    RRE,   Z,   0, 0, 0, 0, mvpg, 0)
  /* MOVE STRING */
@@ -707,6 +712,16 @@
      F(0xed0f, MSEB,    RXF,   Z,   e1, m2_32u, new, e1, mseb, 0, IF_BFP)
      F(0xed1f, MSDB,    RXF,   Z,   f1, m2_64, new, f1, msdb, 0, IF_BFP)
  +/* NAND */
+    C(0xb974, NNRK,    RRF_a, MIE3, r2, r3, new, r1_32, nand, nz32)
+    C(0xb964, NNGRK,   RRF_a, MIE3, r2, r3, r1, 0, nand, nz64)
+/* NOR */
+    C(0xb976, NORK,    RRF_a, MIE3, r2, r3, new, r1_32, nor, nz32)
+    C(0xb966, NOGRK,   RRF_a, MIE3, r2, r3, r1, 0, nor, nz64)
+/* NOT EXCLUSIVE OR */
+    C(0xb977, NXRK,    RRF_a, MIE3, r2, r3, new, r1_32, nxor, nz32)
+    C(0xb967, NXGRK,   RRF_a, MIE3, r2, r3, r1, 0, nxor, nz64)
+
  /* OR */
      C(0x1600, OR,      RR_a,  Z,   r1, r2, new, r1_32, or, nz32)
      C(0xb9f6, ORK,     RRF_a, DO,  r2, r3, new, r1_32, or, nz32)
@@ -725,6 +740,9 @@
      D(0xa50b, OILL,    RI_a,  Z,   r1_o, i2_16u, r1, 0, ori, 0, 0x1000)
      D(0x9600, OI,      SI,    Z,   la1, i2_8u, new, 0, oi, nz64, MO_UB)
      D(0xeb56, OIY,     SIY,   LD,  la1, i2_8u, new, 0, oi, nz64, MO_UB)
+/* OR WITH COMPLEMENT */
+    C(0xb975, OCRK,    RRF_a, MIE3, r2, r3, new, r1_32, orc, nz32)
+    C(0xb965, OCGRK,   RRF_a, MIE3, r2, r3, r1, 0, orc, nz64)
   /* PACK */
      /* Really format SS_b, but we pack both lengths into one argument
@@ -735,6 +753,9 @@
  /* PACK UNICODE */
      C(0xe100, PKU,     SS_f,  E2,  la1, a2, 0, 0, pku, 0)
  +/* POPULATION COUNT */
+    C(0xb9e1, POPCNT,  RRE,   PC,  0, r2_o, r1, 0, popcnt, nz64)
+
  /* PREFETCH */
      /* Implemented as nops of course.  */
      C(0xe336, PFD,     RXY_b, GIE, 0, 0, 0, 0, 0, 0)
@@ -743,9 +764,6 @@
      /* Implemented as nop of course.  */
      C(0xb2e8, PPA,     RRF_c, PPA, 0, 0, 0, 0, 0, 0)
  -/* POPULATION COUNT */
-    C(0xb9e1, POPCNT,  RRE,   PC,  0, r2_o, r1, 0, popcnt, nz64)
-
  /* ROTATE LEFT SINGLE LOGICAL */
      C(0xeb1d, RLL,     RSY_a, Z,   r3_o, sh, new, r1_32, rll32, 0)
      C(0xeb1c, RLLG,    RSY_a, Z,   r3_o, sh, r1, 0, rll64, 0)
@@ -765,6 +783,12 @@
  /* SEARCH STRING UNICODE */
      C(0xb9be, SRSTU,   RRE,   ETF3, 0, 0, 0, 0, srstu, 0)
  +/* SELECT */
+    C(0xb9f0, SELR,    RRF_a, MIE3, r2, r3, new, r1_32, sel, 0)
+    C(0xb9e3, SELGR,   RRF_a, MIE3, r2, r3, r1, 0, sel, 0)
+/* SELECT HIGH */
+    C(0xb9c0, SELFHR,  RRF_a, MIE3, r2, r3, new, r1_32h, sel, 0)
+
  /* SET ACCESS */
      C(0xb24e, SAR,     RRE,   Z,   0, r2_o, 0, 0, sar, 0)
  /* SET ADDRESSING MODE */
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 406578d105..ed1a77ebe8 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -546,6 +546,26 @@ void HELPER(mvc)(CPUS390XState *env, uint32_t l, 
uint64_t dest, uint64_t src)
      do_helper_mvc(env, l, dest, src, GETPC());
  }
  +/* move right to left */
+void HELPER(mvcrl)(CPUS390XState *env, uint64_t l, uint64_t dest, 
uint64_t src)
+{
+    const int mmu_idx = cpu_mmu_index(env, false);
+    const uint64_t ra = GETPC();
+    S390Access srca, desta;
+    int32_t i;
+
+    /* MVCRL always copies one more byte than specified - maximum is 256 */
+    l++;
+
+    srca = access_prepare(env, src, l, MMU_DATA_LOAD, mmu_idx, ra);
+    desta = access_prepare(env, dest, l, MMU_DATA_STORE, mmu_idx, ra);
+
+    for (i = l - 1; i >= 0; i--) {
+        uint8_t byte = access_get_byte(env, &srca, i, ra);
+        access_set_byte(env, &desta, i, byte, ra);
+    }
+}
+
  /* move inverse  */
  void HELPER(mvcin)(CPUS390XState *env, uint32_t l, uint64_t dest, 
uint64_t src)
  {
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 46dea73357..dd765f40fa 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1498,6 +1498,46 @@ static DisasJumpType op_andi(DisasContext *s, 
DisasOps *o)
      return DISAS_NEXT;
  }
  +static DisasJumpType op_andc(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_andc_i64(o->out, o->in1, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_orc(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_orc_i64(o->out, o->in1, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_nand(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_nand_i64(o->out, o->in1, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_nor(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_nor_i64(o->out, o->in1, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_nxor(DisasContext *s, DisasOps *o)
+{
+    tcg_gen_eqv_i64(o->out, o->in1, o->in2);
+    return DISAS_NEXT;
+}
+
+static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
+{
+    DisasCompare c;
+    disas_jcc(s, &c, get_field(s, m4));
+    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
+                        o->in1, o->in2);
+    free_compare(&c);
+    return DISAS_NEXT;
+}
+
  static DisasJumpType op_ni(DisasContext *s, DisasOps *o)
  {
      o->in1 = tcg_temp_new_i64();
@@ -3358,6 +3398,12 @@ static DisasJumpType op_mvc(DisasContext *s, 
DisasOps *o)
      return DISAS_NEXT;
  }
  +static DisasJumpType op_mvcrl(DisasContext *s, DisasOps *o)
+{
+    gen_helper_mvcrl(cpu_env, regs[0], o->addr1, o->in2);
+    return DISAS_NEXT;
+}
+
  static DisasJumpType op_mvcin(DisasContext *s, DisasOps *o)
  {
      TCGv_i32 l = tcg_const_i32(get_field(s, l1));
@@ -3744,7 +3790,14 @@ static DisasJumpType op_pku(DisasContext *s, 
DisasOps *o)
   static DisasJumpType op_popcnt(DisasContext *s, DisasOps *o)
  {
-    gen_helper_popcnt(o->out, o->in2);
+    const uint8_t m3 = get_field(s, m3);
+
+    if ((m3 & 1) && s390_has_feat(S390_FEAT_MISC_INSTRUCTION_EXT3)) {
+        tcg_gen_ctpop_i64(o->out, o->in2);
+    }
+    else {
+        gen_helper_popcnt(o->out, o->in2);
+    }
      return DISAS_NEXT;
  }
  @@ -6170,6 +6223,7 @@ enum DisasInsnEnum {
  #define FAC_V           S390_FEAT_VECTOR /* vector facility */
  #define FAC_VE          S390_FEAT_VECTOR_ENH /* vector enhancements 
facility 1 */
  #define FAC_MIE2        S390_FEAT_MISC_INSTRUCTION_EXT2 /* 
miscellaneous-instruction-extensions facility 2 */
+#define FAC_MIE3        S390_FEAT_MISC_INSTRUCTION_EXT3 /* 
miscellaneous-instruction-extensions facility 3 */
   static const DisasInsn insn_info[] = {
  #include "insn-data.def"
-- 
2.32.0




^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x
  2022-02-15 20:26 [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x David Miller
@ 2022-02-16  9:08 ` David Hildenbrand
  2022-02-16 10:31 ` David Hildenbrand
  2022-02-16 11:18 ` David Hildenbrand
  2 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2022-02-16  9:08 UTC (permalink / raw)
  To: David Miller, qemu-s390x, qemu-devel
  Cc: thuth, cohuck, richard.henderson, farman, pasic, borntraeger

On 15.02.22 21:26, David Miller wrote:
> resolves: https://gitlab.com/qemu-project/qemu/-/issues/737
> implements:
> AND WITH COMPLEMENT   (NCRK, NCGRK)
> NAND                  (NNRK, NNGRK)
> NOT EXCLUSIVE OR      (NXRK, NXGRK)
> NOR                   (NORK, NOGRK)
> OR WITH COMPLEMENT    (OCRK, OCGRK)
> SELECT                (SELR, SELGR)
> SELECT HIGH           (SELFHR)
> MOVE RIGHT TO LEFT    (MVCRL)
> POPULATION COUNT      (POPCNT)

Unfortunately the patch can still not get applied because it's broken.
I strongly assume that either
* your mail sending client
* your MTA (Mail Transfer Agent)
messes with newlines in the patch and wraps long lines, corrupting the patch;

The mails are also not properly threaded.

I can spot:
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
  Thunderbird/91.5.0

Try sending mails with git send-email instead. Let me point out a couple of
cases that are broken in the patch as I received it, so you can check if
they are correct on the source.

> 
> Signed-off-by: David Miller <dmiller423@gmail.com>
> ---
>   target/s390x/gen-features.c    |  1 +
>   target/s390x/helper.h          |  1 +
>   target/s390x/tcg/insn-data.def | 30 ++++++++++++++++--
>   target/s390x/tcg/mem_helper.c  | 20 ++++++++++++
>   target/s390x/tcg/translate.c   | 56 +++++++++++++++++++++++++++++++++-
>   5 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 7cb1a6ec10..a3f30f69d9 100644
> --- a/target/s390x/gen-features.c
> +++ b/target/s390x/gen-features.c
> @@ -740,6 +740,7 @@ static uint16_t qemu_LATEST[] = {
>    /* add all new definitions before this point */
>   static uint16_t qemu_MAX[] = {
> +    S390_FEAT_MISC_INSTRUCTION_EXT3,
>       /* generates a dependency warning, leave it out for now */
>       S390_FEAT_MSA_EXT_5,
>   };
> diff --git a/target/s390x/helper.h b/target/s390x/helper.h
> index 271b081e8c..69f69cf718 100644
> --- a/target/s390x/helper.h
> +++ b/target/s390x/helper.h
> @@ -4,6 +4,7 @@ DEF_HELPER_FLAGS_4(nc, TCG_CALL_NO_WG, i32, env, i32, 
> i64, i64)

^ this line was broken although it shouldn't have been. The hunk should be:

diff --git a/target/s390x/helper.h b/target/s390x/helper.h
index 271b081e8c..69f69cf718 100644
--- a/target/s390x/helper.h
+++ b/target/s390x/helper.h
@@ -4,6 +4,7 @@ DEF_HELPER_FLAGS_4(nc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(oc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(xc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(mvc, TCG_CALL_NO_WG, void, env, i32, i64, i64)
+DEF_HELPER_FLAGS_4(mvcrl, TCG_CALL_NO_WG, void, env, i64, i64, i64)
 DEF_HELPER_FLAGS_4(mvcin, TCG_CALL_NO_WG, void, env, i32, i64, i64)
 DEF_HELPER_FLAGS_4(clc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
 DEF_HELPER_3(mvcl, i32, env, i32, i32)
diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def

>   DEF_HELPER_FLAGS_4(oc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
>   DEF_HELPER_FLAGS_4(xc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
>   DEF_HELPER_FLAGS_4(mvc, TCG_CALL_NO_WG, void, env, i32, i64, i64)
> +DEF_HELPER_FLAGS_4(mvcrl, TCG_CALL_NO_WG, void, env, i64, i64, i64)
>   DEF_HELPER_FLAGS_4(mvcin, TCG_CALL_NO_WG, void, env, i32, i64, i64)
>   DEF_HELPER_FLAGS_4(clc, TCG_CALL_NO_WG, i32, env, i32, i64, i64)
>   DEF_HELPER_3(mvcl, i32, env, i32, i32)
> diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def
> index 1c3e115712..a64555f824 100644
> --- a/target/s390x/tcg/insn-data.def
> +++ b/target/s390x/tcg/insn-data.def
> @@ -105,6 +105,9 @@
>       D(0xa507, NILL,    RI_a,  Z,   r1_o, i2_16u, r1, 0, andi, 0, 0x1000)
>       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)
> +/* AND WITH COMPLEMENT */
> +    C(0xb9f5, NCRK,    RRF_a, MIE3, r2, r3, new, r1_32, andc, nz32)
> +    C(0xb9e5, NCGRK,   RRF_a, MIE3, r2, r3, r1, 0, andc, nz64)

^ note that there is a newline in the code before /* BRANCH AND LINK */,
 but it's gone in your patch. The hunk should have been

@@ -105,6 +105,9 @@
     D(0xa507, NILL,    RI_a,  Z,   r1_o, i2_16u, r1, 0, andi, 0, 0x1000)
     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)
+/* AND WITH COMPLEMENT */
+    C(0xb9f5, NCRK,    RRF_a, MIE3, r2, r3, new, r1_32, andc, nz32)
+    C(0xb9e5, NCGRK,   RRF_a, MIE3, r2, r3, r1, 0, andc, nz64)
 
 /* BRANCH AND LINK */
     C(0x0500, BALR,    RR_a,  Z,   0, r2_nz, r1, 0, bal, 0)


>    /* BRANCH AND LINK */
>       C(0x0500, BALR,    RR_a,  Z,   0, r2_nz, r1, 0, bal, 0)
> @@ -640,6 +643,8 @@
>       C(0xeb8e, MVCLU,   RSY_a, E2,  0, a2, 0, 0, mvclu, 0)
>   /* MOVE NUMERICS */
>       C(0xd100, MVN,     SS_a,  Z,   la1, a2, 0, 0, mvn, 0)
> +/* MOVE RIGHT TO LEFT */
> +    C(0xe50a, MVCRL,   SSE,  MIE3, la1, a2, 0, 0, mvcrl, 0)
>   /* MOVE PAGE */
>       C(0xb254, MVPG,    RRE,   Z,   0, 0, 0, 0, mvpg, 0)
>   /* MOVE STRING */
> @@ -707,6 +712,16 @@
>       F(0xed0f, MSEB,    RXF,   Z,   e1, m2_32u, new, e1, mseb, 0, IF_BFP)
>       F(0xed1f, MSDB,    RXF,   Z,   f1, m2_64, new, f1, msdb, 0, IF_BFP)
>   +/* NAND */

^ note the + is preceded by "   "

This hunk should have been

 /* MOVE STRING */
     C(0xb255, MVST,    RRE,   Z,   0, 0, 0, 0, mvst, 0)
+/* NAND */
+    C(0xb974, NNRK,    RRF_a, MIE3, r2, r3, new, r1_32, nand, nz32)
+    C(0xb964, NNGRK,   RRF_a, MIE3, r2, r3, r1, 0, nand, nz64)
+/* NOR */
+    C(0xb976, NORK,    RRF_a, MIE3, r2, r3, new, r1_32, nor, nz32)
+    C(0xb966, NOGRK,   RRF_a, MIE3, r2, r3, r1, 0, nor, nz64)
+/* NOT EXCLUSIVE OR */
+    C(0xb977, NXRK,    RRF_a, MIE3, r2, r3, new, r1_32, nxor, nz32)
+    C(0xb967, NXGRK,   RRF_a, MIE3, r2, r3, r1, 0, nxor, nz64)
+
 /* MOVE WITH OPTIONAL SPECIFICATION */

[..]

>   {
> -    gen_helper_popcnt(o->out, o->in2);
> +    const uint8_t m3 = get_field(s, m3);
> +
> +    if ((m3 & 1) && s390_has_feat(S390_FEAT_MISC_INSTRUCTION_EXT3)) {
> +        tcg_gen_ctpop_i64(o->out, o->in2);
> +    }
> +    else {

This should not be your patch workflow fault: "} else {"


-- 
Thanks,

David / dhildenb



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x
  2022-02-15 20:26 [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x David Miller
  2022-02-16  9:08 ` David Hildenbrand
@ 2022-02-16 10:31 ` David Hildenbrand
  2022-02-16 10:59   ` David Hildenbrand
  2022-02-16 11:18 ` David Hildenbrand
  2 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2022-02-16 10:31 UTC (permalink / raw)
  To: David Miller, qemu-s390x, qemu-devel, richard.henderson
  Cc: pasic, borntraeger, thuth, cohuck, farman

> +static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
> +{
> +    DisasCompare c;
> +    disas_jcc(s, &c, get_field(s, m4));
> +    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
> +                        o->in1, o->in2);
> +    free_compare(&c);
> +    return DISAS_NEXT;
> +}


I realize that SELECT really is mostly identical to LOAD ON CONDITION,
except that we have a second input.

The following on top would unify both


diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def
index fb482b08b7..493f1d669c 100644
--- a/target/s390x/tcg/insn-data.def
+++ b/target/s390x/tcg/insn-data.def
@@ -781,8 +781,8 @@
 /* SEARCH STRING UNICODE */
     C(0xb9be, SRSTU,   RRE,   ETF3, 0, 0, 0, 0, srstu, 0)
 /* SELECT */
-    C(0xb9f0, SELR,    RRF_a, MIE3, r2, r3, new, r1_32, sel, 0)
-    C(0xb9e3, SELGR,   RRF_a, MIE3, r2, r3, r1, 0, sel, 0)
+    C(0xb9f0, SELR,    RRF_a, MIE3, r3, r2, new, r1_32, loc, 0)
+    C(0xb9e3, SELGR,   RRF_a, MIE3, r3, r2, r1, 0, loc, 0)
 
 /* SET ACCESS */
     C(0xb24e, SAR,     RRE,   Z,   0, r2_o, 0, 0, sar, 0)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index d5c536c60a..7805ffe879 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -1528,16 +1528,6 @@ static DisasJumpType op_nxor(DisasContext *s, DisasOps *o)
     return DISAS_NEXT;
 }
 
-static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
-{
-    DisasCompare c;
-    disas_jcc(s, &c, get_field(s, m4));
-    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
-                        o->in1, o->in2);
-    free_compare(&c);
-    return DISAS_NEXT;
-}
-
 static DisasJumpType op_ni(DisasContext *s, DisasOps *o)
 {
     o->in1 = tcg_temp_new_i64();
@@ -2998,7 +2988,13 @@ static DisasJumpType op_loc(DisasContext *s, DisasOps *o)
 {
     DisasCompare c;
 
-    disas_jcc(s, &c, get_field(s, m3));
+    if (have_field(s, m3)) {
+        /* LOAD * ON CONDITION */
+        disas_jcc(s, &c, get_field(s, m3));
+    } else {
+        /* SELECT */
+        disas_jcc(s, &c, get_field(s, m4));
+    }
 
     if (c.is_64) {
         tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,


I can spot some advanced magic in op_loc for "!c.is64".

But even with that change, the SELECT test still crashes for me.

The problematic part is the last selfhrnz() test that makes QEMU crash.

This might be an existing BUG for op_loc already -- or in the TCG backend.

Maybe the disas_jcc/tcg_gen_movcond_i64 generates something unexpected on my machine?

-- 
Thanks,

David / dhildenb



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x
  2022-02-16 10:31 ` David Hildenbrand
@ 2022-02-16 10:59   ` David Hildenbrand
  0 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2022-02-16 10:59 UTC (permalink / raw)
  To: David Miller, qemu-s390x, qemu-devel, richard.henderson
  Cc: pasic, borntraeger, thuth, cohuck, farman

On 16.02.22 11:31, David Hildenbrand wrote:
>> +static DisasJumpType op_sel(DisasContext *s, DisasOps *o)
>> +{
>> +    DisasCompare c;
>> +    disas_jcc(s, &c, get_field(s, m4));
>> +    tcg_gen_movcond_i64(c.cond, o->out, c.u.s64.a, c.u.s64.b,
>> +                        o->in1, o->in2);
>> +    free_compare(&c);
>> +    return DISAS_NEXT;
>> +}
> 
> 
> I realize that SELECT really is mostly identical to LOAD ON CONDITION,
> except that we have a second input.
> 
> The following on top would unify both
> 
> 
> diff --git a/target/s390x/tcg/insn-data.def b/target/s390x/tcg/insn-data.def
> index fb482b08b7..493f1d669c 100644
> --- a/target/s390x/tcg/insn-data.def
> +++ b/target/s390x/tcg/insn-data.def
> @@ -781,8 +781,8 @@
>  /* SEARCH STRING UNICODE */
>      C(0xb9be, SRSTU,   RRE,   ETF3, 0, 0, 0, 0, srstu, 0)
>  /* SELECT */
> -    C(0xb9f0, SELR,    RRF_a, MIE3, r2, r3, new, r1_32, sel, 0)
> -    C(0xb9e3, SELGR,   RRF_a, MIE3, r2, r3, r1, 0, sel, 0)
> +    C(0xb9f0, SELR,    RRF_a, MIE3, r3, r2, new, r1_32, loc, 0)
> +    C(0xb9e3, SELGR,   RRF_a, MIE3, r3, r2, r1, 0, loc, 0)

I forgot SELECT HIGH, requires similar adjustment.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x
  2022-02-15 20:26 [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x David Miller
  2022-02-16  9:08 ` David Hildenbrand
  2022-02-16 10:31 ` David Hildenbrand
@ 2022-02-16 11:18 ` David Hildenbrand
  2 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2022-02-16 11:18 UTC (permalink / raw)
  To: David Miller, qemu-s390x, qemu-devel
  Cc: thuth, cohuck, richard.henderson, farman, pasic, borntraeger

>       /* Really format SS_b, but we pack both lengths into one argument
> @@ -735,6 +753,9 @@
>   /* PACK UNICODE */
>       C(0xe100, PKU,     SS_f,  E2,  la1, a2, 0, 0, pku, 0)
>   +/* POPULATION COUNT */
> +    C(0xb9e1, POPCNT,  RRE,   PC,  0, r2_o, r1, 0, popcnt, nz64)

You actually need RRF_c instead of RRE.

Otherwise QEMU aborts when the guest executes POPCNT as RRE does not
include the m3 field.


-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-02-16 11:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-15 20:26 [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x David Miller
2022-02-16  9:08 ` David Hildenbrand
2022-02-16 10:31 ` David Hildenbrand
2022-02-16 10:59   ` David Hildenbrand
2022-02-16 11:18 ` David Hildenbrand

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.