From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MsOsx-0001Sz-Bi for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:39:59 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MsOss-0001Qb-2W for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:39:57 -0400 Received: from [199.232.76.173] (port=37614 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MsOsr-0001QO-Fv for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:39:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46445) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MsOsr-0007c6-0C for qemu-devel@nongnu.org; Mon, 28 Sep 2009 18:39:53 -0400 From: Juan Quintela In-Reply-To: <20090928222515.GR29735@mothafucka.localdomain> (Glauber Costa's message of "Mon, 28 Sep 2009 19:25:15 -0300") 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> <20090928222515.GR29735@mothafucka.localdomain> Date: Tue, 29 Sep 2009 00:39:43 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Glauber Costa Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org Glauber Costa wrote: > 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. 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.