:p
atchew
Login
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
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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -XXX,XX +XXX,XX @@ 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
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 XXXXXXX..XXXXXXX 100644 --- a/tests/tcg/s390x/Makefile.target +++ b/tests/tcg/s390x/Makefile.target @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/tcg/s390x/ex-relative-long.c @@ -XXX,XX +XXX,XX @@ +/* 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
v2: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04499.html v2 -> v3: Make mem static (Nina). Initialize cc with cr (Nina). Drop long casts (Nina). Move mask assignment outside of asm. Use "a" constraints instead of "r" where necessary. Drop unnecessary earlyclobbers. 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 | 156 +++++++++++++++++++++++++++++ 5 files changed, 171 insertions(+), 1 deletion(-) create mode 100644 tests/tcg/s390x/ex-relative-long.c -- 2.39.2
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> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: David Hildenbrand <david@redhat.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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -XXX,XX +XXX,XX @@ 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
Test EXECUTE and EXECUTE RELATIVE LONG with relative long instructions as targets. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tests/tcg/s390x/Makefile.target | 1 + tests/tcg/s390x/ex-relative-long.c | 156 +++++++++++++++++++++++++++++ 2 files changed, 157 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 XXXXXXX..XXXXXXX 100644 --- a/tests/tcg/s390x/Makefile.target +++ b/tests/tcg/s390x/Makefile.target @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX --- /dev/null +++ b/tests/tcg/s390x/ex-relative-long.c @@ -XXX,XX +XXX,XX @@ +/* 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. + */ +static 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 r2 asm("r2"); \ + char mask = 0x20; /* make target use %r2 */ \ + long pm, target; \ + \ + r2 = reg; \ + asm("larl %[target],0f\n" \ + "cr %%r0,%%r0\n" /* initial cc */ \ + "ex %[mask],0(%[target])\n" \ + "jg 1f\n" \ + "0: " #insn " %%r0," MEM_ASM "\n" \ + "1: ipm %[pm]\n" \ + : [target] "=&a" (target), [r2] "+r" (r2), [pm] "=r" (pm) \ + : [mask] "a" (mask) \ + : "cc", "memory"); \ + reg = r2; \ + *cc = (pm >> 28) & 3; \ + \ + return reg; \ + } + +#define DEFINE_EXRL_TEST(insn, exp_reg, exp_mem, exp_cc) \ + static long test_exrl_ ## insn(long reg, long *cc) \ + { \ + register long r2 asm("r2"); \ + char mask = 0x20; /* make target use %r2 */ \ + long pm; \ + \ + r2 = reg; \ + asm("cr %%r0,%%r0\n" /* initial cc */ \ + "exrl %[mask],0f\n" \ + "jg 1f\n" \ + "0: " #insn " %%r0," MEM_ASM "\n" \ + "1: ipm %[pm]\n" \ + : [r2] "+r" (r2), [pm] "=r" (pm) \ + : [mask] "a" (mask) \ + : "cc", "memory"); \ + reg = r2; \ + *cc = (pm >> 28) & 3; \ + \ + return reg; \ + } + +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 = (_exp_reg), \ + .exp_mem = (_exp_mem), \ + .exp_cc = (_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