On 11/20/2023 9:15 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sist...@oracle.com> writes:
>
>> A vm in the suspended state is not completely stopped.
>
> Is this a statement of a fact about VMs in the suspended state in
> general or is this describing what this patch is trying to fix?
The former.
>> The VCPUs have been paused, but the cpu clock still runs, and runstate
>> notifiers for the transition to stopped have not been called.
>
> ...it reads like the latter, but then why aren't we fixing this at the
> moment we put the VM in the suspend state?
cpu_get_ticks() must continue to tick while the guest is suspended, so that
QEMU_CLOCK_VIRTUAL continues to tick, so that timeouts based on that clock
will fire. One example is timed wake from suspend, acpi_pm_tmr_timer.
>> Modify vm_stop_force_state to completely stop the vm if the current
>> state is suspended, to be called for live migration and snapshots.
>
> Hm, this changes the meaning of the "force" from:
>
> "force a state even if already stopped"
>
> into:
>
> "force a complete stop if already suspended, otherwise just set the
> state"
vm_stop_force_state has the same behavior as before for all states
except suspended. If suspended, it also:
- stops cpu ticks
- calls runstate stopped handlers
- sets a new runstate
> I don't know what to make of this, shouldn't all vm_stops cause a
> complete stop?
We cannot stop cpu_get_ticks. We could maybe call the runstate stop handlers,
but that requires a careful examination of every handler, and there is no
obvious
correctness or cleanliness reason to stop them immediately on vm_stop(), since
cpu
ticks still needs special handling later.
> We need to at least resolve the overloading of the 'force' term.
How about a more complete function header comment:
/*
* If the machine is running or suspended, completely stop it.
* Force the new runstate to @state.
* The current state is forgotten forever.
*/
- Steve
>> 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();
>> - pause_all_vcpus();
>> + if (running) {
>> + pause_all_vcpus();
>> + }
>> vm_state_notify(0, state);
>> if (send_stop) {
>> qapi_event_send_stop();
