| Message ID | 1699900440-207345-2-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix migration of suspended runstate | expand |
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,
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,
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
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.
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,
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(-)