Re: [PATCH 2/5] target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask

[David Hildenbrand]

On 15.06.21 05:07, Richard Henderson wrote:
> We want to use this function for debugging, and debug should
> not modify cpu state (even non-architectural cpu state) lest
> we introduce heisenbugs.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/s390x/helper.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index d311903b94..559fc3573f 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -321,12 +321,12 @@ uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
>       uint64_t r = env->psw.mask;
>   
>       if (tcg_enabled()) {
> -        env->cc_op = calc_cc(env, env->cc_op, env->cc_src, env->cc_dst,
> -                             env->cc_vr);
> +        uint64_t cc = calc_cc(env, env->cc_op, env->cc_src,
> +                              env->cc_dst, env->cc_vr);
>   
> +        assert(cc <= 3);
>           r &= ~PSW_MASK_CC;
> -        assert(!(env->cc_op & ~3));
> -        r |= (uint64_t)env->cc_op << 44;
> +        r |= cc << 44;
>       }
>   
>       return r;

I wonder if this can actually suboptimal performance-wise, because we 
might be recomputing the same thing. However, I guess it's rather a 
corner case

Reviewed-by: David Hildenbrand <david@redhat.com>

--
Thanks,

David / dhildenb