[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] move vm_start to cpus.c
From: |
Claudio Imbrenda |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] move vm_start to cpus.c |
Date: |
Fri, 27 Jan 2017 18:16:29 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
On 27/01/17 18:05, Alex Bennée wrote:
>
> Claudio Imbrenda <address@hidden> writes:
>
>> On 27/01/17 17:31, Alex Bennée wrote:
>>>
>>> Claudio Imbrenda <address@hidden> 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 <address@hidden>
>>>> ---
>>>> 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
>