Re: [PATCH v3 1/3] s390x/tcg: Implement Miscellaneous-Instruction-Extensions Facility 3 for the s390x

[David Hildenbrand <david@redhat.com>]

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