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

[David Hildenbrand <david@redhat.com>]

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 :)

-- 
Cheers,

David / dhildenb