[01/15] s390x: Cleanup cpu resets
diff mbox series

Message ID 20191120114334.2287-2-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Protected Virtualization support
Related show

Commit Message

Janosch Frank Nov. 20, 2019, 11:43 a.m. UTC
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 <frankja@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |   3 +
 target/s390x/cpu.c         | 111 ++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 62 deletions(-)

Comments

Cornelia Huck Nov. 21, 2019, 11:10 a.m. UTC | #1
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.
Janosch Frank Nov. 21, 2019, 11:32 a.m. UTC | #2
On 11/21/19 12:10 PM, Cornelia Huck wrote:
> 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?

The Ultravisor checks which reset is done to which cpu.
So blindly resetting the calling cpu with a normal reset to then do a
clear/initial reset will return an error.

> 
>>              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?

Ok

> 
> (...)
> 
> The diff is a bit hard to read, but the change seems fine at a glance.
>
Cornelia Huck Nov. 21, 2019, 12:18 p.m. UTC | #3
On Thu, 21 Nov 2019 12:32:38 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/21/19 12:10 PM, Cornelia Huck wrote:
> > 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?  
> 
> The Ultravisor checks which reset is done to which cpu.
> So blindly resetting the calling cpu with a normal reset to then do a
> clear/initial reset will return an error.

Let's split this change out, then? The rest of the patch is a simple
refactoring.
Thomas Huth Nov. 21, 2019, 12:53 p.m. UTC | #4
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 <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/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
Janosch Frank Nov. 21, 2019, 1:11 p.m. UTC | #5
On 11/21/19 1:53 PM, Thomas Huth wrote:
> 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 <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/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.

Sure

> 
>>  {
>>      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?

There was a reason for that, time to load it from cold storage...

> 
> [...]
> 
> 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:

I tried that and I prefer the version in the patch.

> 
> 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
>
Thomas Huth Nov. 21, 2019, 1:17 p.m. UTC | #6
On 21/11/2019 14.11, Janosch Frank wrote:
> On 11/21/19 1:53 PM, Thomas Huth wrote:
>> 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.
[...]
>>> +        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?
> 
> There was a reason for that, time to load it from cold storage...

I just noticed that you rework this in patch 10/15, so it indeed makes
sense to keep it outside of the switch-case-statement above...

>> [...]
>>
>> 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:
> 
> I tried that and I prefer the version in the patch.

... and with patch 10 in mind, it indeed also makes more sense to *not*
use the chaining that I've suggested. So never mind, your switch with
the fallthrough statements is just fine.

 Thomas

Patch
diff mbox series

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;
+            }
             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() */
-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;
 
-    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 */
+        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);
     }
-}
-
-/* CPUClass:reset() */
-static void s390_cpu_full_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);
-
-    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 */
@@ -151,20 +137,21 @@  static void s390_cpu_full_reset(CPUState *s)
         env->cregs[0] |= CR0_VECTOR;
     }
 #endif
+}
 
-    /* architectured initial value for Breaking-Event-Address register */
-    env->gbea = 1;
+static void s390_cpu_reset_normal(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
+}
 
-    env->pfault_token = -1UL;
+static void s390_cpu_reset_initial(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
+}
 
-    /* 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)
@@ -473,9 +460,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