From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43468) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1NHs-0000Cb-8Y for qemu-devel@nongnu.org; Mon, 22 Jul 2013 17:05:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1NHr-0008HA-0O for qemu-devel@nongnu.org; Mon, 22 Jul 2013 17:04:56 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41978 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1NHq-0008Gf-No for qemu-devel@nongnu.org; Mon, 22 Jul 2013 17:04:54 -0400 Message-ID: <51ED9E71.3060809@suse.de> Date: Mon, 22 Jul 2013 23:04:49 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1374415744-6675-1-git-send-email-afaerber@suse.de> <1374415744-6675-5-git-send-email-afaerber@suse.de> <20130721202607.GA15187@redhat.com> <87y58y49tx.fsf@codemonkey.ws> In-Reply-To: <87y58y49tx.fsf@codemonkey.ws> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori , "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org Am 22.07.2013 22:29, schrieb Anthony Liguori: > "Michael S. Tsirkin" writes: >=20 >> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas F=C3=A4rber wrote: >>> Signed-off-by: Andreas F=C3=A4rber >>> --- >>> hw/pci-bridge/ioh3420.c | 23 ++++++++++------------- >>> hw/pci-bridge/xio3130_downstream.c | 23 ++++++++++------------- >>> hw/pci-bridge/xio3130_upstream.c | 15 +++++++-------- >>> hw/pci/pcie_port.c | 22 ++++++++++++++++++++++ >>> include/hw/pci/pcie_port.h | 14 ++++++++++++-- >>> 5 files changed, 61 insertions(+), 36 deletions(-) >>> >>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c >>> index 728f658..83db054 100644 >>> --- a/hw/pci-bridge/ioh3420.c >>> +++ b/hw/pci-bridge/ioh3420.c >>> @@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev) >>> =20 >>> static int ioh3420_initfn(PCIDevice *d) >>> { >>> - PCIBridge *br =3D PCI_BRIDGE(d); >>> - PCIEPort *p =3D DO_UPCAST(PCIEPort, br, br); >>> - PCIESlot *s =3D DO_UPCAST(PCIESlot, port, p); >>> + PCIEPort *p =3D PCIE_PORT(d); >>> + PCIESlot *s =3D PCIE_SLOT(d); >>> int rc; >>> =20 >>> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >>> @@ -148,9 +147,7 @@ err_bridge: >>> =20 >>> static void ioh3420_exitfn(PCIDevice *d) >>> { >>> - PCIBridge *br =3D PCI_BRIDGE(d); >>> - PCIEPort *p =3D DO_UPCAST(PCIEPort, br, br); >>> - PCIESlot *s =3D DO_UPCAST(PCIESlot, port, p); >>> + PCIESlot *s =3D PCIE_SLOT(d); >>> =20 >>> pcie_aer_exit(d); >>> pcie_chassis_del_slot(s); >>> @@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bo= ol multifunction, >>> qdev_prop_set_uint16(qdev, "slot", slot); >>> qdev_init_nofail(qdev); >>> =20 >>> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br)); >>> + return PCIE_SLOT(d); >>> } >>> =20 >>> static const VMStateDescription vmstate_ioh3420 =3D { >>> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420= =3D { >>> .minimum_version_id_old =3D 1, >>> .post_load =3D pcie_cap_slot_post_load, >>> .fields =3D (VMStateField[]) { >>> - VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot), >>> - VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0, >>> + VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge), >>> + VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0, >>> vmstate_pcie_aer_log, PCIEAERLog), >>> VMSTATE_END_OF_LIST() >>> } >>> }; >>> =20 >>> static Property ioh3420_properties[] =3D { >>> - DEFINE_PROP_UINT8("port", PCIESlot, port.port, 0), >>> + DEFINE_PROP_UINT8("port", PCIEPort, port, 0), >>> DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), >>> DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), >>> - DEFINE_PROP_UINT16("aer_log_max", PCIESlot, >>> - port.br.parent_obj.exp.aer_log.log_max, >>> + DEFINE_PROP_UINT16("aer_log_max", PCIDevice, >>> + exp.aer_log.log_max, >>> PCIE_AER_LOG_MAX_DEFAULT), >>> DEFINE_PROP_END_OF_LIST(), >>> }; >> >> >> This looks scary. This does a cast to different types >> without any checks at all. >=20 > What cast? >=20 > VMstate takes a void *. >=20 > One an object is cast to a void *, it's just as much as PCIESlot as it > is a PCIEPort. >=20 > That said, for consistency, I think having everything be relatively to > *one* type for a Property list is pretty helpful. >=20 > Expecting someone to know the type hierarchy by heart such that this > doesn't look like a bug is too much IMHO. I'm updating the patch to that effect for VMState. But I notice the real fix for qdev properties would be to move the PCIEPort property to the new PCIEPort type, so that all derived types inherit it. :) For VMState I believe the real follow-up fix would be mst defining a central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice. Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE() in the first place? Regards, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg