[PATCH 0/4] Fix cpu hotplug - currently broken

[PATCH 0/4] Fix cpu hotplug - currently broken

[Glauber Costa]

Hello guys,

This patchset has already been sent before, and now is sent again
in a new version. It is refactored to account for the way the new code
looks like, plus addressing a comment from Jan, that an apic function
was called from common code.

I've split it in four patches, for easiness of review/bisect.

hope you like it.

[PATCH 1/4] remove duplicated code.

[Glauber Costa]

This is a leftover from old days. There is already a call
from this function in kvm_main_loop_cpu(), which is called
unconditionally on the end of ap_main_loop. No need to duplicate.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/qemu-kvm.c b/qemu-kvm.c
index fd957d9..d16917f 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -433,7 +433,6 @@ static void *ap_main_loop(void *_env)
     sigfillset(&signals);
     sigprocmask(SIG_BLOCK, &signals, NULL);
     kvm_create_vcpu(kvm_context, env->cpu_index);
-    kvm_qemu_init_env(env);
 
 #ifdef USE_KVM_DEVICE_ASSIGNMENT
     /* do ioperm for io ports of assigned devices */
-- 
1.5.6.6

[PATCH 2/4] only execute lapic load when cpu is already initialized

[Glauber Costa]

In a later patch, we will reorder the execution of vcpu initialization.

After that, the first call to KVM_SET_LAPIC ioctl will not find an
existant vcpu. So we introduce a function that tell us that the vcpu
is already initialized, and is it safe to call the ioctl.

This patch is included first rather than after, so nothing breaks,
and we can keep the tree bisectable.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic.c  |   21 +++++++++++----------
 qemu-kvm.c |    5 +++++
 qemu-kvm.h |    4 ++++
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 21aff15..86aa6b6 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -891,6 +891,15 @@ static void kvm_kernel_lapic_load_from_user(APICState *s)
 
 #endif
 
+void qemu_kvm_load_lapic(CPUState *env)
+{
+#ifdef KVM_CAP_IRQCHIP
+    if (kvm_enabled() && kvm_vcpu_inited(env) && qemu_kvm_irqchip_in_kernel()) {
+        kvm_kernel_lapic_load_from_user(env->apic_state);
+    }
+#endif
+}
+
 static void apic_save(QEMUFile *f, void *opaque)
 {
     APICState *s = opaque;
@@ -965,11 +974,7 @@ static int apic_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id >= 2)
         qemu_get_timer(f, s->timer);
 
-#ifdef KVM_CAP_IRQCHIP
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_lapic_load_from_user(s);
-    }
-#endif
+    qemu_kvm_load_lapic(s->cpu_env);
 
     return 0;
 }
@@ -991,11 +996,7 @@ static void apic_reset(void *opaque)
          */
         s->lvt[APIC_LVT_LINT0] = 0x700;
     }
-#ifdef KVM_CAP_IRQCHIP
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_lapic_load_from_user(s);
-    }
-#endif
+    qemu_kvm_load_lapic(s->cpu_env);
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {
diff --git a/qemu-kvm.c b/qemu-kvm.c
index d16917f..68d3b92 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -462,6 +462,11 @@ void kvm_init_vcpu(CPUState *env)
 	qemu_cond_wait(&qemu_vcpu_cond);
 }
 
+int kvm_vcpu_inited(CPUState *env)
+{
+    return env->kvm_cpu_state.created;
+}
+
 int kvm_init_ap(void)
 {
 #ifdef TARGET_I386
diff --git a/qemu-kvm.h b/qemu-kvm.h
index e74ab90..725589b 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -16,6 +16,7 @@ int kvm_main_loop(void);
 int kvm_qemu_init(void);
 int kvm_qemu_create_context(void);
 int kvm_init_ap(void);
+int kvm_vcpu_inited(CPUState *env);
 void kvm_qemu_destroy(void);
 void kvm_load_registers(CPUState *env);
 void kvm_save_registers(CPUState *env);
@@ -31,6 +32,9 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap);
 int kvm_qemu_init_env(CPUState *env);
 int kvm_qemu_check_extension(int ext);
 void kvm_apic_init(CPUState *env);
+/* called from vcpu initialization */
+void qemu_kvm_load_lapic(CPUState *env);
+
 int kvm_set_irq(int irq, int level, int *status);
 
 int kvm_physical_memory_set_dirty_tracking(int enable);
-- 
1.5.6.6

[PATCH 3/4] move kvm_trim_features where it belongs.

[Glauber Costa]

We also kill the unused name argument. It would
introduce an non-necessary depedency on code present at
helper.c

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu-kvm-x86.c       |   23 +++++++++++++++++++++++
 target-i386/helper.c |   29 -----------------------------
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index bbe0312..fcb594c 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -503,6 +503,19 @@ static int get_para_features(kvm_context_t kvm_context)
 	return features;
 }
 
+static void kvm_trim_features(uint32_t *features, uint32_t supported)
+{
+    int i;
+    uint32_t mask;
+
+    for (i = 0; i < 32; ++i) {
+        mask = 1U << i;
+        if ((*features & mask) && !(supported & mask)) {
+            *features &= ~mask;
+        }
+    }
+}
+
 int kvm_arch_qemu_init_env(CPUState *cenv)
 {
     struct kvm_cpuid_entry2 cpuid_ent[100];
@@ -566,6 +579,16 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
 	do_cpuid_ent(&cpuid_ent[cpuid_nent++], i, 0, &copy);
 
     kvm_setup_cpuid2(kvm_context, cenv->cpu_index, cpuid_nent, cpuid_ent);
+
+    kvm_trim_features(&cenv->cpuid_features,
+                      kvm_arch_get_supported_cpuid(cenv, 1, R_EDX));
+    kvm_trim_features(&cenv->cpuid_ext_features,
+                      kvm_arch_get_supported_cpuid(cenv, 1, R_ECX));
+    kvm_trim_features(&cenv->cpuid_ext2_features,
+                      kvm_arch_get_supported_cpuid(cenv, 0x80000001, R_EDX));
+    kvm_trim_features(&cenv->cpuid_ext3_features,
+                      kvm_arch_get_supported_cpuid(cenv, 0x80000001, R_ECX));
+
     return 0;
 }
 
diff --git a/target-i386/helper.c b/target-i386/helper.c
index ed2dc41..848d05b 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -94,20 +94,6 @@ static void add_flagname_to_bitmaps(char *flagname, uint32_t *features,
     }
 }
 
-static void kvm_trim_features(uint32_t *features, uint32_t supported,
-                              const char *names[])
-{
-    int i;
-    uint32_t mask;
-
-    for (i = 0; i < 32; ++i) {
-        mask = 1U << i;
-        if ((*features & mask) && !(supported & mask)) {
-            *features &= ~mask;
-        }
-    }
-}
-
 typedef struct x86_def_t {
     const char *name;
     uint32_t level;
@@ -1715,20 +1701,5 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
 
     qemu_init_vcpu(env);
 
-    if (kvm_enabled()) {
-        kvm_trim_features(&env->cpuid_features,
-                          kvm_arch_get_supported_cpuid(env, 1, R_EDX),
-                          feature_name);
-        kvm_trim_features(&env->cpuid_ext_features,
-                          kvm_arch_get_supported_cpuid(env, 1, R_ECX),
-                          ext_feature_name);
-        kvm_trim_features(&env->cpuid_ext2_features,
-                          kvm_arch_get_supported_cpuid(env, 0x80000001, R_EDX),
-                          ext2_feature_name);
-        kvm_trim_features(&env->cpuid_ext3_features,
-                          kvm_arch_get_supported_cpuid(env, 0x80000001, R_ECX),
-                          ext3_feature_name);
-    }
-
     return env;
 }
-- 
1.5.6.6

[PATCH 4/4] make sure kvm_vpu_init is the last thing called in cpu initialization

[Glauber Costa]

KVM access some state that is only present late in cpu initialization.
This happens in kvm_vcpu_init(). APIC is an example of that.

So we have to make sure that kvm_vcpu_init is the last thing called
in the initialization process.

We also have to explicitly call qemu_kvm_load_apic(), since the
first call will find the vcpu non initialized.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c              |    5 +++++
 qemu-kvm-x86.c       |    2 ++
 target-i386/helper.c |    2 --
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 9e99b7c..45de6d9 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -844,6 +844,11 @@ CPUState *pc_new_cpu(int cpu, const char *cpu_model, int pci_enabled)
         if (pci_enabled) {
             apic_init(env);
         }
+
+    /* kvm needs this to run after the apic is initialized. Otherwise,
+     * it can access invalid state and crash.
+     */
+    qemu_init_vcpu(env);
 	return env;
 }
 
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index fcb594c..98aa530 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -527,6 +527,8 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
     CPUState copy;
     uint32_t i, j, limit;
 
+    qemu_kvm_load_lapic(cenv);
+
     copy = *cenv;
 
 #ifdef KVM_CPUID_SIGNATURE
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 848d05b..6dc0111 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1699,7 +1699,5 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
     kqemu_init(env);
 #endif
 
-    qemu_init_vcpu(env);
-
     return env;
 }
-- 
1.5.6.6

Re: [PATCH 0/4] Fix cpu hotplug - currently broken

[Avi Kivity]

Glauber Costa wrote:
> Hello guys,
>
> This patchset has already been sent before, and now is sent again
> in a new version. It is refactored to account for the way the new code
> looks like, plus addressing a comment from Jan, that an apic function
> was called from common code.
>
> I've split it in four patches, for easiness of review/bisect.
>   

Thanks.

> hope you like it.
>   

I did, so I applied it all.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/4] Fix cpu hotplug - currently broken

[Glauber Costa]

On Sun, May 31, 2009 at 12:40:40PM +0300, Avi Kivity wrote:
>> hope you like it.
>>   
>
> I did, so I applied it all.
my man...