[Qemu-devel] Re: [PATCH] return default values for apic probe functions.

[Marcelo Tosatti]

On Fri, Apr 17, 2009 at 10:59:11AM -0300, Glauber Costa wrote:
> On Fri, Apr 17, 2009 at 10:53 AM, Marcelo Tosatti <address@hidden> wrote:
> > 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.
> >From my understanding, all this is only useful when the whole machine
> is starting, since they are global locks that wait for a system wide 
> condition.
> 
> This is not the case with cpu hotplug, since the box is already on.

Yes, but the above proposes the addition of vcpu "initialized" state
in addition to the system wide state, which would fix the current cpu
hotplug breakage with apic, or any other access to uninitialized state
from the vcpu thread.

> > Because there could be other dependencies other than APIC
> > initialization, for eg in pc_new_cpu
> >
> >        if (cpu != 0)
> >            env->halted = 1;
> 
> it is okay for the cpu to be halted. Btw, I believe this should be
> moved inside cpu init.

Point here is the vcpu might not be halted by the time the vcpu thread
starts to run, while it should (ie: its an example of uninitialized
state other than the apic).