| Message ID | 1425569930-6660-7-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > > object_property_add(obj, "family", "int", > x86_cpuid_version_get_family,
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
On Thu, Mar 05, 2015 at 05:44:58PM +0100, Andreas Färber wrote: > Am 05.03.2015 um 17:42 schrieb Igor Mammedov: [...] > >> @@ -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. ;) Which is already queued at the x86 tree mentioned in the cover letter, BTW: https://github.com/ehabkost/qemu.git x86
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); object_property_add(obj, "family", "int", x86_cpuid_version_get_family,