Re: [PATCH 01/15] s390x: Cleanup cpu resets

[Thomas Huth]

On 20/11/2019 12.43, Janosch Frank wrote:
> Let's move the resets into one function and switch by type, so we can
> use fallthroughs for shared reset actions.
> 
> Signed-off-by: Janosch Frank <address@hidden>
> ---
>  hw/s390x/s390-virtio-ccw.c |   3 +
>  target/s390x/cpu.c         | 111 ++++++++++++++++---------------------
>  2 files changed, 52 insertions(+), 62 deletions(-)
[...]
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 3abe7e80fd..10d5b915d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,67 +82,53 @@ 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)

Please give the enum a name and use that instead of uint8_t for "type".
Or at least make it an "int". uint8_t is not really appropriate here.

>  {
>      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);
> -}
>  
> -/* S390CPUClass::initial_reset() */
> -static void s390_cpu_initial_reset(CPUState *s)
> -{
> -    S390CPU *cpu = S390_CPU(s);
> -    CPUS390XState *env = &cpu->env;
> +    /* Set initial values after clearing */
> +    switch (type) {
> +    case S390_CPU_RESET_CLEAR:
> +        /* Fallthrough will clear the rest */

I think you could drop the above comment, since /* Fallthrough */ two
lines later should be enough.

> +        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
> +        /* Fallthrough */
> +    case S390_CPU_RESET_INITIAL:
> +        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;
>  
> -    s390_cpu_reset(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);
> +        /* 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);
> +        /* Fallthrough */
> +    case S390_CPU_RESET_NORMAL:
> +        env->pfault_token = -1UL;
> +        env->bpbc = false;
> +        break;
> +    }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled()) {
> -        kvm_s390_reset_vcpu(cpu);
> +    if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
> +                          type == S390_CPU_RESET_INITIAL)) {
> +            kvm_s390_reset_vcpu(cpu);
>      }

Why don't you simply move that into the switch-case statement, too?

[...]

Anyway, re-using code is of course a good idea, but I wonder whether it
would be nicer to keep most things in place, and then simply chain the
functions like this:

static void s390_cpu_reset_normal(CPUState *s)
{
   ...
}

static void s390_cpu_reset_initial(CPUState *s)
{
    ...
    s390_cpu_reset_normal(s);
    ...
}

static void s390_cpu_reset_clear(CPUState *s)
{
    ...
    s390_cpu_reset_initial()
    ...
}

Just my 0.02 €, but at least for me, that's easier to understand than a
switch-case statement with fallthroughs inbetween.

 Thomas