[Qemu-devel] [PATCH qom-next for-1.6 00/11] sysbus: QOM cast cleanups -- plus outlook

[Qemu-devel] [PATCH qom-next for-1.6 00/11] sysbus: QOM cast cleanups -- plus outlook

[Andreas Färber]

Hello,

This series finally eliminates the FROM_SYSBUS() macro.
It goes on to fix remaining ->qdev field accesses (less work than I thought).

I've been pushing out device patches ASAP to give them maximum review hours,
now here follows the rationale why I made this effort:

i) Murphy's Law

When there's two ways to write a device and one is considered obsolete,
someone at some point will apply a new device using FROM_SYSBUS().

ii) Broken Window theory

If we let devices languish in an obsolete state, then someone will refuse to
update their device patch, pointing that it is no worse than in-tree devices.
(Ignoring that it makes it even more work for others to convert, who don't
know the code and how to best name the new macros.)

iii) Why for-1.6?

Once the Hard Freeze is lifted, traditionally we will see submaintainers
quickly flushing their queues, with arm-devs.next, ppc-next and others
potentially adding devices using FROM_SYSBUS() or other deprecated constructs.

Applying these cleanups for v1.6-rc0 will lead to build failures for most
obsolete constructs and thereby more fairly distributing the work of rebasing
and/or bouncing patches among submaintainers.

Experience shows that posting such touch-all series to the list and waiting
for weeks does not give each device/target test coverage by reviewers;
having it in qemu.git and -rcX will more likely uncover bugs through testing.
In most cases only initialization and reset will be affected, so we should see
failures immediately. Fixing bugs should be trivial, and I am around to do so.

Bleading-edge version tree available for testing here:
git://github.com/afaerber/qemu-cpu.git qom-devices
https://github.com/afaerber/qemu-cpu/commits/qom-devices

iv) What's up next?

Many preceding SysBusDevice conversion patches grew a DeviceState *dev variable
to prepare for conversion to QOM realize: dev variable and sbd argument will
simply switch place, avoiding variable renaming along with functional changes.

Loved by some, loathed by others, SysBusDevices are the simplest because
no initialization/unintialization really happens at bus level (there is no bus).

PCIDevice and others with multiple levels of initfn are waiting on proposals
under discussion of how to efficiently call the parent's method implementation.
I plan to submit a RFC inspired by Peter C.'s work for ISA/CPU/virtio.

Implementing QOM CPU topologies for x86, s390x and arm SoCs requires recursive
realization - offloading that to QOM infrastructure won't make it into 1.6.
So all devices being worked on still need to manually realize their children
where they qdev_init()ed them before, violating the same ordering constraints
Paolo brought up for the infrastructure-based approach. Feedback appreciated.

Embedding busses still faces challenges wrt device ID and VMState to be solved.

Peter M. has an RFC for how to allow embedding structs while shielding fields.

A script to inspect the QOM tree similar to qtree may come increasingly handy
the more we think about devices not on a bus or actually using the composition
model we've been building up for ACPI or qtests.

qtests will come very handy the more devices we touch. Having each machine
covered by some test would be a start, having each non-default device covered
even better, and be it no-op to exercise instantiation, hot-add/remove a bonus.

Probably many other QOM TODOs not on my mind right now. :-)

Regards,
Andreas

Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>

Andreas Färber (11):
  sysbus: Drop FROM_SYSBUS()
  sysbus: Remove unused sysbus_new() prototype
  exynos4210_i2c: QOM cast cleanup
  opencores_eth: QOM cast cleanup
  mipsnet: QOM cast cleanup
  kvmvapic: QOM cast cleanup
  kvm/ioapic: QOM cast cleanup
  ioapic: QOM cast cleanup
  mips_malta: QOM cast cleanup
  spapr_pci: QOM cast cleanup
  sysbus: QOM parent field cleanup for SysBusDevice

 hw/i2c/exynos4210_i2c.c | 12 +++++++-----
 hw/i386/kvm/ioapic.c    |  4 ++--
 hw/i386/kvmvapic.c      |  4 ++--
 hw/intc/ioapic.c        |  2 +-
 hw/mips/mips_malta.c    | 12 ++++++++----
 hw/net/mipsnet.c        | 20 ++++++++++++--------
 hw/net/opencores_eth.c  | 24 +++++++++++++++---------
 hw/ppc/spapr_pci.c      |  7 ++++---
 include/hw/sysbus.h     |  9 ++++-----
 9 files changed, 55 insertions(+), 39 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 01/11] sysbus: Drop FROM_SYSBUS()

[Andreas Färber]

As a replacement, use your own macro based on OBJECT_CHECK().

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/hw/sysbus.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index 8c17165..fbfc066 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -55,9 +55,6 @@ struct SysBusDevice {
     pio_addr_t pio[QDEV_MAX_PIO];
 };
 
-/* Macros to compensate for lack of type inheritance in C.  */
-#define FROM_SYSBUS(type, dev) DO_UPCAST(type, busdev, dev)
-
 void *sysbus_new(void);
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 02/11] sysbus: Remove unused sysbus_new() prototype

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/hw/sysbus.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index fbfc066..f1d1c5e 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -55,7 +55,6 @@ struct SysBusDevice {
     pio_addr_t pio[QDEV_MAX_PIO];
 };
 
-void *sysbus_new(void);
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 03/11] exynos4210_i2c: QOM cast cleanup

[Andreas Färber]

Fix one ->qdev access and rename parent field.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/i2c/exynos4210_i2c.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
index 42f5e89..ce5f849 100644
--- a/hw/i2c/exynos4210_i2c.c
+++ b/hw/i2c/exynos4210_i2c.c
@@ -80,7 +80,8 @@ static const char *exynos4_i2c_get_regname(unsigned offset)
 #endif
 
 typedef struct Exynos4210I2CState {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;
+
     MemoryRegion iomem;
     i2c_bus *bus;
     qemu_irq irq;
@@ -297,15 +298,16 @@ static void exynos4210_i2c_reset(DeviceState *d)
     s->scl_free = true;
 }
 
-static int exynos4210_i2c_realize(SysBusDevice *dev)
+static int exynos4210_i2c_realize(SysBusDevice *sbd)
 {
+    DeviceState *dev = DEVICE(sbd);
     Exynos4210I2CState *s = EXYNOS4_I2C(dev);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &exynos4210_i2c_ops, s,
                           TYPE_EXYNOS4_I2C, EXYNOS4_I2C_MEM_SIZE);
-    sysbus_init_mmio(dev, &s->iomem);
-    sysbus_init_irq(dev, &s->irq);
-    s->bus = i2c_init_bus(&dev->qdev, "i2c");
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+    s->bus = i2c_init_bus(dev, "i2c");
     return 0;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 04/11] opencores_eth: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/net/opencores_eth.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/hw/net/opencores_eth.c b/hw/net/opencores_eth.c
index 4637557..f2f82f2 100644
--- a/hw/net/opencores_eth.c
+++ b/hw/net/opencores_eth.c
@@ -267,8 +267,12 @@ typedef struct desc {
 
 #define DEFAULT_PHY 1
 
+#define TYPE_OPEN_ETH "open_eth"
+#define OPEN_ETH(obj) OBJECT_CHECK(OpenEthState, (obj), TYPE_OPEN_ETH)
+
 typedef struct OpenEthState {
-    SysBusDevice dev;
+    SysBusDevice parent_obj;
+
     NICState *nic;
     NICConf conf;
     MemoryRegion reg_io;
@@ -677,28 +681,30 @@ static const MemoryRegionOps open_eth_desc_ops = {
     .write = open_eth_desc_write,
 };
 
-static int sysbus_open_eth_init(SysBusDevice *dev)
+static int sysbus_open_eth_init(SysBusDevice *sbd)
 {
-    OpenEthState *s = DO_UPCAST(OpenEthState, dev, dev);
+    DeviceState *dev = DEVICE(sbd);
+    OpenEthState *s = OPEN_ETH(dev);
 
     memory_region_init_io(&s->reg_io, OBJECT(dev), &open_eth_reg_ops, s,
             "open_eth.regs", 0x54);
-    sysbus_init_mmio(dev, &s->reg_io);
+    sysbus_init_mmio(sbd, &s->reg_io);
 
     memory_region_init_io(&s->desc_io, OBJECT(dev), &open_eth_desc_ops, s,
             "open_eth.desc", 0x400);
-    sysbus_init_mmio(dev, &s->desc_io);
+    sysbus_init_mmio(sbd, &s->desc_io);
 
-    sysbus_init_irq(dev, &s->irq);
+    sysbus_init_irq(sbd, &s->irq);
 
     s->nic = qemu_new_nic(&net_open_eth_info, &s->conf,
-                          object_get_typename(OBJECT(s)), s->dev.qdev.id, s);
+                          object_get_typename(OBJECT(s)), dev->id, s);
     return 0;
 }
 
 static void qdev_open_eth_reset(DeviceState *dev)
 {
-    OpenEthState *d = DO_UPCAST(OpenEthState, dev.qdev, dev);
+    OpenEthState *d = OPEN_ETH(dev);
+
     open_eth_reset(d);
 }
 
@@ -719,7 +725,7 @@ static void open_eth_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo open_eth_info = {
-    .name          = "open_eth",
+    .name          = TYPE_OPEN_ETH,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(OpenEthState),
     .class_init    = open_eth_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 05/11] mipsnet: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/net/mipsnet.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/net/mipsnet.c b/hw/net/mipsnet.c
index 9080850..4c5a6f3 100644
--- a/hw/net/mipsnet.c
+++ b/hw/net/mipsnet.c
@@ -19,8 +19,11 @@
 
 #define MAX_ETH_FRAME_SIZE	1514
 
+#define TYPE_MIPS_NET "mipsnet"
+#define MIPS_NET(obj) OBJECT_CHECK(MIPSnetState, (obj), TYPE_MIPS_NET)
+
 typedef struct MIPSnetState {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;
 
     uint32_t busy;
     uint32_t rx_count;
@@ -231,17 +234,18 @@ static const MemoryRegionOps mipsnet_ioport_ops = {
     .impl.max_access_size = 4,
 };
 
-static int mipsnet_sysbus_init(SysBusDevice *dev)
+static int mipsnet_sysbus_init(SysBusDevice *sbd)
 {
-    MIPSnetState *s = DO_UPCAST(MIPSnetState, busdev, dev);
+    DeviceState *dev = DEVICE(sbd);
+    MIPSnetState *s = MIPS_NET(dev);
 
     memory_region_init_io(&s->io, OBJECT(dev), &mipsnet_ioport_ops, s,
                           "mipsnet-io", 36);
-    sysbus_init_mmio(dev, &s->io);
-    sysbus_init_irq(dev, &s->irq);
+    sysbus_init_mmio(sbd, &s->io);
+    sysbus_init_irq(sbd, &s->irq);
 
     s->nic = qemu_new_nic(&net_mipsnet_info, &s->conf,
-                          object_get_typename(OBJECT(dev)), dev->qdev.id, s);
+                          object_get_typename(OBJECT(dev)), dev->id, s);
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
 
     return 0;
@@ -249,7 +253,7 @@ static int mipsnet_sysbus_init(SysBusDevice *dev)
 
 static void mipsnet_sysbus_reset(DeviceState *dev)
 {
-    MIPSnetState *s = DO_UPCAST(MIPSnetState, busdev.qdev, dev);
+    MIPSnetState *s = MIPS_NET(dev);
     mipsnet_reset(s);
 }
 
@@ -271,7 +275,7 @@ static void mipsnet_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo mipsnet_info = {
-    .name          = "mipsnet",
+    .name          = TYPE_MIPS_NET,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(MIPSnetState),
     .class_init    = mipsnet_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 06/11] kvmvapic: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/kvmvapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index a4506bc..15beb80 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -456,7 +456,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
 void vapic_report_tpr_access(DeviceState *dev, CPUState *cs, target_ulong ip,
                              TPRAccess access)
 {
-    VAPICROMState *s = DO_UPCAST(VAPICROMState, busdev.qdev, dev);
+    VAPICROMState *s = VAPIC(dev);
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
 
@@ -508,7 +508,7 @@ static void vapic_enable_tpr_reporting(bool enable)
 
 static void vapic_reset(DeviceState *dev)
 {
-    VAPICROMState *s = DO_UPCAST(VAPICROMState, busdev.qdev, dev);
+    VAPICROMState *s = VAPIC(dev);
 
     if (s->state == VAPIC_ACTIVE) {
         s->state = VAPIC_STANDBY;
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 07/11] kvm/ioapic: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/i386/kvm/ioapic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 688cb5c..f11a540 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -112,7 +112,7 @@ static void kvm_ioapic_put(IOAPICCommonState *s)
 
 static void kvm_ioapic_reset(DeviceState *dev)
 {
-    IOAPICCommonState *s = DO_UPCAST(IOAPICCommonState, busdev.qdev, dev);
+    IOAPICCommonState *s = IOAPIC_COMMON(dev);
 
     ioapic_reset_common(dev);
     kvm_ioapic_put(s);
@@ -131,7 +131,7 @@ static void kvm_ioapic_init(IOAPICCommonState *s, int instance_no)
 {
     memory_region_init_reservation(&s->io_memory, NULL, "kvm-ioapic", 0x1000);
 
-    qdev_init_gpio_in(&s->busdev.qdev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
+    qdev_init_gpio_in(DEVICE(s), kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
 }
 
 static Property kvm_ioapic_properties[] = {
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 08/11] ioapic: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/intc/ioapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 5d064fe..d866e00 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -230,7 +230,7 @@ static void ioapic_init(IOAPICCommonState *s, int instance_no)
     memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
                           "ioapic", 0x1000);
 
-    qdev_init_gpio_in(&s->busdev.qdev, ioapic_set_irq, IOAPIC_NUM_PINS);
+    qdev_init_gpio_in(DEVICE(s), ioapic_set_irq, IOAPIC_NUM_PINS);
 
     ioapics[instance_no] = s;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 09/11] mips_malta: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/mips/mips_malta.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index dad58c0..e932fdc 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -79,8 +79,12 @@ typedef struct {
     SerialState *uart;
 } MaltaFPGAState;
 
+#define TYPE_MIPS_MALTA "mips-malta"
+#define MIPS_MALTA(obj) OBJECT_CHECK(MaltaState, (obj), TYPE_MIPS_MALTA)
+
 typedef struct {
-    SysBusDevice busdev;
+    SysBusDevice parent_obj;
+
     qemu_irq *i8259;
 } MaltaState;
 
@@ -808,8 +812,8 @@ void mips_malta_init(QEMUMachineInitArgs *args)
     int fl_sectors = bios_size >> 16;
     int be;
 
-    DeviceState *dev = qdev_create(NULL, "mips-malta");
-    MaltaState *s = DO_UPCAST(MaltaState, busdev.qdev, dev);
+    DeviceState *dev = qdev_create(NULL, TYPE_MIPS_MALTA);
+    MaltaState *s = MIPS_MALTA(dev);
 
     qdev_init_nofail(dev);
 
@@ -1004,7 +1008,7 @@ static void mips_malta_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo mips_malta_device = {
-    .name          = "mips-malta",
+    .name          = TYPE_MIPS_MALTA,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(MaltaState),
     .class_init    = mips_malta_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 10/11] spapr_pci: QOM cast cleanup

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/ppc/spapr_pci.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index c880a75..e6d34bc 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -479,6 +479,7 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, void *opaque, int devfn)
 
 static int spapr_phb_init(SysBusDevice *s)
 {
+    DeviceState *dev = DEVICE(s);
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     const char *busname;
@@ -596,14 +597,14 @@ static int spapr_phb_init(SysBusDevice *s)
      * since it's unique by construction, and makes the guest visible
      * BUID clear.
      */
-    if (s->qdev.id) {
+    if (dev->id) {
         busname = NULL;
     } else if (sphb->index == 0) {
         busname = "pci";
     } else {
         busname = sphb->dtbusname;
     }
-    bus = pci_register_bus(DEVICE(s), busname,
+    bus = pci_register_bus(dev, busname,
                            pci_spapr_set_irq, pci_spapr_map_irq, sphb,
                            &sphb->memspace, &sphb->iospace,
                            PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
@@ -611,7 +612,7 @@ static int spapr_phb_init(SysBusDevice *s)
 
     sphb->dma_window_start = 0;
     sphb->dma_window_size = 0x40000000;
-    sphb->tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
+    sphb->tcet = spapr_tce_new_table(dev, sphb->dma_liobn,
                                      sphb->dma_window_size);
     if (!sphb->tcet) {
         fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
-- 
1.8.1.4

[Qemu-devel] [PATCH qom-next for-1.6 11/11] sysbus: QOM parent field cleanup for SysBusDevice

[Andreas Färber]

Rename the parent field and hide it from gtk-doc.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/hw/sysbus.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index f1d1c5e..bb50a87 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -42,7 +42,10 @@ typedef struct SysBusDeviceClass {
 } SysBusDeviceClass;
 
 struct SysBusDevice {
-    DeviceState qdev;
+    /*< private >*/
+    DeviceState parent_obj;
+    /*< public >*/
+
     int num_irq;
     qemu_irq irqs[QDEV_MAX_IRQ];
     qemu_irq *irqp[QDEV_MAX_IRQ];
-- 
1.8.1.4