[PATCH 0/2] Fix cpu hotplug in upstream kvm

[PATCH 0/2] Fix cpu hotplug in upstream kvm

[Glauber Costa]

Hello.

Here are a couple of fixes for upstream cpu_hotplug. (currently broken)
After the first patch, it ceases to segfault, and will work with -no-kvm-irqchip.

The second one fixes the issues with kvm-irqchip

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

[Glauber Costa]

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(-)

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);
-- 
1.5.6.6

[PATCH 2/2] Present kvm with corret apic phys id.

[Glauber Costa]

KVM will 24-shift bits in addr 0x20 (APIC_ID) before actually
using it. We currently load phys_id as "s->id". After shifted
by 24 bits, it will result in a meaningless value. We should really
be doing "s->id << 24", which, after shifted, will lead to the correct
value.

This is for the load function. save has the invert problem.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/hw/apic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu/hw/apic.c b/qemu/hw/apic.c
index b926508..0ace5cc 100644
--- a/qemu/hw/apic.c
+++ b/qemu/hw/apic.c
@@ -830,7 +830,7 @@ static void kvm_kernel_lapic_save_to_user(APICState *s)
 
     kvm_get_lapic(kvm_context, s->cpu_env->cpu_index, kapic);
 
-    s->id = kapic_reg(kapic, 0x2);
+    s->id = kapic_reg(kapic, 0x2) >> 24;
     s->tpr = kapic_reg(kapic, 0x8);
     s->arb_id = kapic_reg(kapic, 0x9);
     s->log_dest = kapic_reg(kapic, 0xd) >> 24;
@@ -863,7 +863,7 @@ static void kvm_kernel_lapic_load_from_user(APICState *s)
     int i;
 
     memset(klapic, 0, sizeof apic);
-    kapic_set_reg(klapic, 0x2, s->id);
+    kapic_set_reg(klapic, 0x2, s->id << 24);
     kapic_set_reg(klapic, 0x8, s->tpr);
     kapic_set_reg(klapic, 0xd, s->log_dest << 24);
     kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
-- 
1.5.6.6

Re: [PATCH 2/2] Present kvm with corret apic phys id.

[Avi Kivity]

Glauber Costa wrote:
> KVM will 24-shift bits in addr 0x20 (APIC_ID) before actually
> using it. We currently load phys_id as "s->id". After shifted
> by 24 bits, it will result in a meaningless value. We should really
> be doing "s->id << 24", which, after shifted, will lead to the correct
> value.
>
> This is for the load function. save has the invert problem.
>   

Applied, thanks.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Avi Kivity]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Glauber Costa]

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.

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

[Avi Kivity]

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?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Glauber Costa]

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?

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

[Avi Kivity]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.