[PATCH v2 0/2] target/s390x: Fix emulation of C(G)HRL

[PATCH v2 0/2] target/s390x: Fix emulation of C(G)HRL

[Nina Schoetterl-Glausch]

The second operand of COMPARE HALFWORD RELATIVE LONG is a signed
halfword, it does not have the same size as the first operand.
Fix this and add a tcg test for c(g)hrl.

Nina Schoetterl-Glausch (2):
  target/s390x: Fix emulation of C(G)HRL
  tests/tcg/s390x: Add C(G)HRL test

 target/s390x/tcg/insn-data.h.inc |  4 +-
 target/s390x/tcg/translate.c     |  7 +++
 tests/tcg/s390x/chrl.c           | 76 ++++++++++++++++++++++++++++++++
 tests/tcg/s390x/Makefile.target  |  1 +
 4 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/s390x/chrl.c

Range-diff against v1:
1:  f463507b25 ! 1:  228a1d9cfb target/s390x: Fix emulation of C(G)HRL
    @@ target/s390x/tcg/translate.c: static void in2_m2_64a(DisasContext *s, DisasOps *
      
     +static void in2_mri2_16s(DisasContext *s, DisasOps *o)
     +{
    -+    in2_ri2(s, o);
    -+    tcg_gen_qemu_ld16s(o->in2, o->in2, get_mem_index(s));
    ++    o->in2 = tcg_temp_new_i64();
    ++    tcg_gen_qemu_ld16s(o->in2, gen_ri2(s), get_mem_index(s));
     +}
     +#define SPEC_in2_mri2_16s 0
     +
      static void in2_mri2_16u(DisasContext *s, DisasOps *o)
      {
    -     in2_ri2(s, o);
    +     o->in2 = tcg_temp_new_i64();
2:  316b53ebd9 = 2:  3b1ca6b682 tests/tcg/s390x: Add C(G)HRL test

base-commit: 817fd33836e73812df2f1907612b57750fcb9491
-- 
2.37.2

[PATCH v2 1/2] target/s390x: Fix emulation of C(G)HRL

[Nina Schoetterl-Glausch]

The second operand of COMPARE HALFWORD RELATIVE LONG is a signed
halfword, it does not have the same size as the first operand.

Fixes: a7e836d5eb ("target-s390: Convert COMPARE, COMPARE LOGICAL")
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 target/s390x/tcg/insn-data.h.inc | 4 ++--
 target/s390x/tcg/translate.c     | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 9d2d35f084..6fe8ca5143 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -199,8 +199,8 @@
     C(0xe55c, CHSI,    SIL,   GIE, m1_32s, i2, 0, 0, 0, cmps64)
     C(0xe558, CGHSI,   SIL,   GIE, m1_64, i2, 0, 0, 0, cmps64)
 /* COMPARE HALFWORD RELATIVE LONG */
-    C(0xc605, CHRL,    RIL_b, GIE, r1_o, mri2_32s, 0, 0, 0, cmps32)
-    C(0xc604, CGHRL,   RIL_b, GIE, r1_o, mri2_64, 0, 0, 0, cmps64)
+    C(0xc605, CHRL,    RIL_b, GIE, r1_o, mri2_16s, 0, 0, 0, cmps32)
+    C(0xc604, CGHRL,   RIL_b, GIE, r1_o, mri2_16s, 0, 0, 0, cmps64)
 /* COMPARE HIGH */
     C(0xb9cd, CHHR,    RRE,   HW,  r1_sr32, r2_sr32, 0, 0, 0, cmps32)
     C(0xb9dd, CHLR,    RRE,   HW,  r1_sr32, r2_o, 0, 0, 0, cmps32)
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 21a57d5eb2..d324c0b6f2 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5979,6 +5979,13 @@ static void in2_m2_64a(DisasContext *s, DisasOps *o)
 #define SPEC_in2_m2_64a 0
 #endif
 
+static void in2_mri2_16s(DisasContext *s, DisasOps *o)
+{
+    o->in2 = tcg_temp_new_i64();
+    tcg_gen_qemu_ld16s(o->in2, gen_ri2(s), get_mem_index(s));
+}
+#define SPEC_in2_mri2_16s 0
+
 static void in2_mri2_16u(DisasContext *s, DisasOps *o)
 {
     o->in2 = tcg_temp_new_i64();
-- 
2.37.2

Re: [PATCH v2 1/2] target/s390x: Fix emulation of C(G)HRL

[Richard Henderson]

On 3/8/23 13:02, Nina Schoetterl-Glausch wrote:
> The second operand of COMPARE HALFWORD RELATIVE LONG is a signed
> halfword, it does not have the same size as the first operand.
> 
> Fixes: a7e836d5eb ("target-s390: Convert COMPARE, COMPARE LOGICAL")
> Signed-off-by: Nina Schoetterl-Glausch<nsg@linux.ibm.com>
> ---
>   target/s390x/tcg/insn-data.h.inc | 4 ++--
>   target/s390x/tcg/translate.c     | 7 +++++++
>   2 files changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 2/2] tests/tcg/s390x: Add C(G)HRL test

[Nina Schoetterl-Glausch]

Test COMPARE HALFWORD RELATIVE LONG instructions.
Test that the bytes following the second operand do not affect the
instruction.
Test the sign extension performed on the second operand.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---


I don't know what the coding style is for inline asm.
checkpatch.sh complains about the tabs inside the asm, which I find a
bit surprising given they're inside a string.
IMO emitting tabs makes sense in order to be consistent with gcc output.
I left the tabs in for now, but can remove them.


 tests/tcg/s390x/chrl.c          | 76 +++++++++++++++++++++++++++++++++
 tests/tcg/s390x/Makefile.target |  1 +
 2 files changed, 77 insertions(+)
 create mode 100644 tests/tcg/s390x/chrl.c

diff --git a/tests/tcg/s390x/chrl.c b/tests/tcg/s390x/chrl.c
new file mode 100644
index 0000000000..fa33d43c59
--- /dev/null
+++ b/tests/tcg/s390x/chrl.c
@@ -0,0 +1,76 @@
+#include <stdlib.h>
+#include <assert.h>
+#include <stdint.h>
+
+static void test_chrl(void)
+{
+    uint32_t program_mask, cc;
+
+    asm volatile (
+               ".pushsection .rodata\n"
+        "0:	.short	1,0x8000\n"
+        "	.popsection\n"
+
+        "	chrl	%[r],0b\n"
+        "	ipm	%[program_mask]\n"
+        : [program_mask] "=r" (program_mask)
+        : [r] "r" (1)
+    );
+
+    cc = program_mask >> 28;
+    assert(!cc);
+
+    asm volatile (
+               ".pushsection .rodata\n"
+        "0:	.short	-1,0x8000\n"
+        "	.popsection\n"
+
+        "	chrl	%[r],0b\n"
+        "	ipm	%[program_mask]\n"
+        : [program_mask] "=r" (program_mask)
+        : [r] "r" (-1)
+    );
+
+    cc = program_mask >> 28;
+    assert(!cc);
+}
+
+static void test_cghrl(void)
+{
+    uint32_t program_mask, cc;
+
+    asm volatile (
+               ".pushsection .rodata\n"
+        "0:	.short	1,0x8000,0,0\n"
+        "	.popsection\n"
+
+        "	cghrl	%[r],0b\n"
+        "	ipm	%[program_mask]\n"
+        : [program_mask] "=r" (program_mask)
+        : [r] "r" (1L)
+    );
+
+    cc = program_mask >> 28;
+    assert(!cc);
+
+    asm volatile (
+               ".pushsection .rodata\n"
+        "0:	.short	-1,0x8000,0,0\n"
+        "	.popsection\n"
+
+        "	cghrl	%[r],0b\n"
+        "	ipm	%[program_mask]\n"
+        : [program_mask] "=r" (program_mask)
+        : [r] "r" (-1L)
+    );
+
+    cc = program_mask >> 28;
+    assert(!cc);
+}
+
+int main(void)
+{
+    test_chrl();
+    test_cghrl();
+    return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 72ad309b27..a3d3beeffe 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -28,6 +28,7 @@ TESTS+=div
 TESTS+=clst
 TESTS+=long-double
 TESTS+=cdsg
+TESTS+=chrl
 
 cdsg: CFLAGS+=-pthread
 cdsg: LDFLAGS+=-pthread
-- 
2.37.2

Re: [PATCH v2 2/2] tests/tcg/s390x: Add C(G)HRL test

[Richard Henderson]

On 3/8/23 13:02, Nina Schoetterl-Glausch wrote:
> Test COMPARE HALFWORD RELATIVE LONG instructions.
> Test that the bytes following the second operand do not affect the
> instruction.
> Test the sign extension performed on the second operand.
> 
> Signed-off-by: Nina Schoetterl-Glausch<nsg@linux.ibm.com>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/2] tests/tcg/s390x: Add C(G)HRL test

[Thomas Huth]

On 08/03/2023 22.02, Nina Schoetterl-Glausch wrote:
> Test COMPARE HALFWORD RELATIVE LONG instructions.
> Test that the bytes following the second operand do not affect the
> instruction.
> Test the sign extension performed on the second operand.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
> 
> 
> I don't know what the coding style is for inline asm.
> checkpatch.sh complains about the tabs inside the asm, which I find a
> bit surprising given they're inside a string.
> IMO emitting tabs makes sense in order to be consistent with gcc output.
> I left the tabs in for now, but can remove them.

I don't mind too much, but all the other files use spaces, not tabs, so I 
think it's maybe best to also use spaces here for consistency?

  Thomas

Re: [PATCH v2 2/2] tests/tcg/s390x: Add C(G)HRL test

[Nina Schoetterl-Glausch]

On Thu, 2023-03-09 at 14:47 +0100, Thomas Huth wrote:
> On 08/03/2023 22.02, Nina Schoetterl-Glausch wrote:
> > Test COMPARE HALFWORD RELATIVE LONG instructions.
> > Test that the bytes following the second operand do not affect the
> > instruction.
> > Test the sign extension performed on the second operand.
> > 
> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> > ---
> > 
> > 
> > I don't know what the coding style is for inline asm.
> > checkpatch.sh complains about the tabs inside the asm, which I find a
> > bit surprising given they're inside a string.
> > IMO emitting tabs makes sense in order to be consistent with gcc output.
> > I left the tabs in for now, but can remove them.
> 
> I don't mind too much, but all the other files use spaces, not tabs, so I 
> think it's maybe best to also use spaces here for consistency?

Seems pretty inconsistent. Sometimes \t is used everywhere, sometimes some number of spaces.
I'll put \n\t at the end of lines, put the labels on a separate line and use a single space
between the mnemonic and the operands.
> 
>   Thomas
>