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

[Qemu-devel] [PATCH v2 00/16] 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.

v2 improves internal vs. "public" header separation for GIC.
As before, no feedback was received to address PMM's QOM concerns,
so this is what we have as design patterns for the moment.

Regards,
Andreas

v1 -> v2:
* Renamed MP_TIMER to MPTIMER (Peter C.).
* Don't include gic_internal.h, introduce new arm_gic.h header (PMM).
* a9mpcore: Init only container MemoryRegion in instance_init (Peter C.).
* a9mpcore/a15mpcore: Replaced all qdev_init_nofail()s for error propagation.

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Anthony Liguori <anthony@codemonkey.ws>
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 (16):
  cpu/a9mpcore: QOM casting cleanup
  cpu/a9mpcore: Split off instance_init
  intc/arm_gic: Extract public header hw/intc/arm_gic.h
  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             |  70 +++++++++++++-----------
 hw/cpu/a9mpcore.c              | 117 +++++++++++++++++++++++++----------------
 hw/intc/arm_gic_common.c       |  18 +++----
 hw/intc/gic_internal.h         |  77 +--------------------------
 hw/misc/a9scu.c                |  25 +++------
 hw/timer/arm_mptimer.c         |  62 +++++++++-------------
 include/hw/cpu/a15mpcore.h     |  44 ++++++++++++++++
 include/hw/cpu/a9mpcore.h      |  37 +++++++++++++
 include/hw/intc/arm_gic.h      | 108 +++++++++++++++++++++++++++++++++++++
 include/hw/misc/a9scu.h        |  31 +++++++++++
 include/hw/timer/arm_mptimer.h |  54 +++++++++++++++++++
 11 files changed, 425 insertions(+), 218 deletions(-)
 create mode 100644 include/hw/cpu/a15mpcore.h
 create mode 100644 include/hw/cpu/a9mpcore.h
 create mode 100644 include/hw/intc/arm_gic.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 v2 01/16] 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.

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
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 6c00a59..3e675e3 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 v2 02/16] 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 3e675e3..acbdab5 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, obj, "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, OBJECT(s), "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 v2 03/16] intc/arm_gic: Extract public header hw/intc/arm_gic.h

[Andreas Färber]

Rename NCPU to GIC_NCPU and move GICState away from gic_internal.h.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/intc/arm_gic_common.c  |  18 ++++----
 hw/intc/gic_internal.h    |  77 +--------------------------------
 include/hw/intc/arm_gic.h | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+), 84 deletions(-)
 create mode 100644 include/hw/intc/arm_gic.h

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 08560f2..a89c786 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -64,17 +64,17 @@ static const VMStateDescription vmstate_gic = {
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(enabled, GICState),
-        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, NCPU),
+        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
-        VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, NCPU),
+        VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
-        VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, NCPU),
-        VMSTATE_UINT16_ARRAY(priority_mask, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
-        VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
+        VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
+        VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -84,9 +84,9 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
     GICState *s = ARM_GIC_COMMON(dev);
     int num_irq = s->num_irq;
 
-    if (s->num_cpu > NCPU) {
+    if (s->num_cpu > GIC_NCPU) {
         error_setg(errp, "requested %u CPUs exceeds GIC maximum %d",
-                   s->num_cpu, NCPU);
+                   s->num_cpu, GIC_NCPU);
         return;
     }
     s->num_irq += GIC_BASE_IRQ;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 99a3bc3..3989fd1 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -21,16 +21,9 @@
 #ifndef QEMU_ARM_GIC_INTERNAL_H
 #define QEMU_ARM_GIC_INTERNAL_H
 
-#include "hw/sysbus.h"
+#include "hw/intc/arm_gic.h"
 
-/* Maximum number of possible interrupts, determined by the GIC architecture */
-#define GIC_MAXIRQ 1020
-/* First 32 are private to each CPU (SGIs and PPIs). */
-#define GIC_INTERNAL 32
-/* Maximum number of possible CPU interfaces, determined by GIC architecture */
-#define NCPU 8
-
-#define ALL_CPU_MASK ((unsigned)(((1 << NCPU) - 1)))
+#define ALL_CPU_MASK ((unsigned)(((1 << GIC_NCPU) - 1)))
 
 /* The NVIC has 16 internal vectors.  However these are not exposed
    through the normal GIC interface.  */
@@ -59,45 +52,6 @@
                                     s->priority2[(irq) - GIC_INTERNAL])
 #define GIC_TARGET(irq) s->irq_target[irq]
 
-typedef struct gic_irq_state {
-    /* The enable bits are only banked for per-cpu interrupts.  */
-    uint8_t enabled;
-    uint8_t pending;
-    uint8_t active;
-    uint8_t level;
-    bool model; /* 0 = N:N, 1 = 1:N */
-    bool trigger; /* nonzero = edge triggered.  */
-} gic_irq_state;
-
-typedef struct GICState {
-    SysBusDevice busdev;
-    qemu_irq parent_irq[NCPU];
-    bool enabled;
-    bool cpu_enabled[NCPU];
-
-    gic_irq_state irq_state[GIC_MAXIRQ];
-    uint8_t irq_target[GIC_MAXIRQ];
-    uint8_t priority1[GIC_INTERNAL][NCPU];
-    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
-    uint16_t last_active[GIC_MAXIRQ][NCPU];
-
-    uint16_t priority_mask[NCPU];
-    uint16_t running_irq[NCPU];
-    uint16_t running_priority[NCPU];
-    uint16_t current_pending[NCPU];
-
-    uint32_t num_cpu;
-
-    MemoryRegion iomem; /* Distributor */
-    /* This is just so we can have an opaque pointer which identifies
-     * both this GIC and which CPU interface we should be accessing.
-     */
-    struct GICState *backref[NCPU];
-    MemoryRegion cpuiomem[NCPU+1]; /* CPU interfaces */
-    uint32_t num_irq;
-    uint32_t revision;
-} GICState;
-
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
 #define REV_NVIC 0xffffffff
@@ -108,31 +62,4 @@ void gic_complete_irq(GICState *s, int cpu, int irq);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 
-#define TYPE_ARM_GIC_COMMON "arm_gic_common"
-#define ARM_GIC_COMMON(obj) \
-     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC_COMMON)
-#define ARM_GIC_COMMON_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ARMGICCommonClass, (klass), TYPE_ARM_GIC_COMMON)
-#define ARM_GIC_COMMON_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ARMGICCommonClass, (obj), TYPE_ARM_GIC_COMMON)
-
-typedef struct ARMGICCommonClass {
-    SysBusDeviceClass parent_class;
-    void (*pre_save)(GICState *s);
-    void (*post_load)(GICState *s);
-} ARMGICCommonClass;
-
-#define TYPE_ARM_GIC "arm_gic"
-#define ARM_GIC(obj) \
-     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
-#define ARM_GIC_CLASS(klass) \
-     OBJECT_CLASS_CHECK(ARMGICClass, (klass), TYPE_ARM_GIC)
-#define ARM_GIC_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(ARMGICClass, (obj), TYPE_ARM_GIC)
-
-typedef struct ARMGICClass {
-    ARMGICCommonClass parent_class;
-    DeviceRealize parent_realize;
-} ARMGICClass;
-
 #endif /* !QEMU_ARM_GIC_INTERNAL_H */
diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
new file mode 100644
index 0000000..be945ec
--- /dev/null
+++ b/include/hw/intc/arm_gic.h
@@ -0,0 +1,108 @@
+/*
+ * ARM GIC support
+ *
+ * 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_ARM_GIC_H
+#define HW_ARM_GIC_H
+
+#include "hw/sysbus.h"
+
+/* Maximum number of possible interrupts, determined by the GIC architecture */
+#define GIC_MAXIRQ 1020
+/* First 32 are private to each CPU (SGIs and PPIs). */
+#define GIC_INTERNAL 32
+/* Maximum number of possible CPU interfaces, determined by GIC architecture */
+#define GIC_NCPU 8
+
+typedef struct gic_irq_state {
+    /* The enable bits are only banked for per-cpu interrupts.  */
+    uint8_t enabled;
+    uint8_t pending;
+    uint8_t active;
+    uint8_t level;
+    bool model; /* 0 = N:N, 1 = 1:N */
+    bool trigger; /* nonzero = edge triggered.  */
+} gic_irq_state;
+
+typedef struct GICState {
+    /*< private >*/
+    SysBusDevice busdev;
+    /*< public >*/
+
+    qemu_irq parent_irq[GIC_NCPU];
+    bool enabled;
+    bool cpu_enabled[GIC_NCPU];
+
+    gic_irq_state irq_state[GIC_MAXIRQ];
+    uint8_t irq_target[GIC_MAXIRQ];
+    uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
+    uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
+    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
+
+    uint16_t priority_mask[GIC_NCPU];
+    uint16_t running_irq[GIC_NCPU];
+    uint16_t running_priority[GIC_NCPU];
+    uint16_t current_pending[GIC_NCPU];
+
+    uint32_t num_cpu;
+
+    MemoryRegion iomem; /* Distributor */
+    /* This is just so we can have an opaque pointer which identifies
+     * both this GIC and which CPU interface we should be accessing.
+     */
+    struct GICState *backref[GIC_NCPU];
+    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
+    uint32_t num_irq;
+    uint32_t revision;
+} GICState;
+
+#define TYPE_ARM_GIC_COMMON "arm_gic_common"
+#define ARM_GIC_COMMON(obj) \
+     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC_COMMON)
+#define ARM_GIC_COMMON_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ARMGICCommonClass, (klass), TYPE_ARM_GIC_COMMON)
+#define ARM_GIC_COMMON_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ARMGICCommonClass, (obj), TYPE_ARM_GIC_COMMON)
+
+typedef struct ARMGICCommonClass {
+    /*< private >*/
+    SysBusDeviceClass parent_class;
+    /*< public >*/
+
+    void (*pre_save)(GICState *s);
+    void (*post_load)(GICState *s);
+} ARMGICCommonClass;
+
+#define TYPE_ARM_GIC "arm_gic"
+#define ARM_GIC(obj) \
+     OBJECT_CHECK(GICState, (obj), TYPE_ARM_GIC)
+#define ARM_GIC_CLASS(klass) \
+     OBJECT_CLASS_CHECK(ARMGICClass, (klass), TYPE_ARM_GIC)
+#define ARM_GIC_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(ARMGICClass, (obj), TYPE_ARM_GIC)
+
+typedef struct ARMGICClass {
+    /*< private >*/
+    ARMGICCommonClass parent_class;
+    /*< public >*/
+
+    DeviceRealize parent_realize;
+} ARMGICClass;
+
+#endif
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 04/16] 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 acbdab5..d157387 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -9,6 +9,7 @@
  */
 
 #include "hw/sysbus.h"
+#include "hw/intc/arm_gic.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, obj, "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 v2 05/16] 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.

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/misc/a9scu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 601b573..2661014 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,12 +117,12 @@ 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, OBJECT(dev), &a9_scu_ops, s,
+    memory_region_init_io(&s->iomem, obj, &a9_scu_ops, s,
                           "a9-scu", 0x100);
     sysbus_init_mmio(sbd, &s->iomem);
 }
@@ -144,7 +147,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;
@@ -154,6 +156,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 v2 06/16] 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       | 16 ++++++++++------
 hw/misc/a9scu.c         | 18 +-----------------
 include/hw/misc/a9scu.h | 31 +++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 23 deletions(-)
 create mode 100644 include/hw/misc/a9scu.h

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index d157387..eba2de1 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -10,6 +10,7 @@
 
 #include "hw/sysbus.h"
 #include "hw/intc/arm_gic.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)
@@ -46,12 +47,15 @@ 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());
 }
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *gicdev;
+    DeviceState *gicdev, *scudev;
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
     int i;
 
@@ -67,10 +71,10 @@ 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);
+    scubusdev = SYS_BUS_DEVICE(&s->scu);
 
     s->mptimer = qdev_create(NULL, "arm_mptimer");
     qdev_prop_set_uint32(s->mptimer, "num-cpu", s->num_cpu);
diff --git a/hw/misc/a9scu.c b/hw/misc/a9scu.c
index 2661014..4434945 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 v2 07/16] 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 0ceb240..9277315 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -41,8 +41,15 @@ typedef struct {
     MemoryRegion iomem;
 } TimerBlock;
 
+#define TYPE_ARM_MPTIMER "arm_mptimer"
+#define ARM_MPTIMER(obj) \
+    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MPTIMER)
+
 typedef struct {
-    SysBusDevice busdev;
+    /*< private >*/
+    SysBusDevice parent_obj;
+    /*< public >*/
+
     uint32_t num_cpu;
     TimerBlock timerblock[MAX_CPUS];
     MemoryRegion iomem;
@@ -210,9 +217,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_MPTIMER(dev);
     int i;
+
     for (i = 0; i < ARRAY_SIZE(s->timerblock); i++) {
         timerblock_reset(&s->timerblock[i]);
     }
@@ -220,8 +227,9 @@ static void arm_mptimer_reset(DeviceState *dev)
 
 static int arm_mptimer_init(SysBusDevice *dev)
 {
-    ARMMPTimerState *s = FROM_SYSBUS(ARMMPTimerState, dev);
+    ARMMPTimerState *s = ARM_MPTIMER(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);
     }
@@ -294,7 +302,7 @@ static void arm_mptimer_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo arm_mptimer_info = {
-    .name          = "arm_mptimer",
+    .name          = TYPE_ARM_MPTIMER,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(ARMMPTimerState),
     .class_init    = arm_mptimer_class_init,
-- 
1.8.1.4

[Qemu-devel] [PATCH v2 08/16] 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 9277315..4986bb7 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -225,8 +225,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_MPTIMER(obj);
+
+    memory_region_init_io(&s->iomem, obj, &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_MPTIMER(dev);
     int i;
 
@@ -243,19 +253,14 @@ static int arm_mptimer_init(SysBusDevice *dev)
      *  * timer for core 1
      * and so on.
      */
-    memory_region_init_io(&s->iomem, OBJECT(s), &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, OBJECT(s), &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 = {
@@ -292,9 +297,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;
@@ -305,6 +309,7 @@ static const TypeInfo arm_mptimer_info = {
     .name          = TYPE_ARM_MPTIMER,
     .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 v2 09/16] 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              | 29 ++++++++++++++---------
 hw/timer/arm_mptimer.c         | 35 ++++-----------------------
 include/hw/timer/arm_mptimer.h | 54 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 42 deletions(-)
 create mode 100644 include/hw/timer/arm_mptimer.h

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index eba2de1..1dc1e92 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -11,6 +11,7 @@
 #include "hw/sysbus.h"
 #include "hw/intc/arm_gic.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)
@@ -50,12 +51,18 @@ static void a9mp_priv_initfn(Object *obj)
 
     object_initialize(&s->scu, TYPE_A9_SCU);
     qdev_set_parent_bus(DEVICE(&s->scu), sysbus_get_default());
+
+    object_initialize(&s->mptimer, TYPE_ARM_MPTIMER);
+    qdev_set_parent_bus(DEVICE(&s->mptimer), sysbus_get_default());
+
+    object_initialize(&s->wdt, TYPE_ARM_MPTIMER);
+    qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
 }
 
 static int a9mp_priv_init(SysBusDevice *dev)
 {
     A9MPPrivState *s = A9MPCORE_PRIV(dev);
-    DeviceState *gicdev, *scudev;
+    DeviceState *gicdev, *scudev, *mptimerdev, *wdtdev;
     SysBusDevice *timerbusdev, *wdtbusdev, *gicbusdev, *scubusdev;
     int i;
 
@@ -76,15 +83,15 @@ static int a9mp_priv_init(SysBusDevice *dev)
     qdev_init_nofail(scudev);
     scubusdev = SYS_BUS_DEVICE(&s->scu);
 
-    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
diff --git a/hw/timer/arm_mptimer.c b/hw/timer/arm_mptimer.c
index 4986bb7..5b46361 100644
--- a/hw/timer/arm_mptimer.c
+++ b/hw/timer/arm_mptimer.c
@@ -19,7 +19,7 @@
  * 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"
 #include "qom/cpu.h"
 
@@ -27,34 +27,6 @@
  * 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_MPTIMER "arm_mptimer"
-#define ARM_MPTIMER(obj) \
-    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MPTIMER)
-
-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)
 {
     if (current_cpu->cpu_index >= s->num_cpu) {
@@ -240,8 +212,9 @@ static void arm_mptimer_realize(DeviceState *dev, Error **errp)
     ARMMPTimerState *s = ARM_MPTIMER(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..b34cba0
--- /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_MPTIMER "arm_mptimer"
+#define ARM_MPTIMER(obj) \
+    OBJECT_CHECK(ARMMPTimerState, (obj), TYPE_ARM_MPTIMER)
+
+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 v2 10/16] 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 | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 1dc1e92..5eb1a2f 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -59,38 +59,56 @@ static void a9mp_priv_initfn(Object *obj)
     qdev_set_parent_bus(DEVICE(&s->wdt), sysbus_get_default());
 }
 
-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, *scubusdev;
+    Error *err = NULL;
     int i;
 
     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);
+    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
     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);
-    qdev_init_nofail(scudev);
+    object_property_set_bool(OBJECT(&s->scu), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
     scubusdev = SYS_BUS_DEVICE(&s->scu);
 
     mptimerdev = DEVICE(&s->mptimer);
     qdev_prop_set_uint32(mptimerdev, "num-cpu", s->num_cpu);
-    qdev_init_nofail(mptimerdev);
+    object_property_set_bool(OBJECT(&s->mptimer), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
     timerbusdev = SYS_BUS_DEVICE(&s->mptimer);
 
     wdtdev = DEVICE(&s->wdt);
     qdev_prop_set_uint32(wdtdev, "num-cpu", s->num_cpu);
-    qdev_init_nofail(wdtdev);
+    object_property_set_bool(OBJECT(&s->wdt), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
     wdtbusdev = SYS_BUS_DEVICE(&s->wdt);
 
     /* Memory map (addresses are offsets from PERIPHBASE):
@@ -129,7 +147,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[] = {
@@ -147,9 +164,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 v2 11/16] 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 5eb1a2f..d295bfa 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/arm_gic.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..010489b
--- /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/arm_gic.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 v2 12/16] 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 c736257..4f37964 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
@@ -101,7 +108,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 v2 13/16] 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 | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 4f37964..c261cea 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, obj, "a15mp-priv-container", 0x8000);
+    sysbus_init_mmio(sbd, &s->container);
+}
+
 static int a15mp_priv_init(SysBusDevice *dev)
 {
     A15MPPrivState *s = A15MPCORE_PRIV(dev);
@@ -75,14 +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, OBJECT(s),
-                       "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;
 }
 
@@ -111,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 v2 14/16] 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 c261cea..3f8b07b 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/arm_gic.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, obj, "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 v2 15/16] 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 | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index 3f8b07b..ea6568c 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -67,23 +67,29 @@ 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;
+    Error *err = NULL;
 
     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);
+    object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
+    }
     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 +103,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 +120,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 v2 16/16] 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 ea6568c..ec97de0 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/arm_gic.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..b423533
--- /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/arm_gic.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 v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Peter Maydell]

On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
> v2 improves internal vs. "public" header separation for GIC.
> As before, no feedback was received to address PMM's QOM concerns,
> so this is what we have as design patterns for the moment.

I had a thought about this. Suppose we have our class header
files do something like this:

#ifdef MYCLASS_IMPLEMENTATION
#define PRIVATE
#else
#ifdef __GNUC__
#define PRIVATE __attribute__((deprecated("this is a private field")))
#else
#define PRIVATE
#endif

typedef struct MyObject {
   int publicfield;
   int privatefield PRIVATE;
} MyObject;

Then we can allow both users of the class and the implementation
to share the same header file (obviously only the implementation
defines MYCLASS_IMPLEMENTATION before using it). The users can
embed the struct MyObject with no problems, but if they try
to directly access a private field this happens:

/tmp/zz9.c:22:5: warning: ‘privatefield’ is deprecated (declared at
/tmp/zz9.c:12): this is a private field [-Wdeprecated-declarations]

(Since this is only a safety-guard against accidental uses,
it's OK that it's only present when building with gcc.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Peter Maydell]

On 23 July 2013 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
>> v2 improves internal vs. "public" header separation for GIC.
>> As before, no feedback was received to address PMM's QOM concerns,
>> so this is what we have as design patterns for the moment.
>
> I had a thought about this. Suppose we have our class header
> files do something like this:
>
> #ifdef MYCLASS_IMPLEMENTATION
> #define PRIVATE
> #else
> #ifdef __GNUC__
> #define PRIVATE __attribute__((deprecated("this is a private field")))
> #else
> #define PRIVATE
> #endif
>
> typedef struct MyObject {
>    int publicfield;
>    int privatefield PRIVATE;
> } MyObject;

Forgot to say, but if people don't think this is an
intrinsically terrible idea I'll put together a patch that
does this sometime this week.

-- PMM

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Alexander Graf]

Am 23.07.2013 um 23:16 schrieb Peter Maydell <peter.maydell@linaro.org>:

> On 23 July 2013 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
>>> v2 improves internal vs. "public" header separation for GIC.
>>> As before, no feedback was received to address PMM's QOM concerns,
>>> so this is what we have as design patterns for the moment.
>> 
>> I had a thought about this. Suppose we have our class header
>> files do something like this:
>> 
>> #ifdef MYCLASS_IMPLEMENTATION
>> #define PRIVATE
>> #else
>> #ifdef __GNUC__
>> #define PRIVATE __attribute__((deprecated("this is a private field")))
>> #else
>> #define PRIVATE
>> #endif
>> 
>> typedef struct MyObject {
>>   int publicfield;
>>   int privatefield PRIVATE;
>> } MyObject;
> 
> Forgot to say, but if people don't think this is an
> intrinsically terrible idea I'll put together a patch that
> does this sometime this week.

I like the idea, but could we make this slightly less upper case? Something like

  __private int privatefield;

feels more readable imho. Or maybe

struct MyObject {
  PUBLIC_FIELDS
  __field int publicfield;
  PRIVATE_FIELDS
  __field int privatefield;
}

We already have comments indicating the sections, so replacing them by valid macros feels sensible.
  

Alex

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Peter Maydell]

On 23 July 2013 22:36, Alexander Graf <agraf@suse.de> wrote:
>
>
> Am 23.07.2013 um 23:16 schrieb Peter Maydell <peter.maydell@linaro.org>:
>
>> On 23 July 2013 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I had a thought about this. Suppose we have our class header
>>> files do something like this:
>>>
>>> #ifdef MYCLASS_IMPLEMENTATION
>>> #define PRIVATE
>>> #else
>>> #ifdef __GNUC__
>>> #define PRIVATE __attribute__((deprecated("this is a private field")))
>>> #else
>>> #define PRIVATE
>>> #endif
>>>
>>> typedef struct MyObject {
>>>   int publicfield;
>>>   int privatefield PRIVATE;
>>> } MyObject;
>>
>> Forgot to say, but if people don't think this is an
>> intrinsically terrible idea I'll put together a patch that
>> does this sometime this week.
>
> I like the idea, but could we make this slightly less upper case? Something like
>
>   __private int privatefield;
>
> feels more readable imho.

Well, __ is using the reserved namespace, but we could use something
else, and it looks like gcc lets us put the attribute at the front.
Since we'll want to undef whatever we pick after the struct is defined
we can actually use pretty much anything without worrying about it
stealing namespace.
(We could even use just 'private' if we didn't mind (a) not being
able to compile with a C++ compiler and (b) confusing everybody
completely :-))

> Or maybe
>
> struct MyObject {
>   PUBLIC_FIELDS
>   __field int publicfield;
>   PRIVATE_FIELDS
>   __field int privatefield;
> }

I can't see an obvious way to make those do the right
thing with the C preprocessor... am I missing something?

-- PMM

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Alexander Graf]

On 23.07.2013, at 23:52, Peter Maydell wrote:

> On 23 July 2013 22:36, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>> Am 23.07.2013 um 23:16 schrieb Peter Maydell <peter.maydell@linaro.org>:
>> 
>>> On 23 July 2013 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> I had a thought about this. Suppose we have our class header
>>>> files do something like this:
>>>> 
>>>> #ifdef MYCLASS_IMPLEMENTATION
>>>> #define PRIVATE
>>>> #else
>>>> #ifdef __GNUC__
>>>> #define PRIVATE __attribute__((deprecated("this is a private field")))
>>>> #else
>>>> #define PRIVATE
>>>> #endif
>>>> 
>>>> typedef struct MyObject {
>>>>  int publicfield;
>>>>  int privatefield PRIVATE;
>>>> } MyObject;
>>> 
>>> Forgot to say, but if people don't think this is an
>>> intrinsically terrible idea I'll put together a patch that
>>> does this sometime this week.
>> 
>> I like the idea, but could we make this slightly less upper case? Something like
>> 
>>  __private int privatefield;
>> 
>> feels more readable imho.
> 
> Well, __ is using the reserved namespace, but we could use something
> else, and it looks like gcc lets us put the attribute at the front.
> Since we'll want to undef whatever we pick after the struct is defined
> we can actually use pretty much anything without worrying about it
> stealing namespace.

Very nice :). And it aligns pretty well with the __user hint in Linux.

> (We could even use just 'private' if we didn't mind (a) not being
> able to compile with a C++ compiler and (b) confusing everybody
> completely :-))
> 
>> Or maybe
>> 
>> struct MyObject {
>>  PUBLIC_FIELDS
>>  __field int publicfield;
>>  PRIVATE_FIELDS
>>  __field int privatefield;
>> }
> 
> I can't see an obvious way to make those do the right
> thing with the C preprocessor... am I missing something?

No, I'm probably just daydreaming :). Macros can't redefine other defines, so this probably won't work....

So yes, prepending the visibility on every field seems to be the most straight forward choice.


Alex

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Andreas Färber]

Am 23.07.2013 23:52, schrieb Peter Maydell:
> On 23 July 2013 22:36, Alexander Graf <agraf@suse.de> wrote:
>> Am 23.07.2013 um 23:16 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>> On 23 July 2013 20:15, Peter Maydell <peter.maydell@linaro.org> wrote:
[C needing access to full struct definition for composite use]
>>>> I had a thought about this. Suppose we have our class header
>>>> files do something like this:
>>>>
>>>> #ifdef MYCLASS_IMPLEMENTATION
>>>> #define PRIVATE
>>>> #else
>>>> #ifdef __GNUC__
>>>> #define PRIVATE __attribute__((deprecated("this is a private field")))
>>>> #else
>>>> #define PRIVATE
>>>> #endif
>>>>
>>>> typedef struct MyObject {
>>>>   int publicfield;
>>>>   int privatefield PRIVATE;
>>>> } MyObject;
>>>
>>> Forgot to say, but if people don't think this is an
>>> intrinsically terrible idea I'll put together a patch that
>>> does this sometime this week.
>>
>> I like the idea, but could we make this slightly less upper case? Something like
>>
>>   __private int privatefield;
>>
>> feels more readable imho.
> 
> Well, __ is using the reserved namespace, but we could use something
> else, and it looks like gcc lets us put the attribute at the front.
> Since we'll want to undef whatever we pick after the struct is defined
> we can actually use pretty much anything without worrying about it
> stealing namespace.
> (We could even use just 'private' if we didn't mind (a) not being
> able to compile with a C++ compiler

We still have many occurrences of "klass" around as proof that we try to
be C++ compatible. ;)

> and (b) confusing everybody
> completely :-))

I wouldn't generally mind annotating fields. But let's ask Michael about
this - QIDL tried to annotate fields in a similar way for migration status.

>> Or maybe
>>
>> struct MyObject {
>>   PUBLIC_FIELDS
>>   __field int publicfield;
>>   PRIVATE_FIELDS
>>   __field int privatefield;
>> }
> 
> I can't see an obvious way to make those do the right
> thing with the C preprocessor... am I missing something?

gtk-doc wouldn't understand that, and I can't think of a way that
PRIVATE_FIELDS could redefine __field (or field) to do the right thing.

Anyway, before we get lost in a bikeshed discussion, if the
underscore'ization of the type names is to everyone's liking now, I
would very much like to queue the QOM cast patches on qom-next
(independent of whether you apply parts of the series to arm-devs.next).

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 v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Peter Maydell]

On 23 July 2013 23:08, Andreas Färber <afaerber@suse.de> wrote:
> Anyway, before we get lost in a bikeshed discussion, if the
> underscore'ization of the type names is to everyone's liking now, I
> would very much like to queue the QOM cast patches on qom-next

Which particular patches (whole series?) ? I'll review them,
since I'm now happy that we can add the annotations later.

> (independent of whether you apply parts of the series to arm-devs.next).

I don't at this point plan to apply anything to arm-devs.next
except bugfixes before 1.6 is released.

-- PMM

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Andreas Färber]

Am 23.07.2013 23:55, schrieb Alexander Graf:
> On 23.07.2013, at 23:52, Peter Maydell wrote:
>> On 23 July 2013 22:36, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> Or maybe
>>>
>>> struct MyObject {
>>>  PUBLIC_FIELDS
>>>  __field int publicfield;
>>>  PRIVATE_FIELDS
>>>  __field int privatefield;
>>> }
>>
>> I can't see an obvious way to make those do the right
>> thing with the C preprocessor... am I missing something?
> 
> No, I'm probably just daydreaming :). Macros can't redefine other defines, so this probably won't work....
> 
> So yes, prepending the visibility on every field seems to be the most straight forward choice.

I wonder how many public fields do we actually have? Close to zero?
Might there be a way to mark all fields of a struct as private at struct
level, except for those explicitly marked up as public?

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 v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Andreas Färber]

Am 24.07.2013 00:10, schrieb Peter Maydell:
> On 23 July 2013 23:08, Andreas Färber <afaerber@suse.de> wrote:
>> Anyway, before we get lost in a bikeshed discussion, if the
>> underscore'ization of the type names is to everyone's liking now, I
>> would very much like to queue the QOM cast patches on qom-next
> 
> Which particular patches (whole series?) ?

Patches 01, 07 (changed in v2), 12. That's 4x FROM_SYSBUS(). :)

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

Re: [Qemu-devel] [PATCH v2 00/16] arm: A9MPCore+A15MPCore QOM'ification

[Peter Maydell]

On 23 July 2013 23:12, Andreas Färber <afaerber@suse.de> wrote:
> I wonder how many public fields do we actually have? Close to zero?
> Might there be a way to mark all fields of a struct as private at struct
> level, except for those explicitly marked up as public?

No, I don't think this is possible -- there is no
"not-actually-deprecated" attribute.

IIRC Anthony wanted to be able to do direct field
access for properties (esp. links), which would
then be examples of public fields.

-- PMM

Re: [Qemu-devel] [PATCH v2 01/16] cpu/a9mpcore: QOM casting cleanup

[Peter Maydell]

On 23 July 2013 03:43, 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.
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v2 12/16] cpu/a15mpcore: QOM cast cleanup

[Peter Maydell]

On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
> 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v2 07/16] timer/arm_mptimer: QOM cast cleanup

[Peter Maydell]

On 23 July 2013 03:43, 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v2 03/16] intc/arm_gic: Extract public header hw/intc/arm_gic.h

[Peter Maydell]

On 23 July 2013 03:43, Andreas Färber <afaerber@suse.de> wrote:
> +typedef struct GICState {
> +    /*< private >*/
> +    SysBusDevice busdev;
> +    /*< public >*/
> +
> +    qemu_irq parent_irq[GIC_NCPU];
> +    bool enabled;
> +    bool cpu_enabled[GIC_NCPU];
> +
[etc]

Am I missing some reason why all these fields should be
in the section marked '< public >' ? I would have expected
that they would all be considered private, because only
the GIC implementation (and its subclasses) should be
touching them.

> +    MemoryRegion iomem; /* Distributor */
> +    /* This is just so we can have an opaque pointer which identifies
> +     * both this GIC and which CPU interface we should be accessing.
> +     */
> +    struct GICState *backref[GIC_NCPU];
> +    MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
> +    uint32_t num_irq;
> +    uint32_t revision;

...when we get down to here, should properties be
considered 'private' (ie "don't touch except via the
property APIs") or are they 'public' ?

> +} GICState;

(I noticed this because I thought I'd use these
patches as my demonstrator for the __private macro
I suggested in the other thread.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 03/16] intc/arm_gic: Extract public header hw/intc/arm_gic.h

[Andreas Färber]

Am 23.07.2013 04:43, schrieb Andreas Färber:
> diff --git a/include/hw/intc/arm_gic.h b/include/hw/intc/arm_gic.h
> new file mode 100644
> index 0000000..be945ec
> --- /dev/null
> +++ b/include/hw/intc/arm_gic.h
[...]
> +typedef struct GICState {
> +    /*< private >*/
> +    SysBusDevice busdev;

This missed to rename to parent_obj - therefore there's an occurrence
this patch missed. Addressed in the hw/arm/ FROM_SYSBUS() series.

Andreas

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