| Message ID | 20190718103951.10027-2-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Series to implement suspend for ppc/spapr | expand |
On 18/07/19 12:39, Nicholas Piggin wrote: > Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from > S3") changed system wakeup to avoid calling qapi_event_send_reset. > Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to > have inadvertently broken that logic. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > I'm not quite sure if this patch is correct and haven't tested it, I > found it by inspection. If this patch is incorrect, I will have to > adjust patch 2. > > vl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/vl.c b/vl.c > index 5089fce6c5..ef3c7ab8b8 100644 > --- a/vl.c > +++ b/vl.c > @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) > } else { > qemu_devices_reset(); > } > - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { > + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { > qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); > } > cpu_synchronize_all_post_reset(); > Yes, it seems correct and I've queued it for 4.1. Paolo
On 18.07.19 13:06, Paolo Bonzini wrote: > On 18/07/19 12:39, Nicholas Piggin wrote: >> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from >> S3") changed system wakeup to avoid calling qapi_event_send_reset. >> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to >> have inadvertently broken that logic. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> I'm not quite sure if this patch is correct and haven't tested it, I >> found it by inspection. If this patch is incorrect, I will have to >> adjust patch 2. >> >> vl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/vl.c b/vl.c >> index 5089fce6c5..ef3c7ab8b8 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) >> } else { >> qemu_devices_reset(); >> } >> - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >> + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); >> } >> cpu_synchronize_all_post_reset(); >> > > Yes, it seems correct and I've queued it for 4.1. Yes makes sense. Would it be better to write out if(reason) e.g. replace "reason" with "reason != SHUTDOWN_CAUSE_NONE" ? Going even further, I am asking myself if something like diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c index 60bd081d3ef3..406743566869 100644 --- a/hw/s390x/ipl.c +++ b/hw/s390x/ipl.c @@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type) if (reset_type == S390_RESET_MODIFIED_CLEAR || reset_type == S390_RESET_LOAD_NORMAL) { /* ignore -no-reboot, send no event */ - qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET); + qemu_system_reset_request(SHUTDOWN_CAUSE_NONE); } else { qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); } would also work.
Christian Borntraeger's on July 18, 2019 9:27 pm: > > > On 18.07.19 13:06, Paolo Bonzini wrote: >> On 18/07/19 12:39, Nicholas Piggin wrote: >>> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from >>> S3") changed system wakeup to avoid calling qapi_event_send_reset. >>> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to >>> have inadvertently broken that logic. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> I'm not quite sure if this patch is correct and haven't tested it, I >>> found it by inspection. If this patch is incorrect, I will have to >>> adjust patch 2. >>> >>> vl.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/vl.c b/vl.c >>> index 5089fce6c5..ef3c7ab8b8 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) >>> } else { >>> qemu_devices_reset(); >>> } >>> - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >>> + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >>> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); >>> } >>> cpu_synchronize_all_post_reset(); >>> >> >> Yes, it seems correct and I've queued it for 4.1. > > Yes makes sense. > Would it be better to write out if(reason) e.g. replace "reason" with "reason != SHUTDOWN_CAUSE_NONE" ? I guess it's a matter of preference so I won't weigh in :) My patch did try to revert back to the previous form but I'm happy to change it. > Going even further, I am asking myself if something like > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > index 60bd081d3ef3..406743566869 100644 > --- a/hw/s390x/ipl.c > +++ b/hw/s390x/ipl.c > @@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type) > if (reset_type == S390_RESET_MODIFIED_CLEAR || > reset_type == S390_RESET_LOAD_NORMAL) { > /* ignore -no-reboot, send no event */ > - qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET); > + qemu_system_reset_request(SHUTDOWN_CAUSE_NONE); > } else { > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > } > > would also work. If that works for you, then you could take out the test in the reset code. You would have to modify qemu_system_reset_request too of course. But it seems a bit unsatisfactory to change the reason for the request so as to influence behaviour. Either the requests should ask for particular behaviour, or the logic for determining how to handle the type of request should remain in the reset logic, I would say. Thanks, Nick
On 19.07.19 01:24, Nicholas Piggin wrote: > Christian Borntraeger's on July 18, 2019 9:27 pm: >> >> >> On 18.07.19 13:06, Paolo Bonzini wrote: >>> On 18/07/19 12:39, Nicholas Piggin wrote: >>>> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from >>>> S3") changed system wakeup to avoid calling qapi_event_send_reset. >>>> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to >>>> have inadvertently broken that logic. >>>> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>>> --- >>>> I'm not quite sure if this patch is correct and haven't tested it, I >>>> found it by inspection. If this patch is incorrect, I will have to >>>> adjust patch 2. >>>> >>>> vl.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/vl.c b/vl.c >>>> index 5089fce6c5..ef3c7ab8b8 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) >>>> } else { >>>> qemu_devices_reset(); >>>> } >>>> - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >>>> + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { >>>> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); >>>> } >>>> cpu_synchronize_all_post_reset(); >>>> >>> >>> Yes, it seems correct and I've queued it for 4.1. >> >> Yes makes sense. >> Would it be better to write out if(reason) e.g. replace "reason" with "reason != SHUTDOWN_CAUSE_NONE" ? > > I guess it's a matter of preference so I won't weigh in :) My patch > did try to revert back to the previous form but I'm happy to change > it. > >> Going even further, I am asking myself if something like >> >> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c >> index 60bd081d3ef3..406743566869 100644 >> --- a/hw/s390x/ipl.c >> +++ b/hw/s390x/ipl.c >> @@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type) >> if (reset_type == S390_RESET_MODIFIED_CLEAR || >> reset_type == S390_RESET_LOAD_NORMAL) { >> /* ignore -no-reboot, send no event */ >> - qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET); >> + qemu_system_reset_request(SHUTDOWN_CAUSE_NONE); >> } else { >> qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); >> } >> >> would also work. > > If that works for you, then you could take out the test in the reset > code. You would have to modify qemu_system_reset_request too of course. > > But it seems a bit unsatisfactory to change the reason for the request > so as to influence behaviour. Either the requests should ask for > particular behaviour, or the logic for determining how to handle > the type of request should remain in the reset logic, I would say. I agree. Anyway I think your patch is good as is. Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
On Fri, 19 Jul 2019 09:24:20 +1000 Nicholas Piggin <npiggin@gmail.com> wrote: > Christian Borntraeger's on July 18, 2019 9:27 pm: > > > > > > On 18.07.19 13:06, Paolo Bonzini wrote: > >> On 18/07/19 12:39, Nicholas Piggin wrote: > >>> Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from > >>> S3") changed system wakeup to avoid calling qapi_event_send_reset. > >>> Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to > >>> have inadvertently broken that logic. > >>> > >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >>> --- > >>> I'm not quite sure if this patch is correct and haven't tested it, I > >>> found it by inspection. If this patch is incorrect, I will have to > >>> adjust patch 2. > >>> > >>> vl.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/vl.c b/vl.c > >>> index 5089fce6c5..ef3c7ab8b8 100644 > >>> --- a/vl.c > >>> +++ b/vl.c > >>> @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) > >>> } else { > >>> qemu_devices_reset(); > >>> } > >>> - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { > >>> + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { > >>> qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); > >>> } > >>> cpu_synchronize_all_post_reset(); > >>> > >> > >> Yes, it seems correct and I've queued it for 4.1. FWIW, Acked-by: Cornelia Huck <cohuck@redhat.com> > > > > Yes makes sense. > > Would it be better to write out if(reason) e.g. replace "reason" with "reason != SHUTDOWN_CAUSE_NONE" ? > > I guess it's a matter of preference so I won't weigh in :) My patch > did try to revert back to the previous form but I'm happy to change > it. > > > Going even further, I am asking myself if something like > > > > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c > > index 60bd081d3ef3..406743566869 100644 > > --- a/hw/s390x/ipl.c > > +++ b/hw/s390x/ipl.c > > @@ -577,7 +577,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type) > > if (reset_type == S390_RESET_MODIFIED_CLEAR || > > reset_type == S390_RESET_LOAD_NORMAL) { > > /* ignore -no-reboot, send no event */ > > - qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET); > > + qemu_system_reset_request(SHUTDOWN_CAUSE_NONE); > > } else { > > qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET); > > } > > > > would also work. Doesn't that have side effects in qemu_system_reset_request()? > > If that works for you, then you could take out the test in the reset > code. You would have to modify qemu_system_reset_request too of course. > > But it seems a bit unsatisfactory to change the reason for the request > so as to influence behaviour. Either the requests should ask for > particular behaviour, or the logic for determining how to handle > the type of request should remain in the reset logic, I would say. Agreed.
diff --git a/vl.c b/vl.c index 5089fce6c5..ef3c7ab8b8 100644 --- a/vl.c +++ b/vl.c @@ -1550,7 +1550,7 @@ void qemu_system_reset(ShutdownCause reason) } else { qemu_devices_reset(); } - if (reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { + if (reason && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) { qapi_event_send_reset(shutdown_caused_by_guest(reason), reason); } cpu_synchronize_all_post_reset();
Commit 1405819637f53 ("qmp: don't emit the RESET event on wakeup from S3") changed system wakeup to avoid calling qapi_event_send_reset. Commit 76ed4b18debfe ("s390/ipl: fix ipl with -no-reboot") appears to have inadvertently broken that logic. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- I'm not quite sure if this patch is correct and haven't tested it, I found it by inspection. If this patch is incorrect, I will have to adjust patch 2. vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)