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

Richard Henderson posted 5 patches 4 years, 7 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>, Cornelia Huck <cohuck@redhat.com>, David Hildenbrand <david@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Christian Borntraeger <borntraeger@de.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Thomas Huth <thuth@redhat.com>
[PATCH 2/5] target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
Posted by Richard Henderson 4 years, 7 months ago
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;
-- 
2.25.1


Re: [PATCH 2/5] target/s390x: Do not modify cpu state in s390_cpu_get_psw_mask
Posted by David Hildenbrand 4 years, 7 months ago
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