[Qemu-devel] [PULL 0/2] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Qemu-devel] [PULL 0/2] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Peter A. G. Crosthwaite]

The following changes since commit 3d1d9652978ac5a32a0beb4bdf6065ca39440d89:
  Bruce Rogers (1):
        handle device help before accelerator set up

are available in the git repository at:

  git://developer.petalogix.com/public/qemu.git for-upstream/axi-stream.next

Anthony Liguori (1):
      qom: Reimplement Interfaces

Peter A. G. Crosthwaite (1):
      xilinx_axi*: Re-implemented interconnect

 hw/Makefile.objs         |    1 +
 hw/petalogix_ml605_mmu.c |   24 +++--
 hw/stream.c              |   23 +++++
 hw/stream.h              |   31 +++++++
 hw/xilinx.h              |   22 ++---
 hw/xilinx_axidma.c       |   74 +++++++++-------
 hw/xilinx_axidma.h       |   39 --------
 hw/xilinx_axienet.c      |   32 ++++---
 include/qemu/object.h    |   46 ++++++----
 qom/object.c             |  220 ++++++++++++++++++----------------------------
 10 files changed, 255 insertions(+), 257 deletions(-)
 create mode 100644 hw/stream.c
 create mode 100644 hw/stream.h
 delete mode 100644 hw/xilinx_axidma.h

[Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Peter A. G. Crosthwaite]

From: Anthony Liguori <aliguori@us.ibm.com>

The current implementation of Interfaces is poorly designed.  Each interface
that an object implements ends up being an object that's tracked by the
implementing object.  There's all sorts of gymnastics to deal with casting
between these objects.

But an interface shouldn't be associated with an Object.  Interfaces are global
to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
the relationship between Object and Interfaces.

Interfaces are now abstract (as they should be) but this is okay.  Interfaces
essentially act as additional parents for the classes and are treated as such.

With this new implementation, we should fully support derived interfaces
including reimplementing an inherited interface.

PC: Rebased against qom-next merge Jun-2012.

PC: Removed replication of cast logic for interfaces, i.e. there is only
one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |   46 +++++++----
 qom/object.c          |  220 +++++++++++++++++++------------------------------
 2 files changed, 116 insertions(+), 150 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8b17776..cc75fee 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,7 @@ struct ObjectClass
 {
     /*< private >*/
     Type type;
+    GSList *interfaces;
 };
 
 /**
@@ -260,7 +261,6 @@ struct Object
 {
     /*< private >*/
     ObjectClass *class;
-    GSList *interfaces;
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
@@ -387,6 +387,16 @@ struct TypeInfo
     OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
 
 /**
+ * InterfaceInfo:
+ * @type: The name of the interface.
+ *
+ * The information associated with an interface.
+ */
+struct InterfaceInfo {
+    const char *type;
+};
+
+/**
  * InterfaceClass:
  * @parent_class: the base class
  *
@@ -396,26 +406,30 @@ struct TypeInfo
 struct InterfaceClass
 {
     ObjectClass parent_class;
+    /*< private >*/
+    ObjectClass *concrete_class;
 };
 
+#define TYPE_INTERFACE "interface"
+
 /**
- * InterfaceInfo:
- * @type: The name of the interface.
- * @interface_initfn: This method is called during class initialization and is
- *   used to initialize an interface associated with a class.  This function
- *   should initialize any default virtual functions for a class and/or override
- *   virtual functions in a parent class.
- *
- * The information associated with an interface.
+ * INTERFACE_CLASS:
+ * @klass: class to cast from
+ * Returns: An #InterfaceClass or raise an error if cast is invalid
  */
-struct InterfaceInfo
-{
-    const char *type;
+#define INTERFACE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
 
-    void (*interface_initfn)(ObjectClass *class, void *data);
-};
-
-#define TYPE_INTERFACE "interface"
+/**
+ * INTERFACE_CHECK:
+ * @interface: the type to return
+ * @obj: the object to convert to an interface
+ * @name: the interface type name
+ *
+ * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
+ */
+#define INTERFACE_CHECK(interface, obj, name) \
+    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
 
 /**
  * object_new:
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..a552be2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
 
 struct InterfaceImpl
 {
-    const char *parent;
-    void (*interface_initfn)(ObjectClass *class, void *data);
-    TypeImpl *type;
+    const char *typename;
 };
 
 struct TypeImpl
@@ -64,14 +62,6 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-typedef struct Interface
-{
-    Object parent;
-    Object *obj;
-} Interface;
-
-#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
-
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -98,6 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
 static TypeImpl *type_register_internal(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
+    int i;
 
     g_assert(info->name != NULL);
 
@@ -122,15 +113,10 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
 
     ti->abstract = info->abstract;
 
-    if (info->interfaces) {
-        int i;
-
-        for (i = 0; info->interfaces[i].type; i++) {
-            ti->interfaces[i].parent = info->interfaces[i].type;
-            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
-            ti->num_interfaces++;
-        }
+    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
+        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
     }
+    ti->num_interfaces = i;
 
     type_table_add(ti);
 
@@ -198,26 +184,48 @@ static size_t type_object_get_size(TypeImpl *ti)
     return 0;
 }
 
-static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
+static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
 {
-    TypeInfo info = {
-        .instance_size = sizeof(Interface),
-        .parent = iface->parent,
-        .class_size = sizeof(InterfaceClass),
-        .class_init = iface->interface_initfn,
-        .abstract = true,
-    };
-    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
+    assert(target_type);
+
+    /* Check if typename is a direct ancestor of type */
+    while (type) {
+        if (type == target_type) {
+            return true;
+        }
 
-    info.name = name;
-    iface->type = type_register_internal(&info);
-    g_free(name);
+        type = type_get_parent(type);
+    }
+
+    return false;
+}
+
+static void type_initialize(TypeImpl *ti);
+
+static void type_initialize_interface(TypeImpl *ti, const char *parent)
+{
+    InterfaceClass *new_iface;
+    TypeInfo info = { };
+    TypeImpl *iface_impl;
+
+    info.parent = parent;
+    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
+    info.abstract = true;
+
+    iface_impl = type_register(&info);
+    type_initialize(iface_impl);
+    g_free((char *)info.name);
+
+    new_iface = (InterfaceClass *)iface_impl->class;
+    new_iface->concrete_class = ti->class;
+
+    ti->class->interfaces = g_slist_append(ti->class->interfaces,
+                                           iface_impl->class);
 }
 
 static void type_initialize(TypeImpl *ti)
 {
     TypeImpl *parent;
-    int i;
 
     if (ti->class) {
         return;
@@ -231,9 +239,33 @@ static void type_initialize(TypeImpl *ti)
     parent = type_get_parent(ti);
     if (parent) {
         type_initialize(parent);
+        GSList *e;
+        int i;
 
         g_assert(parent->class_size <= ti->class_size);
         memcpy(ti->class, parent->class, parent->class_size);
+
+        for (e = parent->class->interfaces; e; e = e->next) {
+            ObjectClass *iface = e->data;
+            type_initialize_interface(ti, object_class_get_name(iface));
+        }
+
+        for (i = 0; i < ti->num_interfaces; i++) {
+            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
+            for (e = ti->class->interfaces; e; e = e->next) {
+                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
+
+                if (type_is_ancestor(target_type, t)) {
+                    break;
+                }
+            }
+
+            if (e) {
+                continue;
+            }
+
+            type_initialize_interface(ti, ti->interfaces[i].typename);
+        }
     }
 
     ti->class->type = ti;
@@ -245,38 +277,19 @@ static void type_initialize(TypeImpl *ti)
         parent = type_get_parent(parent);
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        type_class_interface_init(ti, &ti->interfaces[i]);
-    }
-
     if (ti->class_init) {
         ti->class_init(ti->class, ti->class_data);
     }
-}
 
-static void object_interface_init(Object *obj, InterfaceImpl *iface)
-{
-    TypeImpl *ti = iface->type;
-    Interface *iface_obj;
-
-    iface_obj = INTERFACE(object_new(ti->name));
-    iface_obj->obj = obj;
 
-    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
 
 static void object_init_with_type(Object *obj, TypeImpl *ti)
 {
-    int i;
-
     if (type_has_parent(ti)) {
         object_init_with_type(obj, type_get_parent(ti));
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        object_interface_init(obj, &ti->interfaces[i]);
-    }
-
     if (ti->instance_init) {
         ti->instance_init(obj);
     }
@@ -357,12 +370,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
         type->instance_finalize(obj);
     }
 
-    while (obj->interfaces) {
-        Interface *iface_obj = obj->interfaces->data;
-        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
-        object_delete(OBJECT(iface_obj));
-    }
-
     if (type_has_parent(type)) {
         object_deinit(obj, type_get_parent(type));
     }
@@ -409,74 +416,15 @@ void object_delete(Object *obj)
     g_free(obj);
 }
 
-static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
-{
-    assert(target_type);
-
-    /* Check if typename is a direct ancestor of type */
-    while (type) {
-        if (type == target_type) {
-            return true;
-        }
-
-        type = type_get_parent(type);
-    }
-
-    return false;
-}
-
-static bool object_is_type(Object *obj, TypeImpl *target_type)
-{
-    return !target_type || type_is_ancestor(obj->class->type, target_type);
-}
-
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
-    TypeImpl *target_type = type_get_by_name(typename);
-    GSList *i;
-
-    /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
-     * we want to go back from interfaces to the parent.
-    */
-    if (target_type && object_is_type(obj, target_type)) {
+    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
         return obj;
     }
 
-    /* Check if obj is an interface and its containing object is a direct
-     * ancestor of typename.  In principle we could do this test at the very
-     * beginning of object_dynamic_cast, avoiding a second call to
-     * object_is_type.  However, casting between interfaces is relatively
-     * rare, and object_is_type(obj, type_interface) would fail almost always.
-     *
-     * Perhaps we could add a magic value to the object header for increased
-     * (run-time) type safety and to speed up tests like this one.  If we ever
-     * do that we can revisit the order here.
-     */
-    if (object_is_type(obj, type_interface)) {
-        assert(!obj->interfaces);
-        obj = INTERFACE(obj)->obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
-    if (!target_type) {
-        return obj;
-    }
-
-    /* Check if obj has an interface of typename */
-    for (i = obj->interfaces; i; i = i->next) {
-        Interface *iface = i->data;
-
-        if (object_is_type(OBJECT(iface), target_type)) {
-            return OBJECT(iface);
-        }
-    }
-
     return NULL;
 }
 
-
 Object *object_dynamic_cast_assert(Object *obj, const char *typename)
 {
     Object *inst;
@@ -497,16 +445,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 {
     TypeImpl *target_type = type_get_by_name(typename);
     TypeImpl *type = class->type;
+    ObjectClass *ret = NULL;
 
-    while (type) {
-        if (type == target_type) {
-            return class;
-        }
+    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
+        int found = 0;
+        GSList *i;
 
-        type = type_get_parent(type);
+        for (i = class->interfaces; i; i = i->next) {
+            ObjectClass *target_class = i->data;
+
+            if (type_is_ancestor(target_class->type, target_type)) {
+                ret = target_class;
+                found++;
+            }
+         }
+
+        /* The match was ambiguous, don't allow a cast */
+        if (found > 1) {
+            ret = NULL;
+        }
+    } else if (type_is_ancestor(type, target_type)) {
+        ret = class;
     }
 
-    return NULL;
+    return ret;
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
@@ -920,12 +882,6 @@ void object_property_add_child(Object *obj, const char *name,
 {
     gchar *type;
 
-    /* Registering an interface object in the composition tree will mightily
-     * confuse object_get_canonical_path (which, on the other hand, knows how
-     * to get the canonical path of an interface object).
-     */
-    assert(!object_is_type(obj, type_interface));
-
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     object_property_add(obj, name, type, object_get_child_property,
@@ -1022,10 +978,6 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
-        obj = INTERFACE(obj)->obj;
-    }
-
     while (obj != root) {
         ObjectProperty *prop = NULL;
 
@@ -1246,7 +1198,7 @@ static void register_types(void)
 {
     static TypeInfo interface_info = {
         .name = TYPE_INTERFACE,
-        .instance_size = sizeof(Interface),
+        .class_size = sizeof(InterfaceClass),
         .abstract = true,
     };
 
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/2] xilinx_axi*: Re-implemented interconnect

[Peter A. G. Crosthwaite]

Re-implemented the interconnect between the Xilinx AXI ethernet and DMA
controllers. A QOM interface "stream" is created, for the two stream interfaces.

As per Edgars request, this is designed to be more generic than AXI-stream,
so in the future we may see more clients of this interface beyond AXI stream.

This is based primarily on Paolos original refactoring of the interconnect.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter A.G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/Makefile.objs         |    1 +
 hw/petalogix_ml605_mmu.c |   24 +++++++++------
 hw/stream.c              |   23 ++++++++++++++
 hw/stream.h              |   31 +++++++++++++++++++
 hw/xilinx.h              |   22 +++++--------
 hw/xilinx_axidma.c       |   74 +++++++++++++++++++++++++--------------------
 hw/xilinx_axidma.h       |   39 ------------------------
 hw/xilinx_axienet.c      |   32 ++++++++++++-------
 8 files changed, 139 insertions(+), 107 deletions(-)
 create mode 100644 hw/stream.c
 create mode 100644 hw/stream.h
 delete mode 100644 hw/xilinx_axidma.h

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 12cc141..3849643 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -65,6 +65,7 @@ hw-obj-$(CONFIG_XILINX) += xilinx_timer.o
 hw-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
 hw-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
 hw-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
+hw-obj-$(CONFIG_XILINX_AXI) += stream.o
 
 # PCI watchdog devices
 hw-obj-$(CONFIG_PCI) += wdt_i6300esb.o
diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index 6a7d0c0..dced648 100644
--- a/hw/petalogix_ml605_mmu.c
+++ b/hw/petalogix_ml605_mmu.c
@@ -39,7 +39,8 @@
 
 #include "microblaze_boot.h"
 #include "microblaze_pic_cpu.h"
-#include "xilinx_axidma.h"
+
+#include "stream.h"
 
 #define LMB_BRAM_SIZE  (128 * 1024)
 #define FLASH_SIZE     (32 * 1024 * 1024)
@@ -76,7 +77,7 @@ petalogix_ml605_init(ram_addr_t ram_size,
                           const char *initrd_filename, const char *cpu_model)
 {
     MemoryRegion *address_space_mem = get_system_memory();
-    DeviceState *dev;
+    DeviceState *dev, *dma, *eth0;
     MicroBlazeCPU *cpu;
     CPUMBState *env;
     DriveInfo *dinfo;
@@ -125,15 +126,18 @@ petalogix_ml605_init(ram_addr_t ram_size,
     /* 2 timers at irq 2 @ 100 Mhz.  */
     xilinx_timer_create(TIMER_BASEADDR, irq[2], 0, 100 * 1000000);
 
-    /* axi ethernet and dma initialization. TODO: Dynamically connect them.  */
-    {
-        static struct XilinxDMAConnection dmach;
+    /* axi ethernet and dma initialization. */
+    dma = qdev_create(NULL, "xlnx.axi-dma");
 
-        xilinx_axiethernet_create(&dmach, &nd_table[0], 0x82780000,
-                                  irq[3], 0x1000, 0x1000);
-        xilinx_axiethernetdma_create(&dmach, 0x84600000,
-                                     irq[1], irq[0], 100 * 1000000);
-    }
+    /* FIXME: attach to the sysbus instead */
+    object_property_add_child(container_get(qdev_get_machine(), "/unattached"),
+                                  "xilinx-dma", OBJECT(dma), NULL);
+
+    eth0 = xilinx_axiethernet_create(&nd_table[0], STREAM_SLAVE(dma),
+                                     0x82780000, irq[3], 0x1000, 0x1000);
+
+    xilinx_axiethernetdma_init(dma, STREAM_SLAVE(eth0),
+                               0x84600000, irq[1], irq[0], 100 * 1000000);
 
     microblaze_load_kernel(cpu, ddr_base, ram_size, BINARY_DEVICE_TREE_FILE,
                                                             machine_cpu_reset);
diff --git a/hw/stream.c b/hw/stream.c
new file mode 100644
index 0000000..be57e8b
--- /dev/null
+++ b/hw/stream.c
@@ -0,0 +1,23 @@
+#include "stream.h"
+
+void
+stream_push(StreamSlave *sink, uint8_t *buf, size_t len, uint32_t *app)
+{
+    StreamSlaveClass *k =  STREAM_SLAVE_GET_CLASS(sink);
+
+    k->push(sink, buf, len, app);
+}
+
+static TypeInfo stream_slave_info = {
+    .name          = TYPE_STREAM_SLAVE,
+    .parent        = TYPE_INTERFACE,
+    .class_size = sizeof(StreamSlaveClass),
+};
+
+
+static void stream_slave_register_types(void)
+{
+    type_register_static(&stream_slave_info);
+}
+
+type_init(stream_slave_register_types)
diff --git a/hw/stream.h b/hw/stream.h
new file mode 100644
index 0000000..21123a9
--- /dev/null
+++ b/hw/stream.h
@@ -0,0 +1,31 @@
+#ifndef STREAM_H
+#define STREAM_H 1
+
+#include "qemu-common.h"
+#include "qemu/object.h"
+
+/* stream slave. Used until qdev provides a generic way.  */
+#define TYPE_STREAM_SLAVE "stream-slave"
+
+#define STREAM_SLAVE_CLASS(klass) \
+     OBJECT_CLASS_CHECK(StreamSlaveClass, (klass), TYPE_STREAM_SLAVE)
+#define STREAM_SLAVE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(StreamSlaveClass, (obj), TYPE_STREAM_SLAVE)
+#define STREAM_SLAVE(obj) \
+     INTERFACE_CHECK(StreamSlave, (obj), TYPE_STREAM_SLAVE)
+
+typedef struct StreamSlave {
+    Object Parent;
+} StreamSlave;
+
+typedef struct StreamSlaveClass {
+    InterfaceClass parent;
+
+    void (*push)(StreamSlave *obj, unsigned char *buf, size_t len,
+                                                    uint32_t *app);
+} StreamSlaveClass;
+
+void
+stream_push(StreamSlave *sink, uint8_t *buf, size_t len, uint32_t *app);
+
+#endif /* STREAM_H */
diff --git a/hw/xilinx.h b/hw/xilinx.h
index 7df21eb..556c5aa 100644
--- a/hw/xilinx.h
+++ b/hw/xilinx.h
@@ -1,3 +1,4 @@
+#include "stream.h"
 #include "qemu-common.h"
 #include "net.h"
 
@@ -49,8 +50,8 @@ xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
 }
 
 static inline DeviceState *
-xilinx_axiethernet_create(void *dmach,
-                          NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
+xilinx_axiethernet_create(NICInfo *nd, StreamSlave *peer,
+                          target_phys_addr_t base, qemu_irq irq,
                           int txmem, int rxmem)
 {
     DeviceState *dev;
@@ -60,7 +61,7 @@ xilinx_axiethernet_create(void *dmach,
     qdev_set_nic_properties(dev, nd);
     qdev_prop_set_uint32(dev, "rxmem", rxmem);
     qdev_prop_set_uint32(dev, "txmem", txmem);
-    qdev_prop_set_ptr(dev, "dmach", dmach);
+    object_property_set_link(OBJECT(dev), OBJECT(peer), "tx_dev", NULL);
     qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
@@ -68,21 +69,16 @@ xilinx_axiethernet_create(void *dmach,
     return dev;
 }
 
-static inline DeviceState *
-xilinx_axiethernetdma_create(void *dmach,
-                             target_phys_addr_t base, qemu_irq irq,
-                             qemu_irq irq2, int freqhz)
+static inline void
+xilinx_axiethernetdma_init(DeviceState *dev, StreamSlave *peer,
+                           target_phys_addr_t base, qemu_irq irq,
+                           qemu_irq irq2, int freqhz)
 {
-    DeviceState *dev = NULL;
-
-    dev = qdev_create(NULL, "xlnx.axi-dma");
     qdev_prop_set_uint32(dev, "freqhz", freqhz);
-    qdev_prop_set_ptr(dev, "dmach", dmach);
+    object_property_set_link(OBJECT(dev), OBJECT(peer), "tx_dev", NULL);
     qdev_init_nofail(dev);
 
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
     sysbus_connect_irq(sysbus_from_qdev(dev), 1, irq2);
-
-    return dev;
 }
diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
index f4bec37..0e28c51 100644
--- a/hw/xilinx_axidma.c
+++ b/hw/xilinx_axidma.c
@@ -29,7 +29,7 @@
 #include "qemu-log.h"
 #include "qdev-addr.h"
 
-#include "xilinx_axidma.h"
+#include "stream.h"
 
 #define D(x)
 
@@ -77,7 +77,7 @@ enum {
     SDESC_STATUS_COMPLETE = (1 << 31)
 };
 
-struct AXIStream {
+struct Stream {
     QEMUBH *bh;
     ptimer_state *ptimer;
     qemu_irq irq;
@@ -94,9 +94,9 @@ struct XilinxAXIDMA {
     SysBusDevice busdev;
     MemoryRegion iomem;
     uint32_t freqhz;
-    void *dmach;
+    StreamSlave *tx_dev;
 
-    struct AXIStream streams[2];
+    struct Stream streams[2];
 };
 
 /*
@@ -113,27 +113,27 @@ static inline int stream_desc_eof(struct SDesc *d)
     return d->control & SDESC_CTRL_EOF;
 }
 
-static inline int stream_resetting(struct AXIStream *s)
+static inline int stream_resetting(struct Stream *s)
 {
     return !!(s->regs[R_DMACR] & DMACR_RESET);
 }
 
-static inline int stream_running(struct AXIStream *s)
+static inline int stream_running(struct Stream *s)
 {
     return s->regs[R_DMACR] & DMACR_RUNSTOP;
 }
 
-static inline int stream_halted(struct AXIStream *s)
+static inline int stream_halted(struct Stream *s)
 {
     return s->regs[R_DMASR] & DMASR_HALTED;
 }
 
-static inline int stream_idle(struct AXIStream *s)
+static inline int stream_idle(struct Stream *s)
 {
     return !!(s->regs[R_DMASR] & DMASR_IDLE);
 }
 
-static void stream_reset(struct AXIStream *s)
+static void stream_reset(struct Stream *s)
 {
     s->regs[R_DMASR] = DMASR_HALTED;  /* starts up halted.  */
     s->regs[R_DMACR] = 1 << 16; /* Starts with one in compl threshold.  */
@@ -159,7 +159,7 @@ static void stream_desc_show(struct SDesc *d)
 }
 #endif
 
-static void stream_desc_load(struct AXIStream *s, target_phys_addr_t addr)
+static void stream_desc_load(struct Stream *s, target_phys_addr_t addr)
 {
     struct SDesc *d = &s->desc;
     int i;
@@ -176,7 +176,7 @@ static void stream_desc_load(struct AXIStream *s, target_phys_addr_t addr)
     }
 }
 
-static void stream_desc_store(struct AXIStream *s, target_phys_addr_t addr)
+static void stream_desc_store(struct Stream *s, target_phys_addr_t addr)
 {
     struct SDesc *d = &s->desc;
     int i;
@@ -192,7 +192,7 @@ static void stream_desc_store(struct AXIStream *s, target_phys_addr_t addr)
     cpu_physical_memory_write(addr, (void *) d, sizeof *d);
 }
 
-static void stream_update_irq(struct AXIStream *s)
+static void stream_update_irq(struct Stream *s)
 {
     unsigned int pending, mask, irq;
 
@@ -204,7 +204,7 @@ static void stream_update_irq(struct AXIStream *s)
     qemu_set_irq(s->irq, !!irq);
 }
 
-static void stream_reload_complete_cnt(struct AXIStream *s)
+static void stream_reload_complete_cnt(struct Stream *s)
 {
     unsigned int comp_th;
     comp_th = (s->regs[R_DMACR] >> 16) & 0xff;
@@ -213,14 +213,14 @@ static void stream_reload_complete_cnt(struct AXIStream *s)
 
 static void timer_hit(void *opaque)
 {
-    struct AXIStream *s = opaque;
+    struct Stream *s = opaque;
 
     stream_reload_complete_cnt(s);
     s->regs[R_DMASR] |= DMASR_DLY_IRQ;
     stream_update_irq(s);
 }
 
-static void stream_complete(struct AXIStream *s)
+static void stream_complete(struct Stream *s)
 {
     unsigned int comp_delay;
 
@@ -240,8 +240,8 @@ static void stream_complete(struct AXIStream *s)
     }
 }
 
-static void stream_process_mem2s(struct AXIStream *s,
-                                 struct XilinxDMAConnection *dmach)
+static void stream_process_mem2s(struct Stream *s,
+                                 StreamSlave *tx_dev)
 {
     uint32_t prev_d;
     unsigned char txbuf[16 * 1024];
@@ -276,7 +276,7 @@ static void stream_process_mem2s(struct AXIStream *s,
         s->pos += txlen;
 
         if (stream_desc_eof(&s->desc)) {
-            xlx_dma_push_to_client(dmach, txbuf, s->pos, app);
+            stream_push(tx_dev, txbuf, s->pos, app);
             s->pos = 0;
             stream_complete(s);
         }
@@ -295,7 +295,7 @@ static void stream_process_mem2s(struct AXIStream *s,
     }
 }
 
-static void stream_process_s2mem(struct AXIStream *s,
+static void stream_process_s2mem(struct Stream *s,
                                  unsigned char *buf, size_t len, uint32_t *app)
 {
     uint32_t prev_d;
@@ -351,11 +351,11 @@ static void stream_process_s2mem(struct AXIStream *s,
     }
 }
 
-static
-void axidma_push(void *opaque, unsigned char *buf, size_t len, uint32_t *app)
+static void
+axidma_push(StreamSlave *obj, unsigned char *buf, size_t len, uint32_t *app)
 {
-    struct XilinxAXIDMA *d = opaque;
-    struct AXIStream *s = &d->streams[1];
+    struct XilinxAXIDMA *d = FROM_SYSBUS(typeof(*d), SYS_BUS_DEVICE(obj));
+    struct Stream *s = &d->streams[1];
 
     if (!app) {
         hw_error("No stream app data!\n");
@@ -368,7 +368,7 @@ static uint64_t axidma_read(void *opaque, target_phys_addr_t addr,
                             unsigned size)
 {
     struct XilinxAXIDMA *d = opaque;
-    struct AXIStream *s;
+    struct Stream *s;
     uint32_t r = 0;
     int sid;
 
@@ -403,7 +403,7 @@ static void axidma_write(void *opaque, target_phys_addr_t addr,
                          uint64_t value, unsigned size)
 {
     struct XilinxAXIDMA *d = opaque;
-    struct AXIStream *s;
+    struct Stream *s;
     int sid;
 
     sid = streamid_from_addr(addr);
@@ -440,7 +440,7 @@ static void axidma_write(void *opaque, target_phys_addr_t addr,
             s->regs[addr] = value;
             s->regs[R_DMASR] &= ~DMASR_IDLE; /* Not idle.  */
             if (!sid) {
-                stream_process_mem2s(s, d->dmach);
+                stream_process_mem2s(s, d->tx_dev);
             }
             break;
         default:
@@ -466,12 +466,6 @@ static int xilinx_axidma_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->streams[0].irq);
     sysbus_init_irq(dev, &s->streams[1].irq);
 
-    if (!s->dmach) {
-        hw_error("Unconnected DMA channel.\n");
-    }
-
-    xlx_dma_connect_dma(s->dmach, s, axidma_push);
-
     memory_region_init_io(&s->iomem, &axidma_ops, s,
                           "xlnx.axi-dma", R_MAX * 4 * 2);
     sysbus_init_mmio(dev, &s->iomem);
@@ -486,9 +480,16 @@ static int xilinx_axidma_init(SysBusDevice *dev)
     return 0;
 }
 
+static void xilinx_axidma_initfn(Object *obj)
+{
+    struct XilinxAXIDMA *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
+
+    object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
+                             (Object **) &s->tx_dev, NULL);
+}
+
 static Property axidma_properties[] = {
     DEFINE_PROP_UINT32("freqhz", struct XilinxAXIDMA, freqhz, 50000000),
-    DEFINE_PROP_PTR("dmach", struct XilinxAXIDMA, dmach),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -496,9 +497,11 @@ static void axidma_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    StreamSlaveClass *ssc = STREAM_SLAVE_CLASS(klass);
 
     k->init = xilinx_axidma_init;
     dc->props = axidma_properties;
+    ssc->push = axidma_push;
 }
 
 static TypeInfo axidma_info = {
@@ -506,6 +509,11 @@ static TypeInfo axidma_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(struct XilinxAXIDMA),
     .class_init    = axidma_class_init,
+    .instance_init = xilinx_axidma_initfn,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_STREAM_SLAVE },
+        { }
+    }
 };
 
 static void xilinx_axidma_register_types(void)
diff --git a/hw/xilinx_axidma.h b/hw/xilinx_axidma.h
deleted file mode 100644
index 37cb6f0..0000000
--- a/hw/xilinx_axidma.h
+++ /dev/null
@@ -1,39 +0,0 @@
-/* AXI DMA connection. Used until qdev provides a generic way.  */
-typedef void (*DMAPushFn)(void *opaque,
-                            unsigned char *buf, size_t len, uint32_t *app);
-
-struct XilinxDMAConnection {
-    void *dma;
-    void *client;
-
-    DMAPushFn to_dma;
-    DMAPushFn to_client;
-};
-
-static inline void xlx_dma_connect_client(struct XilinxDMAConnection *dmach,
-                                          void *c, DMAPushFn f)
-{
-    dmach->client = c;
-    dmach->to_client = f;
-}
-
-static inline void xlx_dma_connect_dma(struct XilinxDMAConnection *dmach,
-                                       void *d, DMAPushFn f)
-{
-    dmach->dma = d;
-    dmach->to_dma = f;
-}
-
-static inline
-void xlx_dma_push_to_dma(struct XilinxDMAConnection *dmach,
-                         uint8_t *buf, size_t len, uint32_t *app)
-{
-    dmach->to_dma(dmach->dma, buf, len, app);
-}
-static inline
-void xlx_dma_push_to_client(struct XilinxDMAConnection *dmach,
-                            uint8_t *buf, size_t len, uint32_t *app)
-{
-    dmach->to_client(dmach->client, buf, len, app);
-}
-
diff --git a/hw/xilinx_axienet.c b/hw/xilinx_axienet.c
index 9b08c62..eec155d 100644
--- a/hw/xilinx_axienet.c
+++ b/hw/xilinx_axienet.c
@@ -28,7 +28,7 @@
 #include "net.h"
 #include "net/checksum.h"
 
-#include "xilinx_axidma.h"
+#include "stream.h"
 
 #define DPHY(x)
 
@@ -310,7 +310,7 @@ struct XilinxAXIEnet {
     SysBusDevice busdev;
     MemoryRegion iomem;
     qemu_irq irq;
-    void *dmach;
+    StreamSlave *tx_dev;
     NICState *nic;
     NICConf conf;
 
@@ -772,7 +772,7 @@ static ssize_t eth_rx(NetClientState *nc, const uint8_t *buf, size_t size)
     /* Good frame.  */
     app[2] |= 1 << 6;
 
-    xlx_dma_push_to_dma(s->dmach, (void *)s->rxmem, size, app);
+    stream_push(s->tx_dev, (void *)s->rxmem, size, app);
 
     s->regs[R_IS] |= IS_RX_COMPLETE;
     enet_update_irq(s);
@@ -788,9 +788,9 @@ static void eth_cleanup(NetClientState *nc)
 }
 
 static void
-axienet_stream_push(void *opaque, uint8_t *buf, size_t size, uint32_t *hdr)
+axienet_stream_push(StreamSlave *obj, uint8_t *buf, size_t size, uint32_t *hdr)
 {
-    struct XilinxAXIEnet *s = opaque;
+    struct XilinxAXIEnet *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
 
     /* TX enable ?  */
     if (!(s->tc & TC_TX)) {
@@ -844,12 +844,6 @@ static int xilinx_enet_init(SysBusDevice *dev)
 
     sysbus_init_irq(dev, &s->irq);
 
-    if (!s->dmach) {
-        hw_error("Unconnected Xilinx Ethernet MAC.\n");
-    }
-
-    xlx_dma_connect_client(s->dmach, s, axienet_stream_push);
-
     memory_region_init_io(&s->iomem, &enet_ops, s, "enet", 0x40000);
     sysbus_init_mmio(dev, &s->iomem);
 
@@ -869,11 +863,18 @@ static int xilinx_enet_init(SysBusDevice *dev)
     return 0;
 }
 
+static void xilinx_enet_initfn(Object *obj)
+{
+    struct XilinxAXIEnet *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
+
+    object_property_add_link(obj, "axistream-connected", TYPE_STREAM_SLAVE,
+                             (Object **) &s->tx_dev, NULL);
+}
+
 static Property xilinx_enet_properties[] = {
     DEFINE_PROP_UINT32("phyaddr", struct XilinxAXIEnet, c_phyaddr, 7),
     DEFINE_PROP_UINT32("rxmem", struct XilinxAXIEnet, c_rxmem, 0x1000),
     DEFINE_PROP_UINT32("txmem", struct XilinxAXIEnet, c_txmem, 0x1000),
-    DEFINE_PROP_PTR("dmach", struct XilinxAXIEnet, dmach),
     DEFINE_NIC_PROPERTIES(struct XilinxAXIEnet, conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -882,9 +883,11 @@ static void xilinx_enet_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+    StreamSlaveClass *ssc = STREAM_SLAVE_CLASS(klass);
 
     k->init = xilinx_enet_init;
     dc->props = xilinx_enet_properties;
+    ssc->push = axienet_stream_push;
 }
 
 static TypeInfo xilinx_enet_info = {
@@ -892,6 +895,11 @@ static TypeInfo xilinx_enet_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(struct XilinxAXIEnet),
     .class_init    = xilinx_enet_class_init,
+    .instance_init = xilinx_enet_initfn,
+    .interfaces = (InterfaceInfo[]) {
+            { TYPE_STREAM_SLAVE },
+            { }
+    }
 };
 
 static void xilinx_enet_register_types(void)
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Andreas Färber]

Am 10.08.2012 05:16, schrieb Peter A. G. Crosthwaite:
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> The current implementation of Interfaces is poorly designed.  Each interface
> that an object implements ends up being an object that's tracked by the
> implementing object.  There's all sorts of gymnastics to deal with casting
> between these objects.
> 
> But an interface shouldn't be associated with an Object.  Interfaces are global
> to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
> the relationship between Object and Interfaces.
> 
> Interfaces are now abstract (as they should be) but this is okay.  Interfaces
> essentially act as additional parents for the classes and are treated as such.
> 
> With this new implementation, we should fully support derived interfaces
> including reimplementing an inherited interface.
> 
> PC: Rebased against qom-next merge Jun-2012.
> 
> PC: Removed replication of cast logic for interfaces, i.e. there is only
> one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Anthony, didn't you have a whole series to refactor interfaces and add
test cases?

/-F

-- 
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 1/2] qom: Reimplement Interfaces

[Peter Crosthwaite]

On Fri, Aug 10, 2012 at 7:15 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 10.08.2012 05:16, schrieb Peter A. G. Crosthwaite:
>> From: Anthony Liguori <aliguori@us.ibm.com>
>>
>> The current implementation of Interfaces is poorly designed.  Each interface
>> that an object implements ends up being an object that's tracked by the
>> implementing object.  There's all sorts of gymnastics to deal with casting
>> between these objects.
>>
>> But an interface shouldn't be associated with an Object.  Interfaces are global
>> to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
>> the relationship between Object and Interfaces.
>>
>> Interfaces are now abstract (as they should be) but this is okay.  Interfaces
>> essentially act as additional parents for the classes and are treated as such.
>>
>> With this new implementation, we should fully support derived interfaces
>> including reimplementing an inherited interface.
>>
>> PC: Rebased against qom-next merge Jun-2012.
>>
>> PC: Removed replication of cast logic for interfaces, i.e. there is only
>> one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Anthony, didn't you have a whole series to refactor interfaces and add
> test cases?
>

This is patch 1 from that series. I have refactored it a little (as
indicated in the comments). Unless theres been another series since I
missed?

> /-F
>
> --
> 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] [PULL 0/2] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Edgar E. Iglesias]

On Fri, Aug 10, 2012 at 01:16:09PM +1000, Peter A. G. Crosthwaite wrote:
> The following changes since commit 3d1d9652978ac5a32a0beb4bdf6065ca39440d89:
>   Bruce Rogers (1):
>         handle device help before accelerator set up
> 
> are available in the git repository at:
> 
>   git://developer.petalogix.com/public/qemu.git for-upstream/axi-stream.next
> 
> Anthony Liguori (1):
>       qom: Reimplement Interfaces
> 
> Peter A. G. Crosthwaite (1):
>       xilinx_axi*: Re-implemented interconnect

Applied, thanks Peter.

Cheers,
Edgar


> 
>  hw/Makefile.objs         |    1 +
>  hw/petalogix_ml605_mmu.c |   24 +++--
>  hw/stream.c              |   23 +++++
>  hw/stream.h              |   31 +++++++
>  hw/xilinx.h              |   22 ++---
>  hw/xilinx_axidma.c       |   74 +++++++++-------
>  hw/xilinx_axidma.h       |   39 --------
>  hw/xilinx_axienet.c      |   32 ++++---
>  include/qemu/object.h    |   46 ++++++----
>  qom/object.c             |  220 ++++++++++++++++++----------------------------
>  10 files changed, 255 insertions(+), 257 deletions(-)
>  create mode 100644 hw/stream.c
>  create mode 100644 hw/stream.h
>  delete mode 100644 hw/xilinx_axidma.h

Re: [Qemu-devel] [PULL 0/2] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Andreas Färber]

Hej Edgar,

Am 13.08.2012 11:41, schrieb Edgar E. Iglesias:
> On Fri, Aug 10, 2012 at 01:16:09PM +1000, Peter A. G. Crosthwaite wrote:
>> The following changes since commit 3d1d9652978ac5a32a0beb4bdf6065ca39440d89:
>>   Bruce Rogers (1):
>>         handle device help before accelerator set up
>>
>> are available in the git repository at:
>>
>>   git://developer.petalogix.com/public/qemu.git for-upstream/axi-stream.next
>>
>> Anthony Liguori (1):
>>       qom: Reimplement Interfaces
>>
>> Peter A. G. Crosthwaite (1):
>>       xilinx_axi*: Re-implemented interconnect
> 
> Applied, thanks Peter.

Sorry to bug you again, but I notice that in qemu.git HEAD these patches
have changed order. I have no clue if the introduction of a first
interface user before the infrastructure changes causes any
bisectability problems in practice, but since the above branch matches
this pull request it suggests that you are not using git-merge and may
want to check the script you are using in its place (git-rev-list?).

Regards,
Andreas

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

Re: [Qemu-devel] [PULL 0/2] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Edgar E. Iglesias]

On Mon, Aug 13, 2012 at 06:50:43PM +0200, Andreas Färber wrote:
> Hej Edgar,
> 
> Am 13.08.2012 11:41, schrieb Edgar E. Iglesias:
> > On Fri, Aug 10, 2012 at 01:16:09PM +1000, Peter A. G. Crosthwaite wrote:
> >> The following changes since commit 3d1d9652978ac5a32a0beb4bdf6065ca39440d89:
> >>   Bruce Rogers (1):
> >>         handle device help before accelerator set up
> >>
> >> are available in the git repository at:
> >>
> >>   git://developer.petalogix.com/public/qemu.git for-upstream/axi-stream.next
> >>
> >> Anthony Liguori (1):
> >>       qom: Reimplement Interfaces
> >>
> >> Peter A. G. Crosthwaite (1):
> >>       xilinx_axi*: Re-implemented interconnect
> > 
> > Applied, thanks Peter.
> 
> Sorry to bug you again, but I notice that in qemu.git HEAD these patches
> have changed order. I have no clue if the introduction of a first
> interface user before the infrastructure changes causes any
> bisectability problems in practice, but since the above branch matches
> this pull request it suggests that you are not using git-merge and may
> want to check the script you are using in its place (git-rev-list?).

Hi,

Yeah, I had problems reaching the host so applied manually with git am.
I must have reversed the order between these, sorry. I guess the
bisectability harm is already done...

Cheers,
Edgar

[Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Peter A. G. Crosthwaite]

From: Anthony Liguori <aliguori@us.ibm.com>

The current implementation of Interfaces is poorly designed.  Each interface
that an object implements ends up being an object that's tracked by the
implementing object.  There's all sorts of gymnastics to deal with casting
between these objects.

But an interface shouldn't be associated with an Object.  Interfaces are global
to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
the relationship between Object and Interfaces.

Interfaces are now abstract (as they should be) but this is okay.  Interfaces
essentially act as additional parents for the classes and are treated as such.

With this new implementation, we should fully support derived interfaces
including reimplementing an inherited interface.

PC: Rebased against qom-next merge Jun-2012.

PC: Removed replication of cast logic for interfaces, i.e. there is only
one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/object.h |   46 +++++++----
 qom/object.c          |  220 +++++++++++++++++++------------------------------
 2 files changed, 116 insertions(+), 150 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8b17776..cc75fee 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,7 @@ struct ObjectClass
 {
     /*< private >*/
     Type type;
+    GSList *interfaces;
 };
 
 /**
@@ -260,7 +261,6 @@ struct Object
 {
     /*< private >*/
     ObjectClass *class;
-    GSList *interfaces;
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
@@ -387,6 +387,16 @@ struct TypeInfo
     OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
 
 /**
+ * InterfaceInfo:
+ * @type: The name of the interface.
+ *
+ * The information associated with an interface.
+ */
+struct InterfaceInfo {
+    const char *type;
+};
+
+/**
  * InterfaceClass:
  * @parent_class: the base class
  *
@@ -396,26 +406,30 @@ struct TypeInfo
 struct InterfaceClass
 {
     ObjectClass parent_class;
+    /*< private >*/
+    ObjectClass *concrete_class;
 };
 
+#define TYPE_INTERFACE "interface"
+
 /**
- * InterfaceInfo:
- * @type: The name of the interface.
- * @interface_initfn: This method is called during class initialization and is
- *   used to initialize an interface associated with a class.  This function
- *   should initialize any default virtual functions for a class and/or override
- *   virtual functions in a parent class.
- *
- * The information associated with an interface.
+ * INTERFACE_CLASS:
+ * @klass: class to cast from
+ * Returns: An #InterfaceClass or raise an error if cast is invalid
  */
-struct InterfaceInfo
-{
-    const char *type;
+#define INTERFACE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
 
-    void (*interface_initfn)(ObjectClass *class, void *data);
-};
-
-#define TYPE_INTERFACE "interface"
+/**
+ * INTERFACE_CHECK:
+ * @interface: the type to return
+ * @obj: the object to convert to an interface
+ * @name: the interface type name
+ *
+ * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
+ */
+#define INTERFACE_CHECK(interface, obj, name) \
+    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
 
 /**
  * object_new:
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..a552be2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
 
 struct InterfaceImpl
 {
-    const char *parent;
-    void (*interface_initfn)(ObjectClass *class, void *data);
-    TypeImpl *type;
+    const char *typename;
 };
 
 struct TypeImpl
@@ -64,14 +62,6 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-typedef struct Interface
-{
-    Object parent;
-    Object *obj;
-} Interface;
-
-#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
-
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -98,6 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
 static TypeImpl *type_register_internal(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
+    int i;
 
     g_assert(info->name != NULL);
 
@@ -122,15 +113,10 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
 
     ti->abstract = info->abstract;
 
-    if (info->interfaces) {
-        int i;
-
-        for (i = 0; info->interfaces[i].type; i++) {
-            ti->interfaces[i].parent = info->interfaces[i].type;
-            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
-            ti->num_interfaces++;
-        }
+    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
+        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
     }
+    ti->num_interfaces = i;
 
     type_table_add(ti);
 
@@ -198,26 +184,48 @@ static size_t type_object_get_size(TypeImpl *ti)
     return 0;
 }
 
-static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
+static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
 {
-    TypeInfo info = {
-        .instance_size = sizeof(Interface),
-        .parent = iface->parent,
-        .class_size = sizeof(InterfaceClass),
-        .class_init = iface->interface_initfn,
-        .abstract = true,
-    };
-    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
+    assert(target_type);
+
+    /* Check if typename is a direct ancestor of type */
+    while (type) {
+        if (type == target_type) {
+            return true;
+        }
 
-    info.name = name;
-    iface->type = type_register_internal(&info);
-    g_free(name);
+        type = type_get_parent(type);
+    }
+
+    return false;
+}
+
+static void type_initialize(TypeImpl *ti);
+
+static void type_initialize_interface(TypeImpl *ti, const char *parent)
+{
+    InterfaceClass *new_iface;
+    TypeInfo info = { };
+    TypeImpl *iface_impl;
+
+    info.parent = parent;
+    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
+    info.abstract = true;
+
+    iface_impl = type_register(&info);
+    type_initialize(iface_impl);
+    g_free((char *)info.name);
+
+    new_iface = (InterfaceClass *)iface_impl->class;
+    new_iface->concrete_class = ti->class;
+
+    ti->class->interfaces = g_slist_append(ti->class->interfaces,
+                                           iface_impl->class);
 }
 
 static void type_initialize(TypeImpl *ti)
 {
     TypeImpl *parent;
-    int i;
 
     if (ti->class) {
         return;
@@ -231,9 +239,33 @@ static void type_initialize(TypeImpl *ti)
     parent = type_get_parent(ti);
     if (parent) {
         type_initialize(parent);
+        GSList *e;
+        int i;
 
         g_assert(parent->class_size <= ti->class_size);
         memcpy(ti->class, parent->class, parent->class_size);
+
+        for (e = parent->class->interfaces; e; e = e->next) {
+            ObjectClass *iface = e->data;
+            type_initialize_interface(ti, object_class_get_name(iface));
+        }
+
+        for (i = 0; i < ti->num_interfaces; i++) {
+            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
+            for (e = ti->class->interfaces; e; e = e->next) {
+                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
+
+                if (type_is_ancestor(target_type, t)) {
+                    break;
+                }
+            }
+
+            if (e) {
+                continue;
+            }
+
+            type_initialize_interface(ti, ti->interfaces[i].typename);
+        }
     }
 
     ti->class->type = ti;
@@ -245,38 +277,19 @@ static void type_initialize(TypeImpl *ti)
         parent = type_get_parent(parent);
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        type_class_interface_init(ti, &ti->interfaces[i]);
-    }
-
     if (ti->class_init) {
         ti->class_init(ti->class, ti->class_data);
     }
-}
 
-static void object_interface_init(Object *obj, InterfaceImpl *iface)
-{
-    TypeImpl *ti = iface->type;
-    Interface *iface_obj;
-
-    iface_obj = INTERFACE(object_new(ti->name));
-    iface_obj->obj = obj;
 
-    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
 
 static void object_init_with_type(Object *obj, TypeImpl *ti)
 {
-    int i;
-
     if (type_has_parent(ti)) {
         object_init_with_type(obj, type_get_parent(ti));
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        object_interface_init(obj, &ti->interfaces[i]);
-    }
-
     if (ti->instance_init) {
         ti->instance_init(obj);
     }
@@ -357,12 +370,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
         type->instance_finalize(obj);
     }
 
-    while (obj->interfaces) {
-        Interface *iface_obj = obj->interfaces->data;
-        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
-        object_delete(OBJECT(iface_obj));
-    }
-
     if (type_has_parent(type)) {
         object_deinit(obj, type_get_parent(type));
     }
@@ -409,74 +416,15 @@ void object_delete(Object *obj)
     g_free(obj);
 }
 
-static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
-{
-    assert(target_type);
-
-    /* Check if typename is a direct ancestor of type */
-    while (type) {
-        if (type == target_type) {
-            return true;
-        }
-
-        type = type_get_parent(type);
-    }
-
-    return false;
-}
-
-static bool object_is_type(Object *obj, TypeImpl *target_type)
-{
-    return !target_type || type_is_ancestor(obj->class->type, target_type);
-}
-
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
-    TypeImpl *target_type = type_get_by_name(typename);
-    GSList *i;
-
-    /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
-     * we want to go back from interfaces to the parent.
-    */
-    if (target_type && object_is_type(obj, target_type)) {
+    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
         return obj;
     }
 
-    /* Check if obj is an interface and its containing object is a direct
-     * ancestor of typename.  In principle we could do this test at the very
-     * beginning of object_dynamic_cast, avoiding a second call to
-     * object_is_type.  However, casting between interfaces is relatively
-     * rare, and object_is_type(obj, type_interface) would fail almost always.
-     *
-     * Perhaps we could add a magic value to the object header for increased
-     * (run-time) type safety and to speed up tests like this one.  If we ever
-     * do that we can revisit the order here.
-     */
-    if (object_is_type(obj, type_interface)) {
-        assert(!obj->interfaces);
-        obj = INTERFACE(obj)->obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
-    if (!target_type) {
-        return obj;
-    }
-
-    /* Check if obj has an interface of typename */
-    for (i = obj->interfaces; i; i = i->next) {
-        Interface *iface = i->data;
-
-        if (object_is_type(OBJECT(iface), target_type)) {
-            return OBJECT(iface);
-        }
-    }
-
     return NULL;
 }
 
-
 Object *object_dynamic_cast_assert(Object *obj, const char *typename)
 {
     Object *inst;
@@ -497,16 +445,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 {
     TypeImpl *target_type = type_get_by_name(typename);
     TypeImpl *type = class->type;
+    ObjectClass *ret = NULL;
 
-    while (type) {
-        if (type == target_type) {
-            return class;
-        }
+    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
+        int found = 0;
+        GSList *i;
 
-        type = type_get_parent(type);
+        for (i = class->interfaces; i; i = i->next) {
+            ObjectClass *target_class = i->data;
+
+            if (type_is_ancestor(target_class->type, target_type)) {
+                ret = target_class;
+                found++;
+            }
+         }
+
+        /* The match was ambiguous, don't allow a cast */
+        if (found > 1) {
+            ret = NULL;
+        }
+    } else if (type_is_ancestor(type, target_type)) {
+        ret = class;
     }
 
-    return NULL;
+    return ret;
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
@@ -920,12 +882,6 @@ void object_property_add_child(Object *obj, const char *name,
 {
     gchar *type;
 
-    /* Registering an interface object in the composition tree will mightily
-     * confuse object_get_canonical_path (which, on the other hand, knows how
-     * to get the canonical path of an interface object).
-     */
-    assert(!object_is_type(obj, type_interface));
-
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     object_property_add(obj, name, type, object_get_child_property,
@@ -1022,10 +978,6 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
-        obj = INTERFACE(obj)->obj;
-    }
-
     while (obj != root) {
         ObjectProperty *prop = NULL;
 
@@ -1246,7 +1198,7 @@ static void register_types(void)
 {
     static TypeInfo interface_info = {
         .name = TYPE_INTERFACE,
-        .instance_size = sizeof(Interface),
+        .class_size = sizeof(InterfaceClass),
         .abstract = true,
     };
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Paolo Bonzini]

Il 08/08/2012 01:46, Peter Crosthwaite ha scritto:
> Hi All,
> 
> We seem to be having difficulty getting a review/merge on this patch. I
> have sent two series, two pings and a PULL, with only a single reply
> from P. Maydell asking for other reviewers to weigh in:
> 
> --------- on July 17 P. Maydell wrote -----------
> 
> I guess I should mention that I'm assuming somebody else is going to
> review
> this patchset. It looks generally OK to me but whether the stream API
> has the details of QOM interface use right is a bit out of my area
> of expertise.
> 
> -- PMM
> 
> ------------------------------------------------
> 
> What needs to happen (and what can I do) to make this series go through?
> Edgar has reviewed P2 this morning and I have already remade the series,
> but my primary concern is this patch, considering it touches the core
> QOM infrastructure.
> 
> I have tested this patch on target i386 with no obvious breakages along
> with testing on the the embedded platforms we work with (Zynq and MB).

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

:)

Paolo

Re: [Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Peter Crosthwaite]

Hi All,

We seem to be having difficulty getting a review/merge on this patch. I
have sent two series, two pings and a PULL, with only a single reply
from P. Maydell asking for other reviewers to weigh in:

--------- on July 17 P. Maydell wrote -----------

I guess I should mention that I'm assuming somebody else is going to
review
this patchset. It looks generally OK to me but whether the stream API
has the details of QOM interface use right is a bit out of my area
of expertise.

-- PMM

------------------------------------------------

What needs to happen (and what can I do) to make this series go through?
Edgar has reviewed P2 this morning and I have already remade the series,
but my primary concern is this patch, considering it touches the core
QOM infrastructure.

I have tested this patch on target i386 with no obvious breakages along
with testing on the the embedded platforms we work with (Zynq and MB).

Regards,
Peter

On Mon, 2012-08-06 at 13:07 +1000, Peter A. G. Crosthwaite wrote:
> From: Anthony Liguori <aliguori@us.ibm.com>
> 
> The current implementation of Interfaces is poorly designed.  Each interface
> that an object implements ends up being an object that's tracked by the
> implementing object.  There's all sorts of gymnastics to deal with casting
> between these objects.
> 
> But an interface shouldn't be associated with an Object.  Interfaces are global
> to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
> the relationship between Object and Interfaces.
> 
> Interfaces are now abstract (as they should be) but this is okay.  Interfaces
> essentially act as additional parents for the classes and are treated as such.
> 
> With this new implementation, we should fully support derived interfaces
> including reimplementing an inherited interface.
> 
> PC: Rebased against qom-next merge Jun-2012.
> 
> PC: Removed replication of cast logic for interfaces, i.e. there is only
> one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  include/qemu/object.h |   46 +++++++----
>  qom/object.c          |  220 +++++++++++++++++++------------------------------
>  2 files changed, 116 insertions(+), 150 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index 8b17776..cc75fee 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -239,6 +239,7 @@ struct ObjectClass
>  {
>      /*< private >*/
>      Type type;
> +    GSList *interfaces;
>  };
>  
>  /**
> @@ -260,7 +261,6 @@ struct Object
>  {
>      /*< private >*/
>      ObjectClass *class;
> -    GSList *interfaces;
>      QTAILQ_HEAD(, ObjectProperty) properties;
>      uint32_t ref;
>      Object *parent;
> @@ -387,6 +387,16 @@ struct TypeInfo
>      OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
>  
>  /**
> + * InterfaceInfo:
> + * @type: The name of the interface.
> + *
> + * The information associated with an interface.
> + */
> +struct InterfaceInfo {
> +    const char *type;
> +};
> +
> +/**
>   * InterfaceClass:
>   * @parent_class: the base class
>   *
> @@ -396,26 +406,30 @@ struct TypeInfo
>  struct InterfaceClass
>  {
>      ObjectClass parent_class;
> +    /*< private >*/
> +    ObjectClass *concrete_class;
>  };
>  
> +#define TYPE_INTERFACE "interface"
> +
>  /**
> - * InterfaceInfo:
> - * @type: The name of the interface.
> - * @interface_initfn: This method is called during class initialization and is
> - *   used to initialize an interface associated with a class.  This function
> - *   should initialize any default virtual functions for a class and/or override
> - *   virtual functions in a parent class.
> - *
> - * The information associated with an interface.
> + * INTERFACE_CLASS:
> + * @klass: class to cast from
> + * Returns: An #InterfaceClass or raise an error if cast is invalid
>   */
> -struct InterfaceInfo
> -{
> -    const char *type;
> +#define INTERFACE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
>  
> -    void (*interface_initfn)(ObjectClass *class, void *data);
> -};
> -
> -#define TYPE_INTERFACE "interface"
> +/**
> + * INTERFACE_CHECK:
> + * @interface: the type to return
> + * @obj: the object to convert to an interface
> + * @name: the interface type name
> + *
> + * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
> + */
> +#define INTERFACE_CHECK(interface, obj, name) \
> +    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
>  
>  /**
>   * object_new:
> diff --git a/qom/object.c b/qom/object.c
> index 00bb3b0..a552be2 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
>  
>  struct InterfaceImpl
>  {
> -    const char *parent;
> -    void (*interface_initfn)(ObjectClass *class, void *data);
> -    TypeImpl *type;
> +    const char *typename;
>  };
>  
>  struct TypeImpl
> @@ -64,14 +62,6 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> -typedef struct Interface
> -{
> -    Object parent;
> -    Object *obj;
> -} Interface;
> -
> -#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
> -
>  static Type type_interface;
>  
>  static GHashTable *type_table_get(void)
> @@ -98,6 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
>  static TypeImpl *type_register_internal(const TypeInfo *info)
>  {
>      TypeImpl *ti = g_malloc0(sizeof(*ti));
> +    int i;
>  
>      g_assert(info->name != NULL);
>  
> @@ -122,15 +113,10 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
>  
>      ti->abstract = info->abstract;
>  
> -    if (info->interfaces) {
> -        int i;
> -
> -        for (i = 0; info->interfaces[i].type; i++) {
> -            ti->interfaces[i].parent = info->interfaces[i].type;
> -            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
> -            ti->num_interfaces++;
> -        }
> +    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
> +        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
>      }
> +    ti->num_interfaces = i;
>  
>      type_table_add(ti);
>  
> @@ -198,26 +184,48 @@ static size_t type_object_get_size(TypeImpl *ti)
>      return 0;
>  }
>  
> -static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
> +static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
>  {
> -    TypeInfo info = {
> -        .instance_size = sizeof(Interface),
> -        .parent = iface->parent,
> -        .class_size = sizeof(InterfaceClass),
> -        .class_init = iface->interface_initfn,
> -        .abstract = true,
> -    };
> -    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
> +    assert(target_type);
> +
> +    /* Check if typename is a direct ancestor of type */
> +    while (type) {
> +        if (type == target_type) {
> +            return true;
> +        }
>  
> -    info.name = name;
> -    iface->type = type_register_internal(&info);
> -    g_free(name);
> +        type = type_get_parent(type);
> +    }
> +
> +    return false;
> +}
> +
> +static void type_initialize(TypeImpl *ti);
> +
> +static void type_initialize_interface(TypeImpl *ti, const char *parent)
> +{
> +    InterfaceClass *new_iface;
> +    TypeInfo info = { };
> +    TypeImpl *iface_impl;
> +
> +    info.parent = parent;
> +    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
> +    info.abstract = true;
> +
> +    iface_impl = type_register(&info);
> +    type_initialize(iface_impl);
> +    g_free((char *)info.name);
> +
> +    new_iface = (InterfaceClass *)iface_impl->class;
> +    new_iface->concrete_class = ti->class;
> +
> +    ti->class->interfaces = g_slist_append(ti->class->interfaces,
> +                                           iface_impl->class);
>  }
>  
>  static void type_initialize(TypeImpl *ti)
>  {
>      TypeImpl *parent;
> -    int i;
>  
>      if (ti->class) {
>          return;
> @@ -231,9 +239,33 @@ static void type_initialize(TypeImpl *ti)
>      parent = type_get_parent(ti);
>      if (parent) {
>          type_initialize(parent);
> +        GSList *e;
> +        int i;
>  
>          g_assert(parent->class_size <= ti->class_size);
>          memcpy(ti->class, parent->class, parent->class_size);
> +
> +        for (e = parent->class->interfaces; e; e = e->next) {
> +            ObjectClass *iface = e->data;
> +            type_initialize_interface(ti, object_class_get_name(iface));
> +        }
> +
> +        for (i = 0; i < ti->num_interfaces; i++) {
> +            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
> +            for (e = ti->class->interfaces; e; e = e->next) {
> +                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
> +
> +                if (type_is_ancestor(target_type, t)) {
> +                    break;
> +                }
> +            }
> +
> +            if (e) {
> +                continue;
> +            }
> +
> +            type_initialize_interface(ti, ti->interfaces[i].typename);
> +        }
>      }
>  
>      ti->class->type = ti;
> @@ -245,38 +277,19 @@ static void type_initialize(TypeImpl *ti)
>          parent = type_get_parent(parent);
>      }
>  
> -    for (i = 0; i < ti->num_interfaces; i++) {
> -        type_class_interface_init(ti, &ti->interfaces[i]);
> -    }
> -
>      if (ti->class_init) {
>          ti->class_init(ti->class, ti->class_data);
>      }
> -}
>  
> -static void object_interface_init(Object *obj, InterfaceImpl *iface)
> -{
> -    TypeImpl *ti = iface->type;
> -    Interface *iface_obj;
> -
> -    iface_obj = INTERFACE(object_new(ti->name));
> -    iface_obj->obj = obj;
>  
> -    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
>  }
>  
>  static void object_init_with_type(Object *obj, TypeImpl *ti)
>  {
> -    int i;
> -
>      if (type_has_parent(ti)) {
>          object_init_with_type(obj, type_get_parent(ti));
>      }
>  
> -    for (i = 0; i < ti->num_interfaces; i++) {
> -        object_interface_init(obj, &ti->interfaces[i]);
> -    }
> -
>      if (ti->instance_init) {
>          ti->instance_init(obj);
>      }
> @@ -357,12 +370,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
>          type->instance_finalize(obj);
>      }
>  
> -    while (obj->interfaces) {
> -        Interface *iface_obj = obj->interfaces->data;
> -        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
> -        object_delete(OBJECT(iface_obj));
> -    }
> -
>      if (type_has_parent(type)) {
>          object_deinit(obj, type_get_parent(type));
>      }
> @@ -409,74 +416,15 @@ void object_delete(Object *obj)
>      g_free(obj);
>  }
>  
> -static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
> -{
> -    assert(target_type);
> -
> -    /* Check if typename is a direct ancestor of type */
> -    while (type) {
> -        if (type == target_type) {
> -            return true;
> -        }
> -
> -        type = type_get_parent(type);
> -    }
> -
> -    return false;
> -}
> -
> -static bool object_is_type(Object *obj, TypeImpl *target_type)
> -{
> -    return !target_type || type_is_ancestor(obj->class->type, target_type);
> -}
> -
>  Object *object_dynamic_cast(Object *obj, const char *typename)
>  {
> -    TypeImpl *target_type = type_get_by_name(typename);
> -    GSList *i;
> -
> -    /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
> -     * we want to go back from interfaces to the parent.
> -    */
> -    if (target_type && object_is_type(obj, target_type)) {
> +    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
>          return obj;
>      }
>  
> -    /* Check if obj is an interface and its containing object is a direct
> -     * ancestor of typename.  In principle we could do this test at the very
> -     * beginning of object_dynamic_cast, avoiding a second call to
> -     * object_is_type.  However, casting between interfaces is relatively
> -     * rare, and object_is_type(obj, type_interface) would fail almost always.
> -     *
> -     * Perhaps we could add a magic value to the object header for increased
> -     * (run-time) type safety and to speed up tests like this one.  If we ever
> -     * do that we can revisit the order here.
> -     */
> -    if (object_is_type(obj, type_interface)) {
> -        assert(!obj->interfaces);
> -        obj = INTERFACE(obj)->obj;
> -        if (object_is_type(obj, target_type)) {
> -            return obj;
> -        }
> -    }
> -
> -    if (!target_type) {
> -        return obj;
> -    }
> -
> -    /* Check if obj has an interface of typename */
> -    for (i = obj->interfaces; i; i = i->next) {
> -        Interface *iface = i->data;
> -
> -        if (object_is_type(OBJECT(iface), target_type)) {
> -            return OBJECT(iface);
> -        }
> -    }
> -
>      return NULL;
>  }
>  
> -
>  Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>  {
>      Object *inst;
> @@ -497,16 +445,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>  {
>      TypeImpl *target_type = type_get_by_name(typename);
>      TypeImpl *type = class->type;
> +    ObjectClass *ret = NULL;
>  
> -    while (type) {
> -        if (type == target_type) {
> -            return class;
> -        }
> +    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
> +        int found = 0;
> +        GSList *i;
>  
> -        type = type_get_parent(type);
> +        for (i = class->interfaces; i; i = i->next) {
> +            ObjectClass *target_class = i->data;
> +
> +            if (type_is_ancestor(target_class->type, target_type)) {
> +                ret = target_class;
> +                found++;
> +            }
> +         }
> +
> +        /* The match was ambiguous, don't allow a cast */
> +        if (found > 1) {
> +            ret = NULL;
> +        }
> +    } else if (type_is_ancestor(type, target_type)) {
> +        ret = class;
>      }
>  
> -    return NULL;
> +    return ret;
>  }
>  
>  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
> @@ -920,12 +882,6 @@ void object_property_add_child(Object *obj, const char *name,
>  {
>      gchar *type;
>  
> -    /* Registering an interface object in the composition tree will mightily
> -     * confuse object_get_canonical_path (which, on the other hand, knows how
> -     * to get the canonical path of an interface object).
> -     */
> -    assert(!object_is_type(obj, type_interface));
> -
>      type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
>  
>      object_property_add(obj, name, type, object_get_child_property,
> @@ -1022,10 +978,6 @@ gchar *object_get_canonical_path(Object *obj)
>      Object *root = object_get_root();
>      char *newpath = NULL, *path = NULL;
>  
> -    if (object_is_type(obj, type_interface)) {
> -        obj = INTERFACE(obj)->obj;
> -    }
> -
>      while (obj != root) {
>          ObjectProperty *prop = NULL;
>  
> @@ -1246,7 +1198,7 @@ static void register_types(void)
>  {
>      static TypeInfo interface_info = {
>          .name = TYPE_INTERFACE,
> -        .instance_size = sizeof(Interface),
> +        .class_size = sizeof(InterfaceClass),
>          .abstract = true,
>      };
>  

[Qemu-devel] [PATCH 1/2] qom: Reimplement Interfaces

[Peter A. G. Crosthwaite]

From: Anthony Liguori <aliguori@us.ibm.com>

The current implementation of Interfaces is poorly designed.  Each interface
that an object implements ends up being an object that's tracked by the
implementing object.  There's all sorts of gymnastics to deal with casting
between these objects.

But an interface shouldn't be associated with an Object.  Interfaces are global
to a class.  This patch moves all Interface knowledge to ObjectClass eliminating
the relationship between Object and Interfaces.

Interfaces are now abstract (as they should be) but this is okay.  Interfaces
essentially act as additional parents for the classes and are treated as such.

With this new implementation, we should fully support derived interfaces
including reimplementing an inherited interface.

PC: Rebased against qom-next merge Jun-2012.

PC: Removed replication of cast logic for interfaces, i.e. there is only
one cast function - object_dynamic_cast() (and object_dynamic_cast_assert())

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 include/qemu/object.h |   46 +++++++----
 qom/object.c          |  220 +++++++++++++++++++------------------------------
 2 files changed, 116 insertions(+), 150 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index 8b17776..cc75fee 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,7 @@ struct ObjectClass
 {
     /*< private >*/
     Type type;
+    GSList *interfaces;
 };
 
 /**
@@ -260,7 +261,6 @@ struct Object
 {
     /*< private >*/
     ObjectClass *class;
-    GSList *interfaces;
     QTAILQ_HEAD(, ObjectProperty) properties;
     uint32_t ref;
     Object *parent;
@@ -387,6 +387,16 @@ struct TypeInfo
     OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
 
 /**
+ * InterfaceInfo:
+ * @type: The name of the interface.
+ *
+ * The information associated with an interface.
+ */
+struct InterfaceInfo {
+    const char *type;
+};
+
+/**
  * InterfaceClass:
  * @parent_class: the base class
  *
@@ -396,26 +406,30 @@ struct TypeInfo
 struct InterfaceClass
 {
     ObjectClass parent_class;
+    /*< private >*/
+    ObjectClass *concrete_class;
 };
 
+#define TYPE_INTERFACE "interface"
+
 /**
- * InterfaceInfo:
- * @type: The name of the interface.
- * @interface_initfn: This method is called during class initialization and is
- *   used to initialize an interface associated with a class.  This function
- *   should initialize any default virtual functions for a class and/or override
- *   virtual functions in a parent class.
- *
- * The information associated with an interface.
+ * INTERFACE_CLASS:
+ * @klass: class to cast from
+ * Returns: An #InterfaceClass or raise an error if cast is invalid
  */
-struct InterfaceInfo
-{
-    const char *type;
+#define INTERFACE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(InterfaceClass, klass, TYPE_INTERFACE)
 
-    void (*interface_initfn)(ObjectClass *class, void *data);
-};
-
-#define TYPE_INTERFACE "interface"
+/**
+ * INTERFACE_CHECK:
+ * @interface: the type to return
+ * @obj: the object to convert to an interface
+ * @name: the interface type name
+ *
+ * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
+ */
+#define INTERFACE_CHECK(interface, obj, name) \
+    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
 
 /**
  * object_new:
diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..a552be2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -31,9 +31,7 @@ typedef struct TypeImpl TypeImpl;
 
 struct InterfaceImpl
 {
-    const char *parent;
-    void (*interface_initfn)(ObjectClass *class, void *data);
-    TypeImpl *type;
+    const char *typename;
 };
 
 struct TypeImpl
@@ -64,14 +62,6 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-typedef struct Interface
-{
-    Object parent;
-    Object *obj;
-} Interface;
-
-#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
-
 static Type type_interface;
 
 static GHashTable *type_table_get(void)
@@ -98,6 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
 static TypeImpl *type_register_internal(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
+    int i;
 
     g_assert(info->name != NULL);
 
@@ -122,15 +113,10 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
 
     ti->abstract = info->abstract;
 
-    if (info->interfaces) {
-        int i;
-
-        for (i = 0; info->interfaces[i].type; i++) {
-            ti->interfaces[i].parent = info->interfaces[i].type;
-            ti->interfaces[i].interface_initfn = info->interfaces[i].interface_initfn;
-            ti->num_interfaces++;
-        }
+    for (i = 0; info->interfaces && info->interfaces[i].type; i++) {
+        ti->interfaces[i].typename = g_strdup(info->interfaces[i].type);
     }
+    ti->num_interfaces = i;
 
     type_table_add(ti);
 
@@ -198,26 +184,48 @@ static size_t type_object_get_size(TypeImpl *ti)
     return 0;
 }
 
-static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
+static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
 {
-    TypeInfo info = {
-        .instance_size = sizeof(Interface),
-        .parent = iface->parent,
-        .class_size = sizeof(InterfaceClass),
-        .class_init = iface->interface_initfn,
-        .abstract = true,
-    };
-    char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
+    assert(target_type);
+
+    /* Check if typename is a direct ancestor of type */
+    while (type) {
+        if (type == target_type) {
+            return true;
+        }
 
-    info.name = name;
-    iface->type = type_register_internal(&info);
-    g_free(name);
+        type = type_get_parent(type);
+    }
+
+    return false;
+}
+
+static void type_initialize(TypeImpl *ti);
+
+static void type_initialize_interface(TypeImpl *ti, const char *parent)
+{
+    InterfaceClass *new_iface;
+    TypeInfo info = { };
+    TypeImpl *iface_impl;
+
+    info.parent = parent;
+    info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
+    info.abstract = true;
+
+    iface_impl = type_register(&info);
+    type_initialize(iface_impl);
+    g_free((char *)info.name);
+
+    new_iface = (InterfaceClass *)iface_impl->class;
+    new_iface->concrete_class = ti->class;
+
+    ti->class->interfaces = g_slist_append(ti->class->interfaces,
+                                           iface_impl->class);
 }
 
 static void type_initialize(TypeImpl *ti)
 {
     TypeImpl *parent;
-    int i;
 
     if (ti->class) {
         return;
@@ -231,9 +239,33 @@ static void type_initialize(TypeImpl *ti)
     parent = type_get_parent(ti);
     if (parent) {
         type_initialize(parent);
+        GSList *e;
+        int i;
 
         g_assert(parent->class_size <= ti->class_size);
         memcpy(ti->class, parent->class, parent->class_size);
+
+        for (e = parent->class->interfaces; e; e = e->next) {
+            ObjectClass *iface = e->data;
+            type_initialize_interface(ti, object_class_get_name(iface));
+        }
+
+        for (i = 0; i < ti->num_interfaces; i++) {
+            TypeImpl *t = type_get_by_name(ti->interfaces[i].typename);
+            for (e = ti->class->interfaces; e; e = e->next) {
+                TypeImpl *target_type = OBJECT_CLASS(e->data)->type;
+
+                if (type_is_ancestor(target_type, t)) {
+                    break;
+                }
+            }
+
+            if (e) {
+                continue;
+            }
+
+            type_initialize_interface(ti, ti->interfaces[i].typename);
+        }
     }
 
     ti->class->type = ti;
@@ -245,38 +277,19 @@ static void type_initialize(TypeImpl *ti)
         parent = type_get_parent(parent);
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        type_class_interface_init(ti, &ti->interfaces[i]);
-    }
-
     if (ti->class_init) {
         ti->class_init(ti->class, ti->class_data);
     }
-}
 
-static void object_interface_init(Object *obj, InterfaceImpl *iface)
-{
-    TypeImpl *ti = iface->type;
-    Interface *iface_obj;
-
-    iface_obj = INTERFACE(object_new(ti->name));
-    iface_obj->obj = obj;
 
-    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
 
 static void object_init_with_type(Object *obj, TypeImpl *ti)
 {
-    int i;
-
     if (type_has_parent(ti)) {
         object_init_with_type(obj, type_get_parent(ti));
     }
 
-    for (i = 0; i < ti->num_interfaces; i++) {
-        object_interface_init(obj, &ti->interfaces[i]);
-    }
-
     if (ti->instance_init) {
         ti->instance_init(obj);
     }
@@ -357,12 +370,6 @@ static void object_deinit(Object *obj, TypeImpl *type)
         type->instance_finalize(obj);
     }
 
-    while (obj->interfaces) {
-        Interface *iface_obj = obj->interfaces->data;
-        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
-        object_delete(OBJECT(iface_obj));
-    }
-
     if (type_has_parent(type)) {
         object_deinit(obj, type_get_parent(type));
     }
@@ -409,74 +416,15 @@ void object_delete(Object *obj)
     g_free(obj);
 }
 
-static bool type_is_ancestor(TypeImpl *type, TypeImpl *target_type)
-{
-    assert(target_type);
-
-    /* Check if typename is a direct ancestor of type */
-    while (type) {
-        if (type == target_type) {
-            return true;
-        }
-
-        type = type_get_parent(type);
-    }
-
-    return false;
-}
-
-static bool object_is_type(Object *obj, TypeImpl *target_type)
-{
-    return !target_type || type_is_ancestor(obj->class->type, target_type);
-}
-
 Object *object_dynamic_cast(Object *obj, const char *typename)
 {
-    TypeImpl *target_type = type_get_by_name(typename);
-    GSList *i;
-
-    /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
-     * we want to go back from interfaces to the parent.
-    */
-    if (target_type && object_is_type(obj, target_type)) {
+    if (object_class_dynamic_cast(object_get_class(obj), typename)) {
         return obj;
     }
 
-    /* Check if obj is an interface and its containing object is a direct
-     * ancestor of typename.  In principle we could do this test at the very
-     * beginning of object_dynamic_cast, avoiding a second call to
-     * object_is_type.  However, casting between interfaces is relatively
-     * rare, and object_is_type(obj, type_interface) would fail almost always.
-     *
-     * Perhaps we could add a magic value to the object header for increased
-     * (run-time) type safety and to speed up tests like this one.  If we ever
-     * do that we can revisit the order here.
-     */
-    if (object_is_type(obj, type_interface)) {
-        assert(!obj->interfaces);
-        obj = INTERFACE(obj)->obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
-    if (!target_type) {
-        return obj;
-    }
-
-    /* Check if obj has an interface of typename */
-    for (i = obj->interfaces; i; i = i->next) {
-        Interface *iface = i->data;
-
-        if (object_is_type(OBJECT(iface), target_type)) {
-            return OBJECT(iface);
-        }
-    }
-
     return NULL;
 }
 
-
 Object *object_dynamic_cast_assert(Object *obj, const char *typename)
 {
     Object *inst;
@@ -497,16 +445,30 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 {
     TypeImpl *target_type = type_get_by_name(typename);
     TypeImpl *type = class->type;
+    ObjectClass *ret = NULL;
 
-    while (type) {
-        if (type == target_type) {
-            return class;
-        }
+    if (type->num_interfaces && type_is_ancestor(target_type, type_interface)) {
+        int found = 0;
+        GSList *i;
 
-        type = type_get_parent(type);
+        for (i = class->interfaces; i; i = i->next) {
+            ObjectClass *target_class = i->data;
+
+            if (type_is_ancestor(target_class->type, target_type)) {
+                ret = target_class;
+                found++;
+            }
+         }
+
+        /* The match was ambiguous, don't allow a cast */
+        if (found > 1) {
+            ret = NULL;
+        }
+    } else if (type_is_ancestor(type, target_type)) {
+        ret = class;
     }
 
-    return NULL;
+    return ret;
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
@@ -920,12 +882,6 @@ void object_property_add_child(Object *obj, const char *name,
 {
     gchar *type;
 
-    /* Registering an interface object in the composition tree will mightily
-     * confuse object_get_canonical_path (which, on the other hand, knows how
-     * to get the canonical path of an interface object).
-     */
-    assert(!object_is_type(obj, type_interface));
-
     type = g_strdup_printf("child<%s>", object_get_typename(OBJECT(child)));
 
     object_property_add(obj, name, type, object_get_child_property,
@@ -1022,10 +978,6 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
-        obj = INTERFACE(obj)->obj;
-    }
-
     while (obj != root) {
         ObjectProperty *prop = NULL;
 
@@ -1246,7 +1198,7 @@ static void register_types(void)
 {
     static TypeInfo interface_info = {
         .name = TYPE_INTERFACE,
-        .instance_size = sizeof(Interface),
+        .class_size = sizeof(InterfaceClass),
         .abstract = true,
     };
 
-- 
1.7.0.4