From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58209) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V4GC5-000682-AJ for qemu-devel@nongnu.org; Tue, 30 Jul 2013 16:06:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V4GBz-000314-OF for qemu-devel@nongnu.org; Tue, 30 Jul 2013 16:06:53 -0400 Message-ID: <51F81CC6.8090104@reactos.org> Date: Tue, 30 Jul 2013 22:06:30 +0200 From: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= MIME-Version: 1.0 References: <1374614206-9368-1-git-send-email-hpoussin@reactos.org> <1374614206-9368-3-git-send-email-hpoussin@reactos.org> <51F6E46A.6080708@web.de> In-Reply-To: <51F6E46A.6080708@web.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Paolo Bonzini , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Anthony Liguori Andreas F=C3=A4rber a =C3=A9crit : > Hi, >=20 > Am 23.07.2013 23:16, schrieb Herv=C3=A9 Poussineau: >> - i82378 only exists on PCI bus ; do not split implementation in 2 str= uctures >> - remove BARs, which are not specified in datasheet >> - replace custom isa_mmio implementation by PCI bus IO region usage >> - use QOM casts when required >> >> Signed-off-by: Herv=C3=A9 Poussineau >=20 > Thanks for adopting some of the latest patterns without being asked to! >=20 > I've queued this with some style changes, but apart from testing issues > as CC'ed, I have a major question/concern in the bottom... >=20 >> --- >> hw/isa/i82378.c | 220 ++++++++++++----------------------------------= --------- >> 1 file changed, 48 insertions(+), 172 deletions(-) >> >> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c >> index de71d81..f2045de 100644 >> --- a/hw/isa/i82378.c >> +++ b/hw/isa/i82378.c [...] >> @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque,= int irq, int level) >> static void i82378_request_pic_irq(void *opaque, int irq, int level) >> { >> DeviceState *dev =3D opaque; >> - PCIDevice *pci =3D DO_UPCAST(PCIDevice, qdev, dev); >> - PCIi82378State *s =3D DO_UPCAST(PCIi82378State, pci_dev, pci); >> - >> - qemu_set_irq(s->state.i8259[irq], level); >> + qemu_set_irq(I82378(dev)->i8259[irq], level); >=20 > Changed that back to a variable s - FOO(bar)->baz is undesired. OK >=20 >> } >> =20 >> -static void i82378_init(DeviceState *dev, I82378State *s) >> +static void i82378_realize(DeviceState *dev, Error **errp) >> { >> - ISABus *isabus =3D ISA_BUS(qdev_get_child_bus(dev, "isa.0")); >> - ISADevice *pit; >> + PCIDevice *pci =3D PCI_DEVICE(dev); >> + I82378State *s =3D I82378(dev); >> + DeviceClass *dc; >> + uint8_t *pci_conf; >> + ISABus *isabus; >> ISADevice *isa; >> qemu_irq *out0_irq; >> =20 >> + dc =3D DEVICE_CLASS(object_class_get_parent(object_get_class(OBJE= CT(dev)))); >=20 > This is going into uncharted territories. ;) I consider it wrong to use > object_get_class() - we should use object_class_by_name() to allow for > derived types and I'll put it into a macro that I'll try to align with > Peter C.'s and my QOM work. OK >> + assert(dc); >=20 > This shouldn't be necessary? OK. It can be removed. >=20 >> + dc->realize(dev, errp); >> + if (error_is_set(errp)) { >> + return; >=20 > This doesn't do what you want. You need a local err variable to return > here, errp might be NULL =3D> !error_is_set(errp). OK [...] >> =20 >> - k->init =3D pci_i82378_init; >> k->vendor_id =3D PCI_VENDOR_ID_INTEL; >> k->device_id =3D PCI_DEVICE_ID_INTEL_82378; >> k->revision =3D 0x03; >> k->class_id =3D PCI_CLASS_BRIDGE_ISA; >> - k->subsystem_vendor_id =3D 0x0; >> - k->subsystem_id =3D 0x0; >> - dc->vmsd =3D &vmstate_pci_i82378; >> - dc->props =3D i82378_properties; >> + dc->realize =3D i82378_realize; >> + dc->vmsd =3D &vmstate_i82378; >=20 > (FWIW dc->categories has been merged here.) Yes, but it has been merged after I sent this patch... >=20 >> + dc->no_user =3D 1; >=20 > Why do you do this? For one, according to Anthony it should no longer b= e > used, and for another, Paolo's endianness-test (make check) is using > -device i82378 for various other ppc and sh4 machines IIUC. make check > still succeeds for ppc with this patch though, so that might be due tot > -device ignoring DeviceClass::no_user? I probably copied it from another chipset device, maybe i440fx. I don't really mind removing it. Yes, I double-checked that make check still works for all architectures. > Hoping to get this in shape for -rc1. Sure. Should I send a v2, as it seems you already queued it? Herv=C3=A9