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

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 20 Nov 2019 06:43:20 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's move the resets into one function and switch by type, so we can
> use fallthroughs for shared reset actions.

Doing that makes sense.

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |   3 +
>  target/s390x/cpu.c         | 111 ++++++++++++++++---------------------
>  2 files changed, 52 insertions(+), 62 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad..c1d1440272 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -348,6 +348,9 @@ static void s390_machine_reset(MachineState *machine)
>          break;
>      case S390_RESET_LOAD_NORMAL:
>          CPU_FOREACH(t) {
> +            if (t == cs) {
> +                continue;
> +            }

Hm, why is this needed now?

>              run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>          }
>          subsystem_reset();
> 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() */

Not sure if it would be worth keeping these comments near by the
calling functions.

> -static void s390_cpu_reset(CPUState *s)
> +enum {
> +    S390_CPU_RESET_NORMAL,
> +    S390_CPU_RESET_INITIAL,
> +    S390_CPU_RESET_CLEAR,
> +};

Maybe make this into a proper type, so you can use type checking?

(...)

The diff is a bit hard to read, but the change seems fine at a glance.