Patchwork [4/6] provide in-kernel i8259 chip

login
register
mail settings
Submitter Glauber Costa
Date Sept. 28, 2009, 9:15 p.m.
Message ID <1254172517-28216-5-git-send-email-glommer@redhat.com>
Download mbox | patch
Permalink /patch/34398/
State Superseded
Headers show

Comments

Glauber Costa - Sept. 28, 2009, 9:15 p.m.
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(-)
Juan Quintela - Sept. 28, 2009, 10:04 p.m.
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.
Glauber Costa - Sept. 28, 2009, 10:25 p.m.
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.
Juan Quintela - Sept. 28, 2009, 10:39 p.m.
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.
Jan Kiszka - Oct. 2, 2009, 8:33 p.m.
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

Patch

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 */