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

[Christian Borntraeger]

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 <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.

I agree.

Anyway I think your patch is good as is.

Acked-by: Christian Borntraeger <address@hidden>