| Message ID | 20191122075218.23935-5-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Reset cleanup | expand |
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?
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?
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
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 :)
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 :-)
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 :)
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...
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.
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(); }
* 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(-)