Re: [Qemu-devel] [PATCH 03/16] introduce resume_vcpu(), for single CPU

[Andreas Färber]

Am 22.04.2013 16:50, schrieb Igor Mammedov:
> On Mon, 22 Apr 2013 15:42:09 +0200
> Andreas Färber <address@hidden> wrote:
> 
>> Am 22.04.2013 13:45, schrieb Igor Mammedov:
>>> On Mon, 22 Apr 2013 13:59:17 +0300
>>> Gleb Natapov <address@hidden> wrote:
>>>
>>>> On Mon, Apr 22, 2013 at 12:54:04PM +0200, Igor Mammedov wrote:
>>>>> On Mon, 22 Apr 2013 13:40:02 +0300
>>>>> Gleb Natapov <address@hidden> wrote:
>>>>>
>>>>>> On Tue, Apr 16, 2013 at 12:12:43AM +0200, Igor Mammedov wrote:
>>>>>>> ... also add stub for it, to make possible to use it in qom/cpu.c,
>>>>>>> which is shared with user emulators.
>>>>>>>
>>>>>>> Signed-off-by: Igor Mammedov <address@hidden>
>>>>>>> ---
>>>>>>> Note:
>>>>>>>   it will be re-used from qom/cpu.c by hotpluged CPU
>>>>>>> ---
>>>>>>>  cpus.c              | 11 ++++++++---
>>>>>>>  include/qom/cpu.h   |  9 +++++++++
>>>>>>>  stubs/Makefile.objs |  1 +
>>>>>>>  stubs/cpus.c        |  5 +++++
>>>>>>>  4 files changed, 23 insertions(+), 3 deletions(-)
>>>>>>>  create mode 100644 stubs/cpus.c
>>>>>>>
>>>>>>> diff --git a/cpus.c b/cpus.c
>>>>>>> index c15ff6c..c347db5 100644
>>>>>>> --- a/cpus.c
>>>>>>> +++ b/cpus.c
>>>>>>> @@ -993,6 +993,13 @@ void pause_all_vcpus(void)
>>>>>>>      }
>>>>>>>  }
>>>>>>>  
>>>>>>> +void resume_vcpu(CPUState *cpu)
>>>>>>> +{
>>>>>>> +    cpu->stop = false;
>>>>>>> +    cpu->stopped = false;
>>>>>>> +    qemu_cpu_kick(cpu);
>>>>>>> +}
>>>>>>> +
>>>>>>>  void resume_all_vcpus(void)
>>>>>>>  {
>>>>>>>      CPUArchState *penv = first_cpu;
>>>>>>> @@ -1000,9 +1007,7 @@ void resume_all_vcpus(void)
>>>>>>>      qemu_clock_enable(vm_clock, true);
>>>>>>>      while (penv) {
>>>>>>>          CPUState *pcpu = ENV_GET_CPU(penv);
>>>>>>> -        pcpu->stop = false;
>>>>>>> -        pcpu->stopped = false;
>>>>>>> -        qemu_cpu_kick(pcpu);
>>>>>>> +        resume_vcpu(pcpu);
>>>>>>>          penv = penv->next_cpu;
>>>>>>>      }
>>>>>>>  }
>>>>>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>>>>>> index 3664a1b..6ce6f10 100644
>>>>>>> --- a/include/qom/cpu.h
>>>>>>> +++ b/include/qom/cpu.h
>>>>>>> @@ -256,5 +256,14 @@ void cpu_interrupt(CPUState *cpu, int mask);
>>>>>>>   */
>>>>>>>  void cpu_reset_interrupt(CPUState *cpu, int mask);
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * resume_vcpu:
>>>>>>> + * @cpu: The CPU to resume.
>>>>>>> + *
>>>>>>> + * Resumes CPU, i.e. puts CPU into runable state.
>>>>>>> + * KVM requires extra call cpu_synchronize_post_init() to make
>>>>>>> + * KVM's CPU instance runable.
>>>>>>> + */
>>>>>> Can you elaborate on this a bit. Why is it needed and why is it
>>>>>> relevant for this function?
>>>>> If running in KVM mode and resume_vcpu() is called without
>>>>> cpu_synchronize_post_init(), then CPU instance in KVM will be
>>>>> left in UNCONFIGURED state. I've added comment here to document 
>>>>> that resume_vcpu() isn't enough to make CPU run-able in case of KVM.
>>>>>
>>>> But this really relevant only during vcpu creation after that
>>>> resume_vcpu() should just work. My be have launch_vcpu() function that
>>> Agreed, it's applicable only during CPU creation.
>>>
>>>> does the right thing and add assert to resume_vcpu() if it is called on
>>>> vcpu that is not launched yet. In other words made the dependency clear
>>>> in the code, not comment.
>>> Would dropping this comment, suffice
>>> and re-factoring to use launch_vcpu() done later?
>>
>> Either way I had request cpu_... naming for CPUState functions on IRC.
>> Could be a follow-up cleanup.
> How about cpu_resume(), is it suitable?

Sure, that matches the scheme.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg