[PATCH 0/4] apic/ioapic kvm free implementation

[PATCH 0/4] apic/ioapic kvm free implementation

[Glauber Costa]

Same thing,

addressing comments from gleb.

[PATCH 1/4] avoid halted state for in kernel irqchip

[Glauber Costa]

This patch is part of a series that tries to provide
a kvm-free apic implementation. In the last interation,
Jan pointed out that halted state management with in kernel
irqchip gets quite messy. I don't disagree.

It broke this series specifically, as init IPIs had the
halted state set. The solution would be to rework halted
state management. But I'm too lazy. Besides, gleb told me
he would do it, which makes it his problem, not mine. ;-)

This patch can be used to bypass the problem entirely:
if kvm apic do not call apic_init_ipi, but a version of it
that does everything but mangling around with cpu states,
the problem becomes a non issue, and my glorious series
can be applied.

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

diff --git a/hw/apic.c b/hw/apic.c
index 86aa6b6..862289d 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -443,8 +443,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
     }
 }
 
-
-static void apic_init_ipi(APICState *s)
+static void apic_init_ipi_state(APICState *s)
 {
     int i;
 
@@ -466,6 +465,11 @@ static void apic_init_ipi(APICState *s)
     s->next_time = 0;
 
     cpu_reset(s->cpu_env);
+}
+
+static void apic_init_ipi(APICState *s)
+{
+    apic_init_ipi_state(s);
 
     if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
         (!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 862289d..39e1675 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;
@@ -93,6 +95,45 @@ static APICState *local_apics[MAX_APICS + 1];
 static int last_apic_id = 0;
 static int apic_irq_delivered;
 
+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);
 static void apic_set_irq(APICState *s, int vector_num, int trigger_mode);
@@ -475,9 +516,8 @@ static void apic_init_ipi(APICState *s)
         (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
         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 */
@@ -490,8 +530,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..b5954bc 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 && !apic_init_received(env))
 	    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      |  241 ++++++++++++++++++++++++++++++++++----------------------
 hw/pc.c        |    7 ++-
 hw/pc.h        |    1 +
 qemu-kvm-x86.c |    5 +-
 4 files changed, 157 insertions(+), 97 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index 39e1675..e3b606b 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -857,103 +857,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)
 {
     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);
@@ -1017,8 +925,6 @@ 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;
 }
 
@@ -1039,7 +945,6 @@ static void apic_reset(void *opaque)
          */
         s->lvt[APIC_LVT_LINT0] = 0x700;
     }
-    qemu_kvm_load_lapic(s->cpu_env);
 }
 
 static CPUReadMemoryFunc *apic_mem_read[3] = {
@@ -1086,3 +991,149 @@ 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(f, opaque);
+}
+
+static int kvm_apic_load(QEMUFile *f, void *opaque, int version_id)
+{
+    APICState *s = opaque;
+    int r = apic_load(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;
+
+    s->apicbase = 0xfee00000 |
+        (s->id ? 0 : MSR_IA32_APICBASE_BSP) | MSR_IA32_APICBASE_ENABLE;
+
+    apic_init_ipi_state(s);
+
+    if (s->id == 0) {
+        /*
+         * LINT0 delivery mode on CPU #0 is set to ExtInt at initialization
+         * time typically by BIOS, so PIC interrupt can be delivered to the
+         * processor when local APIC is enabled.
+         */
+        s->lvt[APIC_LVT_LINT0] = 0x700;
+    }
+
+    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 0/4] apic/ioapic kvm free implementation

[Gleb Natapov]

On Wed, Jun 03, 2009 at 05:19:26PM -0400, Glauber Costa wrote:
> Same thing,
> 
> addressing comments from gleb.
> 
> 
Jan, can you run your test on this one? It differs from previous one in
halt handling.

--
			Gleb.

Re: [PATCH 0/4] apic/ioapic kvm free implementation

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Wed, Jun 03, 2009 at 05:19:26PM -0400, Glauber Costa wrote:
>> Same thing,
>>
>> addressing comments from gleb.
>>
>>
> Jan, can you run your test on this one? It differs from previous one in
> halt handling.

Still works for me.

Jan


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

Re: [PATCH 0/4] apic/ioapic kvm free implementation

[Gleb Natapov]

On Thu, Jun 04, 2009 at 09:46:23PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Wed, Jun 03, 2009 at 05:19:26PM -0400, Glauber Costa wrote:
> >> Same thing,
> >>
> >> addressing comments from gleb.
> >>
> >>
> > Jan, can you run your test on this one? It differs from previous one in
> > halt handling.
> 
> Still works for me.
> 
Cool, thanks.

--
			Gleb.

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

[Avi Kivity]

Glauber Costa wrote:
> 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.
>
>   

The new functions and data members are kvm specific.  Please update 
their names to reflect that.  'sipi_needed' would be really confusing 
for someone reading the code from a tcg perspective (it's confusing even 
from a kvm perspective, but that's our problem and can be addressed later).


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

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

[Avi Kivity]

Glauber Costa wrote:
> On Mon, Jun 08, 2009 at 12:08:17PM +0300, Avi Kivity wrote:
>   
>> Glauber Costa wrote:
>>     
>>> 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.
>>>
>>>   
>>>       
>> The new functions and data members are kvm specific.  Please update  
>> their names to reflect that.  'sipi_needed' would be really confusing  
>> for someone reading the code from a tcg perspective (it's confusing even  
>> from a kvm perspective, but that's our problem and can be addressed 
>> later).
>>     
> The idea of such series is exactly to write it in a way that is _not_
> kvm specific, although kvm is the only user. So, I may change the
> name if you prefer, but I would not like to introduce any "kvm" string
> on it. To do that, we might as well stick with what we have.
>   

Well, sipi_needed is certainly kvm specific, if you look at its usage.  
I don't see how anything else could use it.

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

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

[Glauber Costa]

On Mon, Jun 08, 2009 at 12:08:17PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> 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.
>>
>>   
>
> The new functions and data members are kvm specific.  Please update  
> their names to reflect that.  'sipi_needed' would be really confusing  
> for someone reading the code from a tcg perspective (it's confusing even  
> from a kvm perspective, but that's our problem and can be addressed 
> later).
The idea of such series is exactly to write it in a way that is _not_
kvm specific, although kvm is the only user. So, I may change the
name if you prefer, but I would not like to introduce any "kvm" string
on it. To do that, we might as well stick with what we have.

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

[Glauber Costa]

On Mon, Jun 08, 2009 at 06:48:38PM +0300, Avi Kivity wrote:
> Glauber Costa wrote:
>> On Mon, Jun 08, 2009 at 12:08:17PM +0300, Avi Kivity wrote:
>>   
>>> Glauber Costa wrote:
>>>     
>>>> 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.
>>>>
>>>>         
>>> The new functions and data members are kvm specific.  Please update   
>>> their names to reflect that.  'sipi_needed' would be really confusing 
>>>  for someone reading the code from a tcg perspective (it's confusing 
>>> even  from a kvm perspective, but that's our problem and can be 
>>> addressed later).
>>>     
>> The idea of such series is exactly to write it in a way that is _not_
>> kvm specific, although kvm is the only user. So, I may change the
>> name if you prefer, but I would not like to introduce any "kvm" string
>> on it. To do that, we might as well stick with what we have.
>>   
>
> Well, sipi_needed is certainly kvm specific, if you look at its usage.   
> I don't see how anything else could use it.
The usage may be. That's why it lives in qemu-kvm.c.
OTOH, a function to verify whether or not a SIPI was received by a cpu,
is totally generic.

Re: [PATCH 1/4] avoid halted state for in kernel irqchip

[Glauber Costa]

On Wed, Jun 03, 2009 at 10:50:59PM +0300, Gleb Natapov wrote:
> On Wed, Jun 03, 2009 at 02:25:41PM -0400, Glauber Costa wrote:
> > This patch is part of a series that tries to provide
> > a kvm-free apic implementation. In the last interation,
> > Jan pointed out that halted state management with in kernel
> > irqchip gets quite messy. I don't disagree.
> > 
> > It broke this series specifically, as init IPIs had the
> > halted state set. The solution would be to rework halted
> > state management. But I'm too lazy. Besides, gleb told me
> > he would do it, which makes it his problem, not mine. ;-)
> > 
> > This patch can be used to bypass the problem entirely:
> > if kvm apic do not call apic_init_ipi, but a version of it
> > that does everything but mangling around with cpu states,
> > the problem becomes a non issue, and my glorious series
> > can be applied.
> > 
> May be I miss something, but I don't see apic_init_ipi_state() called
> outside of apic_init_ipi() in this patch series, so what is the point of
> this patch?
it should be called from patch 3.

But I made a mistake, and in fact it is not. My bad.

> 
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  hw/apic.c |    8 ++++++--
> >  1 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/apic.c b/hw/apic.c
> > index 86aa6b6..862289d 100644
> > --- a/hw/apic.c
> > +++ b/hw/apic.c
> > @@ -443,8 +443,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> >      }
> >  }
> >  
> > -
> > -static void apic_init_ipi(APICState *s)
> > +static void apic_init_ipi_state(APICState *s)
> >  {
> >      int i;
> >  
> > @@ -466,6 +465,11 @@ static void apic_init_ipi(APICState *s)
> >      s->next_time = 0;
> >  
> >      cpu_reset(s->cpu_env);
> > +}
> > +
> > +static void apic_init_ipi(APICState *s)
> > +{
> > +    apic_init_ipi_state(s);
> >  
> >      if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
> >          (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
> > -- 
> > 1.5.6.6
> > 
> > --
> > 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
> 
> --
> 			Gleb.

Re: [PATCH 1/4] avoid halted state for in kernel irqchip

[Gleb Natapov]

On Wed, Jun 03, 2009 at 02:25:41PM -0400, Glauber Costa wrote:
> This patch is part of a series that tries to provide
> a kvm-free apic implementation. In the last interation,
> Jan pointed out that halted state management with in kernel
> irqchip gets quite messy. I don't disagree.
> 
> It broke this series specifically, as init IPIs had the
> halted state set. The solution would be to rework halted
> state management. But I'm too lazy. Besides, gleb told me
> he would do it, which makes it his problem, not mine. ;-)
> 
> This patch can be used to bypass the problem entirely:
> if kvm apic do not call apic_init_ipi, but a version of it
> that does everything but mangling around with cpu states,
> the problem becomes a non issue, and my glorious series
> can be applied.
> 
May be I miss something, but I don't see apic_init_ipi_state() called
outside of apic_init_ipi() in this patch series, so what is the point of
this patch?

> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/apic.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/apic.c b/hw/apic.c
> index 86aa6b6..862289d 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -443,8 +443,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>      }
>  }
>  
> -
> -static void apic_init_ipi(APICState *s)
> +static void apic_init_ipi_state(APICState *s)
>  {
>      int i;
>  
> @@ -466,6 +465,11 @@ static void apic_init_ipi(APICState *s)
>      s->next_time = 0;
>  
>      cpu_reset(s->cpu_env);
> +}
> +
> +static void apic_init_ipi(APICState *s)
> +{
> +    apic_init_ipi_state(s);
>  
>      if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
>          (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
> -- 
> 1.5.6.6
> 
> --
> 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

--
			Gleb.

[PATCH 1/4] avoid halted state for in kernel irqchip

[Glauber Costa]

This patch is part of a series that tries to provide
a kvm-free apic implementation. In the last interation,
Jan pointed out that halted state management with in kernel
irqchip gets quite messy. I don't disagree.

It broke this series specifically, as init IPIs had the
halted state set. The solution would be to rework halted
state management. But I'm too lazy. Besides, gleb told me
he would do it, which makes it his problem, not mine. ;-)

This patch can be used to bypass the problem entirely:
if kvm apic do not call apic_init_ipi, but a version of it
that does everything but mangling around with cpu states,
the problem becomes a non issue, and my glorious series
can be applied.

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

diff --git a/hw/apic.c b/hw/apic.c
index 86aa6b6..862289d 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -443,8 +443,7 @@ static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
     }
 }
 
-
-static void apic_init_ipi(APICState *s)
+static void apic_init_ipi_state(APICState *s)
 {
     int i;
 
@@ -466,6 +465,11 @@ static void apic_init_ipi(APICState *s)
     s->next_time = 0;
 
     cpu_reset(s->cpu_env);
+}
+
+static void apic_init_ipi(APICState *s)
+{
+    apic_init_ipi_state(s);
 
     if (!(s->apicbase & MSR_IA32_APICBASE_BSP) &&
         (!kvm_enabled() || !qemu_kvm_irqchip_in_kernel()))
-- 
1.5.6.6