[4/4] s390x: Beautify machine reset
diff mbox series

Message ID 20191122075218.23935-5-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: Reset cleanup
Related show

Commit Message

Janosch Frank Nov. 22, 2019, 7:52 a.m. UTC
* 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(-)

Comments

David Hildenbrand Nov. 22, 2019, 10:59 a.m. UTC | #1
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?
Janosch Frank Nov. 22, 2019, 11:46 a.m. UTC | #2
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?
David Hildenbrand Nov. 22, 2019, 11:47 a.m. UTC | #3
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
Cornelia Huck Nov. 22, 2019, 12:10 p.m. UTC | #4
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 :)
Janosch Frank Nov. 22, 2019, 12:22 p.m. UTC | #5
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 :-)
David Hildenbrand Nov. 22, 2019, 12:25 p.m. UTC | #6
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 :)
Janosch Frank Nov. 22, 2019, 12:30 p.m. UTC | #7
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...
David Hildenbrand Nov. 22, 2019, 12:42 p.m. UTC | #8
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.

Patch
diff mbox series

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