* [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types
@ 2013-07-21 14:09 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
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Andreas Färber @ 2013-07-21 14:09 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Crosthwaite, Michael S. Tsirkin, Hu Tao, Anthony Liguori,
Paolo Bonzini, Andreas Färber
Hello Michael et al.,
This series turns PCIBridge, PCIEPort and PCIESlot into abstract QOM types,
so that we can use QOM casts to obtain a pointer.
Possibly this was prompted by q35's PCIe? Don't remember ATM...
Regards,
Andreas
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Andreas Färber (4):
pci-bridge: Turn into abstract QOM type
pci-bridge-dev: QOM parent field cleanup
pci-bridge/i82801b11: Rename parent field
pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types
hw/pci-bridge/dec.c | 4 ++--
hw/pci-bridge/i82801b11.c | 10 ++++++----
hw/pci-bridge/ioh3420.c | 29 +++++++++++++--------------
hw/pci-bridge/pci_bridge_dev.c | 26 +++++++++++++++----------
hw/pci-bridge/xio3130_downstream.c | 29 +++++++++++++--------------
hw/pci-bridge/xio3130_upstream.c | 20 +++++++++----------
hw/pci-host/apb.c | 4 ++--
hw/pci/pci_bridge.c | 40 +++++++++++++++++++++++++++-----------
hw/pci/pcie.c | 2 +-
hw/pci/pcie_port.c | 22 +++++++++++++++++++++
include/hw/pci/pci_bus.h | 7 ++++++-
include/hw/pci/pcie_port.h | 14 +++++++++++--
12 files changed, 132 insertions(+), 75 deletions(-)
--
1.8.1.4
^ permalink raw reply [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH RFC qom-next 1/4] pci-bridge: Turn into abstract QOM type 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 ` 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 ` (3 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Andreas Färber @ 2013-07-21 14:09 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, open list:New World, Andreas Färber, Alexander Graf Introduce TYPE_PCI_BRIDGE as base type and use PCI_BRIDGE() casts. Signed-off-by: Andreas Färber <afaerber@suse.de> --- hw/pci-bridge/dec.c | 4 ++-- hw/pci-bridge/i82801b11.c | 6 +++--- hw/pci-bridge/ioh3420.c | 18 ++++++++--------- hw/pci-bridge/pci_bridge_dev.c | 10 +++++----- hw/pci-bridge/xio3130_downstream.c | 18 ++++++++--------- hw/pci-bridge/xio3130_upstream.c | 17 ++++++++-------- hw/pci-host/apb.c | 4 ++-- hw/pci/pci_bridge.c | 40 +++++++++++++++++++++++++++----------- hw/pci/pcie.c | 2 +- include/hw/pci/pci_bus.h | 7 ++++++- 10 files changed, 75 insertions(+), 51 deletions(-) diff --git a/hw/pci-bridge/dec.c b/hw/pci-bridge/dec.c index efc07c4..e5e3be8 100644 --- a/hw/pci-bridge/dec.c +++ b/hw/pci-bridge/dec.c @@ -74,7 +74,7 @@ static void dec_21154_pci_bridge_class_init(ObjectClass *klass, void *data) static const TypeInfo dec_21154_pci_bridge_info = { .name = "dec-21154-p2p-bridge", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(PCIBridge), .class_init = dec_21154_pci_bridge_class_init, }; @@ -86,7 +86,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn) dev = pci_create_multifunction(parent_bus, devfn, false, "dec-21154-p2p-bridge"); - br = DO_UPCAST(PCIBridge, dev, dev); + br = PCI_BRIDGE(dev); pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", dec_map_irq); qdev_init_nofail(&dev->qdev); return pci_bridge_get_sec_bus(br); diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c index b98bfb0..88f489a 100644 --- a/hw/pci-bridge/i82801b11.c +++ b/hw/pci-bridge/i82801b11.c @@ -91,7 +91,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, void *data) static const TypeInfo i82801b11_bridge_info = { .name = "i82801b11-bridge", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(I82801b11Bridge), .class_init = i82801b11_bridge_class_init, }; @@ -107,8 +107,8 @@ PCIBus *ich9_d2pbr_init(PCIBus *bus, int devfn, int sec_bus) if (!d) { return NULL; } - br = DO_UPCAST(PCIBridge, dev, d); - qdev = &br->dev.qdev; + br = PCI_BRIDGE(d); + qdev = DEVICE(d); snprintf(buf, sizeof(buf), "pci.%d", sec_bus); pci_bridge_map_irq(br, buf, pci_swizzle_map_irq_fn); diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c index bb541eb..728f658 100644 --- a/hw/pci-bridge/ioh3420.c +++ b/hw/pci-bridge/ioh3420.c @@ -92,7 +92,7 @@ static void ioh3420_reset(DeviceState *qdev) static int ioh3420_initfn(PCIDevice *d) { - PCIBridge* br = DO_UPCAST(PCIBridge, dev, d); + PCIBridge *br = PCI_BRIDGE(d); PCIEPort *p = DO_UPCAST(PCIEPort, br, br); PCIESlot *s = DO_UPCAST(PCIESlot, port, p); int rc; @@ -148,7 +148,7 @@ err_bridge: static void ioh3420_exitfn(PCIDevice *d) { - PCIBridge* br = DO_UPCAST(PCIBridge, dev, d); + PCIBridge *br = PCI_BRIDGE(d); PCIEPort *p = DO_UPCAST(PCIEPort, br, br); PCIESlot *s = DO_UPCAST(PCIESlot, port, p); @@ -171,9 +171,9 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction, if (!d) { return NULL; } - br = DO_UPCAST(PCIBridge, dev, d); + br = PCI_BRIDGE(d); - qdev = &br->dev.qdev; + qdev = DEVICE(d); pci_bridge_map_irq(br, bus_name, map_irq); qdev_prop_set_uint8(qdev, "port", port); qdev_prop_set_uint8(qdev, "chassis", chassis); @@ -190,8 +190,8 @@ static const VMStateDescription vmstate_ioh3420 = { .minimum_version_id_old = 1, .post_load = pcie_cap_slot_post_load, .fields = (VMStateField[]) { - VMSTATE_PCIE_DEVICE(port.br.dev, PCIESlot), - VMSTATE_STRUCT(port.br.dev.exp.aer_log, PCIESlot, 0, + VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot), + VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog), VMSTATE_END_OF_LIST() } @@ -202,8 +202,8 @@ static Property ioh3420_properties[] = { DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), DEFINE_PROP_UINT16("aer_log_max", PCIESlot, - port.br.dev.exp.aer_log.log_max, - PCIE_AER_LOG_MAX_DEFAULT), + port.br.parent_obj.exp.aer_log.log_max, + PCIE_AER_LOG_MAX_DEFAULT), DEFINE_PROP_END_OF_LIST(), }; @@ -228,7 +228,7 @@ static void ioh3420_class_init(ObjectClass *klass, void *data) static const TypeInfo ioh3420_info = { .name = "ioh3420", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(PCIESlot), .class_init = ioh3420_class_init, }; diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 5f11323..00d2382 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -38,7 +38,7 @@ typedef struct PCIBridgeDev PCIBridgeDev; static int pci_bridge_dev_initfn(PCIDevice *dev) { - PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev); + PCIBridge *br = PCI_BRIDGE(dev); PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br); int err; @@ -81,7 +81,7 @@ bridge_error: static void pci_bridge_dev_exitfn(PCIDevice *dev) { - PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev); + PCIBridge *br = PCI_BRIDGE(dev); PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br); if (msi_present(dev)) { msi_uninit(dev); @@ -120,8 +120,8 @@ static Property pci_bridge_dev_properties[] = { static const VMStateDescription pci_bridge_dev_vmstate = { .name = "pci_bridge", .fields = (VMStateField[]) { - VMSTATE_PCI_DEVICE(bridge.dev, PCIBridgeDev), - SHPC_VMSTATE(bridge.dev.shpc, PCIBridgeDev), + VMSTATE_PCI_DEVICE(bridge.parent_obj, PCIBridgeDev), + SHPC_VMSTATE(bridge.parent_obj.shpc, PCIBridgeDev), VMSTATE_END_OF_LIST() } }; @@ -145,7 +145,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) static const TypeInfo pci_bridge_dev_info = { .name = "pci-bridge", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(PCIBridgeDev), .class_init = pci_bridge_dev_class_init, }; diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c index 1810dd2..9acce3f 100644 --- a/hw/pci-bridge/xio3130_downstream.c +++ b/hw/pci-bridge/xio3130_downstream.c @@ -56,7 +56,7 @@ static void xio3130_downstream_reset(DeviceState *qdev) static int xio3130_downstream_initfn(PCIDevice *d) { - PCIBridge* br = DO_UPCAST(PCIBridge, dev, d); + PCIBridge *br = PCI_BRIDGE(d); PCIEPort *p = DO_UPCAST(PCIEPort, br, br); PCIESlot *s = DO_UPCAST(PCIESlot, port, p); int rc; @@ -113,7 +113,7 @@ err_bridge: static void xio3130_downstream_exitfn(PCIDevice *d) { - PCIBridge* br = DO_UPCAST(PCIBridge, dev, d); + PCIBridge *br = PCI_BRIDGE(d); PCIEPort *p = DO_UPCAST(PCIEPort, br, br); PCIESlot *s = DO_UPCAST(PCIESlot, port, p); @@ -138,9 +138,9 @@ PCIESlot *xio3130_downstream_init(PCIBus *bus, int devfn, bool multifunction, if (!d) { return NULL; } - br = DO_UPCAST(PCIBridge, dev, d); + br = PCI_BRIDGE(d); - qdev = &br->dev.qdev; + qdev = DEVICE(d); pci_bridge_map_irq(br, bus_name, map_irq); qdev_prop_set_uint8(qdev, "port", port); qdev_prop_set_uint8(qdev, "chassis", chassis); @@ -157,8 +157,8 @@ 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.dev, PCIESlot), - VMSTATE_STRUCT(port.br.dev.exp.aer_log, PCIESlot, 0, + VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot), + VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog), VMSTATE_END_OF_LIST() } @@ -169,8 +169,8 @@ static Property xio3130_downstream_properties[] = { DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0), DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0), DEFINE_PROP_UINT16("aer_log_max", PCIESlot, - port.br.dev.exp.aer_log.log_max, - PCIE_AER_LOG_MAX_DEFAULT), + port.br.parent_obj.exp.aer_log.log_max, + PCIE_AER_LOG_MAX_DEFAULT), DEFINE_PROP_END_OF_LIST(), }; @@ -195,7 +195,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data) static const TypeInfo xio3130_downstream_info = { .name = "xio3130-downstream", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .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 8e0d97a..6bbcf98 100644 --- a/hw/pci-bridge/xio3130_upstream.c +++ b/hw/pci-bridge/xio3130_upstream.c @@ -53,7 +53,7 @@ static void xio3130_upstream_reset(DeviceState *qdev) static int xio3130_upstream_initfn(PCIDevice *d) { - PCIBridge* br = DO_UPCAST(PCIBridge, dev, d); + PCIBridge *br = PCI_BRIDGE(d); PCIEPort *p = DO_UPCAST(PCIEPort, br, br); int rc; @@ -118,9 +118,9 @@ PCIEPort *xio3130_upstream_init(PCIBus *bus, int devfn, bool multifunction, if (!d) { return NULL; } - br = DO_UPCAST(PCIBridge, dev, d); + br = PCI_BRIDGE(d); - qdev = &br->dev.qdev; + qdev = DEVICE(d); pci_bridge_map_irq(br, bus_name, map_irq); qdev_prop_set_uint8(qdev, "port", port); qdev_init_nofail(qdev); @@ -134,16 +134,17 @@ static const VMStateDescription vmstate_xio3130_upstream = { .minimum_version_id = 1, .minimum_version_id_old = 1, .fields = (VMStateField[]) { - VMSTATE_PCIE_DEVICE(br.dev, PCIEPort), - VMSTATE_STRUCT(br.dev.exp.aer_log, PCIEPort, 0, vmstate_pcie_aer_log, - PCIEAERLog), + VMSTATE_PCIE_DEVICE(br.parent_obj, PCIEPort), + VMSTATE_STRUCT(br.parent_obj.exp.aer_log, PCIEPort, 0, + vmstate_pcie_aer_log, PCIEAERLog), VMSTATE_END_OF_LIST() } }; static Property xio3130_upstream_properties[] = { DEFINE_PROP_UINT8("port", PCIEPort, port, 0), - DEFINE_PROP_UINT16("aer_log_max", PCIEPort, br.dev.exp.aer_log.log_max, + DEFINE_PROP_UINT16("aer_log_max", PCIEPort, + br.parent_obj.exp.aer_log.log_max, PCIE_AER_LOG_MAX_DEFAULT), DEFINE_PROP_END_OF_LIST(), }; @@ -169,7 +170,7 @@ static void xio3130_upstream_class_init(ObjectClass *klass, void *data) static const TypeInfo xio3130_upstream_info = { .name = "x3130-upstream", - .parent = TYPE_PCI_DEVICE, + .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(PCIEPort), .class_init = xio3130_upstream_class_init, }; diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c index 06ace08..0277aa0 100644 --- a/hw/pci-host/apb.c +++ b/hw/pci-host/apb.c @@ -464,7 +464,7 @@ PCIBus *pci_apb_init(hwaddr special_base, /* APB secondary busses */ pci_dev = pci_create_multifunction(d->bus, PCI_DEVFN(1, 0), true, "pbm-bridge"); - br = DO_UPCAST(PCIBridge, dev, pci_dev); + br = PCI_BRIDGE(pci_dev); pci_bridge_map_irq(br, "Advanced PCI Bus secondary bridge 1", pci_apb_map_irq); qdev_init_nofail(&pci_dev->qdev); @@ -472,7 +472,7 @@ PCIBus *pci_apb_init(hwaddr special_base, pci_dev = pci_create_multifunction(d->bus, PCI_DEVFN(1, 1), true, "pbm-bridge"); - br = DO_UPCAST(PCIBridge, dev, pci_dev); + br = PCI_BRIDGE(pci_dev); pci_bridge_map_irq(br, "Advanced PCI Bus secondary bridge 2", pci_apb_map_irq); qdev_init_nofail(&pci_dev->qdev); diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c index 02a396b..a90671d 100644 --- a/hw/pci/pci_bridge.c +++ b/hw/pci/pci_bridge.c @@ -141,8 +141,9 @@ static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias, MemoryRegion *parent_space, bool enabled) { - pcibus_t base = pci_bridge_get_base(&bridge->dev, type); - pcibus_t limit = pci_bridge_get_limit(&bridge->dev, type); + PCIDevice *bridge_dev = PCI_DEVICE(bridge); + pcibus_t base = pci_bridge_get_base(bridge_dev, type); + pcibus_t limit = pci_bridge_get_limit(bridge_dev, type); /* TODO: this doesn't handle base = 0 limit = 2^64 - 1 correctly. * Apparently no way to do this with existing memory APIs. */ pcibus_t size = enabled && limit >= base ? limit + 1 - base : 0; @@ -154,7 +155,8 @@ static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias, static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent, MemoryRegion *alias_vga) { - uint16_t brctl = pci_get_word(br->dev.config + PCI_BRIDGE_CONTROL); + PCIDevice *pd = PCI_DEVICE(br); + uint16_t brctl = pci_get_word(pd->config + PCI_BRIDGE_CONTROL); memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO], OBJECT(br), "pci_bridge_vga_io_lo", &br->address_space_io, @@ -167,7 +169,7 @@ static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent, QEMU_PCI_VGA_MEM_BASE, QEMU_PCI_VGA_MEM_SIZE); if (brctl & PCI_BRIDGE_CTL_VGA) { - pci_register_vga(&br->dev, &alias_vga[QEMU_PCI_VGA_MEM], + pci_register_vga(pd, &alias_vga[QEMU_PCI_VGA_MEM], &alias_vga[QEMU_PCI_VGA_IO_LO], &alias_vga[QEMU_PCI_VGA_IO_HI]); } @@ -175,9 +177,10 @@ static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent, static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br) { - PCIBus *parent = br->dev.bus; + PCIDevice *pd = PCI_DEVICE(br); + PCIBus *parent = pd->bus; PCIBridgeWindows *w = g_new(PCIBridgeWindows, 1); - uint16_t cmd = pci_get_word(br->dev.config + PCI_COMMAND); + uint16_t cmd = pci_get_word(pd->config + PCI_COMMAND); pci_bridge_init_alias(br, &w->alias_pref_mem, PCI_BASE_ADDRESS_MEM_PREFETCH, @@ -205,12 +208,13 @@ static PCIBridgeWindows *pci_bridge_region_init(PCIBridge *br) static void pci_bridge_region_del(PCIBridge *br, PCIBridgeWindows *w) { - PCIBus *parent = br->dev.bus; + PCIDevice *pd = PCI_DEVICE(br); + PCIBus *parent = pd->bus; memory_region_del_subregion(parent->address_space_io, &w->alias_io); memory_region_del_subregion(parent->address_space_mem, &w->alias_mem); memory_region_del_subregion(parent->address_space_mem, &w->alias_pref_mem); - pci_unregister_vga(&br->dev); + pci_unregister_vga(pd); } static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w) @@ -241,7 +245,7 @@ void pci_bridge_update_mappings(PCIBridge *br) void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { - PCIBridge *s = container_of(d, PCIBridge, dev); + PCIBridge *s = PCI_BRIDGE(d); uint16_t oldctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); uint16_t newctl; @@ -331,7 +335,7 @@ void pci_bridge_reset(DeviceState *qdev) int pci_bridge_initfn(PCIDevice *dev, const char *typename) { PCIBus *parent = dev->bus; - PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev); + PCIBridge *br = PCI_BRIDGE(dev); PCIBus *sec_bus = &br->sec_bus; pci_word_test_and_set_mask(dev->config + PCI_STATUS, @@ -379,7 +383,7 @@ int pci_bridge_initfn(PCIDevice *dev, const char *typename) /* default qdev clean up function for PCI-to-PCI bridge */ void pci_bridge_exitfn(PCIDevice *pci_dev) { - PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); + PCIBridge *s = PCI_BRIDGE(pci_dev); assert(QLIST_EMPTY(&s->sec_bus.child)); QLIST_REMOVE(&s->sec_bus, sibling); pci_bridge_region_del(s, s->windows); @@ -400,3 +404,17 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, br->map_irq = map_irq; br->bus_name = bus_name; } + +static const TypeInfo pci_bridge_type_info = { + .name = TYPE_PCI_BRIDGE, + .parent = TYPE_PCI_DEVICE, + .instance_size = sizeof(PCIBridge), + .abstract = true, +}; + +static void pci_bridge_register_types(void) +{ + type_register_static(&pci_bridge_type_info); +} + +type_init(pci_bridge_register_types) diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 62bd0b8..50af3c1 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -305,7 +305,7 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot) dev->exp.hpev_notified = false; - pci_bus_hotplug(pci_bridge_get_sec_bus(DO_UPCAST(PCIBridge, dev, dev)), + pci_bus_hotplug(pci_bridge_get_sec_bus(PCI_BRIDGE(dev)), pcie_cap_slot_hotplug, &dev->qdev); } diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h index 66762f6..9df1788 100644 --- a/include/hw/pci/pci_bus.h +++ b/include/hw/pci/pci_bus.h @@ -53,8 +53,13 @@ struct PCIBridgeWindows { MemoryRegion alias_vga[QEMU_PCI_VGA_NUM_REGIONS]; }; +#define TYPE_PCI_BRIDGE "base-pci-bridge" +#define PCI_BRIDGE(obj) OBJECT_CHECK(PCIBridge, (obj), TYPE_PCI_BRIDGE) + struct PCIBridge { - PCIDevice dev; + /*< private >*/ + PCIDevice parent_obj; + /*< public >*/ /* private member */ PCIBus sec_bus; -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH RFC qom-next 2/4] pci-bridge-dev: QOM parent field cleanup 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 ` 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 ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Andreas Färber @ 2013-07-21 14:09 UTC (permalink / raw) To: qemu-devel; +Cc: Peter Crosthwaite, Andreas Färber Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Signed-off-by: Andreas Färber <afaerber@suse.de> --- hw/pci-bridge/pci_bridge_dev.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c index 00d2382..cf3c53c 100644 --- a/hw/pci-bridge/pci_bridge_dev.c +++ b/hw/pci-bridge/pci_bridge_dev.c @@ -27,8 +27,15 @@ #include "exec/memory.h" #include "hw/pci/pci_bus.h" +#define TYPE_PCI_BRIDGE_DEV "pci-bridge" +#define PCI_BRIDGE_DEV(obj) \ + OBJECT_CHECK(PCIBridgeDev, (obj), TYPE_PCI_BRIDGE_DEV) + struct PCIBridgeDev { - PCIBridge bridge; + /*< private >*/ + PCIBridge parent_obj; + /*< public >*/ + MemoryRegion bar; uint8_t chassis_nr; #define PCI_BRIDGE_DEV_F_MSI_REQ 0 @@ -39,7 +46,7 @@ typedef struct PCIBridgeDev PCIBridgeDev; static int pci_bridge_dev_initfn(PCIDevice *dev) { PCIBridge *br = PCI_BRIDGE(dev); - PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br); + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); int err; err = pci_bridge_initfn(dev, TYPE_PCI_BUS); @@ -81,8 +88,7 @@ bridge_error: static void pci_bridge_dev_exitfn(PCIDevice *dev) { - PCIBridge *br = PCI_BRIDGE(dev); - PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br); + PCIBridgeDev *bridge_dev = PCI_BRIDGE_DEV(dev); if (msi_present(dev)) { msi_uninit(dev); } @@ -104,7 +110,7 @@ static void pci_bridge_dev_write_config(PCIDevice *d, static void qdev_pci_bridge_dev_reset(DeviceState *qdev) { - PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev); + PCIDevice *dev = PCI_DEVICE(qdev); pci_bridge_reset(qdev); shpc_reset(dev); @@ -120,8 +126,8 @@ static Property pci_bridge_dev_properties[] = { static const VMStateDescription pci_bridge_dev_vmstate = { .name = "pci_bridge", .fields = (VMStateField[]) { - VMSTATE_PCI_DEVICE(bridge.parent_obj, PCIBridgeDev), - SHPC_VMSTATE(bridge.parent_obj.shpc, PCIBridgeDev), + VMSTATE_PCI_DEVICE(parent_obj, PCIBridge), + SHPC_VMSTATE(shpc, PCIDevice), VMSTATE_END_OF_LIST() } }; @@ -144,7 +150,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data) } static const TypeInfo pci_bridge_dev_info = { - .name = "pci-bridge", + .name = TYPE_PCI_BRIDGE_DEV, .parent = TYPE_PCI_BRIDGE, .instance_size = sizeof(PCIBridgeDev), .class_init = pci_bridge_dev_class_init, -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH RFC qom-next 3/4] pci-bridge/i82801b11: Rename parent field 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 ` 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 ` [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types Michael S. Tsirkin 4 siblings, 0 replies; 22+ messages in thread From: Andreas Färber @ 2013-07-21 14:09 UTC (permalink / raw) To: qemu-devel; +Cc: Andreas Färber Signed-off-by: Andreas Färber <afaerber@suse.de> --- hw/pci-bridge/i82801b11.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c index 88f489a..90fc91d 100644 --- a/hw/pci-bridge/i82801b11.c +++ b/hw/pci-bridge/i82801b11.c @@ -52,7 +52,9 @@ #define I82801ba_SSVID_SSID 0 typedef struct I82801b11Bridge { - PCIBridge br; + /*< private >*/ + PCIBridge parent_obj; + /*< public >*/ } I82801b11Bridge; static int i82801b11_bridge_initfn(PCIDevice *d) -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-21 14:09 [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types Andreas Färber ` (2 preceding siblings ...) 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 ` Andreas Färber 2013-07-21 20:26 ` 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 4 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-21 14:09 UTC (permalink / raw) To: qemu-devel; +Cc: Andreas Färber, Michael S. Tsirkin 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(), }; @@ -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(), }; @@ -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(), }; @@ -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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 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 20:29 ` Anthony Liguori 0 siblings, 2 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-21 20:26 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-devel 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 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 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 1 sibling, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-22 17:42 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Peter Crosthwaite, qemu-devel, 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ä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. 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) == 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) >> >> 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 > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-22 17:42 ` Andreas Färber @ 2013-07-22 19:34 ` Michael S. Tsirkin 0 siblings, 0 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-22 19:34 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, Peter Crosthwaite, qemu-devel, Anthony Liguori, Juan Quintela On Mon, Jul 22, 2013 at 07:42:43PM +0200, Andreas Färber wrote: > Am 21.07.2013 22:26, schrieb Michael S. Tsirkin: > > 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. > > 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. Currently VMSTATE macros are safe since they all use the same structure. You are making them use different types with no compile or runtime checking that the correct type is used. > > > 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) == offsetof(ParentState, foo), > this was a neat way to shorten the line. I don't think it's a good reason. > 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 NACK Please find a way to have code that won't silently break when we refactor code. Normal QOM type use has runtime checks to address that. > >> @@ -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 > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-21 20:26 ` Michael S. Tsirkin 2013-07-22 17:42 ` Andreas Färber @ 2013-07-22 20:29 ` Anthony Liguori 2013-07-22 21:04 ` Andreas Färber ` (2 more replies) 1 sibling, 3 replies; 22+ messages in thread From: Anthony Liguori @ 2013-07-22 20:29 UTC (permalink / raw) To: Michael S. Tsirkin, Andreas Färber; +Cc: qemu-devel "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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-22 20:29 ` Anthony Liguori @ 2013-07-22 21:04 ` Andreas Färber 2013-07-23 7:07 ` Michael S. Tsirkin 2013-07-23 7:04 ` Michael S. Tsirkin 2013-07-28 12:36 ` Andreas Färber 2 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-22 21:04 UTC (permalink / raw) To: Anthony Liguori, Michael S. Tsirkin; +Cc: qemu-devel Am 22.07.2013 22:29, schrieb Anthony Liguori: > "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. 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-22 21:04 ` Andreas Färber @ 2013-07-23 7:07 ` Michael S. Tsirkin 2013-07-23 9:10 ` Andreas Färber 0 siblings, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-23 7:07 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-devel, Anthony Liguori On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: > Am 22.07.2013 22:29, schrieb Anthony Liguori: > > "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. > > 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 The real fix is savevm/loadvm taking into account the class hierarchy. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-23 7:07 ` Michael S. Tsirkin @ 2013-07-23 9:10 ` Andreas Färber 2013-07-23 9:59 ` Michael S. Tsirkin 0 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-23 9:10 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: qemu-devel, Anthony Liguori, Juan Quintela Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: > On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: >> 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? > > The real fix is savevm/loadvm taking into account > the class hierarchy. That's not helping, unless you write a patch to show what you mean and how that is going to be migration-compatible. Does your not answering the question mean you don't know? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-23 9:10 ` Andreas Färber @ 2013-07-23 9:59 ` Michael S. Tsirkin 2013-07-23 10:21 ` Andreas Färber 0 siblings, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-23 9:59 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-devel, Anthony Liguori, Juan Quintela On Tue, Jul 23, 2013 at 11:10:45AM +0200, Andreas Färber wrote: > Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: > > On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: > >> 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? > > > > The real fix is savevm/loadvm taking into account > > the class hierarchy. > > That's not helping, unless you write a patch to show what you mean and I merely mean that if I inherit a class I should inherit it's vmstate. So explicitly adding VMSTATE_PCI_DEVICE should not be necessary. > how that is going to be migration-compatible. Most devices put VMSTATE_PCI_DEVICE at the beninning, so just calling that before vmstate for the device should be compatible. > Does your not answering the question mean you don't know? > > Andreas I think the answer is that most pcie devices don't implement AER. AFAIK PCI devices can't support AER at all. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-23 9:59 ` Michael S. Tsirkin @ 2013-07-23 10:21 ` Andreas Färber 2013-07-23 11:21 ` Gerd Hoffmann 0 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-23 10:21 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Gerd Hoffmann, qemu-devel, Anthony Liguori, Juan Quintela Am 23.07.2013 11:59, schrieb Michael S. Tsirkin: > On Tue, Jul 23, 2013 at 11:10:45AM +0200, Andreas Färber wrote: >> Am 23.07.2013 09:07, schrieb Michael S. Tsirkin: >>> On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote: >>>> 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? > > I think the answer is that most pcie devices > don't implement AER. AFAIK PCI devices can't > support AER at all. Okay, so if it's just PCIe, then XHCI is the oddball preventing moving it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, also operating on PCIDevice. Is there a way to detect use of AER or MSIX to place those into subsections of VMSTATE_PCIE_DEVICE()? Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-23 10:21 ` Andreas Färber @ 2013-07-23 11:21 ` Gerd Hoffmann 2013-07-23 12:35 ` Michael S. Tsirkin 0 siblings, 1 reply; 22+ messages in thread From: Gerd Hoffmann @ 2013-07-23 11:21 UTC (permalink / raw) To: Andreas Färber Cc: Juan Quintela, qemu-devel, Anthony Liguori, Michael S. Tsirkin Hi, > Okay, so if it's just PCIe, then XHCI is the oddball preventing moving > it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, > also operating on PCIDevice. Given that live migration support for xhci was added post-1.5 so we don't have a release with it yet this shouldn't be a blocker in case we get this sorted in time for 1.6. > Is there a way to detect use of AER or MSIX to place those into > subsections of VMSTATE_PCIE_DEVICE()? There is msix_enabled() ... Dunno about AER. cheers, Gerd ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-23 11:21 ` Gerd Hoffmann @ 2013-07-23 12:35 ` Michael S. Tsirkin 0 siblings, 0 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-23 12:35 UTC (permalink / raw) To: Gerd Hoffmann Cc: Juan Quintela, Andreas Färber, Anthony Liguori, qemu-devel On Tue, Jul 23, 2013 at 01:21:13PM +0200, Gerd Hoffmann wrote: > Hi, > > > Okay, so if it's just PCIe, then XHCI is the oddball preventing moving > > it into VMSTATE_PCIE_DEVICE(). XHCI has VMSTATE_MSIX() in its place, > > also operating on PCIDevice. > > Given that live migration support for xhci was added post-1.5 so we > don't have a release with it yet this shouldn't be a blocker in case we > get this sorted in time for 1.6. > > > Is there a way to detect use of AER or MSIX to place those into > > subsections of VMSTATE_PCIE_DEVICE()? > > There is msix_enabled() ... msix_present() > Dunno about AER. > > cheers, > Gerd Not at the moment but it's not hard to add. -- MST ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-22 20:29 ` Anthony Liguori 2013-07-22 21:04 ` Andreas Färber @ 2013-07-23 7:04 ` Michael S. Tsirkin 2013-07-28 12:36 ` Andreas Färber 2 siblings, 0 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-23 7:04 UTC (permalink / raw) To: Anthony Liguori; +Cc: Andreas Färber, qemu-devel On Mon, Jul 22, 2013 at 03:29:46PM -0500, Anthony Liguori wrote: > "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 Exactly. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-22 20:29 ` Anthony Liguori 2013-07-22 21:04 ` Andreas Färber 2013-07-23 7:04 ` Michael S. Tsirkin @ 2013-07-28 12:36 ` Andreas Färber 2013-07-28 12:58 ` Michael S. Tsirkin 2 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-28 12:36 UTC (permalink / raw) To: Anthony Liguori Cc: Paolo Bonzini, Juan Quintela, qemu-devel, Michael S. Tsirkin Hi Anthony, Am 22.07.2013 22:29, schrieb Anthony Liguori: > 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. I have changed v2 not to mix different-but-compatible struct types in one VMStateDescription. Could you clarify if that was what you meant with the above? Or would you also be opposed to - post-1.6 - changing VMSTATE_PCIE_DEVICE(parent_obj[.parent_obj], MyStruct) to VMSTATE_PCIE_DEVICE() as suggested elsewhere in this thread? I'm thinking that writing VMSTATE_PCIE_DEVICE() already clearly indicates the developer knows the device inherits from TYPE_PCI_DEVICE. All PCIe devices using VMSTATE_PCIE_DEVICE() today use it at an offset of 0 and so do all PCI devices using VMSTATE_PCI_DEVICE() apparently. VMSTATE_PCI_DEVICE_POINTER() would be unaffected, but is unused anyway. My survey also concluded that luckily all VMSTATE_PCIE_DEVICE() and VMSTATE_PCI_DEVICE() are placed as first VMStateField, so moving parent state to its class might be possible, similar to qdev props todays with class_base_init clearing it for derived types. However this would require to either refactor core VMState code to operate on a list, aggregated from one or more zero-terminated arrays, which I would consider invasive and error-prone, or simply have Device code allocate a new VMStateDescription before registering it in QOM realize (so it can be free'd on unrealize). Thoughts? Either way, it would work for CPU but not for PCI, since there are two different macros, VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() both for PCIDeviceClass. Not sure how to solve that without multi-inheritence. SHPC_VMSTATE() seems to be another macro beyond VMSTATE_MSIX() operating on PCIDevice but placed in an individual device (pci-bridge-dev). Can it be turned into a subsection, Michael? Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types 2013-07-28 12:36 ` Andreas Färber @ 2013-07-28 12:58 ` Michael S. Tsirkin 0 siblings, 0 replies; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-28 12:58 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, qemu-devel, Anthony Liguori, Juan Quintela On Sun, Jul 28, 2013 at 02:36:33PM +0200, Andreas Färber wrote: > Hi Anthony, > > Am 22.07.2013 22:29, schrieb Anthony Liguori: > > 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. > > I have changed v2 not to mix different-but-compatible struct types in > one VMStateDescription. > > Could you clarify if that was what you meant with the above? > > Or would you also be opposed to - post-1.6 - changing > VMSTATE_PCIE_DEVICE(parent_obj[.parent_obj], MyStruct) > to > VMSTATE_PCIE_DEVICE() > as suggested elsewhere in this thread? > > I'm thinking that writing VMSTATE_PCIE_DEVICE() already clearly > indicates the developer knows the device inherits from TYPE_PCI_DEVICE. > > All PCIe devices using VMSTATE_PCIE_DEVICE() today use it at an offset > of 0 and so do all PCI devices using VMSTATE_PCI_DEVICE() apparently. > VMSTATE_PCI_DEVICE_POINTER() would be unaffected, but is unused anyway. > > My survey also concluded that luckily all VMSTATE_PCIE_DEVICE() and > VMSTATE_PCI_DEVICE() are placed as first VMStateField, so moving parent > state to its class might be possible, similar to qdev props todays with > class_base_init clearing it for derived types. > However this would require to either refactor core VMState code to > operate on a list, aggregated from one or more zero-terminated arrays, > which I would consider invasive and error-prone, or simply have Device > code allocate a new VMStateDescription before registering it in QOM > realize (so it can be free'd on unrealize). Thoughts? > > Either way, it would work for CPU but not for PCI, since there are two > different macros, VMSTATE_PCI_DEVICE() and VMSTATE_PCIE_DEVICE() both > for PCIDeviceClass. Not sure how to solve that without multi-inheritence. Maybe combine them and use is_express to select the correct format. > SHPC_VMSTATE() seems to be another macro beyond VMSTATE_MSIX() operating > on PCIDevice but placed in an individual device (pci-bridge-dev). Can it > be turned into a subsection, Michael? > > Regards, > Andreas Not without breaking cross-version migration I think? > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types 2013-07-21 14:09 [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types Andreas Färber ` (3 preceding siblings ...) 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:22 ` Andreas Färber 4 siblings, 1 reply; 22+ messages in thread From: Michael S. Tsirkin @ 2013-07-21 20:26 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, Peter Crosthwaite, qemu-devel, Anthony Liguori, Hu Tao On Sun, Jul 21, 2013 at 04:09:00PM +0200, Andreas Färber wrote: > Hello Michael et al., > > This series turns PCIBridge, PCIEPort and PCIESlot into abstract QOM types, > so that we can use QOM casts to obtain a pointer. > > Possibly this was prompted by q35's PCIe? What was prompted? What's the question exactly? > Don't remember ATM... > > Regards, > Andreas > > Cc: Anthony Liguori <anthony@codemonkey.ws> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Cc: Hu Tao <hutao@cn.fujitsu.com> > > Andreas Färber (4): > pci-bridge: Turn into abstract QOM type > pci-bridge-dev: QOM parent field cleanup > pci-bridge/i82801b11: Rename parent field > pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types > > hw/pci-bridge/dec.c | 4 ++-- > hw/pci-bridge/i82801b11.c | 10 ++++++---- > hw/pci-bridge/ioh3420.c | 29 +++++++++++++-------------- > hw/pci-bridge/pci_bridge_dev.c | 26 +++++++++++++++---------- > hw/pci-bridge/xio3130_downstream.c | 29 +++++++++++++-------------- > hw/pci-bridge/xio3130_upstream.c | 20 +++++++++---------- > hw/pci-host/apb.c | 4 ++-- > hw/pci/pci_bridge.c | 40 +++++++++++++++++++++++++++----------- > hw/pci/pcie.c | 2 +- > hw/pci/pcie_port.c | 22 +++++++++++++++++++++ > include/hw/pci/pci_bus.h | 7 ++++++- > include/hw/pci/pcie_port.h | 14 +++++++++++-- > 12 files changed, 132 insertions(+), 75 deletions(-) > > -- > 1.8.1.4 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types 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 0 siblings, 1 reply; 22+ messages in thread From: Andreas Färber @ 2013-07-22 17:22 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Peter Crosthwaite, qemu-devel, Anthony Liguori, Hu Tao Am 21.07.2013 22:26, schrieb Michael S. Tsirkin: > On Sun, Jul 21, 2013 at 04:09:00PM +0200, Andreas Färber wrote: >> Hello Michael et al., >> >> This series turns PCIBridge, PCIEPort and PCIESlot into abstract QOM types, >> so that we can use QOM casts to obtain a pointer. >> >> Possibly this was prompted by q35's PCIe? > > What was prompted? This refactoring series. > What's the question exactly? Why I prepared this series on an offline train ride on Wednesday. ;) Not for you to answer - something PCIe must've gotten in the way of some QOM realize conversion but the branch is missing the final patch showing what these types and casts are good for. Since we're in Soft Freeze I rather wanted to flush my queues for review though rather than spend more time puzzling why I did this. :) Andreas > >> Don't remember ATM... >> >> Regards, >> Andreas >> >> Cc: Anthony Liguori <anthony@codemonkey.ws> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> Cc: Hu Tao <hutao@cn.fujitsu.com> >> >> Andreas Färber (4): >> pci-bridge: Turn into abstract QOM type >> pci-bridge-dev: QOM parent field cleanup >> pci-bridge/i82801b11: Rename parent field >> pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types >> >> hw/pci-bridge/dec.c | 4 ++-- >> hw/pci-bridge/i82801b11.c | 10 ++++++---- >> hw/pci-bridge/ioh3420.c | 29 +++++++++++++-------------- >> hw/pci-bridge/pci_bridge_dev.c | 26 +++++++++++++++---------- >> hw/pci-bridge/xio3130_downstream.c | 29 +++++++++++++-------------- >> hw/pci-bridge/xio3130_upstream.c | 20 +++++++++---------- >> hw/pci-host/apb.c | 4 ++-- >> hw/pci/pci_bridge.c | 40 +++++++++++++++++++++++++++----------- >> hw/pci/pcie.c | 2 +- >> hw/pci/pcie_port.c | 22 +++++++++++++++++++++ >> include/hw/pci/pci_bus.h | 7 ++++++- >> include/hw/pci/pcie_port.h | 14 +++++++++++-- >> 12 files changed, 132 insertions(+), 75 deletions(-) >> >> -- >> 1.8.1.4 > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH RFC qom-next 0/4] QOM'ification of pci-bridge types 2013-07-22 17:22 ` Andreas Färber @ 2013-07-22 22:05 ` Andreas Färber 0 siblings, 0 replies; 22+ messages in thread From: Andreas Färber @ 2013-07-22 22:05 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Peter Crosthwaite, qemu-devel, Anthony Liguori, Hu Tao Am 22.07.2013 19:22, schrieb Andreas Färber: > Am 21.07.2013 22:26, schrieb Michael S. Tsirkin: >> On Sun, Jul 21, 2013 at 04:09:00PM +0200, Andreas Färber wrote: >>> Hello Michael et al., >>> >>> This series turns PCIBridge, PCIEPort and PCIESlot into abstract QOM types, >>> so that we can use QOM casts to obtain a pointer. >>> >>> Possibly this was prompted by q35's PCIe? >> >> What was prompted? > > This refactoring series. > >> What's the question exactly? > > Why I prepared this series on an offline train ride on Wednesday. ;) Not > for you to answer - something PCIe must've gotten in the way of some QOM > realize conversion but the branch is missing the final patch showing > what these types and casts are good for. Doh, stumbled over it again: pci_bridge_initfn() should become PCIBridge's realizefn, and walking down the type hierarchy pcie_port_init_reg() should become PCIEPort's (which needs TYPE_PCIE_PORT), etc. That's a bigger refactoring though, so v2 will come without it. Andreas > Since we're in Soft Freeze I > rather wanted to flush my queues for review though rather than spend > more time puzzling why I did this. :) > > Andreas > >> >>> Don't remember ATM... >>> >>> Regards, >>> Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-07-28 12:56 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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.