From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MtopC-0003eo-0r for qemu-devel@nongnu.org; Fri, 02 Oct 2009 16:33:58 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mtop7-0003be-Dx for qemu-devel@nongnu.org; Fri, 02 Oct 2009 16:33:57 -0400 Received: from [199.232.76.173] (port=41199 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mtop7-0003bP-1x for qemu-devel@nongnu.org; Fri, 02 Oct 2009 16:33:53 -0400 Received: from fmmailgate03.web.de ([217.72.192.234]:47917) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mtop6-0002pk-DD for qemu-devel@nongnu.org; Fri, 02 Oct 2009 16:33:52 -0400 Message-ID: <4AC663AB.9040200@web.de> Date: Fri, 02 Oct 2009 22:33:47 +0200 From: Jan Kiszka MIME-Version: 1.0 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> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig23087A73BAC40BC47DD7FE60" Sender: jan.kiszka@web.de 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: Glauber Costa , aliguori@us.ibm.com, qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig23087A73BAC40BC47DD7FE60 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Juan Quintela wrote: > 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 current= ly 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" >>>> =20 >>>> /* debug PIC */ >>>> //#define DEBUG_PIC >>>> @@ -446,9 +447,77 @@ static uint32_t elcr_ioport_read(void *opaque, = uint32_t addr1) >>>> return s->elcr; >>>> } >>>> =20 >>>> +static int kvm_kernel_pic_load_from_user(void *opaque) >>>> +{ >>>> +#if defined(TARGET_I386) >>>> + PicState *s =3D (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_irqc= hip >>> is not enabled. Same for save_to_user. >>> >>>> + chip.chip_id =3D (&s->pics_state->pics[0] =3D=3D s) ? >>>> + KVM_IRQCHIP_PIC_MASTER : >>>> + KVM_IRQCHIP_PIC_SLAVE; >>>> + kpic =3D &chip.chip.pic; >>>> + >>>> + kpic->last_irr =3D s->last_irr; >>>> + kpic->irr =3D s->irr; >>>> + kpic->imr =3D s->imr; >>>> + kpic->isr =3D s->isr; >>>> + kpic->priority_add =3D s->priority_add; >>>> + kpic->irq_base =3D s->irq_base; >>>> + kpic->read_reg_select =3D s->read_reg_select; >>>> + kpic->poll =3D s->poll; >>>> + kpic->special_mask =3D s->special_mask; >>>> + kpic->init_state =3D s->init_state; >>>> + kpic->auto_eoi =3D s->auto_eoi; >>>> + kpic->rotate_on_auto_eoi =3D s->rotate_on_auto_eoi; >>>> + kpic->special_fully_nested_mode =3D s->special_fully_nested_mod= e; >>>> + kpic->init4 =3D s->init4; >>>> + kpic->elcr =3D s->elcr; >>>> + kpic->elcr_mask =3D s->elcr_mask; >>>> + >>>> + kvm_set_irqchip(&chip); >>>> +#endif >>>> + return 0; >>>> +} >>> .... >>>> static const VMStateDescription vmstate_pic =3D { >>>> .name =3D "i8259", >>>> .version_id =3D 1, >>>> + .pre_save =3D kvm_kernel_pic_save_to_user, >>>> + .post_load =3D 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 !=3D 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 =3D qemu_mallocz(sizeof(PicState2)); >>>> + >>>> + kvm_pic_init1(0x20, &s->pics[0]); >>>> + kvm_pic_init1(0xa0, &s->pics[1]); >>>> + s->parent_irq =3D parent_irq; >>>> + s->pics[0].pics_state =3D s; >>>> + s->pics[1].pics_state =3D s; >>>> + isa_pic =3D s; >>>> + return qemu_allocate_irqs(kvm_i8259_set_irq, s, 24); >>>> +} >>>> +#endif >>> I think everything would be nicer if this three functions where merge= d >>> with the _non_ kvm ones with a kvm_enable() test. They only differ i= n >>> 2-3 lines. >> I disagree. I think it is a better solution long term to provide irqch= ips >> that are completely free of kvm code. >=20 > 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. >=20 > I see here a very bad mix :( The error showed before is due to the bad= mix. >=20 > I showed 1 error, 1 question of style and 1 suggestion, you only > answered to the suggestion. >=20 > Later, Juan. >=20 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 --------------enig23087A73BAC40BC47DD7FE60 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkrGY6sACgkQitSsb3rl5xRyGACfaXbMtS8VBnZi7L7+Vawg3rCt nHcAoMJDCSgep6wcZvCbsW4Ggd4MhLMt =+rtR -----END PGP SIGNATURE----- --------------enig23087A73BAC40BC47DD7FE60--