Refactor the vm_stop functions into one common subroutine do_vm_stop called
with options.  No functional change.

Signed-off-by: Steve Sistare <steven.sistare@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;
+    }
+
     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,
-- 
1.8.3.1

Steve Sistare <steven.sistare@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.sistare@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.

At vm_shutdown(), this block was not executed at all, but now it is.

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,

On 11/20/2023 8:22 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@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.sistare@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,

Steven Sistare <steven.sistare@oracle.com> writes:

> On 11/20/2023 8:22 AM, Fabiano Rosas wrote:
>> Steve Sistare <steven.sistare@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.sistare@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.

Yes, this is an assumption that will get lost to time unless we document
it or have code to enforce.

On Mon, Nov 20, 2023 at 02:09:31PM -0500, Steven Sistare wrote:
> On 11/20/2023 8:22 AM, Fabiano Rosas wrote:
> > Steve Sistare <steven.sistare@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.sistare@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.

Would you split the patch into two?  Moving qemu_in_vcpu_thread() is one,
the rest can be put into another, IMHO.  That may also help to make the
review easier.  OTOH the code changes look all correct here.  Thanks,

-- 
Peter Xu

On 11/20/2023 2:46 PM, Peter Xu wrote:
> On Mon, Nov 20, 2023 at 02:09:31PM -0500, Steven Sistare wrote:
>> On 11/20/2023 8:22 AM, Fabiano Rosas wrote:
>>> Steve Sistare <steven.sistare@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.sistare@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.
> 
> Would you split the patch into two?  Moving qemu_in_vcpu_thread() is one,
> the rest can be put into another, IMHO.  That may also help to make the
> review easier.  OTOH the code changes look all correct here.  Thanks,

Will do, thanks - steve