Patchwork [06/22] cpu: introduce CPUClass.resume() method

login
register
mail settings
Submitter Igor Mammedov
Date April 5, 2013, 2:36 p.m.
Message ID <1365172636-28628-7-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/234155/
State New
Headers show

Comments

Igor Mammedov - April 5, 2013, 2:36 p.m.
... and call it if defined from CPUClass.realize() if CPU was hotplugged

by default leave .resume() unset (i.e. NULL) and override it for softmmu
in qemu_init_vcpu() if it's still unset.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 cpus.c            | 15 ++++++++++++---
 include/qom/cpu.h |  2 ++
 qom/cpu.c         |  5 +++++
 3 files changed, 19 insertions(+), 3 deletions(-)
liguang - April 8, 2013, 2:27 a.m.
Reviewed-by: liguang <lig.fnst@cn.fujitsu.com>

在 2013-04-05五的 16:36 +0200,Igor Mammedov写道:
> ... and call it if defined from CPUClass.realize() if CPU was hotplugged
> 
> by default leave .resume() unset (i.e. NULL) and override it for softmmu
> in qemu_init_vcpu() if it's still unset.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  cpus.c            | 15 ++++++++++++---
>  include/qom/cpu.h |  2 ++
>  qom/cpu.c         |  5 +++++
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 9b9a32f..6b793c5 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
>      }
>  }
>  
> +static void resume_vcpu(CPUState *cpu)
> +{
> +    cpu->stop = false;
> +    cpu->stopped = false;
> +    qemu_cpu_kick(cpu);
> +}
> +
>  void resume_all_vcpus(void)
>  {
>      CPUArchState *penv = first_cpu;
> @@ -980,9 +987,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;
>      }
>  }
> @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
>  {
>      CPUArchState *env = _env;
>      CPUState *cpu = ENV_GET_CPU(env);
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>  
> +    if (klass->resume == NULL) {
> +        klass->resume = resume_vcpu;
> +    }
>      cpu->nr_cores = smp_cores;
>      cpu->nr_threads = smp_threads;
>      cpu->stopped = true;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 3664a1b..6d6eb7a 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -45,6 +45,7 @@ typedef struct CPUState CPUState;
>   * instantiatable CPU type.
>   * @reset: Callback to reset the #CPUState to its initial state.
>   * @do_interrupt: Callback for interrupt handling.
> + * @resume: Callback for putting CPU in runable state
>   * @vmsd: State description for migration.
>   *
>   * Represents a CPU family or model.
> @@ -58,6 +59,7 @@ typedef struct CPUClass {
>  
>      void (*reset)(CPUState *cpu);
>      void (*do_interrupt)(CPUState *cpu);
> +    void (*resume)(CPUState *cpu);
>  
>      const struct VMStateDescription *vmsd;
>  } CPUClass;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 0c76712..c062e00 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -58,8 +58,13 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
>  
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
> +    CPUClass *klass = CPU_GET_CLASS(dev);
> +
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(CPU(dev));
> +        if (klass->resume != NULL) {
> +            klass->resume(CPU(dev));
> +        }
>      }
>  }
>
Eduardo Habkost - April 8, 2013, 8:13 p.m.
On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote:
> ... and call it if defined from CPUClass.realize() if CPU was hotplugged
> 
> by default leave .resume() unset (i.e. NULL) and override it for softmmu
> in qemu_init_vcpu() if it's still unset.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[...]
> diff --git a/cpus.c b/cpus.c
> index 9b9a32f..6b793c5 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
[...]
> @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
>  {
>      CPUArchState *env = _env;
>      CPUState *cpu = ENV_GET_CPU(env);
> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>  
> +    if (klass->resume == NULL) {
> +        klass->resume = resume_vcpu;
> +    }

So you are initializing a field of CPUClass struct inside a CPU object
initialization function. And that's a function that is not even
converted to QOM yet, and buried inside a non-trivial function call tree
(hence easy to be called at the wrong time if one day we reorder the
initialization steps).

Can't we do this on class_init(), where it belongs? If we need different
implementations for softmmu/user, we can add a stub for *-user. I think
even an explicit #ifdef inside resume_vcpu() would be preferable to
this.


>      cpu->nr_cores = smp_cores;
>      cpu->nr_threads = smp_threads;
>      cpu->stopped = true;
[...]
Igor Mammedov - April 9, 2013, 10:26 a.m.
On Mon, 8 Apr 2013 17:13:11 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote:
> > ... and call it if defined from CPUClass.realize() if CPU was hotplugged
> > 
> > by default leave .resume() unset (i.e. NULL) and override it for softmmu
> > in qemu_init_vcpu() if it's still unset.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [...]
> > diff --git a/cpus.c b/cpus.c
> > index 9b9a32f..6b793c5 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
> [...]
> > @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
> >  {
> >      CPUArchState *env = _env;
> >      CPUState *cpu = ENV_GET_CPU(env);
> > +    CPUClass *klass = CPU_GET_CLASS(cpu);
> >  
> > +    if (klass->resume == NULL) {
> > +        klass->resume = resume_vcpu;
> > +    }
> 
> So you are initializing a field of CPUClass struct inside a CPU object
> initialization function. And that's a function that is not even
> converted to QOM yet, and buried inside a non-trivial function call tree
> (hence easy to be called at the wrong time if one day we reorder the
> initialization steps).
init step are not likely to change this drastically but main reason why
it's here see below.

> 
> Can't we do this on class_init(), where it belongs? If we need different
> implementations for softmmu/user, we can add a stub for *-user. I think
> even an explicit #ifdef inside resume_vcpu() would be preferable to
> this.
Generally I agree with you that class_init() would be more correct, but
ifdefs in qom/cpu.c defeat purpose to build qom/cpu.c only once for all
targets, that Andreas are working towards.

It asks surely asks for resume_vcpu() stub in libqemustubs, and I'd do it
there weren't objections to it.

Paolo,
 it looks like a good candidate for libqemustubs, would it be ok to add stub
 there?

> 
> 
> >      cpu->nr_cores = smp_cores;
> >      cpu->nr_threads = smp_threads;
> >      cpu->stopped = true;
> [...]
>
Paolo Bonzini - April 9, 2013, 11:20 a.m.
Il 08/04/2013 22:13, Eduardo Habkost ha scritto:
> So you are initializing a field of CPUClass struct inside a CPU object
> initialization function. And that's a function that is not even
> converted to QOM yet, and buried inside a non-trivial function call tree
> (hence easy to be called at the wrong time if one day we reorder the
> initialization steps).
> 
> Can't we do this on class_init(), where it belongs? If we need different
> implementations for softmmu/user, we can add a stub for *-user.

Yes, please add a stub for the new function and override it in cpus.c.

> I think even an explicit #ifdef inside resume_vcpu() would be
> preferable to this.

Using an #ifdef basically means putting it in exec.c.  I'm not sure
about that, it seems to fit more in cpus.c.

Paolo
Andreas Färber - April 9, 2013, 1:21 p.m.
Am 09.04.2013 12:26, schrieb Igor Mammedov:
> On Mon, 8 Apr 2013 17:13:11 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote:
>>> ... and call it if defined from CPUClass.realize() if CPU was hotplugged
>>>
>>> by default leave .resume() unset (i.e. NULL) and override it for softmmu
>>> in qemu_init_vcpu() if it's still unset.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> [...]
>>> diff --git a/cpus.c b/cpus.c
>>> index 9b9a32f..6b793c5 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -973,6 +973,13 @@ void pause_all_vcpus(void)
>> [...]
>>> @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env)
>>>  {
>>>      CPUArchState *env = _env;
>>>      CPUState *cpu = ENV_GET_CPU(env);
>>> +    CPUClass *klass = CPU_GET_CLASS(cpu);
>>>  
>>> +    if (klass->resume == NULL) {
>>> +        klass->resume = resume_vcpu;
>>> +    }
>>
>> So you are initializing a field of CPUClass struct inside a CPU object
>> initialization function. And that's a function that is not even
>> converted to QOM yet, and buried inside a non-trivial function call tree
>> (hence easy to be called at the wrong time if one day we reorder the
>> initialization steps).
> init step are not likely to change this drastically but main reason why
> it's here see below.
> 
>>
>> Can't we do this on class_init(), where it belongs? If we need different
>> implementations for softmmu/user, we can add a stub for *-user. I think
>> even an explicit #ifdef inside resume_vcpu() would be preferable to
>> this.
> Generally I agree with you that class_init() would be more correct, but
> ifdefs in qom/cpu.c defeat purpose to build qom/cpu.c only once for all
> targets, that Andreas are working towards.

Indeed. Anthony had suggested to introduce a cpu-softmmu.c or something
like that it we ever need to distinguish. Not sure how they would
interface though. ;)

Andreas

> 
> It asks surely asks for resume_vcpu() stub in libqemustubs, and I'd do it
> there weren't objections to it.
> 
> Paolo,
>  it looks like a good candidate for libqemustubs, would it be ok to add stub
>  there?
> 
>>
>>
>>>      cpu->nr_cores = smp_cores;
>>>      cpu->nr_threads = smp_threads;
>>>      cpu->stopped = true;
>> [...]
>>
>
Igor Mammedov - April 10, 2013, 12:57 p.m.
On Tue, 09 Apr 2013 13:20:27 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 08/04/2013 22:13, Eduardo Habkost ha scritto:
> > So you are initializing a field of CPUClass struct inside a CPU object
> > initialization function. And that's a function that is not even
> > converted to QOM yet, and buried inside a non-trivial function call tree
> > (hence easy to be called at the wrong time if one day we reorder the
> > initialization steps).
> > 
> > Can't we do this on class_init(), where it belongs? If we need different
> > implementations for softmmu/user, we can add a stub for *-user.
> 
> Yes, please add a stub for the new function and override it in cpus.c.
Ok, I'll revert it to a way as it was in RFC
http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg03774.html 
minus crept in cpu_synchronize_post_init() line.

There is no much use in introducing resume method when there are
only one implementation and stub.

> 
> > I think even an explicit #ifdef inside resume_vcpu() would be
> > preferable to this.
> 
> Using an #ifdef basically means putting it in exec.c.  I'm not sure
> about that, it seems to fit more in cpus.c.
> 
> Paolo
>

Patch

diff --git a/cpus.c b/cpus.c
index 9b9a32f..6b793c5 100644
--- a/cpus.c
+++ b/cpus.c
@@ -973,6 +973,13 @@  void pause_all_vcpus(void)
     }
 }
 
+static void resume_vcpu(CPUState *cpu)
+{
+    cpu->stop = false;
+    cpu->stopped = false;
+    qemu_cpu_kick(cpu);
+}
+
 void resume_all_vcpus(void)
 {
     CPUArchState *penv = first_cpu;
@@ -980,9 +987,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;
     }
 }
@@ -1042,7 +1047,11 @@  void qemu_init_vcpu(void *_env)
 {
     CPUArchState *env = _env;
     CPUState *cpu = ENV_GET_CPU(env);
+    CPUClass *klass = CPU_GET_CLASS(cpu);
 
+    if (klass->resume == NULL) {
+        klass->resume = resume_vcpu;
+    }
     cpu->nr_cores = smp_cores;
     cpu->nr_threads = smp_threads;
     cpu->stopped = true;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3664a1b..6d6eb7a 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -45,6 +45,7 @@  typedef struct CPUState CPUState;
  * instantiatable CPU type.
  * @reset: Callback to reset the #CPUState to its initial state.
  * @do_interrupt: Callback for interrupt handling.
+ * @resume: Callback for putting CPU in runable state
  * @vmsd: State description for migration.
  *
  * Represents a CPU family or model.
@@ -58,6 +59,7 @@  typedef struct CPUClass {
 
     void (*reset)(CPUState *cpu);
     void (*do_interrupt)(CPUState *cpu);
+    void (*resume)(CPUState *cpu);
 
     const struct VMStateDescription *vmsd;
 } CPUClass;
diff --git a/qom/cpu.c b/qom/cpu.c
index 0c76712..c062e00 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -58,8 +58,13 @@  static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
 
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
+    CPUClass *klass = CPU_GET_CLASS(dev);
+
     if (dev->hotplugged) {
         cpu_synchronize_post_init(CPU(dev));
+        if (klass->resume != NULL) {
+            klass->resume(CPU(dev));
+        }
     }
 }