[PATCH 0/4] s390x: Reset cleanup

[PATCH 0/4] s390x: Reset cleanup

[Janosch Frank]

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

Janosch Frank (4):
  s390x: Don't do a normal reset on the initial cpu
  s390x: Cleanup cpu resets
  s390x: Beautify diag308 handling
  s390x: Beautify machine reset

 hw/s390x/s390-virtio-ccw.c |  23 ++++----
 target/s390x/cpu.c         | 110 ++++++++++++++++---------------------
 target/s390x/diag.c        |  54 ++++++++++--------
 3 files changed, 93 insertions(+), 94 deletions(-)

-- 
2.20.1

[PATCH 1/4] 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>
---
 hw/s390x/s390-virtio-ccw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef0ad..c1d1440272 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -348,6 +348,9 @@ static void s390_machine_reset(MachineState *machine)
         break;
     case S390_RESET_LOAD_NORMAL:
         CPU_FOREACH(t) {
+            if (t == cs) {
+                continue;
+            }
             run_on_cpu(t, s390_do_cpu_reset, RUN_ON_CPU_NULL);
         }
         subsystem_reset();
-- 
2.20.1

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

[David Hildenbrand]

On 22.11.19 08:52, Janosch Frank 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,
> the Ultravisor will only allow the correct reset to be performed.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.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();
> 

Right, AFAIKS, s390_cpu_initial_reset() does a s390_cpu_reset() right 
no, so nothing should change.

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

-- 

Thanks,

David / dhildenb

[PATCH 2/4] s390x: Cleanup cpu resets

[Janosch Frank]

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

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c | 110 ++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 62 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 3abe7e80fd..556afecbc1 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -82,67 +82,52 @@ static void s390_cpu_load_normal(CPUState *s)
 }
 #endif
 
-/* S390CPUClass::cpu_reset() */
-static void s390_cpu_reset(CPUState *s)
+typedef enum cpu_reset_type {
+    S390_CPU_RESET_NORMAL,
+    S390_CPU_RESET_INITIAL,
+    S390_CPU_RESET_CLEAR,
+} cpu_reset_type;
+
+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);
-}
 
-/* S390CPUClass::initial_reset() */
-static void s390_cpu_initial_reset(CPUState *s)
-{
-    S390CPU *cpu = S390_CPU(s);
-    CPUS390XState *env = &cpu->env;
+    /* Set initial values after clearing */
+    switch (type) {
+    case S390_CPU_RESET_CLEAR:
+        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
+        /* Fallthrough */
+    case S390_CPU_RESET_INITIAL:
+        memset(&env->start_initial_reset_fields, 0,
+               offsetof(CPUS390XState, end_reset_fields) -
+               offsetof(CPUS390XState, start_initial_reset_fields));
+        /* architectured initial values for CR 0 and 14 */
+        env->cregs[0] = CR0_RESET;
+        env->cregs[14] = CR14_RESET;
 
-    s390_cpu_reset(s);
-    /* initial reset does not clear everything! */
-    memset(&env->start_initial_reset_fields, 0,
-        offsetof(CPUS390XState, end_reset_fields) -
-        offsetof(CPUS390XState, start_initial_reset_fields));
-
-    /* architectured initial values for CR 0 and 14 */
-    env->cregs[0] = CR0_RESET;
-    env->cregs[14] = CR14_RESET;
-
-    /* architectured initial value for Breaking-Event-Address register */
-    env->gbea = 1;
-
-    env->pfault_token = -1UL;
-
-    /* tininess for underflow is detected before rounding */
-    set_float_detect_tininess(float_tininess_before_rounding,
-                              &env->fpu_status);
+        /* architectured initial value for Breaking-Event-Address register */
+        env->gbea = 1;
+        /* tininess for underflow is detected before rounding */
+        set_float_detect_tininess(float_tininess_before_rounding,
+                                  &env->fpu_status);
+        /* Fallthrough */
+    case S390_CPU_RESET_NORMAL:
+        env->pfault_token = -1UL;
+        env->bpbc = false;
+        break;
+    }
 
     /* Reset state inside the kernel that we cannot access yet from QEMU. */
-    if (kvm_enabled()) {
-        kvm_s390_reset_vcpu(cpu);
+    if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
+                          type == S390_CPU_RESET_INITIAL)) {
+            kvm_s390_reset_vcpu(cpu);
     }
-}
-
-/* CPUClass:reset() */
-static void s390_cpu_full_reset(CPUState *s)
-{
-    S390CPU *cpu = S390_CPU(s);
-    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
-    CPUS390XState *env = &cpu->env;
-
-    scc->parent_reset(s);
-    cpu->env.sigp_order = 0;
-    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
-
-    memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
-
-    /* architectured initial values for CR 0 and 14 */
-    env->cregs[0] = CR0_RESET;
-    env->cregs[14] = CR14_RESET;
 
 #if defined(CONFIG_USER_ONLY)
     /* user mode should always be allowed to use the full FPU */
@@ -151,20 +136,21 @@ static void s390_cpu_full_reset(CPUState *s)
         env->cregs[0] |= CR0_VECTOR;
     }
 #endif
+}
 
-    /* architectured initial value for Breaking-Event-Address register */
-    env->gbea = 1;
+static void s390_cpu_reset_normal(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
+}
 
-    env->pfault_token = -1UL;
+static void s390_cpu_reset_initial(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
+}
 
-    /* tininess for underflow is detected before rounding */
-    set_float_detect_tininess(float_tininess_before_rounding,
-                              &env->fpu_status);
-
-    /* Reset state inside the kernel that we cannot access yet from QEMU. */
-    if (kvm_enabled()) {
-        kvm_s390_reset_vcpu(cpu);
-    }
+static void s390_cpu_reset_clear(CPUState *s)
+{
+    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
 }
 
 #if !defined(CONFIG_USER_ONLY)
@@ -473,9 +459,9 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 #if !defined(CONFIG_USER_ONLY)
     scc->load_normal = s390_cpu_load_normal;
 #endif
-    scc->cpu_reset = s390_cpu_reset;
-    scc->initial_cpu_reset = s390_cpu_initial_reset;
-    cc->reset = s390_cpu_full_reset;
+    scc->cpu_reset = s390_cpu_reset_normal;
+    scc->initial_cpu_reset = s390_cpu_reset_initial;
+    cc->reset = s390_cpu_reset_clear;
     cc->class_by_name = s390_cpu_class_by_name,
     cc->has_work = s390_cpu_has_work;
 #ifdef CONFIG_TCG
-- 
2.20.1

Re: [PATCH 2/4] s390x: Cleanup cpu resets

[Philippe Mathieu-Daudé]

Hi Janosh,

On 11/22/19 8:52 AM, Janosch Frank wrote:
> Let's move the resets into one function and switch by type, so we can
> use fallthroughs for shared reset actions.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/cpu.c | 110 ++++++++++++++++++++-------------------------
>   1 file changed, 48 insertions(+), 62 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 3abe7e80fd..556afecbc1 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,67 +82,52 @@ static void s390_cpu_load_normal(CPUState *s)
>   }
>   #endif
>   
> -/* S390CPUClass::cpu_reset() */
> -static void s390_cpu_reset(CPUState *s)
> +typedef enum cpu_reset_type {
> +    S390_CPU_RESET_NORMAL,
> +    S390_CPU_RESET_INITIAL,
> +    S390_CPU_RESET_CLEAR,
> +} cpu_reset_type;
> +
> +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);
> -}
>   
> -/* S390CPUClass::initial_reset() */
> -static void s390_cpu_initial_reset(CPUState *s)
> -{
> -    S390CPU *cpu = S390_CPU(s);
> -    CPUS390XState *env = &cpu->env;
> +    /* Set initial values after clearing */
> +    switch (type) {
> +    case S390_CPU_RESET_CLEAR:
> +        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
> +        /* Fallthrough */
> +    case S390_CPU_RESET_INITIAL:
> +        memset(&env->start_initial_reset_fields, 0,
> +               offsetof(CPUS390XState, end_reset_fields) -
> +               offsetof(CPUS390XState, start_initial_reset_fields));
> +        /* architectured initial values for CR 0 and 14 */
> +        env->cregs[0] = CR0_RESET;
> +        env->cregs[14] = CR14_RESET;
>   
> -    s390_cpu_reset(s);
> -    /* initial reset does not clear everything! */
> -    memset(&env->start_initial_reset_fields, 0,
> -        offsetof(CPUS390XState, end_reset_fields) -
> -        offsetof(CPUS390XState, start_initial_reset_fields));
> -
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
> -
> -    /* architectured initial value for Breaking-Event-Address register */
> -    env->gbea = 1;
> -
> -    env->pfault_token = -1UL;
> -
> -    /* tininess for underflow is detected before rounding */
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -                              &env->fpu_status);
> +        /* architectured initial value for Breaking-Event-Address register */
> +        env->gbea = 1;
> +        /* tininess for underflow is detected before rounding */
> +        set_float_detect_tininess(float_tininess_before_rounding,
> +                                  &env->fpu_status);
> +        /* Fallthrough */
> +    case S390_CPU_RESET_NORMAL:
> +        env->pfault_token = -1UL;
> +        env->bpbc = false;
> +        break;
> +    }
>   
>       /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled()) {
> -        kvm_s390_reset_vcpu(cpu);
> +    if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
> +                          type == S390_CPU_RESET_INITIAL)) {
> +            kvm_s390_reset_vcpu(cpu);
>       }
> -}
> -
> -/* CPUClass:reset() */
> -static void s390_cpu_full_reset(CPUState *s)
> -{
> -    S390CPU *cpu = S390_CPU(s);
> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> -    CPUS390XState *env = &cpu->env;
> -
> -    scc->parent_reset(s);
> -    cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> -
> -    memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
> -
> -    /* architectured initial values for CR 0 and 14 */
> -    env->cregs[0] = CR0_RESET;
> -    env->cregs[14] = CR14_RESET;
>   
>   #if defined(CONFIG_USER_ONLY)
>       /* user mode should always be allowed to use the full FPU */
> @@ -151,20 +136,21 @@ static void s390_cpu_full_reset(CPUState *s)
>           env->cregs[0] |= CR0_VECTOR;
>       }
>   #endif
> +}
>   
> -    /* architectured initial value for Breaking-Event-Address register */
> -    env->gbea = 1;
> +static void s390_cpu_reset_normal(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
> +}
>   
> -    env->pfault_token = -1UL;
> +static void s390_cpu_reset_initial(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
> +}
>   
> -    /* tininess for underflow is detected before rounding */
> -    set_float_detect_tininess(float_tininess_before_rounding,
> -                              &env->fpu_status);
> -
> -    /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled()) {
> -        kvm_s390_reset_vcpu(cpu);
> -    }
> +static void s390_cpu_reset_clear(CPUState *s)
> +{
> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
>   }
>   
>   #if !defined(CONFIG_USER_ONLY)
> @@ -473,9 +459,9 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>   #if !defined(CONFIG_USER_ONLY)
>       scc->load_normal = s390_cpu_load_normal;
>   #endif
> -    scc->cpu_reset = s390_cpu_reset;
> -    scc->initial_cpu_reset = s390_cpu_initial_reset;
> -    cc->reset = s390_cpu_full_reset;
> +    scc->cpu_reset = s390_cpu_reset_normal;
> +    scc->initial_cpu_reset = s390_cpu_reset_initial;
> +    cc->reset = s390_cpu_reset_clear;
>       cc->class_by_name = s390_cpu_class_by_name,
>       cc->has_work = s390_cpu_has_work;
>   #ifdef CONFIG_TCG
> 

This is a nice cleanup, but the patch is hard to digest.
Maybe you can split it in 3, one patch for each cpu_reset_type.

Regards,

Phil.

Re: [PATCH 2/4] s390x: Cleanup cpu resets

[Janosch Frank]

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

On 11/22/19 11:35 AM, Philippe Mathieu-Daudé wrote:
> Hi Janosh,
> 
> On 11/22/19 8:52 AM, Janosch Frank wrote:
>> Let's move the resets into one function and switch by type, so we can
>> use fallthroughs for shared reset actions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/cpu.c | 110 ++++++++++++++++++++-------------------------
>>   1 file changed, 48 insertions(+), 62 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 3abe7e80fd..556afecbc1 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -82,67 +82,52 @@ static void s390_cpu_load_normal(CPUState *s)
>>   }
>>   #endif
>>   
>> -/* S390CPUClass::cpu_reset() */
>> -static void s390_cpu_reset(CPUState *s)
>> +typedef enum cpu_reset_type {
>> +    S390_CPU_RESET_NORMAL,
>> +    S390_CPU_RESET_INITIAL,
>> +    S390_CPU_RESET_CLEAR,
>> +} cpu_reset_type;
>> +
>> +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);
>> -}
>>   
>> -/* S390CPUClass::initial_reset() */
>> -static void s390_cpu_initial_reset(CPUState *s)
>> -{
>> -    S390CPU *cpu = S390_CPU(s);
>> -    CPUS390XState *env = &cpu->env;
>> +    /* Set initial values after clearing */
>> +    switch (type) {
>> +    case S390_CPU_RESET_CLEAR:
>> +        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
>> +        /* Fallthrough */
>> +    case S390_CPU_RESET_INITIAL:
>> +        memset(&env->start_initial_reset_fields, 0,
>> +               offsetof(CPUS390XState, end_reset_fields) -
>> +               offsetof(CPUS390XState, start_initial_reset_fields));
>> +        /* architectured initial values for CR 0 and 14 */
>> +        env->cregs[0] = CR0_RESET;
>> +        env->cregs[14] = CR14_RESET;
>>   
>> -    s390_cpu_reset(s);
>> -    /* initial reset does not clear everything! */
>> -    memset(&env->start_initial_reset_fields, 0,
>> -        offsetof(CPUS390XState, end_reset_fields) -
>> -        offsetof(CPUS390XState, start_initial_reset_fields));
>> -
>> -    /* architectured initial values for CR 0 and 14 */
>> -    env->cregs[0] = CR0_RESET;
>> -    env->cregs[14] = CR14_RESET;
>> -
>> -    /* architectured initial value for Breaking-Event-Address register */
>> -    env->gbea = 1;
>> -
>> -    env->pfault_token = -1UL;
>> -
>> -    /* tininess for underflow is detected before rounding */
>> -    set_float_detect_tininess(float_tininess_before_rounding,
>> -                              &env->fpu_status);
>> +        /* architectured initial value for Breaking-Event-Address register */
>> +        env->gbea = 1;
>> +        /* tininess for underflow is detected before rounding */
>> +        set_float_detect_tininess(float_tininess_before_rounding,
>> +                                  &env->fpu_status);
>> +        /* Fallthrough */
>> +    case S390_CPU_RESET_NORMAL:
>> +        env->pfault_token = -1UL;
>> +        env->bpbc = false;
>> +        break;
>> +    }
>>   
>>       /* Reset state inside the kernel that we cannot access yet from QEMU. */
>> -    if (kvm_enabled()) {
>> -        kvm_s390_reset_vcpu(cpu);
>> +    if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
>> +                          type == S390_CPU_RESET_INITIAL)) {
>> +            kvm_s390_reset_vcpu(cpu);
>>       }
>> -}
>> -
>> -/* CPUClass:reset() */
>> -static void s390_cpu_full_reset(CPUState *s)
>> -{
>> -    S390CPU *cpu = S390_CPU(s);
>> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>> -    CPUS390XState *env = &cpu->env;
>> -
>> -    scc->parent_reset(s);
>> -    cpu->env.sigp_order = 0;
>> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>> -
>> -    memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
>> -
>> -    /* architectured initial values for CR 0 and 14 */
>> -    env->cregs[0] = CR0_RESET;
>> -    env->cregs[14] = CR14_RESET;
>>   
>>   #if defined(CONFIG_USER_ONLY)
>>       /* user mode should always be allowed to use the full FPU */
>> @@ -151,20 +136,21 @@ static void s390_cpu_full_reset(CPUState *s)
>>           env->cregs[0] |= CR0_VECTOR;
>>       }
>>   #endif
>> +}
>>   
>> -    /* architectured initial value for Breaking-Event-Address register */
>> -    env->gbea = 1;
>> +static void s390_cpu_reset_normal(CPUState *s)
>> +{
>> +    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
>> +}
>>   
>> -    env->pfault_token = -1UL;
>> +static void s390_cpu_reset_initial(CPUState *s)
>> +{
>> +    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
>> +}
>>   
>> -    /* tininess for underflow is detected before rounding */
>> -    set_float_detect_tininess(float_tininess_before_rounding,
>> -                              &env->fpu_status);
>> -
>> -    /* Reset state inside the kernel that we cannot access yet from QEMU. */
>> -    if (kvm_enabled()) {
>> -        kvm_s390_reset_vcpu(cpu);
>> -    }
>> +static void s390_cpu_reset_clear(CPUState *s)
>> +{
>> +    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
>>   }
>>   
>>   #if !defined(CONFIG_USER_ONLY)
>> @@ -473,9 +459,9 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>   #if !defined(CONFIG_USER_ONLY)
>>       scc->load_normal = s390_cpu_load_normal;
>>   #endif
>> -    scc->cpu_reset = s390_cpu_reset;
>> -    scc->initial_cpu_reset = s390_cpu_initial_reset;
>> -    cc->reset = s390_cpu_full_reset;
>> +    scc->cpu_reset = s390_cpu_reset_normal;
>> +    scc->initial_cpu_reset = s390_cpu_reset_initial;
>> +    cc->reset = s390_cpu_reset_clear;
>>       cc->class_by_name = s390_cpu_class_by_name,
>>       cc->has_work = s390_cpu_has_work;
>>   #ifdef CONFIG_TCG
>>
> 
> This is a nice cleanup, but the patch is hard to digest.
> Maybe you can split it in 3, one patch for each cpu_reset_type.
> 
> Regards,
> 
> Phil.
> 

I'll try to split it up.



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

Re: [PATCH 2/4] s390x: Cleanup cpu resets

[David Hildenbrand]

[...]

>   
>   #if !defined(CONFIG_USER_ONLY)
> @@ -473,9 +459,9 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>   #if !defined(CONFIG_USER_ONLY)
>       scc->load_normal = s390_cpu_load_normal;
>   #endif
> -    scc->cpu_reset = s390_cpu_reset;
> -    scc->initial_cpu_reset = s390_cpu_initial_reset;
> -    cc->reset = s390_cpu_full_reset;
> +    scc->cpu_reset = s390_cpu_reset_normal;
> +    scc->initial_cpu_reset = s390_cpu_reset_initial;

What about having only one function here

scc->cpu_reset(), where you directly pass in the reset type

instead of scc->cpu_reset/scc->initial_cpu_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
> 


-- 

Thanks,

David / dhildenb

[PATCH] Remove wrappers

[Janosch Frank]

That's what it would look like.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu-qom.h | 12 ++++++++----
 target/s390x/cpu.c     | 28 +++++-----------------------
 target/s390x/cpu.h     |  8 +++++---
 target/s390x/sigp.c    |  4 ++--
 4 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index b809ec8418..e8ec999e77 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -34,13 +34,18 @@
 typedef struct S390CPUModel S390CPUModel;
 typedef struct S390CPUDef S390CPUDef;
 
+typedef enum cpu_reset_type {
+    S390_CPU_RESET_NORMAL,
+    S390_CPU_RESET_INITIAL,
+    S390_CPU_RESET_CLEAR,
+} cpu_reset_type;
+
 /**
  * S390CPUClass:
  * @parent_realize: The parent class' realize handler.
  * @parent_reset: The parent class' reset handler.
  * @load_normal: Performs a load normal.
- * @cpu_reset: Performs a CPU reset.
- * @initial_cpu_reset: Performs an initial CPU reset.
+ * @reset: Performs a CPU reset of a given type.
  *
  * An S/390 CPU model.
  */
@@ -57,8 +62,7 @@ typedef struct S390CPUClass {
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
     void (*load_normal)(CPUState *cpu);
-    void (*cpu_reset)(CPUState *cpu);
-    void (*initial_cpu_reset)(CPUState *cpu);
+    void (*reset)(CPUState *cpu, cpu_reset_type type);
 } S390CPUClass;
 
 typedef struct S390CPU S390CPU;
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 556afecbc1..970495d042 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -82,12 +82,6 @@ static void s390_cpu_load_normal(CPUState *s)
 }
 #endif
 
-typedef enum cpu_reset_type {
-    S390_CPU_RESET_NORMAL,
-    S390_CPU_RESET_INITIAL,
-    S390_CPU_RESET_CLEAR,
-} cpu_reset_type;
-
 static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
 {
     S390CPU *cpu = S390_CPU(s);
@@ -138,21 +132,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
 #endif
 }
 
-static void s390_cpu_reset_normal(CPUState *s)
-{
-    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
-}
-
-static void s390_cpu_reset_initial(CPUState *s)
-{
-    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
-}
-
-static void s390_cpu_reset_clear(CPUState *s)
-{
-    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
-}
-
 #if !defined(CONFIG_USER_ONLY)
 static void s390_cpu_machine_reset_cb(void *opaque)
 {
@@ -444,6 +423,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);
@@ -459,8 +443,6 @@ 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_normal;
-    scc->initial_cpu_reset = s390_cpu_reset_initial;
     cc->reset = s390_cpu_reset_clear;
     cc->class_by_name = s390_cpu_class_by_name,
     cc->has_work = s390_cpu_has_work;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 17460ed7b3..687b31d87e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -734,21 +734,23 @@ static inline uint64_t s390_build_validity_mcic(void)
 
 static inline void s390_do_cpu_full_reset(CPUState *cs, run_on_cpu_data arg)
 {
-    cpu_reset(cs);
+    S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
+
+    scc->reset(cs, S390_CPU_RESET_CLEAR);
 }
 
 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_CLEAR);
 }
 
 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 2ce22d4dc1..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;
 }
@@ -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] Remove wrappers

[David Hildenbrand]

On 22.11.19 13:20, Janosch Frank wrote:
> That's what it would look like.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/cpu-qom.h | 12 ++++++++----
>   target/s390x/cpu.c     | 28 +++++-----------------------
>   target/s390x/cpu.h     |  8 +++++---
>   target/s390x/sigp.c    |  4 ++--
>   4 files changed, 20 insertions(+), 32 deletions(-)
> 
> diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
> index b809ec8418..e8ec999e77 100644
> --- a/target/s390x/cpu-qom.h
> +++ b/target/s390x/cpu-qom.h
> @@ -34,13 +34,18 @@
>   typedef struct S390CPUModel S390CPUModel;
>   typedef struct S390CPUDef S390CPUDef;
>   
> +typedef enum cpu_reset_type {
> +    S390_CPU_RESET_NORMAL,
> +    S390_CPU_RESET_INITIAL,
> +    S390_CPU_RESET_CLEAR,
> +} cpu_reset_type;
> +
>   /**
>    * S390CPUClass:
>    * @parent_realize: The parent class' realize handler.
>    * @parent_reset: The parent class' reset handler.
>    * @load_normal: Performs a load normal.
> - * @cpu_reset: Performs a CPU reset.
> - * @initial_cpu_reset: Performs an initial CPU reset.
> + * @reset: Performs a CPU reset of a given type.
>    *
>    * An S/390 CPU model.
>    */
> @@ -57,8 +62,7 @@ typedef struct S390CPUClass {
>       DeviceRealize parent_realize;
>       void (*parent_reset)(CPUState *cpu);
>       void (*load_normal)(CPUState *cpu);
> -    void (*cpu_reset)(CPUState *cpu);
> -    void (*initial_cpu_reset)(CPUState *cpu);
> +    void (*reset)(CPUState *cpu, cpu_reset_type type);
>   } S390CPUClass;
>   
>   typedef struct S390CPU S390CPU;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 556afecbc1..970495d042 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -82,12 +82,6 @@ static void s390_cpu_load_normal(CPUState *s)
>   }
>   #endif
>   
> -typedef enum cpu_reset_type {
> -    S390_CPU_RESET_NORMAL,
> -    S390_CPU_RESET_INITIAL,
> -    S390_CPU_RESET_CLEAR,
> -} cpu_reset_type;
> -
>   static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>   {
>       S390CPU *cpu = S390_CPU(s);
> @@ -138,21 +132,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>   #endif
>   }
>   
> -static void s390_cpu_reset_normal(CPUState *s)
> -{
> -    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
> -}
> -
> -static void s390_cpu_reset_initial(CPUState *s)
> -{
> -    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
> -}
> -
> -static void s390_cpu_reset_clear(CPUState *s)
> -{
> -    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
> -}
> -
>   #if !defined(CONFIG_USER_ONLY)
>   static void s390_cpu_machine_reset_cb(void *opaque)
>   {
> @@ -444,6 +423,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);
> @@ -459,8 +443,6 @@ 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_normal;
> -    scc->initial_cpu_reset = s390_cpu_reset_initial;

You have to set

scc->reset = s390_cpu_reset;

if I'm not wrong.

>       cc->reset = s390_cpu_reset_clear;
>       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 17460ed7b3..687b31d87e 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -734,21 +734,23 @@ static inline uint64_t s390_build_validity_mcic(void)
>   
>   static inline void s390_do_cpu_full_reset(CPUState *cs, run_on_cpu_data arg)
>   {
> -    cpu_reset(cs);
> +    S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
> +
> +    scc->reset(cs, S390_CPU_RESET_CLEAR);
>   }
>   
>   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_CLEAR);
>   }
>   
>   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 2ce22d4dc1..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;
>   }
> @@ -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;
>   }
> 

Looks good and much cleaner to me :)

-- 

Thanks,

David / dhildenb

Re: [PATCH] Remove wrappers

[Janosch Frank]

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

On 11/22/19 1:23 PM, David Hildenbrand wrote:
> On 22.11.19 13:20, Janosch Frank wrote:
>> That's what it would look like.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/cpu-qom.h | 12 ++++++++----
>>   target/s390x/cpu.c     | 28 +++++-----------------------
>>   target/s390x/cpu.h     |  8 +++++---
>>   target/s390x/sigp.c    |  4 ++--
>>   4 files changed, 20 insertions(+), 32 deletions(-)
>>
>> diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
>> index b809ec8418..e8ec999e77 100644
>> --- a/target/s390x/cpu-qom.h
>> +++ b/target/s390x/cpu-qom.h
>> @@ -34,13 +34,18 @@
>>   typedef struct S390CPUModel S390CPUModel;
>>   typedef struct S390CPUDef S390CPUDef;
>>   
>> +typedef enum cpu_reset_type {
>> +    S390_CPU_RESET_NORMAL,
>> +    S390_CPU_RESET_INITIAL,
>> +    S390_CPU_RESET_CLEAR,
>> +} cpu_reset_type;
>> +
>>   /**
>>    * S390CPUClass:
>>    * @parent_realize: The parent class' realize handler.
>>    * @parent_reset: The parent class' reset handler.
>>    * @load_normal: Performs a load normal.
>> - * @cpu_reset: Performs a CPU reset.
>> - * @initial_cpu_reset: Performs an initial CPU reset.
>> + * @reset: Performs a CPU reset of a given type.
>>    *
>>    * An S/390 CPU model.
>>    */
>> @@ -57,8 +62,7 @@ typedef struct S390CPUClass {
>>       DeviceRealize parent_realize;
>>       void (*parent_reset)(CPUState *cpu);
>>       void (*load_normal)(CPUState *cpu);
>> -    void (*cpu_reset)(CPUState *cpu);
>> -    void (*initial_cpu_reset)(CPUState *cpu);
>> +    void (*reset)(CPUState *cpu, cpu_reset_type type);
>>   } S390CPUClass;
>>   
>>   typedef struct S390CPU S390CPU;
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 556afecbc1..970495d042 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -82,12 +82,6 @@ static void s390_cpu_load_normal(CPUState *s)
>>   }
>>   #endif
>>   
>> -typedef enum cpu_reset_type {
>> -    S390_CPU_RESET_NORMAL,
>> -    S390_CPU_RESET_INITIAL,
>> -    S390_CPU_RESET_CLEAR,
>> -} cpu_reset_type;
>> -
>>   static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>   {
>>       S390CPU *cpu = S390_CPU(s);
>> @@ -138,21 +132,6 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>   #endif
>>   }
>>   
>> -static void s390_cpu_reset_normal(CPUState *s)
>> -{
>> -    return s390_cpu_reset(s, S390_CPU_RESET_NORMAL);
>> -}
>> -
>> -static void s390_cpu_reset_initial(CPUState *s)
>> -{
>> -    return s390_cpu_reset(s, S390_CPU_RESET_INITIAL);
>> -}
>> -
>> -static void s390_cpu_reset_clear(CPUState *s)
>> -{
>> -    return s390_cpu_reset(s, S390_CPU_RESET_CLEAR);
>> -}
>> -
>>   #if !defined(CONFIG_USER_ONLY)
>>   static void s390_cpu_machine_reset_cb(void *opaque)
>>   {
>> @@ -444,6 +423,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);
>> @@ -459,8 +443,6 @@ 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_normal;
>> -    scc->initial_cpu_reset = s390_cpu_reset_initial;
> 
> You have to set
> 
> scc->reset = s390_cpu_reset;
> 
> if I'm not wrong.

Yeah, that would make sense :)
I'm also going to tripple check if we are doing the right resets.


> 
>>       cc->reset = s390_cpu_reset_clear;
>>       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 17460ed7b3..687b31d87e 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -734,21 +734,23 @@ static inline uint64_t s390_build_validity_mcic(void)
>>   
>>   static inline void s390_do_cpu_full_reset(CPUState *cs, run_on_cpu_data arg)
>>   {
>> -    cpu_reset(cs);
>> +    S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
>> +
>> +    scc->reset(cs, S390_CPU_RESET_CLEAR);
>>   }
>>   
>>   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_CLEAR);
>>   }
>>   
>>   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 2ce22d4dc1..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;
>>   }
>> @@ -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;
>>   }
>>
> 
> Looks good and much cleaner to me :)
> 



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

[PATCH 3/4] 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>
---
 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..18d33c8492 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 3/4] s390x: Beautify diag308 handling

[David Hildenbrand]

On 22.11.19 08:52, Janosch Frank wrote:
> 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>
> ---
>   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..18d33c8492 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();
> 

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

-- 

Thanks,

David / dhildenb

[PATCH 4/4] s390x: Beautify machine reset

[Janosch Frank]

* Add comments that tell you which diag308 subcode caused the reset
* Sort by diag308 reset subcode

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index c1d1440272..88f7758721 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
     s390_cmma_reset();
 
     switch (reset_type) {
-    case S390_RESET_EXTERNAL:
-    case S390_RESET_REIPL:
-        qemu_devices_reset();
-        s390_crypto_reset();
-
-        /* configure and start the ipl CPU only */
-        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
-        break;
-    case S390_RESET_MODIFIED_CLEAR:
+    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
         CPU_FOREACH(t) {
             run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
         }
@@ -346,7 +338,7 @@ static void s390_machine_reset(MachineState *machine)
         s390_crypto_reset();
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
-    case S390_RESET_LOAD_NORMAL:
+    case S390_RESET_LOAD_NORMAL: /* Subcode 1 */
         CPU_FOREACH(t) {
             if (t == cs) {
                 continue;
@@ -357,6 +349,14 @@ static void s390_machine_reset(MachineState *machine)
         run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
         run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
         break;
+    case S390_RESET_EXTERNAL: /* Externally triggered reboot */
+    case S390_RESET_REIPL: /* Subcode 4 */
+        qemu_devices_reset();
+        s390_crypto_reset();
+
+        /* configure and start the ipl CPU only */
+        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
+        break;
     default:
         g_assert_not_reached();
     }
-- 
2.20.1

Re: [PATCH 4/4] s390x: Beautify machine reset

[David Hildenbrand]

On 22.11.19 08:52, Janosch Frank wrote:
> * Add comments that tell you which diag308 subcode caused the reset
> * Sort by diag308 reset subcode
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c1d1440272..88f7758721 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>       s390_cmma_reset();
>   
>       switch (reset_type) {
> -    case S390_RESET_EXTERNAL:
> -    case S390_RESET_REIPL:
> -        qemu_devices_reset();
> -        s390_crypto_reset();
> -
> -        /* configure and start the ipl CPU only */
> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
> -        break;
> -    case S390_RESET_MODIFIED_CLEAR:
> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */

IMHO "Subcode X" isn't of much help here. We're out of diag handling.

I'd suggest to just document the subcodes along with the definitions, if 
really needed, and drop this patch, at least I don't quite see the value 
of moving code around here... or is the code shuffling of any value on 
your prot virt patches?

-- 

Thanks,

David / dhildenb

Re: [PATCH 4/4] s390x: Beautify machine reset

[Janosch Frank]

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

On 11/22/19 11:59 AM, David Hildenbrand wrote:
> On 22.11.19 08:52, Janosch Frank wrote:
>> * Add comments that tell you which diag308 subcode caused the reset
>> * Sort by diag308 reset subcode
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index c1d1440272..88f7758721 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>       s390_cmma_reset();
>>   
>>       switch (reset_type) {
>> -    case S390_RESET_EXTERNAL:
>> -    case S390_RESET_REIPL:
>> -        qemu_devices_reset();
>> -        s390_crypto_reset();
>> -
>> -        /* configure and start the ipl CPU only */
>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>> -        break;
>> -    case S390_RESET_MODIFIED_CLEAR:
>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
> 
> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
> 
> I'd suggest to just document the subcodes along with the definitions, if 
> really needed, and drop this patch, at least I don't quite see the value 
> of moving code around here... or is the code shuffling of any value on 
> your prot virt patches?
> 

It keeps me from consulting the POP every time I need to change things
in the machine resets. This is basically a 1:1 mapping of diag 308
subcodes to machine resets, so why don't we want to make that obvious
and order them by the subcodes?


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

Re: [PATCH 4/4] s390x: Beautify machine reset

[David Hildenbrand]

On 22.11.19 12:46, Janosch Frank wrote:
> On 11/22/19 11:59 AM, David Hildenbrand wrote:
>> On 22.11.19 08:52, Janosch Frank wrote:
>>> * Add comments that tell you which diag308 subcode caused the reset
>>> * Sort by diag308 reset subcode
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index c1d1440272..88f7758721 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>>        s390_cmma_reset();
>>>    
>>>        switch (reset_type) {
>>> -    case S390_RESET_EXTERNAL:
>>> -    case S390_RESET_REIPL:
>>> -        qemu_devices_reset();
>>> -        s390_crypto_reset();
>>> -
>>> -        /* configure and start the ipl CPU only */
>>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>> -        break;
>>> -    case S390_RESET_MODIFIED_CLEAR:
>>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
>>
>> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
>>
>> I'd suggest to just document the subcodes along with the definitions, if
>> really needed, and drop this patch, at least I don't quite see the value
>> of moving code around here... or is the code shuffling of any value on
>> your prot virt patches?
>>
> 
> It keeps me from consulting the POP every time I need to change things
> in the machine resets. This is basically a 1:1 mapping of diag 308
> subcodes to machine resets, so why don't we want to make that obvious
> and order them by the subcodes?
> 

Because it is not a 1:1 mapping: S390_RESET_EXTERNAL

-- 

Thanks,

David / dhildenb

Re: [PATCH 4/4] s390x: Beautify machine reset

[Cornelia Huck]

On Fri, 22 Nov 2019 12:47:44 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 22.11.19 12:46, Janosch Frank wrote:
> > On 11/22/19 11:59 AM, David Hildenbrand wrote:  
> >> On 22.11.19 08:52, Janosch Frank wrote:  
> >>> * Add comments that tell you which diag308 subcode caused the reset
> >>> * Sort by diag308 reset subcode
> >>>
> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >>> ---
> >>>    hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
> >>>    1 file changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >>> index c1d1440272..88f7758721 100644
> >>> --- a/hw/s390x/s390-virtio-ccw.c
> >>> +++ b/hw/s390x/s390-virtio-ccw.c
> >>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
> >>>        s390_cmma_reset();
> >>>    
> >>>        switch (reset_type) {
> >>> -    case S390_RESET_EXTERNAL:
> >>> -    case S390_RESET_REIPL:
> >>> -        qemu_devices_reset();
> >>> -        s390_crypto_reset();
> >>> -
> >>> -        /* configure and start the ipl CPU only */
> >>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
> >>> -        break;
> >>> -    case S390_RESET_MODIFIED_CLEAR:
> >>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */  
> >>
> >> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
> >>
> >> I'd suggest to just document the subcodes along with the definitions, if
> >> really needed, and drop this patch, at least I don't quite see the value
> >> of moving code around here... or is the code shuffling of any value on
> >> your prot virt patches?
> >>  
> > 
> > It keeps me from consulting the POP every time I need to change things
> > in the machine resets. This is basically a 1:1 mapping of diag 308
> > subcodes to machine resets, so why don't we want to make that obvious
> > and order them by the subcodes?
> >   
> 
> Because it is not a 1:1 mapping: S390_RESET_EXTERNAL
> 

Tack the explanation onto the definitions of S390_RESET_, then?
Probably still quicker than consulting the POP :)

Re: [PATCH 4/4] s390x: Beautify machine reset

[Janosch Frank]

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

On 11/22/19 1:10 PM, Cornelia Huck wrote:
> On Fri, 22 Nov 2019 12:47:44 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 22.11.19 12:46, Janosch Frank wrote:
>>> On 11/22/19 11:59 AM, David Hildenbrand wrote:  
>>>> On 22.11.19 08:52, Janosch Frank wrote:  
>>>>> * Add comments that tell you which diag308 subcode caused the reset
>>>>> * Sort by diag308 reset subcode
>>>>>
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>    hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>>>>    1 file changed, 10 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>> index c1d1440272..88f7758721 100644
>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>>        s390_cmma_reset();
>>>>>    
>>>>>        switch (reset_type) {
>>>>> -    case S390_RESET_EXTERNAL:
>>>>> -    case S390_RESET_REIPL:
>>>>> -        qemu_devices_reset();
>>>>> -        s390_crypto_reset();
>>>>> -
>>>>> -        /* configure and start the ipl CPU only */
>>>>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>>>> -        break;
>>>>> -    case S390_RESET_MODIFIED_CLEAR:
>>>>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */  
>>>>
>>>> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
>>>>
>>>> I'd suggest to just document the subcodes along with the definitions, if
>>>> really needed, and drop this patch, at least I don't quite see the value
>>>> of moving code around here... or is the code shuffling of any value on
>>>> your prot virt patches?
>>>>  
>>>
>>> It keeps me from consulting the POP every time I need to change things
>>> in the machine resets. This is basically a 1:1 mapping of diag 308
>>> subcodes to machine resets, so why don't we want to make that obvious
>>> and order them by the subcodes?
>>>   
>>
>> Because it is not a 1:1 mapping: S390_RESET_EXTERNAL
>>
> 
> Tack the explanation onto the definitions of S390_RESET_, then?
> Probably still quicker than consulting the POP :)
> 

Does it really bother you that much, that I add some explanations to the
things we're doing. The external reset also gets a comment so Conni
won't need that much coffee anymore to understand the code :-)


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

Re: [PATCH 4/4] s390x: Beautify machine reset

[David Hildenbrand]

On 22.11.19 13:22, Janosch Frank wrote:
> On 11/22/19 1:10 PM, Cornelia Huck wrote:
>> On Fri, 22 Nov 2019 12:47:44 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>> On 22.11.19 12:46, Janosch Frank wrote:
>>>> On 11/22/19 11:59 AM, David Hildenbrand wrote:
>>>>> On 22.11.19 08:52, Janosch Frank wrote:
>>>>>> * Add comments that tell you which diag308 subcode caused the reset
>>>>>> * Sort by diag308 reset subcode
>>>>>>
>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>> ---
>>>>>>     hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>>>>>     1 file changed, 10 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>> index c1d1440272..88f7758721 100644
>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>>>         s390_cmma_reset();
>>>>>>     
>>>>>>         switch (reset_type) {
>>>>>> -    case S390_RESET_EXTERNAL:
>>>>>> -    case S390_RESET_REIPL:
>>>>>> -        qemu_devices_reset();
>>>>>> -        s390_crypto_reset();
>>>>>> -
>>>>>> -        /* configure and start the ipl CPU only */
>>>>>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>>>>> -        break;
>>>>>> -    case S390_RESET_MODIFIED_CLEAR:
>>>>>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
>>>>>
>>>>> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
>>>>>
>>>>> I'd suggest to just document the subcodes along with the definitions, if
>>>>> really needed, and drop this patch, at least I don't quite see the value
>>>>> of moving code around here... or is the code shuffling of any value on
>>>>> your prot virt patches?
>>>>>   
>>>>
>>>> It keeps me from consulting the POP every time I need to change things
>>>> in the machine resets. This is basically a 1:1 mapping of diag 308
>>>> subcodes to machine resets, so why don't we want to make that obvious
>>>> and order them by the subcodes?
>>>>    
>>>
>>> Because it is not a 1:1 mapping: S390_RESET_EXTERNAL
>>>
>>
>> Tack the explanation onto the definitions of S390_RESET_, then?
>> Probably still quicker than consulting the POP :)
>>
> 
> Does it really bother you that much, that I add some explanations to the
> things we're doing. The external reset also gets a comment so Conni
> won't need that much coffee anymore to understand the code :-)
> 

I'm really sorry, but I fail to see how "Subcode 0" is *any* better than 
S390_RESET_MODIFIED_CLEAR (and avoids consulting the PoP) and why the 
order should matter at all here to make it easier to understand.

I don't NACK this, I just find it *completely* useless :)

-- 

Thanks,

David / dhildenb

Re: [PATCH 4/4] s390x: Beautify machine reset

[Janosch Frank]

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

On 11/22/19 1:25 PM, David Hildenbrand wrote:
> On 22.11.19 13:22, Janosch Frank wrote:
>> On 11/22/19 1:10 PM, Cornelia Huck wrote:
>>> On Fri, 22 Nov 2019 12:47:44 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>> On 22.11.19 12:46, Janosch Frank wrote:
>>>>> On 11/22/19 11:59 AM, David Hildenbrand wrote:
>>>>>> On 22.11.19 08:52, Janosch Frank wrote:
>>>>>>> * Add comments that tell you which diag308 subcode caused the reset
>>>>>>> * Sort by diag308 reset subcode
>>>>>>>
>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>> ---
>>>>>>>     hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>>>>>>>     1 file changed, 10 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>>>>> index c1d1440272..88f7758721 100644
>>>>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>>>>> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>>>>         s390_cmma_reset();
>>>>>>>     
>>>>>>>         switch (reset_type) {
>>>>>>> -    case S390_RESET_EXTERNAL:
>>>>>>> -    case S390_RESET_REIPL:
>>>>>>> -        qemu_devices_reset();
>>>>>>> -        s390_crypto_reset();
>>>>>>> -
>>>>>>> -        /* configure and start the ipl CPU only */
>>>>>>> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>>>>>>> -        break;
>>>>>>> -    case S390_RESET_MODIFIED_CLEAR:
>>>>>>> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
>>>>>>
>>>>>> IMHO "Subcode X" isn't of much help here. We're out of diag handling.
>>>>>>
>>>>>> I'd suggest to just document the subcodes along with the definitions, if
>>>>>> really needed, and drop this patch, at least I don't quite see the value
>>>>>> of moving code around here... or is the code shuffling of any value on
>>>>>> your prot virt patches?
>>>>>>   
>>>>>
>>>>> It keeps me from consulting the POP every time I need to change things
>>>>> in the machine resets. This is basically a 1:1 mapping of diag 308
>>>>> subcodes to machine resets, so why don't we want to make that obvious
>>>>> and order them by the subcodes?
>>>>>    
>>>>
>>>> Because it is not a 1:1 mapping: S390_RESET_EXTERNAL
>>>>
>>>
>>> Tack the explanation onto the definitions of S390_RESET_, then?
>>> Probably still quicker than consulting the POP :)
>>>
>>
>> Does it really bother you that much, that I add some explanations to the
>> things we're doing. The external reset also gets a comment so Conni
>> won't need that much coffee anymore to understand the code :-)
>>
> 
> I'm really sorry, but I fail to see how "Subcode 0" is *any* better than 
> S390_RESET_MODIFIED_CLEAR (and avoids consulting the PoP) and why the 
> order should matter at all here to make it easier to understand.
> 
> I don't NACK this, I just find it *completely* useless :)
> 

Ugh, time for some rebase conflicts...


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

Re: [PATCH 4/4] s390x: Beautify machine reset

[David Hildenbrand]

On 22.11.19 08:52, Janosch Frank wrote:
> * Add comments that tell you which diag308 subcode caused the reset
> * Sort by diag308 reset subcode
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c1d1440272..88f7758721 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -330,15 +330,7 @@ static void s390_machine_reset(MachineState *machine)
>      s390_cmma_reset();
>  
>      switch (reset_type) {
> -    case S390_RESET_EXTERNAL:
> -    case S390_RESET_REIPL:
> -        qemu_devices_reset();
> -        s390_crypto_reset();
> -
> -        /* configure and start the ipl CPU only */
> -        run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
> -        break;
> -    case S390_RESET_MODIFIED_CLEAR:
> +    case S390_RESET_MODIFIED_CLEAR: /* Subcode 0 */
>          CPU_FOREACH(t) {
>              run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
>          }
> @@ -346,7 +338,7 @@ static void s390_machine_reset(MachineState *machine)
>          s390_crypto_reset();
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
> -    case S390_RESET_LOAD_NORMAL:
> +    case S390_RESET_LOAD_NORMAL: /* Subcode 1 */
>          CPU_FOREACH(t) {
>              if (t == cs) {
>                  continue;
> @@ -357,6 +349,14 @@ static void s390_machine_reset(MachineState *machine)
>          run_on_cpu(cs, s390_do_cpu_initial_reset, RUN_ON_CPU_NULL);
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
> +    case S390_RESET_EXTERNAL: /* Externally triggered reboot */

BTW, if we decide to document this *somehow*, S390_RESET_EXTERNAL is
also used via the diag288 watchdog.

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/4] s390x: Reset cleanup

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191122075218.23935-1-frankja@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/4] s390x: Reset cleanup
Type: series
Message-id: 20191122075218.23935-1-frankja@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
b557dea s390x: Beautify machine reset
e5244c7 s390x: Beautify diag308 handling
288f436 s390x: Cleanup cpu resets
9f85af6 s390x: Don't do a normal reset on the initial cpu

=== OUTPUT BEGIN ===
1/4 Checking commit 9f85af68844a (s390x: Don't do a normal reset on the initial cpu)
2/4 Checking commit 288f436dc8e9 (s390x: Cleanup cpu resets)
3/4 Checking commit e5244c7edde8 (s390x: Beautify diag308 handling)
ERROR: code indent should never use tabs
#25: FILE: target/s390x/diag.c:56:
+#define DIAG308_RESET_MOD_CLR^I^I0$

ERROR: code indent should never use tabs
#26: FILE: target/s390x/diag.c:57:
+#define DIAG308_RESET_LOAD_NORM^I^I1$

ERROR: code indent should never use tabs
#27: FILE: target/s390x/diag.c:58:
+#define DIAG308_LOAD_CLEAR^I^I3$

ERROR: code indent should never use tabs
#28: FILE: target/s390x/diag.c:59:
+#define DIAG308_LOAD_NORMAL_DUMP^I4$

ERROR: code indent should never use tabs
#29: FILE: target/s390x/diag.c:60:
+#define DIAG308_SET^I^I^I5$

ERROR: code indent should never use tabs
#30: FILE: target/s390x/diag.c:61:
+#define DIAG308_STORE^I^I^I6$

total: 6 errors, 0 warnings, 83 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit b557dea18bc1 (s390x: Beautify machine reset)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191122075218.23935-1-frankja@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com