[PATCH 0/4] Provide kvm-free implementations of apic/ioapic

[PATCH 0/4] Provide kvm-free implementations of apic/ioapic

[Glauber Costa]

In the same way as the previous i8259 patch, this patch cleans up
the apic and ioapic code to provide an implementation that is kvm free.

This reduces the impact of kvm on normal qemu. Also, provides a simpler
code base for kvm devices.

[PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

This is not kvm specific, and should do fine in plain qemu

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

diff --git a/hw/apic.c b/hw/apic.c
index 86aa6b6..2eddba0 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s)
 
     cpu_reset(s->cpu_env);
 
-    if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
-        (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
+    if (!(s->apicbase & MSR_IA32_APICBASE_BSP))
         s->cpu_env->halted = 1;
 
     if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
-- 
1.5.6.6

[PATCH 2/4] sipi and init: move common code

[Glauber Costa]

provide functions to query and reset the state of sipi and
init in cpu's apic. This way we can move the kvm specific functions
out of the apic path.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 cpu-defs.h |    2 --
 hw/apic.c  |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
 qemu-kvm.c |   26 ++++++--------------------
 qemu-kvm.h |    7 +++++--
 4 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/cpu-defs.h b/cpu-defs.h
index 1e071e7..e66e928 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -140,8 +140,6 @@ typedef struct CPUWatchpoint {
 struct qemu_work_item;
 
 struct KVMCPUState {
-    int sipi_needed;
-    int init;
     pthread_t thread;
     int signalled;
     int stop;
diff --git a/hw/apic.c b/hw/apic.c
index 2eddba0..c93cbb3 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -86,6 +86,8 @@ typedef struct APICState {
     uint32_t initial_count;
     int64_t initial_count_load_time, next_time;
     QEMUTimer *timer;
+    int sipi_needed;
+    int init;
 } APICState;
 
 static int apic_io_memory;
@@ -443,6 +445,45 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
     }
 }
 
+int apic_init_received(CPUState *env)
+{
+    if (!env)
+        return 0;
+    if (!env->apic_state)
+        return 0;
+
+    return env->apic_state->init;
+}
+
+int apic_sipi_needed(CPUState *env)
+{
+    if (!env)
+        return 0;
+    if (!env->apic_state)
+        return 0;
+
+    return env->apic_state->sipi_needed;
+}
+
+void apic_reset_sipi(CPUState *env)
+{
+    if (!env)
+        return;
+    if (!env->apic_state)
+        return;
+
+    env->apic_state->sipi_needed = 0;
+}
+
+void apic_reset_init(CPUState *env)
+{
+    if (!env)
+        return;
+    if (!env->apic_state)
+        return;
+
+    env->apic_state->init = 0;
+}
 
 static void apic_init_ipi(APICState *s)
 {
@@ -470,9 +511,8 @@ static void apic_init_ipi(APICState *s)
     if (!(s->apicbase & MSR_IA32_APICBASE_BSP))
         s->cpu_env->halted = 1;
 
-    if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
-	if (s->cpu_env)
-	    kvm_apic_init(s->cpu_env);
+    if (s->cpu_env && s->cpu_env->cpu_index != 0)
+	    s->init = 1;
 }
 
 /* send a SIPI message to the CPU to start it */
@@ -485,8 +525,7 @@ static void apic_startup(APICState *s, int vector_num)
     cpu_x86_load_seg_cache(env, R_CS, vector_num << 8, vector_num << 12,
                            0xffff, 0);
     env->halted = 0;
-    if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
-	kvm_update_after_sipi(env);
+    s->sipi_needed = 1;
 }
 
 static void apic_deliver(APICState *s, uint8_t dest, uint8_t dest_mode,
diff --git a/qemu-kvm.c b/qemu-kvm.c
index 68d3b92..1441cef 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -134,19 +134,6 @@ void kvm_update_interrupt_request(CPUState *env)
     }
 }
 
-void kvm_update_after_sipi(CPUState *env)
-{
-    env->kvm_cpu_state.sipi_needed = 1;
-    kvm_update_interrupt_request(env);
-}
-
-void kvm_apic_init(CPUState *env)
-{
-    if (env->cpu_index != 0)
-	env->kvm_cpu_state.init = 1;
-    kvm_update_interrupt_request(env);
-}
-
 #include <signal.h>
 
 static int try_push_interrupts(void *opaque)
@@ -332,7 +319,7 @@ static void kvm_vm_state_change_handler(void *context, int running, int reason)
 static void update_regs_for_sipi(CPUState *env)
 {
     kvm_arch_update_regs_for_sipi(env);
-    env->kvm_cpu_state.sipi_needed = 0;
+    apic_reset_sipi(env);
 }
 
 static void update_regs_for_init(CPUState *env)
@@ -345,11 +332,10 @@ static void update_regs_for_init(CPUState *env)
 
 #ifdef TARGET_I386
     /* restore SIPI vector */
-    if(env->kvm_cpu_state.sipi_needed)
+    if (apic_sipi_needed(env))
         env->segs[R_CS] = cs;
 #endif
-
-    env->kvm_cpu_state.init = 0;
+    apic_reset_init(env);
     kvm_arch_load_regs(env);
 }
 
@@ -407,12 +393,12 @@ static int kvm_main_loop_cpu(CPUState *env)
 	if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI))
 	    env->halted = 0;
     if (!kvm_irqchip_in_kernel(kvm_context)) {
-	    if (env->kvm_cpu_state.init)
+	    if (apic_init_received(env))
 	        update_regs_for_init(env);
-	    if (env->kvm_cpu_state.sipi_needed)
+	    if (apic_sipi_needed(env))
 	        update_regs_for_sipi(env);
     }
-	if (!env->halted && !env->kvm_cpu_state.init)
+	if (!env->halted)
 	    kvm_cpu_exec(env);
 	env->exit_request = 0;
         env->exception_index = EXCP_INTERRUPT;
diff --git a/qemu-kvm.h b/qemu-kvm.h
index 725589b..eb7dc29 100644
--- a/qemu-kvm.h
+++ b/qemu-kvm.h
@@ -31,9 +31,13 @@ void kvm_remove_all_breakpoints(CPUState *current_env);
 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);
+/* FIXME: there should be an apic.h file */
 /* called from vcpu initialization */
 void qemu_kvm_load_lapic(CPUState *env);
+int apic_init_received(CPUState *env);
+int apic_sipi_needed(CPUState *env);
+void apic_reset_sipi(CPUState *env);
+void apic_reset_init(CPUState *env);
 
 int kvm_set_irq(int irq, int level, int *status);
 
@@ -44,7 +48,6 @@ int kvm_get_phys_ram_page_bitmap(unsigned char *bitmap);
 void qemu_kvm_call_with_env(void (*func)(void *), void *data, CPUState *env);
 void qemu_kvm_cpuid_on_env(CPUState *env);
 void kvm_inject_interrupt(CPUState *env, int mask);
-void kvm_update_after_sipi(CPUState *env);
 void kvm_update_interrupt_request(CPUState *env);
 void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
-- 
1.5.6.6

[PATCH 3/4] provide a kvm-free implementation of apic

[Glauber Costa]

Also, provide a kvm_apic that does not depend highly on common code.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic.c      |  249 ++++++++++++++++++++++++++++++++++----------------------
 hw/pc.c        |    7 ++-
 hw/pc.h        |    1 +
 qemu-kvm-x86.c |    5 +-
 4 files changed, 162 insertions(+), 100 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index c93cbb3..d7af08f 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -852,103 +852,11 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
     }
 }
 
-#ifdef KVM_CAP_IRQCHIP
-
-static inline uint32_t kapic_reg(struct kvm_lapic_state *kapic, int reg_id)
-{
-    return *((uint32_t *) (kapic->regs + (reg_id << 4)));
-}
-
-static inline void kapic_set_reg(struct kvm_lapic_state *kapic,
-                                 int reg_id, uint32_t val)
-{
-    *((uint32_t *) (kapic->regs + (reg_id << 4))) = val;
-}
-
-static void kvm_kernel_lapic_save_to_user(APICState *s)
-{
-    struct kvm_lapic_state apic;
-    struct kvm_lapic_state *kapic = &apic;
-    int i, v;
-
-    kvm_get_lapic(kvm_context, s->cpu_env->cpu_index, kapic);
-
-    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;
-    s->dest_mode = kapic_reg(kapic, 0xe) >> 28;
-    s->spurious_vec = kapic_reg(kapic, 0xf);
-    for (i = 0; i < 8; i++) {
-        s->isr[i] = kapic_reg(kapic, 0x10 + i);
-        s->tmr[i] = kapic_reg(kapic, 0x18 + i);
-        s->irr[i] = kapic_reg(kapic, 0x20 + i);
-    }
-    s->esr = kapic_reg(kapic, 0x28);
-    s->icr[0] = kapic_reg(kapic, 0x30);
-    s->icr[1] = kapic_reg(kapic, 0x31);
-    for (i = 0; i < APIC_LVT_NB; i++)
-	s->lvt[i] = kapic_reg(kapic, 0x32 + i);
-    s->initial_count = kapic_reg(kapic, 0x38);
-    s->divide_conf = kapic_reg(kapic, 0x3e);
-
-    v = (s->divide_conf & 3) | ((s->divide_conf >> 1) & 4);
-    s->count_shift = (v + 1) & 7;
-
-    s->initial_count_load_time = qemu_get_clock(vm_clock);
-    apic_timer_update(s, s->initial_count_load_time);
-}
-
-static void kvm_kernel_lapic_load_from_user(APICState *s)
-{
-    struct kvm_lapic_state apic;
-    struct kvm_lapic_state *klapic = &apic;
-    int i;
-
-    memset(klapic, 0, sizeof apic);
-    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);
-    kapic_set_reg(klapic, 0xf, s->spurious_vec);
-    for (i = 0; i < 8; i++) {
-        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
-        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
-        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
-    }
-    kapic_set_reg(klapic, 0x28, s->esr);
-    kapic_set_reg(klapic, 0x30, s->icr[0]);
-    kapic_set_reg(klapic, 0x31, s->icr[1]);
-    for (i = 0; i < APIC_LVT_NB; i++)
-        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
-    kapic_set_reg(klapic, 0x38, s->initial_count);
-    kapic_set_reg(klapic, 0x3e, s->divide_conf);
-
-    kvm_set_lapic(kvm_context, s->cpu_env->cpu_index, klapic);
-}
-
-#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)
+static void apic_save_common(QEMUFile *f, void *opaque)
 {
     APICState *s = opaque;
     int i;
 
-#ifdef KVM_CAP_IRQCHIP
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_lapic_save_to_user(s);
-    }
-#endif
-
     qemu_put_be32s(f, &s->apicbase);
     qemu_put_8s(f, &s->id);
     qemu_put_8s(f, &s->arb_id);
@@ -976,7 +884,12 @@ static void apic_save(QEMUFile *f, void *opaque)
     qemu_put_timer(f, s->timer);
 }
 
-static int apic_load(QEMUFile *f, void *opaque, int version_id)
+static void apic_save(QEMUFile *f, void *opaque)
+{
+    apic_save_common(f, opaque);
+}
+
+static int apic_load_common(QEMUFile *f, void *opaque, int version_id)
 {
     APICState *s = opaque;
     int i;
@@ -1012,12 +925,15 @@ static int apic_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id >= 2)
         qemu_get_timer(f, s->timer);
 
-    qemu_kvm_load_lapic(s->cpu_env);
-
     return 0;
 }
 
-static void apic_reset(void *opaque)
+static int apic_load(QEMUFile *f, void *opaque, int version_id)
+{
+    return apic_load_common(f, opaque, version_id);
+}
+
+static void apic_reset_common(void *opaque)
 {
     APICState *s = opaque;
 
@@ -1034,7 +950,11 @@ static void apic_reset(void *opaque)
          */
         s->lvt[APIC_LVT_LINT0] = 0x700;
     }
-    qemu_kvm_load_lapic(s->cpu_env);
+}
+
+static void apic_reset(void *opaque)
+{
+    apic_reset_common(opaque);
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {
@@ -1081,3 +1001,136 @@ int apic_init(CPUState *env)
     return 0;
 }
 
+#ifdef KVM_CAP_IRQCHIP
+
+static inline uint32_t kapic_reg(struct kvm_lapic_state *kapic, int reg_id)
+{
+    return *((uint32_t *) (kapic->regs + (reg_id << 4)));
+}
+
+static inline void kapic_set_reg(struct kvm_lapic_state *kapic,
+                                 int reg_id, uint32_t val)
+{
+    *((uint32_t *) (kapic->regs + (reg_id << 4))) = val;
+}
+
+static void kvm_kernel_lapic_save_to_user(APICState *s)
+{
+    struct kvm_lapic_state apic;
+    struct kvm_lapic_state *kapic = &apic;
+    int i, v;
+
+    kvm_get_lapic(kvm_context, s->cpu_env->cpu_index, kapic);
+
+    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;
+    s->dest_mode = kapic_reg(kapic, 0xe) >> 28;
+    s->spurious_vec = kapic_reg(kapic, 0xf);
+    for (i = 0; i < 8; i++) {
+        s->isr[i] = kapic_reg(kapic, 0x10 + i);
+        s->tmr[i] = kapic_reg(kapic, 0x18 + i);
+        s->irr[i] = kapic_reg(kapic, 0x20 + i);
+    }
+    s->esr = kapic_reg(kapic, 0x28);
+    s->icr[0] = kapic_reg(kapic, 0x30);
+    s->icr[1] = kapic_reg(kapic, 0x31);
+    for (i = 0; i < APIC_LVT_NB; i++)
+	s->lvt[i] = kapic_reg(kapic, 0x32 + i);
+    s->initial_count = kapic_reg(kapic, 0x38);
+    s->divide_conf = kapic_reg(kapic, 0x3e);
+
+    v = (s->divide_conf & 3) | ((s->divide_conf >> 1) & 4);
+    s->count_shift = (v + 1) & 7;
+
+    s->initial_count_load_time = qemu_get_clock(vm_clock);
+    apic_timer_update(s, s->initial_count_load_time);
+}
+
+static void kvm_kernel_lapic_load_from_user(APICState *s)
+{
+    struct kvm_lapic_state apic;
+    struct kvm_lapic_state *klapic = &apic;
+    int i;
+
+    memset(klapic, 0, sizeof apic);
+    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);
+    kapic_set_reg(klapic, 0xf, s->spurious_vec);
+    for (i = 0; i < 8; i++) {
+        kapic_set_reg(klapic, 0x10 + i, s->isr[i]);
+        kapic_set_reg(klapic, 0x18 + i, s->tmr[i]);
+        kapic_set_reg(klapic, 0x20 + i, s->irr[i]);
+    }
+    kapic_set_reg(klapic, 0x28, s->esr);
+    kapic_set_reg(klapic, 0x30, s->icr[0]);
+    kapic_set_reg(klapic, 0x31, s->icr[1]);
+    for (i = 0; i < APIC_LVT_NB; i++)
+        kapic_set_reg(klapic, 0x32 + i, s->lvt[i]);
+    kapic_set_reg(klapic, 0x38, s->initial_count);
+    kapic_set_reg(klapic, 0x3e, s->divide_conf);
+
+    kvm_set_lapic(kvm_context, s->cpu_env->cpu_index, klapic);
+}
+
+#endif
+
+static void kvm_apic_save(QEMUFile *f, void *opaque)
+{
+    APICState *s = opaque;
+
+    kvm_kernel_lapic_save_to_user(s);
+    apic_save_common(f, opaque);
+}
+
+static int kvm_apic_load(QEMUFile *f, void *opaque, int version_id)
+{
+    APICState *s = opaque;
+    int r = apic_load_common(f, opaque, version_id);
+
+    if (r == 0)
+        qemu_kvm_load_lapic(s->cpu_env);
+    return r;
+}
+
+
+void qemu_kvm_load_lapic(CPUState *env)
+{
+    if (kvm_vcpu_inited(env)) {
+        kvm_kernel_lapic_load_from_user(env->apic_state);
+    }
+}
+
+static void kvm_apic_reset(void *opaque)
+{
+    APICState *s = opaque;
+
+    apic_reset_common(opaque);
+    qemu_kvm_load_lapic(s->cpu_env);
+}
+
+int kvm_apic_init(CPUState *env)
+{
+    APICState *s;
+
+    if (last_apic_id >= MAX_APICS)
+        return -1;
+    s = qemu_mallocz(sizeof(APICState));
+    env->apic_state = s;
+    s->id = last_apic_id++;
+    env->cpuid_apic_id = s->id;
+    s->cpu_env = env;
+
+    kvm_apic_reset(s);
+
+    s->timer = qemu_new_timer(vm_clock, apic_timer, s);
+
+    register_savevm("apic", s->id, 2, kvm_apic_save, kvm_apic_load, s);
+    qemu_register_reset(kvm_apic_reset, 0, s);
+
+    local_apics[s->id] = s;
+    return 0;
+}
diff --git a/hw/pc.c b/hw/pc.c
index 66f4635..a99ab07 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -842,7 +842,12 @@ CPUState *pc_new_cpu(int cpu, const char *cpu_model, int pci_enabled)
         }
         qemu_register_reset(main_cpu_reset, 0, env);
         if (pci_enabled) {
-            apic_init(env);
+#ifdef KVM_CAP_IRQCHIP
+            if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
+                kvm_apic_init(env);
+            } else
+#endif
+                apic_init(env);
         }
 
     /* kvm needs this to run after the apic is initialized. Otherwise,
diff --git a/hw/pc.h b/hw/pc.h
index 3af22f2..ab847b9 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -48,6 +48,7 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
                              uint8_t vector_num, uint8_t polarity,
                              uint8_t trigger_mode);
 int apic_init(CPUState *env);
+int kvm_apic_init(CPUState *env);
 int apic_accept_pic_intr(CPUState *env);
 void apic_deliver_pic_intr(CPUState *env, int level);
 int apic_get_interrupt(CPUState *env);
diff --git a/qemu-kvm-x86.c b/qemu-kvm-x86.c
index 98aa530..925d70e 100644
--- a/qemu-kvm-x86.c
+++ b/qemu-kvm-x86.c
@@ -527,7 +527,10 @@ int kvm_arch_qemu_init_env(CPUState *cenv)
     CPUState copy;
     uint32_t i, j, limit;
 
-    qemu_kvm_load_lapic(cenv);
+#ifdef KVM_CAP_IRQCHIP
+    if (qemu_kvm_irqchip_in_kernel())
+        qemu_kvm_load_lapic(cenv);
+#endif
 
     copy = *cenv;
 
-- 
1.5.6.6

[PATCH 4/4] provide a kvm-free implementation of ioapic

[Glauber Costa]

Also, provide a kvm_ioapic that does not depend highly on common code.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/ioapic.c |  162 +++++++++++++++++++++++++++++++++++++---------------------
 hw/pc.c     |    7 ++-
 hw/pc.h     |    1 +
 3 files changed, 110 insertions(+), 60 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index 6c178c7..3eb5d5f 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -198,58 +198,11 @@ static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t va
     }
 }
 
-static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
-{
-#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
-    struct kvm_irqchip chip;
-    struct kvm_ioapic_state *kioapic;
-    int i;
-
-    chip.chip_id = KVM_IRQCHIP_IOAPIC;
-    kvm_get_irqchip(kvm_context, &chip);
-    kioapic = &chip.chip.ioapic;
-
-    s->id = kioapic->id;
-    s->ioregsel = kioapic->ioregsel;
-    s->base_address = kioapic->base_address;
-    s->irr = kioapic->irr;
-    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
-    }
-#endif
-}
-
-static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
-{
-#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
-    struct kvm_irqchip chip;
-    struct kvm_ioapic_state *kioapic;
-    int i;
-
-    chip.chip_id = KVM_IRQCHIP_IOAPIC;
-    kioapic = &chip.chip.ioapic;
-
-    kioapic->id = s->id;
-    kioapic->ioregsel = s->ioregsel;
-    kioapic->base_address = s->base_address;
-    kioapic->irr = s->irr;
-    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-        kioapic->redirtbl[i].bits = s->ioredtbl[i];
-    }
-
-    kvm_set_irqchip(kvm_context, &chip);
-#endif
-}
-
-static void ioapic_save(QEMUFile *f, void *opaque)
+static void ioapic_save_common(QEMUFile *f, void *opaque)
 {
     IOAPICState *s = opaque;
     int i;
 
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_ioapic_save_to_user(s);
-    }
-
     qemu_put_8s(f, &s->id);
     qemu_put_8s(f, &s->ioregsel);
     qemu_put_be64s(f, &s->base_address);
@@ -259,7 +212,12 @@ static void ioapic_save(QEMUFile *f, void *opaque)
     }
 }
 
-static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
+static void ioapic_save(QEMUFile *f, void *opaque)
+{
+    ioapic_save_common(f, opaque);
+}
+
+static int ioapic_load_common(QEMUFile *f, void *opaque, int version_id)
 {
     IOAPICState *s = opaque;
     int i;
@@ -283,14 +241,15 @@ static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
         qemu_get_be64s(f, &s->ioredtbl[i]);
     }
 
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_ioapic_load_from_user(s);
-    }
-
     return 0;
 }
 
-static void ioapic_reset(void *opaque)
+static int ioapic_load(QEMUFile *f, void *opaque, int version_id)
+{
+    return ioapic_load_common(f, opaque, version_id);
+}
+
+static void ioapic_reset_common(void *opaque)
 {
     IOAPICState *s = opaque;
     int i;
@@ -299,11 +258,11 @@ static void ioapic_reset(void *opaque)
     s->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
     for(i = 0; i < IOAPIC_NUM_PINS; i++)
         s->ioredtbl[i] = 1 << 16; /* mask LVT */
-#ifdef KVM_CAP_IRQCHIP
-    if (kvm_enabled() && qemu_kvm_irqchip_in_kernel()) {
-        kvm_kernel_ioapic_load_from_user(s);
-    }
-#endif
+}
+
+static void ioapic_reset(void *opaque)
+{
+    ioapic_reset_common(opaque);
 }
 
 static CPUReadMemoryFunc *ioapic_mem_read[3] = {
@@ -335,3 +294,88 @@ IOAPICState *ioapic_init(void)
 
     return s;
 }
+
+#ifdef KVM_CAP_IRQCHIP
+static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
+{
+#if defined(TARGET_I386)
+    struct kvm_irqchip chip;
+    struct kvm_ioapic_state *kioapic;
+    int i;
+
+    chip.chip_id = KVM_IRQCHIP_IOAPIC;
+    kvm_get_irqchip(kvm_context, &chip);
+    kioapic = &chip.chip.ioapic;
+
+    s->id = kioapic->id;
+    s->ioregsel = kioapic->ioregsel;
+    s->base_address = kioapic->base_address;
+    s->irr = kioapic->irr;
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
+    }
+#endif
+}
+
+static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
+{
+#if defined(TARGET_I386)
+    struct kvm_irqchip chip;
+    struct kvm_ioapic_state *kioapic;
+    int i;
+
+    chip.chip_id = KVM_IRQCHIP_IOAPIC;
+    kioapic = &chip.chip.ioapic;
+
+    kioapic->id = s->id;
+    kioapic->ioregsel = s->ioregsel;
+    kioapic->base_address = s->base_address;
+    kioapic->irr = s->irr;
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        kioapic->redirtbl[i].bits = s->ioredtbl[i];
+    }
+
+    kvm_set_irqchip(kvm_context, &chip);
+#endif
+}
+
+static void kvm_ioapic_save(QEMUFile *f, void *opaque)
+{
+    IOAPICState *s = opaque;
+    kvm_kernel_ioapic_save_to_user(s);
+
+    ioapic_save_common(f, opaque);
+}
+
+static int kvm_ioapic_load(QEMUFile *f, void *opaque, int version_id)
+{
+    IOAPICState *s = opaque;
+    int r = ioapic_load_common(f, opaque, version_id);
+
+    if (r == 0)
+        kvm_kernel_ioapic_load_from_user(s);
+    return r;
+}
+
+
+static void kvm_ioapic_reset(void *opaque)
+{
+    IOAPICState *s = opaque;
+
+    ioapic_reset_common(opaque);
+
+    kvm_kernel_ioapic_load_from_user(s);
+}
+
+IOAPICState *kvm_ioapic_init(void)
+{
+    IOAPICState *s;
+
+    s = qemu_mallocz(sizeof(IOAPICState));
+    kvm_ioapic_reset(s);
+
+    register_savevm("ioapic", 0, 2, kvm_ioapic_save, kvm_ioapic_load, s);
+    qemu_register_reset(kvm_ioapic_reset, 0, s);
+    return s;
+}
+#endif
diff --git a/hw/pc.c b/hw/pc.c
index a99ab07..861dc34 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1042,7 +1042,12 @@ static void pc_init1(ram_addr_t ram_size,
     register_ioport_write(0x92, 1, 1, ioport92_write, NULL);
 
     if (pci_enabled) {
-        ioapic = ioapic_init();
+#ifdef KVM_CAP_IRQCHIP
+        if (kvm_enabled() && qemu_kvm_irqchip_in_kernel())
+            ioapic = kvm_ioapic_init();
+        else
+#endif
+            ioapic = ioapic_init();
     }
 #ifdef USE_KVM_PIT
     if (kvm_enabled() && qemu_kvm_pit_in_kernel())
diff --git a/hw/pc.h b/hw/pc.h
index ab847b9..d7f4a46 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -53,6 +53,7 @@ int apic_accept_pic_intr(CPUState *env);
 void apic_deliver_pic_intr(CPUState *env, int level);
 int apic_get_interrupt(CPUState *env);
 IOAPICState *ioapic_init(void);
+IOAPICState *kvm_ioapic_init(void);
 void ioapic_set_irq(void *opaque, int vector, int level);
 void apic_reset_irq_delivered(void);
 int apic_get_irq_delivered(void);
-- 
1.5.6.6

Re: [PATCH 1/4] always halt non-bsp cpu.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

Glauber Costa wrote:
> This is not kvm specific, and should do fine in plain qemu

This is fine with plain qemu already. The problem, IIUC, is that
in-kernel kvm irqchip does not have a chance to remove the halted state
again. Did you test the effect of this patch on that scenario? What
makes it safe to be removed now?

> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/apic.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 86aa6b6..2eddba0 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s)
>  
>      cpu_reset(s->cpu_env);
>  
> -    if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
> -        (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
> +    if (!(s->apicbase & MSR_IA32_APICBASE_BSP))
>          s->cpu_env->halted = 1;
>  
>      if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > This is not kvm specific, and should do fine in plain qemu
> 
> This is fine with plain qemu already. The problem, IIUC, is that
> in-kernel kvm irqchip does not have a chance to remove the halted state
> again. Did you test the effect of this patch on that scenario? What
> makes it safe to be removed now?
IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
the vcpu initialization.

It is tested here with in-kernel irqchip and works, so probably not
a problem, unless you can spot something.

Re: [PATCH 1/4] always halt non-bsp cpu.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

Glauber Costa wrote:
> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> This is not kvm specific, and should do fine in plain qemu
>> This is fine with plain qemu already. The problem, IIUC, is that
>> in-kernel kvm irqchip does not have a chance to remove the halted state
>> again. Did you test the effect of this patch on that scenario? What
>> makes it safe to be removed now?
> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> the vcpu initialization.
> 
> It is tested here with in-kernel irqchip and works, so probably not
> a problem, unless you can spot something.

At least your patch applied alone breaks -smp >1 here.

But the whole management of env->halted for the in-kernel irqchip in
qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
nice to always see a consistent halted in user space, specifically for
debugging purposes.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> This is not kvm specific, and should do fine in plain qemu
> >> This is fine with plain qemu already. The problem, IIUC, is that
> >> in-kernel kvm irqchip does not have a chance to remove the halted state
> >> again. Did you test the effect of this patch on that scenario? What
> >> makes it safe to be removed now?
> > IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> > the vcpu initialization.
> > 
> > It is tested here with in-kernel irqchip and works, so probably not
> > a problem, unless you can spot something.
> 
> At least your patch applied alone breaks -smp >1 here.
> 
> But the whole management of env->halted for the in-kernel irqchip in
> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
> nice to always see a consistent halted in user space, specifically for
> debugging purposes.
out of curiosity: did you apply the whole series?

please report with it. I suspect there is a change later on that might
make it work. Of course, this is no excuse, as I'm a huge fan of bisectability.
If this is the case, I'll rework the series in a way that it always work.

Re: [PATCH 1/4] always halt non-bsp cpu.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

Glauber Costa wrote:
> On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
>>>> Glauber Costa wrote:
>>>>> This is not kvm specific, and should do fine in plain qemu
>>>> This is fine with plain qemu already. The problem, IIUC, is that
>>>> in-kernel kvm irqchip does not have a chance to remove the halted state
>>>> again. Did you test the effect of this patch on that scenario? What
>>>> makes it safe to be removed now?
>>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
>>> the vcpu initialization.
>>>
>>> It is tested here with in-kernel irqchip and works, so probably not
>>> a problem, unless you can spot something.
>> At least your patch applied alone breaks -smp >1 here.
>>
>> But the whole management of env->halted for the in-kernel irqchip in
>> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
>> nice to always see a consistent halted in user space, specifically for
>> debugging purposes.
> out of curiosity: did you apply the whole series?

Meanwhile I did, but it makes no difference.

> 
> please report with it. I suspect there is a change later on that might
> make it work. Of course, this is no excuse, as I'm a huge fan of bisectability.
> If this is the case, I'll rework the series in a way that it always work.

I still suspect that dealing with halted for the in-kernel case is a bit
more tricky.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> This is not kvm specific, and should do fine in plain qemu
> >>>> This is fine with plain qemu already. The problem, IIUC, is that
> >>>> in-kernel kvm irqchip does not have a chance to remove the halted state
> >>>> again. Did you test the effect of this patch on that scenario? What
> >>>> makes it safe to be removed now?
> >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> >>> the vcpu initialization.
> >>>
> >>> It is tested here with in-kernel irqchip and works, so probably not
> >>> a problem, unless you can spot something.
> >> At least your patch applied alone breaks -smp >1 here.
> >>
> >> But the whole management of env->halted for the in-kernel irqchip in
> >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
> >> nice to always see a consistent halted in user space, specifically for
> >> debugging purposes.
> > out of curiosity: did you apply the whole series?
> 
> Meanwhile I did, but it makes no difference.
ok, thanks for testing.
I'll take a look at that tomorrow.

Re: [PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> This is not kvm specific, and should do fine in plain qemu
> >>>> This is fine with plain qemu already. The problem, IIUC, is that
> >>>> in-kernel kvm irqchip does not have a chance to remove the halted state
> >>>> again. Did you test the effect of this patch on that scenario? What
> >>>> makes it safe to be removed now?
> >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> >>> the vcpu initialization.
> >>>
> >>> It is tested here with in-kernel irqchip and works, so probably not
> >>> a problem, unless you can spot something.
> >> At least your patch applied alone breaks -smp >1 here.
> >>
> >> But the whole management of env->halted for the in-kernel irqchip in
> >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
> >> nice to always see a consistent halted in user space, specifically for
> >> debugging purposes.
> > out of curiosity: did you apply the whole series?
> 
> Meanwhile I did, but it makes no difference.
Jan, can you be more specific on why it breaks? I'm double trying it
here, and it works for me just fine.

Re: [PATCH 1/4] always halt non-bsp cpu.

[Glauber Costa]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> This is not kvm specific, and should do fine in plain qemu
> >>>> This is fine with plain qemu already. The problem, IIUC, is that
> >>>> in-kernel kvm irqchip does not have a chance to remove the halted state
> >>>> again. Did you test the effect of this patch on that scenario? What
> >>>> makes it safe to be removed now?
> >>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> >>> the vcpu initialization.
> >>>
> >>> It is tested here with in-kernel irqchip and works, so probably not
> >>> a problem, unless you can spot something.
> >> At least your patch applied alone breaks -smp >1 here.
> >>
> >> But the whole management of env->halted for the in-kernel irqchip in
> >> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
> >> nice to always see a consistent halted in user space, specifically for
> >> debugging purposes.
> > out of curiosity: did you apply the whole series?
> 
> Meanwhile I did, but it makes no difference.
> 

Can you try putting the following patch before this one?


[-- Attachment #2: test.patch --]
[-- Type: text/plain, Size: 875 bytes --]

commit 0bca36c13cb402ea33ad615d8d4f9882d1284f02
Author: Glauber Costa <glommer@redhat.com>
Date:   Tue Jun 2 21:15:57 2009 -0400

    test patch
    
    Signed-off-by: Glauber Costa <glommer@redhat.com>

diff --git a/qemu-kvm.c b/qemu-kvm.c
index 68d3b92..519a6ee 100644
--- a/qemu-kvm.c
+++ b/qemu-kvm.c
@@ -390,8 +390,6 @@ static int kvm_main_loop_cpu(CPUState *env)
     setup_kernel_sigmask(env);
 
     pthread_mutex_lock(&qemu_mutex);
-    if (kvm_irqchip_in_kernel(kvm_context))
-	env->halted = 0;
 
     kvm_qemu_init_env(env);
 #ifdef TARGET_I386
@@ -402,6 +400,9 @@ static int kvm_main_loop_cpu(CPUState *env)
     kvm_load_registers(env);
 
     while (1) {
+
+    if (kvm_irqchip_in_kernel(kvm_context))
+    	env->halted = 0;
 	while (!has_work(env))
 	    kvm_main_loop_wait(env, 1000);
 	if (env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI))

Re: [PATCH 1/4] always halt non-bsp cpu.

[Gleb Natapov]

On Tue, Jun 02, 2009 at 03:37:47PM -0400, Glauber Costa wrote:
> This is not kvm specific, and should do fine in plain qemu
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/apic.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 86aa6b6..2eddba0 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -467,8 +467,7 @@ static void apic_init_ipi(APICState *s)
>  
>      cpu_reset(s->cpu_env);
>  
> -    if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
> -        (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
> +    if (!(s->apicbase & MSR_IA32_APICBASE_BSP))
>          s->cpu_env->halted = 1;
>  
>      if (kvm_enabled() && !qemu_kvm_irqchip_in_kernel())
Once upon a time there was a good reason to add this: 358bdca5beb733c983bd5e7406c7893e19e9600b

What have changed?

--
			Gleb.

Re: [PATCH 1/4] always halt non-bsp cpu.

[Jan Kiszka]

Glauber Costa wrote:
> On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
>>>> Glauber Costa wrote:
>>>>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
>>>>>> Glauber Costa wrote:
>>>>>>> This is not kvm specific, and should do fine in plain qemu
>>>>>> This is fine with plain qemu already. The problem, IIUC, is that
>>>>>> in-kernel kvm irqchip does not have a chance to remove the halted state
>>>>>> again. Did you test the effect of this patch on that scenario? What
>>>>>> makes it safe to be removed now?
>>>>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
>>>>> the vcpu initialization.
>>>>>
>>>>> It is tested here with in-kernel irqchip and works, so probably not
>>>>> a problem, unless you can spot something.
>>>> At least your patch applied alone breaks -smp >1 here.
>>>>
>>>> But the whole management of env->halted for the in-kernel irqchip in
>>>> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
>>>> nice to always see a consistent halted in user space, specifically for
>>>> debugging purposes.
>>> out of curiosity: did you apply the whole series?
>> Meanwhile I did, but it makes no difference.
>>
> 
> Can you try putting the following patch before this one?

If it helps you to understand the issue, I will do so later.

But I *really* suggest to take this chance and develop in-kernel irqchip
support that does not mess with halted, rather keeps it consistent (on
register sync) and maybe adds exceptions from "if (!env->halted)" tests
where required. IMHO, this is mandatory for an upstream merge!

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/4] always halt non-bsp cpu.

[Jan Kiszka]

Glauber Costa wrote:
> On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
>>>> Glauber Costa wrote:
>>>>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
>>>>>> Glauber Costa wrote:
>>>>>>> This is not kvm specific, and should do fine in plain qemu
>>>>>> This is fine with plain qemu already. The problem, IIUC, is that
>>>>>> in-kernel kvm irqchip does not have a chance to remove the halted state
>>>>>> again. Did you test the effect of this patch on that scenario? What
>>>>>> makes it safe to be removed now?
>>>>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
>>>>> the vcpu initialization.
>>>>>
>>>>> It is tested here with in-kernel irqchip and works, so probably not
>>>>> a problem, unless you can spot something.
>>>> At least your patch applied alone breaks -smp >1 here.
>>>>
>>>> But the whole management of env->halted for the in-kernel irqchip in
>>>> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
>>>> nice to always see a consistent halted in user space, specifically for
>>>> debugging purposes.
>>> out of curiosity: did you apply the whole series?
>> Meanwhile I did, but it makes no difference.
> Jan, can you be more specific on why it breaks? I'm double trying it
> here, and it works for me just fine.
> 

It locked up in the BIOS after reset from a booted SMP guest, somewhere
before selecting the boot device.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/4] always halt non-bsp cpu.

[Gleb Natapov]

On Wed, Jun 03, 2009 at 01:01:29PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Wed, Jun 03, 2009 at 12:32:04AM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Wed, Jun 03, 2009 at 12:01:00AM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> On Tue, Jun 02, 2009 at 10:35:47PM +0200, Jan Kiszka wrote:
> >>>>>> Glauber Costa wrote:
> >>>>>>> This is not kvm specific, and should do fine in plain qemu
> >>>>>> This is fine with plain qemu already. The problem, IIUC, is that
> >>>>>> in-kernel kvm irqchip does not have a chance to remove the halted state
> >>>>>> again. Did you test the effect of this patch on that scenario? What
> >>>>>> makes it safe to be removed now?
> >>>>> IIRC, the in kernel irqchip sets halted = 0 in the very beginning of
> >>>>> the vcpu initialization.
> >>>>>
> >>>>> It is tested here with in-kernel irqchip and works, so probably not
> >>>>> a problem, unless you can spot something.
> >>>> At least your patch applied alone breaks -smp >1 here.
> >>>>
> >>>> But the whole management of env->halted for the in-kernel irqchip in
> >>>> qemu-kvm is a bit hacky IMHO. Maybe it's time to rethink this. Would be
> >>>> nice to always see a consistent halted in user space, specifically for
> >>>> debugging purposes.
> >>> out of curiosity: did you apply the whole series?
> >> Meanwhile I did, but it makes no difference.
> >>
> > 
> > Can you try putting the following patch before this one?
> 
> If it helps you to understand the issue, I will do so later.
> 
> But I *really* suggest to take this chance and develop in-kernel irqchip
> support that does not mess with halted, rather keeps it consistent (on
> register sync) and maybe adds exceptions from "if (!env->halted)" tests
> where required. IMHO, this is mandatory for an upstream merge!
> 
The difference between kernel/userspace irq chip is that in
former case halted cpu sleeps in the kernel and in later case in the
usespace (well also in the kernel but not in kvm code). We currently
abuse halted to do different sleeps. Cpu loop should be reworked.

--
			Gleb.