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

[David Hildenbrand]

On 21.06.19 16:09, 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 | 123 
> ++++++++++++++++++++++++-----------------------------
>  1 file changed, 55 insertions(+), 68 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index f2d93644d5..2619c60236 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -81,89 +81,76 @@ static void s390_cpu_load_normal(CPUState *s)
>  }
>  #endif
>  
> -/* S390CPUClass::cpu_reset() */
> -static void s390_cpu_reset(CPUState *s)
> +enum {
> +    S390_CPU_RESET_NORMAL,
> +    S390_CPU_RESET_INITIAL,
> +    S390_CPU_RESET_CLEAR,
> +};
> +
> +static void s390_cpu_reset(CPUState *s, uint8_t type)
>  {
>      S390CPU *cpu = S390_CPU(s);
> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>      CPUS390XState *env = &cpu->env;
> +    S390CPUClass *scc = S390_CPU_CLASS(cpu);
>  
> -    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);
> -}
>  
> -/* S390CPUClass::initial_reset() */
> -static void s390_cpu_initial_reset(CPUState *s)
> -{
> -    S390CPU *cpu = S390_CPU(s);
> -    CPUS390XState *env = &cpu->env;
> -
> -    s390_cpu_reset(s);
> -    /* initial reset does not clear everything! */
> -    memset(&env->start_initial_reset_fields, 0,
> +    /* Clear registers */
> +    if (type == S390_CPU_RESET_INITIAL) {
> +        /* initial reset does not clear everything! */
> +        memset(&env->start_initial_reset_fields, 0,
>          offsetof(CPUS390XState, end_reset_fields) -
>          offsetof(CPUS390XState, start_initial_reset_fields));
> +    } else if (type == S390_CPU_RESET_INITIAL) {

Two checks for "type == S390_CPU_RESET_INITIAL", this can't be right ;)

> +        memset(env, 0, offsetof(CPUS390XState, end_reset_fields));

Maybe we should just go ahead and reset fields explicitly. This makes
things *much* more readable. You can actually verify what each reset
type resets. Performance should be neglectable.

Eventually we can then pack everything into one switch-case with
fall-throughs.

> +    }
>  
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
> +    /* Set initial values after clearing */
> +    switch (type) {
> +    case S390_CPU_RESET_CLEAR:
> +#if defined(CONFIG_USER_ONLY)
> +        /* user mode should always be allowed to use the full FPU */
> +        env->cregs[0] |= CR0_AFP;
> +        if (s390_has_feat(S390_FEAT_VECTOR)) {
> +            env->cregs[0] |= CR0_VECTOR;
> +        }
> +#endif

1. For CONFIG_USER_ONLY, there are no SIGP/DIAG triggered resets. There
should only be one "QOM triggered" resets. IOW, you can move this out of
the switch-case, to the very end of this function if I'm not wrong.

2. Can you add "fall through" comments?

> +    case S390_CPU_RESET_INITIAL:
> +        /* architectured initial values for CR 0 and 14 */
> +        env->cregs[0] = CR0_RESET;

You are overwriting "env->cregs[0] |= CR0_AFP;" ... here. Should be
solved by reworking that as suggested.

> +        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);
> -
> -    /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled()) {
> -        kvm_s390_reset_vcpu(cpu);
> +        /* 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);
> +        /* Reset state inside the kernel that we cannot access yet from 
> QEMU. */
> +        if (kvm_enabled()) {
> +            kvm_s390_reset_vcpu(cpu);
> +        }
> +    case S390_CPU_RESET_NORMAL:
> +        env->pfault_token = -1UL;
> +        env->bpbc = false;
> +        break;
>      }
>  }
>  
> -/* CPUClass:reset() */
> -static void s390_cpu_full_reset(CPUState *s)
> +static void s390_cpu_reset_normal(CPUState *s)
>  {
> -    S390CPU *cpu = S390_CPU(s);
> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> -    CPUS390XState *env = &cpu->env;
> +    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
> +}
>  
> -    scc->parent_reset(s);
> -    cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> +static void s390_cpu_reset_initial(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
> +}
>  
> -    memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
> -
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
> -
> -#if defined(CONFIG_USER_ONLY)
> -    /* user mode should always be allowed to use the full FPU */
> -    env->cregs[0] |= CR0_AFP;
> -    if (s390_has_feat(S390_FEAT_VECTOR)) {
> -        env->cregs[0] |= CR0_VECTOR;
> -    }
> -#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);
> -    }
> +static void s390_cpu_reset_clear(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -470,9 +457,9 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
> *data)
>  #if !defined(CONFIG_USER_ONLY)
>      scc->load_normal = s390_cpu_load_normal;
>  #endif
> -    scc->cpu_reset = s390_cpu_reset;
> -    scc->initial_cpu_reset = s390_cpu_initial_reset;
> -    cc->reset = s390_cpu_full_reset;
> +    scc->cpu_reset = s390_cpu_reset_normal;
> +    scc->initial_cpu_reset = s390_cpu_reset_initial;
> +    cc->reset = s390_cpu_reset_clear;
>      cc->class_by_name = s390_cpu_class_by_name,
>      cc->has_work = s390_cpu_has_work;
>  #ifdef CONFIG_TCG
> 


-- 

Thanks,

David / dhildenb