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

[Glauber Costa <glommer@redhat.com>]

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.