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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 22 Apr 2013 15:42:09 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 22.04.2013 13:45, schrieb Igor Mammedov:
> > On Mon, 22 Apr 2013 13:59:17 +0300
> > Gleb Natapov <gleb@redhat.com> 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 <gleb@redhat.com> 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 <imammedo@redhat.com>
> >>>>> ---
> >>>>> 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?

> 
> Andreas
>