From: Anthony Liguori <anthony@codemonkey.ws>
To: "Michael S. Tsirkin" <mst@redhat.com>,
"Andreas Färber" <afaerber@suse.de>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
Date: Mon, 22 Jul 2013 15:29:46 -0500 [thread overview]
Message-ID: <87y58y49tx.fsf@codemonkey.ws> (raw)
In-Reply-To: <20130721202607.GA15187@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> 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)
>>
>> static int ioh3420_initfn(PCIDevice *d)
>> {
>> - PCIBridge *br = PCI_BRIDGE(d);
>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
>> + PCIEPort *p = PCIE_PORT(d);
>> + PCIESlot *s = PCIE_SLOT(d);
>> int rc;
>>
>> rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
>> @@ -148,9 +147,7 @@ err_bridge:
>>
>> static void ioh3420_exitfn(PCIDevice *d)
>> {
>> - PCIBridge *br = PCI_BRIDGE(d);
>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
>> + PCIESlot *s = PCIE_SLOT(d);
>>
>> 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);
>>
>> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
>> + return PCIE_SLOT(d);
>> }
>>
>> static const VMStateDescription vmstate_ioh3420 = {
>> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
>> .minimum_version_id_old = 1,
>> .post_load = pcie_cap_slot_post_load,
>> .fields = (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()
>> }
>> };
>>
>> static Property ioh3420_properties[] = {
>> - 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, void *data)
>>
>> static const TypeInfo ioh3420_info = {
>> .name = "ioh3420",
>> - .parent = TYPE_PCI_BRIDGE,
>> + .parent = TYPE_PCIE_SLOT,
>> .instance_size = sizeof(PCIESlot),
>> .class_init = 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)
>>
>> static int xio3130_downstream_initfn(PCIDevice *d)
>> {
>> - PCIBridge *br = PCI_BRIDGE(d);
>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
>> + PCIEPort *p = PCIE_PORT(d);
>> + PCIESlot *s = PCIE_SLOT(d);
>> int rc;
>>
>> rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
>> @@ -113,9 +112,7 @@ err_bridge:
>>
>> static void xio3130_downstream_exitfn(PCIDevice *d)
>> {
>> - PCIBridge *br = PCI_BRIDGE(d);
>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
>> + PCIESlot *s = PCIE_SLOT(d);
>>
>> 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);
>>
>> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
>> + return PCIE_SLOT(d);
>> }
>>
>> static const VMStateDescription vmstate_xio3130_downstream = {
>> @@ -157,19 +154,19 @@ static const VMStateDescription vmstate_xio3130_downstream = {
>> .minimum_version_id_old = 1,
>> .post_load = pcie_cap_slot_post_load,
>> .fields = (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()
>> }
>> };
>>
>> static Property xio3130_downstream_properties[] = {
>> - 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(ObjectClass *klass, void *data)
>>
>> static const TypeInfo xio3130_downstream_info = {
>> .name = "xio3130-downstream",
>> - .parent = TYPE_PCI_BRIDGE,
>> + .parent = TYPE_PCIE_SLOT,
>> .instance_size = sizeof(PCIESlot),
>> .class_init = 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)
>>
>> static int xio3130_upstream_initfn(PCIDevice *d)
>> {
>> - PCIBridge *br = PCI_BRIDGE(d);
>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
>> + PCIEPort *p = PCIE_PORT(d);
>> int rc;
>>
>> rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
>> @@ -125,7 +124,7 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int devfn, bool multifunction,
>> qdev_prop_set_uint8(qdev, "port", port);
>> qdev_init_nofail(qdev);
>>
>> - return DO_UPCAST(PCIEPort, br, br);
>> + return PCIE_PORT(d);
>> }
>>
>> static const VMStateDescription vmstate_xio3130_upstream = {
>> @@ -134,8 +133,8 @@ static const VMStateDescription vmstate_xio3130_upstream = {
>> .minimum_version_id = 1,
>> .minimum_version_id_old = 1,
>> .fields = (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_upstream = {
>>
>> static Property xio3130_upstream_properties[] = {
>> 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)
>>
>> static const TypeInfo xio3130_upstream_info = {
>> .name = "x3130-upstream",
>> - .parent = TYPE_PCI_BRIDGE,
>> + .parent = TYPE_PCIE_PORT,
>> .instance_size = sizeof(PCIEPort),
>> .class_init = 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 = {
>> + .name = TYPE_PCIE_PORT,
>> + .parent = TYPE_PCI_BRIDGE,
>> + .instance_size = sizeof(PCIEPort),
>> + .abstract = true,
>> +};
>> +
>> +static const TypeInfo pcie_slot_type_info = {
>> + .name = TYPE_PCIE_SLOT,
>> + .parent = TYPE_PCIE_PORT,
>> + .instance_size = sizeof(PCIESlot),
>> + .abstract = 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"
>>
>> +#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 >*/
>>
>> /* pci express switch port */
>> uint8_t port;
>> @@ -33,8 +38,13 @@ struct PCIEPort {
>>
>> void pcie_port_init_reg(PCIDevice *d);
>>
>> +#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 >*/
>>
>> /* pci express switch port with slot */
>> uint8_t chassis;
>> --
>> 1.8.1.4
next prev parent reply other threads:[~2013-07-22 20:29 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-21 14:09 [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types Andreas Färber
2013-07-21 14:09 ` [Qemu-devel] [PATCH RFC qom-next 1/4] pci-bridge: Turn into abstract QOM type Andreas Färber
2013-07-21 14:09 ` [Qemu-devel] [PATCH RFC qom-next 2/4] pci-bridge-dev: QOM parent field cleanup Andreas Färber
2013-07-21 14:09 ` [Qemu-devel] [PATCH RFC qom-next 3/4] pci-bridge/i82801b11: Rename parent field Andreas Färber
2013-07-21 14:09 ` [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types Andreas Färber
2013-07-21 20:26 ` Michael S. Tsirkin
2013-07-22 17:42 ` Andreas Färber
2013-07-22 19:34 ` Michael S. Tsirkin
2013-07-22 20:29 ` Anthony Liguori [this message]
2013-07-22 21:04 ` Andreas Färber
2013-07-23 7:07 ` Michael S. Tsirkin
2013-07-23 9:10 ` Andreas Färber
2013-07-23 9:59 ` Michael S. Tsirkin
2013-07-23 10:21 ` Andreas Färber
2013-07-23 11:21 ` Gerd Hoffmann
2013-07-23 12:35 ` Michael S. Tsirkin
2013-07-23 7:04 ` Michael S. Tsirkin
2013-07-28 12:36 ` Andreas Färber
2013-07-28 12:58 ` Michael S. Tsirkin
2013-07-21 20:26 ` [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types Michael S. Tsirkin
2013-07-22 17:22 ` Andreas Färber
2013-07-22 22:05 ` Andreas Färber
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y58y49tx.fsf@codemonkey.ws \
--to=anthony@codemonkey.ws \
--cc=afaerber@suse.de \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.