[Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

This is a follow up to my previous series to get us started in the QOM
direction.  A few things are different this time around.  Most notably:

 1) Devices no longer have names.  Instead, path names are always used to
    identify devices.

 2) In order to support (1), dynamic properties are now supported.

 3) The concept of a "root device" has been introduced.  The root device is
    roughly modelling the motherboard of a machine.  This type is a container
    type and it's best to think of it as something like a PCB board I guess.

To try it out, here's an example session:

Launch:

anthony@titi:~/build/qemu$ x86_64-softmmu/qemu-system-x86_64 -hda ~/images/linux.img -snapshot -device virtio-balloon-pci,id=foo -qmp unix:/tmp/server.sock,server,nowait

Explore the object model:

anthony@titi:~/git/qemu/QMP$ ./qom-list /
peripheral/
i440fx/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/
piix3/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3
rtc/
anthony@titi:~/git/qemu/QMP$ ./qom-list /i440fx/piix3/rtc
date
base_year
anthony@titi:~/git/qemu/QMP$ ./qom-get /i440fx/piix3/rtc.date
tm_sec: 33
tm_hour: 21
tm_mday: 30
tm_year: 111
tm_mon: 10
tm_min: 2
anthony@titi:~/git/qemu/QMP$ ./qom-get rtc.date
tm_sec: 38
tm_hour: 21
tm_mday: 30
tm_year: 111
tm_mon: 10
tm_min: 2
anthony@titi:~/git/qemu/QMP$ ./qom-list /peripheral
foo/
anthony@titi:~/git/qemu/QMP$ ./qom-list /peripheral/foo
event_idx
indirect_desc

Anthony Liguori (18):
  qom: add a reference count to qdev objects
  qom: add new dynamic property infrastructure based on Visitors (v2)
  qom: register legacy properties as new style properties (v2)
  qom: introduce root device
  qdev: provide an interface to return canonical path from root (v2)
  qdev: provide a path resolution (v2)
  qom: add child properties (composition) (v2)
  qom: add link properties (v2)
  qapi: allow a 'gen' key to suppress code generation
  qmp: add qom-list command
  qom: qom_{get,set} monitor commands (v2)
  qdev: add explicitly named devices to the root complex
  dev: add an anonymous peripheral container
  rtc: make piix3 set the rtc as a child (v2)
  rtc: add a dynamic property for retrieving the date
  qom: optimize qdev_get_canonical_path using a parent link
  qmp: make qmp.py easier to use
  qom: add test tools (v2)

 Makefile.objs            |    2 +-
 QMP/qmp.py               |    6 +
 QMP/qom-get              |   26 +++
 QMP/qom-list             |   30 +++
 QMP/qom-set              |   21 ++
 hw/container.c           |   20 ++
 hw/mc146818rtc.c         |   27 +++
 hw/pc_piix.c             |   11 +
 hw/piix_pci.c            |    3 +
 hw/qdev.c                |  489 +++++++++++++++++++++++++++++++++++++++++++++-
 hw/qdev.h                |  245 +++++++++++++++++++++++
 monitor.h                |    4 +
 qapi-schema.json         |  107 ++++++++++
 qerror.c                 |    4 +
 qerror.h                 |    3 +
 qmp-commands.hx          |   18 ++
 qmp.c                    |   93 +++++++++
 scripts/qapi-commands.py |    1 +
 scripts/qapi-types.py    |    1 +
 19 files changed, 1109 insertions(+), 2 deletions(-)
 create mode 100755 QMP/qom-get
 create mode 100755 QMP/qom-list
 create mode 100755 QMP/qom-set
 create mode 100644 hw/container.c

-- 
1.7.4.1

[Qemu-devel] [PATCH v2 01/18] qom: add a reference count to qdev objects

[Anthony Liguori]

To ensure that a device isn't removed from the graph until all of its links are
broken.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   16 ++++++++++++++++
 hw/qdev.h |   26 ++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 106407f..fdc9843 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -323,6 +323,11 @@ int qdev_unplug(DeviceState *dev)
     }
     assert(dev->info->unplug != NULL);
 
+    if (dev->ref != 0) {
+        qerror_report(QERR_DEVICE_IN_USE, dev->id?:"");
+        return -1;
+    }
+
     qdev_hot_removed = true;
 
     return dev->info->unplug(dev);
@@ -962,3 +967,14 @@ char* qdev_get_fw_dev_path(DeviceState *dev)
 
     return strdup(path);
 }
+
+void qdev_ref(DeviceState *dev)
+{
+    dev->ref++;
+}
+
+void qdev_unref(DeviceState *dev)
+{
+    g_assert(dev->ref > 0);
+    dev->ref--;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 36a4198..2397b4e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -45,6 +45,12 @@ struct DeviceState {
     QTAILQ_ENTRY(DeviceState) sibling;
     int instance_id_alias;
     int alias_required_for_version;
+
+    /**
+     * This tracks the number of references between devices.  See @qdev_ref for
+     * more information.
+     */
+    uint32_t ref;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -329,4 +335,24 @@ char *qdev_get_fw_dev_path(DeviceState *dev);
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 extern struct BusInfo system_bus_info;
 
+/**
+ * @qdev_ref
+ *
+ * Increase the reference count of a device.  A device cannot be freed as long
+ * as its reference count is greater than zero.
+ *
+ * @dev - the device
+ */
+void qdev_ref(DeviceState *dev);
+
+/**
+ * @qdef_unref
+ *
+ * Decrease the reference count of a device.  A device cannot be freed as long
+ * as its reference count is greater than zero.
+ *
+ * @dev - the device
+ */
+void qdev_unref(DeviceState *dev);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 02/18] qom: add new dynamic property infrastructure based on Visitors (v2)

[Anthony Liguori]

qdev properties are settable only during construction and static to classes.
This isn't flexible enough for QOM.

This patch introduces a property interface for qdev that provides dynamic
properties that are tied to objects, instead of classes.  These properties are
Visitor based instead of string based too.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - Etter -> Accessor (Stefan)
 - spelling mistakes (Stefan)
 - switch to qemu-queue (Kevin/Gerd)
---
 hw/qdev.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |  120 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qerror.c  |    4 ++
 qerror.h  |    3 ++
 4 files changed, 224 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index fdc9843..94f14c1 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -98,6 +98,7 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
         qdev_hot_added = true;
     }
     dev->instance_id_alias = -1;
+    QTAILQ_INIT(&dev->properties);
     dev->state = DEV_STATE_CREATED;
     return dev;
 }
@@ -395,12 +396,31 @@ void qdev_init_nofail(DeviceState *dev)
     }
 }
 
+static void qdev_property_del_all(DeviceState *dev)
+{
+    while (!QTAILQ_EMPTY(&dev->properties)) {
+        DeviceProperty *prop = QTAILQ_FIRST(&dev->properties);
+
+        QTAILQ_REMOVE(&dev->properties, prop, node);
+
+        if (prop->release) {
+            prop->release(dev, prop->name, prop->opaque);
+        }
+
+        g_free(prop->name);
+        g_free(prop->type);
+        g_free(prop);
+    }
+}
+
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
     BusState *bus;
     Property *prop;
 
+    qdev_property_del_all(dev);
+
     if (dev->state == DEV_STATE_INITIALIZED) {
         while (dev->num_child_bus) {
             bus = QLIST_FIRST(&dev->child_bus);
@@ -978,3 +998,80 @@ void qdev_unref(DeviceState *dev)
     g_assert(dev->ref > 0);
     dev->ref--;
 }
+
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyAccessor *get, DevicePropertyAccessor *set,
+                       DevicePropertyRelease *release,
+                       void *opaque, Error **errp)
+{
+    DeviceProperty *prop = g_malloc0(sizeof(*prop));
+
+    prop->name = g_strdup(name);
+    prop->type = g_strdup(type);
+
+    prop->get = get;
+    prop->set = set;
+    prop->release = release;
+    prop->opaque = opaque;
+
+    QTAILQ_INSERT_TAIL(&dev->properties, prop, node);
+}
+
+static DeviceProperty *qdev_property_find(DeviceState *dev, const char *name)
+{
+    DeviceProperty *prop;
+
+    QTAILQ_FOREACH(prop, &dev->properties, node) {
+        if (strcmp(prop->name, name) == 0) {
+            return prop;
+        }
+    }
+
+    return NULL;
+}
+
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->get) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->get(dev, v, prop->opaque, name, errp);
+    }
+}
+
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return;
+    }
+
+    if (!prop->set) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    } else {
+        prop->set(dev, prop->opaque, v, name, errp);
+    }
+}
+
+const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **errp)
+{
+    DeviceProperty *prop = qdev_property_find(dev, name);
+
+    if (prop == NULL) {
+        error_set(errp, QERR_PROPERTY_NOT_FOUND, dev->id?:"", name);
+        return NULL;
+    }
+
+    return prop->type;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 2397b4e..2df3bb7 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -5,6 +5,7 @@
 #include "qemu-queue.h"
 #include "qemu-char.h"
 #include "qemu-option.h"
+#include "qapi/qapi-visit-core.h"
 
 typedef struct Property Property;
 
@@ -27,6 +28,44 @@ enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
 };
 
+/**
+ * @DevicePropertyAccessor - called when trying to get/set a property
+ *
+ * @dev the device that owns the property
+ * @v the visitor that contains the property data
+ * @opaque the device property opaque
+ * @name the name of the property
+ * @errp a pointer to an Error that is filled if getting/setting fails.
+ */
+typedef void (DevicePropertyAccessor)(DeviceState *dev,
+                                      Visitor *v,
+                                      void *opaque,
+                                      const char *name,
+                                      Error **errp);
+
+/**
+ * @DevicePropertyRelease - called when a property is removed from a device
+ *
+ * @dev the device that owns the property
+ * @name the name of the property
+ * @opaque the opaque registered with the property
+ */
+typedef void (DevicePropertyRelease)(DeviceState *dev,
+                                     const char *name,
+                                     void *opaque);
+
+typedef struct DeviceProperty
+{
+    gchar *name;
+    gchar *type;
+    DevicePropertyAccessor *get;
+    DevicePropertyAccessor *set;
+    DevicePropertyRelease *release;
+    void *opaque;
+
+    QTAILQ_ENTRY(DeviceProperty) node;
+} DeviceProperty;
+
 /* This structure should not be accessed directly.  We declare it here
    so that it can be embedded in individual device state structures.  */
 struct DeviceState {
@@ -51,6 +90,8 @@ struct DeviceState {
      * more information.
      */
     uint32_t ref;
+
+    QTAILQ_HEAD(, DeviceProperty) properties;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
@@ -355,4 +396,83 @@ void qdev_ref(DeviceState *dev);
  */
 void qdev_unref(DeviceState *dev);
 
+/**
+ * @qdev_property_add - add a new property to a device
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property.  This can contain any character except for
+ *         a forward slash.  In general, you should use hyphens '-' instead of
+ *         underscores '_' when naming properties.
+ *
+ * @type - the type name of the property.  This namespace is pretty loosely
+ *         defined.  Sub namespaces are constructed by using a prefix and then
+ *         to angle brackets.  For instance, the type 'virtio-net-pci' in the
+ *         'link' namespace would be 'link<virtio-net-pci>'.
+ *
+ * @get - the getter to be called to read a property.  If this is NULL, then
+ *        the property cannot be read.
+ *
+ * @set - the setter to be called to write a property.  If this is NULL,
+ *        then the property cannot be written.
+ *
+ * @release - called when the property is removed from the device.  This is
+ *            meant to allow a property to free its opaque upon device
+ *            destruction.  This may be NULL.
+ *
+ * @opaque - an opaque pointer to pass to the callbacks for the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyAccessor *get, DevicePropertyAccessor *set,
+                       DevicePropertyRelease *release,
+                       void *opaque, Error **errp);
+
+/**
+ * @qdev_property_get - reads a property from a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will receive the property value.  This should be an
+ *      Output visitor and the data will be written with @name as the name.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_get(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_set - writes a property to a device
+ *
+ * @dev - the device
+ *
+ * @v - the visitor that will be used to write the property value.  This should
+ *      be an Input visitor and the data will be first read with @name as the
+ *      name and then written as the property value.
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ */
+void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
+                       Error **errp);
+
+/**
+ * @qdev_property_get_type - returns the type of a property
+ *
+ * @dev - the device
+ *
+ * @name - the name of the property
+ *
+ * @errp - returns an error if this function fails
+ *
+ * Returns:
+ *   The type name of the property.
+ */
+const char *qdev_property_get_type(DeviceState *dev, const char *name,
+                                   Error **errp);
+
 #endif
diff --git a/qerror.c b/qerror.c
index fdf62b9..dd0ee76 100644
--- a/qerror.c
+++ b/qerror.c
@@ -178,6 +178,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not open '%(filename)'",
     },
     {
+        .error_fmt = QERR_PERMISSION_DENIED,
+        .desc      = "Insufficient permission to perform this operation",
+    },
+    {
         .error_fmt = QERR_PROPERTY_NOT_FOUND,
         .desc      = "Property '%(device).%(property)' not found",
     },
diff --git a/qerror.h b/qerror.h
index 2d3d43b..2b1d743 100644
--- a/qerror.h
+++ b/qerror.h
@@ -150,6 +150,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
+#define QERR_PERMISSION_DENIED \
+    "{ 'class': 'PermissionDenied', 'data': {} }"
+
 #define QERR_PROPERTY_NOT_FOUND \
     "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
 
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 03/18] qom: register legacy properties as new style properties (v2)

[Anthony Liguori]

Expose all legacy properties through the new QOM property mechanism.  The qdev
property types are exposed through the 'legacy<>' namespace.  They are always
visited as strings since they do their own string parsing.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - add bus properties (Gerd)
---
 hw/qdev.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    7 +++++
 2 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 94f14c1..6f77af9 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -83,6 +83,7 @@ static DeviceInfo *qdev_find_info(BusInfo *bus_info, const char *name)
 static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
 {
     DeviceState *dev;
+    Property *prop;
 
     assert(bus->info == info->bus_info);
     dev = g_malloc0(info->size);
@@ -100,6 +101,15 @@ static DeviceState *qdev_create_from_info(BusState *bus, DeviceInfo *info)
     dev->instance_id_alias = -1;
     QTAILQ_INIT(&dev->properties);
     dev->state = DEV_STATE_CREATED;
+
+    for (prop = dev->info->props; prop && prop->name; prop++) {
+        qdev_property_add_legacy(dev, prop, NULL);
+    }
+
+    for (prop = dev->info->bus_info->props; prop && prop->name; prop++) {
+        qdev_property_add_legacy(dev, prop, NULL);
+    }
+
     return dev;
 }
 
@@ -1075,3 +1085,79 @@ const char *qdev_property_get_type(DeviceState *dev, const char *name, Error **e
 
     return prop->type;
 }
+
+/**
+ * Legacy property handling
+ */
+
+static void qdev_get_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+
+    if (prop->info->print) {
+        char buffer[1024];
+        char *ptr = buffer;
+
+        prop->info->print(dev, prop, buffer, sizeof(buffer));
+        visit_type_str(v, &ptr, name, errp);
+    } else {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    }
+}
+
+static void qdev_set_legacy_property(DeviceState *dev, Visitor *v, void *opaque,
+                                     const char *name, Error **errp)
+{
+    Property *prop = opaque;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    if (prop->info->parse) {
+        Error *local_err = NULL;
+        char *ptr = NULL;
+
+        visit_type_str(v, &ptr, name, &local_err);
+        if (!local_err) {
+            int ret;
+            ret = prop->info->parse(dev, prop, ptr);
+            if (ret != 0) {
+                error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                          name, prop->info->name);
+            }
+            g_free(ptr);
+        } else {
+            error_propagate(errp, local_err);
+        }
+    } else {
+        error_set(errp, QERR_PERMISSION_DENIED);
+    }
+}
+
+/**
+ * @qdev_add_legacy_property - adds a legacy property
+ *
+ * Do not use this is new code!  Properties added through this interface will
+ * be given types in the "legacy<>" type namespace.
+ *
+ * Legacy properties are always processed as strings.  The format of the string
+ * depends on the property type.
+ */
+void qdev_property_add_legacy(DeviceState *dev, Property *prop,
+                              Error **errp)
+{
+    gchar *type;
+
+    type = g_strdup_printf("legacy<%s>", prop->info->name);
+
+    qdev_property_add(dev, prop->name, type,
+                      qdev_get_legacy_property,
+                      qdev_set_legacy_property,
+                      NULL,
+                      prop, errp);
+
+    g_free(type);
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 2df3bb7..3b629d4 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -475,4 +475,11 @@ void qdev_property_set(DeviceState *dev, Visitor *v, const char *name,
 const char *qdev_property_get_type(DeviceState *dev, const char *name,
                                    Error **errp);
 
+/**
+ * @qdev_property_add_legacy - add a legacy @Property to a device
+ *
+ * DO NOT USE THIS IN NEW CODE!
+ */
+void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 04/18] qom: introduce root device

[Anthony Liguori]

This is based on Jan's suggestion for how to do unique naming.  The root device
is the root of composition.  All devices are reachable via child<> links from
this device.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 Makefile.objs  |    2 +-
 hw/container.c |   20 ++++++++++++++++++++
 hw/qdev.c      |   12 ++++++++++++
 hw/qdev.h      |    8 ++++++++
 4 files changed, 41 insertions(+), 1 deletions(-)
 create mode 100644 hw/container.c

diff --git a/Makefile.objs b/Makefile.objs
index d7a6539..10e794c 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -279,7 +279,7 @@ hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
 hw-obj-$(CONFIG_ESP) += esp.o
 
 hw-obj-y += dma-helpers.o sysbus.o isa-bus.o
-hw-obj-y += qdev-addr.o
+hw-obj-y += qdev-addr.o container.o
 
 # VGA
 hw-obj-$(CONFIG_VGA_PCI) += vga-pci.o
diff --git a/hw/container.c b/hw/container.c
new file mode 100644
index 0000000..9cbf399
--- /dev/null
+++ b/hw/container.c
@@ -0,0 +1,20 @@
+#include "sysbus.h"
+
+static int container_initfn(SysBusDevice *dev)
+{
+    return 0;
+}
+
+static SysBusDeviceInfo container_info = {
+    .init = container_initfn,
+    .qdev.name = "container",
+    .qdev.size = sizeof(SysBusDevice),
+    .qdev.no_user = 1,
+};
+
+static void container_init(void)
+{
+    sysbus_register_withprop(&container_info);
+}
+
+device_init(container_init);
diff --git a/hw/qdev.c b/hw/qdev.c
index 6f77af9..bb0b9f7 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1161,3 +1161,15 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop,
 
     g_free(type);
 }
+
+DeviceState *qdev_get_root(void)
+{
+    static DeviceState *qdev_root;
+
+    if (!qdev_root) {
+        qdev_root = qdev_create(NULL, "container");
+        qdev_init_nofail(qdev_root);
+    }
+
+    return qdev_root;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 3b629d4..52aadd2 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -482,4 +482,12 @@ const char *qdev_property_get_type(DeviceState *dev, const char *name,
  */
 void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
 
+/**
+ * @qdev_get_root - returns the root device of the composition tree
+ *
+ * Returns:
+ *   The root of the composition tree.
+ */
+DeviceState *qdev_get_root(void);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 05/18] qdev: provide an interface to return canonical path from root (v2)

[Anthony Liguori]

The canonical path is the path in the composition tree from the root to the
device.  This is effectively the name of the device.

This is an incredibly unefficient implementation that will be optimized in
a future patch.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - change from gslist to qemu-queue (Kevin/Gerd)
---
 hw/qdev.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |    9 +++++++++
 2 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index bb0b9f7..79849c9 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1173,3 +1173,51 @@ DeviceState *qdev_get_root(void)
 
     return qdev_root;
 }
+
+static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
+{
+    DeviceProperty *prop;
+
+    if (parent == dev) {
+        return g_strdup("");
+    }
+
+    QTAILQ_FOREACH(prop, &parent->properties, node) {
+        gchar *subpath;
+
+        if (!strstart(prop->type, "child<", NULL)) {
+            continue;
+        }
+
+        /* Check to see if the device is one of parent's children */
+        if (prop->opaque == dev) {
+            return g_strdup(prop->name);
+        }
+
+        /* Check to see if the device is a child of our child */
+        subpath = qdev_get_path_in(prop->opaque, dev);
+        if (subpath) {
+            gchar *path;
+
+            path = g_strdup_printf("%s/%s", prop->name, subpath);
+            g_free(subpath);
+
+            return path;
+        }
+    }
+
+    return NULL;
+}
+
+gchar *qdev_get_canonical_path(DeviceState *dev)
+{
+    gchar *path, *newpath;
+
+    path = qdev_get_path_in(qdev_get_root(), dev);
+    g_assert(path != NULL);
+
+    newpath = g_strdup_printf("/%s", path);
+    g_free(path);
+
+    return newpath;
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 52aadd2..0f00497 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -490,4 +490,13 @@ void qdev_property_add_legacy(DeviceState *dev, Property *prop, Error **errp);
  */
 DeviceState *qdev_get_root(void);
 
+/**
+ * @qdev_get_canonical_path - returns the canonical path for a device.  This
+ * is the path within the composition tree starting from the root.
+ *
+ * Returns:
+ *   The canonical path in the composition tree.
+ */
+gchar *qdev_get_canonical_path(DeviceState *dev);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 06/18] qdev: provide a path resolution (v2)

[Anthony Liguori]

There are two types of supported paths--absolute paths and partial paths.

Absolute paths are derived from the root device and can follow child<> or
link<> properties.  Since they can follow link<> properties, they can be
arbitrarily long.  Absolute paths look like absolute filenames and are prefixed
with a leading slash.

Partial paths are look like relative filenames.  They do not begin with a
prefix.  The matching rules for partial paths are subtle but designed to make
specifying devices easy.  At each level of the composition tree, the partial
path is matched as an absolute path.  The first match is not returned.  At
least two matches are searched for.  A successful result is only returned if
only one match is founded.  If more than one match is found, a flag is returned
to indicate that the match was ambiguous.

At the end of the day, partial path support means that if you create a device
called 'ide0', you can just say 'ide0' as the path name and it will Just Work.
If we internally create a device called 'i440fx', you can just say 'i440fx' and
it will Just Work and long as you don't do anything silly.

A management tool should probably always use absolute paths since then they
don't have to deal with the possibility of ambiguity.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - fix comments (Stefan)
 - always initialize ambiguous to false (Stefan)
 - change from gslist to qemu-queue (Kevin/Gerd)
---
 hw/qdev.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |   28 ++++++++++++++++
 2 files changed, 131 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 79849c9..2519f00 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1221,3 +1221,106 @@ gchar *qdev_get_canonical_path(DeviceState *dev)
 
     return newpath;
 }
+
+static DeviceState *qdev_resolve_abs_path(DeviceState *parent,
+                                          gchar **parts,
+                                          int index)
+{
+    DeviceProperty *prop;
+    DeviceState *child;
+
+    if (parts[index] == NULL) {
+        return parent;
+    }
+
+    if (strcmp(parts[index], "") == 0) {
+        return qdev_resolve_abs_path(parent, parts, index + 1);
+    }
+
+    prop = qdev_property_find(parent, parts[index]);
+    if (prop == NULL) {
+        return NULL;
+    }
+
+    child = NULL;
+    if (strstart(prop->type, "link<", NULL)) {
+        DeviceState **pchild = prop->opaque;
+        if (*pchild) {
+            child = *pchild;
+        }
+    } else if (strstart(prop->type, "child<", NULL)) {
+        child = prop->opaque;
+    }
+
+    if (!child) {
+        return NULL;
+    }
+
+    return qdev_resolve_abs_path(child, parts, index + 1);
+}
+
+static DeviceState *qdev_resolve_partial_path(DeviceState *parent,
+                                              gchar **parts,
+                                              bool *ambiguous)
+{
+    DeviceState *dev;
+    DeviceProperty *prop;
+
+    dev = qdev_resolve_abs_path(parent, parts, 0);
+
+    QTAILQ_FOREACH(prop, &parent->properties, node) {
+        DeviceState *found;
+
+        if (!strstart(prop->type, "child<", NULL)) {
+            continue;
+        }
+
+        found = qdev_resolve_partial_path(prop->opaque, parts, ambiguous);
+        if (found) {
+            if (dev) {
+                if (ambiguous) {
+                    *ambiguous = true;
+                }
+                return NULL;
+            }
+            dev = found;
+        }
+
+        if (ambiguous && *ambiguous) {
+            return NULL;
+        }
+    }
+
+    return dev;
+}
+
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous)
+{
+    bool partial_path = true;
+    DeviceState *dev;
+    gchar **parts;
+
+    parts = g_strsplit(path, "/", 0);
+    if (parts == NULL || parts[0] == NULL) {
+        g_strfreev(parts);
+        return qdev_get_root();
+    }
+
+    if (strcmp(parts[0], "") == 0) {
+        partial_path = false;
+    }
+
+    if (partial_path) {
+        if (ambiguous) {
+            *ambiguous = false;
+        }
+        dev = qdev_resolve_partial_path(qdev_get_root(), parts, ambiguous);
+    } else {
+        dev = qdev_resolve_abs_path(qdev_get_root(), parts, 1);
+    }
+
+    g_strfreev(parts);
+
+    return dev;
+}
+
diff --git a/hw/qdev.h b/hw/qdev.h
index 0f00497..641d134 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -499,4 +499,32 @@ DeviceState *qdev_get_root(void);
  */
 gchar *qdev_get_canonical_path(DeviceState *dev);
 
+/**
+ * @qdev_resolve_path - resolves a path returning a device
+ *
+ * There are two types of supported paths--absolute paths and partial paths.
+ * 
+ * Absolute paths are derived from the root device and can follow child<> or
+ * link<> properties.  Since they can follow link<> properties, they can be
+ * arbitrarily long.  Absolute paths look like absolute filenames and are
+ * prefixed with a leading slash.
+ * 
+ * Partial paths look like relative filenames.  They do not begin with a
+ * prefix.  The matching rules for partial paths are subtle but designed to make
+ * specifying devices easy.  At each level of the composition tree, the partial
+ * path is matched as an absolute path.  The first match is not returned.  At
+ * least two matches are searched for.  A successful result is only returned if
+ * only one match is founded.  If more than one match is found, a flag is
+ * return to indicate that the match was ambiguous.
+ *
+ * @path - the path to resolve
+ *
+ * @ambiguous - returns true if the path resolution failed because of an
+ *              ambiguous match
+ *
+ * Returns:
+ *   The matched device or NULL on path lookup failure.
+ */
+DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 07/18] qom: add child properties (composition) (v2)

[Anthony Liguori]

Child properties express a relationship of composition.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - fix comments (Kevin)
 - add a reference when adding a child property (Kevin)
---
 hw/qdev.c |   26 ++++++++++++++++++++++++++
 hw/qdev.h |   20 ++++++++++++++++++++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 2519f00..fa6b489 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1174,6 +1174,32 @@ DeviceState *qdev_get_root(void)
     return qdev_root;
 }
 
+static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
+                                    const char *name, Error **errp)
+{
+    DeviceState *child = opaque;
+    gchar *path;
+
+    path = qdev_get_canonical_path(child);
+    visit_type_str(v, &path, name, errp);
+    g_free(path);
+}
+
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp)
+{
+    gchar *type;
+
+    type = g_strdup_printf("child<%s>", child->info->name);
+
+    qdev_property_add(dev, name, type, qdev_get_child_property,
+                      NULL, NULL, child, errp);
+
+    qdev_ref(dev);
+
+    g_free(type);
+}
+
 static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
 {
     DeviceProperty *prop;
diff --git a/hw/qdev.h b/hw/qdev.h
index 641d134..38b36e8 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -527,4 +527,24 @@ gchar *qdev_get_canonical_path(DeviceState *dev);
  */
 DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
 
+/**
+ * @qdev_property_add_child - Add a child property to a device
+ *
+ * Child properties form the composition tree.  All devices need to be a child
+ * of another device.  Devices can only be a child of one device.
+ *
+ * There is no way for a child to determine what its parent is.  It is not
+ * a bidirectional relationship.  This is by design.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @child - the child device
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_child(DeviceState *dev, const char *name,
+                             DeviceState *child, Error **errp);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 08/18] qom: add link properties (v2)

[Anthony Liguori]

Links represent an ephemeral relationship between devices.  They are meant to
replace the qdev concept of busses by allowing more informal relationships
between devices.

Links are fairly limited in their usefulness without implementing QOM-style
subclassing and interfaces.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - comments (Stefan)
 - maintain a reference when adding/removing a link property (Kevin)
---
 hw/qdev.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev.h |   23 +++++++++++++++++++
 2 files changed, 97 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index fa6b489..42430fa 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1200,6 +1200,80 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
     g_free(type);
 }
 
+static void qdev_get_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    gchar *path;
+
+    if (*child) {
+        path = qdev_get_canonical_path(*child);
+        visit_type_str(v, &path, name, errp);
+        g_free(path);
+    } else {
+        path = (gchar *)"";
+        visit_type_str(v, &path, name, errp);
+    }
+}
+
+static void qdev_set_link_property(DeviceState *dev, Visitor *v, void *opaque,
+                                   const char *name, Error **errp)
+{
+    DeviceState **child = opaque;
+    bool ambiguous = false;
+    const char *type;
+    char *path;
+
+    type = qdev_property_get_type(dev, name, NULL);
+
+    visit_type_str(v, &path, name, errp);
+
+    if (*child) {
+        qdev_unref(*child);
+    }
+
+    if (strcmp(path, "") != 0) {
+        DeviceState *target;
+
+        target = qdev_resolve_path(path, &ambiguous);
+        if (target) {
+            gchar *target_type;
+
+            target_type = g_strdup_printf("link<%s>", target->info->name);
+            if (strcmp(target_type, type) == 0) {
+                *child = target;
+                qdev_ref(target);
+            } else {
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE, name, type);
+            }
+
+            g_free(target_type);
+        } else {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        }
+    } else {
+        *child = NULL;
+    }
+
+    g_free(path);
+}
+
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp)
+{
+    gchar *full_type;
+
+    full_type = g_strdup_printf("link<%s>", type);
+
+    qdev_property_add(dev, name, full_type,
+                      qdev_get_link_property,
+                      qdev_set_link_property,
+                      NULL, child, errp);
+
+    g_free(full_type);
+}
+
 static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
 {
     DeviceProperty *prop;
diff --git a/hw/qdev.h b/hw/qdev.h
index 38b36e8..4351e2e 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -547,4 +547,27 @@ DeviceState *qdev_resolve_path(const char *path, bool *ambiguous);
 void qdev_property_add_child(DeviceState *dev, const char *name,
                              DeviceState *child, Error **errp);
 
+/**
+ * @qdev_property_add_link - Add a link property to a device
+ *
+ * Links establish relationships between devices.  Links are unidirectional
+ * although two links can be combined to form a bidirectional relationship
+ * between devices.
+ *
+ * Links form the graph in the device model.
+ *
+ * @dev - the device to add a property to
+ *
+ * @name - the name of the property
+ *
+ * @type - the qdev type of the link
+ *
+ * @child - a pointer to where the link device reference is stored
+ *
+ * @errp - if an error occurs, a pointer to an area to store the area
+ */
+void qdev_property_add_link(DeviceState *dev, const char *name,
+                            const char *type, DeviceState **child,
+                            Error **errp);
+
 #endif
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 09/18] qapi: allow a 'gen' key to suppress code generation

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 scripts/qapi-commands.py |    1 +
 scripts/qapi-types.py    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index f7def16..54d1f5d 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -405,6 +405,7 @@ except os.error, e:
 
 exprs = parse_schema(sys.stdin)
 commands = filter(lambda expr: expr.has_key('command'), exprs)
+commands = filter(lambda expr: not expr.has_key('gen'), commands)
 
 if dispatch_type == "sync":
     fdecl = open(h_file, 'w')
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index f64d84c..267cb49 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -238,6 +238,7 @@ fdecl.write(mcgen('''
                   guard=guardname(h_file)))
 
 exprs = parse_schema(sys.stdin)
+exprs = filter(lambda expr: not expr.has_key('gen'), exprs)
 
 for expr in exprs:
     ret = "\n"
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 10/18] qmp: add qom-list command

[Anthony Liguori]

This can be used to list properties in the device model.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |    6 ++++++
 qmp.c            |   28 ++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index cb1ba77..276b7c3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -887,3 +887,51 @@
 # Notes: Do not use this command.
 ##
 { 'command': 'cpu', 'data': {'index': 'int'} }
+
+##
+# @DevicePropertyInfo:
+#
+# @name: the name of the property
+#
+# @type: the type of the property.  This will typically come in one of four
+#        forms:
+#
+#        1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
+#           These types are mapped to the appropriate JSON type.
+#
+#        2) A legacy type in the form 'legacy<subtype>' where subtype is the
+#           legacy qdev typename.  These types are always treated as strings.
+#
+#        3) A child type in the form 'child<subtype>' where subtype is a qdev
+#           device type name.  Child properties create the composition tree.
+#
+#        4) A link type in the form 'link<subtype>' where subtype is a qdev
+#           device type name.  Link properties form the device model graph.
+#
+# Since: 1.1
+#
+# Notes: This type is experimental.  Its syntax may change in future releases.
+##
+{ 'type': 'DevicePropertyInfo',
+  'data': { 'name': 'str', 'type': 'str' } }
+
+##
+# @qom-list:
+#
+# This command will list any properties of a device given a path in the device
+# model.
+#
+# @path: the path within the device model.  See @qom-get for a description of
+#        this parameter.
+#
+# Returns: a list of @DevicePropertyInfo that describe the properties of the
+#          device.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental.  It's syntax may change in future
+#        releases.
+##
+{ 'command': 'qom-list',
+  'data': { 'path': 'str' },
+  'returns': [ 'DevicePropertyInfo' ] }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 97975a5..d6ff466 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2001,3 +2001,9 @@ EQMP
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_input_query_balloon,
     },
+
+    {
+        .name       = "qom-list",
+        .args_type  = "path:s",
+        .mhandler.cmd_new = qmp_marshal_input_qom_list,
+    },
diff --git a/qmp.c b/qmp.c
index 511dd62..06ae569 100644
--- a/qmp.c
+++ b/qmp.c
@@ -16,6 +16,7 @@
 #include "qmp-commands.h"
 #include "kvm.h"
 #include "arch_init.h"
+#include "hw/qdev.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -117,3 +118,30 @@ SpiceInfo *qmp_query_spice(Error **errp)
     return NULL;
 };
 #endif
+
+DevicePropertyInfoList *qmp_qom_list(const char *path, Error **errp)
+{
+    DeviceState *dev;
+    bool ambiguous = false;
+    DevicePropertyInfoList *props = NULL;
+    DeviceProperty *prop;
+
+    dev = qdev_resolve_path(path, &ambiguous);
+    if (dev == NULL) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, path);
+        return NULL;
+    }
+
+    QTAILQ_FOREACH(prop, &dev->properties, node) {
+        DevicePropertyInfoList *entry = g_malloc0(sizeof(*entry));
+
+        entry->value = g_malloc0(sizeof(DevicePropertyInfo));
+        entry->next = props;
+        props = entry;
+
+        entry->value->name = g_strdup(prop->name);
+        entry->value->type = g_strdup(prop->type);
+    }
+
+    return props;
+}
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 11/18] qom: qom_{get, set} monitor commands (v2)

[Anthony Liguori]

This allows clients to read and write device model properties through QMP.  QAPI
doesn't support Visitor types yet and these commands are special in that they
don't work with fixed types.

I've added a documentation stub to qapi-schema.json so we can keep consistency
there.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - comments (Stefan)
---
 monitor.h        |    4 +++
 qapi-schema.json |   59 +++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   12 ++++++++++
 qmp.c            |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/monitor.h b/monitor.h
index e76795f..efc76c7 100644
--- a/monitor.h
+++ b/monitor.h
@@ -64,4 +64,8 @@ typedef void (MonitorCompletion)(void *opaque, QObject *ret_data);
 
 void monitor_set_error(Monitor *mon, QError *qerror);
 
+int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
+
+int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
+
 #endif /* !MONITOR_H */
diff --git a/qapi-schema.json b/qapi-schema.json
index 276b7c3..76a8a53 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -935,3 +935,62 @@
 { 'command': 'qom-list',
   'data': { 'path': 'str' },
   'returns': [ 'DevicePropertyInfo' ] }
+
+##
+# @qom-get:
+#
+# This command will get a property from a device model path and return the
+# value.
+#
+# @path: The path within the device model.  There are two forms of supported
+#        paths--absolute and partial paths.
+#
+#        Absolute paths are derived from the root device and can follow child<>
+#        or link<> properties.  Since they can follow link<> properties, they
+#        can be arbitrarily long.  Absolute paths look like absolute filenames
+#        and are prefixed  with a leading slash.
+#
+#        Partial paths look like relative filenames.  They do not begin
+#        with a prefix.  The matching rules for partial paths are subtle but
+#        designed to make specifying devices easy.  At each level of the
+#        composition tree, the partial path is matched as an absolute path.
+#        The first match is not returned.  At least two matches are searched
+#        for.  A successful result is only returned if only one match is
+#        found.  If more than one match is found, a flag is return to
+#        indicate that the match was ambiguous.
+#
+# @property: The property name to read
+#
+# Returns: The property value.  The type depends on the property type.  legacy<>
+#          properties are returned as #str.  child<> and link<> properties are
+#          returns as #str pathnames.  All integer property types (u8, u16, etc)
+#          are returned as #int.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental and may change syntax in future releases.
+##
+{ 'command': 'qom-get',
+  'data': { 'path': 'str', 'property': 'str' },
+  'returns': 'visitor',
+  'gen': 'no' }
+
+##
+# @qom-set:
+#
+# This command will set a property from a device model path.
+#
+# @path: see @qom-get for a description of this parameter
+#
+# @property: the property name to set
+#
+# @value: a value who's type is appropriate for the property type.  See @qom-get
+#         for a description of type mapping.
+#
+# Since: 1.1
+#
+# Notes: This command is experimental and may change syntax in future releases.
+##
+{ 'command': 'qom-set',
+  'data': { 'path': 'str', 'property': 'str', 'value': 'visitor' },
+  'gen': 'no' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d6ff466..027189a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2007,3 +2007,15 @@ EQMP
         .args_type  = "path:s",
         .mhandler.cmd_new = qmp_marshal_input_qom_list,
     },
+
+    {
+        .name       = "qom-set",
+	.args_type  = "path:s,property:s,opts:O",
+	.mhandler.cmd_new = qmp_qom_set,
+    },
+
+    {
+        .name       = "qom-get",
+	.args_type  = "path:s,property:s",
+	.mhandler.cmd_new = qmp_qom_get,
+    },
diff --git a/qmp.c b/qmp.c
index 06ae569..641761e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -17,6 +17,8 @@
 #include "kvm.h"
 #include "arch_init.h"
 #include "hw/qdev.h"
+#include "qapi/qmp-input-visitor.h"
+#include "qapi/qmp-output-visitor.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -145,3 +147,66 @@ DevicePropertyInfoList *qmp_qom_list(const char *path, Error **errp)
 
     return props;
 }
+
+/* FIXME: teach qapi about how to pass through Visitors */
+int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    QObject *value = qdict_get(qdict, "value");
+    Error *local_err = NULL;
+    QmpInputVisitor *mi;
+    DeviceState *dev;
+
+    dev = qdev_resolve_path(path, NULL);
+    if (!dev) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, path);
+        goto out;
+    }
+
+    mi = qmp_input_visitor_new(value);
+    qdev_property_set(dev, qmp_input_get_visitor(mi), property, &local_err);
+
+    qmp_input_visitor_cleanup(mi);
+
+out:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
+}
+
+int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
+{
+    const char *path = qdict_get_str(qdict, "path");
+    const char *property = qdict_get_str(qdict, "property");
+    Error *local_err = NULL;
+    QmpOutputVisitor *mo;
+    DeviceState *dev;
+
+    dev = qdev_resolve_path(path, NULL);
+    if (!dev) {
+        error_set(&local_err, QERR_DEVICE_NOT_FOUND, path);
+        goto out;
+    }
+
+    mo = qmp_output_visitor_new();
+    qdev_property_get(dev, qmp_output_get_visitor(mo), property, &local_err);
+    if (!local_err) {
+        *ret = qmp_output_get_qobject(mo);
+    }
+
+    qmp_output_visitor_cleanup(mo);
+
+out:
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return -1;
+    }
+
+    return 0;
+}
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 12/18] qdev: add explicitly named devices to the root complex

[Anthony Liguori]

We first add a 'peripheral' container to the root device that we add user
created devices to.  This provides all user created devices with a unique and
isolated namespace.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 42430fa..af4c6a2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -227,6 +227,19 @@ int qdev_device_help(QemuOpts *opts)
     return 1;
 }
 
+static DeviceState *qdev_get_peripheral(void)
+{
+    static DeviceState *dev;
+
+    if (dev == NULL) {
+        dev = qdev_create(NULL, "container");
+        qdev_property_add_child(qdev_get_root(), "peripheral", dev, NULL);
+        qdev_init_nofail(dev);
+    }
+
+    return dev;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     const char *driver, *path, *id;
@@ -278,6 +291,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     id = qemu_opts_id(opts);
     if (id) {
         qdev->id = id;
+        qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
     }
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 13/18] dev: add an anonymous peripheral container

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index af4c6a2..5348f26 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -240,6 +240,19 @@ static DeviceState *qdev_get_peripheral(void)
     return dev;
 }
 
+static DeviceState *qdev_get_peripheral_anon(void)
+{
+    static DeviceState *dev;
+
+    if (dev == NULL) {
+        dev = qdev_create(NULL, "container");
+        qdev_property_add_child(qdev_get_root(), "peripheral-anon", dev, NULL);
+        qdev_init_nofail(dev);
+    }
+
+    return dev;
+}
+
 DeviceState *qdev_device_add(QemuOpts *opts)
 {
     const char *driver, *path, *id;
@@ -292,7 +305,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
     if (id) {
         qdev->id = id;
         qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
-    }
+    } else {
+        static int anon_count;
+        gchar *name = g_strdup_printf("device[%d]", anon_count++);
+        qdev_property_add_child(qdev_get_peripheral_anon(), name,
+                                qdev, NULL);
+        g_free(name);
+    }        
     if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
         qdev_free(qdev);
         return NULL;
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 14/18] rtc: make piix3 set the rtc as a child (v2)

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - comments (Stefan)
---
 hw/pc_piix.c  |   11 +++++++++++
 hw/piix_pci.c |    3 +++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 970f43c..2d5ea2c 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -205,6 +205,17 @@ static void pc_init1(MemoryRegion *system_memory,
         }
     }
 
+    /* FIXME there's some major spaghetti here.  Somehow we create the devices
+     * on the PIIX before we actually create it.  We create the PIIX3 deep in
+     * the recess of the i440fx creation too and then lose the DeviceState.
+     *
+     * For now, let's "fix" this by making judicious use of paths.  This is not
+     * generally the right way to do this.
+     */
+
+    qdev_property_add_child(qdev_resolve_path("/i440fx/piix3", NULL),
+                            "rtc", (DeviceState *)rtc_state, NULL);
+
     audio_init(gsi, pci_enabled ? pci_bus : NULL);
 
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device,
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index d183443..d785d4b 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -288,6 +288,7 @@ static PCIBus *i440fx_common_init(const char *device_name,
                     address_space_io, 0);
     s->bus = b;
     qdev_init_nofail(dev);
+    qdev_property_add_child(qdev_get_root(), "i440fx", dev, NULL);
 
     d = pci_create_simple(b, 0, device_name);
     *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
@@ -323,6 +324,8 @@ static PCIBus *i440fx_common_init(const char *device_name,
                 pci_create_simple_multifunction(b, -1, true, "PIIX3"));
         pci_bus_irqs(b, piix3_set_irq, pci_slot_get_pirq, piix3,
                 PIIX_NUM_PIRQS);
+
+        qdev_property_add_child(dev, "piix3", &piix3->dev.qdev, NULL);
     }
     piix3->pic = pic;
 
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

This really shows the power of dynamic object properties compared to qdev
static properties.

This property represents a complex structure who's format is preserved over the
wire.  This is enabled by visitors.

It also shows an entirely synthetic property that is not tied to device state.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/mc146818rtc.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2aaca2f..0c23cb0 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -614,6 +614,29 @@ static const MemoryRegionOps cmos_ops = {
     .old_portio = cmos_portio
 };
 
+// FIXME add int32 visitor
+static void visit_type_int32(Visitor *v, int *value, const char *name, Error **errp)
+{
+    int64_t val = *value;
+    visit_type_int(v, &val, name, errp);
+}
+
+static void rtc_get_date(DeviceState *dev, Visitor *v, void *opaque,
+                         const char *name, Error **errp)
+{
+    ISADevice *isa = DO_UPCAST(ISADevice, qdev, dev);
+    RTCState *s = DO_UPCAST(RTCState, dev, isa);
+
+    visit_start_struct(v, NULL, "struct tm", name, 0, errp);
+    visit_type_int32(v, &s->current_tm.tm_year, "tm_year", errp);
+    visit_type_int32(v, &s->current_tm.tm_mon, "tm_mon", errp);
+    visit_type_int32(v, &s->current_tm.tm_mday, "tm_mday", errp);
+    visit_type_int32(v, &s->current_tm.tm_hour, "tm_hour", errp);
+    visit_type_int32(v, &s->current_tm.tm_min, "tm_min", errp);
+    visit_type_int32(v, &s->current_tm.tm_sec, "tm_sec", errp);
+    visit_end_struct(v, errp);
+}
+
 static int rtc_initfn(ISADevice *dev)
 {
     RTCState *s = DO_UPCAST(RTCState, dev, dev);
@@ -647,6 +670,10 @@ static int rtc_initfn(ISADevice *dev)
 
     qdev_set_legacy_instance_id(&dev->qdev, base, 2);
     qemu_register_reset(rtc_reset, s);
+
+    qdev_property_add(&s->dev.qdev, "date", "struct tm",
+                      rtc_get_date, NULL, NULL, s, NULL);
+
     return 0;
 }
 
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 16/18] qom: optimize qdev_get_canonical_path using a parent link

[Anthony Liguori]

The full tree search was a bit unreasonable.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/qdev.c |   56 ++++++++++++++++++++++++--------------------------------
 hw/qdev.h |    4 ++++
 2 files changed, 28 insertions(+), 32 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 5348f26..1102efd 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -1229,6 +1229,8 @@ void qdev_property_add_child(DeviceState *dev, const char *name,
                       NULL, NULL, child, errp);
 
     qdev_ref(dev);
+    g_assert(child->parent == NULL);
+    child->parent = dev;
 
     g_free(type);
 }
@@ -1307,48 +1309,38 @@ void qdev_property_add_link(DeviceState *dev, const char *name,
     g_free(full_type);
 }
 
-static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev)
+gchar *qdev_get_canonical_path(DeviceState *dev)
 {
-    DeviceProperty *prop;
+    DeviceState *root = qdev_get_root();
+    char *newpath = NULL, *path = NULL;
 
-    if (parent == dev) {
-        return g_strdup("");
-    }
+    while (dev != root) {
+        DeviceProperty *prop = NULL;
 
-    QTAILQ_FOREACH(prop, &parent->properties, node) {
-        gchar *subpath;
+        g_assert(dev->parent != NULL);
 
-        if (!strstart(prop->type, "child<", NULL)) {
-            continue;
-        }
+        QTAILQ_FOREACH(prop, &dev->parent->properties, node) {
+            if (!strstart(prop->type, "child<", NULL)) {
+                continue;
+            }
 
-        /* Check to see if the device is one of parent's children */
-        if (prop->opaque == dev) {
-            return g_strdup(prop->name);
+            if (prop->opaque == dev) {
+                if (path) {
+                    newpath = g_strdup_printf("%s/%s", prop->name, path);
+                    g_free(path);
+                    path = newpath;
+                } else {
+                    path = g_strdup(prop->name);
+                }
+                break;
+            }
         }
 
-        /* Check to see if the device is a child of our child */
-        subpath = qdev_get_path_in(prop->opaque, dev);
-        if (subpath) {
-            gchar *path;
+        g_assert(prop != NULL);
 
-            path = g_strdup_printf("%s/%s", prop->name, subpath);
-            g_free(subpath);
-
-            return path;
-        }
+        dev = dev->parent;
     }
 
-    return NULL;
-}
-
-gchar *qdev_get_canonical_path(DeviceState *dev)
-{
-    gchar *path, *newpath;
-
-    path = qdev_get_path_in(qdev_get_root(), dev);
-    g_assert(path != NULL);
-
     newpath = g_strdup_printf("/%s", path);
     g_free(path);
 
diff --git a/hw/qdev.h b/hw/qdev.h
index 4351e2e..fdab848 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -92,6 +92,10 @@ struct DeviceState {
     uint32_t ref;
 
     QTAILQ_HEAD(, DeviceProperty) properties;
+
+    /* Do not, under any circumstance, use this parent link below anywhere
+     * outside of qdev.c.  You have been warned. */
+    DeviceState *parent;
 };
 
 typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 17/18] qmp: make qmp.py easier to use

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 QMP/qmp.py |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp.py b/QMP/qmp.py
index c7dbea0..36ecc1d 100644
--- a/QMP/qmp.py
+++ b/QMP/qmp.py
@@ -128,6 +128,12 @@ class QEMUMonitorProtocol:
             qmp_cmd['id'] = id
         return self.cmd_obj(qmp_cmd)
 
+    def command(self, cmd, **kwds):
+        ret = self.cmd(cmd, kwds)
+        if ret.has_key('error'):
+            raise Exception(ret['error']['desc'])
+        return ret['return']
+
     def get_events(self, wait=False):
         """
         Get a list of available QMP events.
-- 
1.7.4.1

[Qemu-devel] [PATCH v2 18/18] qom: add test tools (v2)

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
v1 -> v2
 - fix comments (Stefan)
---
 QMP/qom-get  |   26 ++++++++++++++++++++++++++
 QMP/qom-list |   30 ++++++++++++++++++++++++++++++
 QMP/qom-set  |   21 +++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)
 create mode 100755 QMP/qom-get
 create mode 100755 QMP/qom-list
 create mode 100755 QMP/qom-set

diff --git a/QMP/qom-get b/QMP/qom-get
new file mode 100755
index 0000000..e6c063a
--- /dev/null
+++ b/QMP/qom-get
@@ -0,0 +1,26 @@
+#!/usr/bin/python
+##
+# QEMU Object Model test tools
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+path, prop = sys.argv[1].rsplit('.', 1)
+rsp = srv.command('qom-get', path=path, property=prop)
+if type(rsp) == dict:
+    for i in rsp.keys():
+        print '%s: %s' % (i, rsp[i])
+else:
+    print rsp
diff --git a/QMP/qom-list b/QMP/qom-list
new file mode 100755
index 0000000..b91f814
--- /dev/null
+++ b/QMP/qom-list
@@ -0,0 +1,30 @@
+#!/usr/bin/python
+##
+# QEMU Object Model test tools
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+if len(sys.argv) == 1:
+    print '/'
+    sys.exit(0)
+
+for item in srv.command('qom-list', path=sys.argv[1]):
+    if item['type'].startswith('child<'):
+        print '%s/' % item['name']
+    elif item['type'].startswith('link<'):
+        print '@%s/' % item['name']
+    else:
+        print '%s' % item['name']
diff --git a/QMP/qom-set b/QMP/qom-set
new file mode 100755
index 0000000..c5589d8
--- /dev/null
+++ b/QMP/qom-set
@@ -0,0 +1,21 @@
+#!/usr/bin/python
+##
+# QEMU Object Model test tools
+#
+# Copyright IBM, Corp. 2011
+#
+# Authors:
+#  Anthony Liguori   <aliguori@us.ibm.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
+# the COPYING file in the top-level directory.
+##
+
+import sys
+from qmp import QEMUMonitorProtocol
+
+srv = QEMUMonitorProtocol('/tmp/server.sock')
+srv.connect()
+
+path, prop = sys.argv[1].rsplit('.', 1)
+print srv.command('qom-set', path=path, property=prop, value=sys.argv[2])
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH v2 07/18] qom: add child properties (composition) (v2)

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> Child properties express a relationship of composition.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> v1 -> v2
>  - fix comments (Kevin)
>  - add a reference when adding a child property (Kevin)
> ---
>  hw/qdev.c |   26 ++++++++++++++++++++++++++
>  hw/qdev.h |   20 ++++++++++++++++++++
>  2 files changed, 46 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 2519f00..fa6b489 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -1174,6 +1174,32 @@ DeviceState *qdev_get_root(void)
>      return qdev_root;
>  }
>  
> +static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
> +                                    const char *name, Error **errp)
> +{
> +    DeviceState *child = opaque;
> +    gchar *path;
> +
> +    path = qdev_get_canonical_path(child);
> +    visit_type_str(v, &path, name, errp);
> +    g_free(path);
> +}
> +
> +void qdev_property_add_child(DeviceState *dev, const char *name,
> +                             DeviceState *child, Error **errp)
> +{
> +    gchar *type;
> +
> +    type = g_strdup_printf("child<%s>", child->info->name);
> +
> +    qdev_property_add(dev, name, type, qdev_get_child_property,
> +                      NULL, NULL, child, errp);
> +
> +    qdev_ref(dev);

Shouldn't you increase the refcount for child rather than dev?

Kevin

Re: [Qemu-devel] [PATCH v2 09/18] qapi: allow a 'gen' key to suppress code generation

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  scripts/qapi-commands.py |    1 +
>  scripts/qapi-types.py    |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index f7def16..54d1f5d 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -405,6 +405,7 @@ except os.error, e:
>  
>  exprs = parse_schema(sys.stdin)
>  commands = filter(lambda expr: expr.has_key('command'), exprs)
> +commands = filter(lambda expr: not expr.has_key('gen'), commands)

Might be confusing if someone uses something like gen = True. Shouldn't
we better check the value instead of just existence?

Kevin

Re: [Qemu-devel] [PATCH v2 13/18] dev: add an anonymous peripheral container

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/qdev.c |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index af4c6a2..5348f26 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -240,6 +240,19 @@ static DeviceState *qdev_get_peripheral(void)
>      return dev;
>  }
>  
> +static DeviceState *qdev_get_peripheral_anon(void)
> +{
> +    static DeviceState *dev;
> +
> +    if (dev == NULL) {
> +        dev = qdev_create(NULL, "container");
> +        qdev_property_add_child(qdev_get_root(), "peripheral-anon", dev, NULL);
> +        qdev_init_nofail(dev);
> +    }
> +
> +    return dev;
> +}
> +
>  DeviceState *qdev_device_add(QemuOpts *opts)
>  {
>      const char *driver, *path, *id;
> @@ -292,7 +305,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      if (id) {
>          qdev->id = id;
>          qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
> -    }
> +    } else {
> +        static int anon_count;
> +        gchar *name = g_strdup_printf("device[%d]", anon_count++);

Does any code depend on this name? If not, I would suggest making it a
bit more convenient for users: g_strdump_printf("%s[%d]", info->name,
info->anon_count++)

Kevin

Re: [Qemu-devel] [PATCH v2 13/18] dev: add an anonymous peripheral container

[Anthony Liguori]

On 12/08/2011 10:27 AM, Kevin Wolf wrote:
> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   hw/qdev.c |   21 ++++++++++++++++++++-
>>   1 files changed, 20 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index af4c6a2..5348f26 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -240,6 +240,19 @@ static DeviceState *qdev_get_peripheral(void)
>>       return dev;
>>   }
>>
>> +static DeviceState *qdev_get_peripheral_anon(void)
>> +{
>> +    static DeviceState *dev;
>> +
>> +    if (dev == NULL) {
>> +        dev = qdev_create(NULL, "container");
>> +        qdev_property_add_child(qdev_get_root(), "peripheral-anon", dev, NULL);
>> +        qdev_init_nofail(dev);
>> +    }
>> +
>> +    return dev;
>> +}
>> +
>>   DeviceState *qdev_device_add(QemuOpts *opts)
>>   {
>>       const char *driver, *path, *id;
>> @@ -292,7 +305,13 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>       if (id) {
>>           qdev->id = id;
>>           qdev_property_add_child(qdev_get_peripheral(), qdev->id, qdev, NULL);
>> -    }
>> +    } else {
>> +        static int anon_count;
>> +        gchar *name = g_strdup_printf("device[%d]", anon_count++);
>
> Does any code depend on this name? If not, I would suggest making it a
> bit more convenient for users: g_strdump_printf("%s[%d]", info->name,
> info->anon_count++)

Nothing does and that's a great suggestion, thanks!

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 07/18] qom: add child properties (composition) (v2)

[Anthony Liguori]

On 12/08/2011 09:38 AM, Kevin Wolf wrote:
> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>> Child properties express a relationship of composition.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>> v1 ->  v2
>>   - fix comments (Kevin)
>>   - add a reference when adding a child property (Kevin)
>> ---
>>   hw/qdev.c |   26 ++++++++++++++++++++++++++
>>   hw/qdev.h |   20 ++++++++++++++++++++
>>   2 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/qdev.c b/hw/qdev.c
>> index 2519f00..fa6b489 100644
>> --- a/hw/qdev.c
>> +++ b/hw/qdev.c
>> @@ -1174,6 +1174,32 @@ DeviceState *qdev_get_root(void)
>>       return qdev_root;
>>   }
>>
>> +static void qdev_get_child_property(DeviceState *dev, Visitor *v, void *opaque,
>> +                                    const char *name, Error **errp)
>> +{
>> +    DeviceState *child = opaque;
>> +    gchar *path;
>> +
>> +    path = qdev_get_canonical_path(child);
>> +    visit_type_str(v,&path, name, errp);
>> +    g_free(path);
>> +}
>> +
>> +void qdev_property_add_child(DeviceState *dev, const char *name,
>> +                             DeviceState *child, Error **errp)
>> +{
>> +    gchar *type;
>> +
>> +    type = g_strdup_printf("child<%s>", child->info->name);
>> +
>> +    qdev_property_add(dev, name, type, qdev_get_child_property,
>> +                      NULL, NULL, child, errp);
>> +
>> +    qdev_ref(dev);
>
> Shouldn't you increase the refcount for child rather than dev?

Indeed, I've fixed it in my tree.  Thanks.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 09/18] qapi: allow a 'gen' key to suppress code generation

[Anthony Liguori]

On 12/08/2011 10:04 AM, Kevin Wolf wrote:
> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   scripts/qapi-commands.py |    1 +
>>   scripts/qapi-types.py    |    1 +
>>   2 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> index f7def16..54d1f5d 100644
>> --- a/scripts/qapi-commands.py
>> +++ b/scripts/qapi-commands.py
>> @@ -405,6 +405,7 @@ except os.error, e:
>>
>>   exprs = parse_schema(sys.stdin)
>>   commands = filter(lambda expr: expr.has_key('command'), exprs)
>> +commands = filter(lambda expr: not expr.has_key('gen'), commands)
>
> Might be confusing if someone uses something like gen = True. Shouldn't
> we better check the value instead of just existence?

Yes, it was a lazy short cut.  I'll fix appropriately.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 16/18] qom: optimize qdev_get_canonical_path using a parent link

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> The full tree search was a bit unreasonable.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

> diff --git a/hw/qdev.h b/hw/qdev.h
> index 4351e2e..fdab848 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -92,6 +92,10 @@ struct DeviceState {
>      uint32_t ref;
>  
>      QTAILQ_HEAD(, DeviceProperty) properties;
> +
> +    /* Do not, under any circumstance, use this parent link below anywhere
> +     * outside of qdev.c.  You have been warned. */
> +    DeviceState *parent;
>  };

I would expect that a warning works better if it tells people _why_
their code will break if they do it anyway (it's not clear to me)

Kevin

Re: [Qemu-devel] [PATCH v2 18/18] qom: add test tools (v2)

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
> v1 -> v2
>  - fix comments (Stefan)
> ---
>  QMP/qom-get  |   26 ++++++++++++++++++++++++++
>  QMP/qom-list |   30 ++++++++++++++++++++++++++++++
>  QMP/qom-set  |   21 +++++++++++++++++++++
>  3 files changed, 77 insertions(+), 0 deletions(-)
>  create mode 100755 QMP/qom-get
>  create mode 100755 QMP/qom-list
>  create mode 100755 QMP/qom-set
> 
> diff --git a/QMP/qom-get b/QMP/qom-get
> new file mode 100755
> index 0000000..e6c063a
> --- /dev/null
> +++ b/QMP/qom-get
> @@ -0,0 +1,26 @@
> +#!/usr/bin/python
> +##
> +# QEMU Object Model test tools
> +#
> +# Copyright IBM, Corp. 2011
> +#
> +# Authors:
> +#  Anthony Liguori   <aliguori@us.ibm.com>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.  See
> +# the COPYING file in the top-level directory.
> +##
> +
> +import sys
> +from qmp import QEMUMonitorProtocol
> +
> +srv = QEMUMonitorProtocol('/tmp/server.sock')

Hard coding paths is fine for initial tests, but they probably shouldn't
make their way into the git. :-)

Kevin

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Kevin Wolf]

Am 02.12.2011 21:20, schrieb Anthony Liguori:
> This really shows the power of dynamic object properties compared to qdev
> static properties.
> 
> This property represents a complex structure who's format is preserved over the
> wire.  This is enabled by visitors.
> 
> It also shows an entirely synthetic property that is not tied to device state.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

There's one thing that I was hoping to find answered when I would have
reviewed the whole series, but it hasn't happened: There is no doubt
that dynamic properties (in the sense of being able to modify them after
constructions) are a useful thing. But you also claim that class-based
properties are not enough for QOM and that we need object-based ones,
which is a requirement not immediately obvious to me.

Can you provide some examples where we would explicitly need
object-based properties?

Kevin

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/09/2011 05:26 AM, Kevin Wolf wrote:
> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>> This really shows the power of dynamic object properties compared to qdev
>> static properties.
>>
>> This property represents a complex structure who's format is preserved over the
>> wire.  This is enabled by visitors.
>>
>> It also shows an entirely synthetic property that is not tied to device state.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> There's one thing that I was hoping to find answered when I would have
> reviewed the whole series, but it hasn't happened: There is no doubt
> that dynamic properties (in the sense of being able to modify them after
> constructions) are a useful thing. But you also claim that class-based
> properties are not enough for QOM and that we need object-based ones,
> which is a requirement not immediately obvious to me.
>
> Can you provide some examples where we would explicitly need
> object-based properties?

Sure.  Any property that's dynamic needs to be object based.  A good example 
would be PCI slots.

Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be 
desirable to eventually make this configurable.  So you can imagine where you 
would have an 'slot-count' property and if that was set to 16, it would result 
in 'slot[0]..slot[15]' being created.

There are other good examples too.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Kevin Wolf]

Am 09.12.2011 14:08, schrieb Anthony Liguori:
> On 12/09/2011 05:26 AM, Kevin Wolf wrote:
>> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>>> This really shows the power of dynamic object properties compared to qdev
>>> static properties.
>>>
>>> This property represents a complex structure who's format is preserved over the
>>> wire.  This is enabled by visitors.
>>>
>>> It also shows an entirely synthetic property that is not tied to device state.
>>>
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> There's one thing that I was hoping to find answered when I would have
>> reviewed the whole series, but it hasn't happened: There is no doubt
>> that dynamic properties (in the sense of being able to modify them after
>> constructions) are a useful thing. But you also claim that class-based
>> properties are not enough for QOM and that we need object-based ones,
>> which is a requirement not immediately obvious to me.
>>
>> Can you provide some examples where we would explicitly need
>> object-based properties?
> 
> Sure.  Any property that's dynamic needs to be object based.  A good example 
> would be PCI slots.
> 
> Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be 
> desirable to eventually make this configurable.  So you can imagine where you 
> would have an 'slot-count' property and if that was set to 16, it would result 
> in 'slot[0]..slot[15]' being created.
> 
> There are other good examples too.

So is it mostly about variably sized arrays, which just happen to be
considered independent properties in your approach? Or are there cases
where a logically separate property may be there or missing depending on
some condition, or possibly even that a new property is created during
runtime?

Kevin

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/09/2011 08:04 AM, Kevin Wolf wrote:
> Am 09.12.2011 14:08, schrieb Anthony Liguori:
>> On 12/09/2011 05:26 AM, Kevin Wolf wrote:
>>> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>>>> This really shows the power of dynamic object properties compared to qdev
>>>> static properties.
>>>>
>>>> This property represents a complex structure who's format is preserved over the
>>>> wire.  This is enabled by visitors.
>>>>
>>>> It also shows an entirely synthetic property that is not tied to device state.
>>>>
>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> There's one thing that I was hoping to find answered when I would have
>>> reviewed the whole series, but it hasn't happened: There is no doubt
>>> that dynamic properties (in the sense of being able to modify them after
>>> constructions) are a useful thing. But you also claim that class-based
>>> properties are not enough for QOM and that we need object-based ones,
>>> which is a requirement not immediately obvious to me.
>>>
>>> Can you provide some examples where we would explicitly need
>>> object-based properties?
>>
>> Sure.  Any property that's dynamic needs to be object based.  A good example
>> would be PCI slots.
>>
>> Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be
>> desirable to eventually make this configurable.  So you can imagine where you
>> would have an 'slot-count' property and if that was set to 16, it would result
>> in 'slot[0]..slot[15]' being created.
>>
>> There are other good examples too.
>
> So is it mostly about variably sized arrays, which just happen to be
> considered independent properties in your approach? Or are there cases
> where a logically separate property may be there or missing depending on
> some condition, or possibly even that a new property is created during
> runtime?

So there are three possibilities for properties:

1) Properties have no per-object state, and exist entirely within the classes. 
This is what qdev does today.

2) Properties are defined in the class, but carry per-object state.

3) Properties are defined in the object and carry per-object state.

We definitely can rule out (1).  Stateful properties are needed to implement 
links, composition, and just about anything interesting.

Another way that (3) is useful is that it allows you to create container devices 
that more or less model a PCB.  That's how peripheral[-anon] is implemented and 
I imagine that it will also be useful for implementing "machine" devices.

Of course, you could find a way to special case this with (2) but that's why I 
ended up going with (3).  You can avoid having a lot of special cases this way.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Kevin Wolf]

Am 09.12.2011 15:25, schrieb Anthony Liguori:
> On 12/09/2011 08:04 AM, Kevin Wolf wrote:
>> Am 09.12.2011 14:08, schrieb Anthony Liguori:
>>> On 12/09/2011 05:26 AM, Kevin Wolf wrote:
>>>> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>>>>> This really shows the power of dynamic object properties compared to qdev
>>>>> static properties.
>>>>>
>>>>> This property represents a complex structure who's format is preserved over the
>>>>> wire.  This is enabled by visitors.
>>>>>
>>>>> It also shows an entirely synthetic property that is not tied to device state.
>>>>>
>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>
>>>> There's one thing that I was hoping to find answered when I would have
>>>> reviewed the whole series, but it hasn't happened: There is no doubt
>>>> that dynamic properties (in the sense of being able to modify them after
>>>> constructions) are a useful thing. But you also claim that class-based
>>>> properties are not enough for QOM and that we need object-based ones,
>>>> which is a requirement not immediately obvious to me.
>>>>
>>>> Can you provide some examples where we would explicitly need
>>>> object-based properties?
>>>
>>> Sure.  Any property that's dynamic needs to be object based.  A good example
>>> would be PCI slots.
>>>
>>> Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be
>>> desirable to eventually make this configurable.  So you can imagine where you
>>> would have an 'slot-count' property and if that was set to 16, it would result
>>> in 'slot[0]..slot[15]' being created.
>>>
>>> There are other good examples too.
>>
>> So is it mostly about variably sized arrays, which just happen to be
>> considered independent properties in your approach? Or are there cases
>> where a logically separate property may be there or missing depending on
>> some condition, or possibly even that a new property is created during
>> runtime?
> 
> So there are three possibilities for properties:
> 
> 1) Properties have no per-object state, and exist entirely within the classes. 
> This is what qdev does today.

Not quite sure what you mean by per-object state. The properties are
fields in the XyzState, so they certainly are per-object?

> 2) Properties are defined in the class, but carry per-object state.
> 
> 3) Properties are defined in the object and carry per-object state.
> 
> We definitely can rule out (1).  Stateful properties are needed to implement 
> links, composition, and just about anything interesting.
> 
> Another way that (3) is useful is that it allows you to create container devices 
> that more or less model a PCB.  That's how peripheral[-anon] is implemented and 
> I imagine that it will also be useful for implementing "machine" devices.

What would this look like? The user creates new child/link properties on
the board, and some more automatically created properties somehow
describe the wiring between them?

> Of course, you could find a way to special case this with (2) but that's why I 
> ended up going with (3).  You can avoid having a lot of special cases this way.

I'm not entirely convinced that we really need this, but on the other
hand I don't feel strong enough about it to argue.

Actually I think my real problem isn't about per-object properties
(although they might add unnecessary complexity), but more about going
away from the qdev style of things where you had _one_ struct definition
that nicely described all of the properties in a central place. Instead,
I'm seeing patches that spread property definitions all over the code.

Now I understand that for dynamically created properties (like on your
PCB) this is necessary and can't be avoided. For about 99% of the
devices static definition of properties would be enough, though.

So basically what I'm asking for is getting the static structs back for
the 99% and have common code that parses them and calls the appropriate
functions to actually the properties. The remaining 1% that
creates/deletes properties during runtime and isn't covered can directly
call whatever it needs.

Kevin

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Gerd Hoffmann]

  Hi,

> Now I understand that for dynamically created properties (like on your
> PCB) this is necessary and can't be avoided. For about 99% of the
> devices static definition of properties would be enough, though.
> 
> So basically what I'm asking for is getting the static structs back for
> the 99% and have common code that parses them and calls the appropriate
> functions to actually the properties. The remaining 1% that
> creates/deletes properties during runtime and isn't covered can directly
> call whatever it needs.

Fully agree.  I guess we can even generate those structs in many cases.
 We will parse the ${device}State structs anyway for visitor-based
vmstate, so with some extra declaration we can generate property
descriptions too.  For example this ...

static PCIDeviceInfo intel_hda_info = {
    .qdev.name    = "intel-hda",
    [ ... ]
    .qdev.props   = (Property[]) {
        DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
        DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
        DEFINE_PROP_END_OF_LIST(),
    }
};

... could be just ...

struct IntelHDAState {
    [ ... ]
    /* properties */
    uint32_t debug __property(0);
    uint32_t msi   __property(1);
};

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/13/2011 03:27 AM, Kevin Wolf wrote:
> Am 09.12.2011 15:25, schrieb Anthony Liguori:
>> On 12/09/2011 08:04 AM, Kevin Wolf wrote:
>>> Am 09.12.2011 14:08, schrieb Anthony Liguori:
>>>> On 12/09/2011 05:26 AM, Kevin Wolf wrote:
>>>>> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>>>>>> This really shows the power of dynamic object properties compared to qdev
>>>>>> static properties.
>>>>>>
>>>>>> This property represents a complex structure who's format is preserved over the
>>>>>> wire.  This is enabled by visitors.
>>>>>>
>>>>>> It also shows an entirely synthetic property that is not tied to device state.
>>>>>>
>>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>>
>>>>> There's one thing that I was hoping to find answered when I would have
>>>>> reviewed the whole series, but it hasn't happened: There is no doubt
>>>>> that dynamic properties (in the sense of being able to modify them after
>>>>> constructions) are a useful thing. But you also claim that class-based
>>>>> properties are not enough for QOM and that we need object-based ones,
>>>>> which is a requirement not immediately obvious to me.
>>>>>
>>>>> Can you provide some examples where we would explicitly need
>>>>> object-based properties?
>>>>
>>>> Sure.  Any property that's dynamic needs to be object based.  A good example
>>>> would be PCI slots.
>>>>
>>>> Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be
>>>> desirable to eventually make this configurable.  So you can imagine where you
>>>> would have an 'slot-count' property and if that was set to 16, it would result
>>>> in 'slot[0]..slot[15]' being created.
>>>>
>>>> There are other good examples too.
>>>
>>> So is it mostly about variably sized arrays, which just happen to be
>>> considered independent properties in your approach? Or are there cases
>>> where a logically separate property may be there or missing depending on
>>> some condition, or possibly even that a new property is created during
>>> runtime?
>>
>> So there are three possibilities for properties:
>>
>> 1) Properties have no per-object state, and exist entirely within the classes.
>> This is what qdev does today.
>
> Not quite sure what you mean by per-object state. The properties are
> fields in the XyzState, so they certainly are per-object?

In qdev today, the objects have no state reflecting properties.  They properties 
refer to object state, but the objects have no knowledge of the properties 
themselves.

>
>> 2) Properties are defined in the class, but carry per-object state.
>>
>> 3) Properties are defined in the object and carry per-object state.
>>
>> We definitely can rule out (1).  Stateful properties are needed to implement
>> links, composition, and just about anything interesting.
>>
>> Another way that (3) is useful is that it allows you to create container devices
>> that more or less model a PCB.  That's how peripheral[-anon] is implemented and
>> I imagine that it will also be useful for implementing "machine" devices.
>
> What would this look like? The user creates new child/link properties on
> the board, and some more automatically created properties somehow
> describe the wiring between them?

The way I imagine this all working out is that a user creates does something 
like this:

(qemu) device_add driver=pc,id=pc
(qemu) qom_list /pc
/pc
   /i440fx
   /ide
     /ide1
       /drive[0]
   /net[0]
   /serial[0]

>
>> Of course, you could find a way to special case this with (2) but that's why I
>> ended up going with (3).  You can avoid having a lot of special cases this way.
>
> I'm not entirely convinced that we really need this, but on the other
> hand I don't feel strong enough about it to argue.
>
> Actually I think my real problem isn't about per-object properties
> (although they might add unnecessary complexity), but more about going
> away from the qdev style of things where you had _one_ struct definition
> that nicely described all of the properties in a central place. Instead,
> I'm seeing patches that spread property definitions all over the code.

There isn't one struct as bus properties get inherited, but I understand your 
point.  The problem with "legacy" properties isn't how they're express, it's 
that the mix parsing with the property types.  I have no problem with using 
DEFINE_ style properties and would expect that we would find a way to make the 
current properties have a string and non-string interface.

> Now I understand that for dynamically created properties (like on your
> PCB) this is necessary and can't be avoided. For about 99% of the
> devices static definition of properties would be enough, though.
>
> So basically what I'm asking for is getting the static structs back for
> the 99% and have common code that parses them and calls the appropriate
> functions to actually the properties. The remaining 1% that
> creates/deletes properties during runtime and isn't covered can directly
> call whatever it needs.

If you look at my qom-next branch, the static structs are still there for 99%er 
devices.  I have no immediate plans of removing them.

Regards,

Anthony Liguori

>
> Kevin
>

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Anthony Liguori]

On 12/13/2011 03:48 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> Now I understand that for dynamically created properties (like on your
>> PCB) this is necessary and can't be avoided. For about 99% of the
>> devices static definition of properties would be enough, though.
>>
>> So basically what I'm asking for is getting the static structs back for
>> the 99% and have common code that parses them and calls the appropriate
>> functions to actually the properties. The remaining 1% that
>> creates/deletes properties during runtime and isn't covered can directly
>> call whatever it needs.
>
> Fully agree.  I guess we can even generate those structs in many cases.
>   We will parse the ${device}State structs anyway for visitor-based
> vmstate, so with some extra declaration we can generate property
> descriptions too.  For example this ...
>
> static PCIDeviceInfo intel_hda_info = {
>      .qdev.name    = "intel-hda",
>      [ ... ]
>      .qdev.props   = (Property[]) {
>          DEFINE_PROP_UINT32("debug", IntelHDAState, debug, 0),
>          DEFINE_PROP_UINT32("msi", IntelHDAState, msi, 1),
>          DEFINE_PROP_END_OF_LIST(),
>      }
> };
>
> ... could be just ...
>
> struct IntelHDAState {
>      [ ... ]
>      /* properties */
>      uint32_t debug __property(0);
>      uint32_t msi   __property(1);
> };


Yup, that's where I want to go.  In qom-next, I've started splitting header 
files out specifically so we can do stuff like this.  There's quite a bit of 
work to do before we can really start exploring here but I think it's not that 
much work to get the pc device models cleaned up such that we could run qc 
against the headers.

Regards,

Anthony Liguori

>
> cheers,
>    Gerd
>
>

Re: [Qemu-devel] [PATCH v2 15/18] rtc: add a dynamic property for retrieving the date

[Kevin Wolf]

Am 13.12.2011 14:31, schrieb Anthony Liguori:
> On 12/13/2011 03:27 AM, Kevin Wolf wrote:
>> Am 09.12.2011 15:25, schrieb Anthony Liguori:
>>> On 12/09/2011 08:04 AM, Kevin Wolf wrote:
>>>> Am 09.12.2011 14:08, schrieb Anthony Liguori:
>>>>> On 12/09/2011 05:26 AM, Kevin Wolf wrote:
>>>>>> Am 02.12.2011 21:20, schrieb Anthony Liguori:
>>>>>>> This really shows the power of dynamic object properties compared to qdev
>>>>>>> static properties.
>>>>>>>
>>>>>>> This property represents a complex structure who's format is preserved over the
>>>>>>> wire.  This is enabled by visitors.
>>>>>>>
>>>>>>> It also shows an entirely synthetic property that is not tied to device state.
>>>>>>>
>>>>>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>>>>
>>>>>> There's one thing that I was hoping to find answered when I would have
>>>>>> reviewed the whole series, but it hasn't happened: There is no doubt
>>>>>> that dynamic properties (in the sense of being able to modify them after
>>>>>> constructions) are a useful thing. But you also claim that class-based
>>>>>> properties are not enough for QOM and that we need object-based ones,
>>>>>> which is a requirement not immediately obvious to me.
>>>>>>
>>>>>> Can you provide some examples where we would explicitly need
>>>>>> object-based properties?
>>>>>
>>>>> Sure.  Any property that's dynamic needs to be object based.  A good example
>>>>> would be PCI slots.
>>>>>
>>>>> Today, we unconditionally advertise 32 slots in our ACPI tables.  It could be
>>>>> desirable to eventually make this configurable.  So you can imagine where you
>>>>> would have an 'slot-count' property and if that was set to 16, it would result
>>>>> in 'slot[0]..slot[15]' being created.
>>>>>
>>>>> There are other good examples too.
>>>>
>>>> So is it mostly about variably sized arrays, which just happen to be
>>>> considered independent properties in your approach? Or are there cases
>>>> where a logically separate property may be there or missing depending on
>>>> some condition, or possibly even that a new property is created during
>>>> runtime?
>>>
>>> So there are three possibilities for properties:
>>>
>>> 1) Properties have no per-object state, and exist entirely within the classes.
>>> This is what qdev does today.
>>
>> Not quite sure what you mean by per-object state. The properties are
>> fields in the XyzState, so they certainly are per-object?
> 
> In qdev today, the objects have no state reflecting properties.  They properties 
> refer to object state, but the objects have no knowledge of the properties 
> themselves.

You mean that an object doesn't have access to the property metadata? Or
what else would an object want to know about them?

>>> 2) Properties are defined in the class, but carry per-object state.
>>>
>>> 3) Properties are defined in the object and carry per-object state.
>>>
>>> We definitely can rule out (1).  Stateful properties are needed to implement
>>> links, composition, and just about anything interesting.
>>>
>>> Another way that (3) is useful is that it allows you to create container devices
>>> that more or less model a PCB.  That's how peripheral[-anon] is implemented and
>>> I imagine that it will also be useful for implementing "machine" devices.
>>
>> What would this look like? The user creates new child/link properties on
>> the board, and some more automatically created properties somehow
>> describe the wiring between them?
> 
> The way I imagine this all working out is that a user creates does something 
> like this:
> 
> (qemu) device_add driver=pc,id=pc
> (qemu) qom_list /pc
> /pc
>    /i440fx
>    /ide
>      /ide1
>        /drive[0]
>    /net[0]
>    /serial[0]

So you instantiated an object of the class pc, right? Which of the
properties qom_list shows is per-object rather than per-class?

>>> Of course, you could find a way to special case this with (2) but that's why I
>>> ended up going with (3).  You can avoid having a lot of special cases this way.
>>
>> I'm not entirely convinced that we really need this, but on the other
>> hand I don't feel strong enough about it to argue.
>>
>> Actually I think my real problem isn't about per-object properties
>> (although they might add unnecessary complexity), but more about going
>> away from the qdev style of things where you had _one_ struct definition
>> that nicely described all of the properties in a central place. Instead,
>> I'm seeing patches that spread property definitions all over the code.
> 
> There isn't one struct as bus properties get inherited, but I understand your 
> point.  The problem with "legacy" properties isn't how they're express, it's 
> that the mix parsing with the property types.  I have no problem with using 
> DEFINE_ style properties and would expect that we would find a way to make the 
> current properties have a string and non-string interface.
> 
>> Now I understand that for dynamically created properties (like on your
>> PCB) this is necessary and can't be avoided. For about 99% of the
>> devices static definition of properties would be enough, though.
>>
>> So basically what I'm asking for is getting the static structs back for
>> the 99% and have common code that parses them and calls the appropriate
>> functions to actually the properties. The remaining 1% that
>> creates/deletes properties during runtime and isn't covered can directly
>> call whatever it needs.
> 
> If you look at my qom-next branch, the static structs are still there for 99%er 
> devices.  I have no immediate plans of removing them.

Right, when looking at your serial examples, I saw that there are
actually some of them left. This first series doesn't have any of them.

I think the pattern may be that it's only children and links that are
spread across the code and "primitive types" generally still use the
declarative approach. Is there a reason not to include children/links
there as well?

I also think in the serial example there was quite some boilerplate code
related to such things, that could probably be common code for most
devices that can be described statically in tables. But I think I'll get
to that once you post these patches, maybe I'll understand the reasons
better by then.

Kevin

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 09:33 AM, Peter Maydell wrote:
> On 5 December 2011 15:04, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 12/05/2011 08:50 AM, Peter Maydell wrote:
>>>
>>> On 5 December 2011 14:36, Anthony Liguori<anthony@codemonkey.ws>    wrote:
>>>>
>>>> struct LSIDevice {
>>>>
>>>>     PCIDevice parent;
>>>> };
>>>>
>>>> static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
>>>> {
>>>>    LSIDevice *dev = LSI_DEVICE(bus);
>>>>    ...
>>>> }
>>>
>>>
>>> What is the LSI_DEVICE macro actually doing here? I assume
>>> it's not just a cast...
>>
>>
>> https://github.com/aliguori/qemu/blob/qom-next/hw/object.c#L376
>>
>> It's quite literally dynamic_cast<LSIDevice>(bus) in C++.
>
> This is confusing "interface I expose to the world" with
> Device. Since "interface I expose to the world shouldn't
> be a subtype of Device [they are fundamentally different kinds
> of object and having exposed interfaces be kinds-of Devices
> is extremely confusing] I'm not sure how this would work.

No, an interface is not a subclass of DeviceState.  SCSIBus is a subclass of 
Interface.

I think this will become more obvious once we start converting BusState's to 
interfaces.  That's still a ways off so I'd suggest we defer this discussion 
until then.

>>> Saying a device is-a interface doesn't match reality. Devices
>>> have multiple interfaces with the rest of the world.
>>
>>
>> There are two ways a device can interact with the rest of the world.  It can
>> expose a portion of it's functionality (such as an IRQ) via a child object.
>> This is how it would expose MemoryRegions too.
>
> Perhaps this is terminology confusion again. To me that is 'exposing
> an interface to the rest of the world' -- it's a named point where
> we expose a specific API. (For an MMIO region probably that's just
> "return a MemoryRegion*".)

Yes, what you describe here is possible and will be very common.  What I'm 
talking about as "Interfaces" is a higher level concept.

> The other point of confusion here is that "child objects" are doing
> two things:
>   * composition, ie "my model of the Foo SoC has a child object FooUART
>     which implements its serial ports" (but that is an internal
>     implementation detail and might change in future)

Yup.  Does you see this as a problem?

>   * providing connections to the rest of the world, ie "my Foo SoC has
>     a child object Pin named 'my_signal' which is part of its API
>     contract with the outside world"

Providing an ABI and providing full backwards compatibility are two different 
things.

An ABI means (or API really) that if you ask QEMU to does something, if it 
doesn't fail in a predictable fashion, then what it ends up doing will always be 
the same.

IOW, if I say, -device virtio-blk-pci, it should create a virtio block device 
using the PCI virtio transport.  If it caused QEMU to print device information 
about that device, that's an ABI breakage.

Backwards compatibility is stronger than this.  It says that if you invoke QEMU 
a certain way, it will behave in an indistinguishable way from an older version.

I want QOM to be ABI compatible but not backwards compatible.  I want to give us 
the flexibility to change device names, break up devices, or add devices.  When 
things are removed, it should result in errors.  It should also be possible to 
do enough introspection to understand what is supported without relying on 
version numbers.

We should provide a backwards compatible interface, but it should be much higher 
level.  The way I think about it, qom-* is the non-backwards compatible API 
within QMP, and then we'll introduce higher level interfaces (much like 
query-pci) that will remain long term backwards compatible.

> Obviously it's useful (and neat) if the implementation of these things
> is broadly similar, but I think that because they're conceptually
> totally different things we should strive to pick terminology (and
> type/class names) that maintain and make clear that difference.
>
>> You can take a subset of the exposed children (and perhaps some mapping
>> logic), and for an ad-hoc interface.
>
> This is composition-of-interfaces, right? That would also be useful
> (but probably not needed to start with).
>
>> But sometimes, you want the entire device to act like a specific thing.  In
>> this case, you want the LSIDevice to act like SCSIBus.  Interfaces are just
>> a more formal form of what would otherwise be an ad-hoc interface.
>
> This is something different again -- it's not just composition-of-interfaces
> to provide a SCSI-bus-connector, it's claiming the whole device is-a
> SCSI-bus-connector.
>
> I think we should have one way of doing connections between devices,
> and we should make sure it does what we want. If we immediately say
> "oh, SCSI and PCI connect in a special way" then we've removed a lot
> of the impetus to make the standard connection method usefully flexible
> and convenient.
>
>>> (This is
>>> one of the major reasons why SysBus exists: it provides a suboptimal
>>> but usuable model of this for the two most common kinds of interface,
>>> MMIO regions and random gpio.)
>>
>> My expectation is that most things that use SysBus today would not implement
>> any interfaces.  They would just expose child properties.
>
> Terminology confusion again. They definitely need to be able to expose
> the equivalent of gpio in and out pins, and memory regions, by name.

Yes, child properties.  Pins and MemoryRegions need to be Objects (not 
necessarily DeviceStates).

Regards,

Anthony Liguori

> -- PMM
>

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

On 12/05/2011 05:38 PM, Anthony Liguori wrote:
>>>
>>
>> -device would use parse/print, not get/set. So it would still use the
>> strings.
>
> I don't want -device to have special knowledge of properties.  I want it
> to set the properties are strings.  The property implementation is
> private to Object.

Ok, I see where you're coming from with legacy properties too.  Yeah, 
then it makes sense to rename properties (to legacy<name> perhaps) as 
long as the equivalent "name" property is available that uses visitors. 
  I.e. each qdev property would be registered twice, with two different 
names, one for QOM and one for -device.

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 10:29 AM, Paolo Bonzini wrote:
> On 12/05/2011 05:13 PM, Anthony Liguori wrote:
>>
>>>> Maybe now is the right time to rename the legacy properties to all be
>>>> prefixed with qdev-? That way we don't need to introduce two different
>>>> types for a single property.
>>>
>>> Why do you need such a prefix?
>>
>> To avoid an all-at-once conversion. I don't want to break -device and
>> maintaining string properties make it quite a bit easier to support
>> -device.
>
> -device would use parse/print, not get/set. So it would still use the strings.

I don't want -device to have special knowledge of properties.  I want it to set 
the properties are strings.  The property implementation is private to Object.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

On 12/05/2011 05:13 PM, Anthony Liguori wrote:
>
>>> Maybe now is the right time to rename the legacy properties to all be
>>> prefixed with qdev-? That way we don't need to introduce two different
>>> types for a single property.
>>
>> Why do you need such a prefix?
>
> To avoid an all-at-once conversion.  I don't want to break -device and
> maintaining string properties make it quite a bit easier to support
> -device.

-device would use parse/print, not get/set.  So it would still use the 
strings.

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 09:47 AM, Paolo Bonzini wrote:
> On 12/05/2011 03:36 PM, Anthony Liguori wrote:
>> No. A device is-a interface. Hopefully the above example will make it
>> more clear.
>
> No, but I'm confident that there will be a sane way to access the list of
> interfaces that you embed in the Object type. :)

We definitely need to think through introspection.  There are a few things we 
need to address with introspection:

0) What are all of the classes available

1) What are all of the classes/interfaces implemented by a type

2) What are all of the properties available in a class

(0) and (1) just need QMP interfaces.  It's very straight forward.

(2) is a bit more tricky.  You can instantiate a dummy object and introspect on 
the live object.  Dynamic properties make that a bit challenging through.

>> Maybe now is the right time to rename the legacy properties to all be
>> prefixed with qdev-? That way we don't need to introduce two different
>> types for a single property.
>
> Why do you need such a prefix?

To avoid an all-at-once conversion.  I don't want to break -device and 
maintaining string properties make it quite a bit easier to support -device.

I'd like to leave the string properties in place until we have a good way to 
create objects via QMP.

My rough thinking is that we keep -device around until 2.0.

Regards,

Anthony Liguori

>
> Paolo
>

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

On 12/05/2011 03:36 PM, Anthony Liguori wrote:
> It's to support method inheritance. In qdev, the various DeviceInfo
> structures correspond roughly to the class of the object. When you
> create an ISADeviceInfo (the ISA subclass), you declare it statically.
>
> Any methods you aren't overriding are going to be initialized to zero.
> You want those methods to inherit their values from the base class. To
> do this in qdev, you have to introduce a base-class specific
> registration function (isa_qdev_register).
>
> There's not a lot of discipline in how these functions are implemented
> and generally makes type registration more complicated as you have to
> understand what methods get overridden.

Yeah, that's true.  I think in general our class hierarchy is shallow 
enough that we could live with that, but I appreciate that dynamic 
initialization has advantages.

> links are nullable and usually start out as NULL.
>
> childs are not nullable. I can't really think of a reason why they
> should be nullable. What are you thinking here?

Ok, I understand now better what children are.

> I've thought a lot about bus properties. I've looked at a lot of code at
> this point and for the most part, I think that the reason they even
> exist is because we can't inherit a default set of properties.
>
> SCSI is a good example. The bus properties really make more sense as
> SCSIDevice properties that are inherited.

Yeah, bus properties *are* most of the time properties that you add to 
the abstract class, so...

> I dislike these properties in the first place, but I'd like to find a
> way to convert everything to the QOM type system before we start
> rearchitecting how hotplug works.

... just change them to properties on the abstract class.

>> Perhaps hidden with some macro that lets me just write
>> SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
>> pretty much what all object models do. GObject has
>> G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.
>>
>> If I understood everything so far, then here is my question. Are
>> interfaces properties?
>
> No. A device is-a interface. Hopefully the above example will make it
> more clear.

No, but I'm confident that there will be a sane way to access the list 
of interfaces that you embed in the Object type. :)

>> That's not what I meant. The legacy<> namespace splits the set of QOM
>> properties in two parts, sane ones and legacy ones. That's wrong,
>> because the old broken interface remains there. Worse, it remains
>> there as user-visible API in the JSON etc., and it will remain forever
>> since we
>> cannot get rid of -device overnight.
>>
>> What I suggested is to provide two implementations for each old-style
>> property: an old string-based one (used for -device etc.) and a modern
>> visitor-based one (used for qom_*). In other words, old-style
>> properties would expose both a print/parse legacy interface, and a sane
>> get/set visitor interface. No need for a legacy<> namespace, because
>> new-style consumers would not see the old-style string ABI.
>
> Yeah, I'd like to do something like this but I'm in no rush. I agree
> that when we declare QOM as a supported interface, we should have
> replacements for anything that's in the legacy<> space. That may be from
> some magic Property tricks where we introduce Visitor to parse/print or
> because we introduce new and improved properties.

Yeah, extending Property looks like a feasible plan.  The get/set pair 
is an adaptor between JSON/Visitor-type data and C struct fields, the 
parse/print pair is an adaptor between strings and C struct fields.

> Maybe now is the right time to rename the legacy properties to all be
> prefixed with qdev-? That way we don't need to introduce two different
> types for a single property.

Why do you need such a prefix?

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Peter Maydell]

On 5 December 2011 15:04, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 12/05/2011 08:50 AM, Peter Maydell wrote:
>>
>> On 5 December 2011 14:36, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>>
>>> struct LSIDevice {
>>>
>>>    PCIDevice parent;
>>> };
>>>
>>> static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
>>> {
>>>   LSIDevice *dev = LSI_DEVICE(bus);
>>>   ...
>>> }
>>
>>
>> What is the LSI_DEVICE macro actually doing here? I assume
>> it's not just a cast...
>
>
> https://github.com/aliguori/qemu/blob/qom-next/hw/object.c#L376
>
> It's quite literally dynamic_cast<LSIDevice>(bus) in C++.

This is confusing "interface I expose to the world" with
Device. Since "interface I expose to the world shouldn't
be a subtype of Device [they are fundamentally different kinds
of object and having exposed interfaces be kinds-of Devices
is extremely confusing] I'm not sure how this would work.

>> Saying a device is-a interface doesn't match reality. Devices
>> have multiple interfaces with the rest of the world.
>
>
> There are two ways a device can interact with the rest of the world.  It can
> expose a portion of it's functionality (such as an IRQ) via a child object.
> This is how it would expose MemoryRegions too.

Perhaps this is terminology confusion again. To me that is 'exposing
an interface to the rest of the world' -- it's a named point where
we expose a specific API. (For an MMIO region probably that's just
"return a MemoryRegion*".)

The other point of confusion here is that "child objects" are doing
two things:
 * composition, ie "my model of the Foo SoC has a child object FooUART
   which implements its serial ports" (but that is an internal
   implementation detail and might change in future)
 * providing connections to the rest of the world, ie "my Foo SoC has
   a child object Pin named 'my_signal' which is part of its API
   contract with the outside world"

Obviously it's useful (and neat) if the implementation of these things
is broadly similar, but I think that because they're conceptually
totally different things we should strive to pick terminology (and
type/class names) that maintain and make clear that difference.

> You can take a subset of the exposed children (and perhaps some mapping
> logic), and for an ad-hoc interface.

This is composition-of-interfaces, right? That would also be useful
(but probably not needed to start with).

> But sometimes, you want the entire device to act like a specific thing.  In
> this case, you want the LSIDevice to act like SCSIBus.  Interfaces are just
> a more formal form of what would otherwise be an ad-hoc interface.

This is something different again -- it's not just composition-of-interfaces
to provide a SCSI-bus-connector, it's claiming the whole device is-a
SCSI-bus-connector.

I think we should have one way of doing connections between devices,
and we should make sure it does what we want. If we immediately say
"oh, SCSI and PCI connect in a special way" then we've removed a lot
of the impetus to make the standard connection method usefully flexible
and convenient.

>> (This is
>> one of the major reasons why SysBus exists: it provides a suboptimal
>> but usuable model of this for the two most common kinds of interface,
>> MMIO regions and random gpio.)
>
> My expectation is that most things that use SysBus today would not implement
> any interfaces.  They would just expose child properties.

Terminology confusion again. They definitely need to be able to expose
the equivalent of gpio in and out pins, and memory regions, by name.

-- PMM

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 08:50 AM, Peter Maydell wrote:
> On 5 December 2011 14:36, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> struct LSIDevice {
>>     PCIDevice parent;
>> };
>>
>> static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
>> {
>>    LSIDevice *dev = LSI_DEVICE(bus);
>>    ...
>> }
>
> What is the LSI_DEVICE macro actually doing here? I assume
> it's not just a cast...

https://github.com/aliguori/qemu/blob/qom-next/hw/object.c#L376

It's quite literally dynamic_cast<LSIDevice>(bus) in C++.

>> static void lsi_scsi_bus_initfn(Interface *iface)
>> {
>>    SCSIBus *bus = SCSI_BUS(iface);
>>
>>    bus->command_complete = lsi_command_complete;
>> }
>>
>> TypeInfo lsi_device_info = {
>>   .name = TYPE_LSI,
>>   .parent = TYPE_PCI_DEVICE,
>>   .interfaces = (Interface[]){
>>      {
>>         .name = TYPE_SCSI_BUS,
>>         .interface_initfn = lsi_scsi_bus_initfn,
>>      }, {
>>      }
>>   },
>> };
>>
>> type_register_static(&lsi_device_info);
>>
>>
>>>
>>> Perhaps hidden with some macro that lets me just write
>>> SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
>>> pretty much what all object models do. GObject has
>>> G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.
>>>
>>> If I understood everything so far, then here is my question. Are
>>> interfaces properties?
>>
>>
>> No.  A device is-a interface.  Hopefully the above example will make it more
>> clear.
>
> Saying a device is-a interface doesn't match reality. Devices
> have multiple interfaces with the rest of the world.

There are two ways a device can interact with the rest of the world.  It can 
expose a portion of it's functionality (such as an IRQ) via a child object. 
This is how it would expose MemoryRegions too.

You can take a subset of the exposed children (and perhaps some mapping logic), 
and for an ad-hoc interface.

But sometimes, you want the entire device to act like a specific thing.  In this 
case, you want the LSIDevice to act like SCSIBus.  Interfaces are just a more 
formal form of what would otherwise be an ad-hoc interface.

> (This is
> one of the major reasons why SysBus exists: it provides a suboptimal
> but usuable model of this for the two most common kinds of interface,
> MMIO regions and random gpio.)

My expectation is that most things that use SysBus today would not implement any 
interfaces.  They would just expose child properties.

Regards,

Anthony Liguori


> -- PMM
>

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Peter Maydell]

On 5 December 2011 14:36, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 12/05/2011 03:52 AM, Paolo Bonzini wrote:
> struct SCSIBus {
>   Interface parent;
>   void (*command_complete)(SCSIBus *bus, SCSIRequest *req);
> };
>
> TypeInfo scsi_bus_info = {
>   .name = TYPE_SCSI_BUS,
>   .parent = TYPE_INTERFACE,
> };
>
> type_register_static(&scsi_bus_info);
>
> --------
>
> struct LSIDevice {
>    PCIDevice parent;
> };
>
> static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
> {
>   LSIDevice *dev = LSI_DEVICE(bus);
>   ...
> }

What is the LSI_DEVICE macro actually doing here? I assume
it's not just a cast...

> static void lsi_scsi_bus_initfn(Interface *iface)
> {
>   SCSIBus *bus = SCSI_BUS(iface);
>
>   bus->command_complete = lsi_command_complete;
> }
>
> TypeInfo lsi_device_info = {
>  .name = TYPE_LSI,
>  .parent = TYPE_PCI_DEVICE,
>  .interfaces = (Interface[]){
>     {
>        .name = TYPE_SCSI_BUS,
>        .interface_initfn = lsi_scsi_bus_initfn,
>     }, {
>     }
>  },
> };
>
> type_register_static(&lsi_device_info);
>
>
>>
>> Perhaps hidden with some macro that lets me just write
>> SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
>> pretty much what all object models do. GObject has
>> G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.
>>
>> If I understood everything so far, then here is my question. Are
>> interfaces properties?
>
>
> No.  A device is-a interface.  Hopefully the above example will make it more
> clear.

Saying a device is-a interface doesn't match reality. Devices
have multiple interfaces with the rest of the world. (This is
one of the major reasons why SysBus exists: it provides a suboptimal
but usuable model of this for the two most common kinds of interface,
MMIO regions and random gpio.)

-- PMM

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/05/2011 03:52 AM, Paolo Bonzini wrote:
> On 12/04/2011 10:01 PM, Anthony Liguori wrote:
>>>
>>
>> I've begun the work of introducing proper inheritance. There's a
>> lot going on but the basic idea is:
>>
>> 1) introduce QOM base type (Object), make qdev inherit from it
>>
>> 2) create a dynamic typeinfo based DeviceInfo, make device class
>> point to deviceinfo
>>
>> 3) model qdev hierarchy in QOM
>>
>> 4) starting from the bottom of the hierarchy, remove DeviceInfo
>> subclass and push that functionality into QOM classes
>
> Ok, now we're talking. I looked at the SCSI bits and it looks
> sane---though I still don't understand your loathe for static
> initializers (which also causes a lot of code churn).

It's to support method inheritance.  In qdev, the various DeviceInfo structures 
correspond roughly to the class of the object.  When you create an ISADeviceInfo 
(the ISA subclass), you declare it statically.

Any methods you aren't overriding are going to be initialized to zero.  You want 
those methods to inherit their values from the base class.  To do this in qdev, 
you have to introduce a base-class specific registration function 
(isa_qdev_register).

There's not a lot of discipline in how these functions are implemented and 
generally makes type registration more complicated as you have to understand 
what methods get overridden.

What QOM does (GObject does this too) is use a function to override methods.  I 
agree that it's not as pretty, but it means you can just memcpy() the parent 
class' contents into the derived class.  This makes inheritance much cleaner and 
also means you can register all types through a single interface.

You can see this refactoring happening in that branch.  I'm slowly getting rid 
of all of the DeviceInfo derivatives.  Once everything is using DeviceInfo 
directly, then I'll replace DeviceInfo with TypeInfo.  This is going to be the 
biggest single change in the QOM conversion.  It's going to have to be done by a 
script.

Getting rid of SysBusInfo will probably also require a script.

>> I do want to get rid of qdev busses but I'm in no rush at all. We
>> can do 95% of the necessary refactoring without touching busses and
>> hot plug and I think that's the right strategy.
>
> Yes, as long as you have a plan for them. Some of the buses have data
> so they will have to become objects of their own, with a bidirectional
> link between them and the parent device. (Because I'm not going to
> write code like this for each HBA:
>
> void lsi_set_unit_attention(Device *dev, SCSISense sense)
> {
> LSIDevice *lsi = LSI_DEVICE(dev);
> lsi->unit_attention = sense;
> }
>
> No, I'm not. Over my dead body).

Yes, my current plan is to make buses interfaces which means you would need to 
have getters/setters for state.  Yes, I know that can get ugly quickly.

But this comes down to refactoring the bus interfaces I think.  I honestly can't 
tell you how I would handle this cleaner without sitting down and thinking about 
the SCSI bus interface.

Generally speaking, accessors are a very good thing.  It makes code much easier 
to refactor and you'll notice that a lot of my refactoring patches start by 
adding accessors/wrapper functions that probably should have been there since 
day one.

But I agree, we don't want to end up with something where you add 10 lines 
instead of one line.

>>> Right now I can hardly understand how the composition tree will
>>> work, for example: how do you constrain children to be subclasses
>>> of some class (or to implement some interface)?
>>
>> Composition never involves subclasses. The tree I point you to above
>> is about as complete as it can be right now without doing more qdev
>> conversions. It should answer all of your questions re: composition.
>
> All except one: why can't child (and link too IIRC) properties be created with a
> NULL value?

links are nullable and usually start out as NULL.

childs are not nullable. I can't really think of a reason why they should be 
nullable.  What are you thinking here?

>>> And how do they? How much code churn does that entail, in the
>>> devices and in general? In fact, why do they need conversion at
>>> all? Static properties are special enough to warrant something
>>> special.
>>
>> They can stay around forever (or until someone needs non-static
>> properties) but I strongly suspect that we'll get rid of most of them
>> as part of refactoring.
>>
>> An awful lot of properties deal with connecting to back ends and/or
>> bus addressing. None of that will be needed anymore.
>
> True for PCI, but what about ISA or MMIO devices? They do their own
> address/IRQ decoding. You don't see those properties in many cases
> because they're hidden beneath sysbus magic, but there are hundreds.

I've thought a lot about bus properties.  I've looked at a lot of code at this 
point and for the most part, I think that the reason they even exist is because 
we can't inherit a default set of properties.

SCSI is a good example.  The bus properties really make more sense as SCSIDevice 
properties that are inherited.

I dislike these properties in the first place, but I'd like to find a way to 
convert everything to the QOM type system before we start rearchitecting how 
hotplug works.

>>> Let's write documentation on that already.
>>
>> I have written lots of documentation. Just take a look at the wiki.
>
> It's down. :(

It's up now.

>
>>> Once the type is written as child<RTCState>, you've lost all info
>>> on it. Each property type should have its own representation for
>>> types (it can be implicit, or it can be an enum, or it can be a
>>> DeviceInfo), with a method to pretty-print it.
>>
>> I don't know what you mean by "lost all info on it" but I'm strongly
>> opposed to a "pretty-print" method. Having the pretty printing
>> information in the type is almost never what you want.
>
> I agree---I was talking about pretty-printing *the type itself*. The
> type falls outside the visitor model, which is only concerned about the
> contents.
>
>>>> You may find it odd to hear me say this, but I grow weary of
>>>> adding too much class hierarchy and inheritance with properties.
>>>> Yes, we could make it very sophisticated but in practice it
>>>> doesn't have to be. Properties should be one of two things:
>>>> primitive types or link/childs.
>>>
>>> ... and interfaces. Accessing them by name as a property should
>>> work well.
>>
>> No, not interfaces. The object *is-a* interface. It doesn't has-a
>> interface.>
>>
>> We've gone through this debate multiple times.
>
> You misunderstood (and I wasn't clear enough).
>
> The is-a/has-a debate is indeed settled for things such as PCI-ness
> where we'll keep the current three-level class hierarchy (two abstract,
> one concrete):
>
> Device
> PCIDevice
> ... concrete PCI devices ...
> ISADevice
> ... concrete ISA devices ...
>
> The problem is that, in addition to the is-a class hierarchy, you have
> the interface hierarchy. Here, C-level struct "inheritance" does not
> help you because you do not have a fixed place for the vtable. So, for
> example, instead of
>
> struct SCSIBusInfo {
> .command_complete = lsi_command_complete,
> ...
> };
>
> I'll have something like this:
>
> Interface *interface;
> SCSIBusIface *sbi;
> interface = qdev_add_interface(&dev->qdev, TYPE_SCSI_BUS_INTERFACE);
> sbi = SCSI_BUS_INTERFACE(interface);
> sbi->command_complete = lsi_command_complete;
> ...
>
> Right? Then I create the SCSIBus object with something as simple as:
>
> struct LSIDevice {
> ...
> SCSIBus *bus;
> }
>
> dev->bus = scsi_bus_new(&dev->qdev);
>
> and scsi_bus_new will do something like
>
> Interface *interface;
> interface = qdev_get_interface(dev, TYPE_SCSI_BUS_INTERFACE);
> scsi_bus->sbi = SCSI_BUS_INTERFACE(interface);


No, you would do:

struct SCSIBus {
    Interface parent;
    void (*command_complete)(SCSIBus *bus, SCSIRequest *req);
};

TypeInfo scsi_bus_info = {
    .name = TYPE_SCSI_BUS,
    .parent = TYPE_INTERFACE,
};

type_register_static(&scsi_bus_info);

--------

struct LSIDevice {
     PCIDevice parent;
};

static void lsi_command_complete(SCSIBus *bus, SCSIRequest *req)
{
    LSIDevice *dev = LSI_DEVICE(bus);
    ...
}

static void lsi_scsi_bus_initfn(Interface *iface)
{
    SCSIBus *bus = SCSI_BUS(iface);

    bus->command_complete = lsi_command_complete;
}

TypeInfo lsi_device_info = {
   .name = TYPE_LSI,
   .parent = TYPE_PCI_DEVICE,
   .interfaces = (Interface[]){
      {
         .name = TYPE_SCSI_BUS,
         .interface_initfn = lsi_scsi_bus_initfn,
      }, {
      }
   },
};

type_register_static(&lsi_device_info);

>
> Perhaps hidden with some macro that lets me just write
> SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
> pretty much what all object models do. GObject has
> G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.
>
> If I understood everything so far, then here is my question. Are
> interfaces properties?

No.  A device is-a interface.  Hopefully the above example will make it more clear.

> I'm starting to understand now that the answer
> is probably "no because interfaces have nothing to do with visitors",
> and that's fine. Then I suppose you'll have another list to lookup
> besides properties, no problem with that.

Yes, there's a list of interfaces embedded in the Object type.

>>>> This is where qdev goes very wrong. It mixes up user interface
>>>> details (how to format an integer on the command line) with
>>>> implementation details. There's no reason we should be indicating
>>>> whether a property should be display in hex or decimal in the
>>>> device itself.
>>>
>>> That's totally true. But there's no reason why qdev properties
>>> cannot be split in two parts, a sane one and a legacy one.
>>
>> Bingo! Hence, the 'legacy<>' namespace. If you want to do a
>> declare, struct member based syntax that encodes/decodes as primitive
>> types to a Visitor, be my guest
>
> That's not what I meant. The legacy<> namespace splits the set of QOM
> properties in two parts, sane ones and legacy ones. That's wrong,
> because the old broken interface remains there. Worse, it remains
> there as user-visible API in the JSON etc., and it will remain forever since we
> cannot get rid of -device overnight.
>
> What I suggested is to provide two implementations for each old-style
> property: an old string-based one (used for -device etc.) and a modern
> visitor-based one (used for qom_*). In other words, old-style
> properties would expose both a print/parse legacy interface, and a sane
> get/set visitor interface. No need for a legacy<> namespace, because
> new-style consumers would not see the old-style string ABI.

Yeah, I'd like to do something like this but I'm in no rush.  I agree that when 
we declare QOM as a supported interface, we should have replacements for 
anything that's in the legacy<> space.  That may be from some magic Property 
tricks where we introduce Visitor to parse/print or because we introduce new and 
improved properties.

Maybe now is the right time to rename the legacy properties to all be prefixed 
with qdev-?  That way we don't need to introduce two different types for a 
single property.

Regards,

Anthony Liguori

>
>>> I was thinking of one-off types such as rtc's struct tm.
>>
>> I'd really like to get to a point where these type visitors were
>> being generated.
>
> Yeah, long term that'd be great.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

On 12/04/2011 10:01 PM, Anthony Liguori wrote:
>>
>
> I've begun the work of introducing proper inheritance.  There's a
> lot going on but the basic idea is:
>
> 1) introduce QOM base type (Object), make qdev inherit from it
>
> 2) create a dynamic typeinfo based DeviceInfo, make device class
> point to deviceinfo
>
> 3) model qdev hierarchy in QOM
>
> 4) starting from the bottom of the hierarchy, remove DeviceInfo
> subclass and push that functionality into QOM classes

Ok, now we're talking.  I looked at the SCSI bits and it looks
sane---though I still don't understand your loathe for static
initializers (which also causes a lot of code churn).

> I do want to get rid of qdev busses but I'm in no rush at all.  We
> can do 95% of the necessary refactoring without touching busses and
> hot plug and I think that's the right strategy.

Yes, as long as you have a plan for them.  Some of the buses have data
so they will have to become objects of their own, with a bidirectional
link between them and the parent device.  (Because I'm not going to
write code like this for each HBA:

     void lsi_set_unit_attention(Device *dev, SCSISense sense)
     {
          LSIDevice *lsi = LSI_DEVICE(dev);
          lsi->unit_attention = sense;
     }

No, I'm not.  Over my dead body).

>> Right now I can hardly understand how the composition tree will
>> work, for example: how do you constrain children to be subclasses
>> of some class (or to implement some interface)?
>
> Composition never involves subclasses.  The tree I point you to above
> is about as complete as it can be right now without doing more qdev
> conversions.  It should answer all of your questions re: composition.

All except one: why can't child (and link too IIRC) properties be 
created with a NULL value?

>> And how do they? How much code churn does that entail, in the
>> devices and in general? In fact, why do they need conversion at
>> all? Static properties are special enough to warrant something
>> special.
>
> They can stay around forever (or until someone needs non-static
> properties) but I strongly suspect that we'll get rid of most of them
> as part of refactoring.
>
> An awful lot of properties deal with connecting to back ends and/or
> bus addressing.  None of that will be needed anymore.

True for PCI, but what about ISA or MMIO devices?  They do their own
address/IRQ decoding.  You don't see those properties in many cases
because they're hidden beneath sysbus magic, but there are hundreds.

>> Let's write documentation on that already.
>
> I have written lots of documentation.  Just take a look at the wiki.

It's down. :(

>> Once the type is written as child<RTCState>, you've lost all info
>> on it. Each property type should have its own representation for
>> types (it can be implicit, or it can be an enum, or it can be a
>> DeviceInfo), with a method to pretty-print it.
>
> I don't know what you mean by "lost all info on it" but I'm strongly
> opposed to a "pretty-print" method.  Having the pretty printing
> information in the type is almost never what you want.

I agree---I was talking about pretty-printing *the type itself*.  The
type falls outside the visitor model, which is only concerned about the
contents.

>>> You may find it odd to hear me say this, but I grow weary of
>>> adding too much class hierarchy and inheritance with properties.
>>>  Yes, we could make it very sophisticated but in practice it
>>> doesn't have to be. Properties should be one of two things:
>>> primitive types or link/childs.
>>
>> ... and interfaces. Accessing them by name as a property should
>> work well.
>
> No, not interfaces.  The object *is-a* interface.  It doesn't has-a
> interface.>
>
> We've gone through this debate multiple times.

You misunderstood (and I wasn't clear enough).

The is-a/has-a debate is indeed settled for things such as PCI-ness
where we'll keep the current three-level class hierarchy (two abstract,
one concrete):

     Device
         PCIDevice
             ... concrete PCI devices ...
         ISADevice
             ... concrete ISA devices ...

The problem is that, in addition to the is-a class hierarchy, you have
the interface hierarchy.  Here, C-level struct "inheritance" does not
help you because you do not have a fixed place for the vtable.  So, for
example, instead of

     struct SCSIBusInfo {
         .command_complete = lsi_command_complete,
         ...
     };

I'll have something like this:

     Interface *interface;
     SCSIBusIface *sbi;
     interface = qdev_add_interface(&dev->qdev, TYPE_SCSI_BUS_INTERFACE);
     sbi = SCSI_BUS_INTERFACE(interface);
     sbi->command_complete = lsi_command_complete;
     ...

Right?  Then I create the SCSIBus object with something as simple as:

     struct LSIDevice {
         ...
         SCSIBus *bus;
     }

     dev->bus = scsi_bus_new(&dev->qdev);

and scsi_bus_new will do something like

     Interface *interface;
     interface = qdev_get_interface(dev, TYPE_SCSI_BUS_INTERFACE);
     scsi_bus->sbi = SCSI_BUS_INTERFACE(interface);

Perhaps hidden with some macro that lets me just write
SCSI_BUS_INTERFACE(dev), but that's the idea; such a lookup function is
pretty much what all object models do.  GObject has
G_TYPE_INSTANCE_GET_INTERFACE, COM/XPCOM has QueryInterface, etc.

If I understood everything so far, then here is my question.  Are
interfaces properties?  I'm starting to understand now that the answer
is probably "no because interfaces have nothing to do with visitors",
and that's fine.  Then I suppose you'll have another list to lookup
besides properties, no problem with that.

>>> This is where qdev goes very wrong. It mixes up user interface
>>> details (how to format an integer on the command line) with
>>> implementation details. There's no reason we should be indicating
>>> whether a property should be display in hex or decimal in the
>>> device itself.
>>
>> That's totally true. But there's no reason why qdev properties
>> cannot be split in two parts, a sane one and a legacy one.
>
> Bingo!  Hence, the 'legacy<>' namespace.  If you want to do a
> declare, struct member based syntax that encodes/decodes as primitive
> types to a Visitor, be my guest

That's not what I meant.  The legacy<> namespace splits the set of QOM
properties in two parts, sane ones and legacy ones.  That's wrong,
because the old broken interface remains there.  Worse, it remains
there as user-visible API in the JSON etc., and it will remain forever 
since we cannot get rid of -device overnight.

What I suggested is to provide two implementations for each old-style
property: an old string-based one (used for -device etc.) and a modern
visitor-based one (used for qom_*).  In other words, old-style
properties would expose both a print/parse legacy interface, and a sane
get/set visitor interface.  No need for a legacy<> namespace, because
new-style consumers would not see the old-style string ABI.

>> I was thinking of one-off types such as rtc's struct tm.
>
> I'd really like to get to a point where these type visitors were
> being generated.

Yeah, long term that'd be great.

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/03/2011 03:34 PM, Anthony Liguori wrote:
> On 12/03/2011 08:24 AM, Paolo Bonzini wrote:
>> On 12/03/2011 03:40 AM, Anthony Liguori wrote:
>>> That is still true. The next step, inheritance, will pull the properties
>>> into a base class. That base class can be used elsewhere outside of the
>>> device model.
>>>
>>> But this is already a 20 patch series. If you want all of that in one
>>> series, it's going to be 100 patches that are not terribly easy to
>>> review at once.
>>
>> Without a design document and a roadmap, however, it's impossible to try to
>> understand how the pieces will be together. 100 patches may require some time to
>> digest, but 20 patches require a crystal ball to figure out what's ahead.
>
> You can see a bit further by looking at:
>
> https://github.com/aliguori/qemu/commits/qom-next
>
> That fills out the composition tree pretty well for the pc. The next step is
> aggressive refactoring such that the qdev objects reflect the composition. IOW,
> we should create the rtc from within the piix3 initialization function.

I've begun the work of introducing proper inheritance.  There's a lot going on 
but the basic idea is:

1) introduce QOM base type (Object), make qdev inherit from it

2) create a dynamic typeinfo based DeviceInfo, make device class point to deviceinfo

3) model qdev hierarchy in QOM

4) starting from the bottom of the hierarchy, remove DeviceInfo subclass and 
push that functionality into QOM classes

5) once (4) is complete, remove DeviceInfo

6) refactor any use of multiple child busses into separate devices with one bus

7) refactor busstate as an interface

8) refactor device model to make more aggressive use of composition

9) refactor life cycle events into virtual methods

The tree I've posted is on step (4).

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/03/2011 08:24 AM, Paolo Bonzini wrote:
> On 12/03/2011 03:40 AM, Anthony Liguori wrote:
>> That is still true. The next step, inheritance, will pull the properties
>> into a base class. That base class can be used elsewhere outside of the
>> device model.
>>
>> But this is already a 20 patch series. If you want all of that in one
>> series, it's going to be 100 patches that are not terribly easy to
>> review at once.
>
> Without a design document and a roadmap, however, it's impossible to try to
> understand how the pieces will be together. 100 patches may require some time to
> digest, but 20 patches require a crystal ball to figure out what's ahead.

You can see a bit further by looking at:

https://github.com/aliguori/qemu/commits/qom-next

That fills out the composition tree pretty well for the pc.  The next step is 
aggressive refactoring such that the qdev objects reflect the composition.  IOW, 
we should create the rtc from within the piix3 initialization function.

But before we can do that, we need to split construction and introduce realize 
which requires inheritance.

So I'd like to fill out the composition tree next (HEAD~2 in that branch, but 
split up nicely).  Having a flushed out composition tree really helps figure out 
what the code should look like and my hope is that other platforms can start 
refactoring too.

Inheritance is not terribly hard.  First step is adding a type pointer to 
DeviceState *, and then introducing some dynamic_cast-like macros and then 
touching every file to use them.

Second step is QOM-style methods and polymorphism.  That's not terribly hard either.

Once we have that, we can do realize.

Note that I haven't really talked about busses.  I do want to get rid of qdev 
busses but I'm in no rush at all.  We can do 95% of the necessary refactoring 
without touching busses and hot plug and I think that's the right strategy.

>
>>> Make sure that all required abstractions can be implemented in
>>> terms of a QOM composition tree.
>>
>> Not composition tree, you mean via the link graph.
>
> I mean both, but the link graph is already understandable (1-to-1 is easy).
> Right now I can hardly understand how the composition tree will work, for
> example: how do you constrain children to be subclasses of some class (or to
> implement some interface)?

Composition never involves subclasses.  The tree I point you to above is about 
as complete as it can be right now without doing more qdev conversions.  It 
should answer all of your questions re: composition.

>>> 2) Related to this, you have a lot of nice infrastructure, but (unlike
>>> what you did with QAPI) you haven't provided yet a clear plan for how
>>> to get rid of the old code. You also have only very limited uses of
>>> the infrastructure (see above).
>>
>> Old style properties go away as they're converted to new style properties.
>
> And how do they? How much code churn does that entail, in the devices and in
> general? In fact, why do they need conversion at all? Static properties are
> special enough to warrant something special.

They can stay around forever (or until someone needs non-static properties) but 
I strongly suspect that we'll get rid of most of them as part of refactoring.

An awful lot of properties deal with connecting to back ends and/or bus 
addressing.  None of that will be needed anymore.

>> If you want to see how this all is going to look, look at the old tree
>> :-) That's where we're going to end up.
>
> Let's write documentation on that already.

I have written lots of documentation.  Just take a look at the wiki.  Nothing 
speaks better than code.

>>> It's better for various reasons--type safety and ease of use--even if
>>> it costs some boilerplate. For example the child property should be
>>> as simple as
>>>
>>> +struct ChildDeviceProperty {
>>> + DeviceProperty prop;
>>> + DeviceState *child;
>>> +}
>>> +
>>> +struct DevicePropertyInfo child_device_property_info = {
>>> + .size = sizeof(ChildDeviceProperty);
>>> + .get = qdev_get_child_property,
>>> +};
>>> +
>>> void qdev_property_add_child(DeviceState *dev, const char *name,
>>> DeviceState *child, Error **errp)
>>> {
>>> gchar *type;
>>>
>>> type = g_strdup_printf("child<%s>", child->info->name);
>>>
>>> - qdev_property_add(dev, name, type, qdev_get_child_property,
>>> - NULL, NULL, child, errp);
>>> + prop = (ChildDeviceProperty *)
>>> + qdev_property_add(&child_device_property_info,
>>> + dev, name, type, errp);
>>> +
>>> + /* TODO: check errp, if it is NULL -> return immediately. */
>>> + prop->child = child;
>>
>> This seems quite a bit uglier to me.
>
> I did say it costs some boilerplate, but it's once per type and you said there's
> a dozen of those. The type safety and more flexibility (an arbitrary struct for
> subclasses rather than an opaque pointer) is worth the ugliness.

I disagree.  Take a look at how string properties work in the above tree.  It's 
strongly type safe.  My old qom tree builds properties based on code generation 
too so the whole discussion on safety is somewhat moot.

>
>>> I think also that the type should not be part of DeviceProperty.
>>> Instead it should be defined by subclasses, with the
>>> DevicePropertyInfo providing a function to format the type to a
>>> string.
>>
>> I don't really know what you mean by this.
>
> Once the type is written as child<RTCState>, you've lost all info on it. Each
> property type should have its own representation for types (it can be implicit,
> or it can be an enum, or it can be a DeviceInfo), with a method to pretty-print it.

I don't know what you mean by "lost all info on it" but I'm strongly opposed to 
a "pretty-print" method.  Having the pretty printing information in the type is 
almost never what you want.

>> We jump too quickly at
>> doing conversions without thinking through the underlying interface.
>
> I hoped you had thought it through. ;)

I have :-)  But I take your position as arguing that we should convert half the 
tree before merging anything which I disagree with.

>> You may find it odd to hear me say this, but I grow weary of adding too
>> much class hierarchy and inheritance with properties. Yes, we could make
>> it very sophisticated but in practice it doesn't have to be. Properties
>> should be one of two things: primitive types or link/childs.
>
> ... and interfaces. Accessing them by name as a property should work well.

No, not interfaces.  The object *is-a* interface.  It doesn't has-a interface.

We've gone through this debate multiple times.  As I said above, we really don't 
need to even worry about this until most of the work is done so let's avoid this 
debate for now.  The property infrastructure is flexible enough to support both 
models.

>> This is where qdev goes very wrong. It mixes up user interface details
>> (how to format an integer on the command line) with implementation
>> details. There's no reason we should be indicating whether a property
>> should be display in hex or decimal in the device itself.
>
> That's totally true. But there's no reason why qdev properties cannot be split
> in two parts, a sane one and a legacy one.

Bingo!  Hence, the 'legacy<>' namespace.  If you want to do a declare, struct 
member based syntax that encodes/decodes as primitive types to a Visitor, be my 
guest :-)

I don't think it's usually what we want, but I'm not opposed to it.

>>> For artificial properties you would need one-off classes; but you can
>>> still provide a helper that creates a specialized
>>> DeviceProperty+DevicePropertyInfo from the functions. I'm thinking of
>>> how people implement prototype-based OO on top of class-based OO, but
>>> it might be just a macro.
>>
>> I think you're over thinking the problem. There are going to be maybe a
>> dozen property types and that's it. They all with correspond exactly to
>> C types with the exception of links/children.
>
> I was thinking of one-off types such as rtc's struct tm.

I'd really like to get to a point where these type visitors were being generated.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

On 12/03/2011 03:40 AM, Anthony Liguori wrote:
> That is still true. The next step, inheritance, will pull the properties
> into a base class. That base class can be used elsewhere outside of the
> device model.
>
> But this is already a 20 patch series. If you want all of that in one
> series, it's going to be 100 patches that are not terribly easy to
> review at once.

Without a design document and a roadmap, however, it's impossible to try 
to understand how the pieces will be together.  100 patches may require 
some time to digest, but 20 patches require a crystal ball to figure out 
what's ahead.

>> Make sure that all required abstractions can be implemented in
>> terms of a QOM composition tree.
>
> Not composition tree, you mean via the link graph.

I mean both, but the link graph is already understandable (1-to-1 is 
easy).  Right now I can hardly understand how the composition tree will 
work, for example: how do you constrain children to be subclasses of 
some class (or to implement some interface)?

>> 2) Related to this, you have a lot of nice infrastructure, but (unlike
>> what you did with QAPI) you haven't provided yet a clear plan for how
>> to get rid of the old code. You also have only very limited uses of
>> the infrastructure (see above).
>
> Old style properties go away as they're converted to new style properties.

And how do they?  How much code churn does that entail, in the devices 
and in general?  In fact, why do they need conversion at all?  Static 
properties are special enough to warrant something special.

> If you want to see how this all is going to look, look at the old tree
> :-) That's where we're going to end up.

Let's write documentation on that already.

>> It's better for various reasons--type safety and ease of use--even if
>> it costs some boilerplate. For example the child property should be
>> as simple as
>>
>> +struct ChildDeviceProperty {
>> + DeviceProperty prop;
>> + DeviceState *child;
>> +}
>> +
>> +struct DevicePropertyInfo child_device_property_info = {
>> + .size = sizeof(ChildDeviceProperty);
>> + .get = qdev_get_child_property,
>> +};
>> +
>> void qdev_property_add_child(DeviceState *dev, const char *name,
>> DeviceState *child, Error **errp)
>> {
>> gchar *type;
>>
>> type = g_strdup_printf("child<%s>", child->info->name);
>>
>> - qdev_property_add(dev, name, type, qdev_get_child_property,
>> - NULL, NULL, child, errp);
>> + prop = (ChildDeviceProperty *)
>> + qdev_property_add(&child_device_property_info,
>> + dev, name, type, errp);
>> +
>> + /* TODO: check errp, if it is NULL -> return immediately. */
>> + prop->child = child;
>
> This seems quite a bit uglier to me.

I did say it costs some boilerplate, but it's once per type and you said 
there's a dozen of those.  The type safety and more flexibility (an 
arbitrary struct for subclasses rather than an opaque pointer) is worth 
the ugliness.

>> I think also that the type should not be part of DeviceProperty.
>> Instead it should be defined by subclasses, with the
>> DevicePropertyInfo providing a function to format the type to a
>> string.
>
> I don't really know what you mean by this.

Once the type is written as child<RTCState>, you've lost all info on it. 
  Each property type should have its own representation for types (it 
can be implicit, or it can be an enum, or it can be a DeviceInfo), with 
a method to pretty-print it.

> We jump too quickly at
> doing conversions without thinking through the underlying interface.

I hoped you had thought it through. ;)

> You may find it odd to hear me say this, but I grow weary of adding too
> much class hierarchy and inheritance with properties. Yes, we could make
> it very sophisticated but in practice it doesn't have to be. Properties
> should be one of two things: primitive types or link/childs.

... and interfaces.  Accessing them by name as a property should work well.

> This is where qdev goes very wrong. It mixes up user interface details
> (how to format an integer on the command line) with implementation
> details. There's no reason we should be indicating whether a property
> should be display in hex or decimal in the device itself.

That's totally true.  But there's no reason why qdev properties cannot 
be split in two parts, a sane one and a legacy one.  If you don't do 
this, you take the unmodifiable ABI and force its use as an API 
throughout all QOM.  We can do better.

>> For artificial properties you would need one-off classes; but you can
>> still provide a helper that creates a specialized
>> DeviceProperty+DevicePropertyInfo from the functions. I'm thinking of
>> how people implement prototype-based OO on top of class-based OO, but
>> it might be just a macro.
>
> I think you're over thinking the problem. There are going to be maybe a
> dozen property types and that's it. They all with correspond exactly to
> C types with the exception of links/children.

I was thinking of one-off types such as rtc's struct tm.  Also, besides 
C primitive types there will be property types for structs: for example 
virtio or SCSI requests, S/G lists, and so on.

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Anthony Liguori]

On 12/02/2011 06:56 PM, Paolo Bonzini wrote:
> I have a few comments on the series.  I like it in general, but I
> think it's way too incomplete to be a first incremental step, and it's
> not clear what your plans are about this.  I think I could boil it
> down to two requirements before committing this.
>
> 1) The biggest comment is that you have a huge difference between your
> original proposal and this one:

It's actually exactly the same.  It's just an incremental conversion of qdev 
into QOM.  I can't think of a way to maintain compatibility (and sanity) without 
morphing qdev incrementally into QOM.

> in the full proposal as I remember it,
> link properties work on any QOM object;

That is still true.  The next step, inheritance, will pull the properties into a 
base class.  That base class can be used elsewhere outside of the device model.

But this is already a 20 patch series.  If you want all of that in one series, 
it's going to be 100 patches that are not terribly easy to review at once.

> right now, however, buses are
> not QOM-ified.
>
> This is a problem because a qdev bus mediates between the parent and
> children's interfaces.  You have the ingredients here for doing this
> (the bus can be any device with a link to the parent and child
> properties for the children; children also have backwards link), but
> the bus in the middle right now is not QOM-ified and so the mediation
> cannot be represented in the prototype.
>
> Also, it is not clear how the bus will get hold of this interface from
> the parent.  For the children it is a bit simpler, since the bus has
> access to the DeviceInfo; I'm not sure however what the plan is and
> whether you still plan on keeping the DeviceInfo descendents.
>
> So, my #1 requirement for this to be committed is: Have all
> relationships in the device tree expressed as properties.

This series is about one thing: introducing stable paths.  The fact that link 
properties are being added is just because it's easy.  I'm perfect happy pulling 
out link properties and waiting until we have enough of the object model to 
introduce links.

But I want to get started filling out the composition tree throughout QEMU. 
There's a lot of value in this.  Most notably, we can switch to referencing 
devices based on their canonical path during live migration which means all of 
the non-sense around instance ids can disappear.

> Make sure
> that all required abstractions can be implemented in terms of a QOM
> composition tree.

Not composition tree, you mean via the link graph.  But you're getting ahead of 
yourself.

>
> 2) Related to this, you have a lot of nice infrastructure, but (unlike
> what you did with QAPI) you haven't provided yet a clear plan for how
> to get rid of the old code.  You also have only very limited uses of
> the infrastructure (see above).

Old style properties go away as they're converted to new style properties.

> And it's not clear how the infrastructure will evolve.  For example,
> how do you plan to implement the realization phase that you had in the
> original model?

That's step three.  realize is a property but it's best implemented as a virtual 
function that is dispatched.  We can't do this until we have virtual method 
dispatch.

If you want to see how this all is going to look, look at the old tree :-) 
That's where we're going to end up.

> 3) I still don't understand why the vtable need to be per-property and
> not per-class.  I see no reason not to have DevicePropertyInfo
> (statically defined) + DeviceProperty (a pointer to DevicePropertyInfo
> + the queue entry + name/type/whatever).  Please do make an attempt at
> using composition to derive property types, as this is in general what
> _you_ gave in the past as the direction for QEMU (see Notifiers).
> It's better for various reasons--type safety and ease of use--even if
> it costs some boilerplate.  For example the child property should be
> as simple as
>
> +struct ChildDeviceProperty {
> +     DeviceProperty prop;
> +     DeviceState *child;
> +}
> +
> +struct DevicePropertyInfo child_device_property_info = {
> +    .size = sizeof(ChildDeviceProperty);
> +    .get = qdev_get_child_property,
> +};
> +
>   void qdev_property_add_child(DeviceState *dev, const char *name,
>                                DeviceState *child, Error **errp)
>   {
>       gchar *type;
>
>       type = g_strdup_printf("child<%s>", child->info->name);
>
> -    qdev_property_add(dev, name, type, qdev_get_child_property,
> -                      NULL, NULL, child, errp);
> +    prop = (ChildDeviceProperty *)
> +        qdev_property_add(&child_device_property_info,
> +                      dev, name, type, errp);
> +
> +    /* TODO: check errp, if it is NULL ->  return immediately.  */
> +    prop->child = child;

This seems quite a bit uglier to me.

> +
> +     /* Shouldn't this ref the child instead??  Also, where is the matching
> +       unref??  Needs a release method? */
>       qdev_ref(dev);

Yup, that's a bug.  Thanks.

In terms of unref, we need to tie life cycles.  That's probably something that 
should go in this patch (and this is how it works in my qom branch btw).

>
>       g_free(type);
> }
>
> I think also that the type should not be part of DeviceProperty.
> Instead it should be defined by subclasses, with the
> DevicePropertyInfo providing a function to format the type to a
> string.

I don't really know what you mean by this.

>
> Also in favor of containment and a property hierarchy, there is no
> reason why Property cannot become a subclass of DeviceProperty.  The
> DeviceInfo/BusInfo would contain a prototype that you can clone to the
> heap when building the device's dynamic properties.  If instead you
> want the declarative nature of qdev properties to be replaced by
> imperative descriptions, that's fine, but please provide a proof of
> concept of how to do the conversion (a perl script would be fine, even
> if it works only for 2-3 device models).

I think you see just as well as I do that it's not that complicated.  Working 
out the details of how properties are implemented are more important IMHO than 
starting to do conversions.  We jump too quickly at doing conversions without 
thinking through the underlying interface.

You may find it odd to hear me say this, but I grow weary of adding too much 
class hierarchy and inheritance with properties.  Yes, we could make it very 
sophisticated but in practice it doesn't have to be.  Properties should be one 
of two things: primitive types or link/childs.

This is where qdev goes very wrong.  It mixes up user interface details (how to 
format an integer on the command line) with implementation details.  There's no 
reason we should be indicating whether a property should be display in hex or 
decimal in the device itself.

> For artificial properties you would need one-off classes; but you can
> still provide a helper that creates a specialized
> DeviceProperty+DevicePropertyInfo from the functions.  I'm thinking of
> how people implement prototype-based OO on top of class-based OO, but
> it might be just a macro.

I think you're over thinking the problem.  There are going to be maybe a dozen 
property types and that's it.  They all with correspond exactly to C types with 
the exception of links/children.

> I wouldn't make this a requirement, but you really need a stronger
> justification that what you gave so far.
>
> 4) I really hate naming something as "legacy", particularly when we
> have hundreds of examples and no plan for converting them.  In fact,
> there's nothing legacy in them except the string-based parsing.  Even
> if you convert them away from the Property model to the visitor model,
> they do not disappear, not at all.  They're simply not dynamic.  If
> you call them legacy, I might be worried that I have to replace all
>
>       dev->num_queues
>
> with
>
>      ((int) (intptr_t) dev->num_queues_prop->opaque)
>
> So, my #2 requirement is to make it clear what the fate will be for
> "legacy" properties, and possibly make enough conversion work that
> they can just be called "static".  Do consider converting the
> parse/print functions in Property to visitor-style (it's trivial).
> Then provide a "legacy" bridge in the _reverse_ direction: there are
> hundreds of qdev properties, and just a handful of users of them.

The trouble with the legacy properties are how they mix stringification with the 
properties themselves.  In order to preserve compatibility, we're probably going 
to have to do something like mark all legacy properties deprecated, replace them 
with different named properties, and declare a flag day where we remove all 
legacy properties.

Yes, it sucks, but it's part of the ABI that we cannot now change.  We have to 
treat the input to pcidevice.address as a hex integer in a string.

Regards,

Anthony Liguori

> Going to bed now...
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2 00/18] qom: dynamic properties and composition tree (v2)

[Paolo Bonzini]

I have a few comments on the series.  I like it in general, but I
think it's way too incomplete to be a first incremental step, and it's
not clear what your plans are about this.  I think I could boil it
down to two requirements before committing this.

1) The biggest comment is that you have a huge difference between your
original proposal and this one: in the full proposal as I remember it,
link properties work on any QOM object; right now, however, buses are
not QOM-ified.

This is a problem because a qdev bus mediates between the parent and
children's interfaces.  You have the ingredients here for doing this
(the bus can be any device with a link to the parent and child
properties for the children; children also have backwards link), but
the bus in the middle right now is not QOM-ified and so the mediation
cannot be represented in the prototype.

Also, it is not clear how the bus will get hold of this interface from
the parent.  For the children it is a bit simpler, since the bus has
access to the DeviceInfo; I'm not sure however what the plan is and
whether you still plan on keeping the DeviceInfo descendents.

So, my #1 requirement for this to be committed is: Have all
relationships in the device tree expressed as properties.  Make sure
that all required abstractions can be implemented in terms of a QOM
composition tree.

2) Related to this, you have a lot of nice infrastructure, but (unlike
what you did with QAPI) you haven't provided yet a clear plan for how
to get rid of the old code.  You also have only very limited uses of
the infrastructure (see above).

And it's not clear how the infrastructure will evolve.  For example,
how do you plan to implement the realization phase that you had in the
original model?

(Regarding this, I'm partially guilty of the same with my SCSI
refactoring.  In the end almost everything came nicely out of the
refactoring, but I did have to backtrack a couple of times.  But the
damage I could do, and the churn caused by the backtracking, was much
more limited than with something like QOM!)

3) I still don't understand why the vtable need to be per-property and
not per-class.  I see no reason not to have DevicePropertyInfo
(statically defined) + DeviceProperty (a pointer to DevicePropertyInfo
+ the queue entry + name/type/whatever).  Please do make an attempt at
using composition to derive property types, as this is in general what
_you_ gave in the past as the direction for QEMU (see Notifiers).
It's better for various reasons--type safety and ease of use--even if
it costs some boilerplate.  For example the child property should be
as simple as

+struct ChildDeviceProperty {
+     DeviceProperty prop;
+     DeviceState *child;
+}
+
+struct DevicePropertyInfo child_device_property_info = {
+    .size = sizeof(ChildDeviceProperty);
+    .get = qdev_get_child_property,
+};
+
 void qdev_property_add_child(DeviceState *dev, const char *name,
                              DeviceState *child, Error **errp)
 {
     gchar *type;

     type = g_strdup_printf("child<%s>", child->info->name);

-    qdev_property_add(dev, name, type, qdev_get_child_property,
-                      NULL, NULL, child, errp);
+    prop = (ChildDeviceProperty *)
+        qdev_property_add(&child_device_property_info,
+                      dev, name, type, errp);
+
+    /* TODO: check errp, if it is NULL -> return immediately.  */
+    prop->child = child;
+
+     /* Shouldn't this ref the child instead??  Also, where is the matching
+       unref??  Needs a release method? */
     qdev_ref(dev);

     g_free(type);
}

I think also that the type should not be part of DeviceProperty.
Instead it should be defined by subclasses, with the
DevicePropertyInfo providing a function to format the type to a
string.

Also in favor of containment and a property hierarchy, there is no
reason why Property cannot become a subclass of DeviceProperty.  The
DeviceInfo/BusInfo would contain a prototype that you can clone to the
heap when building the device's dynamic properties.  If instead you
want the declarative nature of qdev properties to be replaced by
imperative descriptions, that's fine, but please provide a proof of
concept of how to do the conversion (a perl script would be fine, even
if it works only for 2-3 device models).

For artificial properties you would need one-off classes; but you can
still provide a helper that creates a specialized
DeviceProperty+DevicePropertyInfo from the functions.  I'm thinking of
how people implement prototype-based OO on top of class-based OO, but
it might be just a macro.

I wouldn't make this a requirement, but you really need a stronger
justification that what you gave so far.

4) I really hate naming something as "legacy", particularly when we
have hundreds of examples and no plan for converting them.  In fact,
there's nothing legacy in them except the string-based parsing.  Even
if you convert them away from the Property model to the visitor model,
they do not disappear, not at all.  They're simply not dynamic.  If
you call them legacy, I might be worried that I have to replace all

     dev->num_queues

with

    ((int) (intptr_t) dev->num_queues_prop->opaque)

So, my #2 requirement is to make it clear what the fate will be for
"legacy" properties, and possibly make enough conversion work that
they can just be called "static".  Do consider converting the
parse/print functions in Property to visitor-style (it's trivial).
Then provide a "legacy" bridge in the _reverse_ direction: there are
hundreds of qdev properties, and just a handful of users of them.

Going to bed now...

Paolo