On Fri, 19 Jul 2019 09:24:20 +1000 Nicholas Piggin <npig...@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 <npig...@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 <coh...@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.