* [PATCH v2 0/2] Fix EXECUTE of relative long instructions
@ 2023-03-15 0:11 Ilya Leoshkevich
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
0 siblings, 2 replies; 8+ messages in thread
From: Ilya Leoshkevich @ 2023-03-15 0:11 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand, Thomas Huth
Cc: qemu-s390x, qemu-devel, Ilya Leoshkevich
v1: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04316.html
v1 -> v2: Address the middle of an array in the test (Richard).
Rebase - not 100% trivial, so not carrying Reviewed-bys.
Hi,
This series fixes EXECUTE of instructions like LARL, LGLR, etc.
Currently the address calculation uses EXECUTE's address as a base,
while it should be using that of the target instruction.
Patch 1 fixes the issue, patch 2 adds a test.
Best regards,
Ilya
Ilya Leoshkevich (2):
target/s390x: Fix EXECUTE of relative long instructions
tests/tcg/s390x: Add ex-relative-long.c
target/s390x/cpu.h | 1 +
target/s390x/tcg/mem_helper.c | 1 +
target/s390x/tcg/translate.c | 13 ++-
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/ex-relative-long.c | 159 +++++++++++++++++++++++++++++
5 files changed, 174 insertions(+), 1 deletion(-)
create mode 100644 tests/tcg/s390x/ex-relative-long.c
--
2.39.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-15 0:11 [PATCH v2 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
@ 2023-03-15 0:11 ` Ilya Leoshkevich
2023-03-15 8:58 ` David Hildenbrand
2023-03-15 14:33 ` Richard Henderson
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
1 sibling, 2 replies; 8+ messages in thread
From: Ilya Leoshkevich @ 2023-03-15 0:11 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand, Thomas Huth
Cc: qemu-s390x, qemu-devel, Ilya Leoshkevich, Nina Schoetterl-Glausch
The code uses the wrong base for relative addressing: it should use the
target instruction address and not the EXECUTE's address.
Fix by storing the target instruction address in the new CPUS390XState
member and loading it from the code generated by gen_ri2().
Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
target/s390x/cpu.h | 1 +
target/s390x/tcg/mem_helper.c | 1 +
target/s390x/tcg/translate.c | 13 ++++++++++++-
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b2..8aaf8dd5a3b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -87,6 +87,7 @@ struct CPUArchState {
uint64_t cc_vr;
uint64_t ex_value;
+ uint64_t ex_target;
uint64_t __excp_addr;
uint64_t psa;
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index 6835c26dda4..00afae2b640 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2530,6 +2530,7 @@ void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr)
that ex_value is non-zero, which flags that we are in a state
that requires such execution. */
env->ex_value = insn | ilen;
+ env->ex_target = addr;
}
uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 14c3896d529..e938d8538f8 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5747,7 +5747,18 @@ static void in2_a2(DisasContext *s, DisasOps *o)
static TCGv gen_ri2(DisasContext *s)
{
- return tcg_constant_i64(s->base.pc_next + (int64_t)get_field(s, i2) * 2);
+ int64_t delta = (int64_t)get_field(s, i2) * 2;
+ TCGv ri2;
+
+ if (unlikely(s->ex_value)) {
+ ri2 = tcg_temp_new_i64();
+ tcg_gen_ld_i64(ri2, cpu_env, offsetof(CPUS390XState, ex_target));
+ tcg_gen_addi_i64(ri2, ri2, delta);
+ } else {
+ ri2 = tcg_constant_i64(s->base.pc_next + delta);
+ }
+
+ return ri2;
}
static void in2_ri2(DisasContext *s, DisasOps *o)
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-15 0:11 [PATCH v2 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
@ 2023-03-15 0:11 ` Ilya Leoshkevich
2023-03-15 14:33 ` Richard Henderson
2023-03-16 17:50 ` Nina Schoetterl-Glausch
1 sibling, 2 replies; 8+ messages in thread
From: Ilya Leoshkevich @ 2023-03-15 0:11 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand, Thomas Huth
Cc: qemu-s390x, qemu-devel, Ilya Leoshkevich
Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
as targets.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/ex-relative-long.c | 159 +++++++++++++++++++++++++++++
2 files changed, 160 insertions(+)
create mode 100644 tests/tcg/s390x/ex-relative-long.c
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index cf93b966862..90bc48227db 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -29,6 +29,7 @@ TESTS+=clst
TESTS+=long-double
TESTS+=cdsg
TESTS+=chrl
+TESTS+=ex-relative-long
cdsg: CFLAGS+=-pthread
cdsg: LDFLAGS+=-pthread
diff --git a/tests/tcg/s390x/ex-relative-long.c b/tests/tcg/s390x/ex-relative-long.c
new file mode 100644
index 00000000000..4caa8c1b962
--- /dev/null
+++ b/tests/tcg/s390x/ex-relative-long.c
@@ -0,0 +1,159 @@
+/* Check EXECUTE with relative long instructions as targets. */
+#include <stdlib.h>
+#include <stdio.h>
+
+struct test {
+ const char *name;
+ long (*func)(long reg, long *cc);
+ long exp_reg;
+ long exp_mem;
+ long exp_cc;
+};
+
+/*
+ * Each test sets the MEM_IDXth element of the mem array to MEM and uses a
+ * single relative long instruction on it. The other elements remain zero.
+ * This is in order to prevent stumbling upon MEM in random memory in case
+ * there is an off-by-a-small-value bug.
+ *
+ * Note that while gcc supports the ZL constraint for relative long operands,
+ * clang doesn't, so the assembly code accesses mem[MEM_IDX] using MEM_ASM.
+ */
+long mem[0x1000];
+#define MEM_IDX 0x800
+#define MEM_ASM "mem+0x800*8"
+
+/* Initial %r2 value. */
+#define REG 0x1234567887654321
+
+/* Initial mem[MEM_IDX] value. */
+#define MEM 0xfedcba9889abcdef
+
+/* Initial cc value. */
+#define CC 0
+
+/* Relative long instructions and their expected effects. */
+#define FOR_EACH_INSN(F) \
+ F(cgfrl, REG, MEM, 2) \
+ F(cghrl, REG, MEM, 2) \
+ F(cgrl, REG, MEM, 2) \
+ F(chrl, REG, MEM, 1) \
+ F(clgfrl, REG, MEM, 2) \
+ F(clghrl, REG, MEM, 2) \
+ F(clgrl, REG, MEM, 1) \
+ F(clhrl, REG, MEM, 2) \
+ F(clrl, REG, MEM, 1) \
+ F(crl, REG, MEM, 1) \
+ F(larl, (long)&mem[MEM_IDX], MEM, CC) \
+ F(lgfrl, 0xfffffffffedcba98, MEM, CC) \
+ F(lghrl, 0xfffffffffffffedc, MEM, CC) \
+ F(lgrl, MEM, MEM, CC) \
+ F(lhrl, 0x12345678fffffedc, MEM, CC) \
+ F(llghrl, 0x000000000000fedc, MEM, CC) \
+ F(llhrl, 0x123456780000fedc, MEM, CC) \
+ F(lrl, 0x12345678fedcba98, MEM, CC) \
+ F(stgrl, REG, REG, CC) \
+ F(sthrl, REG, 0x4321ba9889abcdef, CC) \
+ F(strl, REG, 0x8765432189abcdef, CC)
+
+/* Test functions. */
+#define DEFINE_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
+ static long test_ex_ ## insn(long reg, long *cc) \
+ { \
+ register long reg_val asm("r2"); \
+ long cc_val, mask, target; \
+ \
+ reg_val = reg; \
+ asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
+ "lghi %[mask],0x20\n" /* make target use %r2 */ \
+ "larl %[target],0f\n" \
+ "ex %[mask],0(%[target])\n" \
+ "jg 1f\n" \
+ "0: " #insn " %%r0," MEM_ASM "\n" \
+ "1: ipm %[cc_val]\n" \
+ : [cc_val] "=&r" (cc_val) \
+ , [mask] "=&r" (mask) \
+ , [target] "=&r" (target) \
+ , [reg_val] "+&r" (reg_val) \
+ : : "cc", "memory"); \
+ reg = reg_val; \
+ *cc = (cc_val >> 28) & 3; \
+ \
+ return reg_val; \
+ }
+
+#define DEFINE_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
+ static long test_exrl_ ## insn(long reg, long *cc) \
+ { \
+ register long reg_val asm("r2"); \
+ long cc_val, mask; \
+ \
+ reg_val = reg; \
+ asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
+ "lghi %[mask],0x20\n" /* make target use %r2 */ \
+ "exrl %[mask],0f\n" \
+ "jg 1f\n" \
+ "0: " #insn " %%r0," MEM_ASM "\n" \
+ "1: ipm %[cc_val]\n" \
+ : [cc_val] "=&r" (cc_val) \
+ , [mask] "=&r" (mask) \
+ , [reg_val] "+&r" (reg_val) \
+ : : "cc", "memory"); \
+ reg = reg_val; \
+ *cc = (cc_val >> 28) & 3; \
+ \
+ return reg_val; \
+ }
+
+FOR_EACH_INSN(DEFINE_EX_TEST)
+FOR_EACH_INSN(DEFINE_EXRL_TEST)
+
+/* Test definitions. */
+#define REGISTER_EX_EXRL_TEST(ex_insn, insn, _exp_reg, _exp_mem, _exp_cc) \
+ { \
+ .name = #ex_insn " " #insn, \
+ .func = test_ ## ex_insn ## _ ## insn, \
+ .exp_reg = (long)(_exp_reg), \
+ .exp_mem = (long)(_exp_mem), \
+ .exp_cc = (long)(_exp_cc), \
+ },
+
+#define REGISTER_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
+ REGISTER_EX_EXRL_TEST(ex, insn, exp_reg, exp_mem, exp_cc)
+
+#define REGISTER_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
+ REGISTER_EX_EXRL_TEST(exrl, insn, exp_reg, exp_mem, exp_cc)
+
+static const struct test tests[] = {
+ FOR_EACH_INSN(REGISTER_EX_TEST)
+ FOR_EACH_INSN(REGISTER_EXRL_TEST)
+};
+
+/* Loop over all tests and run them. */
+int main(void)
+{
+ const struct test *test;
+ int ret = EXIT_SUCCESS;
+ long reg, cc;
+ size_t i;
+
+ for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
+ test = &tests[i];
+ mem[MEM_IDX] = MEM;
+ cc = -1;
+ reg = test->func(REG, &cc);
+#define ASSERT_EQ(expected, actual) do { \
+ if (expected != actual) { \
+ fprintf(stderr, "%s: " #expected " (0x%lx) != " #actual " (0x%lx)\n", \
+ test->name, expected, actual); \
+ ret = EXIT_FAILURE; \
+ } \
+} while (0)
+ ASSERT_EQ(test->exp_reg, reg);
+ ASSERT_EQ(test->exp_mem, mem[MEM_IDX]);
+ ASSERT_EQ(test->exp_cc, cc);
+#undef ASSERT_EQ
+ }
+
+ return ret;
+}
--
2.39.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
@ 2023-03-15 8:58 ` David Hildenbrand
2023-03-15 14:33 ` Richard Henderson
1 sibling, 0 replies; 8+ messages in thread
From: David Hildenbrand @ 2023-03-15 8:58 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, Thomas Huth
Cc: qemu-s390x, qemu-devel, Nina Schoetterl-Glausch
On 15.03.23 01:11, Ilya Leoshkevich wrote:
> The code uses the wrong base for relative addressing: it should use the
> target instruction address and not the EXECUTE's address.
>
> Fix by storing the target instruction address in the new CPUS390XState
> member and loading it from the code generated by gen_ri2().
>
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
2023-03-15 8:58 ` David Hildenbrand
@ 2023-03-15 14:33 ` Richard Henderson
1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2023-03-15 14:33 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand, Thomas Huth
Cc: qemu-s390x, qemu-devel, Nina Schoetterl-Glausch
On 3/14/23 17:11, Ilya Leoshkevich wrote:
> The code uses the wrong base for relative addressing: it should use the
> target instruction address and not the EXECUTE's address.
>
> Fix by storing the target instruction address in the new CPUS390XState
> member and loading it from the code generated by gen_ri2().
>
> Reported-by: Nina Schoetterl-Glausch<nsg@linux.ibm.com>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
> target/s390x/cpu.h | 1 +
> target/s390x/tcg/mem_helper.c | 1 +
> target/s390x/tcg/translate.c | 13 ++++++++++++-
> 3 files chang
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
@ 2023-03-15 14:33 ` Richard Henderson
2023-03-16 17:50 ` Nina Schoetterl-Glausch
1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2023-03-15 14:33 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand, Thomas Huth; +Cc: qemu-s390x, qemu-devel
On 3/14/23 17:11, Ilya Leoshkevich wrote:
> Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
> as targets.
>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
> tests/tcg/s390x/Makefile.target | 1 +
> tests/tcg/s390x/ex-relative-long.c | 159 +++++++++++++++++++++++++++++
> 2 files changed, 160 insertions(+)
> create mode 100644 tests/tcg/s390x/ex-relative-long.c
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
2023-03-15 14:33 ` Richard Henderson
@ 2023-03-16 17:50 ` Nina Schoetterl-Glausch
2023-03-16 20:55 ` Ilya Leoshkevich
1 sibling, 1 reply; 8+ messages in thread
From: Nina Schoetterl-Glausch @ 2023-03-16 17:50 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson, David Hildenbrand,
Thomas Huth
Cc: qemu-s390x, qemu-devel
On Wed, 2023-03-15 at 01:11 +0100, Ilya Leoshkevich wrote:
> > Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions
> > as targets.
> >
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Some comments below.
> > ---
> > tests/tcg/s390x/Makefile.target | 1 +
> > tests/tcg/s390x/ex-relative-long.c | 159 +++++++++++++++++++++++++++++
> > 2 files changed, 160 insertions(+)
> > create mode 100644 tests/tcg/s390x/ex-relative-long.c
> >
> > diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> > index cf93b966862..90bc48227db 100644
> > --- a/tests/tcg/s390x/Makefile.target
> > +++ b/tests/tcg/s390x/Makefile.target
> > @@ -29,6 +29,7 @@ TESTS+=clst
> > TESTS+=long-double
> > TESTS+=cdsg
> > TESTS+=chrl
> > +TESTS+=ex-relative-long
> >
> > cdsg: CFLAGS+=-pthread
> > cdsg: LDFLAGS+=-pthread
> > diff --git a/tests/tcg/s390x/ex-relative-long.c b/tests/tcg/s390x/ex-relative-long.c
> > new file mode 100644
> > index 00000000000..4caa8c1b962
> > --- /dev/null
> > +++ b/tests/tcg/s390x/ex-relative-long.c
> > @@ -0,0 +1,159 @@
> > +/* Check EXECUTE with relative long instructions as targets. */
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +struct test {
> > + const char *name;
> > + long (*func)(long reg, long *cc);
> > + long exp_reg;
> > + long exp_mem;
> > + long exp_cc;
> > +};
> > +
> > +/*
> > + * Each test sets the MEM_IDXth element of the mem array to MEM and uses a
> > + * single relative long instruction on it. The other elements remain zero.
> > + * This is in order to prevent stumbling upon MEM in random memory in case
> > + * there is an off-by-a-small-value bug.
> > + *
> > + * Note that while gcc supports the ZL constraint for relative long operands,
> > + * clang doesn't, so the assembly code accesses mem[MEM_IDX] using MEM_ASM.
> > + */
> > +long mem[0x1000];
This could be static, no?
> > +#define MEM_IDX 0x800
> > +#define MEM_ASM "mem+0x800*8"
> > +
> > +/* Initial %r2 value. */
> > +#define REG 0x1234567887654321
> > +
> > +/* Initial mem[MEM_IDX] value. */
> > +#define MEM 0xfedcba9889abcdef
> > +
> > +/* Initial cc value. */
> > +#define CC 0
> > +
> > +/* Relative long instructions and their expected effects. */
> > +#define FOR_EACH_INSN(F) \
You could define some macros and then calculate a bunch of values in the table, i.e.
#define SL(v) ((long)(v))
#define UL(v) ((unsigned long)(v))
#define SI(v, i) ((int)(v >> ((1 - i) * 32)))
#define UI(v, i) ((unsigned int)(v >> ((1 - i) * 32)))
#define SH(v, i) ((short)(v >> ((3 - i) * 16)))
#define UH(v, i) ((unsigned short)(v >> ((3 - i) * 16)))
#define CMP(f, s) ((f) == (s) ? 0 : ((f) < (s) ? 1 : 2 ))
F(cgfrl, REG, MEM, CMP(SL(REG), SI(MEM, 0))
But everything checks out, so no need.
> > + F(cgfrl, REG, MEM, 2) \
> > + F(cghrl, REG, MEM, 2) \
> > + F(cgrl, REG, MEM, 2) \
> > + F(chrl, REG, MEM, 1) \
> > + F(clgfrl, REG, MEM, 2) \
> > + F(clghrl, REG, MEM, 2) \
> > + F(clgrl, REG, MEM, 1) \
> > + F(clhrl, REG, MEM, 2) \
> > + F(clrl, REG, MEM, 1) \
> > + F(crl, REG, MEM, 1) \
> > + F(larl, (long)&mem[MEM_IDX], MEM, CC) \
> > + F(lgfrl, 0xfffffffffedcba98, MEM, CC) \
> > + F(lghrl, 0xfffffffffffffedc, MEM, CC) \
> > + F(lgrl, MEM, MEM, CC) \
> > + F(lhrl, 0x12345678fffffedc, MEM, CC) \
> > + F(llghrl, 0x000000000000fedc, MEM, CC) \
> > + F(llhrl, 0x123456780000fedc, MEM, CC) \
> > + F(lrl, 0x12345678fedcba98, MEM, CC) \
> > + F(stgrl, REG, REG, CC) \
> > + F(sthrl, REG, 0x4321ba9889abcdef, CC) \
> > + F(strl, REG, 0x8765432189abcdef, CC)
> > +
> > +/* Test functions. */
> > +#define DEFINE_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + static long test_ex_ ## insn(long reg, long *cc) \
> > + { \
> > + register long reg_val asm("r2"); \
> > + long cc_val, mask, target; \
> > + \
> > + reg_val = reg; \
> > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
This confused me for a bit because it's not about cc_val at all.
Maybe do "cr %%r0,%%r0\n" instead?
Could also push it down before the ex, since that's where the change we care about
takes place.
> > + "lghi %[mask],0x20\n" /* make target use %r2 */ \
> > + "larl %[target],0f\n" \
> > + "ex %[mask],0(%[target])\n" \
> > + "jg 1f\n" \
> > + "0: " #insn " %%r0," MEM_ASM "\n" \
> > + "1: ipm %[cc_val]\n" \
> > + : [cc_val] "=&r" (cc_val) \
> > + , [mask] "=&r" (mask) \
> > + , [target] "=&r" (target) \
> > + , [reg_val] "+&r" (reg_val) \
> > + : : "cc", "memory"); \
> > + reg = reg_val; \
What is the point of this assignment?
> > + *cc = (cc_val >> 28) & 3; \
> > + \
> > + return reg_val; \
> > + }
> > +
> > +#define DEFINE_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + static long test_exrl_ ## insn(long reg, long *cc) \
> > + { \
> > + register long reg_val asm("r2"); \
> > + long cc_val, mask; \
> > + \
> > + reg_val = reg; \
> > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc */ \
> > + "lghi %[mask],0x20\n" /* make target use %r2 */ \
> > + "exrl %[mask],0f\n" \
> > + "jg 1f\n" \
> > + "0: " #insn " %%r0," MEM_ASM "\n" \
> > + "1: ipm %[cc_val]\n" \
> > + : [cc_val] "=&r" (cc_val) \
> > + , [mask] "=&r" (mask) \
> > + , [reg_val] "+&r" (reg_val) \
> > + : : "cc", "memory"); \
> > + reg = reg_val; \
> > + *cc = (cc_val >> 28) & 3; \
> > + \
> > + return reg_val; \
> > + }
> > +
> > +FOR_EACH_INSN(DEFINE_EX_TEST)
> > +FOR_EACH_INSN(DEFINE_EXRL_TEST)
> > +
> > +/* Test definitions. */
> > +#define REGISTER_EX_EXRL_TEST(ex_insn, insn, _exp_reg, _exp_mem, _exp_cc) \
> > + { \
> > + .name = #ex_insn " " #insn, \
> > + .func = test_ ## ex_insn ## _ ## insn, \
> > + .exp_reg = (long)(_exp_reg), \
> > + .exp_mem = (long)(_exp_mem), \
> > + .exp_cc = (long)(_exp_cc), \
You don't need the casts, do you?
> > + },
> > +
> > +#define REGISTER_EX_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + REGISTER_EX_EXRL_TEST(ex, insn, exp_reg, exp_mem, exp_cc)
> > +
> > +#define REGISTER_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \
> > + REGISTER_EX_EXRL_TEST(exrl, insn, exp_reg, exp_mem, exp_cc)
> > +
> > +static const struct test tests[] = {
> > + FOR_EACH_INSN(REGISTER_EX_TEST)
> > + FOR_EACH_INSN(REGISTER_EXRL_TEST)
> > +};
> > +
> > +/* Loop over all tests and run them. */
> > +int main(void)
> > +{
> > + const struct test *test;
> > + int ret = EXIT_SUCCESS;
> > + long reg, cc;
> > + size_t i;
> > +
> > + for (i = 0; i < sizeof(tests) / sizeof(tests[0]); i++) {
> > + test = &tests[i];
> > + mem[MEM_IDX] = MEM;
> > + cc = -1;
> > + reg = test->func(REG, &cc);
> > +#define ASSERT_EQ(expected, actual) do { \
> > + if (expected != actual) { \
> > + fprintf(stderr, "%s: " #expected " (0x%lx) != " #actual " (0x%lx)\n", \
> > + test->name, expected, actual); \
> > + ret = EXIT_FAILURE; \
> > + } \
> > +} while (0)
> > + ASSERT_EQ(test->exp_reg, reg);
> > + ASSERT_EQ(test->exp_mem, mem[MEM_IDX]);
> > + ASSERT_EQ(test->exp_cc, cc);
> > +#undef ASSERT_EQ
> > + }
> > +
> > + return ret;
> > +}
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-16 17:50 ` Nina Schoetterl-Glausch
@ 2023-03-16 20:55 ` Ilya Leoshkevich
0 siblings, 0 replies; 8+ messages in thread
From: Ilya Leoshkevich @ 2023-03-16 20:55 UTC (permalink / raw)
To: Nina Schoetterl-Glausch, Richard Henderson, David Hildenbrand,
Thomas Huth
Cc: qemu-s390x, qemu-devel
On Thu, 2023-03-16 at 18:50 +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-03-15 at 01:11 +0100, Ilya Leoshkevich wrote:
> > > Test EXECUTE and EXECUTE RELATIVE LONG with relative long
> > > instructions
> > > as targets.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>
> Some comments below.
>
> > > ---
> > > tests/tcg/s390x/Makefile.target | 1 +
> > > tests/tcg/s390x/ex-relative-long.c | 159
> > > +++++++++++++++++++++++++++++
> > > 2 files changed, 160 insertions(+)
> > > create mode 100644 tests/tcg/s390x/ex-relative-long.c
> > >
> > > diff --git a/tests/tcg/s390x/Makefile.target
> > > b/tests/tcg/s390x/Makefile.target
> > > index cf93b966862..90bc48227db 100644
> > > --- a/tests/tcg/s390x/Makefile.target
> > > +++ b/tests/tcg/s390x/Makefile.target
> > > @@ -29,6 +29,7 @@ TESTS+=clst
> > > TESTS+=long-double
> > > TESTS+=cdsg
> > > TESTS+=chrl
> > > +TESTS+=ex-relative-long
> > >
> > > cdsg: CFLAGS+=-pthread
> > > cdsg: LDFLAGS+=-pthread
> > > diff --git a/tests/tcg/s390x/ex-relative-long.c
> > > b/tests/tcg/s390x/ex-relative-long.c
> > > new file mode 100644
> > > index 00000000000..4caa8c1b962
> > > --- /dev/null
> > > +++ b/tests/tcg/s390x/ex-relative-long.c
> > > @@ -0,0 +1,159 @@
> > > +/* Check EXECUTE with relative long instructions as targets. */
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +
> > > +struct test {
> > > + const char *name;
> > > + long (*func)(long reg, long *cc);
> > > + long exp_reg;
> > > + long exp_mem;
> > > + long exp_cc;
> > > +};
> > > +
> > > +/*
> > > + * Each test sets the MEM_IDXth element of the mem array to MEM
> > > and uses a
> > > + * single relative long instruction on it. The other elements
> > > remain zero.
> > > + * This is in order to prevent stumbling upon MEM in random
> > > memory in case
> > > + * there is an off-by-a-small-value bug.
> > > + *
> > > + * Note that while gcc supports the ZL constraint for relative
> > > long operands,
> > > + * clang doesn't, so the assembly code accesses mem[MEM_IDX]
> > > using MEM_ASM.
> > > + */
> > > +long mem[0x1000];
>
> This could be static, no?
I was worried that mem would become inaccessible from the asm block,
but apparently it still works if I make mem static.
> > > +#define MEM_IDX 0x800
> > > +#define MEM_ASM "mem+0x800*8"
> > > +
> > > +/* Initial %r2 value. */
> > > +#define REG 0x1234567887654321
> > > +
> > > +/* Initial mem[MEM_IDX] value. */
> > > +#define MEM 0xfedcba9889abcdef
> > > +
> > > +/* Initial cc value. */
> > > +#define CC 0
> > > +
> > > +/* Relative long instructions and their expected effects. */
> > > +#define
> > > FOR_EACH_INSN(F)
> > > \
>
> You could define some macros and then calculate a bunch of values in
> the table, i.e.
> #define SL(v) ((long)(v))
> #define UL(v) ((unsigned long)(v))
> #define SI(v, i) ((int)(v >> ((1 - i) * 32)))
> #define UI(v, i) ((unsigned int)(v >> ((1 - i) * 32)))
> #define SH(v, i) ((short)(v >> ((3 - i) * 16)))
> #define UH(v, i) ((unsigned short)(v >> ((3 - i) * 16)))
> #define CMP(f, s) ((f) == (s) ? 0 : ((f) < (s) ? 1 : 2 ))
>
> F(cgfrl, REG, MEM, CMP(SL(REG),
> SI(MEM, 0))
>
> But everything checks out, so no need.
>
> > > + F(cgfrl, REG, MEM,
> > > 2) \
> > > + F(cghrl, REG, MEM,
> > > 2) \
> > > + F(cgrl, REG, MEM,
> > > 2) \
> > > + F(chrl, REG, MEM,
> > > 1) \
> > > + F(clgfrl, REG, MEM,
> > > 2) \
> > > + F(clghrl, REG, MEM,
> > > 2) \
> > > + F(clgrl, REG, MEM,
> > > 1) \
> > > + F(clhrl, REG, MEM,
> > > 2) \
> > > + F(clrl, REG, MEM,
> > > 1) \
> > > + F(crl, REG, MEM,
> > > 1) \
> > > + F(larl, (long)&mem[MEM_IDX], MEM,
> > > CC) \
> > > + F(lgfrl, 0xfffffffffedcba98, MEM,
> > > CC) \
> > > + F(lghrl, 0xfffffffffffffedc, MEM,
> > > CC) \
> > > + F(lgrl, MEM, MEM,
> > > CC) \
> > > + F(lhrl, 0x12345678fffffedc, MEM,
> > > CC) \
> > > + F(llghrl, 0x000000000000fedc, MEM,
> > > CC) \
> > > + F(llhrl, 0x123456780000fedc, MEM,
> > > CC) \
> > > + F(lrl, 0x12345678fedcba98, MEM,
> > > CC) \
> > > + F(stgrl, REG, REG,
> > > CC) \
> > > + F(sthrl, REG, 0x4321ba9889abcdef,
> > > CC) \
> > > + F(strl, REG, 0x8765432189abcdef, CC)
> > > +
> > > +/* Test functions. */
> > > +#define DEFINE_EX_TEST(insn, exp_reg, exp_mem,
> > > exp_cc) \
> > > + static long test_ex_ ## insn(long reg, long
> > > *cc) \
> > > +
> > > {
> > > \
> > > + register long reg_val
> > > asm("r2"); \
> > > + long cc_val, mask,
> > > target; \
> > > +
> > > \
> > > + reg_val =
> > > reg; \
> > > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc
> > > */ \
>
> This confused me for a bit because it's not about cc_val at all.
> Maybe do "cr %%r0,%%r0\n" instead?
> Could also push it down before the ex, since that's where the change
> we care about
> takes place.
Ok, will do.
> > > + "lghi %[mask],0x20\n" /* make target use %r2
> > > */ \
> > > + "larl
> > > %[target],0f\n" \
> > > + "ex
> > > %[mask],0(%[target])\n" \
> > > + "jg
> > > 1f\n" \
> > > + "0: " #insn " %%r0," MEM_ASM
> > > "\n" \
> > > + "1: ipm
> > > %[cc_val]\n" \
> > > + : [cc_val] "=&r"
> > > (cc_val) \
> > > + , [mask] "=&r"
> > > (mask) \
OT: I just realized I needed to use "a" instead of "r" here and in a
few other places.
> > > + , [target] "=&r"
> > > (target) \
> > > + , [reg_val] "+&r"
> > > (reg_val) \
> > > + : : "cc",
> > > "memory"); \
> > > + reg =
> > > reg_val;
> > > \
>
> What is the point of this assignment?
This is paranoia induced by the conservative reading of [1].
In this case I think the compiler is free to clobber reg_val during the
assignment to *cc.
[1] https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html
>
> > > + *cc = (cc_val >> 28) &
> > > 3; \
> > > +
> > > \
> > > + return
> > > reg_val; \
... so I should rather be returning reg here.
> > > + }
> > > +
> > > +#define DEFINE_EXRL_TEST(insn, exp_reg, exp_mem,
> > > exp_cc) \
> > > + static long test_exrl_ ## insn(long reg, long
> > > *cc) \
> > > +
> > > {
> > > \
> > > + register long reg_val
> > > asm("r2"); \
> > > + long cc_val,
> > > mask; \
> > > +
> > > \
> > > + reg_val =
> > > reg; \
> > > + asm("xgr %[cc_val],%[cc_val]\n" /* initial cc
> > > */ \
> > > + "lghi %[mask],0x20\n" /* make target use %r2
> > > */ \
> > > + "exrl
> > > %[mask],0f\n" \
> > > + "jg
> > > 1f\n" \
> > > + "0: " #insn " %%r0," MEM_ASM
> > > "\n" \
> > > + "1: ipm
> > > %[cc_val]\n" \
> > > + : [cc_val] "=&r"
> > > (cc_val) \
> > > + , [mask] "=&r"
> > > (mask) \
> > > + , [reg_val] "+&r"
> > > (reg_val) \
> > > + : : "cc",
> > > "memory"); \
> > > + reg =
> > > reg_val;
> > > \
> > > + *cc = (cc_val >> 28) &
> > > 3; \
> > > +
> > > \
> > > + return
> > > reg_val; \
Same here.
> > > + }
> > > +
> > > +FOR_EACH_INSN(DEFINE_EX_TEST)
> > > +FOR_EACH_INSN(DEFINE_EXRL_TEST)
> > > +
> > > +/* Test definitions. */
> > > +#define REGISTER_EX_EXRL_TEST(ex_insn, insn, _exp_reg, _exp_mem,
> > > _exp_cc) \
> > > +
> > > {
> > > \
> > > + .name = #ex_insn " "
> > > #insn, \
> > > + .func = test_ ## ex_insn ## _ ##
> > > insn, \
> > > + .exp_reg =
> > > (long)(_exp_reg), \
> > > + .exp_mem =
> > > (long)(_exp_mem), \
> > > + .exp_cc =
> > > (long)(_exp_cc), \
>
> You don't need the casts, do you?
Right, that's also a leftover. I'll clean this up.
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-03-16 20:56 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-15 0:11 [PATCH v2 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
2023-03-15 0:11 ` [PATCH v2 1/2] target/s390x: " Ilya Leoshkevich
2023-03-15 8:58 ` David Hildenbrand
2023-03-15 14:33 ` Richard Henderson
2023-03-15 0:11 ` [PATCH v2 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
2023-03-15 14:33 ` Richard Henderson
2023-03-16 17:50 ` Nina Schoetterl-Glausch
2023-03-16 20:55 ` Ilya Leoshkevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).