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

["Andreas Färber" <afaerber@suse.de>]

Am 05.03.2015 um 17:42 schrieb Igor Mammedov:
> On Thu,  5 Mar 2015 12:38:50 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
>> ---
>>  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

Being tackled in Eduardo's APIC series. ;)

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)