On 11/20/2023 8:22 AM, Fabiano Rosas wrote: > Steve Sistare <steven.sist...@oracle.com> writes: >> Refactor the vm_stop functions into one common subroutine do_vm_stop called >> with options. No functional change. >> >> Signed-off-by: Steve Sistare <steven.sist...@oracle.com> >> --- >> system/cpus.c | 44 +++++++++++++++++--------------------------- >> 1 file changed, 17 insertions(+), 27 deletions(-) >> >> diff --git a/system/cpus.c b/system/cpus.c >> index 0848e0d..f72c4be 100644 >> --- a/system/cpus.c >> +++ b/system/cpus.c >> @@ -252,10 +252,21 @@ void cpu_interrupt(CPUState *cpu, int mask) >> } >> } >> >> -static int do_vm_stop(RunState state, bool send_stop) >> +static int do_vm_stop(RunState state, bool send_stop, bool force) >> { >> int ret = 0; >> >> + if (qemu_in_vcpu_thread()) { >> + qemu_system_vmstop_request_prepare(); >> + qemu_system_vmstop_request(state); >> + /* >> + * FIXME: should not return to device code in case >> + * vm_stop() has been requested. >> + */ >> + cpu_stop_current(); >> + return 0; >> + } > > At vm_stop_force_state(), this block used to be under > runstate_is_running(), but now it runs unconditionally.
vm_stop_force_state callers should never be called in a vcpu thread, so this block is never executed for them. I could assert that. > At vm_shutdown(), this block was not executed at all, but now it is. vm_shutdown should never be called from a vcpu thread. I could assert that. - Steve > We might need some words to explain why this patch doesn't affect > functionality. >> + >> if (runstate_is_running()) { >> runstate_set(state); >> cpu_disable_ticks(); >> @@ -264,6 +275,8 @@ static int do_vm_stop(RunState state, bool send_stop) >> if (send_stop) { >> qapi_event_send_stop(); >> } >> + } else if (force) { >> + runstate_set(state); >> } >> >> bdrv_drain_all(); >> @@ -278,7 +291,7 @@ static int do_vm_stop(RunState state, bool send_stop) >> */ >> int vm_shutdown(void) >> { >> - return do_vm_stop(RUN_STATE_SHUTDOWN, false); >> + return do_vm_stop(RUN_STATE_SHUTDOWN, false, false); >> } >> >> bool cpu_can_run(CPUState *cpu) >> @@ -656,18 +669,7 @@ void cpu_stop_current(void) >> >> int vm_stop(RunState state) >> { >> - if (qemu_in_vcpu_thread()) { >> - qemu_system_vmstop_request_prepare(); >> - qemu_system_vmstop_request(state); >> - /* >> - * FIXME: should not return to device code in case >> - * vm_stop() has been requested. >> - */ >> - cpu_stop_current(); >> - return 0; >> - } >> - >> - return do_vm_stop(state, true); >> + return do_vm_stop(state, true, false); >> } >> >> /** >> @@ -723,19 +725,7 @@ void vm_start(void) >> current state is forgotten forever */ >> int vm_stop_force_state(RunState state) >> { >> - if (runstate_is_running()) { >> - return vm_stop(state); >> - } else { >> - int ret; >> - runstate_set(state); >> - >> - bdrv_drain_all(); >> - /* Make sure to return an error if the flush in a previous vm_stop() >> - * failed. */ >> - ret = bdrv_flush_all(); >> - trace_vm_stop_flush_all(ret); >> - return ret; >> - } >> + return do_vm_stop(state, true, true); >> } >> >> void qmp_memsave(int64_t addr, int64_t size, const char *filename,