[Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Glauber Costa]

Hi people,

This (sent with my correct e-mail address) is a port from qemu-kvm.git of
the in-kernel irqchip support. Right now, I am not providing any way to disable
it. I am ready to advocate that it should be the default, whenever available.

I plan to, however, provide a patch that disables it whenever this gets merged.

I gave it basic testing, specially with reboot scenarios (due to the load/save)
functions, and all seems to be working well. But I have only tested linux guests
so far.

Anthony, I believe it is, although not perfect, in inclusion state.

[Qemu-devel] [PATCH 1/6] provide in-kernel ioapic

[Glauber Costa]

This patch provides kvm with an in-kernel ioapic. We are currently not enabling it.
The code is heavily based on what's in qemu-kvm.git.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/ioapic.c |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 kvm-all.c   |   20 ++++++++++++++
 kvm.h       |    4 +++
 3 files changed, 108 insertions(+), 1 deletions(-)

diff --git a/hw/ioapic.c b/hw/ioapic.c
index b0ad78f..3af86f1 100644
--- a/hw/ioapic.c
+++ b/hw/ioapic.c
@@ -24,10 +24,12 @@
 #include "pc.h"
 #include "qemu-timer.h"
 #include "host-utils.h"
+#include "kvm.h"
 
 //#define DEBUG_IOAPIC
 
 #define IOAPIC_NUM_PINS			0x18
+#define IOAPIC_DEFAULT_BASE_ADDRESS  0xfec00000
 #define IOAPIC_LVT_MASKED 		(1<<16)
 
 #define IOAPIC_TRIGGER_EDGE		0
@@ -191,6 +193,79 @@ static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t va
     }
 }
 
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
+{
+    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 = IOAPIC_DEFAULT_BASE_ADDRESS;
+    kioapic->irr = s->irr;
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        kioapic->redirtbl[i].bits = s->ioredtbl[i];
+    }
+
+    kvm_set_irqchip(&chip);
+}
+
+static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
+{
+    struct kvm_irqchip chip;
+    struct kvm_ioapic_state *kioapic;
+    int i;
+
+    chip.chip_id = KVM_IRQCHIP_IOAPIC;
+    kvm_get_irqchip(&chip);
+    kioapic = &chip.chip.ioapic;
+
+    s->id = kioapic->id;
+    s->ioregsel = kioapic->ioregsel;
+    s->irr = kioapic->irr;
+    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
+        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
+    }
+}
+#endif
+
+static void ioapic_pre_save(const void *opaque)
+{
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+    IOAPICState *s = (void *)opaque;
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_ioapic_save_to_user(s);
+    }
+#endif
+}
+
+static int ioapic_pre_load(void *opaque)
+{
+    IOAPICState *s = opaque;
+
+    /* in case we are doing version 1, we just set these to sane values */
+    s->irr = 0;
+    return 0;
+}
+
+static int ioapic_post_load(void *opaque)
+{
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+    IOAPICState *s = opaque;
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_ioapic_load_from_user(s);
+    }
+#endif
+    return 0;
+}
+
+
 static const VMStateDescription vmstate_ioapic = {
     .name = "ioapic",
     .version_id = 1,
@@ -201,7 +276,10 @@ static const VMStateDescription vmstate_ioapic = {
         VMSTATE_UINT8(ioregsel, IOAPICState),
         VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
         VMSTATE_END_OF_LIST()
-    }
+    },
+    .pre_load = ioapic_pre_load,
+    .post_load = ioapic_post_load,
+    .pre_save = ioapic_pre_save,
 };
 
 static void ioapic_reset(void *opaque)
@@ -212,6 +290,11 @@ static void ioapic_reset(void *opaque)
     memset(s, 0, sizeof(*s));
     for(i = 0; i < IOAPIC_NUM_PINS; i++)
         s->ioredtbl[i] = 1 << 16; /* mask LVT */
+#ifdef KVM_CAP_IRQCHIP
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_ioapic_load_from_user(s);
+    }
+#endif
 }
 
 static CPUReadMemoryFunc * const ioapic_mem_read[3] = {
diff --git a/kvm-all.c b/kvm-all.c
index f8a05cc..ff439a5 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -411,6 +411,26 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
     return ret;
 }
 
+#ifdef KVM_CAP_IRQCHIP
+int kvm_set_irqchip(struct kvm_irqchip *chip)
+{
+    if (!kvm_state->irqchip_in_kernel) {
+        return 0;
+    }
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_IRQCHIP, chip);
+}
+
+int kvm_get_irqchip(struct kvm_irqchip *chip)
+{
+    if (!kvm_state->irqchip_in_kernel) {
+        return 0;
+    }
+
+    return kvm_vm_ioctl(kvm_state, KVM_GET_IRQCHIP, chip);
+}
+#endif
+
 int kvm_init(int smp_cpus)
 {
     static const char upgrade_note[] =
diff --git a/kvm.h b/kvm.h
index e7d5beb..8d4afa0 100644
--- a/kvm.h
+++ b/kvm.h
@@ -16,6 +16,7 @@
 
 #include "config.h"
 #include "qemu-queue.h"
+#include <linux/kvm.h>
 
 #ifdef CONFIG_KVM
 extern int kvm_allowed;
@@ -63,6 +64,9 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap);
 int kvm_pit_in_kernel(void);
 int kvm_irqchip_in_kernel(void);
 
+int kvm_set_irqchip(struct kvm_irqchip *chip);
+int kvm_get_irqchip(struct kvm_irqchip *chip);
+
 /* internal API */
 
 struct KVMState;
-- 
1.6.2.5

[Qemu-devel] [PATCH 2/6] provide in-kernel apic

[Glauber Costa]

This patch provides kvm with an in-kernel apic. We are currently not enabling it.
The code is heavily based on what's in qemu-kvm.git.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/apic.c         |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 kvm.h             |    5 ++-
 target-i386/kvm.c |   18 +++++++
 3 files changed, 154 insertions(+), 5 deletions(-)

diff --git a/hw/apic.c b/hw/apic.c
index c89008e..b3f5179 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val)
 #endif
     if (!s)
         return;
-    s->apicbase = (val & 0xfffff000) |
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel())
+        s->apicbase = val;
+    else
+        s->apicbase = (val & 0xfffff000) |
         (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE));
     /* if disabled, cannot be enabled again */
     if (!(val & MSR_IA32_APICBASE_ENABLE)) {
@@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env)
     s->wait_for_sipi = 1;
 
     env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP);
+
+#ifdef KVM_CAP_MP_STATE
+    if (kvm_enabled() && kvm_irqchip_in_kernel())
+        env->mp_state
+            = env->halted ? KVM_MP_STATE_UNINITIALIZED : KVM_MP_STATE_RUNNABLE;
+#endif
+
 }
 
 static void apic_startup(APICState *s, int vector_num)
@@ -903,12 +914,121 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+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_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(s->cpu_env, klapic);
+}
+
+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(s->cpu_env, 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);
+}
+#endif
+
+static void qemu_kvm_load_lapic(CPUState *env)
+{
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_lapic_load_from_user(env->apic_state);
+    }
+#endif
+}
+
+static void apic_pre_save(const void *opaque)
+{
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+    APICState *s = (void *)opaque;
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_lapic_save_to_user(s);
+    }
+#endif
+}
+
+static int apic_post_load(void *opaque)
+{
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+    APICState *s = opaque;
+
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        kvm_kernel_lapic_load_from_user(s);
+    }
+#endif
+    return 0;
+}
+
 static const VMStateDescription vmstate_apic = {
     .name = "apic",
     .version_id = 3,
     .minimum_version_id = 3,
     .minimum_version_id_old = 1,
     .load_state_old = apic_load_old,
+    .pre_save = apic_pre_save,
+    .post_load = apic_post_load,
     .fields      = (VMStateField []) {
         VMSTATE_UINT32(apicbase, APICState),
         VMSTATE_UINT8(id, APICState),
@@ -933,9 +1053,8 @@ static const VMStateDescription vmstate_apic = {
     }
 };
 
-static void apic_reset(void *opaque)
+static void apic_do_reset(APICState *s)
 {
-    APICState *s = opaque;
     int bsp;
 
     cpu_synchronize_state(s->cpu_env);
@@ -957,6 +1076,15 @@ static void apic_reset(void *opaque)
     }
 }
 
+static void apic_reset(void *opaque)
+{
+    APICState *s = opaque;
+
+    apic_do_reset(s);
+
+    qemu_kvm_load_lapic(s->cpu_env);
+}
+
 static CPUReadMemoryFunc * const apic_mem_read[3] = {
     apic_mem_readb,
     apic_mem_readw,
@@ -981,7 +1109,7 @@ int apic_init(CPUState *env)
     s->id = env->cpuid_apic_id;
     s->cpu_env = env;
 
-    apic_reset(s);
+    apic_do_reset(s);
     msix_supported = 1;
 
     /* XXX: mapping more APICs at the same memory location */
diff --git a/kvm.h b/kvm.h
index 8d4afa0..ae5223b 100644
--- a/kvm.h
+++ b/kvm.h
@@ -66,7 +66,7 @@ int kvm_irqchip_in_kernel(void);
 
 int kvm_set_irqchip(struct kvm_irqchip *chip);
 int kvm_get_irqchip(struct kvm_irqchip *chip);
-
+ 
 /* internal API */
 
 struct KVMState;
@@ -97,6 +97,9 @@ int kvm_arch_init(KVMState *s, int smp_cpus);
 
 int kvm_arch_init_vcpu(CPUState *env);
 
+int kvm_set_lapic(CPUState *env, struct kvm_lapic_state *s);
+int kvm_get_lapic(CPUState *env, struct kvm_lapic_state *s);
+
 struct kvm_guest_debug;
 struct kvm_debug_exit_arch;
 
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index a0e261c..14c9785 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -959,3 +959,21 @@ void kvm_arch_update_guest_debug(CPUState *env, struct kvm_guest_debug *dbg)
     }
 }
 #endif /* KVM_CAP_SET_GUEST_DEBUG */
+
+#ifdef KVM_CAP_IRQCHIP
+int kvm_set_lapic(CPUState *env, struct kvm_lapic_state *s)
+{
+    if (!kvm_irqchip_in_kernel())
+        return 0;
+
+    return kvm_vcpu_ioctl(env, KVM_SET_LAPIC, s);
+}
+
+int kvm_get_lapic(CPUState *env, struct kvm_lapic_state *s)
+{
+    if (!kvm_irqchip_in_kernel())
+        return 0;
+
+    return kvm_vcpu_ioctl(env, KVM_GET_LAPIC, s);
+}
+#endif
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/6] provide apic_set_irq_delivered

[Glauber Costa]

i8259 chip will use it, so provide it, and export it through pc.h

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

diff --git a/hw/apic.c b/hw/apic.c
index b3f5179..f6b5542 100644
--- a/hw/apic.c
+++ b/hw/apic.c
@@ -392,6 +392,11 @@ void apic_reset_irq_delivered(void)
     apic_irq_delivered = 0;
 }
 
+void apic_set_irq_delivered(void)
+{
+    apic_irq_delivered = 1;
+}
+
 int apic_get_irq_delivered(void)
 {
     return apic_irq_delivered;
diff --git a/hw/pc.h b/hw/pc.h
index c9cdd4a..f7c1a88 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -45,6 +45,7 @@ int apic_get_interrupt(CPUState *env);
 qemu_irq *ioapic_init(void);
 void ioapic_set_irq(void *opaque, int vector, int level);
 void apic_reset_irq_delivered(void);
+void apic_set_irq_delivered(void);
 int apic_get_irq_delivered(void);
 
 /* i8254.c */
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/6] provide in-kernel i8259 chip

[Glauber Costa]

This patch provides kvm with an in-kernel i8259 chip. We are currently not enabling it.
The code is heavily based on what's in qemu-kvm.git.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/i8259.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pc.h    |    1 +
 kvm-all.c  |   24 ++++++++++++++
 kvm.h      |    2 +
 4 files changed, 130 insertions(+), 0 deletions(-)

diff --git a/hw/i8259.c b/hw/i8259.c
index 3de22e3..31524f5 100644
--- a/hw/i8259.c
+++ b/hw/i8259.c
@@ -26,6 +26,7 @@
 #include "isa.h"
 #include "monitor.h"
 #include "qemu-timer.h"
+#include "kvm.h"
 
 /* debug PIC */
 //#define DEBUG_PIC
@@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, uint32_t addr1)
     return s->elcr;
 }
 
+static int kvm_kernel_pic_load_from_user(void *opaque)
+{
+#if defined(TARGET_I386)
+    PicState *s = (void *)opaque;
+    struct kvm_irqchip chip;
+    struct kvm_pic_state *kpic;
+
+    chip.chip_id = (&s->pics_state->pics[0] == s) ?
+                   KVM_IRQCHIP_PIC_MASTER :
+                   KVM_IRQCHIP_PIC_SLAVE;
+    kpic = &chip.chip.pic;
+
+    kpic->last_irr = s->last_irr;
+    kpic->irr = s->irr;
+    kpic->imr = s->imr;
+    kpic->isr = s->isr;
+    kpic->priority_add = s->priority_add;
+    kpic->irq_base = s->irq_base;
+    kpic->read_reg_select = s->read_reg_select;
+    kpic->poll = s->poll;
+    kpic->special_mask = s->special_mask;
+    kpic->init_state = s->init_state;
+    kpic->auto_eoi = s->auto_eoi;
+    kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
+    kpic->special_fully_nested_mode = s->special_fully_nested_mode;
+    kpic->init4 = s->init4;
+    kpic->elcr = s->elcr;
+    kpic->elcr_mask = s->elcr_mask;
+
+    kvm_set_irqchip(&chip);
+#endif
+    return 0;
+}
+
+static void kvm_kernel_pic_save_to_user(const void *opaque)
+{
+#if defined(TARGET_I386)
+    PicState *s = (void *)opaque;
+    struct kvm_irqchip chip;
+    struct kvm_pic_state *kpic;
+
+    chip.chip_id = (&s->pics_state->pics[0] == s) ?
+                   KVM_IRQCHIP_PIC_MASTER :
+                   KVM_IRQCHIP_PIC_SLAVE;
+    kvm_get_irqchip(&chip);
+    kpic = &chip.chip.pic;
+
+    s->last_irr = kpic->last_irr;
+    s->irr = kpic->irr;
+    s->imr = kpic->imr;
+    s->isr = kpic->isr;
+    s->priority_add = kpic->priority_add;
+    s->irq_base = kpic->irq_base;
+    s->read_reg_select = kpic->read_reg_select;
+    s->poll = kpic->poll;
+    s->special_mask = kpic->special_mask;
+    s->init_state = kpic->init_state;
+    s->auto_eoi = kpic->auto_eoi;
+    s->rotate_on_auto_eoi = kpic->rotate_on_auto_eoi;
+    s->special_fully_nested_mode = kpic->special_fully_nested_mode;
+    s->init4 = kpic->init4;
+    s->elcr = kpic->elcr;
+    s->elcr_mask = kpic->elcr_mask;
+#endif
+}
+
 static const VMStateDescription vmstate_pic = {
     .name = "i8259",
     .version_id = 1,
+    .pre_save = kvm_kernel_pic_save_to_user,
+    .post_load = kvm_kernel_pic_load_from_user,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields      = (VMStateField []) {
@@ -535,3 +604,37 @@ qemu_irq *i8259_init(qemu_irq parent_irq)
     isa_pic = s;
     return qemu_allocate_irqs(i8259_set_irq, s, 16);
 }
+
+#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
+static void kvm_i8259_set_irq(void *opaque, int irq, int level)
+{
+    int pic_ret;
+    if (kvm_set_irq(irq, level, &pic_ret)) {
+        if (pic_ret != 0)
+            apic_set_irq_delivered();
+        return;
+    }
+}
+
+static void kvm_pic_init1(int io_addr, PicState *s)
+{
+    vmstate_register(io_addr, &vmstate_pic, s);
+    qemu_register_reset(pic_reset, s);
+}
+
+qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
+{
+    PicState2 *s;
+
+    s = qemu_mallocz(sizeof(PicState2));
+
+    kvm_pic_init1(0x20, &s->pics[0]);
+    kvm_pic_init1(0xa0, &s->pics[1]);
+    s->parent_irq = parent_irq;
+    s->pics[0].pics_state = s;
+    s->pics[1].pics_state = s;
+    isa_pic = s;
+    return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
+}
+#endif
+
diff --git a/hw/pc.h b/hw/pc.h
index f7c1a88..ad97d22 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -26,6 +26,7 @@ extern PicState2 *isa_pic;
 void pic_set_irq(int irq, int level);
 void pic_set_irq_new(void *opaque, int irq, int level);
 qemu_irq *i8259_init(qemu_irq parent_irq);
+qemu_irq *kvm_i8259_init(qemu_irq parent_irq);
 int pic_read_irq(PicState2 *s);
 void pic_update_irq(PicState2 *s);
 uint32_t pic_intack_read(PicState2 *s);
diff --git a/kvm-all.c b/kvm-all.c
index ff439a5..662017f 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -431,6 +431,30 @@ int kvm_get_irqchip(struct kvm_irqchip *chip)
 }
 #endif
 
+int kvm_set_irq(int irq, int level, int *status)
+{
+    struct kvm_irq_level event;
+    int r;
+
+    if (!kvm_state->irqchip_in_kernel) {
+        return 0;
+    }
+
+    event.level = level;
+    event.irq = irq;
+
+    r = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE_STATUS, &event);
+
+    if (r < 0)
+        return 0;
+
+    if (status) {
+        *status = event.status;
+    }
+
+    return 1;
+}
+
 int kvm_init(int smp_cpus)
 {
     static const char upgrade_note[] =
diff --git a/kvm.h b/kvm.h
index ae5223b..8536258 100644
--- a/kvm.h
+++ b/kvm.h
@@ -66,6 +66,8 @@ int kvm_irqchip_in_kernel(void);
 
 int kvm_set_irqchip(struct kvm_irqchip *chip);
 int kvm_get_irqchip(struct kvm_irqchip *chip);
+
+int kvm_set_irq(int irq, int level, int *status);
  
 /* internal API */
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/6] initialize i8259 chip

[Glauber Costa]

If we have irqchip in kernel (which we currently do not), intialize
the i8259 chip. This code is heavily inspirated by the one in qemu-kvm.git

Note that we wire isa irqs trough it too.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 hw/pc.c   |   14 +++++++++++---
 kvm-all.c |    2 +-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index fa82b58..3049e67 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -39,6 +39,7 @@
 #include "ide.h"
 #include "loader.h"
 #include "elf.h"
+#include "kvm.h"
 
 /* output Bochs bios info messages */
 //#define DEBUG_BIOS
@@ -1284,10 +1285,17 @@ static void pc_init1(ram_addr_t ram_size,
     }
 
     cpu_irq = qemu_allocate_irqs(pic_irq_request, NULL, 1);
-    i8259 = i8259_init(cpu_irq[0]);
     isa_irq_state = qemu_mallocz(sizeof(*isa_irq_state));
-    isa_irq_state->i8259 = i8259;
-    isa_irq = qemu_allocate_irqs(isa_irq_handler, isa_irq_state, 24);
+
+#ifdef KVM_CAP_IRQCHIP
+    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
+        isa_irq = i8259 = kvm_i8259_init(cpu_irq[0]);
+    } else
+#endif
+    {
+        isa_irq_state->i8259 = i8259 = i8259_init(cpu_irq[0]);
+        isa_irq = qemu_allocate_irqs(isa_irq_handler, isa_irq_state, 24);
+    }
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, &piix3_devfn, isa_irq);
diff --git a/kvm-all.c b/kvm-all.c
index 662017f..50616d1 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -429,7 +429,6 @@ int kvm_get_irqchip(struct kvm_irqchip *chip)
 
     return kvm_vm_ioctl(kvm_state, KVM_GET_IRQCHIP, chip);
 }
-#endif
 
 int kvm_set_irq(int irq, int level, int *status)
 {
@@ -454,6 +453,7 @@ int kvm_set_irq(int irq, int level, int *status)
 
     return 1;
 }
+#endif
 
 int kvm_init(int smp_cpus)
 {
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/6] Initialize in-kernel irqchip

[Glauber Costa]

Now that we have all devices set up, this patch initializes the irqchip.
This is dependant on the io-thread, since we need someone to pull ourselves
out of the halted state.

I believe this should be the default when we are running over the io-thread.
Later on, I plan to post a patch that makes it optional.

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

diff --git a/kvm-all.c b/kvm-all.c
index 50616d1..30df418 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -453,6 +453,22 @@ int kvm_set_irq(int irq, int level, int *status)
 
     return 1;
 }
+
+static int kvm_create_irqchip(KVMState *s)
+{
+    int ret;
+
+    if (!kvm_check_extension(s, KVM_CAP_IRQCHIP))
+        return -1;
+
+    ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
+    if (ret < 0)
+        return ret;
+
+    s->irqchip_in_kernel = 1;
+
+    return 0;
+}
 #endif
 
 int kvm_init(int smp_cpus)
@@ -536,6 +552,11 @@ int kvm_init(int smp_cpus)
     }
 #endif
 
+#if defined(CONFIG_IOTHREAD) && defined(KVM_CAP_IRQCHIP)
+    ret = kvm_create_irqchip(s);
+    if (ret < 0)
+        goto err;
+#endif
     ret = kvm_arch_init(s, smp_cpus);
     if (ret < 0)
         goto err;
-- 
1.6.2.5

[Qemu-devel] Re: [PATCH 6/6] Initialize in-kernel irqchip

[Jan Kiszka]

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

Glauber Costa wrote:
> Now that we have all devices set up, this patch initializes the irqchip.
> This is dependant on the io-thread, since we need someone to pull ourselves
> out of the halted state.
> 
> I believe this should be the default when we are running over the io-thread.
> Later on, I plan to post a patch that makes it optional.

I agree that in-kernel chips should be default if possible, but I would
keep the possibility to disable them like in qemu-kvm. This helps
specifically to track down issues in their implementations or their
potential side effects.

I have no good suggestion yet how the user interface should look like
(likely not as in qemu-kvm). Maybe some qdev property of the related
devices? But it seems like those are not yet user visible nor configurable.

Jan



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

Re: [Qemu-devel] Re: [PATCH 6/6] Initialize in-kernel irqchip

[Jamie Lokier]

Jan Kiszka wrote:
> I agree that in-kernel chips should be default if possible, but I would
> keep the possibility to disable them like in qemu-kvm. This helps
> specifically to track down issues in their implementations or their
> potential side effects.

As one of the side effects is to prevent migration to non-KVM hosts
(according to an earlier answer), I'd say the option to disable it is
essential.

Won't that option come for free when we have machine configuration
files - by simply specifying one irqchip or the other?

-- Jamie

Re: [Qemu-devel] Re: [PATCH 6/6] Initialize in-kernel irqchip

[Glauber Costa]

On Fri, Oct 2, 2009 at 6:59 PM, Jamie Lokier <jamie@shareable.org> wrote:
> Jan Kiszka wrote:
>> I agree that in-kernel chips should be default if possible, but I would
>> keep the possibility to disable them like in qemu-kvm. This helps
>> specifically to track down issues in their implementations or their
>> potential side effects.
>
> As one of the side effects is to prevent migration to non-KVM hosts
> (according to an earlier answer), I'd say the option to disable it is
> essential.
>
> Won't that option come for free when we have machine configuration
> files - by simply specifying one irqchip or the other?

Even if it does, I have code for that already.

I will post when I repost that series.

> -- Jamie
>
>
>



-- 
Glauber  Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."

[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip

[Juan Quintela]

Glauber Costa <glommer@redhat.com> wrote:
> This patch provides kvm with an in-kernel i8259 chip. We are currently not enabling it.
> The code is heavily based on what's in qemu-kvm.git.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/i8259.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/pc.h    |    1 +
>  kvm-all.c  |   24 ++++++++++++++
>  kvm.h      |    2 +
>  4 files changed, 130 insertions(+), 0 deletions(-)
>
> diff --git a/hw/i8259.c b/hw/i8259.c
> index 3de22e3..31524f5 100644
> --- a/hw/i8259.c
> +++ b/hw/i8259.c
> @@ -26,6 +26,7 @@
>  #include "isa.h"
>  #include "monitor.h"
>  #include "qemu-timer.h"
> +#include "kvm.h"
>  
>  /* debug PIC */
>  //#define DEBUG_PIC
> @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, uint32_t addr1)
>      return s->elcr;
>  }
>  
> +static int kvm_kernel_pic_load_from_user(void *opaque)
> +{
> +#if defined(TARGET_I386)
> +    PicState *s = (void *)opaque;
> +    struct kvm_irqchip chip;
> +    struct kvm_pic_state *kpic;

It miss:
   if (!kvm_enabled() && !kvm_irqchip_enabled()) {
      return 0;
   }

Or similar logic, otherwise kvm_set_irqchip() is called when kvm_irqchip
is not enabled.  Same for save_to_user.

> +    chip.chip_id = (&s->pics_state->pics[0] == s) ?
> +                   KVM_IRQCHIP_PIC_MASTER :
> +                   KVM_IRQCHIP_PIC_SLAVE;
> +    kpic = &chip.chip.pic;
> +
> +    kpic->last_irr = s->last_irr;
> +    kpic->irr = s->irr;
> +    kpic->imr = s->imr;
> +    kpic->isr = s->isr;
> +    kpic->priority_add = s->priority_add;
> +    kpic->irq_base = s->irq_base;
> +    kpic->read_reg_select = s->read_reg_select;
> +    kpic->poll = s->poll;
> +    kpic->special_mask = s->special_mask;
> +    kpic->init_state = s->init_state;
> +    kpic->auto_eoi = s->auto_eoi;
> +    kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
> +    kpic->special_fully_nested_mode = s->special_fully_nested_mode;
> +    kpic->init4 = s->init4;
> +    kpic->elcr = s->elcr;
> +    kpic->elcr_mask = s->elcr_mask;
> +
> +    kvm_set_irqchip(&chip);
> +#endif
> +    return 0;
> +}
....
>  static const VMStateDescription vmstate_pic = {
>      .name = "i8259",
>      .version_id = 1,
> +    .pre_save = kvm_kernel_pic_save_to_user,
> +    .post_load = kvm_kernel_pic_load_from_user,

Let the three version_id fields together, please.


> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> +static void kvm_i8259_set_irq(void *opaque, int irq, int level)
> +{
> +    int pic_ret;
> +    if (kvm_set_irq(irq, level, &pic_ret)) {
> +        if (pic_ret != 0)
> +            apic_set_irq_delivered();
> +        return;
> +    }
> +}
> +
> +static void kvm_pic_init1(int io_addr, PicState *s)
> +{
> +    vmstate_register(io_addr, &vmstate_pic, s);
> +    qemu_register_reset(pic_reset, s);
> +}
> +
> +qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
> +{
> +    PicState2 *s;
> +
> +    s = qemu_mallocz(sizeof(PicState2));
> +
> +    kvm_pic_init1(0x20, &s->pics[0]);
> +    kvm_pic_init1(0xa0, &s->pics[1]);
> +    s->parent_irq = parent_irq;
> +    s->pics[0].pics_state = s;
> +    s->pics[1].pics_state = s;
> +    isa_pic = s;
> +    return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
> +}
> +#endif

I think everything would be nicer if this three functions where merged
with the _non_ kvm ones with a kvm_enable() test.  They only differ in
2-3 lines.

Later, Juan.

[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip

[Glauber Costa]

On Tue, Sep 29, 2009 at 12:04:54AM +0200, Juan Quintela wrote:
> Glauber Costa <glommer@redhat.com> wrote:
> > This patch provides kvm with an in-kernel i8259 chip. We are currently not enabling it.
> > The code is heavily based on what's in qemu-kvm.git.
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > ---
> >  hw/i8259.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/pc.h    |    1 +
> >  kvm-all.c  |   24 ++++++++++++++
> >  kvm.h      |    2 +
> >  4 files changed, 130 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/i8259.c b/hw/i8259.c
> > index 3de22e3..31524f5 100644
> > --- a/hw/i8259.c
> > +++ b/hw/i8259.c
> > @@ -26,6 +26,7 @@
> >  #include "isa.h"
> >  #include "monitor.h"
> >  #include "qemu-timer.h"
> > +#include "kvm.h"
> >  
> >  /* debug PIC */
> >  //#define DEBUG_PIC
> > @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, uint32_t addr1)
> >      return s->elcr;
> >  }
> >  
> > +static int kvm_kernel_pic_load_from_user(void *opaque)
> > +{
> > +#if defined(TARGET_I386)
> > +    PicState *s = (void *)opaque;
> > +    struct kvm_irqchip chip;
> > +    struct kvm_pic_state *kpic;
> 
> It miss:
>    if (!kvm_enabled() && !kvm_irqchip_enabled()) {
>       return 0;
>    }
> 
> Or similar logic, otherwise kvm_set_irqchip() is called when kvm_irqchip
> is not enabled.  Same for save_to_user.
> 
> > +    chip.chip_id = (&s->pics_state->pics[0] == s) ?
> > +                   KVM_IRQCHIP_PIC_MASTER :
> > +                   KVM_IRQCHIP_PIC_SLAVE;
> > +    kpic = &chip.chip.pic;
> > +
> > +    kpic->last_irr = s->last_irr;
> > +    kpic->irr = s->irr;
> > +    kpic->imr = s->imr;
> > +    kpic->isr = s->isr;
> > +    kpic->priority_add = s->priority_add;
> > +    kpic->irq_base = s->irq_base;
> > +    kpic->read_reg_select = s->read_reg_select;
> > +    kpic->poll = s->poll;
> > +    kpic->special_mask = s->special_mask;
> > +    kpic->init_state = s->init_state;
> > +    kpic->auto_eoi = s->auto_eoi;
> > +    kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
> > +    kpic->special_fully_nested_mode = s->special_fully_nested_mode;
> > +    kpic->init4 = s->init4;
> > +    kpic->elcr = s->elcr;
> > +    kpic->elcr_mask = s->elcr_mask;
> > +
> > +    kvm_set_irqchip(&chip);
> > +#endif
> > +    return 0;
> > +}
> ....
> >  static const VMStateDescription vmstate_pic = {
> >      .name = "i8259",
> >      .version_id = 1,
> > +    .pre_save = kvm_kernel_pic_save_to_user,
> > +    .post_load = kvm_kernel_pic_load_from_user,
> 
> Let the three version_id fields together, please.
> 
> 
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +static void kvm_i8259_set_irq(void *opaque, int irq, int level)
> > +{
> > +    int pic_ret;
> > +    if (kvm_set_irq(irq, level, &pic_ret)) {
> > +        if (pic_ret != 0)
> > +            apic_set_irq_delivered();
> > +        return;
> > +    }
> > +}
> > +
> > +static void kvm_pic_init1(int io_addr, PicState *s)
> > +{
> > +    vmstate_register(io_addr, &vmstate_pic, s);
> > +    qemu_register_reset(pic_reset, s);
> > +}
> > +
> > +qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
> > +{
> > +    PicState2 *s;
> > +
> > +    s = qemu_mallocz(sizeof(PicState2));
> > +
> > +    kvm_pic_init1(0x20, &s->pics[0]);
> > +    kvm_pic_init1(0xa0, &s->pics[1]);
> > +    s->parent_irq = parent_irq;
> > +    s->pics[0].pics_state = s;
> > +    s->pics[1].pics_state = s;
> > +    isa_pic = s;
> > +    return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
> > +}
> > +#endif
> 
> I think everything would be nicer if this three functions where merged
> with the _non_ kvm ones with a kvm_enable() test.  They only differ in
> 2-3 lines.
I disagree. I think it is a better solution long term to provide irqchips
that are completely free of kvm code.

[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip

[Juan Quintela]

Glauber Costa <glommer@redhat.com> wrote:
> On Tue, Sep 29, 2009 at 12:04:54AM +0200, Juan Quintela wrote:
>> Glauber Costa <glommer@redhat.com> wrote:
>> > This patch provides kvm with an in-kernel i8259 chip. We are currently not enabling it.
>> > The code is heavily based on what's in qemu-kvm.git.
>> >
>> > Signed-off-by: Glauber Costa <glommer@redhat.com>
>> > ---
>> >  hw/i8259.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  hw/pc.h    |    1 +
>> >  kvm-all.c  |   24 ++++++++++++++
>> >  kvm.h      |    2 +
>> >  4 files changed, 130 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/hw/i8259.c b/hw/i8259.c
>> > index 3de22e3..31524f5 100644
>> > --- a/hw/i8259.c
>> > +++ b/hw/i8259.c
>> > @@ -26,6 +26,7 @@
>> >  #include "isa.h"
>> >  #include "monitor.h"
>> >  #include "qemu-timer.h"
>> > +#include "kvm.h"
>> >  
>> >  /* debug PIC */
>> >  //#define DEBUG_PIC
>> > @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, uint32_t addr1)
>> >      return s->elcr;
>> >  }
>> >  
>> > +static int kvm_kernel_pic_load_from_user(void *opaque)
>> > +{
>> > +#if defined(TARGET_I386)
>> > +    PicState *s = (void *)opaque;
>> > +    struct kvm_irqchip chip;
>> > +    struct kvm_pic_state *kpic;
>> 
>> It miss:
>>    if (!kvm_enabled() && !kvm_irqchip_enabled()) {
>>       return 0;
>>    }
>> 
>> Or similar logic, otherwise kvm_set_irqchip() is called when kvm_irqchip
>> is not enabled.  Same for save_to_user.
>> 
>> > +    chip.chip_id = (&s->pics_state->pics[0] == s) ?
>> > +                   KVM_IRQCHIP_PIC_MASTER :
>> > +                   KVM_IRQCHIP_PIC_SLAVE;
>> > +    kpic = &chip.chip.pic;
>> > +
>> > +    kpic->last_irr = s->last_irr;
>> > +    kpic->irr = s->irr;
>> > +    kpic->imr = s->imr;
>> > +    kpic->isr = s->isr;
>> > +    kpic->priority_add = s->priority_add;
>> > +    kpic->irq_base = s->irq_base;
>> > +    kpic->read_reg_select = s->read_reg_select;
>> > +    kpic->poll = s->poll;
>> > +    kpic->special_mask = s->special_mask;
>> > +    kpic->init_state = s->init_state;
>> > +    kpic->auto_eoi = s->auto_eoi;
>> > +    kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
>> > +    kpic->special_fully_nested_mode = s->special_fully_nested_mode;
>> > +    kpic->init4 = s->init4;
>> > +    kpic->elcr = s->elcr;
>> > +    kpic->elcr_mask = s->elcr_mask;
>> > +
>> > +    kvm_set_irqchip(&chip);
>> > +#endif
>> > +    return 0;
>> > +}
>> ....
>> >  static const VMStateDescription vmstate_pic = {
>> >      .name = "i8259",
>> >      .version_id = 1,
>> > +    .pre_save = kvm_kernel_pic_save_to_user,
>> > +    .post_load = kvm_kernel_pic_load_from_user,
>> 
>> Let the three version_id fields together, please.
>> 
>> 
>> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>> > +static void kvm_i8259_set_irq(void *opaque, int irq, int level)
>> > +{
>> > +    int pic_ret;
>> > +    if (kvm_set_irq(irq, level, &pic_ret)) {
>> > +        if (pic_ret != 0)
>> > +            apic_set_irq_delivered();
>> > +        return;
>> > +    }
>> > +}
>> > +
>> > +static void kvm_pic_init1(int io_addr, PicState *s)
>> > +{
>> > +    vmstate_register(io_addr, &vmstate_pic, s);
>> > +    qemu_register_reset(pic_reset, s);
>> > +}
>> > +
>> > +qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
>> > +{
>> > +    PicState2 *s;
>> > +
>> > +    s = qemu_mallocz(sizeof(PicState2));
>> > +
>> > +    kvm_pic_init1(0x20, &s->pics[0]);
>> > +    kvm_pic_init1(0xa0, &s->pics[1]);
>> > +    s->parent_irq = parent_irq;
>> > +    s->pics[0].pics_state = s;
>> > +    s->pics[1].pics_state = s;
>> > +    isa_pic = s;
>> > +    return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
>> > +}
>> > +#endif
>> 
>> I think everything would be nicer if this three functions where merged
>> with the _non_ kvm ones with a kvm_enable() test.  They only differ in
>> 2-3 lines.
> I disagree. I think it is a better solution long term to provide irqchips
> that are completely free of kvm code.

Solutions:
- you copy the file and lives synchronizing the changes
- you export the needed funtions and then implement in the other file
  the kvm bits.
- you merge the kvm and non kvm bits.

I see here a very bad mix :(  The error showed before is due to the bad mix.

I showed 1 error, 1 question of style and 1 suggestion, you only
answered to the suggestion.

Later, Juan.

[Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip

[Jan Kiszka]

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

Juan Quintela wrote:
> Glauber Costa <glommer@redhat.com> wrote:
>> On Tue, Sep 29, 2009 at 12:04:54AM +0200, Juan Quintela wrote:
>>> Glauber Costa <glommer@redhat.com> wrote:
>>>> This patch provides kvm with an in-kernel i8259 chip. We are currently not enabling it.
>>>> The code is heavily based on what's in qemu-kvm.git.
>>>>
>>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>>> ---
>>>>  hw/i8259.c |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  hw/pc.h    |    1 +
>>>>  kvm-all.c  |   24 ++++++++++++++
>>>>  kvm.h      |    2 +
>>>>  4 files changed, 130 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/i8259.c b/hw/i8259.c
>>>> index 3de22e3..31524f5 100644
>>>> --- a/hw/i8259.c
>>>> +++ b/hw/i8259.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include "isa.h"
>>>>  #include "monitor.h"
>>>>  #include "qemu-timer.h"
>>>> +#include "kvm.h"
>>>>  
>>>>  /* debug PIC */
>>>>  //#define DEBUG_PIC
>>>> @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, uint32_t addr1)
>>>>      return s->elcr;
>>>>  }
>>>>  
>>>> +static int kvm_kernel_pic_load_from_user(void *opaque)
>>>> +{
>>>> +#if defined(TARGET_I386)
>>>> +    PicState *s = (void *)opaque;
>>>> +    struct kvm_irqchip chip;
>>>> +    struct kvm_pic_state *kpic;
>>> It miss:
>>>    if (!kvm_enabled() && !kvm_irqchip_enabled()) {
>>>       return 0;
>>>    }
>>>
>>> Or similar logic, otherwise kvm_set_irqchip() is called when kvm_irqchip
>>> is not enabled.  Same for save_to_user.
>>>
>>>> +    chip.chip_id = (&s->pics_state->pics[0] == s) ?
>>>> +                   KVM_IRQCHIP_PIC_MASTER :
>>>> +                   KVM_IRQCHIP_PIC_SLAVE;
>>>> +    kpic = &chip.chip.pic;
>>>> +
>>>> +    kpic->last_irr = s->last_irr;
>>>> +    kpic->irr = s->irr;
>>>> +    kpic->imr = s->imr;
>>>> +    kpic->isr = s->isr;
>>>> +    kpic->priority_add = s->priority_add;
>>>> +    kpic->irq_base = s->irq_base;
>>>> +    kpic->read_reg_select = s->read_reg_select;
>>>> +    kpic->poll = s->poll;
>>>> +    kpic->special_mask = s->special_mask;
>>>> +    kpic->init_state = s->init_state;
>>>> +    kpic->auto_eoi = s->auto_eoi;
>>>> +    kpic->rotate_on_auto_eoi = s->rotate_on_auto_eoi;
>>>> +    kpic->special_fully_nested_mode = s->special_fully_nested_mode;
>>>> +    kpic->init4 = s->init4;
>>>> +    kpic->elcr = s->elcr;
>>>> +    kpic->elcr_mask = s->elcr_mask;
>>>> +
>>>> +    kvm_set_irqchip(&chip);
>>>> +#endif
>>>> +    return 0;
>>>> +}
>>> ....
>>>>  static const VMStateDescription vmstate_pic = {
>>>>      .name = "i8259",
>>>>      .version_id = 1,
>>>> +    .pre_save = kvm_kernel_pic_save_to_user,
>>>> +    .post_load = kvm_kernel_pic_load_from_user,
>>> Let the three version_id fields together, please.
>>>
>>>
>>>> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
>>>> +static void kvm_i8259_set_irq(void *opaque, int irq, int level)
>>>> +{
>>>> +    int pic_ret;
>>>> +    if (kvm_set_irq(irq, level, &pic_ret)) {
>>>> +        if (pic_ret != 0)
>>>> +            apic_set_irq_delivered();
>>>> +        return;
>>>> +    }
>>>> +}
>>>> +
>>>> +static void kvm_pic_init1(int io_addr, PicState *s)
>>>> +{
>>>> +    vmstate_register(io_addr, &vmstate_pic, s);
>>>> +    qemu_register_reset(pic_reset, s);
>>>> +}
>>>> +
>>>> +qemu_irq *kvm_i8259_init(qemu_irq parent_irq)
>>>> +{
>>>> +    PicState2 *s;
>>>> +
>>>> +    s = qemu_mallocz(sizeof(PicState2));
>>>> +
>>>> +    kvm_pic_init1(0x20, &s->pics[0]);
>>>> +    kvm_pic_init1(0xa0, &s->pics[1]);
>>>> +    s->parent_irq = parent_irq;
>>>> +    s->pics[0].pics_state = s;
>>>> +    s->pics[1].pics_state = s;
>>>> +    isa_pic = s;
>>>> +    return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24);
>>>> +}
>>>> +#endif
>>> I think everything would be nicer if this three functions where merged
>>> with the _non_ kvm ones with a kvm_enable() test.  They only differ in
>>> 2-3 lines.
>> I disagree. I think it is a better solution long term to provide irqchips
>> that are completely free of kvm code.
> 
> Solutions:
> - you copy the file and lives synchronizing the changes
> - you export the needed funtions and then implement in the other file
>   the kvm bits.
> - you merge the kvm and non kvm bits.
> 
> I see here a very bad mix :(  The error showed before is due to the bad mix.
> 
> I showed 1 error, 1 question of style and 1 suggestion, you only
> answered to the suggestion.
> 
> Later, Juan.
> 

I agree with Juan here. And I would recommend to go the second path:
factor out shared code, e.g. into apic_common.c, and implement the
different variants in different files, maybe even as separate qdev
devices. The current situation is fairly unfortunate IMO.

Jan


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

[Qemu-devel] Re: [PATCH 1/6] provide in-kernel ioapic

[Glauber Costa]

On Mon, Sep 28, 2009 at 11:50:46PM +0200, Juan Quintela wrote:
> Glauber Costa <glommer@redhat.com> wrote:
> > This patch provides kvm with an in-kernel ioapic. We are currently not enabling it.
> > The code is heavily based on what's in qemu-kvm.git.
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> 
> 
> base_address IOAPICState field missing.
> 
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +static void kvm_kernel_ioapic_load_from_user(IOAPICState *s)
> > +{
> > +    struct kvm_irqchip chip;
> > +    struct kvm_ioapic_state *kioapic;
> > +    int i;
> > +
> 
> if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
>    return;
No need. if this function is ever called and this is not true, this is a bug.

> 
> > +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> > +    kioapic = &chip.chip.ioapic;
> > +
> > +    kioapic->id = s->id;
> > +    kioapic->ioregsel = s->ioregsel;
> > +    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
> > +    kioapic->irr = s->irr;
> > +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> > +        kioapic->redirtbl[i].bits = s->ioredtbl[i];
> > +    }
> > +
> > +    kvm_set_irqchip(&chip);
> > +}
> > +
> > +static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
> > +{
> > +    struct kvm_irqchip chip;
> > +    struct kvm_ioapic_state *kioapic;
> > +    int i;
> 
> if (!kvm_enabled() || !kvm_irqchip_in_kernel()) {
>    return;
ditto.

> 
> > +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> > +    kvm_get_irqchip(&chip);
> > +    kioapic = &chip.chip.ioapic;
> > +
> > +    s->id = kioapic->id;
> > +    s->ioregsel = kioapic->ioregsel;
> > +    s->irr = kioapic->irr;
> > +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> > +        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
> > +    }
> > +}
> > +#endif
> > +
> > +static void ioapic_pre_save(const void *opaque)
> > +{
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +    IOAPICState *s = (void *)opaque;
> > +
> > +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> 
> Can we put this test inside the functions?  Yes, I know that qemu-kvm
> uses always this syntax, but it just makes the API simpler to use.

I am not sure if we are aiming for a simpler api here. Those functions should never
be called if we're not using in-kernel irqchip, and it is probably better to have
callers to enforce it.

> 
> > +        kvm_kernel_ioapic_save_to_user(s);
> > +    }
> > +#endif
> > +}
> > +
> > +static int ioapic_pre_load(void *opaque)
> > +{
> > +    IOAPICState *s = opaque;
> > +
> > +    /* in case we are doing version 1, we just set these to sane
> > values */
> 
> You forgot to update vmstate_ioapic to version 2.
ok.

> 
> > +    s->irr = 0;
> > +    return 0;
> > +}
> > +
> > +static int ioapic_post_load(void *opaque)
> > +{
> > +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> > +    IOAPICState *s = opaque;
> > +
> > +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> > +        kvm_kernel_ioapic_load_from_user(s);
> > +    }
> > +#endif
> > +    return 0;
> > +}
> > +
> > +
> >  static const VMStateDescription vmstate_ioapic = {
> >      .name = "ioapic",
> >      .version_id = 1,
> > @@ -201,7 +276,10 @@ static const VMStateDescription vmstate_ioapic = {
> >          VMSTATE_UINT8(ioregsel, IOAPICState),
> >          VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
> >          VMSTATE_END_OF_LIST()
> > -    }
> > +    },
> > +    .pre_load = ioapic_pre_load,
> > +    .post_load = ioapic_post_load,
> > +    .pre_save = ioapic_pre_save,
> >  };
> 

Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Jamie Lokier]

Glauber Costa wrote:
>I am ready to advocate that it should be the default, whenever
>available.  I plan to, however, provide a patch that disables it
>whenever this gets merged.

Is the in-kernel irqchip simply intended to accelerate some part of
the CPU/irq emulation, so that you could enable or disable it at will,
as well as migrating to TCG-using systems and back, with no observable
effect except performance?

-- Jamie

Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Glauber Costa]

On Tue, Sep 29, 2009 at 01:39:58AM +0100, Jamie Lokier wrote:
> Glauber Costa wrote:
> >I am ready to advocate that it should be the default, whenever
> >available.  I plan to, however, provide a patch that disables it
> >whenever this gets merged.
> 
> Is the in-kernel irqchip simply intended to accelerate some part of
> the CPU/irq emulation, so that you could enable or disable it at will,
> as well as migrating to TCG-using systems and back, with no observable
> effect except performance?
I don't think we can migrate between kvm and tcg. Although it should be possible
in theory, since we just load the state, there are some differences, specially
with respect to memory layout between kvm and pure tcg. i.e., IIRC, kvm puts two
extra pages in the end of the registered physical memory. Although it can be possible,
I don't think anyone has enough interest to make it happen.

As for in-kernel chip, to be honest, I have never tried migrating between two system,
one having it, and one lacking it. But I believe it should be possible, and this
is a much easier goal to accomplish.

Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Avi Kivity]

On 09/28/2009 11:15 PM, Glauber Costa wrote:
> Hi people,
>
> This (sent with my correct e-mail address) is a port from qemu-kvm.git of
> the in-kernel irqchip support. Right now, I am not providing any way to disable
> it. I am ready to advocate that it should be the default, whenever available.
>
> I plan to, however, provide a patch that disables it whenever this gets merged.
>
> I gave it basic testing, specially with reboot scenarios (due to the load/save)
> functions, and all seems to be working well. But I have only tested linux guests
> so far.
>
> Anthony, I believe it is, although not perfect, in inclusion state.
>
>    

Very nice work.  But please, if there are any differences between the 
code and qemu-kvm.git, send patches to qemu-kvm.git so we don't have 
needless breakage later.

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

Re: [Qemu-devel] [PATCH 0/6] in kernel irqchip support

[Anthony Liguori]

Glauber Costa wrote:
> Hi people,
>
> This (sent with my correct e-mail address) is a port from qemu-kvm.git of
> the in-kernel irqchip support. Right now, I am not providing any way to disable
> it. I am ready to advocate that it should be the default, whenever available.
>
> I plan to, however, provide a patch that disables it whenever this gets merged.
>
> I gave it basic testing, specially with reboot scenarios (due to the load/save)
> functions, and all seems to be working well. But I have only tested linux guests
> so far.
>
> Anthony, I believe it is, although not perfect, in inclusion state.
>   

It breaks the build badly when !CONFIG_IOTHREAD--which is still the default.

Regards,

Anthony Liguori