Re: [Qemu-devel] [PATCH 06/22] cpu: introduce CPUClass.resume() method

[Igor Mammedov]

On Mon, 8 Apr 2013 17:13:11 -0300
Eduardo Habkost <address@hidden> 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 <address@hidden>
> [...]
> > 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;
> [...]
>