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

[Nicholas Piggin]

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 <address@hidden>
>>> ---
>>> 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