[Qemu-devel] [PATCH 0/3] Series to implement suspend for ppc/spapr

[Qemu-devel] [PATCH 0/3] Series to implement suspend for ppc/spapr

[Nicholas Piggin]

Any comments on this series would be welcome. Hopefully someone who
knows i386 can give some feedback on the possible bug fix, and
whether the new wakeup method will suit i386.

Thanks,
Nick

Nicholas Piggin (3):
  qmp: don't emit the RESET event on wakeup
  machine: Add wakeup method to MachineClass
  spapr: Implement ibm,suspend-me

 hw/ppc/spapr.c         | 11 +++++++++++
 hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/boards.h    |  1 +
 include/hw/ppc/spapr.h |  3 ++-
 vl.c                   | 31 +++++++++++++++++++++++++++++--
 5 files changed, 75 insertions(+), 3 deletions(-)

-- 
2.20.1

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

[Nicholas Piggin]

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

[Qemu-devel] [PATCH 2/3] machine: Add wakeup method to MachineClass

[Nicholas Piggin]

Waking from suspend is not logically a machine reset on all machines,
particularly in the paravirtualized case rather than hardware
emulated. The ppc spapr machine for example just invokes hypervisor
to suspend, and expects that call to return with the machine in the
same state (modulo some possible migration and reconfiguration
details).

Implement a machine ->wakeup method and use that if it exists.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/boards.h |  1 +
 vl.c                | 29 ++++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index a71d1a53a5..915ac3352b 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -180,6 +180,7 @@ struct MachineClass {
 
     void (*init)(MachineState *state);
     void (*reset)(MachineState *state);
+    void (*wakeup)(MachineState *state);
     void (*hot_add_cpu)(MachineState *state, const int64_t id, Error **errp);
     int (*kvm_type)(MachineState *machine, const char *arg);
     void (*smp_parse)(MachineState *ms, QemuOpts *opts);
diff --git a/vl.c b/vl.c
index ef3c7ab8b8..b9e9943458 100644
--- a/vl.c
+++ b/vl.c
@@ -1556,6 +1556,33 @@ void qemu_system_reset(ShutdownCause reason)
     cpu_synchronize_all_post_reset();
 }
 
+/*
+ * Wake the VM after suspend.
+ */
+static void qemu_system_wakeup(void)
+{
+    MachineClass *mc;
+
+    mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;
+
+    if (mc && mc->wakeup) {
+        mc->wakeup(current_machine);
+    } else {
+        /*
+         * Fall back to old reset wakeup method. Platforms supporting
+         * wakeup should be converted over to use ->wakeup and this
+         * fallback code removed.
+         */
+        cpu_synchronize_all_states();
+        if (mc && mc->reset) {
+            mc->reset(current_machine);
+        } else {
+            qemu_devices_reset();
+        }
+        cpu_synchronize_all_post_reset();
+    }
+}
+
 void qemu_system_guest_panicked(GuestPanicInformation *info)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed");
@@ -1764,7 +1791,7 @@ static bool main_loop_should_exit(void)
     }
     if (qemu_wakeup_requested()) {
         pause_all_vcpus();
-        qemu_system_reset(SHUTDOWN_CAUSE_NONE);
+        qemu_system_wakeup();
         notifier_list_notify(&wakeup_notifiers, &wakeup_reason);
         wakeup_reason = QEMU_WAKEUP_REASON_NONE;
         resume_all_vcpus();
-- 
2.20.1

[Qemu-devel] [PATCH 3/3] spapr: Implement ibm,suspend-me

[Nicholas Piggin]

This has been useful to modify and test the Linux pseries suspend
code but it requires modification to the guest to call it (due to
being gated by other unimplemented features). It is not otherwise
used by Linux yet, but work is slowly progressing there.

This allows a (lightly modified) guest kernel to suspend with
`echo mem > /sys/power/state` and be resumed with system_wakeup
monitor command.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c         | 11 +++++++++++
 hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  3 ++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 00f7735a31..c7725d3586 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1701,6 +1701,14 @@ static int spapr_reset_drcs(Object *child, void *opaque)
     return 0;
 }
 
+static void spapr_machine_wakeup(MachineState *machine)
+{
+    /*
+     * Nothing needs to be done to resume a suspended guest because
+     * suspending does not change the machine state.
+     */
+}
+
 static void spapr_machine_reset(MachineState *machine)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(machine);
@@ -3078,6 +3086,8 @@ static void spapr_machine_init(MachineState *machine)
 
     qemu_register_boot_set(spapr_boot_set, spapr);
 
+    qemu_register_wakeup_support();
+
     if (kvm_enabled()) {
         /* to stop and start vmclock */
         qemu_add_vm_change_state_handler(cpu_ppc_clock_vm_state_change,
@@ -4373,6 +4383,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
      */
     mc->init = spapr_machine_init;
     mc->reset = spapr_machine_reset;
+    mc->wakeup = spapr_machine_wakeup;
     mc->block_default_type = IF_SCSI;
     mc->max_cpus = 1024;
     mc->no_parallel = 1;
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index a618a2ac0f..87175c1e0a 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -216,6 +216,36 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
     qemu_cpu_kick(cs);
 }
 
+static void rtas_ibm_suspend_me(PowerPCCPU *cpu, SpaprMachineState *spapr,
+                           uint32_t token, uint32_t nargs,
+                           target_ulong args,
+                           uint32_t nret, target_ulong rets)
+{
+    CPUState *cs;
+
+    if (nargs != 0 || nret != 1) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+        return;
+    }
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *c = POWERPC_CPU(cs);
+        CPUPPCState *e = &c->env;
+        if (c == cpu) {
+            continue;
+        }
+
+        /* See h_join */
+        if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
+            rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
+            return;
+        }
+    }
+
+    qemu_system_suspend_request();
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+}
+
 static inline int sysparm_st(target_ulong addr, target_ulong len,
                              const void *val, uint16_t vallen)
 {
@@ -483,6 +513,8 @@ static void core_rtas_register_types(void)
                         rtas_query_cpu_stopped_state);
     spapr_rtas_register(RTAS_START_CPU, "start-cpu", rtas_start_cpu);
     spapr_rtas_register(RTAS_STOP_SELF, "stop-self", rtas_stop_self);
+    spapr_rtas_register(RTAS_IBM_SUSPEND_ME, "ibm,suspend-me",
+                        rtas_ibm_suspend_me);
     spapr_rtas_register(RTAS_IBM_GET_SYSTEM_PARAMETER,
                         "ibm,get-system-parameter",
                         rtas_ibm_get_system_parameter);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5d36eec9d0..6e8e18b077 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -631,8 +631,9 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 #define RTAS_IBM_CREATE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x27)
 #define RTAS_IBM_REMOVE_PE_DMA_WINDOW           (RTAS_TOKEN_BASE + 0x28)
 #define RTAS_IBM_RESET_PE_DMA_WINDOW            (RTAS_TOKEN_BASE + 0x29)
+#define RTAS_IBM_SUSPEND_ME                     (RTAS_TOKEN_BASE + 0x2A)
 
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2A)
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x2B)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
-- 
2.20.1

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

[Paolo Bonzini]

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 0/3] Series to implement suspend for ppc/spapr

[Paolo Bonzini]

On 18/07/19 12:39, Nicholas Piggin wrote:
> Any comments on this series would be welcome. Hopefully someone who
> knows i386 can give some feedback on the possible bug fix, and
> whether the new wakeup method will suit i386.

Looks good, though only i386 supports wakeup so perhaps it's better to
DTRT and move the reset to the PC machine's wakeup method.  Then pseries
need not implement mc->wakeup at all.

Paolo

> Thanks,
> Nick
> 
> Nicholas Piggin (3):
>   qmp: don't emit the RESET event on wakeup
>   machine: Add wakeup method to MachineClass
>   spapr: Implement ibm,suspend-me
> 
>  hw/ppc/spapr.c         | 11 +++++++++++
>  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
>  include/hw/boards.h    |  1 +
>  include/hw/ppc/spapr.h |  3 ++-
>  vl.c                   | 31 +++++++++++++++++++++++++++++--
>  5 files changed, 75 insertions(+), 3 deletions(-)
> 

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

[Christian Borntraeger]

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

[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 <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] [PATCH 0/3] Series to implement suspend for ppc/spapr

[Nicholas Piggin]

Paolo Bonzini's on July 18, 2019 9:08 pm:
> On 18/07/19 12:39, Nicholas Piggin wrote:
>> Any comments on this series would be welcome. Hopefully someone who
>> knows i386 can give some feedback on the possible bug fix, and
>> whether the new wakeup method will suit i386.
> 
> Looks good, though only i386 supports wakeup so perhaps it's better to
> DTRT and move the reset to the PC machine's wakeup method.  Then pseries
> need not implement mc->wakeup at all.

Yeah that probably makes more sense because the i386 patch should be 
quite trivial. I will try that and re-send.

Thanks,
Nick

Re: [Qemu-devel] [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 <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

[Cornelia Huck]

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.