[PATCH v3 0/5] s390x: Reset cleanup

[PATCH v3 0/5] s390x: Reset cleanup

[Janosch Frank]

Adding comments and reordering code for better readability in the
diag308 and machine reset functions.

v3:
	* Added user only afp set again
	* Added rev-by
	* Minor review cleanups

Janosch Frank (5):
  s390x: Don't do a normal reset on the initial cpu
  s390x: Move reset normal to shared reset handler
  s390x: Move initial reset
  s390x: Move clear reset
  s390x: Beautify diag308 handling

 hw/s390x/s390-virtio-ccw.c |   3 ++
 target/s390x/cpu-qom.h     |   9 +++-
 target/s390x/cpu.c         | 104 +++++++++++++------------------------
 target/s390x/cpu.h         |   4 +-
 target/s390x/diag.c        |  54 +++++++++++--------
 target/s390x/sigp.c        |   4 +-
 6 files changed, 83 insertions(+), 95 deletions(-)

-- 
2.20.1

[PATCH v3 1/5] s390x: Don't do a normal reset on the initial cpu

[Janosch Frank]

The initiating cpu needs to be reset with an initial reset. While
doing a normal reset followed by a initial reset is not wrong per se,
the Ultravisor will only allow the correct reset to be performed.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 3 +++
 1 file changed, 3 insertions(+)

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();
-- 
2.20.1

Re: [PATCH v3 1/5] s390x: Don't do a normal reset on the initial cpu

[Cornelia Huck]

On Mon, 25 Nov 2019 04:03:44 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> The initiating cpu needs to be reset with an initial reset. While
> doing a normal reset followed by a initial reset is not wrong per se,
> the Ultravisor will only allow the correct reset to be performed.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> 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();

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

[PATCH v3 2/5] s390x: Move reset normal to shared reset handler

[Janosch Frank]

Let's start moving the cpu reset functions into a single function with
a switch/case, so we can use fallthroughs and share more code between
resets.

This patch introduces the reset function by renaming cpu_reset() and
cleaning up leftovers.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h |  6 +++++-
 target/s390x/cpu.c     | 16 ++++++++++------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index b809ec8418..f3b71bac67 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -34,6 +34,10 @@
 typedef struct S390CPUModel S390CPUModel;
 typedef struct S390CPUDef S390CPUDef;
 
+typedef enum cpu_reset_type {
+    S390_CPU_RESET_NORMAL,
+} cpu_reset_type;
+
 /**
  * S390CPUClass:
  * @parent_realize: The parent class' realize handler.
@@ -57,7 +61,7 @@ typedef struct S390CPUClass {
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
     void (*load_normal)(CPUState *cpu);
-    void (*cpu_reset)(CPUState *cpu);
+    void (*reset)(CPUState *cpu, cpu_reset_type type);
     void (*initial_cpu_reset)(CPUState *cpu);
 } S390CPUClass;
 
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3abe7e80fd..8d35a2a723 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -82,18 +82,22 @@ static void s390_cpu_load_normal(CPUState *s)
 }
 #endif
 
-/* S390CPUClass::cpu_reset() */
-static void s390_cpu_reset(CPUState *s)
+/* S390CPUClass::reset() */
+static void s390_cpu_reset(CPUState *s, cpu_reset_type 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);
+
+    switch (type) {
+    case S390_CPU_RESET_NORMAL:
+        env->pfault_token = -1UL;
+        env->bpbc = false;
+    }
 }
 
 /* S390CPUClass::initial_reset() */
@@ -102,7 +106,7 @@ static void s390_cpu_initial_reset(CPUState *s)
     S390CPU *cpu = S390_CPU(s);
     CPUS390XState *env = &cpu->env;
 
-    s390_cpu_reset(s);
+    s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
     /* initial reset does not clear everything! */
     memset(&env->start_initial_reset_fields, 0,
         offsetof(CPUS390XState, end_reset_fields) -
@@ -473,7 +477,7 @@ 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->reset = s390_cpu_reset;
     scc->initial_cpu_reset = s390_cpu_initial_reset;
     cc->reset = s390_cpu_full_reset;
     cc->class_by_name = s390_cpu_class_by_name,
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 17460ed7b3..18123dfd5b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -741,7 +741,7 @@ static inline void s390_do_cpu_reset(CPUState *cs, run_on_cpu_data arg)
 {
     S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
 
-    scc->cpu_reset(cs);
+    scc->reset(cs, S390_CPU_RESET_NORMAL);
 }
 
 static inline void s390_do_cpu_initial_reset(CPUState *cs, run_on_cpu_data arg)
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 2ce22d4dc1..850139b9cd 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -266,7 +266,7 @@ static void sigp_cpu_reset(CPUState *cs, run_on_cpu_data arg)
     SigpInfo *si = arg.host_ptr;
 
     cpu_synchronize_state(cs);
-    scc->cpu_reset(cs);
+    scc->reset(cs, S390_CPU_RESET_NORMAL);
     cpu_synchronize_post_reset(cs);
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 }
-- 
2.20.1

Re: [PATCH v3 2/5] s390x: Move reset normal to shared reset handler

[Cornelia Huck]

On Mon, 25 Nov 2019 04:03:45 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's start moving the cpu reset functions into a single function with
> a switch/case, so we can use fallthroughs and share more code between
> resets.
> 
> This patch introduces the reset function by renaming cpu_reset() and
> cleaning up leftovers.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu-qom.h |  6 +++++-
>  target/s390x/cpu.c     | 16 ++++++++++------
>  target/s390x/cpu.h     |  2 +-
>  target/s390x/sigp.c    |  2 +-
>  4 files changed, 17 insertions(+), 9 deletions(-)
> 

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 3abe7e80fd..8d35a2a723 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,18 +82,22 @@ static void s390_cpu_load_normal(CPUState *s)
>  }
>  #endif
>  
> -/* S390CPUClass::cpu_reset() */
> -static void s390_cpu_reset(CPUState *s)
> +/* S390CPUClass::reset() */
> +static void s390_cpu_reset(CPUState *s, cpu_reset_type 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);
> +
> +    switch (type) {
> +    case S390_CPU_RESET_NORMAL:
> +        env->pfault_token = -1UL;
> +        env->bpbc = false;

I'm wondering whether we want a default case here to catch programming
errors.

> +    }
>  }
>  
>  /* S390CPUClass::initial_reset() */

Re: [PATCH v3 2/5] s390x: Move reset normal to shared reset handler

[David Hildenbrand]

On 25.11.19 14:19, Cornelia Huck wrote:
> On Mon, 25 Nov 2019 04:03:45 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Let's start moving the cpu reset functions into a single function with
>> a switch/case, so we can use fallthroughs and share more code between
>> resets.
>>
>> This patch introduces the reset function by renaming cpu_reset() and
>> cleaning up leftovers.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  target/s390x/cpu-qom.h |  6 +++++-
>>  target/s390x/cpu.c     | 16 ++++++++++------
>>  target/s390x/cpu.h     |  2 +-
>>  target/s390x/sigp.c    |  2 +-
>>  4 files changed, 17 insertions(+), 9 deletions(-)
>>
> 
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 3abe7e80fd..8d35a2a723 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -82,18 +82,22 @@ static void s390_cpu_load_normal(CPUState *s)
>>  }
>>  #endif
>>  
>> -/* S390CPUClass::cpu_reset() */
>> -static void s390_cpu_reset(CPUState *s)
>> +/* S390CPUClass::reset() */
>> +static void s390_cpu_reset(CPUState *s, cpu_reset_type 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);
>> +
>> +    switch (type) {
>> +    case S390_CPU_RESET_NORMAL:
>> +        env->pfault_token = -1UL;
>> +        env->bpbc = false;
> 
> I'm wondering whether we want a default case here to catch programming
> errors.

+1, g_assert_not_reached().


-- 
Thanks,

David / dhildenb

[PATCH v3 3/5] s390x: Move initial reset

[Janosch Frank]

Let's move the intial reset into the reset handler and cleanup
afterwards.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 44 ++++++++++++++++--------------------------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 20 insertions(+), 30 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index f3b71bac67..6f0a12042e 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -36,6 +36,7 @@ typedef struct S390CPUDef S390CPUDef;
 
 typedef enum cpu_reset_type {
     S390_CPU_RESET_NORMAL,
+    S390_CPU_RESET_INITIAL,
 } cpu_reset_type;
 
 /**
@@ -62,7 +63,6 @@ typedef struct S390CPUClass {
     void (*parent_reset)(CPUState *cpu);
     void (*load_normal)(CPUState *cpu);
     void (*reset)(CPUState *cpu, cpu_reset_type type);
-    void (*initial_cpu_reset)(CPUState *cpu);
 } S390CPUClass;
 
 typedef struct S390CPU S390CPU;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 8d35a2a723..9f009b226c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -94,36 +94,27 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 
     switch (type) {
+    case 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));
+
+        /* architectured initial value for Breaking-Event-Address register */
+        env->gbea = 1;
+
+        /* architectured initial values for CR 0 and 14 */
+        env->cregs[0] = CR0_RESET;
+        env->cregs[14] = CR14_RESET;
+
+        /* tininess for underflow is detected before rounding */
+        set_float_detect_tininess(float_tininess_before_rounding,
+                                  &env->fpu_status);
+       /* fall through */
     case S390_CPU_RESET_NORMAL:
         env->pfault_token = -1UL;
         env->bpbc = false;
     }
-}
-
-/* S390CPUClass::initial_reset() */
-static void s390_cpu_initial_reset(CPUState *s)
-{
-    S390CPU *cpu = S390_CPU(s);
-    CPUS390XState *env = &cpu->env;
-
-    s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
-    /* 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);
 
     /* Reset state inside the kernel that we cannot access yet from QEMU. */
     if (kvm_enabled()) {
@@ -478,7 +469,6 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->load_normal = s390_cpu_load_normal;
 #endif
     scc->reset = s390_cpu_reset;
-    scc->initial_cpu_reset = s390_cpu_initial_reset;
     cc->reset = s390_cpu_full_reset;
     cc->class_by_name = s390_cpu_class_by_name,
     cc->has_work = s390_cpu_has_work;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 18123dfd5b..d2af13b345 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -748,7 +748,7 @@ static inline void s390_do_cpu_initial_reset(CPUState *cs, run_on_cpu_data arg)
 {
     S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
 
-    scc->initial_cpu_reset(cs);
+    scc->reset(cs, S390_CPU_RESET_INITIAL);
 }
 
 static inline void s390_do_cpu_load_normal(CPUState *cs, run_on_cpu_data arg)
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 850139b9cd..727875bb4a 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -254,7 +254,7 @@ static void sigp_initial_cpu_reset(CPUState *cs, run_on_cpu_data arg)
     SigpInfo *si = arg.host_ptr;
 
     cpu_synchronize_state(cs);
-    scc->initial_cpu_reset(cs);
+    scc->reset(cs, S390_CPU_RESET_INITIAL);
     cpu_synchronize_post_reset(cs);
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 }
-- 
2.20.1

[PATCH v3 4/5] s390x: Move clear reset

[Janosch Frank]

Let's also move the clear reset function into the reset handler.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu-qom.h |  1 +
 target/s390x/cpu.c     | 58 +++++++++++++-----------------------------
 2 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index 6f0a12042e..dbe5346ec9 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -37,6 +37,7 @@ typedef struct S390CPUDef S390CPUDef;
 typedef enum cpu_reset_type {
     S390_CPU_RESET_NORMAL,
     S390_CPU_RESET_INITIAL,
+    S390_CPU_RESET_CLEAR,
 } cpu_reset_type;
 
 /**
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 9f009b226c..4853cef204 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -94,6 +94,9 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 
     switch (type) {
+    case S390_CPU_RESET_CLEAR:
+        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
+        /* fall through */
     case S390_CPU_RESET_INITIAL:
         /* initial reset does not clear everything! */
         memset(&env->start_initial_reset_fields, 0,
@@ -107,6 +110,14 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
         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
+
         /* tininess for underflow is detected before rounding */
         set_float_detect_tininess(float_tininess_before_rounding,
                                   &env->fpu_status);
@@ -122,46 +133,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     }
 }
 
-/* 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 */
-    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);
-    }
-}
-
 #if !defined(CONFIG_USER_ONLY)
 static void s390_cpu_machine_reset_cb(void *opaque)
 {
@@ -453,6 +424,11 @@ static Property s390x_cpu_properties[] = {
     DEFINE_PROP_END_OF_LIST()
 };
 
+static void s390_cpu_reset_clear(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
+}
+
 static void s390_cpu_class_init(ObjectClass *oc, void *data)
 {
     S390CPUClass *scc = S390_CPU_CLASS(oc);
@@ -469,7 +445,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     scc->load_normal = s390_cpu_load_normal;
 #endif
     scc->reset = s390_cpu_reset;
-    cc->reset = s390_cpu_full_reset;
+    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
-- 
2.20.1

Re: [PATCH v3 4/5] s390x: Move clear reset

[Cornelia Huck]

On Mon, 25 Nov 2019 04:03:47 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's also move the clear reset function into the reset handler.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu-qom.h |  1 +
>  target/s390x/cpu.c     | 58 +++++++++++++-----------------------------
>  2 files changed, 18 insertions(+), 41 deletions(-)
> 

> @@ -453,6 +424,11 @@ static Property s390x_cpu_properties[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> +static void s390_cpu_reset_clear(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
> +}
> +
>  static void s390_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      S390CPUClass *scc = S390_CPU_CLASS(oc);
> @@ -469,7 +445,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      scc->load_normal = s390_cpu_load_normal;
>  #endif
>      scc->reset = s390_cpu_reset;
> -    cc->reset = s390_cpu_full_reset;
> +    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

One thing I liked about the previous naming is that it is more obvious
that the clear reset is actually the full reset of a cpu. Not sure if
keeping that is better than matching the function name to the name of
the reset being performed. Opinions?

Re: [PATCH v3 4/5] s390x: Move clear reset

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1601 bytes --]

On 11/25/19 2:37 PM, Cornelia Huck wrote:
> On Mon, 25 Nov 2019 04:03:47 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Let's also move the clear reset function into the reset handler.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/cpu-qom.h |  1 +
>>  target/s390x/cpu.c     | 58 +++++++++++++-----------------------------
>>  2 files changed, 18 insertions(+), 41 deletions(-)
>>
> 
>> @@ -453,6 +424,11 @@ static Property s390x_cpu_properties[] = {
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>>  
>> +static void s390_cpu_reset_clear(CPUState *s)
>> +{
>> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
>> +}
>> +
>>  static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>  {
>>      S390CPUClass *scc = S390_CPU_CLASS(oc);
>> @@ -469,7 +445,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>      scc->load_normal = s390_cpu_load_normal;
>>  #endif
>>      scc->reset = s390_cpu_reset;
>> -    cc->reset = s390_cpu_full_reset;
>> +    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
> 
> One thing I liked about the previous naming is that it is more obvious
> that the clear reset is actually the full reset of a cpu. Not sure if
> keeping that is better than matching the function name to the name of
> the reset being performed. Opinions?
> 

Are you only worrying for this particular wrapper or in general?
I'd be happy to rename the wrapper to s390_cpu_reset_full()


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 4/5] s390x: Move clear reset

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

On Mon, 25 Nov 2019 14:49:54 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/25/19 2:37 PM, Cornelia Huck wrote:
> > On Mon, 25 Nov 2019 04:03:47 -0500
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Let's also move the clear reset function into the reset handler.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  target/s390x/cpu-qom.h |  1 +
> >>  target/s390x/cpu.c     | 58 +++++++++++++-----------------------------
> >>  2 files changed, 18 insertions(+), 41 deletions(-)
> >>  
> >   
> >> @@ -453,6 +424,11 @@ static Property s390x_cpu_properties[] = {
> >>      DEFINE_PROP_END_OF_LIST()
> >>  };
> >>  
> >> +static void s390_cpu_reset_clear(CPUState *s)
> >> +{
> >> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
> >> +}
> >> +
> >>  static void s390_cpu_class_init(ObjectClass *oc, void *data)
> >>  {
> >>      S390CPUClass *scc = S390_CPU_CLASS(oc);
> >> @@ -469,7 +445,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
> >>      scc->load_normal = s390_cpu_load_normal;
> >>  #endif
> >>      scc->reset = s390_cpu_reset;
> >> -    cc->reset = s390_cpu_full_reset;
> >> +    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  
> > 
> > One thing I liked about the previous naming is that it is more obvious
> > that the clear reset is actually the full reset of a cpu. Not sure if
> > keeping that is better than matching the function name to the name of
> > the reset being performed. Opinions?
> >   
> 
> Are you only worrying for this particular wrapper or in general?
> I'd be happy to rename the wrapper to s390_cpu_reset_full()

Yes, I was thinking about this wrapper only, but don't feel too
strongly.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 5/5] s390x: Beautify diag308 handling

[Janosch Frank]

Let's improve readability by:
* Using constants for the subcodes
* Moving parameter checking into a function
* Removing subcode > 6 check as the default case catches that

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/diag.c | 54 +++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index 53c2f81f2a..b5aec06d6b 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -53,6 +53,29 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
 #define DIAG_308_RC_NO_CONF         0x0102
 #define DIAG_308_RC_INVALID         0x0402
 
+#define DIAG308_RESET_MOD_CLR       0
+#define DIAG308_RESET_LOAD_NORM     1
+#define DIAG308_LOAD_CLEAR          3
+#define DIAG308_LOAD_NORMAL_DUMP    4
+#define DIAG308_SET                 5
+#define DIAG308_STORE               6
+
+static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
+                              uintptr_t ra, bool write)
+{
+    if ((r1 & 1) || (addr & ~TARGET_PAGE_MASK)) {
+        s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+        return -1;
+    }
+    if (!address_space_access_valid(&address_space_memory, addr,
+                                    sizeof(IplParameterBlock), write,
+                                    MEMTXATTRS_UNSPECIFIED)) {
+        s390_program_interrupt(env, PGM_ADDRESSING, ra);
+        return -1;
+    }
+    return 0;
+}
+
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 {
     CPUState *cs = env_cpu(env);
@@ -65,30 +88,24 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
         return;
     }
 
-    if ((subcode & ~0x0ffffULL) || (subcode > 6)) {
+    if (subcode & ~0x0ffffULL) {
         s390_program_interrupt(env, PGM_SPECIFICATION, ra);
         return;
     }
 
     switch (subcode) {
-    case 0:
+    case DIAG308_RESET_MOD_CLR:
         s390_ipl_reset_request(cs, S390_RESET_MODIFIED_CLEAR);
         break;
-    case 1:
+    case DIAG308_RESET_LOAD_NORM:
         s390_ipl_reset_request(cs, S390_RESET_LOAD_NORMAL);
         break;
-    case 3:
+    case DIAG308_LOAD_CLEAR:
+        /* Well we still lack the clearing bit... */
         s390_ipl_reset_request(cs, S390_RESET_REIPL);
         break;
-    case 5:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), false,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ra);
+    case DIAG308_SET:
+        if (diag308_parm_check(env, r1, addr, ra, false)) {
             return;
         }
         iplb = g_new0(IplParameterBlock, 1);
@@ -110,15 +127,8 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
 out:
         g_free(iplb);
         return;
-    case 6:
-        if ((r1 & 1) || (addr & 0x0fffULL)) {
-            s390_program_interrupt(env, PGM_SPECIFICATION, ra);
-            return;
-        }
-        if (!address_space_access_valid(&address_space_memory, addr,
-                                        sizeof(IplParameterBlock), true,
-                                        MEMTXATTRS_UNSPECIFIED)) {
-            s390_program_interrupt(env, PGM_ADDRESSING, ra);
+    case DIAG308_STORE:
+        if (diag308_parm_check(env, r1, addr, ra, true)) {
             return;
         }
         iplb = s390_ipl_get_iplb();
-- 
2.20.1