[Qemu-devel] [RFC v0 0/8] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Qemu-devel] [RFC v0 0/8] QOMify AXI stream for Xilinx AXI ethernet/DMA

[Peter A. G. Crosthwaite]

This series builds on Paolos work-in-progress on the Xilinx IP to get it all working and get the interface generalised to AXI-stream.

I have cc'd everyone from the RFC i put out for AXI stream, for most the patch of interest are P2 and P8, which together are the QOMification of AXI stream. Paolos original work (P2) was pritty much what Andreas proposed as the solution, so all I did was fix the Names (P8).

The series is a preliminary "quick and dirty" RFC (so i called it v0). It is not bisectable as several bugfixes are created as individual patches. This is for ease of review and to track the diff between Paolos and my branch. Ill remake as atomic patches in V1.

First two patches are cherry picked from Paolos tree.

P1 is a refactoring of the QOM Interface system
P2 sets up the xilinx IP to use Interfaces rather than PROP_PTRs

P3 is the type registration for the interface type (missing from P2)
P4 is a workaround for a bug that needs to be resolved (with canonical paths or objects)
P5 is a bugfix on the interface system. should fold into P1 (i think?)
P6 is a workaround for a bug in the object link system. Needs some attention
P7 is a typo fix for P2
P8 changes the names of all the types etc to better match AXI stream

Paolo Bonzini (2):
  qom: revamp interfaces
  xilinx: remove PROP_PTR properties

Peter A. G. Crosthwaite (6):
  xilinx_axidma: Added missing TypeInfo
  object: create default canonical paths for orphans
  object: make interfaces concrete
  xilinx  dont cast to interface types with links
  petalogix_ml605_mmu: fixed qdev create for dma
  axidma: renamed interconnect to axi-stream

 hw/axi-stream.c             |   15 ++++++
 hw/axi-stream.h             |   35 ++++++++++++++
 hw/microblaze/Makefile.objs |    1 +
 hw/petalogix_ml605_mmu.c    |   18 ++++----
 hw/xilinx.h                 |   22 ++++-----
 hw/xilinx_axidma.c          |   45 ++++++++++++------
 hw/xilinx_axidma.h          |   39 ---------------
 hw/xilinx_axienet.c         |   37 ++++++++++-----
 include/qemu/object.h       |  110 +++++++++++++++++++++++++++++++++++++++++++
 qom/object.c                |   23 +++++----
 10 files changed, 247 insertions(+), 98 deletions(-)
 create mode 100644 hw/axi-stream.c
 create mode 100644 hw/axi-stream.h
 delete mode 100644 hw/xilinx_axidma.h

-- 
1.7.3.2

[Qemu-devel] [RFC v0 1/8] qom: revamp interfaces

[Peter A. G. Crosthwaite]

From: Paolo Bonzini <pbonzini@redhat.com>

This commit is composed of several changes:

1) it provides a complete set of macros that serve as building blocks
for the definition of Interface types.

2) make Interface public.  This is because this type serves as the
base class for the actual implementation objects that are returned
by dynamic_cast.  While the type itself need not be subclassed by
clients and in principle could remain opaque, it is useful in helper
macros.

3) Change the definition of Interface, prefixing field names with
"iface_" so that the macros can perform a limited a amount of static
type checking.

4) Add documentation for interfaces.

It doesn't change the interface SList to a type-safe qemu-queue.h
list.  This can be left for later.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter A.G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 include/qemu/object.h |  110 +++++++++++++++++++++++++++++++++++++++++++++++++
 qom/object.c          |   14 ++----
 2 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index d93b772..a82be5f 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -186,6 +186,40 @@ typedef struct InterfaceInfo InterfaceInfo;
  * similar to normal types except for the fact that are only defined by
  * their classes and never carry any state.  You can dynamically cast an object
  * to one of its #Interface types and vice versa.
+ *
+ * Interfaces are special in that a cast from or to an interface type returns
+ * a different object pointer than what you pass.  Interface methods typically
+ * pass the whole object as the first argument, rather than the interface.
+ *
+ * <example>
+ *   <title>Defining an interface</title>
+ *   <programlisting>
+ * // An interface has the same basic pieces as a class.
+ * #define TYPE_DMA_PEER "dma-peer"
+ * typedef Interface DMAPeer;
+ * typedef struct DMAPeerIface DMAPeerIface;
+ * struct DMAPeerIface {
+ *     InterfaceClass parent;
+ *     void (*push)(Object *obj, int channel, uint8_t *buf, size_t len);
+ * };
+ *
+ * // Interfaces also define utility macros for casting.
+ * #define DMA_PEER_GET_IFACE(obj) \
+ *    INTERFACE_GET_IFACE(DMAPeerIface, obj, TYPE_DMA_PEER)
+ * #define DMA_PEER_IFACE(klass) \
+ *    INTERFACE_IFACE_CHECK(DMAPeerIface, klass, TYPE_DMA_PEER)
+ * #define DMA_PEER(obj) \
+ *    INTERFACE_CHECK(obj, TYPE_DMA_PEER)
+ *
+ * // This is the wrapper that calls the method.
+ * void dma_push(DMAPeer *peer, int channel, uint8_t *buf, size_t len)
+ * {
+ *     DMAPeerIface *iface = DMA_PEER_GET_IFACE(peer);
+ *
+ *     peer->push(INTERFACE_OBJECT(peer), channel, buf, len);
+ * }
+ *    </programlisting>
+ *  </example>
  */
 
 
@@ -392,6 +426,82 @@ struct InterfaceClass
     ObjectClass parent_class;
 };
 
+typedef struct Interface
+{
+    Object iface_parent;
+    Object *iface_obj;
+} Interface;
+
+/**
+ * INTERFACE_CLASS:
+ * @class: A derivative of #InterfaceClass.
+ *
+ * Converts a class to an #InterfaceClass.  Right now it really has the
+ * same layout as #ObjectClass, so this function will always succeed.
+ * This may change in the future.
+ */
+#define INTERFACE_CLASS(class) \
+    ((InterfaceClass *)(class))
+
+/**
+ * INTERFACE_GET_CLASS:
+ * @class: An interface object (an instance of Interface)
+ *
+ * Retrieve the class object (vtable) of the implementation object for an
+ * interface.  Since all such objects have the same layout, this function
+ * can be statically type-checked and will always succeed.  However, note
+ * that the result is typically passed to an OBJECT_CLASS_CHECK macro,
+ * which actually may fail.
+ */
+#define INTERFACE_GET_CLASS(iface_obj) \
+    INTERFACE_CLASS(object_get_class(&(iface_obj)->iface_parent))
+
+/**
+ * INTERFACE_IFACE_CHECK:
+ * @class: The C type to use for the return value.
+ * @obj: A derivative of @type to cast.
+ * @name: the QOM typename of @class.
+ *
+ * A type safe version of @object_class_dynamic_cast_assert.  This macro is
+ * typically wrapped by each type to perform type safe casts of an interface
+ * to a specific type.
+ */
+#define INTERFACE_IFACE_CHECK(class, obj, name) \
+    OBJECT_CLASS_CHECK(class, obj, name)
+
+/**
+ * INTERFACE_GET_IFACE:
+ * @class: An interface object (an instance of Interface)
+ *
+ * Retrieve the class object (vtable) of the implementation object for an
+ * interface.  This macro is typically wrapped by each type to safely
+ * retrieve a specific vtable from an implementation object.
+ */
+#define INTERFACE_GET_IFACE(iface, obj, type) \
+     INTERFACE_IFACE_CHECK(iface, INTERFACE_GET_CLASS(obj), type)
+
+/**
+ * INTERFACE_CHECK:
+ * @obj: The object to obtain the class for.
+ * @name: The QOM typename of @obj.
+ *
+ * This function will return the specific implementation object for a given
+ * interface.  It's generally used by each type to provide a type safe macro
+ * to get a specific class type from an object.
+ */
+#define INTERFACE_CHECK(obj, name) OBJECT_CHECK(Interface, obj, name)
+
+/**
+ * INTERFACE_OBJECT:
+ * @impl: The interface implementation to obtain the parent object for.
+ *
+ * This function will return the parent object associated to any object
+ * that is an interface implementation.  It expects a pointer to Interface.
+ * Since all interface objects have the same layout, this function can be
+ * statically type-checked and will always succeed.
+ */
+#define INTERFACE_OBJECT(impl)      ((impl)->iface_obj)
+
 /**
  * InterfaceInfo:
  * @type: The name of the interface.
diff --git a/qom/object.c b/qom/object.c
index 6f839ad..e6c3cfb 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -63,13 +63,7 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-typedef struct Interface
-{
-    Object parent;
-    Object *obj;
-} Interface;
-
-#define INTERFACE(obj) OBJECT_CHECK(Interface, obj, TYPE_INTERFACE)
+#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
 
 static Type type_interface;
 
@@ -251,7 +245,7 @@ static void object_interface_init(Object *obj, InterfaceImpl *iface)
     Interface *iface_obj;
 
     iface_obj = INTERFACE(object_new(ti->name));
-    iface_obj->obj = obj;
+    iface_obj->iface_obj = obj;
 
     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
@@ -435,7 +429,7 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
      */
     if (object_is_type(obj, type_interface)) {
         assert(!obj->interfaces);
-        obj = INTERFACE(obj)->obj;
+        obj = INTERFACE(obj)->iface_obj;
         if (object_is_type(obj, target_type)) {
             return obj;
         }
@@ -986,7 +980,7 @@ gchar *object_get_canonical_path(Object *obj)
     char *newpath = NULL, *path = NULL;
 
     if (object_is_type(obj, type_interface)) {
-        obj = INTERFACE(obj)->obj;
+        obj = INTERFACE(obj)->iface_obj;
     }
 
     while (obj != root) {
-- 
1.7.3.2

[Qemu-devel] [RFC v0 2/8] xilinx: remove PROP_PTR properties

[Peter A. G. Crosthwaite]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/petalogix_ml605_mmu.c |   17 +++++++--------
 hw/xilinx.h              |   22 ++++++++-----------
 hw/xilinx_axidma.c       |   39 +++++++++++++++++++++++-----------
 hw/xilinx_axidma.h       |   52 ++++++++++++++++++++-------------------------
 hw/xilinx_axienet.c      |   35 +++++++++++++++++++++---------
 5 files changed, 90 insertions(+), 75 deletions(-)

diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index bff63e3..37866f4 100644
--- a/hw/petalogix_ml605_mmu.c
+++ b/hw/petalogix_ml605_mmu.c
@@ -125,15 +125,14 @@ petalogix_ml605_init(ram_addr_t ram_size,
     /* 2 timers at irq 2 @ 100 Mhz.  */
     xilinx_timer_create(TIMER_BASEADDR, irq[2], 2, 100 * 1000000);
 
-    /* axi ethernet and dma initialization. TODO: Dynamically connect them.  */
-    {
-        static struct XilinxDMAConnection dmach;
-
-        xilinx_axiethernet_create(&dmach, &nd_table[0], 0x82780000,
-                                  irq[3], 0x1000, 0x1000);
-        xilinx_axiethernetdma_create(&dmach, 0x84600000,
-                                     irq[1], irq[0], 100 * 1000000);
-    }
+    /* axi ethernet and dma initialization. */
+    DeviceState *dma = qdev_create(NULL, "xilinx-axidma");
+    DeviceState *eth0;
+
+    eth0 = xilinx_axiethernet_create(&nd_table[0], XILINX_AXIDMA_PEER(dma),
+                                     0x82780000, irq[3], 0x1000, 0x1000);
+    xilinx_axiethernetdma_init(dma, XILINX_AXIDMA_PEER(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/xilinx.h b/hw/xilinx.h
index 35f35bd..5ccf3fa 100644
--- a/hw/xilinx.h
+++ b/hw/xilinx.h
@@ -1,3 +1,4 @@
+#include "xilinx_axidma.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, XilinxAXIDMAPeer *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, "c_rxmem", rxmem);
     qdev_prop_set_uint32(dev, "c_txmem", txmem);
-    qdev_prop_set_ptr(dev, "dmach", dmach);
+    object_property_set_link(OBJECT(dev), OBJECT(peer), "peer", 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, XilinxAXIDMAPeer *peer,
+                           target_phys_addr_t base, qemu_irq irq,
+                           qemu_irq irq2, int freqhz)
 {
-    DeviceState *dev = NULL;
-
-    dev = qdev_create(NULL, "xilinx,axidma");
     qdev_prop_set_uint32(dev, "freqhz", freqhz);
-    qdev_prop_set_ptr(dev, "dmach", dmach);
+    object_property_set_link(OBJECT(dev), OBJECT(peer), "peer", NULL);
     qdev_init_nofail(dev);
 
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq2);
     sysbus_connect_irq(sysbus_from_qdev(dev), 1, irq);
-
-    return dev;
 }
diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
index 85dfcbf..b82c02e 100644
--- a/hw/xilinx_axidma.c
+++ b/hw/xilinx_axidma.c
@@ -94,7 +94,7 @@ struct XilinxAXIDMA {
     SysBusDevice busdev;
     MemoryRegion iomem;
     uint32_t freqhz;
-    void *dmach;
+    XilinxAXIDMAPeer *peer;
 
     struct AXIStream streams[2];
 };
@@ -241,7 +241,7 @@ static void stream_complete(struct AXIStream *s)
 }
 
 static void stream_process_mem2s(struct AXIStream *s,
-                                 struct XilinxDMAConnection *dmach)
+                                 XilinxAXIDMAPeer *peer)
 {
     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);
+            xlx_dma_push(peer, txbuf, s->pos, app);
             s->pos = 0;
             stream_complete(s);
         }
@@ -352,9 +352,9 @@ static void stream_process_s2mem(struct AXIStream *s,
 }
 
 static
-void axidma_push(void *opaque, unsigned char *buf, size_t len, uint32_t *app)
+void axidma_push(Object *obj, unsigned char *buf, size_t len, uint32_t *app)
 {
-    struct XilinxAXIDMA *d = opaque;
+    struct XilinxAXIDMA *d = FROM_SYSBUS(typeof(*d), SYS_BUS_DEVICE(obj));
     struct AXIStream *s = &d->streams[1];
 
     if (!app) {
@@ -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->peer);
             }
             break;
         default:
@@ -466,12 +466,6 @@ static int xilinx_axidma_init(SysBusDevice *dev)
     sysbus_init_irq(dev, &s->streams[1].irq);
     sysbus_init_irq(dev, &s->streams[0].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,
                           "axidma", R_MAX * 4 * 2);
     sysbus_init_mmio(dev, &s->iomem);
@@ -486,9 +480,23 @@ 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, "peer", TYPE_XILINX_AXIDMA_PEER, 
+                             (Object **) &s->peer, NULL);
+}
+
+static void xilinx_axidma_peer_initfn(ObjectClass *klass, void *data)
+{
+    XilinxAXIDMAPeerIface *k = XILINX_AXIDMA_PEER_IFACE(klass);
+
+    k->push = axidma_push;
+}
+
 static Property axidma_properties[] = {
     DEFINE_PROP_UINT32("freqhz", struct XilinxAXIDMA, freqhz, 50000000),
-    DEFINE_PROP_PTR("dmach", struct XilinxAXIDMA, dmach),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -506,6 +514,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_XILINX_AXIDMA_PEER, xilinx_axidma_peer_initfn },
+	    { }
+    }
 };
 
 static void xilinx_axidma_register_types(void)
diff --git a/hw/xilinx_axidma.h b/hw/xilinx_axidma.h
index 37cb6f0..99c8371 100644
--- a/hw/xilinx_axidma.h
+++ b/hw/xilinx_axidma.h
@@ -1,39 +1,33 @@
+#ifndef XILINX_AXIDMA_H
+#define XILINX_AXIDMA_H 1
+
 /* AXI DMA connection. Used until qdev provides a generic way.  */
-typedef void (*DMAPushFn)(void *opaque,
-                            unsigned char *buf, size_t len, uint32_t *app);
+#define TYPE_XILINX_AXIDMA_PEER "xilinx-axidma-peer"
 
-struct XilinxDMAConnection {
-    void *dma;
-    void *client;
+#define XILINX_AXIDMA_PEER_IFACE(klass) \
+     OBJECT_CLASS_CHECK(XilinxAXIDMAPeerIface, klass, TYPE_XILINX_AXIDMA_PEER)
 
-    DMAPushFn to_dma;
-    DMAPushFn to_client;
-};
+/* This is usually done implicitly by object_set_link_property.  */
+#define XILINX_AXIDMA_PEER(obj) \
+     OBJECT_CHECK(XilinxAXIDMAPeer, obj, TYPE_XILINX_AXIDMA_PEER)
 
-static inline void xlx_dma_connect_client(struct XilinxDMAConnection *dmach,
-                                          void *c, DMAPushFn f)
-{
-    dmach->client = c;
-    dmach->to_client = f;
-}
+typedef Interface XilinxAXIDMAPeer;
+typedef struct XilinxAXIDMAPeerIface XilinxAXIDMAPeerIface;
 
-static inline void xlx_dma_connect_dma(struct XilinxDMAConnection *dmach,
-                                       void *d, DMAPushFn f)
-{
-    dmach->dma = d;
-    dmach->to_dma = f;
-}
+struct XilinxAXIDMAPeerIface {
+    InterfaceClass parent;
+
+    void (*push)(Object *obj, unsigned char *buf, size_t len, uint32_t *app);
+};
 
 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)
+void xlx_dma_push(XilinxAXIDMAPeer *peer,
+                  uint8_t *buf, size_t len, uint32_t *app)
 {
-    dmach->to_client(dmach->client, buf, len, app);
+    XilinxAXIDMAPeerIface *iface = container_of(INTERFACE_GET_CLASS(peer),
+                                                XilinxAXIDMAPeerIface,
+                                                parent);
+    iface->push(INTERFACE_OBJECT(peer), buf, len, app);
 }
 
+#endif
diff --git a/hw/xilinx_axienet.c b/hw/xilinx_axienet.c
index a25949b..a340cf2 100644
--- a/hw/xilinx_axienet.c
+++ b/hw/xilinx_axienet.c
@@ -310,7 +310,7 @@ struct XilinxAXIEnet {
     SysBusDevice busdev;
     MemoryRegion iomem;
     qemu_irq irq;
-    void *dmach;
+    XilinxAXIDMAPeer *peer;
     NICState *nic;
     NICConf conf;
 
@@ -773,7 +773,7 @@ static ssize_t eth_rx(VLANClientState *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);
+    xlx_dma_push(s->peer, (void *)s->rxmem, size, app);
 
     s->regs[R_IS] |= IS_RX_COMPLETE;
     enet_update_irq(s);
@@ -789,9 +789,9 @@ static void eth_cleanup(VLANClientState *nc)
 }
 
 static void
-axienet_stream_push(void *opaque, uint8_t *buf, size_t size, uint32_t *hdr)
+axienet_stream_push(Object *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)) {
@@ -845,12 +845,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);
 
@@ -870,11 +864,25 @@ 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, "peer", TYPE_XILINX_AXIDMA_PEER,
+                             (Object **) &s->peer, NULL);
+}
+
+static void xilinx_enet_peer_initfn(ObjectClass *klass, void *data)
+{
+    XilinxAXIDMAPeerIface *k = XILINX_AXIDMA_PEER_IFACE(klass);
+
+    k->push = axienet_stream_push;
+}
+
 static Property xilinx_enet_properties[] = {
     DEFINE_PROP_UINT32("phyaddr", struct XilinxAXIEnet, c_phyaddr, 7),
     DEFINE_PROP_UINT32("c_rxmem", struct XilinxAXIEnet, c_rxmem, 0x1000),
     DEFINE_PROP_UINT32("c_txmem", struct XilinxAXIEnet, c_txmem, 0x1000),
-    DEFINE_PROP_PTR("dmach", struct XilinxAXIEnet, dmach),
     DEFINE_NIC_PROPERTIES(struct XilinxAXIEnet, conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -893,6 +901,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_XILINX_AXIDMA_PEER, xilinx_enet_peer_initfn },
+            { }
+    }
 };
 
 static void xilinx_enet_register_types(void)
-- 
1.7.3.2

[Qemu-devel] [RFC v0 3/8] xilinx_axidma: Added missing TypeInfo

[Peter A. G. Crosthwaite]

Will fold into previous patch on next revision, but made seperate for review
of interdiff

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/xilinx_axidma.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
index b82c02e..267b19b 100644
--- a/hw/xilinx_axidma.c
+++ b/hw/xilinx_axidma.c
@@ -521,9 +521,16 @@ static TypeInfo axidma_info = {
     }
 };
 
+static TypeInfo axidma_peer_info = {
+    .name          = TYPE_XILINX_AXIDMA_PEER,
+    .parent        = TYPE_INTERFACE,
+    .instance_size = sizeof(struct XilinxAXIDMAPeerIface),
+};
+
 static void xilinx_axidma_register_types(void)
 {
     type_register_static(&axidma_info);
+    type_register_static(&axidma_peer_info);
 }
 
 type_init(xilinx_axidma_register_types)
-- 
1.7.3.2

[Qemu-devel] [RFC v0 4/8] object: create default canonical paths for orphans

[Peter A. G. Crosthwaite]

Create a default canonical path for an object which doesnt have one. This
is a workaround for a bug probably buried deep with qdev as objects should
always have valid canonical paths, but the need came about with the
petalogix_ml605_mmu platform, the ethernet and dma devices as created by
qdev have bogus paths (apparently).

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 qom/object.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index e6c3cfb..1eba795 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -983,6 +983,13 @@ gchar *object_get_canonical_path(Object *obj)
         obj = INTERFACE(obj)->iface_obj;
     }
 
+    if (obj->parent == NULL) {
+        static int num_orphans = 0;
+        char orphan_name [16];
+        snprintf(orphan_name, 16, "orphan%d", num_orphans++);
+        object_property_add_child(root, orphan_name, obj, NULL);
+    }
+
     while (obj != root) {
         ObjectProperty *prop = NULL;
 
-- 
1.7.3.2

[Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Peter A. G. Crosthwaite]

Objects that define interface delegate the creation of the interface object
to the interface type. These means that object_new() when called recursively by
the interface instantior is going to bork because its trying to instantiate
an abstract type. Fixed by making interface types concrete.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 qom/object.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 1eba795..c3a7a47 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -191,7 +191,7 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
         .parent = iface->parent,
         .class_size = sizeof(InterfaceClass),
         .class_init = iface->interface_initfn,
-        .abstract = true,
+        .abstract = false,
     };
     char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
 
-- 
1.7.3.2

[Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Peter A. G. Crosthwaite]

Something is broken with casting to an interface type then setting a link. Cast
these two to object for linking instead and it all works.

Needs investigation why.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/petalogix_ml605_mmu.c |    4 ++--
 hw/xilinx.h              |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index 37866f4..4b1996b 100644
--- a/hw/petalogix_ml605_mmu.c
+++ b/hw/petalogix_ml605_mmu.c
@@ -129,9 +129,9 @@ petalogix_ml605_init(ram_addr_t ram_size,
     DeviceState *dma = qdev_create(NULL, "xilinx-axidma");
     DeviceState *eth0;
 
-    eth0 = xilinx_axiethernet_create(&nd_table[0], XILINX_AXIDMA_PEER(dma),
+    eth0 = xilinx_axiethernet_create(&nd_table[0], OBJECT(dma),
                                      0x82780000, irq[3], 0x1000, 0x1000);
-    xilinx_axiethernetdma_init(dma, XILINX_AXIDMA_PEER(eth0),
+    xilinx_axiethernetdma_init(dma, OBJECT(eth0),
                                0x84600000, irq[1], irq[0], 100 * 1000000);
 
     microblaze_load_kernel(cpu, ddr_base, ram_size, BINARY_DEVICE_TREE_FILE,
diff --git a/hw/xilinx.h b/hw/xilinx.h
index 5ccf3fa..0a940b8 100644
--- a/hw/xilinx.h
+++ b/hw/xilinx.h
@@ -50,7 +50,7 @@ xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
 }
 
 static inline DeviceState *
-xilinx_axiethernet_create(NICInfo *nd, XilinxAXIDMAPeer *peer,
+xilinx_axiethernet_create(NICInfo *nd, Object *peer,
                           target_phys_addr_t base, qemu_irq irq,
                           int txmem, int rxmem)
 {
@@ -70,7 +70,7 @@ xilinx_axiethernet_create(NICInfo *nd, XilinxAXIDMAPeer *peer,
 }
 
 static inline void
-xilinx_axiethernetdma_init(DeviceState *dev, XilinxAXIDMAPeer *peer,
+xilinx_axiethernetdma_init(DeviceState *dev, Object *peer,
                            target_phys_addr_t base, qemu_irq irq,
                            qemu_irq irq2, int freqhz)
 {
-- 
1.7.3.2

[Qemu-devel] [RFC v0 7/8] petalogix_ml605_mmu: fixed qdev create for dma

[Peter A. G. Crosthwaite]

The qdev name was wrong, fixed. This will fold into a patch 2 of this series,
but creating seperately for ease of review and diff tracking.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/petalogix_ml605_mmu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index 4b1996b..3739db7 100644
--- a/hw/petalogix_ml605_mmu.c
+++ b/hw/petalogix_ml605_mmu.c
@@ -126,7 +126,7 @@ petalogix_ml605_init(ram_addr_t ram_size,
     xilinx_timer_create(TIMER_BASEADDR, irq[2], 2, 100 * 1000000);
 
     /* axi ethernet and dma initialization. */
-    DeviceState *dma = qdev_create(NULL, "xilinx-axidma");
+    DeviceState *dma = qdev_create(NULL, "xilinx,axidma");
     DeviceState *eth0;
 
     eth0 = xilinx_axiethernet_create(&nd_table[0], OBJECT(dma),
-- 
1.7.3.2

[Qemu-devel] [RFC v0 8/8] axidma: renamed interconnect to axi-stream

[Peter A. G. Crosthwaite]

The "axidma peer" connection is really the axi stream interface. renamed
appropriately.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/axi-stream.c             |   15 +++++++++++++++
 hw/axi-stream.h             |   35 +++++++++++++++++++++++++++++++++++
 hw/microblaze/Makefile.objs |    1 +
 hw/petalogix_ml605_mmu.c    |    3 ++-
 hw/xilinx.h                 |    6 +++---
 hw/xilinx_axidma.c          |   31 ++++++++++++++-----------------
 hw/xilinx_axidma.h          |   33 ---------------------------------
 hw/xilinx_axienet.c         |   16 ++++++++--------
 8 files changed, 78 insertions(+), 62 deletions(-)
 create mode 100644 hw/axi-stream.c
 create mode 100644 hw/axi-stream.h
 delete mode 100644 hw/xilinx_axidma.h

diff --git a/hw/axi-stream.c b/hw/axi-stream.c
new file mode 100644
index 0000000..d0d9bb8
--- /dev/null
+++ b/hw/axi-stream.c
@@ -0,0 +1,15 @@
+#include "axi-stream.h"
+
+static TypeInfo axi_stream_slave_info = {
+    .name          = TYPE_AXI_STREAM_SLAVE,
+    .parent        = TYPE_INTERFACE,
+    .instance_size = sizeof(struct AXIStreamSlaveIface),
+};
+
+
+static void axi_stream_slave_register_types(void)
+{
+    type_register_static(&axi_stream_slave_info);
+}
+
+type_init(axi_stream_slave_register_types)
diff --git a/hw/axi-stream.h b/hw/axi-stream.h
new file mode 100644
index 0000000..f1a60ca
--- /dev/null
+++ b/hw/axi-stream.h
@@ -0,0 +1,35 @@
+#ifndef AXI_STREAM_H
+#define AXI_STREAM_H 1
+
+#include "qemu-common.h"
+#include "qemu/object.h"
+
+/* AXI stream slave. Used until qdev provides a generic way.  */
+#define TYPE_AXI_STREAM_SLAVE "axi-stream-slave"
+
+#define AXI_STREAM_SLAVE_IFACE(klass) \
+     OBJECT_CLASS_CHECK(AXIStreamSlaveIface, klass, TYPE_AXI_STREAM_SLAVE)
+
+/* This is usually done implicitly by object_set_link_property.  */
+#define AXI_STREAM_SLAVE(obj) \
+     OBJECT_CHECK(AXIStreamSlave, obj, TYPE_AXI_STREAM_SLAVE)
+
+typedef Interface AXIStreamSlave;
+typedef struct AXIStreamSlaveIface AXIStreamSlaveIface;
+
+struct AXIStreamSlaveIface {
+    InterfaceClass parent;
+
+    void (*push)(Object *obj, unsigned char *buf, size_t len, uint32_t *app);
+};
+
+static inline
+void axi_stream_push(AXIStreamSlave *peer, uint8_t *buf, size_t len, uint32_t *app)
+{
+    AXIStreamSlaveIface *iface = container_of(INTERFACE_GET_CLASS(peer),
+                                                AXIStreamSlaveIface,
+                                                parent);
+    iface->push(INTERFACE_OBJECT(peer), buf, len, app);
+}
+
+#endif
diff --git a/hw/microblaze/Makefile.objs b/hw/microblaze/Makefile.objs
index 020f7b6..cc7c222 100644
--- a/hw/microblaze/Makefile.objs
+++ b/hw/microblaze/Makefile.objs
@@ -9,6 +9,7 @@ obj-y += xilinx_uartlite.o
 obj-y += xilinx_ethlite.o
 obj-y += xilinx_axidma.o
 obj-y += xilinx_axienet.o
+obj-y += axi-stream.o
 obj-$(CONFIG_FDT) += ../device_tree.o
 
 obj-y := $(addprefix ../,$(obj-y))
diff --git a/hw/petalogix_ml605_mmu.c b/hw/petalogix_ml605_mmu.c
index 3739db7..40d91cf 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 "axi-stream.h"
 
 #define LMB_BRAM_SIZE  (128 * 1024)
 #define FLASH_SIZE     (32 * 1024 * 1024)
diff --git a/hw/xilinx.h b/hw/xilinx.h
index 0a940b8..f1d07fc 100644
--- a/hw/xilinx.h
+++ b/hw/xilinx.h
@@ -1,4 +1,4 @@
-#include "xilinx_axidma.h"
+#include "axi-stream.h"
 #include "qemu-common.h"
 #include "net.h"
 
@@ -61,7 +61,7 @@ xilinx_axiethernet_create(NICInfo *nd, Object *peer,
     qdev_set_nic_properties(dev, nd);
     qdev_prop_set_uint32(dev, "c_rxmem", rxmem);
     qdev_prop_set_uint32(dev, "c_txmem", txmem);
-    object_property_set_link(OBJECT(dev), OBJECT(peer), "peer", NULL);
+    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);
@@ -75,7 +75,7 @@ xilinx_axiethernetdma_init(DeviceState *dev, Object *peer,
                            qemu_irq irq2, int freqhz)
 {
     qdev_prop_set_uint32(dev, "freqhz", freqhz);
-    object_property_set_link(OBJECT(dev), OBJECT(peer), "peer", NULL);
+    object_property_set_link(OBJECT(dev), OBJECT(peer), "tx_dev", NULL);
     qdev_init_nofail(dev);
 
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
index 267b19b..9d4efe3 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 "axi-stream.h"
 
 #define D(x)
 
@@ -77,6 +77,10 @@ enum {
     SDESC_STATUS_COMPLETE = (1 << 31)
 };
 
+/* FIXME: this is a misnomer, it refers to the control subregion of the DMA to
+ * control a stream, not a AXIStream connection
+ */
+
 struct AXIStream {
     QEMUBH *bh;
     ptimer_state *ptimer;
@@ -94,7 +98,7 @@ struct XilinxAXIDMA {
     SysBusDevice busdev;
     MemoryRegion iomem;
     uint32_t freqhz;
-    XilinxAXIDMAPeer *peer;
+    AXIStreamSlave *tx_dev;
 
     struct AXIStream streams[2];
 };
@@ -241,7 +245,7 @@ static void stream_complete(struct AXIStream *s)
 }
 
 static void stream_process_mem2s(struct AXIStream *s,
-                                 XilinxAXIDMAPeer *peer)
+                                 AXIStreamSlave *tx_dev)
 {
     uint32_t prev_d;
     unsigned char txbuf[16 * 1024];
@@ -276,7 +280,7 @@ static void stream_process_mem2s(struct AXIStream *s,
         s->pos += txlen;
 
         if (stream_desc_eof(&s->desc)) {
-            xlx_dma_push(peer, txbuf, s->pos, app);
+            axi_stream_push(tx_dev, txbuf, s->pos, app);
             s->pos = 0;
             stream_complete(s);
         }
@@ -440,7 +444,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->peer);
+                stream_process_mem2s(s, d->tx_dev);
             }
             break;
         default:
@@ -484,13 +488,13 @@ static void xilinx_axidma_initfn(Object *obj)
 {
     struct XilinxAXIDMA *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
 
-    object_property_add_link(obj, "peer", TYPE_XILINX_AXIDMA_PEER, 
-                             (Object **) &s->peer, NULL);
+    object_property_add_link(obj, "tx_dev", TYPE_AXI_STREAM_SLAVE,
+                             (Object **) &s->tx_dev, NULL);
 }
 
-static void xilinx_axidma_peer_initfn(ObjectClass *klass, void *data)
+static void xilinx_axidma_rx_stream_initfn(ObjectClass *klass, void *data)
 {
-    XilinxAXIDMAPeerIface *k = XILINX_AXIDMA_PEER_IFACE(klass);
+    AXIStreamSlaveIface *k = AXI_STREAM_SLAVE_IFACE(klass);
 
     k->push = axidma_push;
 }
@@ -516,21 +520,14 @@ static TypeInfo axidma_info = {
     .class_init    = axidma_class_init,
     .instance_init = xilinx_axidma_initfn,
     .interfaces = (InterfaceInfo[]) {
-	    { TYPE_XILINX_AXIDMA_PEER, xilinx_axidma_peer_initfn },
+	    { TYPE_AXI_STREAM_SLAVE, xilinx_axidma_rx_stream_initfn },
 	    { }
     }
 };
 
-static TypeInfo axidma_peer_info = {
-    .name          = TYPE_XILINX_AXIDMA_PEER,
-    .parent        = TYPE_INTERFACE,
-    .instance_size = sizeof(struct XilinxAXIDMAPeerIface),
-};
-
 static void xilinx_axidma_register_types(void)
 {
     type_register_static(&axidma_info);
-    type_register_static(&axidma_peer_info);
 }
 
 type_init(xilinx_axidma_register_types)
diff --git a/hw/xilinx_axidma.h b/hw/xilinx_axidma.h
deleted file mode 100644
index 99c8371..0000000
--- a/hw/xilinx_axidma.h
+++ /dev/null
@@ -1,33 +0,0 @@
-#ifndef XILINX_AXIDMA_H
-#define XILINX_AXIDMA_H 1
-
-/* AXI DMA connection. Used until qdev provides a generic way.  */
-#define TYPE_XILINX_AXIDMA_PEER "xilinx-axidma-peer"
-
-#define XILINX_AXIDMA_PEER_IFACE(klass) \
-     OBJECT_CLASS_CHECK(XilinxAXIDMAPeerIface, klass, TYPE_XILINX_AXIDMA_PEER)
-
-/* This is usually done implicitly by object_set_link_property.  */
-#define XILINX_AXIDMA_PEER(obj) \
-     OBJECT_CHECK(XilinxAXIDMAPeer, obj, TYPE_XILINX_AXIDMA_PEER)
-
-typedef Interface XilinxAXIDMAPeer;
-typedef struct XilinxAXIDMAPeerIface XilinxAXIDMAPeerIface;
-
-struct XilinxAXIDMAPeerIface {
-    InterfaceClass parent;
-
-    void (*push)(Object *obj, unsigned char *buf, size_t len, uint32_t *app);
-};
-
-static inline
-void xlx_dma_push(XilinxAXIDMAPeer *peer,
-                  uint8_t *buf, size_t len, uint32_t *app)
-{
-    XilinxAXIDMAPeerIface *iface = container_of(INTERFACE_GET_CLASS(peer),
-                                                XilinxAXIDMAPeerIface,
-                                                parent);
-    iface->push(INTERFACE_OBJECT(peer), buf, len, app);
-}
-
-#endif
diff --git a/hw/xilinx_axienet.c b/hw/xilinx_axienet.c
index a340cf2..62b36e7 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 "axi-stream.h"
 
 #define DPHY(x)
 
@@ -310,7 +310,7 @@ struct XilinxAXIEnet {
     SysBusDevice busdev;
     MemoryRegion iomem;
     qemu_irq irq;
-    XilinxAXIDMAPeer *peer;
+    AXIStreamSlave *tx_dev;
     NICState *nic;
     NICConf conf;
 
@@ -773,7 +773,7 @@ static ssize_t eth_rx(VLANClientState *nc, const uint8_t *buf, size_t size)
     /* Good frame.  */
     app[2] |= 1 << 6;
 
-    xlx_dma_push(s->peer, (void *)s->rxmem, size, app);
+    axi_stream_push(s->tx_dev, (void *)s->rxmem, size, app);
 
     s->regs[R_IS] |= IS_RX_COMPLETE;
     enet_update_irq(s);
@@ -868,13 +868,13 @@ static void xilinx_enet_initfn(Object *obj)
 {
     struct XilinxAXIEnet *s = FROM_SYSBUS(typeof(*s), SYS_BUS_DEVICE(obj));
 
-    object_property_add_link(obj, "peer", TYPE_XILINX_AXIDMA_PEER,
-                             (Object **) &s->peer, NULL);
+    object_property_add_link(obj, "tx_dev", TYPE_AXI_STREAM_SLAVE,
+                             (Object **) &s->tx_dev, NULL);
 }
 
-static void xilinx_enet_peer_initfn(ObjectClass *klass, void *data)
+static void xilinx_enet_rx_stream_initfn(ObjectClass *klass, void *data)
 {
-    XilinxAXIDMAPeerIface *k = XILINX_AXIDMA_PEER_IFACE(klass);
+    AXIStreamSlaveIface *k = AXI_STREAM_SLAVE_IFACE(klass);
 
     k->push = axienet_stream_push;
 }
@@ -903,7 +903,7 @@ static TypeInfo xilinx_enet_info = {
     .class_init    = xilinx_enet_class_init,
     .instance_init = xilinx_enet_initfn,
     .interfaces = (InterfaceInfo[]) {
-            { TYPE_XILINX_AXIDMA_PEER, xilinx_enet_peer_initfn },
+            { TYPE_AXI_STREAM_SLAVE, xilinx_enet_rx_stream_initfn },
             { }
     }
 };
-- 
1.7.3.2

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Paolo Bonzini]

Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Objects that define interface delegate the creation of the interface object
> to the interface type. These means that object_new() when called recursively by
> the interface instantior is going to bork because its trying to instantiate
> an abstract type. Fixed by making interface types concrete.
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  qom/object.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 1eba795..c3a7a47 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -191,7 +191,7 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
>          .parent = iface->parent,
>          .class_size = sizeof(InterfaceClass),
>          .class_init = iface->interface_initfn,
> -        .abstract = true,
> +        .abstract = false,
>      };
>      char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
>  
> 

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

Re: [Qemu-devel] [RFC v0 3/8] xilinx_axidma: Added missing TypeInfo

[Paolo Bonzini]

Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Will fold into previous patch on next revision, but made seperate for review
> of interdiff
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> ---
>  hw/xilinx_axidma.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/xilinx_axidma.c b/hw/xilinx_axidma.c
> index b82c02e..267b19b 100644
> --- a/hw/xilinx_axidma.c
> +++ b/hw/xilinx_axidma.c
> @@ -521,9 +521,16 @@ static TypeInfo axidma_info = {
>      }
>  };
>  
> +static TypeInfo axidma_peer_info = {
> +    .name          = TYPE_XILINX_AXIDMA_PEER,
> +    .parent        = TYPE_INTERFACE,
> +    .instance_size = sizeof(struct XilinxAXIDMAPeerIface),
> +};
> +
>  static void xilinx_axidma_register_types(void)
>  {
>      type_register_static(&axidma_info);
> +    type_register_static(&axidma_peer_info);
>  }
>  
>  type_init(xilinx_axidma_register_types)
> 

Oops, thanks.

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

Re: [Qemu-devel] [RFC v0 4/8] object: create default canonical paths for orphans

[Paolo Bonzini]

Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Create a default canonical path for an object which doesnt have one. This
> is a workaround for a bug probably buried deep with qdev as objects should
> always have valid canonical paths, but the need came about with the
> petalogix_ml605_mmu platform, the ethernet and dma devices as created by
> qdev have bogus paths (apparently).

We already do this in qdev_init_nofail:

    if (!OBJECT(dev)->parent) {
        static int unattached_count = 0;
        gchar *name = g_strdup_printf("device[%d]", unattached_count++);

        object_property_add_child(container_get(qdev_get_machine(),
                                                "/unattached"),
                                  name, OBJECT(dev), NULL);
        g_free(name);
    }

Probably we should move this piece to a function qdev_attach, so that
you can call it before object_property_set_link.

Alternatively, we could make object_property_set_link bypass the
canonical paths and write the pointer directly, but I think this would
be bad.

Paolo

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Andreas Färber]

Am 13.06.2012 12:03, schrieb Paolo Bonzini:
> Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
>> Objects that define interface delegate the creation of the interface object
>> to the interface type. These means that object_new() when called recursively by

I am pretty certain that object_new() is NOT called recursively! The
static helpers of object_instantiate() are, and abstractness should not
matter there or none of the, e.g., CPU subclasses could be created... So
if there is a problem this description is bogus.

>> the interface instantior is going to bork because its trying to instantiate
>> an abstract type. Fixed by making interface types concrete.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>>  qom/object.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 1eba795..c3a7a47 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -191,7 +191,7 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
>>          .parent = iface->parent,
>>          .class_size = sizeof(InterfaceClass),
>>          .class_init = iface->interface_initfn,
>> -        .abstract = true,
>> +        .abstract = false,
>>      };
>>      char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
>>  
>>
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Why? Object is abstract, too, and in patch 3/8 a type is being derived
from TYPE_INTERFACE. So if we want to make interface non-abstract then
we don't need the Container type either since it is simply the
non-abstract version of Object. My guess is that, if at all, something
else is going wrong and this is papering over it.

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] [RFC v0 6/8] xilinx dont cast to interface types with links

[Paolo Bonzini]

Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Something is broken with casting to an interface type then setting a link. Cast
> these two to object for linking instead and it all works.

I don't have a board image but I don't see anything visibly bad without this patch.

However, something that _is_ bad indeed happens; we try to ref/unref an interface
object.  This patch fixes it while keeping things efficient (and in fact fixes one
TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?

Anthony, is this above your disgust level?

Paolo


------------------------ 8< -----------------------

diff --git a/qom/object.c b/qom/object.c
index c3a7a47..bd60838 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -63,7 +63,7 @@ struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
+#define INTERFACE(obj) ((Interface *)(obj))
 
 static Type type_interface;
 
@@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
     }
 }
 
+#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
+
+static inline bool object_is_interface(Object *obj) {
+    return obj->interfaces == INTERFACE_MAGIC;
+}
+
 static void object_interface_init(Object *obj, InterfaceImpl *iface)
 {
     TypeImpl *ti = iface->type;
     Interface *iface_obj;
 
+    assert(!object_is_interface(obj));
     iface_obj = INTERFACE(object_new(ti->name));
     iface_obj->iface_obj = obj;
+    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
 
     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
@@ -332,10 +340,12 @@ 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 (!object_is_interface(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)) {
@@ -410,6 +420,13 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
     TypeImpl *target_type = type_get_by_name(typename);
     GSList *i;
 
+    /* Check if obj is an interface and its containing object is a direct
+     * ancestor of typename.  object_is_interface is a very fast test.
+     */
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
      * we want to go back from interfaces to the parent.
     */
@@ -417,24 +434,6 @@ Object *object_dynamic_cast(Object *obj, const char *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)->iface_obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
     if (!target_type) {
         return obj;
     }
@@ -597,11 +596,19 @@ GSList *object_class_get_list(const char *implements_type,
 
 void object_ref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     obj->ref++;
 }
 
 void object_unref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     g_assert(obj->ref > 0);
     obj->ref--;
 
@@ -979,7 +986,7 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
+    if (object_is_interface(obj)) {
         obj = INTERFACE(obj)->iface_obj;
     }
 

Paolo

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Paolo Bonzini]

Il 13/06/2012 12:28, Andreas Färber ha scritto:
> I am pretty certain that object_new() is NOT called recursively!

It is for interfaces:

object_new
  ->object_new_with_type
  ->object_initialize_with_type
  ->object_init_with_type
  ->object_interface_init

and then:

static void object_interface_init(Object *obj, InterfaceImpl *iface)
{
    TypeImpl *ti = iface->type;
    Interface *iface_obj;

    assert(!object_is_interface(obj));
    iface_obj = INTERFACE(object_new(ti->name));
    iface_obj->iface_obj = obj;
    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;

    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
}

Paolo

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Avi Kivity]

On 06/13/2012 01:41 PM, Paolo Bonzini wrote:
> 
> However, something that _is_ bad indeed happens; we try to ref/unref an interface
> object.  This patch fixes it while keeping things efficient (and in fact fixes one
> TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?
> 
> Anthony, is this above your disgust level?
> 
> Paolo
> 
> 
> ------------------------ 8< -----------------------
> 
> diff --git a/qom/object.c b/qom/object.c
> index c3a7a47..bd60838 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -63,7 +63,7 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> -#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
> +#define INTERFACE(obj) ((Interface *)(obj))
>  
>  static Type type_interface;
>  
> @@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
>      }
>  }
>  
> +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
> +
> +static inline bool object_is_interface(Object *obj) {
> +    return obj->interfaces == INTERFACE_MAGIC;
> +}


Why play games?

  static GSList interface_magic;

  static inline bool object_is_interface(Object *obj)
  {
     return obj->interfaces == &interface_magic;
  }

though I think we can spare a bool (and developer's sanity) in Object.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Paolo Bonzini]

Il 13/06/2012 12:55, Avi Kivity ha scritto:
>> > +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
>> > +
>> > +static inline bool object_is_interface(Object *obj) {
>> > +    return obj->interfaces == INTERFACE_MAGIC;
>> > +}
> 
> Why play games?
> 
>   static GSList interface_magic;
> 
>   static inline bool object_is_interface(Object *obj)
>   {
>      return obj->interfaces == &interface_magic;
>   }

Indeed that's beautiful. :)

> though I think we can spare a bool (and developer's sanity) in Object.

Yes, we could shrink the refcount to 16-bit and get place for some
bitfields.

Paolo

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Avi Kivity]

On 06/13/2012 01:59 PM, Paolo Bonzini wrote:
> 
>> though I think we can spare a bool (and developer's sanity) in Object.
> 
> Yes, we could shrink the refcount to 16-bit and get place for some
> bitfields.


I fact there's a four-byte hole after ->ref as it is.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Paolo Bonzini]

Il 13/06/2012 13:02, Avi Kivity ha scritto:
>>> >> though I think we can spare a bool (and developer's sanity) in Object.
>> > 
>> > Yes, we could shrink the refcount to 16-bit and get place for some
>> > bitfields.
> 
> I fact there's a four-byte hole after ->ref as it is.

Not on 32-bits...

Paolo

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Avi Kivity]

On 06/13/2012 02:05 PM, Paolo Bonzini wrote:
> Il 13/06/2012 13:02, Avi Kivity ha scritto:
>>>> >> though I think we can spare a bool (and developer's sanity) in Object.
>>> > 
>>> > Yes, we could shrink the refcount to 16-bit and get place for some
>>> > bitfields.
>> 
>> I fact there's a four-byte hole after ->ref as it is.
> 
> Not on 32-bits...

Those hardly exists.  And anyway it's silly to save a few bytes of
memory when you're going to drop tons of it on the guest itself.  Better
to spend the effort where we have O(guest size) allocations and keep the
code clear and efficient.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Andreas Färber]

Am 13.06.2012 12:42, schrieb Paolo Bonzini:
> Il 13/06/2012 12:28, Andreas Färber ha scritto:
>> I am pretty certain that object_new() is NOT called recursively!
> 
> It is for interfaces:
> 
> object_new
>   ->object_new_with_type
>   ->object_initialize_with_type
>   ->object_init_with_type
>   ->object_interface_init
> 
> and then:
> 
> static void object_interface_init(Object *obj, InterfaceImpl *iface)
> {
>     TypeImpl *ti = iface->type;
>     Interface *iface_obj;
> 
>     assert(!object_is_interface(obj));
>     iface_obj = INTERFACE(object_new(ti->name));
>     iface_obj->iface_obj = obj;
>     iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
> 
>     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
> }

Ouch! One can argue that's still not recursive, but what matters more
this borks Anthony's in-place object_initialize() concept.

Two solutions come to mind:
* allocate the interfaces as part of object_new() beyond instance_size
* let initialize stage return Error*

I'm starting to tend towards the latter since that also addresses
failure to add properties.

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] [RFC v0 5/8] object: make interfaces concrete

[Paolo Bonzini]

Il 13/06/2012 14:59, Andreas Färber ha scritto:
> Ouch! One can argue that's still not recursive, but what matters more
> this borks Anthony's in-place object_initialize() concept.
> 
> Two solutions come to mind:
> * allocate the interfaces as part of object_new() beyond instance_size

That won't work if you initialize in place, because you cannot allocate
the room for the interface.  It is possible to put Interface objects
explicitly in the class, and pass an offset when registering the type so
that they can be initialized in place.

But I still think we're fighting windmills...

Paolo

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

On 06/13/2012 08:02 AM, Paolo Bonzini wrote:
> Il 13/06/2012 14:59, Andreas Färber ha scritto:
>> Ouch! One can argue that's still not recursive, but what matters more
>> this borks Anthony's in-place object_initialize() concept.
>>
>> Two solutions come to mind:
>> * allocate the interfaces as part of object_new() beyond instance_size
>
> That won't work if you initialize in place, because you cannot allocate
> the room for the interface.  It is possible to put Interface objects
> explicitly in the class, and pass an offset when registering the type so
> that they can be initialized in place.
>
> But I still think we're fighting windmills...

There's no problem in my mind with allocating interfaces on the heap.  in-place 
initialization is a readability thing, it's not a memory management thing.  It's 
so you can see that:

struct PIIX3 {
   ...

   RTCState rtc;
   APICState *apic;
};

'rtc' is a child and 'apic' is a link.

Anyway, I don't like the idea of making interfaces concrete.  That means that a 
user could directly instantiate an interface which doesn't make a lot of sense.

Here's a different solution.  This has not been even compile tested but I think 
the concept is sound.

Regards,

Anthony Liguori

>
> Paolo
>
>


[-- Attachment #2: 0001-qom-allow-interfaces-to-be-created-while-abstract-du.patch --]
[-- Type: text/x-patch, Size: 2861 bytes --]

>From c7e0661bd261ac4919dca0e3a5dd78d3871c3292 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Wed, 13 Jun 2012 08:29:30 -0500
Subject: [PATCH] qom: allow interfaces to be created while abstract during
 object initialization

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qom/object.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 6f839ad..1a436d8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -73,6 +73,8 @@ typedef struct Interface
 
 static Type type_interface;
 
+static Object *object_new_with_type_full(Type type, bool concrete_only);
+
 static GHashTable *type_table_get(void)
 {
     static GHashTable *type_table;
@@ -250,7 +252,7 @@ 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 = INTERFACE(object_new_with_type_full(ti, false));
     iface_obj->obj = obj;
 
     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
@@ -273,7 +275,8 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
-void object_initialize_with_type(void *data, TypeImpl *type)
+static void object_initialize_with_type_full(void *data, TypeImpl *type,
+                                             bool concrete_only)
 {
     Object *obj = data;
 
@@ -281,7 +284,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     type_initialize(type);
 
     g_assert(type->instance_size >= sizeof(Object));
-    g_assert(type->abstract == false);
+    g_assert(!concrete_only || type->abstract == false);
 
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
@@ -289,6 +292,11 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     object_init_with_type(obj, type);
 }
 
+void object_initialize_with_type(void *data, TypeImpl *type)
+{
+    object_initialize_with_type_full(data, type, false);
+}
+
 void object_initialize(void *data, const char *typename)
 {
     TypeImpl *type = type_get_by_name(typename);
@@ -362,7 +370,7 @@ void object_finalize(void *data)
     g_assert(obj->ref == 0);
 }
 
-Object *object_new_with_type(Type type)
+static Object *object_new_with_type_full(Type type, bool concrete_only)
 {
     Object *obj;
 
@@ -370,12 +378,17 @@ Object *object_new_with_type(Type type)
     type_initialize(type);
 
     obj = g_malloc(type->instance_size);
-    object_initialize_with_type(obj, type);
+    object_initialize_with_type_full(obj, type, concrete_only);
     object_ref(obj);
 
     return obj;
 }
 
+Object *object_new_with_type(Type type)
+{
+    return object_new_with_type_full(type, true);
+}
+
 Object *object_new(const char *typename)
 {
     TypeImpl *ti = type_get_by_name(typename);
-- 
1.7.5.4

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Paolo Bonzini]

Il 13/06/2012 15:30, Anthony Liguori ha scritto:
> Anyway, I don't like the idea of making interfaces concrete.  That means
> that a user could directly instantiate an interface which doesn't make a
> lot of sense.

Concrete doesn't mean "instantiatable by the user".  It means
"instantiatable period".

Paolo

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Anthony Liguori]

On 06/13/2012 08:33 AM, Paolo Bonzini wrote:
> Il 13/06/2012 15:30, Anthony Liguori ha scritto:
>> Anyway, I don't like the idea of making interfaces concrete.  That means
>> that a user could directly instantiate an interface which doesn't make a
>> lot of sense.
>
> Concrete doesn't mean "instantiatable by the user".  It means
> "instantiatable period".

Interfaces are not supposed to be instantiatable by anyone.  The fact that 
object_new() is used to create the interface is an internal implementation detail.

Interfaces are stateless and by definition, never have an implementation on 
their own.  So object_new() of an interface type directly results in a useless 
object.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Paolo Bonzini]

Il 13/06/2012 15:36, Anthony Liguori ha scritto:
>>>
>>
>> Concrete doesn't mean "instantiatable by the user".  It means
>> "instantiatable period".
> 
> Interfaces are not supposed to be instantiatable by anyone.  The fact
> that object_new() is used to create the interface is an internal
> implementation detail.

Whose side effect is that interfaces need to be concrete.

> Interfaces are stateless and by definition, never have an implementation
> on their own.  So object_new() of an interface type directly results in
> a useless object.

Yes, and that's why you needn't really care about what the user does.
Garbage in, garbage out...

Paolo

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Anthony Liguori]

On 06/13/2012 08:38 AM, Paolo Bonzini wrote:
> Il 13/06/2012 15:36, Anthony Liguori ha scritto:
>>>>
>>>
>>> Concrete doesn't mean "instantiatable by the user".  It means
>>> "instantiatable period".
>>
>> Interfaces are not supposed to be instantiatable by anyone.  The fact
>> that object_new() is used to create the interface is an internal
>> implementation detail.
>
> Whose side effect is that interfaces need to be concrete.
>
>> Interfaces are stateless and by definition, never have an implementation
>> on their own.  So object_new() of an interface type directly results in
>> a useless object.
>
> Yes, and that's why you needn't really care about what the user does.
> Garbage in, garbage out...

Quick chat on IRC and I think we both agree that we need to promote the notion 
of Interface to a first class concept in QOM.

I'll send out a patch later today (that's actually tested..).

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [RFC v0 6/8] xilinx dont cast to interface types with links

[Anthony Liguori]

On 06/13/2012 05:41 AM, Paolo Bonzini wrote:
> Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
>> Something is broken with casting to an interface type then setting a link. Cast
>> these two to object for linking instead and it all works.
>
> I don't have a board image but I don't see anything visibly bad without this patch.
>
> However, something that _is_ bad indeed happens; we try to ref/unref an interface
> object.  This patch fixes it while keeping things efficient (and in fact fixes one
> TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?
>
> Anthony, is this above your disgust level?

Isn't there a patch floating around to make Object a proper TypeInfo?  In which 
case, couldn't we make Interface not inherit from Object and change the OBJECT() 
cast macro to tell the difference?

Regards,

Anthony Liguori

>
> Paolo
>
>
> ------------------------ 8<  -----------------------
>
> diff --git a/qom/object.c b/qom/object.c
> index c3a7a47..bd60838 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -63,7 +63,7 @@ struct TypeImpl
>       InterfaceImpl interfaces[MAX_INTERFACES];
>   };
>
> -#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
> +#define INTERFACE(obj) ((Interface *)(obj))
>
>   static Type type_interface;
>
> @@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
>       }
>   }
>
> +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
> +
> +static inline bool object_is_interface(Object *obj) {
> +    return obj->interfaces == INTERFACE_MAGIC;
> +}
> +
>   static void object_interface_init(Object *obj, InterfaceImpl *iface)
>   {
>       TypeImpl *ti = iface->type;
>       Interface *iface_obj;
>
> +    assert(!object_is_interface(obj));
>       iface_obj = INTERFACE(object_new(ti->name));
>       iface_obj->iface_obj = obj;
> +    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
>
>       obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
>   }
> @@ -332,10 +340,12 @@ 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 (!object_is_interface(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)) {
> @@ -410,6 +420,13 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>       TypeImpl *target_type = type_get_by_name(typename);
>       GSList *i;
>
> +    /* Check if obj is an interface and its containing object is a direct
> +     * ancestor of typename.  object_is_interface is a very fast test.
> +     */
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
>        * we want to go back from interfaces to the parent.
>       */
> @@ -417,24 +434,6 @@ Object *object_dynamic_cast(Object *obj, const char *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)->iface_obj;
> -        if (object_is_type(obj, target_type)) {
> -            return obj;
> -        }
> -    }
> -
>       if (!target_type) {
>           return obj;
>       }
> @@ -597,11 +596,19 @@ GSList *object_class_get_list(const char *implements_type,
>
>   void object_ref(Object *obj)
>   {
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       obj->ref++;
>   }
>
>   void object_unref(Object *obj)
>   {
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       g_assert(obj->ref>  0);
>       obj->ref--;
>
> @@ -979,7 +986,7 @@ gchar *object_get_canonical_path(Object *obj)
>       Object *root = object_get_root();
>       char *newpath = NULL, *path = NULL;
>
> -    if (object_is_type(obj, type_interface)) {
> +    if (object_is_interface(obj)) {
>           obj = INTERFACE(obj)->iface_obj;
>       }
>
>
> Paolo
>
>

Re: [Qemu-devel] [RFC v0 5/8] object: make interfaces concrete

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

this is not super on topic and might be a dumb question but now that we use
glib, why aren't we using gobjects more than we are? I'm guessing the glib
ppl have figured out how all this is suposed to work already...
On Jun 13, 2012 3:50 PM, "Anthony Liguori" <anthony@codemonkey.ws> wrote:

> On 06/13/2012 08:38 AM, Paolo Bonzini wrote:
>
>> Il 13/06/2012 15:36, Anthony Liguori ha scritto:
>>
>>>
>>>>>
>>>> Concrete doesn't mean "instantiatable by the user".  It means
>>>> "instantiatable period".
>>>>
>>>
>>> Interfaces are not supposed to be instantiatable by anyone.  The fact
>>> that object_new() is used to create the interface is an internal
>>> implementation detail.
>>>
>>
>> Whose side effect is that interfaces need to be concrete.
>>
>>  Interfaces are stateless and by definition, never have an implementation
>>> on their own.  So object_new() of an interface type directly results in
>>> a useless object.
>>>
>>
>> Yes, and that's why you needn't really care about what the user does.
>> Garbage in, garbage out...
>>
>
> Quick chat on IRC and I think we both agree that we need to promote the
> notion of Interface to a first class concept in QOM.
>
> I'll send out a patch later today (that's actually tested..).
>
> Regards,
>
> Anthony Liguori
>
>
>> Paolo
>>
>>
>

[-- Attachment #2: Type: text/html, Size: 2339 bytes --]