[PATCH v4 0/6] s390x: Cleanup

[PATCH v4 0/6] s390x: Cleanup

[Janosch Frank]

Adding comments and reordering code for better readability in the
diag308 and machine reset functions. Also making the kvm sclp function
void and refactoring it.

v4
	* Added sclp cleanup
	* Renamed ccpu reset wrapper
	* Added assert to cpu reset function

Janosch Frank (6):
  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
  s390x: kvm: Make kvm_sclp_service_call void

 hw/s390x/s390-virtio-ccw.c |   3 ++
 target/s390x/cpu-qom.h     |   9 +++-
 target/s390x/cpu.c         | 107 ++++++++++++++-----------------------
 target/s390x/cpu.h         |   4 +-
 target/s390x/diag.c        |  54 +++++++++++--------
 target/s390x/kvm.c         |  12 ++---
 target/s390x/sigp.c        |   4 +-
 7 files changed, 91 insertions(+), 102 deletions(-)

-- 
2.20.1

[PATCH v4 1/6] 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>
Reviewed-by: Cornelia Huck <cohuck@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 v4 2/6] 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     | 19 +++++++++++++------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 20 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..67d6fbfa44 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -82,18 +82,25 @@ 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;
+    default:
+        g_assert_not_reached();
+    }
 }
 
 /* S390CPUClass::initial_reset() */
@@ -102,7 +109,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 +480,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 v4 2/6] s390x: Move reset normal to shared reset handler

[Thomas Huth]

On 27/11/2019 18.50, 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.

Nit: I'd add a "later" in above sentence, since you don't use 
fallthroughs yet.

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

Hmm, which leftovers? I can mainly see the renaming here...

> 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     | 19 +++++++++++++------
>   target/s390x/cpu.h     |  2 +-
>   target/s390x/sigp.c    |  2 +-
>   4 files changed, 20 insertions(+), 9 deletions(-)

Anyway,
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 2/6] s390x: Move reset normal to shared reset handler

[Cornelia Huck]

On Thu, 28 Nov 2019 07:32:53 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 27/11/2019 18.50, 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.  
> 
> Nit: I'd add a "later" in above sentence, since you don't use 
> fallthroughs yet.

If nobody objects, I can apply this with the sentence changed to "so we
can later use...".

> 
> > This patch introduces the reset function by renaming cpu_reset() and
> > cleaning up leftovers.  
> 
> Hmm, which leftovers? I can mainly see the renaming here...

That's probably a leftover from before splitting the original patch in
three... I can drop the leftover part when applying :)

> 
> > 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     | 19 +++++++++++++------
> >   target/s390x/cpu.h     |  2 +-
> >   target/s390x/sigp.c    |  2 +-
> >   4 files changed, 20 insertions(+), 9 deletions(-)  
> 
> Anyway,
> Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v4 3/6] 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 67d6fbfa44..55e2d1fe7b 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -94,6 +94,23 @@ 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;
@@ -101,32 +118,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     default:
         g_assert_not_reached();
     }
-}
-
-/* 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()) {
@@ -481,7 +472,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

Re: [PATCH v4 3/6] s390x: Move initial reset

[Thomas Huth]

On 27/11/2019 18.50, 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>
> 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 67d6fbfa44..55e2d1fe7b 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -94,6 +94,23 @@ 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;
> @@ -101,32 +118,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>       default:
>           g_assert_not_reached();
>       }
> -}
> -
> -/* 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()) {

You're doing the if (kvm_enabled()) now also for S390_CPU_RESET_NORMAL 
... is that OK? It's doing an KVM_S390_INITIAL_RESET ioctl(), so that 
sounds suspicious to me. Don't you have to add a check for type != 
S390_CPU_RESET_NORMAL here?

  Thomas

Re: [PATCH v4 3/6] s390x: Move initial reset

[Janosch Frank]

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

On 11/28/19 8:00 AM, Thomas Huth wrote:
> On 27/11/2019 18.50, 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>
>> 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 67d6fbfa44..55e2d1fe7b 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -94,6 +94,23 @@ 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;
>> @@ -101,32 +118,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>       default:
>>           g_assert_not_reached();
>>       }
>> -}
>> -
>> -/* 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()) {
> 
> You're doing the if (kvm_enabled()) now also for S390_CPU_RESET_NORMAL 
> ... is that OK? It's doing an KVM_S390_INITIAL_RESET ioctl(), so that 
> sounds suspicious to me. Don't you have to add a check for type != 
> S390_CPU_RESET_NORMAL here?
> 
>   Thomas

Yes, I need to fence the NORMAL case again.

> 
> 



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

[PATCH v5] 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>
---

Fixed the kvm vcpu reset.

---
 target/s390x/cpu-qom.h |  2 +-
 target/s390x/cpu.c     | 46 +++++++++++++++++-------------------------
 target/s390x/cpu.h     |  2 +-
 target/s390x/sigp.c    |  2 +-
 4 files changed, 21 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 67d6fbfa44..ca62fe7685 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -94,6 +94,23 @@ 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;
@@ -101,35 +118,9 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     default:
         g_assert_not_reached();
     }
-}
-
-/* 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()) {
+    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
         kvm_s390_reset_vcpu(cpu);
     }
 }
@@ -481,7 +472,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

Re: [PATCH v5] s390x: Move initial reset

[Thomas Huth]

On 28/11/2019 09.37, 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>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
> 
> Fixed the kvm vcpu reset.
> 
> ---
>   target/s390x/cpu-qom.h |  2 +-
>   target/s390x/cpu.c     | 46 +++++++++++++++++-------------------------
>   target/s390x/cpu.h     |  2 +-
>   target/s390x/sigp.c    |  2 +-
>   4 files changed, 21 insertions(+), 31 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v4 4/6] 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 55e2d1fe7b..3e745e8ecc 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);
@@ -125,46 +136,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)
 {
@@ -456,6 +427,11 @@ static Property s390x_cpu_properties[] = {
     DEFINE_PROP_END_OF_LIST()
 };
 
+static void s390_cpu_reset_full(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);
@@ -472,7 +448,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_full;
     cc->class_by_name = s390_cpu_class_by_name,
     cc->has_work = s390_cpu_has_work;
 #ifdef CONFIG_TCG
-- 
2.20.1

Re: [PATCH v4 4/6] s390x: Move clear reset

[David Hildenbrand]

On 27.11.19 18:50, Janosch Frank wrote:
> Let's also move the clear reset function into the reset handler.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]

Gave the first 4 patches a tcg test. Looks good.

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v4 4/6] s390x: Move clear reset

[Janosch Frank]

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

On 11/27/19 7:15 PM, David Hildenbrand wrote:
> On 27.11.19 18:50, Janosch Frank wrote:
>> Let's also move the clear reset function into the reset handler.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
> [...]
> 
> Gave the first 4 patches a tcg test. Looks good.

Great to hear :)

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

Thanks



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

Re: [PATCH v4 4/6] s390x: Move clear reset

[Thomas Huth]

On 27/11/2019 18.50, 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     | 58 +++++++++++++-----------------------------
>   2 files changed, 18 insertions(+), 41 deletions(-)

I really wonder why we had so much duplicated code here before and 
nobody dared to clean it up in all those years... Thanks for doing that now!

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 4/6] s390x: Move clear reset

[Janosch Frank]

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

On 11/28/19 8:09 AM, Thomas Huth wrote:
> On 27/11/2019 18.50, 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     | 58 +++++++++++++-----------------------------
>>   2 files changed, 18 insertions(+), 41 deletions(-)
> 
> I really wonder why we had so much duplicated code here before and 
> nobody dared to clean it up in all those years... Thanks for doing that now!

Looking back at my old patch series, I most often start with a cleanup
before I change or add anything...

I'd guess that QEMU hasn't seen some love for a longer time.

> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!



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

Re: [PATCH v4 4/6] s390x: Move clear reset

[David Hildenbrand]

On 28.11.19 09:35, Janosch Frank wrote:
> On 11/28/19 8:09 AM, Thomas Huth wrote:
>> On 27/11/2019 18.50, 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     | 58 +++++++++++++-----------------------------
>>>   2 files changed, 18 insertions(+), 41 deletions(-)
>>
>> I really wonder why we had so much duplicated code here before and 
>> nobody dared to clean it up in all those years... Thanks for doing that now!
> 
> Looking back at my old patch series, I most often start with a cleanup
> before I change or add anything...
> 
> I'd guess that QEMU hasn't seen some love for a longer time.

Oh, I give QEMU some love quite often ... I just can't love every part
of it equally well ;)

I'd like to second the "thanks" for doing this cleanup :)

-- 
Thanks,

David / dhildenb

[PATCH v4 5/6] 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

[PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Janosch Frank]

It defaults to returning 0 anyway and that return value is not
necessary, as 0 is also the default rc that the caller would return.

While doing that we can simplify the logic a bit and return early if
we inject a PGM exception. Also we always set a 0 cc, so let's not
base it on the rc of the sclp emulation functions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/kvm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 0c9d14b4b1..08bb1edca0 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
     kvm_s390_vcpu_interrupt(cpu, &irq);
 }
 
-static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
+static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
                                  uint16_t ipbh0)
 {
     CPUS390XState *env = &cpu->env;
     uint64_t sccb;
     uint32_t code;
-    int r = 0;
+    int r;
 
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
@@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     r = sclp_service_call(env, sccb, code);
     if (r < 0) {
         kvm_s390_program_interrupt(cpu, -r);
-    } else {
-        setcc(cpu, r);
+        return;
     }
-
-    return 0;
+    setcc(cpu, 0);
 }
 
 static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
@@ -1240,7 +1238,7 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
         setcc(cpu, 3);
         break;
     case PRIV_B2_SCLP_CALL:
-        rc = kvm_sclp_service_call(cpu, run, ipbh0);
+        kvm_sclp_service_call(cpu, run, ipbh0);
         break;
     default:
         rc = -1;
-- 
2.20.1

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[David Hildenbrand]

On 27.11.19 18:50, Janosch Frank wrote:
> It defaults to returning 0 anyway and that return value is not
> necessary, as 0 is also the default rc that the caller would return.
> 
> While doing that we can simplify the logic a bit and return early if
> we inject a PGM exception. Also we always set a 0 cc, so let's not
> base it on the rc of the sclp emulation functions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/kvm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0c9d14b4b1..08bb1edca0 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
>      kvm_s390_vcpu_interrupt(cpu, &irq);
>  }
>  
> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>                                   uint16_t ipbh0)
>  {
>      CPUS390XState *env = &cpu->env;
>      uint64_t sccb;
>      uint32_t code;
> -    int r = 0;
> +    int r;
>  
>      sccb = env->regs[ipbh0 & 0xf];
>      code = env->regs[(ipbh0 & 0xf0) >> 4];
> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>      r = sclp_service_call(env, sccb, code);
>      if (r < 0) {
>          kvm_s390_program_interrupt(cpu, -r);
> -    } else {
> -        setcc(cpu, r);
> +        return;
>      }
> -
> -    return 0;
> +    setcc(cpu, 0);

For now, sclp_service_call will return <= 0 ... but don't we actually
have the option to return a cc? What does the spec say? Always set to 0?

At least also the TCG implementation sets the CC to whatever is returned
here .... and Claudio's unit tests have code to handle cc != 0 ... and
the kernel as well (drivers/s390/char/sclp.h:sclp_service_call())



-- 
Thanks,

David / dhildenb

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[David Hildenbrand]

On 27.11.19 19:07, David Hildenbrand wrote:
> On 27.11.19 18:50, Janosch Frank wrote:
>> It defaults to returning 0 anyway and that return value is not
>> necessary, as 0 is also the default rc that the caller would return.
>>
>> While doing that we can simplify the logic a bit and return early if
>> we inject a PGM exception. Also we always set a 0 cc, so let's not
>> base it on the rc of the sclp emulation functions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/kvm.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 0c9d14b4b1..08bb1edca0 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
>>      kvm_s390_vcpu_interrupt(cpu, &irq);
>>  }
>>  
>> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>>                                   uint16_t ipbh0)
>>  {
>>      CPUS390XState *env = &cpu->env;
>>      uint64_t sccb;
>>      uint32_t code;
>> -    int r = 0;
>> +    int r;
>>  
>>      sccb = env->regs[ipbh0 & 0xf];
>>      code = env->regs[(ipbh0 & 0xf0) >> 4];
>> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>>      r = sclp_service_call(env, sccb, code);
>>      if (r < 0) {
>>          kvm_s390_program_interrupt(cpu, -r);
>> -    } else {
>> -        setcc(cpu, r);
>> +        return;
>>      }
>> -
>> -    return 0;
>> +    setcc(cpu, 0);
> 
> For now, sclp_service_call will return <= 0 ... but don't we actually
> have the option to return a cc? What does the spec say? Always set to 0?
> 
> At least also the TCG implementation sets the CC to whatever is returned
> here .... and Claudio's unit tests have code to handle cc != 0 ... and
> the kernel as well (drivers/s390/char/sclp.h:sclp_service_call())
> 

FWIW, I'd keep this as

setcc(cpu, r);

if we ever have to set a cc != 0.

-- 
Thanks,

David / dhildenb

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Janosch Frank]

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

On 11/27/19 7:07 PM, David Hildenbrand wrote:
> On 27.11.19 18:50, Janosch Frank wrote:
>> It defaults to returning 0 anyway and that return value is not
>> necessary, as 0 is also the default rc that the caller would return.
>>
>> While doing that we can simplify the logic a bit and return early if
>> we inject a PGM exception. Also we always set a 0 cc, so let's not
>> base it on the rc of the sclp emulation functions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/kvm.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 0c9d14b4b1..08bb1edca0 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
>>      kvm_s390_vcpu_interrupt(cpu, &irq);
>>  }
>>  
>> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>>                                   uint16_t ipbh0)
>>  {
>>      CPUS390XState *env = &cpu->env;
>>      uint64_t sccb;
>>      uint32_t code;
>> -    int r = 0;
>> +    int r;
>>  
>>      sccb = env->regs[ipbh0 & 0xf];
>>      code = env->regs[(ipbh0 & 0xf0) >> 4];
>> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>>      r = sclp_service_call(env, sccb, code);
>>      if (r < 0) {
>>          kvm_s390_program_interrupt(cpu, -r);
>> -    } else {
>> -        setcc(cpu, r);
>> +        return;
>>      }
>> -
>> -    return 0;
>> +    setcc(cpu, 0);
> 
> For now, sclp_service_call will return <= 0 ... but don't we actually
> have the option to return a cc? What does the spec say? Always set to 0?
> 
> At least also the TCG implementation sets the CC to whatever is returned
> here .... and Claudio's unit tests have code to handle cc != 0 ... and
> the kernel as well (drivers/s390/char/sclp.h:sclp_service_call())


There's 0 (initiated), busy and operational and as far as I know we
implement neither.
sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to
check when we're after the pgm injection code.



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

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Janosch Frank]

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

On 11/27/19 7:25 PM, Janosch Frank wrote:
> 
> There's 0 (initiated), busy and operational and as far as I know we
> implement neither.

That came out wrong...
s/operational/not operational/

We only implement "command initiated" / cc = 0
We can never have busy, because we handle sclp calls synchronously.
The spec does not give any indication when we could return "not
operational". I guess that's just a free pass for hypervisors.

> sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to
> check when we're after the pgm injection code.



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

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Cornelia Huck]

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

On Wed, 27 Nov 2019 19:38:06 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/27/19 7:25 PM, Janosch Frank wrote:
> > 
> > There's 0 (initiated), busy and operational and as far as I know we
> > implement neither.  
> 
> That came out wrong...
> s/operational/not operational/
> 
> We only implement "command initiated" / cc = 0
> We can never have busy, because we handle sclp calls synchronously.
> The spec does not give any indication when we could return "not
> operational". I guess that's just a free pass for hypervisors.

Regardless, setcc(cpu, r) also feels a bit cleaner to me...

> 
> > sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to
> > check when we're after the pgm injection code.  
> 
> 


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

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Janosch Frank]

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

On 11/28/19 6:34 PM, Cornelia Huck wrote:
> On Wed, 27 Nov 2019 19:38:06 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 11/27/19 7:25 PM, Janosch Frank wrote:
>>>
>>> There's 0 (initiated), busy and operational and as far as I know we
>>> implement neither.  
>>
>> That came out wrong...
>> s/operational/not operational/
>>
>> We only implement "command initiated" / cc = 0
>> We can never have busy, because we handle sclp calls synchronously.
>> The spec does not give any indication when we could return "not
>> operational". I guess that's just a free pass for hypervisors.
> 
> Regardless, setcc(cpu, r) also feels a bit cleaner to me...

Ok, do you want to change that while picking the patches up or shall I
send a new version?

> 
>>
>>> sclp_service_call() returns either 0 or -PGM_CODE, so we don't need to
>>> check when we're after the pgm injection code.  
>>
>>
> 



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

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Cornelia Huck]

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

On Fri, 29 Nov 2019 09:16:21 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 11/28/19 6:34 PM, Cornelia Huck wrote:
> > On Wed, 27 Nov 2019 19:38:06 +0100
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> On 11/27/19 7:25 PM, Janosch Frank wrote:  
> >>>
> >>> There's 0 (initiated), busy and operational and as far as I know we
> >>> implement neither.    
> >>
> >> That came out wrong...
> >> s/operational/not operational/
> >>
> >> We only implement "command initiated" / cc = 0
> >> We can never have busy, because we handle sclp calls synchronously.
> >> The spec does not give any indication when we could return "not
> >> operational". I guess that's just a free pass for hypervisors.  
> > 
> > Regardless, setcc(cpu, r) also feels a bit cleaner to me...  
> 
> Ok, do you want to change that while picking the patches up or shall I
> send a new version?

New version (of this patch), please :) Easier for me...

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

[PATCH v5] s390x: kvm: Make kvm_sclp_service_call void

[Janosch Frank]

It defaults to returning 0 anyway and that return value is not
necessary, as 0 is also the default rc that the caller would return.

While doing that we can simplify the logic a bit and return early if
we inject a PGM exception.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/kvm.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 0c9d14b4b1..ad6e38c876 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
     kvm_s390_vcpu_interrupt(cpu, &irq);
 }
 
-static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
+static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
                                  uint16_t ipbh0)
 {
     CPUS390XState *env = &cpu->env;
     uint64_t sccb;
     uint32_t code;
-    int r = 0;
+    int r;
 
     sccb = env->regs[ipbh0 & 0xf];
     code = env->regs[(ipbh0 & 0xf0) >> 4];
@@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
     r = sclp_service_call(env, sccb, code);
     if (r < 0) {
         kvm_s390_program_interrupt(cpu, -r);
-    } else {
-        setcc(cpu, r);
+        return;
     }
-
-    return 0;
+    setcc(cpu, r);
 }
 
 static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
@@ -1240,7 +1238,7 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
         setcc(cpu, 3);
         break;
     case PRIV_B2_SCLP_CALL:
-        rc = kvm_sclp_service_call(cpu, run, ipbh0);
+        kvm_sclp_service_call(cpu, run, ipbh0);
         break;
     default:
         rc = -1;
-- 
2.20.1

Re: [PATCH v5] s390x: kvm: Make kvm_sclp_service_call void

[David Hildenbrand]

On 29.11.19 10:17, Janosch Frank wrote:
> It defaults to returning 0 anyway and that return value is not
> necessary, as 0 is also the default rc that the caller would return.
> 
> While doing that we can simplify the logic a bit and return early if
> we inject a PGM exception.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/kvm.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0c9d14b4b1..ad6e38c876 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
>      kvm_s390_vcpu_interrupt(cpu, &irq);
>  }
>  
> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>                                   uint16_t ipbh0)
>  {
>      CPUS390XState *env = &cpu->env;
>      uint64_t sccb;
>      uint32_t code;
> -    int r = 0;
> +    int r;
>  
>      sccb = env->regs[ipbh0 & 0xf];
>      code = env->regs[(ipbh0 & 0xf0) >> 4];
> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>      r = sclp_service_call(env, sccb, code);
>      if (r < 0) {
>          kvm_s390_program_interrupt(cpu, -r);
> -    } else {
> -        setcc(cpu, r);
> +        return;
>      }
> -
> -    return 0;
> +    setcc(cpu, r);
>  }
>  
>  static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
> @@ -1240,7 +1238,7 @@ static int handle_b2(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
>          setcc(cpu, 3);
>          break;
>      case PRIV_B2_SCLP_CALL:
> -        rc = kvm_sclp_service_call(cpu, run, ipbh0);
> +        kvm_sclp_service_call(cpu, run, ipbh0);
>          break;
>      default:
>          rc = -1;
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v4 6/6] s390x: kvm: Make kvm_sclp_service_call void

[Thomas Huth]

On 27/11/2019 18.50, Janosch Frank wrote:
> It defaults to returning 0 anyway and that return value is not
> necessary, as 0 is also the default rc that the caller would return.
> 
> While doing that we can simplify the logic a bit and return early if
> we inject a PGM exception. Also we always set a 0 cc, so let's not
> base it on the rc of the sclp emulation functions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/kvm.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0c9d14b4b1..08bb1edca0 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1159,13 +1159,13 @@ void kvm_s390_access_exception(S390CPU *cpu, uint16_t code, uint64_t te_code)
>       kvm_s390_vcpu_interrupt(cpu, &irq);
>   }
>   
> -static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
> +static void kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>                                    uint16_t ipbh0)
>   {
>       CPUS390XState *env = &cpu->env;
>       uint64_t sccb;
>       uint32_t code;
> -    int r = 0;
> +    int r;
>   
>       sccb = env->regs[ipbh0 & 0xf];
>       code = env->regs[(ipbh0 & 0xf0) >> 4];
> @@ -1173,11 +1173,9 @@ static int kvm_sclp_service_call(S390CPU *cpu, struct kvm_run *run,
>       r = sclp_service_call(env, sccb, code);
>       if (r < 0) {
>           kvm_s390_program_interrupt(cpu, -r);
> -    } else {
> -        setcc(cpu, r);
> +        return;
>       }
> -
> -    return 0;
> +    setcc(cpu, 0);

I think I'd also slightly prefer setcc(cpu, r) here ... but anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 0/6] s390x: Cleanup

[Cornelia Huck]

On Wed, 27 Nov 2019 12:50:40 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Adding comments and reordering code for better readability in the
> diag308 and machine reset functions. Also making the kvm sclp function
> void and refactoring it.
> 
> v4
> 	* Added sclp cleanup
> 	* Renamed ccpu reset wrapper
> 	* Added assert to cpu reset function
> 
> Janosch Frank (6):
>   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
>   s390x: kvm: Make kvm_sclp_service_call void
> 
>  hw/s390x/s390-virtio-ccw.c |   3 ++
>  target/s390x/cpu-qom.h     |   9 +++-
>  target/s390x/cpu.c         | 107 ++++++++++++++-----------------------
>  target/s390x/cpu.h         |   4 +-
>  target/s390x/diag.c        |  54 +++++++++++--------
>  target/s390x/kvm.c         |  12 ++---
>  target/s390x/sigp.c        |   4 +-
>  7 files changed, 91 insertions(+), 102 deletions(-)
> 

Thanks, applied.