[Qemu-devel] [PATCH RFC 00/15] arm: A9MPCore+A15MPCore QOM'ification

[Qemu-devel] [PATCH RFC 00/15] arm: A9MPCore+A15MPCore QOM'ification

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Hello Peter,

This series fully QOM'ifies A9MPCore so that it can be embedded for Tegra2.
It goes on to do the same for A15MPCore, which had previously been taken as
template for Cortex-A57 by John Rigby.

Separate headers are introduced to only expose device state to whom asks for it.

Regards,
Andreas

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Cc: Hu Tao <hutao@cn.fujitsu.com>
Cc: Mian M. Hamayun <m.hamayun@virtualopensystems.com>
Cc: Claudio Fontana <claudio.fontana@huawei.com>
Cc: kvmarm@lists.cs.columbia.edu

Andreas Färber (15):
  cpu/a9mpcore: QOM casting cleanup
  cpu/a9mpcore: Split off instance_init
  cpu/a9mpcore: Embed GICState
  misc/a9scu: QOM cleanups
  cpu/a9mpcore: Embed A9SCUState
  timer/arm_mptimer: QOM cast cleanup
  timer/arm_mptimer: Convert to QOM realize
  cpu/a9mpcore: Embed ARMMPTimerState
  cpu/a9mpcore: Convert to QOM realize
  cpu/a9mpcore: Prepare for QOM embedding
  cpu/a15mpcore: QOM cast cleanup
  cpu/a15mpcore: Split off instance_init
  cpu/a15mpcore: Embed GICState
  cpu/a15mpcore: Convert to QOM realize
  cpu/a15mpcore: Prepare for QOM embedding

 hw/cpu/a15mpcore.c             |  64 +++++++++++-----------
 hw/cpu/a9mpcore.c              | 118 +++++++++++++++++++++++------------------
 hw/misc/a9scu.c                |  23 ++------
 hw/timer/arm_mptimer.c         |  62 +++++++++-------------
 include/hw/cpu/a15mpcore.h     |  44 +++++++++++++++
 include/hw/cpu/a9mpcore.h      |  37 +++++++++++++
 include/hw/misc/a9scu.h        |  31 +++++++++++
 include/hw/timer/arm_mptimer.h |  54 +++++++++++++++++++
 8 files changed, 294 insertions(+), 139 deletions(-)
 create mode 100644 include/hw/cpu/a15mpcore.h
 create mode 100644 include/hw/cpu/a9mpcore.h
 create mode 100644 include/hw/misc/a9scu.h
 create mode 100644 include/hw/timer/arm_mptimer.h

-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 01/15] cpu/a9mpcore: QOM casting cleanup

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Introduce type constant and cast macro and enforce its use by
renaming A9MPPrivState::busdev field to parent_obj.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 0a1a10f..36254e9 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -10,8 +10,15 @@
 
 #include "hw/sysbus.h"
 
+#define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
+#define A9MPCORE_PRIV(obj) \
+    OBJECT_CHECK(A9MPPrivState, (obj), TYPE_A9MPCORE_PRIV)
+
 typedef struct A9MPPrivState {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     uint32_t num_cpu;
     MemoryRegion container;
     DeviceState *mptimer;
@@ -29,7 +36,7 @@ static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
-    A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
+    A9MPPrivState *s = A9MPCORE_PRIV(dev);
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
     int i;
 
@@ -43,7 +50,7 @@ static int a9mp_priv_init(SysBusDevice *dev)
     sysbus_pass_irq(dev, gicbusdev);
 
     /* Pass through inbound GPIO lines to the GIC */
-    qdev_init_gpio_in(&s->busdev.qdev, a9mp_priv_set_irq, s->num_irq - 32);
+    qdev_init_gpio_in(DEVICE(dev), a9mp_priv_set_irq, s->num_irq - 32);
 
     s->scu = qdev_create(NULL, "a9-scu");
     qdev_prop_set_uint32(s->scu, "num-cpu", s->num_cpu);
@@ -124,7 +131,7 @@ static void a9mp_priv_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo a9mp_priv_info = {
-    .name          = "a9mpcore_priv",
+    .name          = TYPE_A9MPCORE_PRIV,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(A9MPPrivState),
     .class_init    = a9mp_priv_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 02/15] cpu/a9mpcore: Split off instance_init

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 36254e9..63a4eb1 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -34,6 +34,14 @@ static void a9mp_priv_set_irq(void *opaque, int irq, int level)
     qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
 }
 
+static void a9mp_priv_initfn(Object *obj)
+{
+    A9MPPrivState *s = A9MPCORE_PRIV(obj);
+
+    memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
+}
+
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
@@ -78,7 +86,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
      *
      * We should implement the global timer but don't currently do so.
      */
-    memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
     memory_region_add_subregion(&s->container, 0,
                                 sysbus_mmio_get_region(scubusdev, 0));
     /* GIC CPU interface */
@@ -94,8 +101,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
     memory_region_add_subregion(&s->container, 0x1000,
                                 sysbus_mmio_get_region(gicbusdev, 0));
 
-    sysbus_init_mmio(dev, &s->container);
-
     /* Wire up the interrupt from each watchdog and timer.
      * For each core the timer is PPI 29 and the watchdog PPI 30.
      */
@@ -134,6 +139,7 @@ static const TypeInfo a9mp_priv_info = {
     .name          = TYPE_A9MPCORE_PRIV,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(A9MPPrivState),
+    .instance_init = a9mp_priv_initfn,
     .class_init    = a9mp_priv_class_init,
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 03/15] cpu/a9mpcore: Embed GICState

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Prepares for conversion to QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 63a4eb1..6340b0f 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -9,6 +9,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "hw/intc/gic_internal.h"
 
 #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
 #define A9MPCORE_PRIV(obj) \
@@ -23,15 +24,17 @@ typedef struct A9MPPrivState {
     MemoryRegion container;
     DeviceState *mptimer;
     DeviceState *wdt;
-    DeviceState *gic;
     DeviceState *scu;
     uint32_t num_irq;
+
+    GICState gic;
 } A9MPPrivState;
 
 static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 {
     A9MPPrivState *s = (A9MPPrivState *)opaque;
-    qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
+
+    qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->gic), irq), level);
 }
 
 static void a9mp_priv_initfn(Object *obj)
@@ -40,19 +43,23 @@ static void a9mp_priv_initfn(Object *obj)
 
     memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->container);
+
+    object_initialize(&s->gic, TYPE_ARM_GIC);
+    qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
 }
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
+    DeviceState *gicdev;
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
     int i;
 
-    s->gic = qdev_create(NULL, "arm_gic");
-    qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu);
-    qdev_prop_set_uint32(s->gic, "num-irq", s->num_irq);
-    qdev_init_nofail(s->gic);
-    gicbusdev = SYS_BUS_DEVICE(s->gic);
+    gicdev = DEVICE(&s->gic);
+    qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
+    qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+    qdev_init_nofail(gicdev);
+    gicbusdev = SYS_BUS_DEVICE(&s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
     sysbus_pass_irq(dev, gicbusdev);
@@ -107,9 +114,9 @@ static int a9mp_priv_init(SysBusDevice *dev)
     for (i = 0; i < s->num_cpu; i++) {
         int ppibase = (s->num_irq - 32) + i * 32;
         sysbus_connect_irq(timerbusdev, i,
-                           qdev_get_gpio_in(s->gic, ppibase + 29));
+                           qdev_get_gpio_in(gicdev, ppibase + 29));
         sysbus_connect_irq(wdtbusdev, i,
-                           qdev_get_gpio_in(s->gic, ppibase + 30));
+                           qdev_get_gpio_in(gicdev, ppibase + 30));
     }
     return 0;
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 04/15] misc/a9scu: QOM cleanups

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Rename A9SCUState::busdev field to parent_obj and turn realizefn into an
instance_init function to allow early MMIO mapping.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/misc/a9scu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 05897c2..30f6fac 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -13,7 +13,10 @@
 /* A9MP private memory region.  */
 
 typedef struct A9SCUState {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     MemoryRegion iomem;
     uint32_t control;
     uint32_t status;
@@ -114,10 +117,10 @@ static void a9_scu_reset(DeviceState *dev)
     s->control = 0;
 }
 
-static void a9_scu_realize(DeviceState *dev, Error ** errp)
+static void a9_scu_init(Object *obj)
 {
-    A9SCUState *s = A9_SCU(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    A9SCUState *s = A9_SCU(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     memory_region_init_io(&s->iomem, &a9_scu_ops, s, "a9-scu", 0x100);
     sysbus_init_mmio(sbd, &s->iomem);
@@ -143,7 +146,6 @@ static void a9_scu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
-    dc->realize = a9_scu_realize;
     dc->props = a9_scu_properties;
     dc->vmsd = &vmstate_a9_scu;
     dc->reset = a9_scu_reset;
@@ -153,6 +155,7 @@ static const TypeInfo a9_scu_info = {
     .name          = TYPE_A9_SCU,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(A9SCUState),
+    .instance_init = a9_scu_init,
     .class_init    = a9_scu_class_init,
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 05/15] cpu/a9mpcore: Embed A9SCUState

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c       | 23 ++++++++++++++---------
 hw/misc/a9scu.c         | 18 +-----------------
 include/hw/misc/a9scu.h | 31 +++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 26 deletions(-)
 create mode 100644 include/hw/misc/a9scu.h

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 6340b0f..b7148fa 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -10,6 +10,7 @@
 
 #include "hw/sysbus.h"
 #include "hw/intc/gic_internal.h"
+#include "hw/misc/a9scu.h"
 
 #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
 #define A9MPCORE_PRIV(obj) \
@@ -24,10 +25,10 @@ typedef struct A9MPPrivState {
     MemoryRegion container;
     DeviceState *mptimer;
     DeviceState *wdt;
-    DeviceState *scu;
     uint32_t num_irq;
 
     GICState gic;
+    A9SCUState scu;
 } A9MPPrivState;
 
 static void a9mp_priv_set_irq(void *opaque, int irq, int level)
@@ -39,6 +40,7 @@ static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 
 static void a9mp_priv_initfn(Object *obj)
 {
+    SysBusDevice *sbd;
     A9MPPrivState *s = A9MPCORE_PRIV(obj);
 
     memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
@@ -46,13 +48,19 @@ static void a9mp_priv_initfn(Object *obj)
 
     object_initialize(&s->gic, TYPE_ARM_GIC);
     qdev_set_parent_bus(DEVICE(&s->gic), sysbus_get_default());
+
+    object_initialize(&s->scu, TYPE_A9_SCU);
+    qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+    sbd = SYS_BUS_DEVICE(&s->scu);
+    memory_region_add_subregion(&s->container, 0,
+                                sysbus_mmio_get_region(sbd, 0));
 }
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *gicdev;
-    SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
+    DeviceState *gicdev, *scudev;
+    SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev;
     int i;
 
     gicdev = DEVICE(&s->gic);
@@ -67,10 +75,9 @@ static int a9mp_priv_init(SysBusDevice *dev)
     /* Pass through inbound GPIO lines to the GIC */
     qdev_init_gpio_in(DEVICE(dev), a9mp_priv_set_irq, s->num_irq - 32);
 
-    s->scu = qdev_create(NULL, "a9-scu");
-    qdev_prop_set_uint32(s->scu, "num-cpu", s->num_cpu);
-    qdev_init_nofail(s->scu);
-    scubusdev = SYS_BUS_DEVICE(s->scu);
+    scudev = DEVICE(&s->scu);
+    qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
+    qdev_init_nofail(scudev);
 
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
@@ -93,8 +100,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
      *
      * We should implement the global timer but don't currently do so.
      */
-    memory_region_add_subregion(&s->container, 0,
-                                sysbus_mmio_get_region(scubusdev, 0));
     /* GIC CPU interface */
     memory_region_add_subregion(&s->container, 0x100,
                                 sysbus_mmio_get_region(gicbusdev, 1));
diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 30f6fac..b5a6087 100644
--- a/hw/misc/a9scu.c
+++ b/hw/misc/a9scu.c
@@ -8,23 +8,7 @@
  * This code is licensed under the GPL.
  */
 
-#include "hw/sysbus.h"
-
-/* A9MP private memory region.  */
-
-typedef struct A9SCUState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    MemoryRegion iomem;
-    uint32_t control;
-    uint32_t status;
-    uint32_t num_cpu;
-} A9SCUState;
-
-#define TYPE_A9_SCU "a9-scu"
-#define A9_SCU(obj) OBJECT_CHECK(A9SCUState, (obj), TYPE_A9_SCU)
+#include "hw/misc/a9scu.h"
 
 static uint64_t a9_scu_read(void *opaque, hwaddr offset,
                             unsigned size)
diff --git a/include/hw/misc/a9scu.h b/include/hw/misc/a9scu.h
new file mode 100644
index 0000000..efb0c30
--- /dev/null
+++ b/include/hw/misc/a9scu.h
@@ -0,0 +1,31 @@
+/*
+ * Cortex-A9MPCore Snoop Control Unit (SCU) emulation.
+ *
+ * Copyright (c) 2009 CodeSourcery.
+ * Copyright (c) 2011 Linaro Limited.
+ * Written by Paul Brook, Peter Maydell.
+ *
+ * This code is licensed under the GPL.
+ */
+#ifndef HW_MISC_A9SCU_H
+#define HW_MISC_A9SCU_H
+
+#include "hw/sysbus.h"
+
+/* A9MP private memory region.  */
+
+typedef struct A9SCUState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    MemoryRegion iomem;
+    uint32_t control;
+    uint32_t status;
+    uint32_t num_cpu;
+} A9SCUState;
+
+#define TYPE_A9_SCU "a9-scu"
+#define A9_SCU(obj) OBJECT_CHECK(A9SCUState, (obj), TYPE_A9_SCU)
+
+#endif
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 06/15] timer/arm_mptimer: QOM cast cleanup

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Introduce type constant and cast macro and rename
ARMMPTimerState::busdev to enforce its use.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/timer/arm_mptimer.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 317f5e4..588e34b 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -40,8 +40,15 @@ typedef struct {
     MemoryRegion iomem;
 } TimerBlock;
 
+#define TYPE_ARM_MP_TIMER "arm_mptimer"
+#define ARM_MP_TIMER(obj) \
+    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MP_TIMER)
+
 typedef struct {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     uint32_t num_cpu;
     TimerBlock timerblock[MAX_CPUS];
     MemoryRegion iomem;
@@ -211,9 +218,9 @@ static void timerblock_reset(TimerBlock *tb)
 
 static void arm_mptimer_reset(DeviceState *dev)
 {
-    ARMMPTimerState *s =
-        FROM_SYSBUS(ARMMPTimerState, SYS_BUS_DEVICE(dev));
+    ARMMPTimerState *s = ARM_MP_TIMER(dev);
     int i;
+
     for (i = 0; i < ARRAY_SIZE(s->timerblock); i++) {
         timerblock_reset(&s->timerblock[i]);
     }
@@ -221,8 +228,9 @@ static void arm_mptimer_reset(DeviceState *dev)
 
 static int arm_mptimer_init(SysBusDevice *dev)
 {
-    ARMMPTimerState *s = FROM_SYSBUS(ARMMPTimerState, dev);
+    ARMMPTimerState *s = ARM_MP_TIMER(dev);
     int i;
+
     if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
         hw_error("%s: num-cpu must be between 1 and %d\n", __func__, MAX_CPUS);
     }
@@ -295,7 +303,7 @@ static void arm_mptimer_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo arm_mptimer_info = {
-    .name          = "arm_mptimer",
+    .name          = TYPE_ARM_MP_TIMER,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(ARMMPTimerState),
     .class_init    = arm_mptimer_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 07/15] timer/arm_mptimer: Convert to QOM realize

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Split the SysBusDevice initfn into instance_init and realizefn.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/timer/arm_mptimer.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 588e34b..a19ffa3 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev)
     }
 }
 
-static int arm_mptimer_init(SysBusDevice *dev)
+static void arm_mptimer_init(Object *obj)
 {
+    ARMMPTimerState *s = ARM_MP_TIMER(obj);
+
+    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
+                          "arm_mptimer_timer", 0x20);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static void arm_mptimer_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     ARMMPTimerState *s = ARM_MP_TIMER(dev);
     int i;
 
@@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev)
      *  * timer for core 1
      * and so on.
      */
-    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
-                          "arm_mptimer_timer", 0x20);
-    sysbus_init_mmio(dev, &s->iomem);
     for (i = 0; i < s->num_cpu; i++) {
         TimerBlock *tb = &s->timerblock[i];
         tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb);
-        sysbus_init_irq(dev, &tb->irq);
+        sysbus_init_irq(sbd, &tb->irq);
         memory_region_init_io(&tb->iomem, &timerblock_ops, tb,
                               "arm_mptimer_timerblock", 0x20);
-        sysbus_init_mmio(dev, &tb->iomem);
+        sysbus_init_mmio(sbd, &tb->iomem);
     }
-
-    return 0;
 }
 
 static const VMStateDescription vmstate_timerblock = {
@@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = {
 static void arm_mptimer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
 
-    sbc->init = arm_mptimer_init;
+    dc->realize = arm_mptimer_realize;
     dc->vmsd = &vmstate_arm_mptimer;
     dc->reset = arm_mptimer_reset;
     dc->no_user = 1;
@@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = {
     .name          = TYPE_ARM_MP_TIMER,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(ARMMPTimerState),
+    .instance_init = arm_mptimer_init,
     .class_init    = arm_mptimer_class_init,
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 08/15] cpu/a9mpcore: Embed ARMMPTimerState

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c              | 46 +++++++++++++++++++++--------------
 hw/timer/arm_mptimer.c         | 35 ++++-----------------------
 include/hw/timer/arm_mptimer.h | 54 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 49 deletions(-)
 create mode 100644 include/hw/timer/arm_mptimer.h

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index b7148fa..48f5897 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -11,6 +11,7 @@
 #include "hw/sysbus.h"
 #include "hw/intc/gic_internal.h"
 #include "hw/misc/a9scu.h"
+#include "hw/timer/arm_mptimer.h"
 
 #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
 #define A9MPCORE_PRIV(obj) \
@@ -23,12 +24,12 @@ typedef struct A9MPPrivState {
 
     uint32_t num_cpu;
     MemoryRegion container;
-    DeviceState *mptimer;
-    DeviceState *wdt;
     uint32_t num_irq;
 
     GICState gic;
     A9SCUState scu;
+    ARMMPTimerState mptimer;
+    ARMMPTimerState wdt;
 } A9MPPrivState;
 
 static void a9mp_priv_set_irq(void *opaque, int irq, int level)
@@ -54,12 +55,28 @@ static void a9mp_priv_initfn(Object *obj)
     sbd = SYS_BUS_DEVICE(&s->scu);
     memory_region_add_subregion(&s->container, 0,
                                 sysbus_mmio_get_region(sbd, 0));
+
+    object_initialize(&s->mptimer, TYPE_ARM_MP_TIMER);
+    qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
+
+    object_initialize(&s->wdt, TYPE_ARM_MP_TIMER);
+    qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
+
+    /* Note that the A9 exposes only the "timer/watchdog for this core"
+     * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
+     */
+    sbd = SYS_BUS_DEVICE(&s->mptimer);
+    memory_region_add_subregion(&s->container, 0x600,
+                                sysbus_mmio_get_region(sbd, 0));
+    sbd = SYS_BUS_DEVICE(&s->wdt);
+    memory_region_add_subregion(&s->container, 0x620,
+                                sysbus_mmio_get_region(sbd, 0));
 }
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *gicdev, *scudev;
+    DeviceState *gicdev, *scudev, *mptimerdev, *wdtdev;
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev;
     int i;
 
@@ -79,15 +96,15 @@ static int a9mp_priv_init(SysBusDevice *dev)
     qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
     qdev_init_nofail(scudev);
 
-    s->mptimer = qdev_create(NULL, "arm_mptimer");
-    qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
-    qdev_init_nofail(s->mptimer);
-    timerbusdev = SYS_BUS_DEVICE(s->mptimer);
+    mptimerdev = DEVICE(&s->mptimer);
+    qdev_prop_set_uint32(mptimerdev, "num-cpu", s->num_cpu);
+    qdev_init_nofail(mptimerdev);
+    timerbusdev = SYS_BUS_DEVICE(&s->mptimer);
 
-    s->wdt = qdev_create(NULL, "arm_mptimer");
-    qdev_prop_set_uint32(s->wdt, "num-cpu", s->num_cpu);
-    qdev_init_nofail(s->wdt);
-    wdtbusdev = SYS_BUS_DEVICE(s->wdt);
+    wdtdev = DEVICE(&s->wdt);
+    qdev_prop_set_uint32(wdtdev, "num-cpu", s->num_cpu);
+    qdev_init_nofail(wdtdev);
+    wdtbusdev = SYS_BUS_DEVICE(&s->wdt);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
      *  0x0000-0x00ff -- Snoop Control Unit
@@ -103,13 +120,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
     /* GIC CPU interface */
     memory_region_add_subregion(&s->container, 0x100,
                                 sysbus_mmio_get_region(gicbusdev, 1));
-    /* Note that the A9 exposes only the "timer/watchdog for this core"
-     * memory region, not the "timer/watchdog for core X" ones 11MPcore has.
-     */
-    memory_region_add_subregion(&s->container, 0x600,
-                                sysbus_mmio_get_region(timerbusdev, 0));
-    memory_region_add_subregion(&s->container, 0x620,
-                                sysbus_mmio_get_region(wdtbusdev, 0));
     memory_region_add_subregion(&s->container, 0x1000,
                                 sysbus_mmio_get_region(gicbusdev, 0));
 
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index a19ffa3..5558f40 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -19,41 +19,13 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "hw/sysbus.h"
+#include "hw/timer/arm_mptimer.h"
 #include "qemu/timer.h"
 
 /* This device implements the per-cpu private timer and watchdog block
  * which is used in both the ARM11MPCore and Cortex-A9MP.
  */
 
-#define MAX_CPUS 4
-
-/* State of a single timer or watchdog block */
-typedef struct {
-    uint32_t count;
-    uint32_t load;
-    uint32_t control;
-    uint32_t status;
-    int64_t tick;
-    QEMUTimer *timer;
-    qemu_irq irq;
-    MemoryRegion iomem;
-} TimerBlock;
-
-#define TYPE_ARM_MP_TIMER "arm_mptimer"
-#define ARM_MP_TIMER(obj) \
-    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MP_TIMER)
-
-typedef struct {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint32_t num_cpu;
-    TimerBlock timerblock[MAX_CPUS];
-    MemoryRegion iomem;
-} ARMMPTimerState;
-
 static inline int get_current_cpu(ARMMPTimerState *s)
 {
     CPUState *cpu_single_cpu = ENV_GET_CPU(cpu_single_env);
@@ -241,8 +213,9 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
     ARMMPTimerState *s = ARM_MP_TIMER(dev);
     int i;
 
-    if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
-        hw_error("%s: num-cpu must be between 1 and %d\n", __func__, MAX_CPUS);
+    if (s->num_cpu < 1 || s->num_cpu > ARM_MPTIMER_MAX_CPUS) {
+        hw_error("%s: num-cpu must be between 1 and %d\n",
+                 __func__, ARM_MPTIMER_MAX_CPUS);
     }
     /* We implement one timer block per CPU, and expose multiple MMIO regions:
      *  * region 0 is "timer for this core"
diff --git a/include/hw/timer/arm_mptimer.h b/include/hw/timer/arm_mptimer.h
new file mode 100644
index 0000000..40a5fee
--- /dev/null
+++ b/include/hw/timer/arm_mptimer.h
@@ -0,0 +1,54 @@
+/*
+ * Private peripheral timer/watchdog blocks for ARM 11MPCore and A9MP
+ *
+ * Copyright (c) 2006-2007 CodeSourcery.
+ * Copyright (c) 2011 Linaro Limited
+ * Written by Paul Brook, Peter Maydell
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef HW_TIMER_ARM_MPTIMER_H
+#define HW_TIMER_ARM_MPTIMER_H
+
+#include "hw/sysbus.h"
+
+#define ARM_MPTIMER_MAX_CPUS 4
+
+/* State of a single timer or watchdog block */
+typedef struct {
+    uint32_t count;
+    uint32_t load;
+    uint32_t control;
+    uint32_t status;
+    int64_t tick;
+    QEMUTimer *timer;
+    qemu_irq irq;
+    MemoryRegion iomem;
+} TimerBlock;
+
+#define TYPE_ARM_MP_TIMER "arm_mptimer"
+#define ARM_MP_TIMER(obj) \
+    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MP_TIMER)
+
+typedef struct {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint32_t num_cpu;
+    TimerBlock timerblock[ARM_MPTIMER_MAX_CPUS];
+    MemoryRegion iomem;
+} ARMMPTimerState;
+
+#endif
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 09/15] cpu/a9mpcore: Convert to QOM realize

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 48f5897..67ecf5d 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -73,8 +73,9 @@ static void a9mp_priv_initfn(Object *obj)
                                 sysbus_mmio_get_region(sbd, 0));
 }
 
-static int a9mp_priv_init(SysBusDevice *dev)
+static void a9mp_priv_realize(DeviceState *dev, Error **errp)
 {
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
     DeviceState *gicdev, *scudev, *mptimerdev, *wdtdev;
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev;
@@ -87,10 +88,10 @@ static int a9mp_priv_init(SysBusDevice *dev)
     gicbusdev = SYS_BUS_DEVICE(&s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
-    sysbus_pass_irq(dev, gicbusdev);
+    sysbus_pass_irq(sbd, gicbusdev);
 
     /* Pass through inbound GPIO lines to the GIC */
-    qdev_init_gpio_in(DEVICE(dev), a9mp_priv_set_irq, s->num_irq - 32);
+    qdev_init_gpio_in(dev, a9mp_priv_set_irq, s->num_irq - 32);
 
     scudev = DEVICE(&s->scu);
     qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
@@ -133,7 +134,6 @@ static int a9mp_priv_init(SysBusDevice *dev)
         sysbus_connect_irq(wdtbusdev, i,
                            qdev_get_gpio_in(gicdev, ppibase + 30));
     }
-    return 0;
 }
 
 static Property a9mp_priv_properties[] = {
@@ -151,9 +151,8 @@ static Property a9mp_priv_properties[] = {
 static void a9mp_priv_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
 
-    k->init = a9mp_priv_init;
+    dc->realize = a9mp_priv_realize;
     dc->props = a9mp_priv_properties;
 }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 10/15] cpu/a9mpcore: Prepare for QOM embedding

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a9mpcore.c         | 24 +-----------------------
 include/hw/cpu/a9mpcore.h | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 23 deletions(-)
 create mode 100644 include/hw/cpu/a9mpcore.h

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 67ecf5d..d70eae6 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -8,29 +8,7 @@
  * This code is licensed under the GPL.
  */
 
-#include "hw/sysbus.h"
-#include "hw/intc/gic_internal.h"
-#include "hw/misc/a9scu.h"
-#include "hw/timer/arm_mptimer.h"
-
-#define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
-#define A9MPCORE_PRIV(obj) \
-    OBJECT_CHECK(A9MPPrivState, (obj), TYPE_A9MPCORE_PRIV)
-
-typedef struct A9MPPrivState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint32_t num_cpu;
-    MemoryRegion container;
-    uint32_t num_irq;
-
-    GICState gic;
-    A9SCUState scu;
-    ARMMPTimerState mptimer;
-    ARMMPTimerState wdt;
-} A9MPPrivState;
+#include "hw/cpu/a9mpcore.h"
 
 static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 {
diff --git a/include/hw/cpu/a9mpcore.h b/include/hw/cpu/a9mpcore.h
new file mode 100644
index 0000000..4a9ffc8
--- /dev/null
+++ b/include/hw/cpu/a9mpcore.h
@@ -0,0 +1,37 @@
+/*
+ * Cortex-A9MPCore internal peripheral emulation.
+ *
+ * Copyright (c) 2009 CodeSourcery.
+ * Copyright (c) 2011 Linaro Limited.
+ * Written by Paul Brook, Peter Maydell.
+ *
+ * This code is licensed under the GPL.
+ */
+#ifndef HW_CPU_A9MPCORE_H
+#define HW_CPU_A9MPCORE_H
+
+#include "hw/sysbus.h"
+#include "hw/intc/gic_internal.h"
+#include "hw/misc/a9scu.h"
+#include "hw/timer/arm_mptimer.h"
+
+#define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
+#define A9MPCORE_PRIV(obj) \
+    OBJECT_CHECK(A9MPPrivState, (obj), TYPE_A9MPCORE_PRIV)
+
+typedef struct A9MPPrivState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint32_t num_cpu;
+    MemoryRegion container;
+    uint32_t num_irq;
+
+    GICState gic;
+    A9SCUState scu;
+    ARMMPTimerState mptimer;
+    ARMMPTimerState wdt;
+} A9MPPrivState;
+
+#endif
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 11/15] cpu/a15mpcore: QOM cast cleanup

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Introduce type constant and cast macro and rename A15MPPrivState::busdev
field to parent_obj to enforce its use.

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a15mpcore.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 648656d..b30fe0c 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -23,8 +23,15 @@
 
 /* A15MP private memory region.  */
 
+#define TYPE_A15MPCORE_PRIV "a15mpcore_priv"
+#define A15MPCORE_PRIV(obj) \
+    OBJECT_CHECK(A15MPPrivState, (obj), TYPE_A15MPCORE_PRIV)
+
 typedef struct A15MPPrivState {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     uint32_t num_cpu;
     uint32_t num_irq;
     MemoryRegion container;
@@ -39,7 +46,7 @@ static void a15mp_priv_set_irq(void *opaque, int irq, int level)
 
 static int a15mp_priv_init(SysBusDevice *dev)
 {
-    A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev);
+    A15MPPrivState *s = A15MPCORE_PRIV(dev);
     SysBusDevice *busdev;
     const char *gictype = "arm_gic";
 
@@ -58,7 +65,7 @@ static int a15mp_priv_init(SysBusDevice *dev)
     sysbus_pass_irq(dev, busdev);
 
     /* Pass through inbound GPIO lines to the GIC */
-    qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq - 32);
+    qdev_init_gpio_in(DEVICE(dev), a15mp_priv_set_irq, s->num_irq - 32);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
      *  0x0000-0x0fff -- reserved
@@ -100,7 +107,7 @@ static void a15mp_priv_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo a15mp_priv_info = {
-    .name  = "a15mpcore_priv",
+    .name  = TYPE_A15MPCORE_PRIV,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size  = sizeof(A15MPPrivState),
     .class_init = a15mp_priv_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 12/15] cpu/a15mpcore: Split off instance_init

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a15mpcore.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index b30fe0c..1a15bf8 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -44,6 +44,15 @@ static void a15mp_priv_set_irq(void *opaque, int irq, int level)
     qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
 }
 
+static void a15mp_priv_initfn(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    A15MPPrivState *s = A15MPCORE_PRIV(obj);
+
+    memory_region_init(&s->container, "a15mp-priv-container", 0x8000);
+    sysbus_init_mmio(sbd, &s->container);
+}
+
 static int a15mp_priv_init(SysBusDevice *dev)
 {
     A15MPPrivState *s = A15MPCORE_PRIV(dev);
@@ -75,13 +84,11 @@ static int a15mp_priv_init(SysBusDevice *dev)
      *  0x5000-0x5fff -- GIC virtual interface control (not modelled)
      *  0x6000-0x7fff -- GIC virtual CPU interface (not modelled)
      */
-    memory_region_init(&s->container, "a15mp-priv-container", 0x8000);
     memory_region_add_subregion(&s->container, 0x1000,
                                 sysbus_mmio_get_region(busdev, 0));
     memory_region_add_subregion(&s->container, 0x2000,
                                 sysbus_mmio_get_region(busdev, 1));
 
-    sysbus_init_mmio(dev, &s->container);
     return 0;
 }
 
@@ -110,6 +117,7 @@ static const TypeInfo a15mp_priv_info = {
     .name  = TYPE_A15MPCORE_PRIV,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size  = sizeof(A15MPPrivState),
+    .instance_init = a15mp_priv_initfn,
     .class_init = a15mp_priv_class_init,
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 13/15] cpu/a15mpcore: Embed GICState

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

This covers both emulated and KVM GIC.

Prepares for QOM realize.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a15mpcore.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 1a15bf8..ee09b57 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -20,6 +20,7 @@
 
 #include "hw/sysbus.h"
 #include "sysemu/kvm.h"
+#include "hw/intc/gic_internal.h"
 
 /* A15MP private memory region.  */
 
@@ -35,40 +36,48 @@ typedef struct A15MPPrivState {
     uint32_t num_cpu;
     uint32_t num_irq;
     MemoryRegion container;
-    DeviceState *gic;
+
+    GICState gic;
 } A15MPPrivState;
 
 static void a15mp_priv_set_irq(void *opaque, int irq, int level)
 {
     A15MPPrivState *s = (A15MPPrivState *)opaque;
-    qemu_set_irq(qdev_get_gpio_in(s->gic, irq), level);
+
+    qemu_set_irq(qdev_get_gpio_in(DEVICE(&s->gic), irq), level);
 }
 
 static void a15mp_priv_initfn(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     A15MPPrivState *s = A15MPCORE_PRIV(obj);
+    DeviceState *gicdev;
+    const char *gictype = "arm_gic";
+
+    if (kvm_irqchip_in_kernel()) {
+        gictype = "kvm-arm-gic";
+    }
 
     memory_region_init(&s->container, "a15mp-priv-container", 0x8000);
     sysbus_init_mmio(sbd, &s->container);
+
+    object_initialize(&s->gic, gictype);
+    gicdev = DEVICE(&s->gic);
+    qdev_set_parent_bus(gicdev, sysbus_get_default());
+    qdev_prop_set_uint32(gicdev, "revision", 2);
 }
 
 static int a15mp_priv_init(SysBusDevice *dev)
 {
     A15MPPrivState *s = A15MPCORE_PRIV(dev);
+    DeviceState *gicdev;
     SysBusDevice *busdev;
-    const char *gictype = "arm_gic";
-
-    if (kvm_irqchip_in_kernel()) {
-        gictype = "kvm-arm-gic";
-    }
 
-    s->gic = qdev_create(NULL, gictype);
-    qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu);
-    qdev_prop_set_uint32(s->gic, "num-irq", s->num_irq);
-    qdev_prop_set_uint32(s->gic, "revision", 2);
-    qdev_init_nofail(s->gic);
-    busdev = SYS_BUS_DEVICE(s->gic);
+    gicdev = DEVICE(&s->gic);
+    qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
+    qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+    qdev_init_nofail(gicdev);
+    busdev = SYS_BUS_DEVICE(&s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
     sysbus_pass_irq(dev, busdev);
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 14/15] cpu/a15mpcore: Convert to QOM realize

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Turn SysBusDevice initfn into a QOM realizefn.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a15mpcore.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index ee09b57..7fd120f 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -67,8 +67,9 @@ static void a15mp_priv_initfn(Object *obj)
     qdev_prop_set_uint32(gicdev, "revision", 2);
 }
 
-static int a15mp_priv_init(SysBusDevice *dev)
+static void a15mp_priv_realize(DeviceState *dev, Error **errp)
 {
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     A15MPPrivState *s = A15MPCORE_PRIV(dev);
     DeviceState *gicdev;
     SysBusDevice *busdev;
@@ -80,10 +81,10 @@ static int a15mp_priv_init(SysBusDevice *dev)
     busdev = SYS_BUS_DEVICE(&s->gic);
 
     /* Pass through outbound IRQ lines from the GIC */
-    sysbus_pass_irq(dev, busdev);
+    sysbus_pass_irq(sbd, busdev);
 
     /* Pass through inbound GPIO lines to the GIC */
-    qdev_init_gpio_in(DEVICE(dev), a15mp_priv_set_irq, s->num_irq - 32);
+    qdev_init_gpio_in(dev, a15mp_priv_set_irq, s->num_irq - 32);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
      *  0x0000-0x0fff -- reserved
@@ -97,8 +98,6 @@ static int a15mp_priv_init(SysBusDevice *dev)
                                 sysbus_mmio_get_region(busdev, 0));
     memory_region_add_subregion(&s->container, 0x2000,
                                 sysbus_mmio_get_region(busdev, 1));
-
-    return 0;
 }
 
 static Property a15mp_priv_properties[] = {
@@ -116,8 +115,8 @@ static Property a15mp_priv_properties[] = {
 static void a15mp_priv_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
-    k->init = a15mp_priv_init;
+
+    dc->realize = a15mp_priv_realize;
     dc->props = a15mp_priv_properties;
     /* We currently have no savable state */
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH RFC 15/15] cpu/a15mpcore: Prepare for QOM embedding

[Andreas Färber]

From: Andreas Färber <andreas.faerber@web.de>

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/cpu/a15mpcore.c         | 21 +--------------------
 include/hw/cpu/a15mpcore.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 20 deletions(-)
 create mode 100644 include/hw/cpu/a15mpcore.h

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 7fd120f..1b275b1 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -18,27 +18,8 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "hw/sysbus.h"
+#include "hw/cpu/a15mpcore.h"
 #include "sysemu/kvm.h"
-#include "hw/intc/gic_internal.h"
-
-/* A15MP private memory region.  */
-
-#define TYPE_A15MPCORE_PRIV "a15mpcore_priv"
-#define A15MPCORE_PRIV(obj) \
-    OBJECT_CHECK(A15MPPrivState, (obj), TYPE_A15MPCORE_PRIV)
-
-typedef struct A15MPPrivState {
-    /*< private >*/
-    SysBusDevice parent_obj;
-    /*< public >*/
-
-    uint32_t num_cpu;
-    uint32_t num_irq;
-    MemoryRegion container;
-
-    GICState gic;
-} A15MPPrivState;
 
 static void a15mp_priv_set_irq(void *opaque, int irq, int level)
 {
diff --git a/include/hw/cpu/a15mpcore.h b/include/hw/cpu/a15mpcore.h
new file mode 100644
index 0000000..e9e9d52
--- /dev/null
+++ b/include/hw/cpu/a15mpcore.h
@@ -0,0 +1,44 @@
+/*
+ * Cortex-A15MPCore internal peripheral emulation.
+ *
+ * Copyright (c) 2012 Linaro Limited.
+ * Written by Peter Maydell.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef HW_CPU_A15MPCORE_H
+#define HW_CPU_A15MPCORE_H
+
+#include "hw/sysbus.h"
+#include "hw/intc/gic_internal.h"
+
+/* A15MP private memory region.  */
+
+#define TYPE_A15MPCORE_PRIV "a15mpcore_priv"
+#define A15MPCORE_PRIV(obj) \
+    OBJECT_CHECK(A15MPPrivState, (obj), TYPE_A15MPCORE_PRIV)
+
+typedef struct A15MPPrivState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
+    uint32_t num_cpu;
+    uint32_t num_irq;
+    MemoryRegion container;
+
+    GICState gic;
+} A15MPPrivState;
+
+#endif
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH RFC 03/15] cpu/a9mpcore: Embed GICState

[Peter Maydell]

On 30 June 2013 22:00, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> Prepares for conversion to QOM realize.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/cpu/a9mpcore.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index 63a4eb1..6340b0f 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -9,6 +9,7 @@
>   */
>
>  #include "hw/sysbus.h"
> +#include "hw/intc/gic_internal.h"

This doesn't look right -- the GIC internals are supposed
to be internal to the GIC implementation (and its subclasses).
They shouldn't be exposed to users. (That's why the header
is in hw/intc and not in include/.)

>  #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
>  #define A9MPCORE_PRIV(obj) \
> @@ -23,15 +24,17 @@ typedef struct A9MPPrivState {
>      MemoryRegion container;
>      DeviceState *mptimer;
>      DeviceState *wdt;
> -    DeviceState *gic;
>      DeviceState *scu;
>      uint32_t num_irq;
> +
> +    GICState gic;
>  } A9MPPrivState;

So we sort of had a discussion about this before, but I
still don't think we have a sensible way of doing
embedding of devices into container device structures
like this properly (ie without exposing implementation
internals that qdev doesn't require you to expose).

If we want to do struct-embedding then I think we should
come up with a standard way of writing that code (eg
a .h file under include/hw with type name macros and a
struct with only the public-facing bits and internal
members hidden behind a typedef somehow, and a .priv.h
in hw/whatever/ with the actual struct that the device
needs itself), and then start using that. Otherwise we
should just keep using pointers since they can happily
be opaque about the details of the struct they point to.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 01/15] cpu/a9mpcore: QOM casting cleanup

[Peter Crosthwaite]

On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> Introduce type constant and cast macro and enforce its use by
> renaming A9MPPrivState::busdev field to parent_obj.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/cpu/a9mpcore.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index 0a1a10f..36254e9 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -10,8 +10,15 @@
>
>  #include "hw/sysbus.h"
>
> +#define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
> +#define A9MPCORE_PRIV(obj) \
> +    OBJECT_CHECK(A9MPPrivState, (obj), TYPE_A9MPCORE_PRIV)
> +
>  typedef struct A9MPPrivState {
> -    SysBusDevice busdev;
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
>      uint32_t num_cpu;
>      MemoryRegion container;
>      DeviceState *mptimer;
> @@ -29,7 +36,7 @@ static void a9mp_priv_set_irq(void *opaque, int irq, int level)
>
>  static int a9mp_priv_init(SysBusDevice *dev)
>  {
> -    A9MPPrivState *s = FROM_SYSBUS(A9MPPrivState, dev);
> +    A9MPPrivState *s = A9MPCORE_PRIV(dev);
>      SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
>      int i;
>
> @@ -43,7 +50,7 @@ static int a9mp_priv_init(SysBusDevice *dev)
>      sysbus_pass_irq(dev, gicbusdev);
>
>      /* Pass through inbound GPIO lines to the GIC */
> -    qdev_init_gpio_in(&s->busdev.qdev, a9mp_priv_set_irq, s->num_irq - 32);
> +    qdev_init_gpio_in(DEVICE(dev), a9mp_priv_set_irq, s->num_irq - 32);
>
>      s->scu = qdev_create(NULL, "a9-scu");
>      qdev_prop_set_uint32(s->scu, "num-cpu", s->num_cpu);
> @@ -124,7 +131,7 @@ static void a9mp_priv_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo a9mp_priv_info = {
> -    .name          = "a9mpcore_priv",
> +    .name          = TYPE_A9MPCORE_PRIV,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(A9MPPrivState),
>      .class_init    = a9mp_priv_class_init,
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH RFC 04/15] misc/a9scu: QOM cleanups

[Peter Crosthwaite]

On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> Rename A9SCUState::busdev field to parent_obj and turn realizefn into an
> instance_init function to allow early MMIO mapping.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/misc/a9scu.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
> index 05897c2..30f6fac 100644
> --- a/hw/misc/a9scu.c
> +++ b/hw/misc/a9scu.c
> @@ -13,7 +13,10 @@
>  /* A9MP private memory region.  */
>
>  typedef struct A9SCUState {
> -    SysBusDevice busdev;
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
>      MemoryRegion iomem;
>      uint32_t control;
>      uint32_t status;
> @@ -114,10 +117,10 @@ static void a9_scu_reset(DeviceState *dev)
>      s->control = 0;
>  }
>
> -static void a9_scu_realize(DeviceState *dev, Error ** errp)
> +static void a9_scu_init(Object *obj)
>  {
> -    A9SCUState *s = A9_SCU(dev);
> -    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    A9SCUState *s = A9_SCU(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>
>      memory_region_init_io(&s->iomem, &a9_scu_ops, s, "a9-scu", 0x100);
>      sysbus_init_mmio(sbd, &s->iomem);
> @@ -143,7 +146,6 @@ static void a9_scu_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
> -    dc->realize = a9_scu_realize;
>      dc->props = a9_scu_properties;
>      dc->vmsd = &vmstate_a9_scu;
>      dc->reset = a9_scu_reset;
> @@ -153,6 +155,7 @@ static const TypeInfo a9_scu_info = {
>      .name          = TYPE_A9_SCU,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(A9SCUState),
> +    .instance_init = a9_scu_init,
>      .class_init    = a9_scu_class_init,
>  };
>
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH RFC 06/15] timer/arm_mptimer: QOM cast cleanup

[Peter Crosthwaite]

On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> Introduce type constant and cast macro and rename
> ARMMPTimerState::busdev to enforce its use.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/timer/arm_mptimer.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 317f5e4..588e34b 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -40,8 +40,15 @@ typedef struct {
>      MemoryRegion iomem;
>  } TimerBlock;
>
> +#define TYPE_ARM_MP_TIMER "arm_mptimer"

The _ in "MP_TIMER" seems inconsistent with the type string and
"A9MPCORE" as introduced in P1.

Regards,
Peter

> +#define ARM_MP_TIMER(obj) \
> +    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MP_TIMER)
> +
>  typedef struct {
> -    SysBusDevice busdev;
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +
>      uint32_t num_cpu;
>      TimerBlock timerblock[MAX_CPUS];
>      MemoryRegion iomem;
> @@ -211,9 +218,9 @@ static void timerblock_reset(TimerBlock *tb)
>
>  static void arm_mptimer_reset(DeviceState *dev)
>  {
> -    ARMMPTimerState *s =
> -        FROM_SYSBUS(ARMMPTimerState, SYS_BUS_DEVICE(dev));
> +    ARMMPTimerState *s = ARM_MP_TIMER(dev);
>      int i;
> +
>      for (i = 0; i < ARRAY_SIZE(s->timerblock); i++) {
>          timerblock_reset(&s->timerblock[i]);
>      }
> @@ -221,8 +228,9 @@ static void arm_mptimer_reset(DeviceState *dev)
>
>  static int arm_mptimer_init(SysBusDevice *dev)
>  {
> -    ARMMPTimerState *s = FROM_SYSBUS(ARMMPTimerState, dev);
> +    ARMMPTimerState *s = ARM_MP_TIMER(dev);
>      int i;
> +
>      if (s->num_cpu < 1 || s->num_cpu > MAX_CPUS) {
>          hw_error("%s: num-cpu must be between 1 and %d\n", __func__, MAX_CPUS);
>      }
> @@ -295,7 +303,7 @@ static void arm_mptimer_class_init(ObjectClass *klass, void *data)
>  }
>
>  static const TypeInfo arm_mptimer_info = {
> -    .name          = "arm_mptimer",
> +    .name          = TYPE_ARM_MP_TIMER,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(ARMMPTimerState),
>      .class_init    = arm_mptimer_class_init,
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH RFC 07/15] timer/arm_mptimer: Convert to QOM realize

[Peter Crosthwaite]

Hi Andreas,

On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
> From: Andreas Färber <andreas.faerber@web.de>
>
> Split the SysBusDevice initfn into instance_init and realizefn.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/timer/arm_mptimer.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
> index 588e34b..a19ffa3 100644
> --- a/hw/timer/arm_mptimer.c
> +++ b/hw/timer/arm_mptimer.c
> @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev)
>      }
>  }
>
> -static int arm_mptimer_init(SysBusDevice *dev)
> +static void arm_mptimer_init(Object *obj)
>  {
> +    ARMMPTimerState *s = ARM_MP_TIMER(obj);
> +
> +    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
> +                          "arm_mptimer_timer", 0x20);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +}

Splitting off only one of the memory region setups to init seems a bit
awkward. Is it really worth it given that we need to wait for
realization for the rest to occur anyway?

Regards,
Peter

> +
> +static void arm_mptimer_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      ARMMPTimerState *s = ARM_MP_TIMER(dev);
>      int i;
>
> @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev)
>       *  * timer for core 1
>       * and so on.
>       */
> -    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
> -                          "arm_mptimer_timer", 0x20);
> -    sysbus_init_mmio(dev, &s->iomem);
>      for (i = 0; i < s->num_cpu; i++) {
>          TimerBlock *tb = &s->timerblock[i];
>          tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb);
> -        sysbus_init_irq(dev, &tb->irq);
> +        sysbus_init_irq(sbd, &tb->irq);
>          memory_region_init_io(&tb->iomem, &timerblock_ops, tb,
>                                "arm_mptimer_timerblock", 0x20);
> -        sysbus_init_mmio(dev, &tb->iomem);
> +        sysbus_init_mmio(sbd, &tb->iomem);
>      }
> -
> -    return 0;
>  }
>
>  static const VMStateDescription vmstate_timerblock = {
> @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = {
>  static void arm_mptimer_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>
> -    sbc->init = arm_mptimer_init;
> +    dc->realize = arm_mptimer_realize;
>      dc->vmsd = &vmstate_arm_mptimer;
>      dc->reset = arm_mptimer_reset;
>      dc->no_user = 1;
> @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = {
>      .name          = TYPE_ARM_MP_TIMER,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(ARMMPTimerState),
> +    .instance_init = arm_mptimer_init,
>      .class_init    = arm_mptimer_class_init,
>  };
>
> --
> 1.8.1.4
>
>

Re: [Qemu-devel] [PATCH RFC 06/15] timer/arm_mptimer: QOM cast cleanup

[Andreas Färber]

Am 01.07.2013 11:29, schrieb Peter Crosthwaite:
> On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> Introduce type constant and cast macro and rename
>> ARMMPTimerState::busdev to enforce its use.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>  hw/timer/arm_mptimer.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
>> index 317f5e4..588e34b 100644
>> --- a/hw/timer/arm_mptimer.c
>> +++ b/hw/timer/arm_mptimer.c
>> @@ -40,8 +40,15 @@ typedef struct {
>>      MemoryRegion iomem;
>>  } TimerBlock;
>>
>> +#define TYPE_ARM_MP_TIMER "arm_mptimer"
> 
> The _ in "MP_TIMER" seems inconsistent with the type string and
> "A9MPCORE" as introduced in P1.

It is consistent with "PC_NET" though. ;) I can surely change it to
"MPTIMER" if preferred.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH RFC 07/15] timer/arm_mptimer: Convert to QOM realize

[Andreas Färber]

Am 01.07.2013 11:33, schrieb Peter Crosthwaite:
> Hi Andreas,
> 
> On Mon, Jul 1, 2013 at 7:00 AM, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> Split the SysBusDevice initfn into instance_init and realizefn.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>  hw/timer/arm_mptimer.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
>> index 588e34b..a19ffa3 100644
>> --- a/hw/timer/arm_mptimer.c
>> +++ b/hw/timer/arm_mptimer.c
>> @@ -226,8 +226,18 @@ static void arm_mptimer_reset(DeviceState *dev)
>>      }
>>  }
>>
>> -static int arm_mptimer_init(SysBusDevice *dev)
>> +static void arm_mptimer_init(Object *obj)
>>  {
>> +    ARMMPTimerState *s = ARM_MP_TIMER(obj);
>> +
>> +    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
>> +                          "arm_mptimer_timer", 0x20);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +}
> 
> Splitting off only one of the memory region setups to init seems a bit
> awkward. Is it really worth it given that we need to wait for
> realization for the rest to occur anyway?

Well, my main interest wrt MemoryRegions here is to have the
*mpcore_priv container MemoryRegion mappable before realize and to avoid
having to implement unnecessary cleanups in unrealize.

An alternative would be for PMM to use his array properties to
dynamically allocate the MemoryRegions before realize, but so far he has
failed to come up with a solution...
Another solution, since this is a fixed-length array, would be to always
initialize all MemoryRegions and just keep adding all 3(?) in realize.

FWIU adding subregions is desired in instance_init as long as it affects
only containing devices and not a global address space.

Regards,
Andreas

>> +
>> +static void arm_mptimer_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      ARMMPTimerState *s = ARM_MP_TIMER(dev);
>>      int i;
>>
>> @@ -244,19 +254,14 @@ static int arm_mptimer_init(SysBusDevice *dev)
>>       *  * timer for core 1
>>       * and so on.
>>       */
>> -    memory_region_init_io(&s->iomem, &arm_thistimer_ops, s,
>> -                          "arm_mptimer_timer", 0x20);
>> -    sysbus_init_mmio(dev, &s->iomem);
>>      for (i = 0; i < s->num_cpu; i++) {
>>          TimerBlock *tb = &s->timerblock[i];
>>          tb->timer = qemu_new_timer_ns(vm_clock, timerblock_tick, tb);
>> -        sysbus_init_irq(dev, &tb->irq);
>> +        sysbus_init_irq(sbd, &tb->irq);
>>          memory_region_init_io(&tb->iomem, &timerblock_ops, tb,
>>                                "arm_mptimer_timerblock", 0x20);
>> -        sysbus_init_mmio(dev, &tb->iomem);
>> +        sysbus_init_mmio(sbd, &tb->iomem);
>>      }
>> -
>> -    return 0;
>>  }
>>
>>  static const VMStateDescription vmstate_timerblock = {
>> @@ -293,9 +298,8 @@ static Property arm_mptimer_properties[] = {
>>  static void arm_mptimer_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> -    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
>>
>> -    sbc->init = arm_mptimer_init;
>> +    dc->realize = arm_mptimer_realize;
>>      dc->vmsd = &vmstate_arm_mptimer;
>>      dc->reset = arm_mptimer_reset;
>>      dc->no_user = 1;
>> @@ -306,6 +310,7 @@ static const TypeInfo arm_mptimer_info = {
>>      .name          = TYPE_ARM_MP_TIMER,
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>>      .instance_size = sizeof(ARMMPTimerState),
>> +    .instance_init = arm_mptimer_init,
>>      .class_init    = arm_mptimer_class_init,
>>  };
>>
>> --
>> 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

Re: [Qemu-devel] [PATCH RFC 03/15] cpu/a9mpcore: Embed GICState

[Andreas Färber]

Am 01.07.2013 00:13, schrieb Peter Maydell:
> On 30 June 2013 22:00, Andreas Färber <afaerber@suse.de> wrote:
>> From: Andreas Färber <andreas.faerber@web.de>
>>
>> Prepares for conversion to QOM realize.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>  hw/cpu/a9mpcore.c | 25 ++++++++++++++++---------
>>  1 file changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
>> index 63a4eb1..6340b0f 100644
>> --- a/hw/cpu/a9mpcore.c
>> +++ b/hw/cpu/a9mpcore.c
>> @@ -9,6 +9,7 @@
>>   */
>>
>>  #include "hw/sysbus.h"
>> +#include "hw/intc/gic_internal.h"
> 
> This doesn't look right -- the GIC internals are supposed
> to be internal to the GIC implementation (and its subclasses).
> They shouldn't be exposed to users. (That's why the header
> is in hw/intc and not in include/.)

I can easily add an include/hw/intc/arm_gic.h header to address that.
(Previously all device headers were in hw/.)

>>  #define TYPE_A9MPCORE_PRIV "a9mpcore_priv"
>>  #define A9MPCORE_PRIV(obj) \
>> @@ -23,15 +24,17 @@ typedef struct A9MPPrivState {
>>      MemoryRegion container;
>>      DeviceState *mptimer;
>>      DeviceState *wdt;
>> -    DeviceState *gic;
>>      DeviceState *scu;
>>      uint32_t num_irq;
>> +
>> +    GICState gic;
>>  } A9MPPrivState;
> 
> So we sort of had a discussion about this before, but I
> still don't think we have a sensible way of doing
> embedding of devices into container device structures
> like this properly (ie without exposing implementation
> internals that qdev doesn't require you to expose).

We didn't have a discussion, we had you complaining about how great qdev
was and how little you like QOM. Anthony told you quite clearly that
this is how it's supposed to be done with his QOM:

* in TypeInfo::instance_init initialize child devices with
object_initialize(), which is not supposed to fail,
* property setters can report failure via **errp and may therefore
dynamically allocate children - you implemented your array static
properties as a consequence,
* DeviceClass::realize shall not create new devices as they would be
hidden from management tools in the future QMP phase (-S) before
realization and would break recursive realization due to devices
appearing while iterating.

So when we have one or a fixed number of children, it seems clear to me
that they shall be embedded into the struct as done in this series.

You had several months to come up with a solution of your liking, but
apparently you didn't step forward with any patchset or proposal. ;)
I'd be more than happy if you refactored ARM code yourself, that would
mean less work for me and/or Tao. Saying no is not constructive though.

The only way I can think of to shield struct internals while still
reserving space to avoid dynamic allocation would be:

uint8_t gic[sizeof(GICState)];

which would still need access to the real GICState struct through a
header, while prohibiting access so s->gic.foo. This could be hidden in
a macro CHILD(typename, varname) to assure proper alignment.
CC'ing Anthony and Paolo.

But casting to the struct type is so simple to do that we can just use
the struct in the first place and let the compiler handle alignment for
us. As you can see from my Tegra2 patches, I am doing nothing bad with
a9mpcore_priv, not accessing its internals at all! And since you're
reviewing all ARM patches anyway, you could still reject any patches
accessing device internals.

Me being prompted by AArch64, we need an improvement over qdev code
*now* before its patterns get copied into further devices, creating even
more obstacles to implementing recursive realization.

> If we want to do struct-embedding then I think we should
> come up with a standard way of writing that code (eg
> a .h file under include/hw with type name macros and a
> struct with only the public-facing bits and internal
> members hidden behind a typedef somehow,

That is simply not possible in C. There are no public vs. internal
fields since everything can be accessed via pointer dereference anyway,
whatever way we use to declare pointers and fields.

By Anthony's QOM conventions, the /*< private >*/ gtk-doc marker hides
stuff from documentation completely (i.e., the parent_obj field not to
be accessed), whereas device fields should be documented for use within
the device, thus we need to keep them /*< public >*/ there.

Personally, I consider all device fields private and consider direct
access to random devices' fields as an abstraction breach and prefer to
use accessor functions for that (Data Caging). But I remember you
rejecting the accessor functions Anthony introduced for interacting with
QOM pins as "boilerplate", so this argument feels double-tongued.

> and a .priv.h
> in hw/whatever/ with the actual struct that the device
> needs itself), and then start using that. Otherwise we
> should just keep using pointers since they can happily
> be opaque about the details of the struct they point to.

Again, pointers in instance_init break the QOM allocation model.

The point is that g_malloc0(sizeof(MySoCState)) is expected to allocate
the size of the object including its children, whereas the current state
of things allocates the device only and allocates children as part of
qdev_create() in DeviceClass::init, which is too late for the QOM
realize model.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg