| Message ID | 20230703155801.179167-6-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/s390x: Miscellaneous TCG fixes | expand |
On 03.07.23 17:50, Ilya Leoshkevich wrote: > When a DAT error occurs, LRA is supposed to write the error information > to the bottom 32 bits of R1, and leave the top 32 bits of R1 alone. > > Fix by passing the original value of R1 into helper and copying the > top 32 bits to the return value. > > Fixes: d8fe4a9c284f ("target-s390: Convert LRA") > Cc: qemu-stable@nongnu.org > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > target/s390x/helper.h | 2 +- > target/s390x/tcg/mem_helper.c | 4 ++-- > target/s390x/tcg/translate.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h > index 6bc01df73d7..05102578fc9 100644 > --- a/target/s390x/helper.h > +++ b/target/s390x/helper.h > @@ -355,7 +355,7 @@ DEF_HELPER_FLAGS_4(idte, TCG_CALL_NO_RWG, void, env, i64, i64, i32) > DEF_HELPER_FLAGS_4(ipte, TCG_CALL_NO_RWG, void, env, i64, i64, i32) > DEF_HELPER_FLAGS_1(ptlb, TCG_CALL_NO_RWG, void, env) > DEF_HELPER_FLAGS_1(purge, TCG_CALL_NO_RWG, void, env) > -DEF_HELPER_2(lra, i64, env, i64) > +DEF_HELPER_3(lra, i64, env, i64, i64) > DEF_HELPER_1(per_check_exception, void, env) > DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64) > DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64) > diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c > index 84ad85212c9..94d93d7ea78 100644 > --- a/target/s390x/tcg/mem_helper.c > +++ b/target/s390x/tcg/mem_helper.c > @@ -2356,7 +2356,7 @@ void HELPER(purge)(CPUS390XState *env) > } > > /* load real address */ > -uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) > +uint64_t HELPER(lra)(CPUS390XState *env, uint64_t r1, uint64_t addr) > { > uint64_t asc = env->psw.mask & PSW_MASK_ASC; > uint64_t ret, tec; > @@ -2370,7 +2370,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) > exc = mmu_translate(env, addr, MMU_S390_LRA, asc, &ret, &flags, &tec); > if (exc) { > cc = 3; > - ret = exc | 0x80000000; > + ret = (r1 & 0xFFFFFFFF00000000) | exc | 0x80000000; ull missing for large constant? > } else { > cc = 0; > ret |= addr & ~TARGET_PAGE_MASK; > diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c > index 0cef6efbef4..a6079ab7b4f 100644 > --- a/target/s390x/tcg/translate.c > +++ b/target/s390x/tcg/translate.c > @@ -2932,7 +2932,7 @@ static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o) > > static DisasJumpType op_lra(DisasContext *s, DisasOps *o) > { > - gen_helper_lra(o->out, cpu_env, o->in2); > + gen_helper_lra(o->out, cpu_env, o->out, o->in2); > set_cc_static(s); > return DISAS_NEXT; > } Can't we use something like in1_r1 + wout_r1_32 instead ? *maybe* cleaner :)
On Tue, 2023-07-04 at 09:47 +0200, David Hildenbrand wrote: > On 03.07.23 17:50, Ilya Leoshkevich wrote: > > When a DAT error occurs, LRA is supposed to write the error > > information > > to the bottom 32 bits of R1, and leave the top 32 bits of R1 alone. > > > > Fix by passing the original value of R1 into helper and copying the > > top 32 bits to the return value. > > > > Fixes: d8fe4a9c284f ("target-s390: Convert LRA") > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > --- > > target/s390x/helper.h | 2 +- > > target/s390x/tcg/mem_helper.c | 4 ++-- > > target/s390x/tcg/translate.c | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h > > index 6bc01df73d7..05102578fc9 100644 > > --- a/target/s390x/helper.h > > +++ b/target/s390x/helper.h > > @@ -355,7 +355,7 @@ DEF_HELPER_FLAGS_4(idte, TCG_CALL_NO_RWG, void, > > env, i64, i64, i32) > > DEF_HELPER_FLAGS_4(ipte, TCG_CALL_NO_RWG, void, env, i64, i64, > > i32) > > DEF_HELPER_FLAGS_1(ptlb, TCG_CALL_NO_RWG, void, env) > > DEF_HELPER_FLAGS_1(purge, TCG_CALL_NO_RWG, void, env) > > -DEF_HELPER_2(lra, i64, env, i64) > > +DEF_HELPER_3(lra, i64, env, i64, i64) > > DEF_HELPER_1(per_check_exception, void, env) > > DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, > > i64) > > DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64) > > diff --git a/target/s390x/tcg/mem_helper.c > > b/target/s390x/tcg/mem_helper.c > > index 84ad85212c9..94d93d7ea78 100644 > > --- a/target/s390x/tcg/mem_helper.c > > +++ b/target/s390x/tcg/mem_helper.c > > @@ -2356,7 +2356,7 @@ void HELPER(purge)(CPUS390XState *env) > > } > > > > /* load real address */ > > -uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) > > +uint64_t HELPER(lra)(CPUS390XState *env, uint64_t r1, uint64_t > > addr) > > { > > uint64_t asc = env->psw.mask & PSW_MASK_ASC; > > uint64_t ret, tec; > > @@ -2370,7 +2370,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, > > uint64_t addr) > > exc = mmu_translate(env, addr, MMU_S390_LRA, asc, &ret, > > &flags, &tec); > > if (exc) { > > cc = 3; > > - ret = exc | 0x80000000; > > + ret = (r1 & 0xFFFFFFFF00000000) | exc | 0x80000000; > > ull missing for large constant? Will do. Just for my understanding, why is this necessary? The current code base tends towards using ULL, but it's a little bit inconsistent: $ git grep -i 0xfffffffff | wc -l 2338 $ git grep -i 0xfffffffff | grep -i -v ul | wc -l 95 > > > } else { > > cc = 0; > > ret |= addr & ~TARGET_PAGE_MASK; > > diff --git a/target/s390x/tcg/translate.c > > b/target/s390x/tcg/translate.c > > index 0cef6efbef4..a6079ab7b4f 100644 > > --- a/target/s390x/tcg/translate.c > > +++ b/target/s390x/tcg/translate.c > > @@ -2932,7 +2932,7 @@ static DisasJumpType op_lctlg(DisasContext > > *s, DisasOps *o) > > > > static DisasJumpType op_lra(DisasContext *s, DisasOps *o) > > { > > - gen_helper_lra(o->out, cpu_env, o->in2); > > + gen_helper_lra(o->out, cpu_env, o->out, o->in2); > > set_cc_static(s); > > return DISAS_NEXT; > > } > > Can't we use something like in1_r1 + wout_r1_32 instead ? *maybe* > cleaner :) > The problem is that we want all 64 bits for the non-error case.
On 7/4/23 10:05, Ilya Leoshkevich wrote: >>> + ret = (r1 & 0xFFFFFFFF00000000) | exc | 0x80000000; >> >> ull missing for large constant? > > Will do. > > Just for my understanding, why is this necessary? 32-bit host; you'll get a warning for the large constant. r~
>> >>> } else { >>> cc = 0; >>> ret |= addr & ~TARGET_PAGE_MASK; >>> diff --git a/target/s390x/tcg/translate.c >>> b/target/s390x/tcg/translate.c >>> index 0cef6efbef4..a6079ab7b4f 100644 >>> --- a/target/s390x/tcg/translate.c >>> +++ b/target/s390x/tcg/translate.c >>> @@ -2932,7 +2932,7 @@ static DisasJumpType op_lctlg(DisasContext >>> *s, DisasOps *o) >>> >>> static DisasJumpType op_lra(DisasContext *s, DisasOps *o) >>> { >>> - gen_helper_lra(o->out, cpu_env, o->in2); >>> + gen_helper_lra(o->out, cpu_env, o->out, o->in2); >>> set_cc_static(s); >>> return DISAS_NEXT; >>> } >> >> Can't we use something like in1_r1 + wout_r1_32 instead ? *maybe* >> cleaner :) >> > > The problem is that we want all 64 bits for the non-error case. > Ah, I missed that detail, thanks.
diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 6bc01df73d7..05102578fc9 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -355,7 +355,7 @@ DEF_HELPER_FLAGS_4(idte, TCG_CALL_NO_RWG, void, env, i64, i64, i32) DEF_HELPER_FLAGS_4(ipte, TCG_CALL_NO_RWG, void, env, i64, i64, i32) DEF_HELPER_FLAGS_1(ptlb, TCG_CALL_NO_RWG, void, env) DEF_HELPER_FLAGS_1(purge, TCG_CALL_NO_RWG, void, env) -DEF_HELPER_2(lra, i64, env, i64) +DEF_HELPER_3(lra, i64, env, i64, i64) DEF_HELPER_1(per_check_exception, void, env) DEF_HELPER_FLAGS_3(per_branch, TCG_CALL_NO_RWG, void, env, i64, i64) DEF_HELPER_FLAGS_2(per_ifetch, TCG_CALL_NO_RWG, void, env, i64) diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c index 84ad85212c9..94d93d7ea78 100644 --- a/target/s390x/tcg/mem_helper.c +++ b/target/s390x/tcg/mem_helper.c @@ -2356,7 +2356,7 @@ void HELPER(purge)(CPUS390XState *env) } /* load real address */ -uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) +uint64_t HELPER(lra)(CPUS390XState *env, uint64_t r1, uint64_t addr) { uint64_t asc = env->psw.mask & PSW_MASK_ASC; uint64_t ret, tec; @@ -2370,7 +2370,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr) exc = mmu_translate(env, addr, MMU_S390_LRA, asc, &ret, &flags, &tec); if (exc) { cc = 3; - ret = exc | 0x80000000; + ret = (r1 & 0xFFFFFFFF00000000) | exc | 0x80000000; } else { cc = 0; ret |= addr & ~TARGET_PAGE_MASK; diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c index 0cef6efbef4..a6079ab7b4f 100644 --- a/target/s390x/tcg/translate.c +++ b/target/s390x/tcg/translate.c @@ -2932,7 +2932,7 @@ static DisasJumpType op_lctlg(DisasContext *s, DisasOps *o) static DisasJumpType op_lra(DisasContext *s, DisasOps *o) { - gen_helper_lra(o->out, cpu_env, o->in2); + gen_helper_lra(o->out, cpu_env, o->out, o->in2); set_cc_static(s); return DISAS_NEXT; }
When a DAT error occurs, LRA is supposed to write the error information to the bottom 32 bits of R1, and leave the top 32 bits of R1 alone. Fix by passing the original value of R1 into helper and copying the top 32 bits to the return value. Fixes: d8fe4a9c284f ("target-s390: Convert LRA") Cc: qemu-stable@nongnu.org Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- target/s390x/helper.h | 2 +- target/s390x/tcg/mem_helper.c | 4 ++-- target/s390x/tcg/translate.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)