Re: [PATCH] target/s390x: Use explicit stores to env->cc_op

[Philippe Mathieu-Daudé]

On 6/29/21 2:29 AM, Richard Henderson wrote:
> Remove cc_op as a TCG global and store to it directly.
> This will help simplify the fix for some fpu bugs.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/s390x/helper.h        |  98 ++++++-------
>  target/s390x/crypto_helper.c |   6 +-
>  target/s390x/fpu_helper.c    |  40 +++---
>  target/s390x/mem_helper.c    | 265 ++++++++++++++++++-----------------
>  target/s390x/misc_helper.c   |  39 +++---

Minor one comment in tprot(), I reviewed the helper changes.

>  target/s390x/translate.c     | 218 ++++++++++++++--------------

For this file I got quickly lost (too many changes in a single
patch, but I see it is hardly splitable). I might revisit to
complete the review.

>  6 files changed, 344 insertions(+), 322 deletions(-)
...

> -uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
> +void HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
>  {
>      S390CPU *cpu = env_archcpu(env);
>      CPUState *cs = env_cpu(env);
> +    int cc;

This new variable isn't really useful, can you store to env->cc_op
directly instead?

>  
>      /*
>       * TODO: we currently don't handle all access protection types
> @@ -2144,7 +2143,8 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, 
> uint64_t a2)
>       */
>      if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
>          /* Fetching permitted; storing permitted */
> -        return 0;
> +        env->cc_op = 0;
> +        return;
>      }
>  
>      if (env->int_pgm_code == PGM_PROTECTION) {
> @@ -2152,7 +2152,8 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, 
> uint64_t a2)
>          cs->exception_index = -1;
>          if (!s390_cpu_virt_mem_check_read(cpu, a1, 0, 1)) {
>              /* Fetching permitted; storing not permitted */
> -            return 1;
> +            env->cc_op = 1;
> +            return;
>          }
>      }
>  
> @@ -2160,17 +2161,21 @@ uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t 
> a1, uint64_t a2)
>      case PGM_PROTECTION:
>          /* Fetching not permitted; storing not permitted */
>          cs->exception_index = -1;
> -        return 2;
> +        cc = 2;
> +        break;
>      case PGM_ADDRESSING:
>      case PGM_TRANS_SPEC:
>          /* exceptions forwarded to the guest */
>          s390_cpu_virt_mem_handle_exc(cpu, GETPC());
> -        return 0;
> +        cc = 0;
> +        break;
> +    default:
> +        /* Translation not available */
> +        cs->exception_index = -1;
> +        cc = 3;
> +        break;
>      }
> -
> -    /* Translation not available */
> -    cs->exception_index = -1;
> -    return 3;
> +    env->cc_op = cc;
>  }
...