diff mbox

[1/2] don't start cpu main loop while there is still init work to do.

Message ID 1241037101-24842-2-git-send-email-glommer@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Glauber Costa April 29, 2009, 8:31 p.m. UTC
As soon as we call kvm_init_vcpu(), we start the vcpu thread.
However, there is still things that has to be done, as soon
as the new CPUState is created. Examples include initializing the
apic, halting the cpu, etc.

Without this patch, it is possible that the cpu may want to start
using those things, before initializing them, leading to segfaults.
We introduce another state variable, "initialized", meaning that
the cpu is already created, but not totally initialized,
to serialize it.

Before this patch:
(qemu) cpu_set X online => segfaults ~ 80 % of the time
After this patch:
(qemu) cpu_set X online => works.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/cpu-defs.h |    1 +
 qemu/hw/pc.c    |    1 +
 qemu/qemu-kvm.c |   11 +++++++++++
 3 files changed, 13 insertions(+), 0 deletions(-)

Comments

Avi Kivity May 4, 2009, 8:30 a.m. UTC | #1
Glauber Costa wrote:
> As soon as we call kvm_init_vcpu(), we start the vcpu thread.
> However, there is still things that has to be done, as soon
> as the new CPUState is created. Examples include initializing the
> apic, halting the cpu, etc.
>
> Without this patch, it is possible that the cpu may want to start
> using those things, before initializing them, leading to segfaults.
> We introduce another state variable, "initialized", meaning that
> the cpu is already created, but not totally initialized,
> to serialize it.
>
> Before this patch:
> (qemu) cpu_set X online => segfaults ~ 80 % of the time
> After this patch:
> (qemu) cpu_set X online => works.
>
>   

Is it possible to move all those things to the vcpu thread, so it 
serializes naturally?

I'd like to avoid vcpu ioctls from more than one thread, in case we ever 
move to a syscall implementation.
Glauber Costa May 4, 2009, 2:26 p.m. UTC | #2
On Mon, May 04, 2009 at 11:30:58AM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> As soon as we call kvm_init_vcpu(), we start the vcpu thread.
>> However, there is still things that has to be done, as soon
>> as the new CPUState is created. Examples include initializing the
>> apic, halting the cpu, etc.
>>
>> Without this patch, it is possible that the cpu may want to start
>> using those things, before initializing them, leading to segfaults.
>> We introduce another state variable, "initialized", meaning that
>> the cpu is already created, but not totally initialized,
>> to serialize it.
>>
>> Before this patch:
>> (qemu) cpu_set X online => segfaults ~ 80 % of the time
>> After this patch:
>> (qemu) cpu_set X online => works.
>>
>>   
>
> Is it possible to move all those things to the vcpu thread, so it  
> serializes naturally?
Everything is possible. moving everything to inside cpu_x86_init would be best,
IMHO. We have to remember qemu will have the same problem when kvm gets in there.

However, we might as well remember that cpu_x86_init creates a x86 cpu. It does not
have to be a pc cpu. So initializing apic and the like inside cpu_x86_init could break
this separability. Of course, right now we don't do anything other than pc, so we might
not care. But theorectically...

>
> I'd like to avoid vcpu ioctls from more than one thread, in case we ever  
> move to a syscall implementation.

Although I don't see exactly what's your point in here.
We're just adding a serialization points through pthreads function, not doing any ioctl from
the outside.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity May 4, 2009, 2:33 p.m. UTC | #3
Glauber Costa wrote:
>> I'd like to avoid vcpu ioctls from more than one thread, in case we ever  
>> move to a syscall implementation.
>>     
>
> Although I don't see exactly what's your point in here.
> We're just adding a serialization points through pthreads function, not doing any ioctl from
> the outside.
>   

Doesn't the lapic creation call KVM_CREATE_LAPIC?
Glauber Costa May 4, 2009, 2:44 p.m. UTC | #4
On Mon, May 04, 2009 at 05:33:26PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>>> I'd like to avoid vcpu ioctls from more than one thread, in case we 
>>> ever  move to a syscall implementation.
>>>     
>>
>> Although I don't see exactly what's your point in here.
>> We're just adding a serialization points through pthreads function, not doing any ioctl from
>> the outside.
>>   
>
> Doesn't the lapic creation call KVM_CREATE_LAPIC?
Oh yeah, that.

Maybe we could then move kvm_vcpu_init to the end of pc_new_cpu.

This way we don't break the separability of pc and x86 concepts. We would then issue the lapic creation
ioctl right after the vcpu is created.

How would you feel about it?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity May 4, 2009, 2:48 p.m. UTC | #5
Glauber Costa wrote:
> On Mon, May 04, 2009 at 05:33:26PM +0300, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>>> I'd like to avoid vcpu ioctls from more than one thread, in case we 
>>>> ever  move to a syscall implementation.
>>>>     
>>>>         
>>> Although I don't see exactly what's your point in here.
>>> We're just adding a serialization points through pthreads function, not doing any ioctl from
>>> the outside.
>>>   
>>>       
>> Doesn't the lapic creation call KVM_CREATE_LAPIC?
>>     
> Oh yeah, that.
>
> Maybe we could then move kvm_vcpu_init to the end of pc_new_cpu.
>
> This way we don't break the separability of pc and x86 concepts. We would then issue the lapic creation
> ioctl right after the vcpu is created.
>
> How would you feel about it

Like I said, I'd like to see the lapic creation come from the vcpu thread.
diff mbox

Patch

diff --git a/qemu/cpu-defs.h b/qemu/cpu-defs.h
index f439ac0..83bddad 100644
--- a/qemu/cpu-defs.h
+++ b/qemu/cpu-defs.h
@@ -170,6 +170,7 @@  struct KVMCPUState {
     int stop;
     int stopped;
     int created;
+    int initialized;
     struct qemu_work_item *queued_work_first, *queued_work_last;
 };
 
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 19d75b9..64e6ca5 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -800,6 +800,7 @@  CPUState *pc_new_cpu(int cpu, const char *cpu_model, int pci_enabled)
         if (pci_enabled) {
             apic_init(env);
         }
+    kvm_signal_vcpu_creation(env);
 	return env;
 }
 
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index ed76367..c032618 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -37,6 +37,7 @@  kvm_context_t kvm_context;
 
 pthread_mutex_t qemu_mutex = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t qemu_vcpu_cond = PTHREAD_COND_INITIALIZER;
+pthread_cond_t qemu_vcpu_init_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_system_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_pause_cond = PTHREAD_COND_INITIALIZER;
 pthread_cond_t qemu_work_cond = PTHREAD_COND_INITIALIZER;
@@ -439,12 +440,22 @@  static void *ap_main_loop(void *_env)
     /* and wait for machine initialization */
     while (!qemu_system_ready)
 	qemu_cond_wait(&qemu_system_cond);
+
+    while (!env->kvm_cpu_state.initialized)
+	qemu_cond_wait(&qemu_vcpu_init_cond);
+
     pthread_mutex_unlock(&qemu_mutex);
 
     kvm_main_loop_cpu(env);
     return NULL;
 }
 
+void kvm_signal_vcpu_creation(CPUState *env)
+{
+    env->kvm_cpu_state.initialized = 1;
+    pthread_cond_signal(&qemu_vcpu_init_cond);
+}
+
 void kvm_init_vcpu(CPUState *env)
 {
     pthread_create(&env->kvm_cpu_state.thread, NULL, ap_main_loop, env);