Re: [Qemu-devel] [PATCH 6/6] target-i386: Call cpu_exec_init() on realize

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:50 -0300
Eduardo Habkost <address@hidden> wrote:

> To allow new code to ask the CPU classes for CPU model information and
> allow QOM properties to be queried by qmp_device_list_properties(), we
> need to be able to safely instantiate a X86CPU object without any
> side-effects.
> 
> cpu_exec_init() has lots of side-effects on global QEMU state, move it
> to realize so it will be called only if the X86CPU instance is realized.
> 
> For reference, this is the current cpu_exec_init() code:
> 
> > void cpu_exec_init(CPUArchState *env)
> > {
> >     CPUState *cpu = ENV_GET_CPU(env);
> >     CPUClass *cc = CPU_GET_CLASS(cpu);
> >     CPUState *some_cpu;
> >     int cpu_index;
> >
> > #ifndef CONFIG_USER_ONLY
> >     cpu->as = &address_space_memory;
> >     cpu->thread_id = qemu_get_thread_id();
> > #endif
> 
> Those fields should be used only after actually starting the VCPU and can be
> initialized on realize.
> 
> >
> > #if defined(CONFIG_USER_ONLY)
> >     cpu_list_lock();
> > #endif
> >     cpu_index = 0;
> >     CPU_FOREACH(some_cpu) {
> >         cpu_index++;
> >     }
> >     cpu->cpu_index = cpu_index;
> >     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
> > #if defined(CONFIG_USER_ONLY)
> >     cpu_list_unlock();
> > #endif
> 
> The above initializes cpu_index and add the CPU to the global CPU list.
> This affects QEMU global state and must be done only on realize.
> 
> >     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> >         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
> >     }
> > #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> >     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
> >                     cpu_save, cpu_load, env);
> >     assert(cc->vmsd == NULL);
> >     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
> > #endif
> >     if (cc->vmsd != NULL) {
> >         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
> >     }
> 
> vmstate and savevm registration also affects global QEMU state and should be
> done only on realize.
> 
> > }
> 
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 400b1e0..8b76604 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2758,6 +2758,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error 
> **errp)
>      static bool ht_warned;
>      static bool tcg_initialized;
>  
> +    cpu_exec_init(env);
> +
>      if (tcg_enabled() && !tcg_initialized) {
>          tcg_initialized = 1;
>          tcg_x86_init();
> @@ -2840,7 +2842,6 @@ static void x86_cpu_initfn(Object *obj)
>      CPUX86State *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(env);
looks wrong, later in this function we do
 env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
and with this patch will always yield 0

>  
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,