Re: [PATCH 05/12] target/s390x: Fix LRA overwriting the top 32 bits on DAT error

[Ilya Leoshkevich]

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.