Re: [qemu-s390x] [PATCH] s390x: Cleanup cpu resets

[David Hildenbrand]

On 21.06.19 13:28, Janosch Frank wrote:
> S390x CPU resets are cascading, hence initial reset and clear reset
> need to do all things a plain reset does.
> 
> Let's make sure that's done and consolidate some reset code.
> Also let's always explicitly set boolean cpu state members to their
> type value (e.g. bpbc).
> 
> Signed-off-by: Janosch Frank <address@hidden>
> ---
>  target/s390x/cpu.c | 79 
> +++++++++++++++++++++++++++---------------------------
>  1 file changed, 39 insertions(+), 40 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index f2d93644d5..b96fbe4838 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -81,18 +81,44 @@ static void s390_cpu_load_normal(CPUState *s)
>  }
>  #endif
>  
> +static void s390_reset_pre_memset(CPUState *s) {
> +    S390CPU *cpu = S390_CPU(s);
> +    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> +
> +    scc->parent_reset(s);
> +    cpu->env.sigp_order = 0;
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> +}
> +
> +/* Values that need to be set after the memset for all reset types */
> +static void s390_reset_post_memset_normal(CPUS390XState *env) {
> +    env->pfault_token = -1UL;
> +    env->bpbc = false;
> +}
> +
> +/* Values that need to be reset additionally for initial and clear resets */
> +static void s390_reset_post_memset_initial(CPUS390XState *env)
> +{
> +    s390_reset_post_memset_normal(env);
> +    /* architectured initial values for CR 0 and 14 */
> +    env->cregs[0] = CR0_RESET;
> +    env->cregs[14] = CR14_RESET;
> +
> +    /* architectured initial value for Breaking-Event-Address register */
> +    env->gbea = 1;
> +    /* tininess for underflow is detected before rounding */
> +    set_float_detect_tininess(float_tininess_before_rounding,
> +                              &env->fpu_status);
> +}
> +
>  /* S390CPUClass::cpu_reset() */
>  static void s390_cpu_reset(CPUState *s)
>  {
>      S390CPU *cpu = S390_CPU(s);
> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>      CPUS390XState *env = &cpu->env;
>  
> -    env->pfault_token = -1UL;
> -    env->bpbc = false;
> -    scc->parent_reset(s);
> -    cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> +    s390_reset_pre_memset(s);
> +    s390_reset_post_memset_normal(env);
>  }
>  
>  /* S390CPUClass::initial_reset() */
> @@ -101,24 +127,12 @@ static void s390_cpu_initial_reset(CPUState *s)
>      S390CPU *cpu = S390_CPU(s);
>      CPUS390XState *env = &cpu->env;
>  
> -    s390_cpu_reset(s);
> +    s390_reset_pre_memset(s);
>      /* initial reset does not clear everything! */
>      memset(&env->start_initial_reset_fields, 0,
>          offsetof(CPUS390XState, end_reset_fields) -
>          offsetof(CPUS390XState, start_initial_reset_fields));
> -
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
> -
> -    /* architectured initial value for Breaking-Event-Address register */
> -    env->gbea = 1;
> -
> -    env->pfault_token = -1UL;
> -
> -    /* tininess for underflow is detected before rounding */
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -                              &env->fpu_status);
> +    s390_reset_post_memset_initial(env);
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>      if (kvm_enabled()) {
> @@ -127,22 +141,16 @@ static void s390_cpu_initial_reset(CPUState *s)
>  }
>  
>  /* CPUClass:reset() */
> -static void s390_cpu_full_reset(CPUState *s)
> +static void s390_cpu_clear_reset(CPUState *s)
>  {
>      S390CPU *cpu = S390_CPU(s);
> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>      CPUS390XState *env = &cpu->env;
>  
> -    scc->parent_reset(s);
> -    cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> -
> +    s390_reset_pre_memset(s);
>      memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
> +    s390_reset_post_memset_initial(env);
>  
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
> -
> +    /* Specific to clear reset */
>  #if defined(CONFIG_USER_ONLY)
>      /* user mode should always be allowed to use the full FPU */
>      env->cregs[0] |= CR0_AFP;
> @@ -151,15 +159,6 @@ static void s390_cpu_full_reset(CPUState *s)
>      }
>  #endif
>  
> -    /* architectured initial value for Breaking-Event-Address register */
> -    env->gbea = 1;
> -
> -    env->pfault_token = -1UL;
> -
> -    /* tininess for underflow is detected before rounding */
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -                              &env->fpu_status);
> -
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>      if (kvm_enabled()) {
>          kvm_s390_reset_vcpu(cpu);
> @@ -472,7 +471,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
> *data)
>  #endif
>      scc->cpu_reset = s390_cpu_reset;
>      scc->initial_cpu_reset = s390_cpu_initial_reset;
> -    cc->reset = s390_cpu_full_reset;
> +    cc->reset = s390_cpu_clear_reset;
>      cc->class_by_name = s390_cpu_class_by_name,
>      cc->has_work = s390_cpu_has_work;
>  #ifdef CONFIG_TCG
> 

Hmm, can we instead have a single function and pass in a S390_RESET_TYPE
enum value from the resets? Then we can avoid all these functions and
have a better overview of what is done in which order.

Just an idea.

-- 

Thanks,

David / dhildenb