Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup

[Cornelia Huck <cohuck@redhat.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.