On Mon, Nov 13, 2023 at 10:33:50AM -0800, Steve Sistare wrote:
> A vm in the suspended state is not completely stopped. The VCPUs have been
> paused, but the cpu clock still runs, and runstate notifiers for the
> transition to stopped have not been called. Modify vm_stop_force_state to
> completely stop the vm if the current state is suspended, to be called for
> live migration and snapshots.
>
> Suggested-by: Peter Xu <pet...@redhat.com>
> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
> ---
> system/cpus.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/system/cpus.c b/system/cpus.c
> index f72c4be..c772708 100644
> --- a/system/cpus.c
> +++ b/system/cpus.c
> @@ -255,6 +255,8 @@ void cpu_interrupt(CPUState *cpu, int mask)
> static int do_vm_stop(RunState state, bool send_stop, bool force)
> {
> int ret = 0;
> + bool running = runstate_is_running();
> + bool suspended = runstate_check(RUN_STATE_SUSPENDED);
>
> if (qemu_in_vcpu_thread()) {
> qemu_system_vmstop_request_prepare();
> @@ -267,10 +269,12 @@ static int do_vm_stop(RunState state, bool send_stop,
> bool force)
> return 0;
> }
>
> - if (runstate_is_running()) {
> + if (running || (suspended && force)) {
> runstate_set(state);
> cpu_disable_ticks();
Not directly relevant, but this is weird that I just notice.
If we disable ticks before stopping vCPUs, IIUC it means vcpus can see
stall ticks. I checked the vm_start() and indeed that one did it in the
other way round: we'll stop vCPUs before stopping the ticks.
> - pause_all_vcpus();
> + if (running) {
> + pause_all_vcpus();
> + }
> vm_state_notify(0, state);
> if (send_stop) {
> qapi_event_send_stop();
IIUC the "force" is not actually needed. It's only used when SUSPENDED,
right?
In general, considering all above, I'm wondering something like this would
be much cleaner (and dropping force)?
===8<===
static int do_vm_stop(RunState state, bool send_stop)
{
+ bool suspended = runstate_check(RUN_STATE_SUSPENDED);
+ bool running = runstate_is_running();
int ret = 0;
- if (runstate_is_running()) {
+ /*
+ * RUNNING: VM and vCPUs are all running
+ * SUSPENDED: VM is running, VCPUs are stopped
+ * Others: VM and vCPUs are all stopped
+ */
+
+ /* Whether do we need to stop vCPUs? */
+ if (running) {
+ pause_all_vcpus();
+ }
+
+ /* Whether do we need to stop the VM in general? */
+ if (running || suspended) {
runstate_set(state);
cpu_disable_ticks();
- pause_all_vcpus();
vm_state_notify(0, state);
if (send_stop) {
qapi_event_send_stop();
--
Peter Xu
