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

[Eduardo Habkost]

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

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;
[...]

-- 
Eduardo