On 27/01/17 18:05, Alex Bennée wrote: > > Claudio Imbrenda <imbre...@linux.vnet.ibm.com> writes: > >> On 27/01/17 17:31, Alex Bennée wrote: >>> >>> Claudio Imbrenda <imbre...@linux.vnet.ibm.com> writes: >>> >>>> This patch: >>>> >>>> * moves vm_start to cpus.c . >>>> * exports qemu_vmstop_requested, since it's needed by vm_start . >>>> * extracts vm_prepare_start from vm_start; it does what vm_start did, >>>> except restarting the cpus. vm_start now calls vm_prepare_start. >>>> * moves the call to qemu_clock_enable away from resume_all_vcpus, and >>>> add an explicit call to it before each instance of resume_all_vcpus >>>> in the code. >>> >>> Can you be a bit more explicit about this? Shouldn't CPU time still >>> accrue even if you are only starting some of them? >> >> This is what's happening in the newest version of this patch, which I >> sent around yesterday, although I forgot to update the patch >> description; I'll send a fixed version ASAP. >> >>> I'd prefer making resume_all_vcpus() a private function called from >> >> resume_all_vcpus is already being used in other files too, doesn't make >> sense to make it private now. > > Yeah I would rename the call-sites across QEMU. Perhaps just one entry > point of rename_vcpus() which does the right thing given a list or NULL. > But pushing the details of restarting the timer to the call sites just
this is not happening any longer in the patch I sent yesterday (v6). the only thing happening now in the first patch is moving vm_start and splitting it. clocks are not affected, no further code changes are needed. > sounds like a way of it getting missed next time someone adds a resume > somewhere. > >> >>> resume_some_vcpus() which can then make the QEMU_CLOCK_VIRTUAL decision >>> in one place - with a comment. >>> >>>> * adds resume_some_vcpus, to selectively restart only some CPUs. >>>> >>>> Signed-off-by: Claudio Imbrenda <imbre...@linux.vnet.ibm.com> >>>> --- >>>> cpus.c | 61 >>>> +++++++++++++++++++++++++++++++++++++++++++++- >>>> hw/i386/kvmvapic.c | 2 ++ >>>> include/sysemu/cpus.h | 1 + >>>> include/sysemu/sysemu.h | 2 ++ >>>> target-s390x/misc_helper.c | 2 ++ >>>> vl.c | 32 +++--------------------- >>>> 6 files changed, 70 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/cpus.c b/cpus.c >>>> index 31204bb..c102624 100644 >>>> --- a/cpus.c >>>> +++ b/cpus.c >>>> @@ -1260,12 +1260,28 @@ void resume_all_vcpus(void) >>>> { >>>> CPUState *cpu; >>>> >>>> - qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> CPU_FOREACH(cpu) { >>>> cpu_resume(cpu); >>>> } >>>> } >>>> >>>> +/** >>>> + * resume_some_vcpus - resumes some vCPUs, indicated in a NULL-terminated >>>> + * array of CPUState *; if the parameter is NULL, all CPUs are resumed. >>>> + */ >>>> +void resume_some_vcpus(CPUState **cpus) >>>> +{ >>>> + int idx; >>>> + >>>> + if (!cpus) { >>>> + resume_all_vcpus(); >>>> + return; >>>> + } >>>> + for (idx = 0; cpus[idx]; idx++) { >>>> + cpu_resume(cpus[idx]); >>>> + } >>>> +} >>>> + >>>> void cpu_remove(CPUState *cpu) >>>> { >>>> cpu->stop = true; >>>> @@ -1396,6 +1412,49 @@ int vm_stop(RunState state) >>>> return do_vm_stop(state); >>>> } >>>> >>>> +/** >>>> + * Prepare for (re)starting the VM. >>>> + * Returns -1 if the vCPUs are not to be restarted (e.g. if they are >>>> already >>>> + * running or in case of an error condition), 0 otherwise. >>>> + */ >>>> +int vm_prepare_start(void) >>>> +{ >>>> + RunState requested; >>>> + int res = 0; >>>> + >>>> + qemu_vmstop_requested(&requested); >>>> + if (runstate_is_running() && requested == RUN_STATE__MAX) { >>>> + return -1; >>>> + } >>>> + >>>> + /* Ensure that a STOP/RESUME pair of events is emitted if a >>>> + * vmstop request was pending. The BLOCK_IO_ERROR event, for >>>> + * example, according to documentation is always followed by >>>> + * the STOP event. >>>> + */ >>>> + if (runstate_is_running()) { >>>> + qapi_event_send_stop(&error_abort); >>>> + res = -1; >>>> + } else { >>>> + replay_enable_events(); >>>> + cpu_enable_ticks(); >>>> + runstate_set(RUN_STATE_RUNNING); >>>> + vm_state_notify(1, RUN_STATE_RUNNING); >>>> + } >>>> + >>>> + /* XXX: is it ok to send this even before actually resuming the CPUs? >>>> */ >>>> + qapi_event_send_resume(&error_abort); >>>> + return res; >>>> +} >>>> + >>>> +void vm_start(void) >>>> +{ >>>> + if (!vm_prepare_start()) { >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> + resume_all_vcpus(); >>>> + } >>>> +} >>>> + >>>> /* does a state transition even if the VM is already stopped, >>>> current state is forgotten forever */ >>>> int vm_stop_force_state(RunState state) >>>> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c >>>> index 74a549b..3101860 100644 >>>> --- a/hw/i386/kvmvapic.c >>>> +++ b/hw/i386/kvmvapic.c >>>> @@ -446,6 +446,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU >>>> *cpu, target_ulong ip) >>>> abort(); >>>> } >>>> >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> >>>> if (!kvm_enabled()) { >>>> @@ -686,6 +687,7 @@ static void vapic_write(void *opaque, hwaddr addr, >>>> uint64_t data, >>>> pause_all_vcpus(); >>>> patch_byte(cpu, env->eip - 2, 0x66); >>>> patch_byte(cpu, env->eip - 1, 0x90); >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> } >>>> >>>> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h >>>> index 3728a1e..5fa074b 100644 >>>> --- a/include/sysemu/cpus.h >>>> +++ b/include/sysemu/cpus.h >>>> @@ -5,6 +5,7 @@ >>>> bool qemu_in_vcpu_thread(void); >>>> void qemu_init_cpu_loop(void); >>>> void resume_all_vcpus(void); >>>> +void resume_some_vcpus(CPUState **cpus); >>>> void pause_all_vcpus(void); >>>> void cpu_stop_current(void); >>>> void cpu_ticks_init(void); >>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >>>> index ef2c50b..ac301d6 100644 >>>> --- a/include/sysemu/sysemu.h >>>> +++ b/include/sysemu/sysemu.h >>>> @@ -37,6 +37,7 @@ void vm_state_notify(int running, RunState state); >>>> #define VMRESET_REPORT true >>>> >>>> void vm_start(void); >>>> +int vm_prepare_start(void); >>>> int vm_stop(RunState state); >>>> int vm_stop_force_state(RunState state); >>>> >>>> @@ -60,6 +61,7 @@ void qemu_register_powerdown_notifier(Notifier >>>> *notifier); >>>> void qemu_system_debug_request(void); >>>> void qemu_system_vmstop_request(RunState reason); >>>> void qemu_system_vmstop_request_prepare(void); >>>> +bool qemu_vmstop_requested(RunState *r); >>>> int qemu_shutdown_requested_get(void); >>>> int qemu_reset_requested_get(void); >>>> void qemu_system_killed(int signal, pid_t pid); >>>> diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c >>>> index 4df2ec6..2b74710 100644 >>>> --- a/target-s390x/misc_helper.c >>>> +++ b/target-s390x/misc_helper.c >>>> @@ -133,6 +133,7 @@ static int modified_clear_reset(S390CPU *cpu) >>>> s390_crypto_reset(); >>>> scc->load_normal(CPU(cpu)); >>>> cpu_synchronize_all_post_reset(); >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> return 0; >>>> } >>>> @@ -152,6 +153,7 @@ static int load_normal_reset(S390CPU *cpu) >>>> scc->initial_cpu_reset(CPU(cpu)); >>>> scc->load_normal(CPU(cpu)); >>>> cpu_synchronize_all_post_reset(); >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> return 0; >>>> } >>>> diff --git a/vl.c b/vl.c >>>> index f3abd99..42addbb 100644 >>>> --- a/vl.c >>>> +++ b/vl.c >>>> @@ -746,7 +746,7 @@ StatusInfo *qmp_query_status(Error **errp) >>>> return info; >>>> } >>>> >>>> -static bool qemu_vmstop_requested(RunState *r) >>>> +bool qemu_vmstop_requested(RunState *r) >>>> { >>>> qemu_mutex_lock(&vmstop_lock); >>>> *r = vmstop_requested; >>>> @@ -767,34 +767,6 @@ void qemu_system_vmstop_request(RunState state) >>>> qemu_notify_event(); >>>> } >>>> >>>> -void vm_start(void) >>>> -{ >>>> - RunState requested; >>>> - >>>> - qemu_vmstop_requested(&requested); >>>> - if (runstate_is_running() && requested == RUN_STATE__MAX) { >>>> - return; >>>> - } >>>> - >>>> - /* Ensure that a STOP/RESUME pair of events is emitted if a >>>> - * vmstop request was pending. The BLOCK_IO_ERROR event, for >>>> - * example, according to documentation is always followed by >>>> - * the STOP event. >>>> - */ >>>> - if (runstate_is_running()) { >>>> - qapi_event_send_stop(&error_abort); >>>> - } else { >>>> - replay_enable_events(); >>>> - cpu_enable_ticks(); >>>> - runstate_set(RUN_STATE_RUNNING); >>>> - vm_state_notify(1, RUN_STATE_RUNNING); >>>> - resume_all_vcpus(); >>>> - } >>>> - >>>> - qapi_event_send_resume(&error_abort); >>>> -} >>>> - >>>> - >>>> /***********************************************************/ >>>> /* real time host monotonic timer */ >>>> >>>> @@ -1910,6 +1882,7 @@ static bool main_loop_should_exit(void) >>>> if (qemu_reset_requested()) { >>>> pause_all_vcpus(); >>>> qemu_system_reset(VMRESET_REPORT); >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> if (!runstate_check(RUN_STATE_RUNNING) && >>>> !runstate_check(RUN_STATE_INMIGRATE)) { >>>> @@ -1921,6 +1894,7 @@ static bool main_loop_should_exit(void) >>>> qemu_system_reset(VMRESET_SILENT); >>>> notifier_list_notify(&wakeup_notifiers, &wakeup_reason); >>>> wakeup_reason = QEMU_WAKEUP_REASON_NONE; >>>> + qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true); >>>> resume_all_vcpus(); >>>> qapi_event_send_wakeup(&error_abort); >>>> } >>> >>> >>> -- >>> Alex Bennée >>> > > > -- > Alex Bennée >