Re: [Qemu-devel] [PATCH v2 1/2] move vm_start to cpus.c

[Alex Bennée]

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
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