* [PATCH 0/2] Fix EXECUTE of relative long instructions
@ 2023-03-13 23:38 Ilya Leoshkevich
2023-03-13 23:38 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
2023-03-13 23:38 ` [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
0 siblings, 2 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2023-03-13 23:38 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
Cc: qemu-s390x, qemu-devel, Ilya Leoshkevich
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 | 10 +-
tests/tcg/s390x/Makefile.target | 1 +
tests/tcg/s390x/ex-relative-long.c | 149 +++++++++++++++++++++++++++++
5 files changed, 161 insertions(+), 1 deletion(-)
create mode 100644 tests/tcg/s390x/ex-relative-long.c
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-13 23:38 [PATCH 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
@ 2023-03-13 23:38 ` Ilya Leoshkevich
2023-03-14 9:10 ` David Hildenbrand
2023-03-14 16:29 ` Richard Henderson
2023-03-13 23:38 ` [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
1 sibling, 2 replies; 6+ messages in thread
From: Ilya Leoshkevich @ 2023-03-13 23:38 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
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 in2_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 | 10 +++++++++-
3 files changed, 11 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 811049ea281..fefff95b91c 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -5888,7 +5888,15 @@ static void in2_a2(DisasContext *s, DisasOps *o)
static void in2_ri2(DisasContext *s, DisasOps *o)
{
- o->in2 = tcg_const_i64(s->base.pc_next + (int64_t)get_field(s, i2) * 2);
+ int64_t delta = (int64_t)get_field(s, i2) * 2;
+
+ if (unlikely(s->ex_value)) {
+ o->in2 = tcg_temp_new_i64();
+ tcg_gen_ld_i64(o->in2, cpu_env, offsetof(CPUS390XState, ex_target));
+ tcg_gen_addi_i64(o->in2, o->in2, delta);
+ } else {
+ o->in2 = tcg_const_i64(s->base.pc_next + delta);
+ }
}
#define SPEC_in2_ri2 0
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-13 23:38 [PATCH 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
2023-03-13 23:38 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
@ 2023-03-13 23:38 ` Ilya Leoshkevich
2023-03-14 17:05 ` Richard Henderson
1 sibling, 1 reply; 6+ messages in thread
From: Ilya Leoshkevich @ 2023-03-13 23:38 UTC (permalink / raw)
To: Richard Henderson, David Hildenbrand
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 | 149 +++++++++++++++++++++++++++++
2 files changed, 150 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 72ad309b273..ed2709ee2c3 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+=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..e47dac7e2c3
--- /dev/null
+++ b/tests/tcg/s390x/ex-relative-long.c
@@ -0,0 +1,149 @@
+/* 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;
+};
+
+/* Variable targeted by relative long instructions. */
+long mem;
+
+/* Initial %r2 value. */
+#define REG 0x1234567887654321
+
+/* Initial "mem" value. */
+#define MEM 0xfedcba9889abcdef
+
+/* Initial cc value. */
+#define CC 0
+
+/* Relative long instructions. */
+#define FOR_EACH_TEST(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, 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\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\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_TEST(DEFINE_EX_TEST)
+FOR_EACH_TEST(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_TEST(REGISTER_EX_TEST)
+ FOR_EACH_TEST(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;
+ 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);
+ ASSERT_EQ(test->exp_cc, cc);
+#undef ASSERT_EQ
+ }
+
+ return ret;
+}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-13 23:38 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
@ 2023-03-14 9:10 ` David Hildenbrand
2023-03-14 16:29 ` Richard Henderson
1 sibling, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2023-03-14 9:10 UTC (permalink / raw)
To: Ilya Leoshkevich, Richard Henderson
Cc: qemu-s390x, qemu-devel, Nina Schoetterl-Glausch
On 14.03.23 00:38, 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 in2_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 | 10 +++++++++-
> 3 files changed, 11 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 811049ea281..fefff95b91c 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -5888,7 +5888,15 @@ static void in2_a2(DisasContext *s, DisasOps *o)
>
> static void in2_ri2(DisasContext *s, DisasOps *o)
> {
> - o->in2 = tcg_const_i64(s->base.pc_next + (int64_t)get_field(s, i2) * 2);
> + int64_t delta = (int64_t)get_field(s, i2) * 2;
> +
> + if (unlikely(s->ex_value)) {
> + o->in2 = tcg_temp_new_i64();
> + tcg_gen_ld_i64(o->in2, cpu_env, offsetof(CPUS390XState, ex_target));
> + tcg_gen_addi_i64(o->in2, o->in2, delta);
> + } else {
> + o->in2 = tcg_const_i64(s->base.pc_next + delta);
> + }
> }
> #define SPEC_in2_ri2 0
>
LGTM, hopefully Richard can have another look. Thanks!
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target/s390x: Fix EXECUTE of relative long instructions
2023-03-13 23:38 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
2023-03-14 9:10 ` David Hildenbrand
@ 2023-03-14 16:29 ` Richard Henderson
1 sibling, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2023-03-14 16:29 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand
Cc: qemu-s390x, qemu-devel, Nina Schoetterl-Glausch
On 3/13/23 16:38, 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 in2_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 | 10 +++++++++-
> 3 files changed, 11 insertions(+), 1 deletion(-)
Good solution, reading the value from env.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c
2023-03-13 23:38 ` [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
@ 2023-03-14 17:05 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2023-03-14 17:05 UTC (permalink / raw)
To: Ilya Leoshkevich, David Hildenbrand; +Cc: qemu-s390x, qemu-devel
On 3/13/23 16:38, 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 | 149 +++++++++++++++++++++++++++++
> 2 files changed, 150 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 72ad309b273..ed2709ee2c3 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+=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..e47dac7e2c3
> --- /dev/null
> +++ b/tests/tcg/s390x/ex-relative-long.c
> @@ -0,0 +1,149 @@
> +/* 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;
> +};
> +
> +/* Variable targeted by relative long instructions. */
> +long mem;
I guess you're assuming that the adjacent memory, which the buggy qemu would address,
contains something other than
> +/* Initial "mem" value. */
> +#define MEM 0xfedcba9889abcdef
this? Perhaps better to use an array, and address the middle of it?
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-03-14 17:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-13 23:38 [PATCH 0/2] Fix EXECUTE of relative long instructions Ilya Leoshkevich
2023-03-13 23:38 ` [PATCH 1/2] target/s390x: " Ilya Leoshkevich
2023-03-14 9:10 ` David Hildenbrand
2023-03-14 16:29 ` Richard Henderson
2023-03-13 23:38 ` [PATCH 2/2] tests/tcg/s390x: Add ex-relative-long.c Ilya Leoshkevich
2023-03-14 17:05 ` Richard Henderson
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.