From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54930) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1K8K-0006x5-Hu for qemu-devel@nongnu.org; Mon, 22 Jul 2013 13:42:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1K8G-00072l-Ix for qemu-devel@nongnu.org; Mon, 22 Jul 2013 13:42:52 -0400 Received: from cantor2.suse.de ([195.135.220.15]:36038 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1K8G-00072g-5O for qemu-devel@nongnu.org; Mon, 22 Jul 2013 13:42:48 -0400 Message-ID: <51ED6F13.6090803@suse.de> Date: Mon, 22 Jul 2013 19:42:43 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= 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> In-Reply-To: <20130721202607.GA15187@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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" Cc: Paolo Bonzini , Peter Crosthwaite , qemu-devel@nongnu.org, Anthony Liguori , Juan Quintela Am 21.07.2013 22:26, schrieb Michael S. Tsirkin: > On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas F=E4rber wrote: >> Signed-off-by: Andreas F=E4rber >> --- >> 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, boo= l 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(), >> }; >=20 >=20 > This looks scary. This does a cast to different types > without any checks at all. It doesn't. AFAIU both VMStateDescription and qdev properties store a numerical offset from the object pointer. CC'ing Juan and Paolo/Anthony. I.e., if I don't find a counter-example we can drop the arguments from VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() completely since the offset would always be 0 and the field name can be hardcoded, cf. VMSTATE_CPU() in qom/cpu.h. > What are the advantages of this hack? The concrete problems I encountered in this series were a) 80 chars limit and b) parent_obj.parent_obj.foo. Since offsetof(MyState, parent_obj.foo) =3D=3D offsetof(ParentState, foo)= , this was a neat way to shorten the line. The main point of the series is introducing abstract QOM types matching a specific struct. Renaming the parent field helps catch users that should no longer be accessing it, both in compile-testing and in future patch review. What we would do about VMState when someone at some future point wants to move PCI state into a QOM interface I have no clue, I'll let the person who throws the first stone worry about that. ;) Regards, Andreas >> @@ -228,7 +225,7 @@ static void ioh3420_class_init(ObjectClass *klass,= void *data) >> =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/xio313= 0_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 *qd= ev) >> =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 >> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >> @@ -113,9 +112,7 @@ err_bridge: >> =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 >> pcie_aer_exit(d); >> pcie_chassis_del_slot(s); >> @@ -147,7 +144,7 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int= devfn, bool 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_xio3130_downstream =3D { >> @@ -157,19 +154,19 @@ static const VMStateDescription vmstate_xio3130_= downstream =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 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(), >> }; >=20 >=20 > Same question. >=20 >> @@ -195,7 +192,7 @@ static void xio3130_downstream_class_init(ObjectCl= ass *klass, void *data) >> =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_= upstream.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 >> 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 >> rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS); >> @@ -125,7 +124,7 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int d= evfn, bool multifunction, >> qdev_prop_set_uint8(qdev, "port", port); >> qdev_init_nofail(qdev); >> =20 >> - return DO_UPCAST(PCIEPort, br, br); >> + return PCIE_PORT(d); >> } >> =20 >> static const VMStateDescription vmstate_xio3130_upstream =3D { >> @@ -134,8 +133,8 @@ static const VMStateDescription vmstate_xio3130_up= stream =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_up= stream =3D { >> =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(), >> }; >=20 > Same question. >=20 >=20 >> @@ -170,7 +169,7 @@ static void xio3130_upstream_class_init(ObjectClas= s *klass, void *data) >> =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 >> +#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 >> /* pci express switch port */ >> uint8_t port; >> @@ -33,8 +38,13 @@ struct PCIEPort { >> =20 >> void pcie_port_init_reg(PCIDevice *d); >> =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 >> /* pci express switch port with slot */ >> uint8_t chassis; >> --=20 >> 1.8.1.4 >=20 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg