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.1Steve 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
© 2016 - 2026 Red Hat, Inc.