1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] Series to implement suspend for ppc/spapr
  4. View patch

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

Nicholas Piggin posted 3 patches 6 years, 6 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, David Gibson <david@gibson.dropbear.id.au>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
There is a newer version of this series
[Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Nicholas Piggin 6 years, 6 months ago 
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();
-- 
2.20.1
Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Paolo Bonzini 6 years, 6 months ago 
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
Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Christian Borntraeger 6 years, 6 months ago 

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.
Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Nicholas Piggin 6 years, 6 months ago 
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
Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Christian Borntraeger 6 years, 6 months ago 

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>
Re: [Qemu-devel] [PATCH 1/3] qmp: don't emit the RESET event on wakeup
Posted by Cornelia Huck 6 years, 6 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.