From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MsOeq-00020o-Pe for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:25:24 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MsOem-00020W-2z for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:25:24 -0400 Received: from [199.232.76.173] (port=40044 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MsOel-00020T-UV for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:25:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46055) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MsOel-00057H-GH for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:25:19 -0400 Date: Mon, 28 Sep 2009 19:25:15 -0300 From: Glauber Costa Message-ID: <20090928222515.GR29735@mothafucka.localdomain> References: <1254172517-28216-1-git-send-email-glommer@redhat.com> <1254172517-28216-2-git-send-email-glommer@redhat.com> <1254172517-28216-3-git-send-email-glommer@redhat.com> <1254172517-28216-4-git-send-email-glommer@redhat.com> <1254172517-28216-5-git-send-email-glommer@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH 4/6] provide in-kernel i8259 chip List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On Tue, Sep 29, 2009 at 12:04:54AM +0200, Juan Quintela wrote: > Glauber Costa 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 > > --- > > 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.