Re: [PATCH V5 02/12] cpus: stop vm in suspended state

[Steven Sistare]

On 11/20/2023 9:15 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@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 <peterx@redhat.com>
>> Signed-off-by: Steve Sistare <steven.sistare@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();