| Message ID | 20170428021317.24711-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Eric Blake (eblake@redhat.com) wrote: > We want to track why a guest was shutdown; in particular, being able > to tell the difference between a guest request (such as ACPI request) > and host request (such as SIGINT) will prove useful to libvirt. > Since all requests eventually end up changing shutdown_requested in > vl.c, the logical change is to make that value track the reason, > rather than its current 0/1 contents. > > Since command-line options control whether a reset request is turned > into a shutdown request instead, the same treatment is given to > reset_requested. > > This patch adds a QAPI enum ShutdownCause that describes reasons > that a shutdown can be requested, and changes qemu_system_reset() to > pass the reason through, although for now it is not reported. The > next patch will actually wire things up to modify events to report > data based on the reason, and to pass the correct enum value in from > various call-sites that can trigger a reset/shutdown. Since QAPI > generates enums starting at 0, it's easier if we use a different > number as our sentinel that no request has happened yet. Most of > the changes are in vl.c, but xen was using things externally. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution > v3: new patch > --- > qapi-schema.json | 23 +++++++++++++++++++++++ > include/sysemu/sysemu.h | 2 +- > vl.c | 44 ++++++++++++++++++++++++++++---------------- > hw/i386/xen/xen-hvm.c | 9 ++++++--- > migration/colo.c | 2 +- > migration/savevm.c | 2 +- > 6 files changed, 60 insertions(+), 22 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 01b087f..a4ebdd1 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2304,6 +2304,29 @@ > { 'command': 'system_powerdown' } > > ## > +# @ShutdownCause: > +# > +# Enumeration of various causes for shutdown. > +# > +# @host-qmp: Reaction to a QMP command, such as 'quit' > +# @host-signal: Reaction to a signal, such as SIGINT > +# @host-ui: Reaction to a UI event, such as closing the window > +# @host-replay: The host is replaying an earlier shutdown event > +# @host-error: Qemu encountered an error that prevents further use of the guest > +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or > +# other hardware-specific action > +# @guest-reset: The guest requested a reset, and the command line > +# response to a reset is to instead trigger a shutdown > +# @guest-panic: The guest panicked, and the command line response to > +# a panic is to trigger a shutdown It's a little coarse grained; is there anyway to pass platform specific information for debug? I ask because I spent a while debugging a few bugs with unexpected resets and had to figure out which of x86's many reset causes triggered it. Dave > +# Since: 2.10 > +## > +{ 'enum': 'ShutdownCause', > + 'data': [ 'host-qmp', 'host-signal', 'host-ui', 'host-replay', 'host-error', > + 'guest-shutdown', 'guest-reset', 'guest-panic' ] } > + > +## > # @cpu: > # > # This command is a nop that is only provided for the purposes of compatibility. > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 16175f7..00a907f 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -65,7 +65,7 @@ bool qemu_vmstop_requested(RunState *r); > int qemu_shutdown_requested_get(void); > int qemu_reset_requested_get(void); > void qemu_system_killed(int signal, pid_t pid); > -void qemu_system_reset(bool report); > +void qemu_system_reset(bool report, int reason); > void qemu_system_guest_panicked(GuestPanicInformation *info); > size_t qemu_target_page_size(void); > > diff --git a/vl.c b/vl.c > index 879786a..2b95b7f 100644 > --- a/vl.c > +++ b/vl.c > @@ -1597,8 +1597,8 @@ void vm_state_notify(int running, RunState state) > } > } > > -static int reset_requested; > -static int shutdown_requested, shutdown_signal; > +static int reset_requested = -1; > +static int shutdown_requested = -1, shutdown_signal; > static pid_t shutdown_pid; > static int powerdown_requested; > static int debug_requested; > @@ -1624,7 +1624,7 @@ int qemu_reset_requested_get(void) > > static int qemu_shutdown_requested(void) > { > - return atomic_xchg(&shutdown_requested, 0); > + return atomic_xchg(&shutdown_requested, -1); > } > > static void qemu_kill_report(void) > @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) > static int qemu_reset_requested(void) > { > int r = reset_requested; > - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { > - reset_requested = 0; > + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { > + reset_requested = -1; > return r; > } > - return false; > + return -1; > } > > static int qemu_suspend_requested(void) > @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void) > return r; > } > > -void qemu_system_reset(bool report) > +/* > + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using > + * the @reason interpreted as ShutdownType for details. Otherwise, > + * @report is VMRESET_SILENT and @reason is ignored. > + */ > +void qemu_system_reset(bool report, int reason) > { > MachineClass *mc; > > @@ -1700,6 +1705,7 @@ void qemu_system_reset(bool report) > qemu_devices_reset(); > } > if (report) { > + assert(reason >= 0); > qapi_event_send_reset(&error_abort); > } > cpu_synchronize_all_post_reset(); > @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) > void qemu_system_reset_request(void) > { > if (no_reboot) { > - shutdown_requested = 1; > + /* FIXME - add a parameter to allow callers to specify reason */ > + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; > } else { > - reset_requested = 1; > + reset_requested = SHUTDOWN_CAUSE_GUEST_RESET; > } > cpu_stop_current(); > qemu_notify_event(); > @@ -1807,7 +1814,7 @@ void qemu_system_killed(int signal, pid_t pid) > /* Cannot call qemu_system_shutdown_request directly because > * we are in a signal handler. > */ > - shutdown_requested = 1; > + shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL; > qemu_notify_event(); > } > > @@ -1815,7 +1822,8 @@ void qemu_system_shutdown_request(void) > { > trace_qemu_system_shutdown_request(); > replay_shutdown_request(); > - shutdown_requested = 1; > + /* FIXME - add a parameter to allow callers to specify reason */ > + shutdown_requested = SHUTDOWN_CAUSE_GUEST_SHUTDOWN; > qemu_notify_event(); > } > > @@ -1846,13 +1854,16 @@ void qemu_system_debug_request(void) > static bool main_loop_should_exit(void) > { > RunState r; > + int request; > + > if (qemu_debug_requested()) { > vm_stop(RUN_STATE_DEBUG); > } > if (qemu_suspend_requested()) { > qemu_system_suspend(); > } > - if (qemu_shutdown_requested()) { > + request = qemu_shutdown_requested(); > + if (request >= 0) { > qemu_kill_report(); > qapi_event_send_shutdown(&error_abort); > if (no_shutdown) { > @@ -1861,9 +1872,10 @@ static bool main_loop_should_exit(void) > return true; > } > } > - if (qemu_reset_requested()) { > + request = qemu_reset_requested(); > + if (request >= 0) { > pause_all_vcpus(); > - qemu_system_reset(VMRESET_REPORT); > + qemu_system_reset(VMRESET_REPORT, request); > resume_all_vcpus(); > if (!runstate_check(RUN_STATE_RUNNING) && > !runstate_check(RUN_STATE_INMIGRATE)) { > @@ -1872,7 +1884,7 @@ static bool main_loop_should_exit(void) > } > if (qemu_wakeup_requested()) { > pause_all_vcpus(); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > notifier_list_notify(&wakeup_notifiers, &wakeup_reason); > wakeup_reason = QEMU_WAKEUP_REASON_NONE; > resume_all_vcpus(); > @@ -4684,7 +4696,7 @@ int main(int argc, char **argv, char **envp) > reading from the other reads, because timer polling functions query > clock values from the log. */ > replay_checkpoint(CHECKPOINT_RESET); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > register_global_state(); > if (replay_mode != REPLAY_MODE_NONE) { > replay_vmstate_init(); > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c > index b1c05ff..3a6484c 100644 > --- a/hw/i386/xen/xen-hvm.c > +++ b/hw/i386/xen/xen-hvm.c > @@ -1089,11 +1089,14 @@ static void cpu_handle_ioreq(void *opaque) > * causes Xen to powerdown the domain. > */ > if (runstate_is_running()) { > - if (qemu_shutdown_requested_get()) { > + int request; > + > + if (qemu_shutdown_requested_get() >= 0) { > destroy_hvm_domain(false); > } > - if (qemu_reset_requested_get()) { > - qemu_system_reset(VMRESET_REPORT); > + request = qemu_reset_requested_get(); > + if (request >= 0) { > + qemu_system_reset(VMRESET_REPORT, request); > destroy_hvm_domain(true); > } > } > diff --git a/migration/colo.c b/migration/colo.c > index c19eb3f..17a5482 100644 > --- a/migration/colo.c > +++ b/migration/colo.c > @@ -611,7 +611,7 @@ void *colo_process_incoming_thread(void *opaque) > } > > qemu_mutex_lock_iothread(); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > vmstate_loading = true; > if (qemu_loadvm_state(fb) < 0) { > error_report("COLO: loadvm failed"); > diff --git a/migration/savevm.c b/migration/savevm.c > index 03ae1bd..dcbaf00 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2292,7 +2292,7 @@ int load_vmstate(const char *name) > return -EINVAL; > } > > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > mis->from_src_file = f; > > aio_context_acquire(aio_context); > -- > 2.9.3 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/28/2017 03:08 AM, Dr. David Alan Gilbert wrote: > * Eric Blake (eblake@redhat.com) wrote: >> We want to track why a guest was shutdown; in particular, being able >> to tell the difference between a guest request (such as ACPI request) >> and host request (such as SIGINT) will prove useful to libvirt. >> Since all requests eventually end up changing shutdown_requested in >> vl.c, the logical change is to make that value track the reason, >> rather than its current 0/1 contents. >> >> ## >> +# @ShutdownCause: >> +# >> +# Enumeration of various causes for shutdown. >> +# >> +# @host-qmp: Reaction to a QMP command, such as 'quit' >> +# @host-signal: Reaction to a signal, such as SIGINT >> +# @host-ui: Reaction to a UI event, such as closing the window >> +# @host-replay: The host is replaying an earlier shutdown event >> +# @host-error: Qemu encountered an error that prevents further use of the guest >> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or >> +# other hardware-specific action >> +# @guest-reset: The guest requested a reset, and the command line >> +# response to a reset is to instead trigger a shutdown >> +# @guest-panic: The guest panicked, and the command line response to >> +# a panic is to trigger a shutdown > > It's a little coarse grained; is there anyway to pass platform specific information > for debug? I ask because I spent a while debugging a few bugs with unexpected > resets and had to figure out which of x86's many reset causes triggered it. I'm open to any followup patches that add further enum values and adjusts the various callers (patch 3 shows how MANY callers use qemu_system_shutdown_request). But I don't think it's necessarily in scope for this series - remember, my goal here was merely to distinguish between host- and guest-triggered resets (which libvirt and higher management tasks want to know), rather than which of multiple reset paths was taken (I agree that it is useful during a qemu debug session - but that's a different audience). I also don't consider myself an expert in the many ways that x86 can reset - it was easy to blindly rewrite qemu_system_shutdown_request() into qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN) based solely on directory, but it would be harder to distinguish which of the multiple files should have which finer-grained cause.
Eric Blake <eblake@redhat.com> writes: > We want to track why a guest was shutdown; in particular, being able > to tell the difference between a guest request (such as ACPI request) > and host request (such as SIGINT) will prove useful to libvirt. > Since all requests eventually end up changing shutdown_requested in > vl.c, the logical change is to make that value track the reason, > rather than its current 0/1 contents. > > Since command-line options control whether a reset request is turned > into a shutdown request instead, the same treatment is given to > reset_requested. > > This patch adds a QAPI enum ShutdownCause that describes reasons > that a shutdown can be requested, and changes qemu_system_reset() to > pass the reason through, although for now it is not reported. The > next patch will actually wire things up to modify events to report > data based on the reason, and to pass the correct enum value in from > various call-sites that can trigger a reset/shutdown. Since QAPI > generates enums starting at 0, it's easier if we use a different > number as our sentinel that no request has happened yet. Most of > the changes are in vl.c, but xen was using things externally. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution > v3: new patch > --- > qapi-schema.json | 23 +++++++++++++++++++++++ > include/sysemu/sysemu.h | 2 +- > vl.c | 44 ++++++++++++++++++++++++++++---------------- > hw/i386/xen/xen-hvm.c | 9 ++++++--- > migration/colo.c | 2 +- > migration/savevm.c | 2 +- > 6 files changed, 60 insertions(+), 22 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 01b087f..a4ebdd1 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2304,6 +2304,29 @@ > { 'command': 'system_powerdown' } > > ## > +# @ShutdownCause: > +# > +# Enumeration of various causes for shutdown. > +# > +# @host-qmp: Reaction to a QMP command, such as 'quit' > +# @host-signal: Reaction to a signal, such as SIGINT > +# @host-ui: Reaction to a UI event, such as closing the window > +# @host-replay: The host is replaying an earlier shutdown event > +# @host-error: Qemu encountered an error that prevents further use of the guest > +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or > +# other hardware-specific action > +# @guest-reset: The guest requested a reset, and the command line > +# response to a reset is to instead trigger a shutdown > +# @guest-panic: The guest panicked, and the command line response to > +# a panic is to trigger a shutdown > +# > +# Since: 2.10 > +## > +{ 'enum': 'ShutdownCause', > + 'data': [ 'host-qmp', 'host-signal', 'host-ui', 'host-replay', 'host-error', > + 'guest-shutdown', 'guest-reset', 'guest-panic' ] } > + > +## > # @cpu: > # > # This command is a nop that is only provided for the purposes of compatibility. > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h > index 16175f7..00a907f 100644 > --- a/include/sysemu/sysemu.h > +++ b/include/sysemu/sysemu.h > @@ -65,7 +65,7 @@ bool qemu_vmstop_requested(RunState *r); > int qemu_shutdown_requested_get(void); > int qemu_reset_requested_get(void); > void qemu_system_killed(int signal, pid_t pid); > -void qemu_system_reset(bool report); > +void qemu_system_reset(bool report, int reason); > void qemu_system_guest_panicked(GuestPanicInformation *info); > size_t qemu_target_page_size(void); > > diff --git a/vl.c b/vl.c > index 879786a..2b95b7f 100644 > --- a/vl.c > +++ b/vl.c > @@ -1597,8 +1597,8 @@ void vm_state_notify(int running, RunState state) > } > } > > -static int reset_requested; > -static int shutdown_requested, shutdown_signal; > +static int reset_requested = -1; > +static int shutdown_requested = -1, shutdown_signal; Peeking ahead, I see that shutdown_requested and reset_requested take ShutdownCause values and -1. The latter means "no shutdown requested". What about adding 'none' to ShutdownCause, with value 0, und use that instead of literal -1? Would avoid the unusual "negative means false, non-negative means true". PATCH 4 exposes ShutdownCause in event SHUTDOWN, and 'none' must not occur there. However, if we ever add a query-shutdown to go with this event, we will need 'none' there. I'd be tempted to reshuffle declarations here, because shutdown_signal's int is a different one than reset_requested's and shutdown_requested, and the latter two's "negative means false, non-negative means true" is unusual enough to justify a comment. > static pid_t shutdown_pid; > static int powerdown_requested; > static int debug_requested; > @@ -1624,7 +1624,7 @@ int qemu_reset_requested_get(void) > > static int qemu_shutdown_requested(void) > { > - return atomic_xchg(&shutdown_requested, 0); > + return atomic_xchg(&shutdown_requested, -1); > } Hmm. In case we stick to literal -1: consider splitting this patch into a part that changes @shutdown_requested from zero/non-zero to negative/non-negative, and a part that uses ShutdownCause for the non-negative values. > > static void qemu_kill_report(void) > @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) > static int qemu_reset_requested(void) > { > int r = reset_requested; > - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { > - reset_requested = 0; > + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { > + reset_requested = -1; > return r; > } > - return false; > + return -1; "return false" in a function returning int smells, good riddance. > } > > static int qemu_suspend_requested(void) > @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void) > return r; > } > > -void qemu_system_reset(bool report) > +/* > + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using > + * the @reason interpreted as ShutdownType for details. Otherwise, > + * @report is VMRESET_SILENT and @reason is ignored. > + */ > +void qemu_system_reset(bool report, int reason) Why int reason and not ShutdownCause? Hmm, peeking ahead, I see you pass -1 with VMRESET_SILENT. Yet another place where you use int for type ShutdownCause + { -1 }. Adding 'none' to ShutdownCause looks even more attractive to me now. > { > MachineClass *mc; > > @@ -1700,6 +1705,7 @@ void qemu_system_reset(bool report) > qemu_devices_reset(); > } > if (report) { > + assert(reason >= 0); > qapi_event_send_reset(&error_abort); > } > cpu_synchronize_all_post_reset(); > @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) > void qemu_system_reset_request(void) > { > if (no_reboot) { > - shutdown_requested = 1; > + /* FIXME - add a parameter to allow callers to specify reason */ FIXME addressed in the next patch. Mention in this one's commit message? > + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; > } else { > - reset_requested = 1; > + reset_requested = SHUTDOWN_CAUSE_GUEST_RESET; > } > cpu_stop_current(); > qemu_notify_event(); > @@ -1807,7 +1814,7 @@ void qemu_system_killed(int signal, pid_t pid) > /* Cannot call qemu_system_shutdown_request directly because > * we are in a signal handler. > */ > - shutdown_requested = 1; > + shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL; > qemu_notify_event(); > } > > @@ -1815,7 +1822,8 @@ void qemu_system_shutdown_request(void) > { > trace_qemu_system_shutdown_request(); > replay_shutdown_request(); > - shutdown_requested = 1; > + /* FIXME - add a parameter to allow callers to specify reason */ Likewise. > + shutdown_requested = SHUTDOWN_CAUSE_GUEST_SHUTDOWN; > qemu_notify_event(); > } > > @@ -1846,13 +1854,16 @@ void qemu_system_debug_request(void) > static bool main_loop_should_exit(void) > { > RunState r; > + int request; > + > if (qemu_debug_requested()) { > vm_stop(RUN_STATE_DEBUG); > } > if (qemu_suspend_requested()) { > qemu_system_suspend(); > } > - if (qemu_shutdown_requested()) { > + request = qemu_shutdown_requested(); > + if (request >= 0) { > qemu_kill_report(); > qapi_event_send_shutdown(&error_abort); > if (no_shutdown) { > @@ -1861,9 +1872,10 @@ static bool main_loop_should_exit(void) > return true; > } > } > - if (qemu_reset_requested()) { > + request = qemu_reset_requested(); > + if (request >= 0) { > pause_all_vcpus(); > - qemu_system_reset(VMRESET_REPORT); > + qemu_system_reset(VMRESET_REPORT, request); > resume_all_vcpus(); > if (!runstate_check(RUN_STATE_RUNNING) && > !runstate_check(RUN_STATE_INMIGRATE)) { > @@ -1872,7 +1884,7 @@ static bool main_loop_should_exit(void) > } > if (qemu_wakeup_requested()) { > pause_all_vcpus(); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > notifier_list_notify(&wakeup_notifiers, &wakeup_reason); > wakeup_reason = QEMU_WAKEUP_REASON_NONE; > resume_all_vcpus(); > @@ -4684,7 +4696,7 @@ int main(int argc, char **argv, char **envp) > reading from the other reads, because timer polling functions query > clock values from the log. */ > replay_checkpoint(CHECKPOINT_RESET); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > register_global_state(); > if (replay_mode != REPLAY_MODE_NONE) { > replay_vmstate_init(); > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c > index b1c05ff..3a6484c 100644 > --- a/hw/i386/xen/xen-hvm.c > +++ b/hw/i386/xen/xen-hvm.c > @@ -1089,11 +1089,14 @@ static void cpu_handle_ioreq(void *opaque) > * causes Xen to powerdown the domain. > */ > if (runstate_is_running()) { > - if (qemu_shutdown_requested_get()) { > + int request; > + > + if (qemu_shutdown_requested_get() >= 0) { > destroy_hvm_domain(false); > } > - if (qemu_reset_requested_get()) { > - qemu_system_reset(VMRESET_REPORT); > + request = qemu_reset_requested_get(); > + if (request >= 0) { > + qemu_system_reset(VMRESET_REPORT, request); > destroy_hvm_domain(true); > } > } > diff --git a/migration/colo.c b/migration/colo.c > index c19eb3f..17a5482 100644 > --- a/migration/colo.c > +++ b/migration/colo.c > @@ -611,7 +611,7 @@ void *colo_process_incoming_thread(void *opaque) > } > > qemu_mutex_lock_iothread(); > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > vmstate_loading = true; > if (qemu_loadvm_state(fb) < 0) { > error_report("COLO: loadvm failed"); > diff --git a/migration/savevm.c b/migration/savevm.c > index 03ae1bd..dcbaf00 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2292,7 +2292,7 @@ int load_vmstate(const char *name) > return -EINVAL; > } > > - qemu_system_reset(VMRESET_SILENT); > + qemu_system_reset(VMRESET_SILENT, -1); > mis->from_src_file = f; > > aio_context_acquire(aio_context);
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Eric Blake (eblake@redhat.com) wrote: >> We want to track why a guest was shutdown; in particular, being able >> to tell the difference between a guest request (such as ACPI request) >> and host request (such as SIGINT) will prove useful to libvirt. >> Since all requests eventually end up changing shutdown_requested in >> vl.c, the logical change is to make that value track the reason, >> rather than its current 0/1 contents. >> >> Since command-line options control whether a reset request is turned >> into a shutdown request instead, the same treatment is given to >> reset_requested. >> >> This patch adds a QAPI enum ShutdownCause that describes reasons >> that a shutdown can be requested, and changes qemu_system_reset() to >> pass the reason through, although for now it is not reported. The >> next patch will actually wire things up to modify events to report >> data based on the reason, and to pass the correct enum value in from >> various call-sites that can trigger a reset/shutdown. Since QAPI >> generates enums starting at 0, it's easier if we use a different >> number as our sentinel that no request has happened yet. Most of >> the changes are in vl.c, but xen was using things externally. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution >> v3: new patch >> --- >> qapi-schema.json | 23 +++++++++++++++++++++++ >> include/sysemu/sysemu.h | 2 +- >> vl.c | 44 ++++++++++++++++++++++++++++---------------- >> hw/i386/xen/xen-hvm.c | 9 ++++++--- >> migration/colo.c | 2 +- >> migration/savevm.c | 2 +- >> 6 files changed, 60 insertions(+), 22 deletions(-) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 01b087f..a4ebdd1 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -2304,6 +2304,29 @@ >> { 'command': 'system_powerdown' } >> >> ## >> +# @ShutdownCause: >> +# >> +# Enumeration of various causes for shutdown. >> +# >> +# @host-qmp: Reaction to a QMP command, such as 'quit' >> +# @host-signal: Reaction to a signal, such as SIGINT >> +# @host-ui: Reaction to a UI event, such as closing the window >> +# @host-replay: The host is replaying an earlier shutdown event >> +# @host-error: Qemu encountered an error that prevents further use of the guest >> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or >> +# other hardware-specific action >> +# @guest-reset: The guest requested a reset, and the command line >> +# response to a reset is to instead trigger a shutdown >> +# @guest-panic: The guest panicked, and the command line response to >> +# a panic is to trigger a shutdown > > It's a little coarse grained; is there anyway to pass platform specific information > for debug? I ask because I spent a while debugging a few bugs with unexpected > resets and had to figure out which of x86's many reset causes triggered it. Asking for more help with debugging is fair, but I think the need is better served by tracepoints than by exposing even more detail in QMP, where compatibility promises apply.
* Eric Blake (eblake@redhat.com) wrote: > On 04/28/2017 03:08 AM, Dr. David Alan Gilbert wrote: > > * Eric Blake (eblake@redhat.com) wrote: > >> We want to track why a guest was shutdown; in particular, being able > >> to tell the difference between a guest request (such as ACPI request) > >> and host request (such as SIGINT) will prove useful to libvirt. > >> Since all requests eventually end up changing shutdown_requested in > >> vl.c, the logical change is to make that value track the reason, > >> rather than its current 0/1 contents. > >> > > >> ## > >> +# @ShutdownCause: > >> +# > >> +# Enumeration of various causes for shutdown. > >> +# > >> +# @host-qmp: Reaction to a QMP command, such as 'quit' > >> +# @host-signal: Reaction to a signal, such as SIGINT > >> +# @host-ui: Reaction to a UI event, such as closing the window > >> +# @host-replay: The host is replaying an earlier shutdown event > >> +# @host-error: Qemu encountered an error that prevents further use of the guest > >> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or > >> +# other hardware-specific action > >> +# @guest-reset: The guest requested a reset, and the command line > >> +# response to a reset is to instead trigger a shutdown > >> +# @guest-panic: The guest panicked, and the command line response to > >> +# a panic is to trigger a shutdown > > > > It's a little coarse grained; is there anyway to pass platform specific information > > for debug? I ask because I spent a while debugging a few bugs with unexpected > > resets and had to figure out which of x86's many reset causes triggered it. > > I'm open to any followup patches that add further enum values and > adjusts the various callers (patch 3 shows how MANY callers use > qemu_system_shutdown_request). But I don't think it's necessarily in > scope for this series - remember, my goal here was merely to distinguish > between host- and guest-triggered resets (which libvirt and higher > management tasks want to know) Yep, that's fine. > rather than which of multiple reset > paths was taken (I agree that it is useful during a qemu debug session - > but that's a different audience). I also don't consider myself an > expert in the many ways that x86 can reset - it was easy to blindly > rewrite qemu_system_shutdown_request() into > qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN) based solely > on directory, but it would be harder to distinguish which of the > multiple files should have which finer-grained cause. Yes, I'm also not an expert on x86 resets - but when I was debugging I just added a tag in every place it called the reset code. At a higher level, using your tags, I'm not sure where a reset triggered by a fault detected by the hypervisor lives - e.g. an x86 triple fault where the guest screws up so badly that it just gets reset. Is that a guest-reset or a guest-panic or what - neither case was actually asked for by the guest itself. Dave > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/28/2017 10:27 AM, Dr. David Alan Gilbert wrote: >>>> +# Enumeration of various causes for shutdown. >>>> +# >>>> +# @host-qmp: Reaction to a QMP command, such as 'quit' >>>> +# @host-signal: Reaction to a signal, such as SIGINT >>>> +# @host-ui: Reaction to a UI event, such as closing the window >>>> +# @host-replay: The host is replaying an earlier shutdown event >>>> +# @host-error: Qemu encountered an error that prevents further use of the guest >>>> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or >>>> +# other hardware-specific action >>>> +# @guest-reset: The guest requested a reset, and the command line >>>> +# response to a reset is to instead trigger a shutdown >>>> +# @guest-panic: The guest panicked, and the command line response to >>>> +# a panic is to trigger a shutdown >>> > At a higher level, using your tags, I'm not sure where a reset triggered > by a fault detected by the hypervisor lives - e.g. an x86 triple fault > where the guest screws up so badly that it just gets reset. Is > that a guest-reset or a guest-panic or what - neither case > was actually asked for by the guest itself. Wouldn't that be host-error (qemu detected an error that prevents further execution of the guest without a reset - and a triple fault seems to fall into the category of the guest getting itself wedged rather than actually trying to reset)? Except patch 3 only used SHUTDOWN_TYPE_HOST_ERROR in the xen portion of the patch. So if any x86 expert has an opinion on where triple-fault handling is emulated, and what category should be used there, I'm welcome to tweaking this series.
* Eric Blake (eblake@redhat.com) wrote: > On 04/28/2017 10:27 AM, Dr. David Alan Gilbert wrote: > > >>>> +# Enumeration of various causes for shutdown. > >>>> +# > >>>> +# @host-qmp: Reaction to a QMP command, such as 'quit' > >>>> +# @host-signal: Reaction to a signal, such as SIGINT > >>>> +# @host-ui: Reaction to a UI event, such as closing the window > >>>> +# @host-replay: The host is replaying an earlier shutdown event > >>>> +# @host-error: Qemu encountered an error that prevents further use of the guest > >>>> +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or > >>>> +# other hardware-specific action > >>>> +# @guest-reset: The guest requested a reset, and the command line > >>>> +# response to a reset is to instead trigger a shutdown > >>>> +# @guest-panic: The guest panicked, and the command line response to > >>>> +# a panic is to trigger a shutdown > >>> > > > At a higher level, using your tags, I'm not sure where a reset triggered > > by a fault detected by the hypervisor lives - e.g. an x86 triple fault > > where the guest screws up so badly that it just gets reset. Is > > that a guest-reset or a guest-panic or what - neither case > > was actually asked for by the guest itself. > > Wouldn't that be host-error (qemu detected an error that prevents > further execution of the guest without a reset - and a triple fault > seems to fall into the category of the guest getting itself wedged > rather than actually trying to reset)? Except patch 3 only used > SHUTDOWN_TYPE_HOST_ERROR in the xen portion of the patch. > > So if any x86 expert has an opinion on where triple-fault handling is > emulated, and what category should be used there, I'm welcome to > tweaking this series. It's pretty much on the border anyway, I don't think it matters too much; it sounds perfectly reasonable. Dave > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/28/2017 11:09 AM, Dr. David Alan Gilbert wrote: >>> At a higher level, using your tags, I'm not sure where a reset triggered >>> by a fault detected by the hypervisor lives - e.g. an x86 triple fault >>> where the guest screws up so badly that it just gets reset. Is >>> that a guest-reset or a guest-panic or what - neither case >>> was actually asked for by the guest itself. >> >> Wouldn't that be host-error (qemu detected an error that prevents >> further execution of the guest without a reset - and a triple fault >> seems to fall into the category of the guest getting itself wedged >> rather than actually trying to reset)? Except patch 3 only used >> SHUTDOWN_TYPE_HOST_ERROR in the xen portion of the patch. >> >> So if any x86 expert has an opinion on where triple-fault handling is >> emulated, and what category should be used there, I'm welcome to >> tweaking this series. > > It's pretty much on the border anyway, I don't think it matters too > much; it sounds perfectly reasonable. Actually, reading https://blogs.msdn.microsoft.com/larryosterman/2005/02/08/faster-syscall-trap-redux/ makes it sound like the triple-fault = reset is exploited by existing OS (dating back to days of targetting 286 machines), so it is bare-metal behavior that we have to faithfully emulate as a guest-triggered reset, and not something where the guest has wedged itself to the point where qemu can no longer execute the guest.
* Eric Blake (eblake@redhat.com) wrote: > On 04/28/2017 11:09 AM, Dr. David Alan Gilbert wrote: > > >>> At a higher level, using your tags, I'm not sure where a reset triggered > >>> by a fault detected by the hypervisor lives - e.g. an x86 triple fault > >>> where the guest screws up so badly that it just gets reset. Is > >>> that a guest-reset or a guest-panic or what - neither case > >>> was actually asked for by the guest itself. > >> > >> Wouldn't that be host-error (qemu detected an error that prevents > >> further execution of the guest without a reset - and a triple fault > >> seems to fall into the category of the guest getting itself wedged > >> rather than actually trying to reset)? Except patch 3 only used > >> SHUTDOWN_TYPE_HOST_ERROR in the xen portion of the patch. > >> > >> So if any x86 expert has an opinion on where triple-fault handling is > >> emulated, and what category should be used there, I'm welcome to > >> tweaking this series. > > > > It's pretty much on the border anyway, I don't think it matters too > > much; it sounds perfectly reasonable. > > Actually, reading > https://blogs.msdn.microsoft.com/larryosterman/2005/02/08/faster-syscall-trap-redux/ > makes it sound like the triple-fault = reset is exploited by existing OS > (dating back to days of targetting 286 machines), so it is bare-metal > behavior that we have to faithfully emulate as a guest-triggered reset, > and not something where the guest has wedged itself to the point where > qemu can no longer execute the guest. The point is it's both :-) A lot of x86 reset code tries four or five different ways to invoke a reset and if all else fails they triple fault. Dave > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/28/2017 09:42 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> We want to track why a guest was shutdown; in particular, being able >> to tell the difference between a guest request (such as ACPI request) >> and host request (such as SIGINT) will prove useful to libvirt. >> Since all requests eventually end up changing shutdown_requested in >> vl.c, the logical change is to make that value track the reason, >> rather than its current 0/1 contents. >> >> Since command-line options control whether a reset request is turned >> into a shutdown request instead, the same treatment is given to >> reset_requested. >> >> This patch adds a QAPI enum ShutdownCause that describes reasons >> that a shutdown can be requested, and changes qemu_system_reset() to >> pass the reason through, although for now it is not reported. The >> next patch will actually wire things up to modify events to report >> data based on the reason, and to pass the correct enum value in from >> various call-sites that can trigger a reset/shutdown. Since QAPI >> generates enums starting at 0, it's easier if we use a different >> number as our sentinel that no request has happened yet. Most of >> the changes are in vl.c, but xen was using things externally. >> >> -static int reset_requested; >> -static int shutdown_requested, shutdown_signal; >> +static int reset_requested = -1; >> +static int shutdown_requested = -1, shutdown_signal; > > Peeking ahead, I see that shutdown_requested and reset_requested take > ShutdownCause values and -1. The latter means "no shutdown requested". > What about adding 'none' to ShutdownCause, with value 0, und use that > instead of literal -1? Would avoid the unusual "negative means false, > non-negative means true". Works nicely if the enum is internal-use only. Gets a bit more awkward if the enum is exposed to the end-user. The fact that I let QAPI generate the enum in patch 3 is evidence that I'm leaning towards exposing it to the end user (patch 4); if we want to keep it internal-only, a better place for the enum might be in sysemu.h (where we also have the weird '#define VMRESET_SILENT false' '#define VMRESET_REPORT true' to name a boolean parameter). > > PATCH 4 exposes ShutdownCause in event SHUTDOWN, and 'none' must not > occur there. However, if we ever add a query-shutdown to go with this > event, we will need 'none' there. So, query-shutdown would basically be: what is the last-reported shutdown event (normally none, when the guest is still running; but if, like libvirt, you start qemu -no-shutdown, it can then be the cause of why we are in a shutdown/stopped state while waiting for final cleanup)? How important/likely is such an event? (Hmm, from libvirt's perspective, events are usually reliable, but can be lost; if we can restart libvirtd and reconnect to a qemu process that is hanging on to life only because no one has cleaned it up yet, query-shutdown does seem like a useful thing for libvirt to have at the time it reconnects to that qemu process). We could always include 'none' in the QAPI enum, then document in 'SHUTDOWN' and 'RESET' events that the cause will never be 'none'. Doc hacks like that feel a little unclean, but not so horrible as to be unforgivable. > > I'd be tempted to reshuffle declarations here, because shutdown_signal's > int is a different one than reset_requested's and shutdown_requested, > and the latter two's "negative means false, non-negative means true" is > unusual enough to justify a comment. ... > > Hmm. In case we stick to literal -1: consider splitting this patch into > a part that changes @shutdown_requested from zero/non-zero to > negative/non-negative, and a part that uses ShutdownCause for the > non-negative values. You're definitely right that if the enum doesn't have a nice none=0 state, then reshuffling to the magic -1 as no request is awkward enough to be done alone. But part of the answer is also dependent on whether we want PATCH 4 or not (or, as you brought up, the possibility of a query-shutdown command with even more persistent storage of the last-reported event). >> @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) >> static int qemu_reset_requested(void) >> { >> int r = reset_requested; >> - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >> - reset_requested = 0; >> + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >> + reset_requested = -1; >> return r; >> } >> - return false; >> + return -1; > > "return false" in a function returning int smells, good riddance. > In one of my earlier drafts of the patch, I even tried to change the return type from int to bool, but decided that keeping it as int worked best (if I have to use the -1/cause dichotomy). But you're also right that with a 'none' value in the enum, I could directly return ShutdownCause. >> } >> >> static int qemu_suspend_requested(void) >> @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void) >> return r; >> } >> >> -void qemu_system_reset(bool report) >> +/* >> + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using >> + * the @reason interpreted as ShutdownType for details. Otherwise, >> + * @report is VMRESET_SILENT and @reason is ignored. >> + */ >> +void qemu_system_reset(bool report, int reason) > > Why int reason and not ShutdownCause? Hmm, peeking ahead, I see you > pass -1 with VMRESET_SILENT. Yet another place where you use int for > type ShutdownCause + { -1 }. Adding 'none' to ShutdownCause looks > even more attractive to me now. Yeah, it's getting to be that way to me to, even if it just means that I may have volunteered myself into writing a query-shutdown QMP command. >> @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) >> void qemu_system_reset_request(void) >> { >> if (no_reboot) { >> - shutdown_requested = 1; >> + /* FIXME - add a parameter to allow callers to specify reason */ > > FIXME addressed in the next patch. Mention in this one's commit > message? Sure. Something like "Mark a couple of places as FIXME where we have to guess a value to use; a later patch will fix things to supply a correct value". > >> + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; I've also debated about splitting patch 3 into two parts: the event member additions (affecting .json and vl.c) and the parameter additions (affecting all other call-sites). If I add the event parameter first, then supplying a bogus value to the event means extra churn to qemu iotests output files unless I change THIS line of code to guess SHUTDOWN_CAUSE_HOST_QMP; the other option is to wire up parameter passing first and event reporting last. I'll wait for more inputs before respinning this series (I already did a poor enough job slamming mailboxes by sending 3 iterations of the series in one day). As you mention, I'd still like to hear ideas for the replay side of things, and I wouldn't mind if Dan has any ideas from the libvirt/upper-layer stack usage side of things on the fate of patch 4.
Eric Blake <eblake@redhat.com> writes: > On 04/28/2017 09:42 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> We want to track why a guest was shutdown; in particular, being able >>> to tell the difference between a guest request (such as ACPI request) >>> and host request (such as SIGINT) will prove useful to libvirt. >>> Since all requests eventually end up changing shutdown_requested in >>> vl.c, the logical change is to make that value track the reason, >>> rather than its current 0/1 contents. >>> >>> Since command-line options control whether a reset request is turned >>> into a shutdown request instead, the same treatment is given to >>> reset_requested. >>> >>> This patch adds a QAPI enum ShutdownCause that describes reasons >>> that a shutdown can be requested, and changes qemu_system_reset() to >>> pass the reason through, although for now it is not reported. The >>> next patch will actually wire things up to modify events to report >>> data based on the reason, and to pass the correct enum value in from >>> various call-sites that can trigger a reset/shutdown. Since QAPI >>> generates enums starting at 0, it's easier if we use a different >>> number as our sentinel that no request has happened yet. Most of >>> the changes are in vl.c, but xen was using things externally. >>> > >>> -static int reset_requested; >>> -static int shutdown_requested, shutdown_signal; >>> +static int reset_requested = -1; >>> +static int shutdown_requested = -1, shutdown_signal; >> >> Peeking ahead, I see that shutdown_requested and reset_requested take >> ShutdownCause values and -1. The latter means "no shutdown requested". >> What about adding 'none' to ShutdownCause, with value 0, und use that >> instead of literal -1? Would avoid the unusual "negative means false, >> non-negative means true". > > Works nicely if the enum is internal-use only. Gets a bit more awkward > if the enum is exposed to the end-user. > > The fact that I let QAPI generate the enum in patch 3 is evidence that > I'm leaning towards exposing it to the end user (patch 4); if we want to > keep it internal-only, a better place for the enum might be in sysemu.h Yes, unless you need the generated ShutdownCause_lookup[]. > (where we also have the weird '#define VMRESET_SILENT false' '#define > VMRESET_REPORT true' to name a boolean parameter). Some people believe such defines make code more readable, others hate them. Regardless, they're unusual in QEMU. Unusual is best avoided. >> PATCH 4 exposes ShutdownCause in event SHUTDOWN, and 'none' must not >> occur there. However, if we ever add a query-shutdown to go with this >> event, we will need 'none' there. > > So, query-shutdown would basically be: what is the last-reported > shutdown event (normally none, when the guest is still running; but if, > like libvirt, you start qemu -no-shutdown, it can then be the cause of > why we are in a shutdown/stopped state while waiting for final cleanup)? Sounds right. > How important/likely is such an event? (Hmm, from libvirt's > perspective, events are usually reliable, but can be lost; if we can > restart libvirtd and reconnect to a qemu process that is hanging on to > life only because no one has cleaned it up yet, query-shutdown does seem > like a useful thing for libvirt to have at the time it reconnects to > that qemu process). Rule of thumb: if we need an event, we probably need a query, too. > We could always include 'none' in the QAPI enum, then document in > 'SHUTDOWN' and 'RESET' events that the cause will never be 'none'. Yes. > Doc > hacks like that feel a little unclean, but not so horrible as to be > unforgivable. I wouldn't call it an unclean hack. For me, it's coping with an insufficiently expressive type system: we can't define ShutdownCause + { 'none' } as a supertype of ShutdownCause. Even if we could, I'm not sure it would be worth the bother. >> I'd be tempted to reshuffle declarations here, because shutdown_signal's >> int is a different one than reset_requested's and shutdown_requested, >> and the latter two's "negative means false, non-negative means true" is >> unusual enough to justify a comment. > ... >> >> Hmm. In case we stick to literal -1: consider splitting this patch into >> a part that changes @shutdown_requested from zero/non-zero to >> negative/non-negative, and a part that uses ShutdownCause for the >> non-negative values. > > > You're definitely right that if the enum doesn't have a nice none=0 > state, then reshuffling to the magic -1 as no request is awkward enough > to be done alone. > > But part of the answer is also dependent on whether we want PATCH 4 or > not (or, as you brought up, the possibility of a query-shutdown command > with even more persistent storage of the last-reported event). Yes. >>> @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) >>> static int qemu_reset_requested(void) >>> { >>> int r = reset_requested; >>> - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >>> - reset_requested = 0; >>> + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { >>> + reset_requested = -1; >>> return r; >>> } >>> - return false; >>> + return -1; >> >> "return false" in a function returning int smells, good riddance. >> > > In one of my earlier drafts of the patch, I even tried to change the > return type from int to bool, but decided that keeping it as int worked > best (if I have to use the -1/cause dichotomy). But you're also right > that with a 'none' value in the enum, I could directly return ShutdownCause. > >>> } >>> >>> static int qemu_suspend_requested(void) >>> @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void) >>> return r; >>> } >>> >>> -void qemu_system_reset(bool report) >>> +/* >>> + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using >>> + * the @reason interpreted as ShutdownType for details. Otherwise, >>> + * @report is VMRESET_SILENT and @reason is ignored. >>> + */ >>> +void qemu_system_reset(bool report, int reason) >> >> Why int reason and not ShutdownCause? Hmm, peeking ahead, I see you >> pass -1 with VMRESET_SILENT. Yet another place where you use int for >> type ShutdownCause + { -1 }. Adding 'none' to ShutdownCause looks >> even more attractive to me now. > > Yeah, it's getting to be that way to me to, even if it just means that I > may have volunteered myself into writing a query-shutdown QMP command. The reward for doing good work is more work. >>> @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) >>> void qemu_system_reset_request(void) >>> { >>> if (no_reboot) { >>> - shutdown_requested = 1; >>> + /* FIXME - add a parameter to allow callers to specify reason */ >> >> FIXME addressed in the next patch. Mention in this one's commit >> message? > > Sure. Something like "Mark a couple of places as FIXME where we have to > guess a value to use; a later patch will fix things to supply a correct > value". Works for me, provided the meaning of "value" is clear from context. >> >>> + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; > > I've also debated about splitting patch 3 into two parts: the event > member additions (affecting .json and vl.c) and the parameter additions > (affecting all other call-sites). If I add the event parameter first, > then supplying a bogus value to the event means extra churn to qemu > iotests output files unless I change THIS line of code to guess > SHUTDOWN_CAUSE_HOST_QMP; the other option is to wire up parameter > passing first and event reporting last. > > I'll wait for more inputs before respinning this series (I already did a > poor enough job slamming mailboxes by sending 3 iterations of the series > in one day). As you mention, I'd still like to hear ideas for the Your v3 and v4 cost me no time, don't worry. > replay side of things, and I wouldn't mind if Dan has any ideas from the > libvirt/upper-layer stack usage side of things on the fate of patch 4. Okay.
diff --git a/qapi-schema.json b/qapi-schema.json index 01b087f..a4ebdd1 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2304,6 +2304,29 @@ { 'command': 'system_powerdown' } ## +# @ShutdownCause: +# +# Enumeration of various causes for shutdown. +# +# @host-qmp: Reaction to a QMP command, such as 'quit' +# @host-signal: Reaction to a signal, such as SIGINT +# @host-ui: Reaction to a UI event, such as closing the window +# @host-replay: The host is replaying an earlier shutdown event +# @host-error: Qemu encountered an error that prevents further use of the guest +# @guest-shutdown: The guest requested a shutdown, such as via ACPI or +# other hardware-specific action +# @guest-reset: The guest requested a reset, and the command line +# response to a reset is to instead trigger a shutdown +# @guest-panic: The guest panicked, and the command line response to +# a panic is to trigger a shutdown +# +# Since: 2.10 +## +{ 'enum': 'ShutdownCause', + 'data': [ 'host-qmp', 'host-signal', 'host-ui', 'host-replay', 'host-error', + 'guest-shutdown', 'guest-reset', 'guest-panic' ] } + +## # @cpu: # # This command is a nop that is only provided for the purposes of compatibility. diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 16175f7..00a907f 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -65,7 +65,7 @@ bool qemu_vmstop_requested(RunState *r); int qemu_shutdown_requested_get(void); int qemu_reset_requested_get(void); void qemu_system_killed(int signal, pid_t pid); -void qemu_system_reset(bool report); +void qemu_system_reset(bool report, int reason); void qemu_system_guest_panicked(GuestPanicInformation *info); size_t qemu_target_page_size(void); diff --git a/vl.c b/vl.c index 879786a..2b95b7f 100644 --- a/vl.c +++ b/vl.c @@ -1597,8 +1597,8 @@ void vm_state_notify(int running, RunState state) } } -static int reset_requested; -static int shutdown_requested, shutdown_signal; +static int reset_requested = -1; +static int shutdown_requested = -1, shutdown_signal; static pid_t shutdown_pid; static int powerdown_requested; static int debug_requested; @@ -1624,7 +1624,7 @@ int qemu_reset_requested_get(void) static int qemu_shutdown_requested(void) { - return atomic_xchg(&shutdown_requested, 0); + return atomic_xchg(&shutdown_requested, -1); } static void qemu_kill_report(void) @@ -1650,11 +1650,11 @@ static void qemu_kill_report(void) static int qemu_reset_requested(void) { int r = reset_requested; - if (r && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { - reset_requested = 0; + if (r >= 0 && replay_checkpoint(CHECKPOINT_RESET_REQUESTED)) { + reset_requested = -1; return r; } - return false; + return -1; } static int qemu_suspend_requested(void) @@ -1686,7 +1686,12 @@ static int qemu_debug_requested(void) return r; } -void qemu_system_reset(bool report) +/* + * Reset the VM. If @report is VMRESET_REPORT, issue an event, using + * the @reason interpreted as ShutdownType for details. Otherwise, + * @report is VMRESET_SILENT and @reason is ignored. + */ +void qemu_system_reset(bool report, int reason) { MachineClass *mc; @@ -1700,6 +1705,7 @@ void qemu_system_reset(bool report) qemu_devices_reset(); } if (report) { + assert(reason >= 0); qapi_event_send_reset(&error_abort); } cpu_synchronize_all_post_reset(); @@ -1738,9 +1744,10 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) void qemu_system_reset_request(void) { if (no_reboot) { - shutdown_requested = 1; + /* FIXME - add a parameter to allow callers to specify reason */ + shutdown_requested = SHUTDOWN_CAUSE_GUEST_RESET; } else { - reset_requested = 1; + reset_requested = SHUTDOWN_CAUSE_GUEST_RESET; } cpu_stop_current(); qemu_notify_event(); @@ -1807,7 +1814,7 @@ void qemu_system_killed(int signal, pid_t pid) /* Cannot call qemu_system_shutdown_request directly because * we are in a signal handler. */ - shutdown_requested = 1; + shutdown_requested = SHUTDOWN_CAUSE_HOST_SIGNAL; qemu_notify_event(); } @@ -1815,7 +1822,8 @@ void qemu_system_shutdown_request(void) { trace_qemu_system_shutdown_request(); replay_shutdown_request(); - shutdown_requested = 1; + /* FIXME - add a parameter to allow callers to specify reason */ + shutdown_requested = SHUTDOWN_CAUSE_GUEST_SHUTDOWN; qemu_notify_event(); } @@ -1846,13 +1854,16 @@ void qemu_system_debug_request(void) static bool main_loop_should_exit(void) { RunState r; + int request; + if (qemu_debug_requested()) { vm_stop(RUN_STATE_DEBUG); } if (qemu_suspend_requested()) { qemu_system_suspend(); } - if (qemu_shutdown_requested()) { + request = qemu_shutdown_requested(); + if (request >= 0) { qemu_kill_report(); qapi_event_send_shutdown(&error_abort); if (no_shutdown) { @@ -1861,9 +1872,10 @@ static bool main_loop_should_exit(void) return true; } } - if (qemu_reset_requested()) { + request = qemu_reset_requested(); + if (request >= 0) { pause_all_vcpus(); - qemu_system_reset(VMRESET_REPORT); + qemu_system_reset(VMRESET_REPORT, request); resume_all_vcpus(); if (!runstate_check(RUN_STATE_RUNNING) && !runstate_check(RUN_STATE_INMIGRATE)) { @@ -1872,7 +1884,7 @@ static bool main_loop_should_exit(void) } if (qemu_wakeup_requested()) { pause_all_vcpus(); - qemu_system_reset(VMRESET_SILENT); + qemu_system_reset(VMRESET_SILENT, -1); notifier_list_notify(&wakeup_notifiers, &wakeup_reason); wakeup_reason = QEMU_WAKEUP_REASON_NONE; resume_all_vcpus(); @@ -4684,7 +4696,7 @@ int main(int argc, char **argv, char **envp) reading from the other reads, because timer polling functions query clock values from the log. */ replay_checkpoint(CHECKPOINT_RESET); - qemu_system_reset(VMRESET_SILENT); + qemu_system_reset(VMRESET_SILENT, -1); register_global_state(); if (replay_mode != REPLAY_MODE_NONE) { replay_vmstate_init(); diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index b1c05ff..3a6484c 100644 --- a/hw/i386/xen/xen-hvm.c +++ b/hw/i386/xen/xen-hvm.c @@ -1089,11 +1089,14 @@ static void cpu_handle_ioreq(void *opaque) * causes Xen to powerdown the domain. */ if (runstate_is_running()) { - if (qemu_shutdown_requested_get()) { + int request; + + if (qemu_shutdown_requested_get() >= 0) { destroy_hvm_domain(false); } - if (qemu_reset_requested_get()) { - qemu_system_reset(VMRESET_REPORT); + request = qemu_reset_requested_get(); + if (request >= 0) { + qemu_system_reset(VMRESET_REPORT, request); destroy_hvm_domain(true); } } diff --git a/migration/colo.c b/migration/colo.c index c19eb3f..17a5482 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -611,7 +611,7 @@ void *colo_process_incoming_thread(void *opaque) } qemu_mutex_lock_iothread(); - qemu_system_reset(VMRESET_SILENT); + qemu_system_reset(VMRESET_SILENT, -1); vmstate_loading = true; if (qemu_loadvm_state(fb) < 0) { error_report("COLO: loadvm failed"); diff --git a/migration/savevm.c b/migration/savevm.c index 03ae1bd..dcbaf00 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2292,7 +2292,7 @@ int load_vmstate(const char *name) return -EINVAL; } - qemu_system_reset(VMRESET_SILENT); + qemu_system_reset(VMRESET_SILENT, -1); mis->from_src_file = f; aio_context_acquire(aio_context);
We want to track why a guest was shutdown; in particular, being able to tell the difference between a guest request (such as ACPI request) and host request (such as SIGINT) will prove useful to libvirt. Since all requests eventually end up changing shutdown_requested in vl.c, the logical change is to make that value track the reason, rather than its current 0/1 contents. Since command-line options control whether a reset request is turned into a shutdown request instead, the same treatment is given to reset_requested. This patch adds a QAPI enum ShutdownCause that describes reasons that a shutdown can be requested, and changes qemu_system_reset() to pass the reason through, although for now it is not reported. The next patch will actually wire things up to modify events to report data based on the reason, and to pass the correct enum value in from various call-sites that can trigger a reset/shutdown. Since QAPI generates enums starting at 0, it's easier if we use a different number as our sentinel that no request has happened yet. Most of the changes are in vl.c, but xen was using things externally. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: s/ShutdownType/ShutdownCause/, no thanks to mingw header pollution v3: new patch --- qapi-schema.json | 23 +++++++++++++++++++++++ include/sysemu/sysemu.h | 2 +- vl.c | 44 ++++++++++++++++++++++++++++---------------- hw/i386/xen/xen-hvm.c | 9 ++++++--- migration/colo.c | 2 +- migration/savevm.c | 2 +- 6 files changed, 60 insertions(+), 22 deletions(-)