Hi Glauber, On Fri, Apr 17, 2009 at 01:15:21AM -0400, Glauber Costa wrote: > As KVM cpus runs on threads, it is possible that > we call kvm_load_registers() from a cpu thread, while the > apic has not yet fully initialized. kvm_load_registers() is called > from ap_main_loop. > > This is not a problem when we're starting the whole machine together, > but is a problem for hotplug, since we don't have the protection > of the locks that protect machine initialization. Currently, some executions > of cpu hotplug on rainy sundays fail with a segfault. /* and wait for machine initialization */ while (!qemu_system_ready) qemu_cond_wait(&qemu_system_cond); pthread_mutex_unlock(&qemu_mutex); Shouldnt this cover the cpu hotplug case too? Perhaps have: /* wait for machine initialization */ while (!qemu_system_ready) qemu_cond_wait(&qemu_system_cond); /* wait for vcpu initialization */ while (!env->initialized) qemu_cond_wait(&qemu_system_cond); pthread_mutex_unlock(&qemu_mutex); And then set env->initialized when the cpu is good to go. Because there could be other dependencies other than APIC initialization, for eg in pc_new_cpu if (cpu != 0) env->halted = 1; > Moving apic initialization to before kvm_init_vpcu proved fruitful, > as there are some dependencies involved. (kvm irqchip would fail to > initialize). > > This patch provides default values to be used for tpr and apic_base, > that will be returned when the apic is not yet properly initialized. > It is aimed at kvm, where the problem exists, but it could equally be > used for qemu too, if there is agreement. > > Signed-off-by: Glauber Costa <glommer@xxxxxxxxxx> > --- > qemu/hw/apic.c | 12 ++++++++++-- > 1 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c > index b926508..06fb9b5 100644 > --- a/qemu/hw/apic.c > +++ b/qemu/hw/apic.c > @@ -301,7 +301,12 @@ uint64_t cpu_get_apic_base(CPUState *env) > #ifdef DEBUG_APIC > printf("cpu_get_apic_base: %016" PRIx64 "\n", (uint64_t)s->apicbase); > #endif > - return s->apicbase; > + if (s) { > + return s->apicbase; > + } > + else { > + return 0xfee00000 | MSR_IA32_APICBASE_ENABLE; > + } > } > > void cpu_set_apic_tpr(CPUX86State *env, uint8_t val) > @@ -314,7 +319,10 @@ void cpu_set_apic_tpr(CPUX86State *env, uint8_t val) > uint8_t cpu_get_apic_tpr(CPUX86State *env) > { > APICState *s = env->apic_state; > - return s->tpr >> 4; > + if (s) > + return s->tpr >> 4; > + else > + return 0; > } > > /* return -1 if no bit is set */ > -- > 1.5.6.6 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html