From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1Mjw-00074y-KP for qemu-devel@nongnu.org; Mon, 22 Jul 2013 16:29:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1Mju-0003aS-Np for qemu-devel@nongnu.org; Mon, 22 Jul 2013 16:29:52 -0400 Received: from mail-ie0-f180.google.com ([209.85.223.180]:42834) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1Mju-0003aO-Gc for qemu-devel@nongnu.org; Mon, 22 Jul 2013 16:29:50 -0400 Received: by mail-ie0-f180.google.com with SMTP id f4so16371525iea.11 for ; Mon, 22 Jul 2013 13:29:49 -0700 (PDT) From: Anthony Liguori In-Reply-To: <20130721202607.GA15187@redhat.com> References: <1374415744-6675-1-git-send-email-afaerber@suse.de> <1374415744-6675-5-git-send-email-afaerber@suse.de> <20130721202607.GA15187@redhat.com> Date: Mon, 22 Jul 2013 15:29:46 -0500 Message-ID: <87y58y49tx.fsf@codemonkey.ws> MIME-Version: 1.0 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: "Michael S. Tsirkin" , Andreas =?utf-8?Q?F=C3=A4rber?= Cc: qemu-devel@nongnu.org "Michael S. Tsirkin" writes: > 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(-) >>=20 >> 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=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=20 >> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >> @@ -148,9 +147,7 @@ err_bridge: >>=20=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=20 >> pcie_aer_exit(d); >> pcie_chassis_del_slot(s); >> @@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool = multifunction, >> qdev_prop_set_uint16(qdev, "slot", slot); >> qdev_init_nofail(qdev); >>=20=20 >> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br)); >> + return PCIE_SLOT(d); >> } >>=20=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=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. What cast? VMstate takes a void *. One an object is cast to a void *, it's just as much as PCIESlot as it is a PCIEPort. That said, for consistency, I think having everything be relatively to *one* type for a Property list is pretty helpful. Expecting someone to know the type hierarchy by heart such that this doesn't look like a bug is too much IMHO. Regards, Anthony Liguori > > What are the advantages of this hack? > > >> @@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass, v= oid *data) >>=20=20 >> static const TypeInfo ioh3420_info =3D { >> .name =3D "ioh3420", >> - .parent =3D TYPE_PCI_BRIDGE, >> + .parent =3D TYPE_PCIE_SLOT, >> .instance_size =3D sizeof(PCIESlot), >> .class_init =3D ioh3420_class_init, >> }; >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_= downstream.c >> index 9acce3f..549ef26 100644 >> --- a/hw/pci-bridge/xio3130_downstream.c >> +++ b/hw/pci-bridge/xio3130_downstream.c >> @@ -56,9 +56,8 @@ static void xio3130_downstream_reset(DeviceState *qdev) >>=20=20 >> static int xio3130_downstream_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=20 >> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >> @@ -113,9 +112,7 @@ err_bridge: >>=20=20 >> static void xio3130_downstream_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=20 >> pcie_aer_exit(d); >> pcie_chassis_del_slot(s); >> @@ -147,7 +144,7 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int d= evfn, bool multifunction, >> qdev_prop_set_uint16(qdev, "slot", slot); >> qdev_init_nofail(qdev); >>=20=20 >> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br)); >> + return PCIE_SLOT(d); >> } >>=20=20 >> static const VMStateDescription vmstate_xio3130_downstream =3D { >> @@ -157,19 +154,19 @@ static const VMStateDescription vmstate_xio3130_do= wnstream =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=20 >> static Property xio3130_downstream_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(), >> }; > > > Same question. > >> @@ -195,7 +192,7 @@ static void xio3130_downstream_class_init(ObjectClas= s *klass, void *data) >>=20=20 >> static const TypeInfo xio3130_downstream_info =3D { >> .name =3D "xio3130-downstream", >> - .parent =3D TYPE_PCI_BRIDGE, >> + .parent =3D TYPE_PCIE_SLOT, >> .instance_size =3D sizeof(PCIESlot), >> .class_init =3D xio3130_downstream_class_init, >> }; >> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_up= stream.c >> index 6bbcf98..b1ee464 100644 >> --- a/hw/pci-bridge/xio3130_upstream.c >> +++ b/hw/pci-bridge/xio3130_upstream.c >> @@ -53,8 +53,7 @@ static void xio3130_upstream_reset(DeviceState *qdev) >>=20=20 >> static int xio3130_upstream_initfn(PCIDevice *d) >> { >> - PCIBridge *br =3D PCI_BRIDGE(d); >> - PCIEPort *p =3D DO_UPCAST(PCIEPort, br, br); >> + PCIEPort *p =3D PCIE_PORT(d); >> int rc; >>=20=20 >> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >> @@ -125,7 +124,7 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int dev= fn, bool multifunction, >> qdev_prop_set_uint8(qdev, "port", port); >> qdev_init_nofail(qdev); >>=20=20 >> - return DO_UPCAST(PCIEPort, br, br); >> + return PCIE_PORT(d); >> } >>=20=20 >> static const VMStateDescription vmstate_xio3130_upstream =3D { >> @@ -134,8 +133,8 @@ static const VMStateDescription vmstate_xio3130_upst= ream =3D { >> .minimum_version_id =3D 1, >> .minimum_version_id_old =3D 1, >> .fields =3D (VMStateField[]) { >> - VMSTATE_PCIE_DEVICE(br.parent_obj, PCIEPort), >> - VMSTATE_STRUCT(br.parent_obj.exp.aer_log, PCIEPort, 0, >> + VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge), >> + VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0, >> vmstate_pcie_aer_log, PCIEAERLog), >> VMSTATE_END_OF_LIST() >> } >> @@ -143,8 +142,8 @@ static const VMStateDescription vmstate_xio3130_upst= ream =3D { >>=20=20 >> static Property xio3130_upstream_properties[] =3D { >> DEFINE_PROP_UINT8("port", PCIEPort, port, 0), >> - DEFINE_PROP_UINT16("aer_log_max", PCIEPort, >> - 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(), >> }; > > Same question. > > >> @@ -170,7 +169,7 @@ static void xio3130_upstream_class_init(ObjectClass = *klass, void *data) >>=20=20 >> static const TypeInfo xio3130_upstream_info =3D { >> .name =3D "x3130-upstream", >> - .parent =3D TYPE_PCI_BRIDGE, >> + .parent =3D TYPE_PCIE_PORT, >> .instance_size =3D sizeof(PCIEPort), >> .class_init =3D xio3130_upstream_class_init, >> }; >> diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c >> index 91b53a0..a5333a7 100644 >> --- a/hw/pci/pcie_port.c >> +++ b/hw/pci/pcie_port.c >> @@ -116,3 +116,25 @@ void pcie_chassis_del_slot(PCIESlot *s) >> { >> QLIST_REMOVE(s, next); >> } >> + >> +static const TypeInfo pcie_port_type_info =3D { >> + .name =3D TYPE_PCIE_PORT, >> + .parent =3D TYPE_PCI_BRIDGE, >> + .instance_size =3D sizeof(PCIEPort), >> + .abstract =3D true, >> +}; >> + >> +static const TypeInfo pcie_slot_type_info =3D { >> + .name =3D TYPE_PCIE_SLOT, >> + .parent =3D TYPE_PCIE_PORT, >> + .instance_size =3D sizeof(PCIESlot), >> + .abstract =3D true, >> +}; >> + >> +static void pcie_port_register_types(void) >> +{ >> + type_register_static(&pcie_port_type_info); >> + type_register_static(&pcie_slot_type_info); >> +} >> + >> +type_init(pcie_port_register_types) >> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h >> index d89aa61..e167bf7 100644 >> --- a/include/hw/pci/pcie_port.h >> +++ b/include/hw/pci/pcie_port.h >> @@ -24,8 +24,13 @@ >> #include "hw/pci/pci_bridge.h" >> #include "hw/pci/pci_bus.h" >>=20=20 >> +#define TYPE_PCIE_PORT "pcie-port" >> +#define PCIE_PORT(obj) OBJECT_CHECK(PCIEPort, (obj), TYPE_PCIE_PORT) >> + >> struct PCIEPort { >> - PCIBridge br; >> + /*< private >*/ >> + PCIBridge parent_obj; >> + /*< public >*/ >>=20=20 >> /* pci express switch port */ >> uint8_t port; >> @@ -33,8 +38,13 @@ struct PCIEPort { >>=20=20 >> void pcie_port_init_reg(PCIDevice *d); >>=20=20 >> +#define TYPE_PCIE_SLOT "pcie-slot" >> +#define PCIE_SLOT(obj) OBJECT_CHECK(PCIESlot, (obj), TYPE_PCIE_SLOT) >> + >> struct PCIESlot { >> - PCIEPort port; >> + /*< private >*/ >> + PCIEPort parent_obj; >> + /*< public >*/ >>=20=20 >> /* pci express switch port with slot */ >> uint8_t chassis; >> --=20 >> 1.8.1.4