[PATCH v2 0/5] s390x: Reset cleanup

[PATCH v2 0/5] s390x: Reset cleanup

[Janosch Frank]

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

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         | 99 ++++++++++++--------------------------
 target/s390x/cpu.h         |  4 +-
 target/s390x/diag.c        | 54 ++++++++++++---------
 target/s390x/sigp.c        |  4 +-
 6 files changed, 76 insertions(+), 97 deletions(-)

-- 
2.20.1

[PATCH v2 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 wron 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

[PATCH v2 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>
---
 target/s390x/cpu-qom.h |  6 +++++-
 target/s390x/cpu.c     | 17 +++++++++++------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 18 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..cf13472472 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -82,18 +82,23 @@ 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;
+        break;
+    }
 }
 
 /* S390CPUClass::initial_reset() */
@@ -102,7 +107,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 +478,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

[PATCH v2 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>
---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 44 +++++++++++++++---------------------------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 19 insertions(+), 31 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 cf13472472..1f423fb676 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -94,37 +94,26 @@ 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);
     case S390_CPU_RESET_NORMAL:
         env->pfault_token = -1UL;
         env->bpbc = false;
-        break;
     }
-}
-
-/* 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()) {
@@ -479,7 +468,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 v2 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     | 50 ++++++++----------------------------------
 2 files changed, 10 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 1f423fb676..017181fe4a 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));
+        /* Fallthrough */
     case S390_CPU_RESET_INITIAL:
         /* initial reset does not clear everything! */
         memset(&env->start_initial_reset_fields, 0,
@@ -121,46 +124,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)
 {
@@ -452,6 +415,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);
@@ -468,7 +436,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

[PATCH v2 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

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

[David Hildenbrand]

On 22.11.19 14:59, Janosch Frank 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>
> ---
>  target/s390x/cpu-qom.h |  6 +++++-
>  target/s390x/cpu.c     | 17 +++++++++++------
>  target/s390x/cpu.h     |  2 +-
>  target/s390x/sigp.c    |  2 +-
>  4 files changed, 18 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..cf13472472 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,18 +82,23 @@ 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;
> +        break;
> +    }
>  }
>  
>  /* S390CPUClass::initial_reset() */
> @@ -102,7 +107,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 +478,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;
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [PATCH v2 3/5] s390x: Move initial reset

[David Hildenbrand]

On 22.11.19 15:00, Janosch Frank wrote:
> Let's move the intial reset into the reset handler and cleanup
> afterwards.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.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, 19 insertions(+), 31 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 cf13472472..1f423fb676 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -94,37 +94,26 @@ 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;
> -        break;
>      }

Removing the break here seems strange. I guess this belongs to another
patch.


Apart from that

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 22.11.19 15:00, Janosch Frank 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     | 50 ++++++++----------------------------------
>  2 files changed, 10 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 1f423fb676..017181fe4a 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));

I think the preferred term in QEMU is "fall through".

> +        /* Fallthrough */
>      case S390_CPU_RESET_INITIAL:
>          /* initial reset does not clear everything! */
>          memset(&env->start_initial_reset_fields, 0,
> @@ -121,46 +124,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

Huh, what happened to that change?

Note that we now also do "env->bpbc = false" - is that ok?

-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Fri, 22 Nov 2019 08:59:58 -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 wron per-se,

s/wron per-se/wrong per se/

> the Ultravisor will only allow the correct reset to be performed.

So... the uv has stricter rules than the architecture has in that
respect?

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

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

[Janosch Frank]

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

On 11/22/19 3:30 PM, David Hildenbrand wrote:
> On 22.11.19 15:00, Janosch Frank 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     | 50 ++++++++----------------------------------
>>  2 files changed, 10 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 1f423fb676..017181fe4a 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));
> 
> I think the preferred term in QEMU is "fall through".
> 
>> +        /* Fallthrough */
>>      case S390_CPU_RESET_INITIAL:
>>          /* initial reset does not clear everything! */
>>          memset(&env->start_initial_reset_fields, 0,
>> @@ -121,46 +124,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
> 
> Huh, what happened to that change?

Seems like I missed it

> 
> Note that we now also do "env->bpbc = false" - is that ok?

That's ok, clear and initial reset do a memset to bpbc, but as reset
normal doesn't we need to set it explicitly.



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

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

[Janosch Frank]

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

On 11/22/19 5:17 PM, Cornelia Huck wrote:
> On Fri, 22 Nov 2019 08:59:58 -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 wron per-se,
> 
> s/wron per-se/wrong per se/

Ups

> 
>> the Ultravisor will only allow the correct reset to be performed.
> 
> So... the uv has stricter rules than the architecture has in that
> respect?

Yeah, the architecture only cares about the state that the cpu will be
in after the reset. So we can do as many changes to vcpu run as we like,
if at the end we are in the reset state we intended to be in.

The UV guards all resets including the sigp initiated ones.

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



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

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

[Janosch Frank]

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

On 11/22/19 3:30 PM, David Hildenbrand wrote:
> On 22.11.19 15:00, Janosch Frank 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     | 50 ++++++++----------------------------------
>>  2 files changed, 10 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 1f423fb676..017181fe4a 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));
> 
> I think the preferred term in QEMU is "fall through".
> 
>> +        /* Fallthrough */
>>      case S390_CPU_RESET_INITIAL:
>>          /* initial reset does not clear everything! */
>>          memset(&env->start_initial_reset_fields, 0,
>> @@ -121,46 +124,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
> 
> Huh, what happened to that change?

Btw., wouldn't we need that for both initial and clear reset?

> 
> Note that we now also do "env->bpbc = false" - is that ok?
> 



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

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

[David Hildenbrand]

On 22.11.19 18:15, Janosch Frank wrote:
> On 11/22/19 3:30 PM, David Hildenbrand wrote:
>> On 22.11.19 15:00, Janosch Frank 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     | 50 ++++++++----------------------------------
>>>  2 files changed, 10 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 1f423fb676..017181fe4a 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));
>>
>> I think the preferred term in QEMU is "fall through".
>>
>>> +        /* Fallthrough */
>>>      case S390_CPU_RESET_INITIAL:
>>>          /* initial reset does not clear everything! */
>>>          memset(&env->start_initial_reset_fields, 0,
>>> @@ -121,46 +124,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
>>
>> Huh, what happened to that change?
> 
> Btw., wouldn't we need that for both initial and clear reset?

user-only only does a cpu reset when starting up to initialize the cpu.
no other resets will be triggered.
-- 

Thanks,

David / dhildenb