[Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Marcel Apfelbaum]

Most of the "Cc" list is due to patch 8: (Should I send each patch to a different list?)
    machine-opts: replace qemu_opt_get by QOM QemuMachine queries.

Status:
    - machine_opts are mapped into QemuMachineState's properties,
      which can be queried as regular QOM properties.
    - Subclassing QemuMachineClass allows to add a command line
      option specific to a machine type, error mechanism should
      work if this option is used on another machine. (Not tested, on the todo list.)
    - Next big step would be to completely remove the qemu machines initialization
      and replace it by regular QOM type registration.

RFC v1 -> RFC v2
	Replaced QemuOpts access by QOM queries. (The main addition)
    Addressed Paolo Bonzini's comments:
    - Eliminated duplicate fields (of QEMUMachineInitArgs and QemuMachineState)
      I am not sure about this one, it does mess with the "const" usage.
      Maybe delay this duplication removal until after QEMUMachineInitArgs disappears
      completely?
    - Added "machine-" prefix to QOM machine type.
    - An instance of QEMUMachineInitArgs os is used by QemuMachineState and not a pointer.

The main benefit of QOMifying the qemu machine would be the possibility
to have options per machine type and not global.
However, there are other benefits as:
  - accessing qemu object properties instead of a global QemuOpts
    list from different code subsystems.
  - improving the machine "initialization" code (compat and stuff)
  - getting more close to QOM's vision of single interface for
    device creation and so on.

Basically the series aims (in the long run) to convert:
    QEMUMachine -> QemuMachineClass
    QEMUMachineInitArgs -> QemuMachineState.
As a first step, in order to make possible an incremental development,
both QEMUMachine and QEMUMachineInitArgs are being embedded into the
new types.

Your comments are welcomed,
Marcel

Marcel Apfelbaum (9):
  hw/core: introduced qemu machine as QOM object
  vl: use qemu machine QOM class instead of global machines list
  hw/boards: converted current_machine to be an instance of
    QemuMachineCLass
  hw/machine: add qemu machine opts as properties to QemuMachineState
  qapi: output visitor crashes qemu if it encounters a NULL value
  vl.c: do not set 'type' property in obj_set_property
  qom: add object_property_is_set
  machine-opts: replace qemu_opt_get by QOM QemuMachine queries
  hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove
    duplication

 device-hotplug.c          |   4 +-
 device_tree.c             |   9 +-
 exec.c                    |  21 +++-
 hw/arm/boot.c             |   3 +-
 hw/core/Makefile.objs     |   2 +-
 hw/core/machine.c         | 289 ++++++++++++++++++++++++++++++++++++++++++++++
 hw/microblaze/boot.c      |  13 ++-
 hw/ppc/e500.c             |  21 ++--
 hw/ppc/spapr.c            |  14 ++-
 hw/ppc/virtex_ml507.c     |   3 +-
 include/hw/boards.h       |  60 +++++++++-
 include/qom/object.h      |  11 ++
 kvm-all.c                 |  11 +-
 qapi/qmp-output-visitor.c |   5 +
 qmp.c                     |   7 +-
 qom/object.c              |  12 ++
 target-i386/kvm.c         |  10 +-
 vl.c                      | 150 ++++++++++++++++--------
 18 files changed, 555 insertions(+), 90 deletions(-)
 create mode 100644 hw/core/machine.c

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 1/9] hw/core: introduced qemu machine as QOM object

[Marcel Apfelbaum]

The main functionality change is to convert QEMUMachine into QemuMachineClass
and QEMUMachineInitArgs into QemuMachineState, instance of QemuMachineClass.

As a first step, in order to make possible an incremental developement,
both QEMUMachine and QEMUMachineInitArgs are being embeded into the
new types.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/core/Makefile.objs |  2 +-
 hw/core/machine.c     | 38 +++++++++++++++++++++++++++++++++++++
 include/hw/boards.h   | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 1 deletion(-)
 create mode 100644 hw/core/machine.c

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 9e324be..f80c13c 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -11,4 +11,4 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
 common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
-
+common-obj-$(CONFIG_SOFTMMU) += machine.o
diff --git a/hw/core/machine.c b/hw/core/machine.c
new file mode 100644
index 0000000..2c6e1a3
--- /dev/null
+++ b/hw/core/machine.c
@@ -0,0 +1,38 @@
+/*
+ * QEMU Machine
+ *
+ * Copyright (C) 2013 Red Hat Inc
+ *
+ * Authors:
+ *   Marcel Apfelbaum <marcel.a@redhat.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.
+ */
+
+#include "hw/boards.h"
+
+static void qemu_machine_initfn(Object *obj)
+{
+}
+
+static void qemu_machine_class_init(ObjectClass *oc, void *data)
+{
+}
+
+static const TypeInfo qemu_machine_info = {
+    .name = TYPE_QEMU_MACHINE,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(QemuMachineClass),
+    .class_init = qemu_machine_class_init,
+    .instance_size = sizeof(QemuMachineState),
+    .instance_init = qemu_machine_initfn,
+};
+
+static void register_types(void)
+{
+    type_register_static(&qemu_machine_info);
+}
+
+type_init(register_types);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2151460..7b4708d 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -5,6 +5,7 @@
 
 #include "sysemu/blockdev.h"
 #include "hw/qdev.h"
+#include "qom/object.h"
 
 typedef struct QEMUMachine QEMUMachine;
 
@@ -53,4 +54,55 @@ QEMUMachine *find_default_machine(void);
 
 extern QEMUMachine *current_machine;
 
+#define TYPE_QEMU_MACHINE "machine"
+#define QEMU_MACHINE(obj) \
+    OBJECT_CHECK(QemuMachineState, (obj), TYPE_QEMU_MACHINE)
+#define QEMU_MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(QemuMachineClass, (obj), TYPE_QEMU_MACHINE)
+#define QEMU_MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(QemuMachineClass, (klass), TYPE_QEMU_MACHINE)
+
+typedef struct QemuMachineState QemuMachineState;
+typedef struct QemuMachineClass QemuMachineClass;
+
+/**
+ * @QemuMachineClass
+ *
+ * @parent_class: opaque parent class container
+ */
+struct QemuMachineClass {
+    ObjectClass parent_class;
+
+    QEMUMachine *qemu_machine;
+};
+
+/**
+ * @QemuMachineState
+ *
+ * @parent: opaque parent object container
+ */
+struct QemuMachineState {
+    /* private */
+    Object parent;
+    /* public */
+
+
+    char *accel;
+    bool kernel_irqchip;
+    int kvm_shadow_mem;
+    char *kernel;
+    char *initrd;
+    char *append;
+    char *dtb;
+    char *dumpdtb;
+    int phandle_start;
+    char *dt_compatible;
+    bool dump_guest_core;
+    bool mem_merge;
+    bool usb;
+    char *firmware;
+
+    QEMUMachineInitArgs init_args;
+};
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Marcel Apfelbaum]

The machine registration flow is refactored to use the QOM functionality.
Instead of linking the machines into a list, each machine has a type
and the types can be traversed in the QOM way.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/hw/boards.h |  1 +
 vl.c                | 75 ++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 55 insertions(+), 21 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 7b4708d..65e1e03 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -49,6 +49,7 @@ struct QEMUMachine {
     const char *hw_version;
 };
 
+#define TYPE_QEMU_MACHINE_PREFIX "machine-"
 int qemu_register_machine(QEMUMachine *m);
 QEMUMachine *find_default_machine(void);
 
diff --git a/vl.c b/vl.c
index 9379d33..50c880f 100644
--- a/vl.c
+++ b/vl.c
@@ -1529,54 +1529,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
 /***********************************************************/
 /* machine registration */
 
-static QEMUMachine *first_machine = NULL;
 QEMUMachine *current_machine = NULL;
 
+static void qemu_machine_class_init(ObjectClass *klass, void *data)
+{
+    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);
+
+    k->qemu_machine = data;
+}
+
 int qemu_register_machine(QEMUMachine *m)
 {
-    QEMUMachine **pm;
-    pm = &first_machine;
-    while (*pm != NULL)
-        pm = &(*pm)->next;
-    m->next = NULL;
-    *pm = m;
+    TypeInfo ti = {
+        .name       = g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->name, NULL),
+        .parent     = TYPE_QEMU_MACHINE,
+        .class_init = qemu_machine_class_init,
+        .class_data = (void *)m,
+    };
+
+    type_register(&ti);
+
     return 0;
 }
 
 static QEMUMachine *find_machine(const char *name)
 {
-    QEMUMachine *m;
+    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
+    QEMUMachine *m = NULL;
+
+    for (el = machines; el; el = el->next) {
+        QemuMachineClass *k = el->data;
 
-    for(m = first_machine; m != NULL; m = m->next) {
-        if (!strcmp(m->name, name))
-            return m;
-        if (m->alias && !strcmp(m->alias, name))
-            return m;
+        if (!strcmp(k->qemu_machine->name, name)) {
+            m = k->qemu_machine;
+            break;
+        }
+        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
+            m = k->qemu_machine;
+            break;
+        }
     }
-    return NULL;
+
+    g_slist_free(machines);
+    return m;
 }
 
 QEMUMachine *find_default_machine(void)
 {
-    QEMUMachine *m;
+    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
+    QEMUMachine *m = NULL;
 
-    for(m = first_machine; m != NULL; m = m->next) {
-        if (m->is_default) {
-            return m;
+    for (el = machines; el; el = el->next) {
+        QemuMachineClass *k = el->data;
+
+        if (k->qemu_machine->is_default) {
+            m = k->qemu_machine;
+            break;
         }
     }
-    return NULL;
+
+    g_slist_free(machines);
+    return m;
 }
 
 MachineInfoList *qmp_query_machines(Error **errp)
 {
+    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
     MachineInfoList *mach_list = NULL;
     QEMUMachine *m;
 
-    for (m = first_machine; m; m = m->next) {
+    for (el = machines; el; el = el->next) {
+        QemuMachineClass *k = el->data;
         MachineInfoList *entry;
         MachineInfo *info;
 
+        m = k->qemu_machine;
         info = g_malloc0(sizeof(*info));
         if (m->is_default) {
             info->has_is_default = true;
@@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
         mach_list = entry;
     }
 
+    g_slist_free(machines);
     return mach_list;
 }
 
@@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname)
 static QEMUMachine *machine_parse(const char *name)
 {
     QEMUMachine *m, *machine = NULL;
+    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
 
     if (name) {
         machine = find_machine(name);
@@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char *name)
         return machine;
     }
     printf("Supported machines are:\n");
-    for (m = first_machine; m != NULL; m = m->next) {
+    for (el = machines; el; el = el->next) {
+        QemuMachineClass *k = el->data;
+        m = k->qemu_machine;
         if (m->alias) {
             printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
         }
         printf("%-20s %s%s\n", m->name, m->desc,
                m->is_default ? " (default)" : "");
     }
+
+    g_slist_free(machines);
     exit(!name || !is_help_option(name));
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass

[Marcel Apfelbaum]

In order to allow attaching machine options to a machine instance,
current_machine is converted into QemuMachineState.
As a first step of deprecating QEMUMachine, some of the functions
were modified to return QemuMachineCLass.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 device-hotplug.c    |  4 +++-
 include/hw/boards.h |  6 ++---
 qmp.c               |  7 ++++--
 vl.c                | 69 +++++++++++++++++++++++++++++++----------------------
 4 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/device-hotplug.c b/device-hotplug.c
index 103d34a..fb5eb01 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -33,12 +33,14 @@ DriveInfo *add_init_drive(const char *optstr)
 {
     DriveInfo *dinfo;
     QemuOpts *opts;
+    QemuMachineClass *machine_class;
 
     opts = drive_def(optstr);
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->block_default_type);
+    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
+    dinfo = drive_init(opts, machine_class->qemu_machine->block_default_type);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 65e1e03..053c113 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -51,9 +51,6 @@ struct QEMUMachine {
 
 #define TYPE_QEMU_MACHINE_PREFIX "machine-"
 int qemu_register_machine(QEMUMachine *m);
-QEMUMachine *find_default_machine(void);
-
-extern QEMUMachine *current_machine;
 
 #define TYPE_QEMU_MACHINE "machine"
 #define QEMU_MACHINE(obj) \
@@ -66,6 +63,9 @@ extern QEMUMachine *current_machine;
 typedef struct QemuMachineState QemuMachineState;
 typedef struct QemuMachineClass QemuMachineClass;
 
+QemuMachineClass *find_default_machine(void);
+extern QemuMachineState *current_machine;
+
 /**
  * @QemuMachineClass
  *
diff --git a/qmp.c b/qmp.c
index d0d98e7..df5d8d9 100644
--- a/qmp.c
+++ b/qmp.c
@@ -114,8 +114,11 @@ void qmp_cpu(int64_t index, Error **errp)
 
 void qmp_cpu_add(int64_t id, Error **errp)
 {
-    if (current_machine->hot_add_cpu) {
-        current_machine->hot_add_cpu(id, errp);
+    QemuMachineClass *machine_class;
+
+    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
+    if (machine_class->qemu_machine->hot_add_cpu) {
+        machine_class->qemu_machine->hot_add_cpu(id, errp);
     } else {
         error_setg(errp, "Not supported");
     }
diff --git a/vl.c b/vl.c
index 50c880f..c4939ef 100644
--- a/vl.c
+++ b/vl.c
@@ -1529,7 +1529,7 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
 /***********************************************************/
 /* machine registration */
 
-QEMUMachine *current_machine = NULL;
+QemuMachineState *current_machine;
 
 static void qemu_machine_class_init(ObjectClass *klass, void *data)
 {
@@ -1552,44 +1552,45 @@ int qemu_register_machine(QEMUMachine *m)
     return 0;
 }
 
-static QEMUMachine *find_machine(const char *name)
+static QemuMachineClass *find_machine(const char *name)
 {
     GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
-    QEMUMachine *m = NULL;
+    QemuMachineClass *k = NULL;
 
     for (el = machines; el; el = el->next) {
-        QemuMachineClass *k = el->data;
+        QemuMachineClass *temp = el->data;
 
-        if (!strcmp(k->qemu_machine->name, name)) {
-            m = k->qemu_machine;
+        if (!strcmp(temp->qemu_machine->name, name)) {
+            k = temp;
             break;
         }
-        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
-            m = k->qemu_machine;
+        if (temp->qemu_machine->alias &&
+            !strcmp(temp->qemu_machine->alias, name)) {
+            k = temp;
             break;
         }
     }
 
     g_slist_free(machines);
-    return m;
+    return k;
 }
 
-QEMUMachine *find_default_machine(void)
+QemuMachineClass *find_default_machine(void)
 {
     GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
-    QEMUMachine *m = NULL;
+    QemuMachineClass *k = NULL;
 
     for (el = machines; el; el = el->next) {
-        QemuMachineClass *k = el->data;
+        QemuMachineClass *temp = el->data;
 
-        if (k->qemu_machine->is_default) {
-            m = k->qemu_machine;
+        if (temp->qemu_machine->is_default) {
+            k = temp;
             break;
         }
     }
 
     g_slist_free(machines);
-    return m;
+    return k;
 }
 
 MachineInfoList *qmp_query_machines(Error **errp)
@@ -1818,8 +1819,13 @@ void qemu_devices_reset(void)
 
 void qemu_system_reset(bool report)
 {
-    if (current_machine && current_machine->reset) {
-        current_machine->reset();
+    QemuMachineClass *machine_class;
+
+    machine_class = current_machine ? QEMU_MACHINE_GET_CLASS(current_machine)
+                    : NULL;
+
+    if (machine_class && machine_class->qemu_machine->reset) {
+        machine_class->qemu_machine->reset();
     } else {
         qemu_devices_reset();
     }
@@ -2565,21 +2571,21 @@ static int debugcon_parse(const char *devname)
     return 0;
 }
 
-static QEMUMachine *machine_parse(const char *name)
+static QemuMachineClass *machine_parse(const char *name)
 {
-    QEMUMachine *m, *machine = NULL;
+    QemuMachineClass *machine_class = NULL;
     GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
 
     if (name) {
-        machine = find_machine(name);
+        machine_class = find_machine(name);
     }
-    if (machine) {
-        return machine;
+    if (machine_class) {
+        return machine_class;
     }
     printf("Supported machines are:\n");
     for (el = machines; el; el = el->next) {
         QemuMachineClass *k = el->data;
-        m = k->qemu_machine;
+        QEMUMachine *m = k->qemu_machine;
         if (m->alias) {
             printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
         }
@@ -2836,6 +2842,7 @@ int main(int argc, char **argv, char **envp)
     int optind;
     const char *optarg;
     const char *loadvm = NULL;
+    QemuMachineClass *machine_class;
     QEMUMachine *machine;
     const char *cpu_model;
     const char *vga_model = "none";
@@ -2908,7 +2915,7 @@ int main(int argc, char **argv, char **envp)
     os_setup_early_signal_handling();
 
     module_call_init(MODULE_INIT_MACHINE);
-    machine = find_default_machine();
+    machine_class = find_default_machine();
     cpu_model = NULL;
     ram_size = 0;
     snapshot = 0;
@@ -2974,7 +2981,7 @@ int main(int argc, char **argv, char **envp)
             }
             switch(popt->index) {
             case QEMU_OPTION_M:
-                machine = machine_parse(optarg);
+                machine_class = machine_parse(optarg);
                 break;
             case QEMU_OPTION_no_kvm_irqchip: {
                 olist = qemu_find_opts("machine");
@@ -3530,7 +3537,7 @@ int main(int argc, char **argv, char **envp)
                 }
                 optarg = qemu_opt_get(opts, "type");
                 if (optarg) {
-                    machine = machine_parse(optarg);
+                    machine_class = machine_parse(optarg);
                 }
                 break;
              case QEMU_OPTION_no_kvm:
@@ -3844,11 +3851,15 @@ int main(int argc, char **argv, char **envp)
     }
 #endif
 
-    if (machine == NULL) {
+    if (machine_class == NULL) {
         fprintf(stderr, "No machine found.\n");
         exit(1);
     }
 
+    current_machine = QEMU_MACHINE(object_new(object_class_get_name(
+                          OBJECT_CLASS(machine_class))));
+
+    machine = machine_class->qemu_machine;
     if (machine->hw_version) {
         qemu_set_version(machine->hw_version);
     }
@@ -4277,6 +4288,8 @@ int main(int argc, char **argv, char **envp)
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
                                  .cpu_model = cpu_model };
+
+    current_machine->init_args = args;
     machine->init(&args);
 
     audio_init();
@@ -4285,8 +4298,6 @@ int main(int argc, char **argv, char **envp)
 
     set_numa_modes();
 
-    current_machine = machine;
-
     /* init USB devices */
     if (usb_enabled(false)) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 4/9] hw/machine: add qemu machine opts as properties to QemuMachineState

[Marcel Apfelbaum]

It is cleaner to query current_machine's properties than
accessing QemuOpts from the code.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/core/machine.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 251 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2c6e1a3..436829c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -11,15 +11,265 @@
  */
 
 #include "hw/boards.h"
+#include "qapi/visitor.h"
+
+static char *machine_get_accel(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->accel);
+}
+
+static void machine_set_accel(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->accel = g_strdup(value);
+}
+
+static bool machine_get_kernel_irqchip(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return machine_state->kernel_irqchip;
+}
+
+static void machine_set_kernel_irqchip(Object *obj, bool value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->kernel_irqchip = value;
+}
+
+static void machine_get_kvm_shadow_mem(Object *obj, Visitor *v,
+                                       void *opaque, const char *name,
+                                       Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    int64_t value = machine_state->kvm_shadow_mem;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
+                                       void *opaque, const char *name,
+                                       Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    machine_state->kvm_shadow_mem = value;
+}
+
+static char *machine_get_kernel(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->kernel);
+}
+
+static void machine_set_kernel(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->kernel = g_strdup(value);
+}
+
+static char *machine_get_initrd(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->initrd);
+}
+
+static void machine_set_initrd(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->initrd = g_strdup(value);
+}
+
+static char *machine_get_append(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->append);
+}
+
+static void machine_set_append(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->append = g_strdup(value);
+}
+
+static char *machine_get_dtb(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->dtb);
+}
+
+static void machine_set_dtb(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->dtb = g_strdup(value);
+}
+
+static char *machine_get_dumpdtb(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->dumpdtb);
+}
+
+static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->dumpdtb = g_strdup(value);
+}
+
+static void machine_get_phandle_start(Object *obj, Visitor *v,
+                                       void *opaque, const char *name,
+                                       Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    int64_t value = machine_state->phandle_start;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void machine_set_phandle_start(Object *obj, Visitor *v,
+                                       void *opaque, const char *name,
+                                       Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    machine_state->phandle_start = value;
+}
+
+static char *machine_get_dt_compatible(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->dt_compatible);
+}
+
+static void machine_set_dt_compatible(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->dt_compatible = g_strdup(value);
+}
+
+static bool machine_get_dump_guest_core(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return machine_state->dump_guest_core;
+}
+
+static void machine_set_dump_guest_core(Object *obj, bool value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->dump_guest_core = value;
+}
+
+static bool machine_get_mem_merge(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return machine_state->mem_merge;
+}
+
+static void machine_set_mem_merge(Object *obj, bool value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->mem_merge = value;
+}
+
+static bool machine_get_usb(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return machine_state->usb;
+}
+
+static void machine_set_usb(Object *obj, bool value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->usb = value;
+}
+
+static char *machine_get_firmware(Object *obj, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    return g_strdup(machine_state->firmware);
+}
+
+static void machine_set_firmware(Object *obj, const char *value, Error **errp)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+    machine_state->firmware = g_strdup(value);
+}
 
 static void qemu_machine_initfn(Object *obj)
 {
+    object_property_add_str(obj, "accel",
+                            machine_get_accel, machine_set_accel, NULL);
+    object_property_add_bool(obj, "kernel_irqchip",
+                             machine_get_kernel_irqchip,
+                             machine_set_kernel_irqchip,
+                             NULL);
+    object_property_add(obj, "kvm_shadow_mem", "int",
+                        machine_get_kvm_shadow_mem,
+                        machine_set_kvm_shadow_mem,
+                        NULL, NULL, NULL);
+    object_property_add_str(obj, "kernel",
+                            machine_get_kernel, machine_set_kernel, NULL);
+    object_property_add_str(obj, "initrd",
+                            machine_get_initrd, machine_set_initrd, NULL);
+    object_property_add_str(obj, "append",
+                            machine_get_append, machine_set_append, NULL);
+    object_property_add_str(obj, "dtb",
+                            machine_get_dtb, machine_set_dtb, NULL);
+    object_property_add_str(obj, "dumpdtb",
+                            machine_get_dumpdtb, machine_set_dumpdtb, NULL);
+    object_property_add(obj, "phandle_start", "int",
+                        machine_get_phandle_start,
+                        machine_set_phandle_start,
+                        NULL, NULL, NULL);
+    object_property_add_str(obj, "dt_compatible",
+                            machine_get_dt_compatible,
+                            machine_set_dt_compatible,
+                            NULL);
+    object_property_add_bool(obj, "dump-guest-core",
+                             machine_get_dump_guest_core,
+                             machine_set_dump_guest_core,
+                             NULL);
+    object_property_add_bool(obj, "mem-merge",
+                             machine_get_mem_merge, machine_set_mem_merge, NULL);
+    object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, NULL);
+    object_property_add_str(obj, "firmware",
+                            machine_get_firmware, machine_set_firmware, NULL);
 }
 
 static void qemu_machine_class_init(ObjectClass *oc, void *data)
 {
 }
 
+static void qemu_machine_finalize(Object *obj)
+{
+    QemuMachineState *machine_state = QEMU_MACHINE(obj);
+
+    g_free(machine_state->accel);
+    g_free(machine_state->kernel);
+    g_free(machine_state->initrd);
+    g_free(machine_state->append);
+    g_free(machine_state->dtb);
+    g_free(machine_state->dumpdtb);
+    g_free(machine_state->dt_compatible);
+    g_free(machine_state->firmware);
+}
+
 static const TypeInfo qemu_machine_info = {
     .name = TYPE_QEMU_MACHINE,
     .parent = TYPE_OBJECT,
@@ -28,6 +278,7 @@ static const TypeInfo qemu_machine_info = {
     .class_init = qemu_machine_class_init,
     .instance_size = sizeof(QemuMachineState),
     .instance_init = qemu_machine_initfn,
+    .instance_finalize = qemu_machine_finalize,
 };
 
 static void register_types(void)
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 5/9] qapi: output visitor crashes qemu if it encounters a NULL value

[Marcel Apfelbaum]

A NULL value is not added to visitor's stack, but there
is no check for that when the visitor tries to return
that value, leading to Qemu crash.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 qapi/qmp-output-visitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 74a5684..0562f49 100644
--- a/qapi/qmp-output-visitor.c
+++ b/qapi/qmp-output-visitor.c
@@ -66,6 +66,11 @@ static QObject *qmp_output_pop(QmpOutputVisitor *qov)
 static QObject *qmp_output_first(QmpOutputVisitor *qov)
 {
     QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack);
+
+    if (!e) {
+        return NULL;
+    }
+
     return e->value;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 6/9] vl.c: do not set 'type' property in obj_set_property

[Marcel Apfelbaum]

Filter out also 'type' property when setting
object's properties

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 vl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index c4939ef..dc206e1 100644
--- a/vl.c
+++ b/vl.c
@@ -2766,7 +2766,8 @@ static int object_set_property(const char *name, const char *value, void *opaque
     StringInputVisitor *siv;
     Error *local_err = NULL;
 
-    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0) {
+    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 ||
+        strcmp(name, "type") == 0) {
         return 0;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 7/9] qom: add object_property_is_set

[Marcel Apfelbaum]

Sometimes is not enough to get property's value,
but it is needed to know if the value was actually set.

This is especially useful when querying bool properties
and having different defaults on different scenarios.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/qom/object.h | 11 +++++++++++
 qom/object.c         | 12 ++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 9c7c361..4a4f026 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -319,6 +319,7 @@ typedef struct ObjectProperty
 {
     gchar *name;
     gchar *type;
+    bool is_set;
     ObjectPropertyAccessor *get;
     ObjectPropertyAccessor *set;
     ObjectPropertyRelease *release;
@@ -931,6 +932,16 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
                          Error **errp);
 
 /**
+ * object_property_is_set:
+ * @obj: the object
+ * @name: the name of the property
+ * @errp: returns an error if this function fails
+ *
+ * Returns: true if object's property is set, false otherwise.
+ */
+bool object_property_is_set(Object *obj, const char *name,
+                            Error **errp);
+/**
  * object_property_parse:
  * @obj: the object
  * @string: the string that will be used to parse the property value.
diff --git a/qom/object.c b/qom/object.c
index 660859c..5b9d8af 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -817,9 +817,21 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
         error_set(errp, QERR_PERMISSION_DENIED);
     } else {
         prop->set(obj, v, prop->opaque, name, errp);
+        prop->is_set = true;
     }
 }
 
+bool object_property_is_set(Object *obj, const char *name,
+                            Error **errp)
+{
+    ObjectProperty *prop = object_property_find(obj, name, errp);
+    if (prop == NULL) {
+        return false;
+    }
+
+    return prop->is_set;
+}
+
 void object_property_set_str(Object *obj, const char *value,
                              const char *name, Error **errp)
 {
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 8/9] machine-opts: replace qemu_opt_get by QOM QemuMachine queries

[Marcel Apfelbaum]

It is cleaner to query current_machine's properties than
accessing QemuOpts from the code.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 device_tree.c         |  9 +++++----
 exec.c                | 21 ++++++++++++++++++---
 hw/arm/boot.c         |  3 ++-
 hw/microblaze/boot.c  | 13 +++++++------
 hw/ppc/e500.c         | 21 +++++++++++++--------
 hw/ppc/spapr.c        | 14 +++++++++-----
 hw/ppc/virtex_ml507.c |  3 ++-
 kvm-all.c             | 11 +++++++++--
 target-i386/kvm.c     | 10 ++++++++--
 vl.c                  | 26 ++++++++++++++++++--------
 10 files changed, 91 insertions(+), 40 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index ca83504..11dfdb4 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -23,7 +23,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/sysemu.h"
 #include "hw/loader.h"
-#include "qemu/option.h"
+#include "hw/boards.h"
 #include "qemu/config-file.h"
 
 #include <libfdt.h>
@@ -244,8 +244,8 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
      * which phandle id to start allocting phandles.
      */
     if (!phandle) {
-        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
-                                      "phandle_start", 0);
+        phandle = object_property_get_int(OBJECT(current_machine),
+                                          "phandle_start", &error_abort);
     }
 
     if (!phandle) {
@@ -305,7 +305,8 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
 
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
-    const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
+    const char *dumpdtb = object_property_get_str(OBJECT(current_machine),
+                                                  "dumpdtb", &error_abort);
 
     if (dumpdtb) {
         /* Dump the dtb to a file and quit */
diff --git a/exec.c b/exec.c
index b69fd29..790287c 100644
--- a/exec.c
+++ b/exec.c
@@ -43,6 +43,7 @@
 #else /* !CONFIG_USER_ONLY */
 #include "sysemu/xen-mapcache.h"
 #include "trace.h"
+#include "hw/boards.h"
 #endif
 #include "exec/cpu-all.h"
 
@@ -1182,10 +1183,17 @@ ram_addr_t last_ram_offset(void)
 static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
 {
     int ret;
+    bool dump_guest_core = true;
+
+    if (object_property_is_set(OBJECT(current_machine),
+                               "dump-guest-core", &error_abort)) {
+        dump_guest_core = object_property_get_bool(OBJECT(current_machine),
+                                                   "dump-guest-core",
+                                                   &error_abort);
+    }
 
     /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
-                           "dump-guest-core", true)) {
+    if (!dump_guest_core) {
         ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
         if (ret) {
             perror("qemu_madvise");
@@ -1232,7 +1240,14 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
 
 static int memory_try_enable_merging(void *addr, size_t len)
 {
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
+    bool mem_merge = true;
+    if (object_property_is_set(OBJECT(current_machine),
+                               "mem-merge", &error_abort)) {
+        mem_merge = object_property_get_bool(OBJECT(current_machine),
+                                             "mem-merge", &error_abort);
+    }
+
+    if (!mem_merge) {
         /* disabled by the user */
         return 0;
     }
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index dc62918..384b7e6 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -468,7 +468,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         kernel_load_offset = KERNEL_LOAD_ADDR;
     }
 
-    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+    info->dtb_filename = object_property_get_str(OBJECT(current_machine),
+                                                 "dtb", &error_abort);
 
     if (!info->secondary_cpu_reset_hook) {
         info->secondary_cpu_reset_hook = default_reset_secondary;
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 48d9e7a..eace912 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -24,7 +24,7 @@
  * THE SOFTWARE.
  */
 
-#include "qemu/option.h"
+#include "hw/boards.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu-common.h"
@@ -109,15 +109,16 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
                             const char *dtb_filename,
                             void (*machine_cpu_reset)(MicroBlazeCPU *))
 {
-    QemuOpts *machine_opts;
     const char *kernel_filename;
     const char *kernel_cmdline;
     const char *dtb_arg;
 
-    machine_opts = qemu_get_machine_opts();
-    kernel_filename = qemu_opt_get(machine_opts, "kernel");
-    kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    dtb_arg = qemu_opt_get(machine_opts, "dtb");
+    kernel_filename = object_property_get_str(OBJECT(current_machine),
+                                              "kernel", &error_abort);
+    kernel_cmdline = object_property_get_str(OBJECT(current_machine),
+                                              "append", &error_abort);
+    dtb_arg = object_property_get_str(OBJECT(current_machine),
+                                      "dtb", &error_abort);
     if (dtb_arg) { /* Preference a -dtb argument */
         dtb_filename = dtb_arg;
     } else { /* default to pcbios dtb as passed by machine_init */
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b37ce9d..ca9a0c1 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -159,9 +159,11 @@ static int ppce500_load_device_tree(QEMUMachineInitArgs *args,
             0x0, 0xe1000000,
             0x0, 0x10000,
         };
-    QemuOpts *machine_opts = qemu_get_machine_opts();
-    const char *dtb_file = qemu_opt_get(machine_opts, "dtb");
-    const char *toplevel_compat = qemu_opt_get(machine_opts, "dt_compatible");
+    const char *dtb_file = object_property_get_str(OBJECT(current_machine),
+                                                  "dtb", &error_abort);
+    const char *toplevel_compat = object_property_get_str(OBJECT(current_machine),
+                                                          "dt_compatible",
+                                                          &error_abort);
 
     if (dtb_file) {
         char *filename;
@@ -563,11 +565,14 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     mpic = g_new(qemu_irq, 256);
 
     if (kvm_enabled()) {
-        QemuOpts *machine_opts = qemu_get_machine_opts();
-        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
-                                                "kernel_irqchip", true);
-        bool irqchip_required = qemu_opt_get_bool(machine_opts,
-                                                  "kernel_irqchip", false);
+        bool irqchip_allowed = true;
+        bool irqchip_required = false;
+        if (object_property_is_set(OBJECT(current_machine),
+                                   "kernel_irqchip", &error_abort)) {
+            irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
+                                                       "kernel_irqchip", &error_abort);
+            irqchip_required = irqchip_allowed;
+        }
 
         if (irqchip_allowed) {
             dev = ppce500_init_mpic_kvm(params, irqs);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 93d02c1e..a176f8f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -169,11 +169,15 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
     XICSState *icp = NULL;
 
     if (kvm_enabled()) {
-        QemuOpts *machine_opts = qemu_get_machine_opts();
-        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
-                                                "kernel_irqchip", true);
-        bool irqchip_required = qemu_opt_get_bool(machine_opts,
-                                                  "kernel_irqchip", false);
+        bool irqchip_allowed = true;
+        bool irqchip_required = false;
+        if (object_property_is_set(OBJECT(current_machine),
+                                   "kernel_irqchip", &error_abort)) {
+            irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
+                                                       "kernel_irqchip", &error_abort);
+            irqchip_required = irqchip_allowed;
+        }
+
         if (irqchip_allowed) {
             icp = try_create_xics(TYPE_KVM_XICS, nr_servers, nr_irqs);
         }
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index bdb057e..8651167 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -145,7 +145,8 @@ static int xilinx_load_device_tree(hwaddr addr,
     int r;
     const char *dtb_filename;
 
-    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
+    dtb_filename = object_property_get_str(OBJECT(current_machine),
+                                           "dtb", &error_abort);
     if (dtb_filename) {
         fdt = load_device_tree(dtb_filename, &fdt_size);
         if (!fdt) {
diff --git a/kvm-all.c b/kvm-all.c
index 2ca9143..e483c3c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -22,7 +22,7 @@
 
 #include "qemu-common.h"
 #include "qemu/atomic.h"
-#include "qemu/option.h"
+#include "hw/boards.h"
 #include "qemu/config-file.h"
 #include "sysemu/sysemu.h"
 #include "hw/hw.h"
@@ -1292,8 +1292,15 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
 static int kvm_irqchip_create(KVMState *s)
 {
     int ret;
+    bool irqchip_allowed = true;
 
-    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
+    if (object_property_is_set(OBJECT(current_machine),
+                               "kernel_irqchip", &error_abort)) {
+        irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
+                                                   "kernel_irqchip", &error_abort);
+    }
+
+    if (!irqchip_allowed ||
         !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
         return 0;
     }
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index e555040..ca7e6bc 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -33,6 +33,7 @@
 #include "exec/ioport.h"
 #include <asm/hyperv.h>
 #include "hw/pci/pci.h"
+#include "hw/boards.h"
 
 //#define DEBUG_KVM
 
@@ -853,8 +854,13 @@ int kvm_arch_init(KVMState *s)
     }
     qemu_register_reset(kvm_unpoison_all, NULL);
 
-    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
-                                   "kvm_shadow_mem", -1);
+    shadow_mem = -1;
+    if (object_property_is_set(OBJECT(current_machine),
+                               "kvm_shadow_mem", &error_abort)) {
+        shadow_mem = object_property_get_int(OBJECT(current_machine),
+                                             "kvm_shadow_mem", &error_abort);
+    }
+
     if (shadow_mem != -1) {
         shadow_mem /= 4096;
         ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
diff --git a/vl.c b/vl.c
index dc206e1..007342c 100644
--- a/vl.c
+++ b/vl.c
@@ -2624,7 +2624,8 @@ static int configure_accelerator(void)
     bool accel_initialised = false;
     bool init_failed = false;
 
-    p = qemu_opt_get(qemu_get_machine_opts(), "accel");
+    p = object_property_get_str(OBJECT(current_machine),
+                                "accel", &error_abort);
     if (p == NULL) {
         /* Use the default "accelerator", tcg */
         p = "tcg";
@@ -4077,6 +4078,12 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    machine_opts = qemu_get_machine_opts();
+    if (qemu_opt_foreach(machine_opts, object_set_property, current_machine, 1) < 0) {
+        object_unref(OBJECT(current_machine));
+        exit(1);
+    }
+
     configure_accelerator();
 
     if (qtest_chrdev) {
@@ -4089,12 +4096,14 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    machine_opts = qemu_get_machine_opts();
-    kernel_filename = qemu_opt_get(machine_opts, "kernel");
-    initrd_filename = qemu_opt_get(machine_opts, "initrd");
-    kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    bios_name = qemu_opt_get(machine_opts, "firmware");
-
+    kernel_filename = object_property_get_str(OBJECT(current_machine),
+                                              "kernel", &error_abort);
+    initrd_filename = object_property_get_str(OBJECT(current_machine),
+                                              "initrd", &error_abort);
+    kernel_cmdline = object_property_get_str(OBJECT(current_machine),
+                                             "append", &error_abort);
+    bios_name = object_property_get_str(OBJECT(current_machine),
+                                        "firmware", &error_abort);
     boot_order = machine->default_boot_order;
     opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
     if (opts) {
@@ -4135,7 +4144,8 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
+    if (!linux_boot && object_property_get_str(OBJECT(current_machine),
+                                               "dtb", &error_abort)) {
         fprintf(stderr, "-dtb only allowed with -kernel option\n");
         exit(1);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC V2 9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication

[Marcel Apfelbaum]

The QOM machine has both field per QemuOpt and an instance of
QEMUMachineInitArgs. Removed dupplications.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/core/machine.c   | 18 +++++++++---------
 include/hw/boards.h |  9 +++------
 vl.c                |  6 +++---
 3 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 436829c..73f61bd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -67,37 +67,37 @@ static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
 static char *machine_get_kernel(Object *obj, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    return g_strdup(machine_state->kernel);
+    return g_strdup(machine_state->init_args.kernel_filename);
 }
 
 static void machine_set_kernel(Object *obj, const char *value, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    machine_state->kernel = g_strdup(value);
+    machine_state->init_args.kernel_filename = g_strdup(value);
 }
 
 static char *machine_get_initrd(Object *obj, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    return g_strdup(machine_state->initrd);
+    return g_strdup(machine_state->init_args.initrd_filename);
 }
 
 static void machine_set_initrd(Object *obj, const char *value, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    machine_state->initrd = g_strdup(value);
+    machine_state->init_args.initrd_filename = g_strdup(value);
 }
 
 static char *machine_get_append(Object *obj, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    return g_strdup(machine_state->append);
+    return g_strdup(machine_state->init_args.kernel_cmdline);
 }
 
 static void machine_set_append(Object *obj, const char *value, Error **errp)
 {
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
-    machine_state->append = g_strdup(value);
+    machine_state->init_args.kernel_cmdline = g_strdup(value);
 }
 
 static char *machine_get_dtb(Object *obj, Error **errp)
@@ -261,9 +261,9 @@ static void qemu_machine_finalize(Object *obj)
     QemuMachineState *machine_state = QEMU_MACHINE(obj);
 
     g_free(machine_state->accel);
-    g_free(machine_state->kernel);
-    g_free(machine_state->initrd);
-    g_free(machine_state->append);
+    g_free(machine_state->init_args.kernel_filename);
+    g_free(machine_state->init_args.initrd_filename);
+    g_free(machine_state->init_args.kernel_cmdline);
     g_free(machine_state->dtb);
     g_free(machine_state->dumpdtb);
     g_free(machine_state->dt_compatible);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 053c113..4b1979e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -13,9 +13,9 @@ typedef struct QEMUMachineInitArgs {
     const QEMUMachine *machine;
     ram_addr_t ram_size;
     const char *boot_order;
-    const char *kernel_filename;
-    const char *kernel_cmdline;
-    const char *initrd_filename;
+    char *kernel_filename;
+    char *kernel_cmdline;
+    char *initrd_filename;
     const char *cpu_model;
 } QEMUMachineInitArgs;
 
@@ -91,9 +91,6 @@ struct QemuMachineState {
     char *accel;
     bool kernel_irqchip;
     int kvm_shadow_mem;
-    char *kernel;
-    char *initrd;
-    char *append;
     char *dtb;
     char *dumpdtb;
     int phandle_start;
diff --git a/vl.c b/vl.c
index 007342c..3d7357e 100644
--- a/vl.c
+++ b/vl.c
@@ -2834,8 +2834,8 @@ int main(int argc, char **argv, char **envp)
     int i;
     int snapshot, linux_boot;
     const char *icount_option = NULL;
-    const char *initrd_filename;
-    const char *kernel_filename, *kernel_cmdline;
+    char *initrd_filename;
+    char *kernel_filename, *kernel_cmdline;
     const char *boot_order;
     DisplayState *ds;
     int cyls, heads, secs, translation;
@@ -4129,7 +4129,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     if (!kernel_cmdline) {
-        kernel_cmdline = "";
+        kernel_cmdline = (char *)"";
     }
 
     linux_boot = (kernel_filename != NULL);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC V2 8/9] machine-opts: replace qemu_opt_get by QOM QemuMachine queries

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> @@ -1182,10 +1183,17 @@ ram_addr_t last_ram_offset(void)
>  static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
>  {
>      int ret;
> +    bool dump_guest_core = true;
> +
> +    if (object_property_is_set(OBJECT(current_machine),
> +                               "dump-guest-core", &error_abort)) {
> +        dump_guest_core = object_property_get_bool(OBJECT(current_machine),
> +                                                   "dump-guest-core",
> +                                                   &error_abort);
> +    }

You do not need object_property_is_set.  You can just initialize the 
field to true in the instance_init function.  Same for mem_merge.

> @@ -563,11 +565,14 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
>      mpic = g_new(qemu_irq, 256);
>
>      if (kvm_enabled()) {
> -        QemuOpts *machine_opts = qemu_get_machine_opts();
> -        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> -                                                "kernel_irqchip", true);
> -        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> -                                                  "kernel_irqchip", false);
> +        bool irqchip_allowed = true;
> +        bool irqchip_required = false;
> +        if (object_property_is_set(OBJECT(current_machine),
> +                                   "kernel_irqchip", &error_abort)) {
> +            irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
> +                                                       "kernel_irqchip", &error_abort);
> +            irqchip_required = irqchip_allowed;
> +        }

Alternatively, you could have three properties: kernel_irqchip, 
write-only (no getter); kernel-irqchip-allowed; kernel-irqchip-required. 
  Setting irqchip writes to both kernel-irqchip-allowed and 
kernel-irqchip-required.  kernel-irqchip-allowed and 
kernel-irqchip-required are initialized to true and false respectively 
in the instance_init.


> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index bdb057e..8651167 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -145,7 +145,8 @@ static int xilinx_load_device_tree(hwaddr addr,
>      int r;
>      const char *dtb_filename;
>
> -    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> +    dtb_filename = object_property_get_str(OBJECT(current_machine),
> +                                           "dtb", &error_abort);
>      if (dtb_filename) {
>          fdt = load_device_tree(dtb_filename, &fdt_size);
>          if (!fdt) {
> diff --git a/kvm-all.c b/kvm-all.c
> index 2ca9143..e483c3c 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -22,7 +22,7 @@
>
>  #include "qemu-common.h"
>  #include "qemu/atomic.h"
> -#include "qemu/option.h"
> +#include "hw/boards.h"
>  #include "qemu/config-file.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/hw.h"
> @@ -1292,8 +1292,15 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
>  static int kvm_irqchip_create(KVMState *s)
>  {
>      int ret;
> +    bool irqchip_allowed = true;
>
> -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
> +    if (object_property_is_set(OBJECT(current_machine),
> +                               "kernel_irqchip", &error_abort)) {
> +        irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
> +                                                   "kernel_irqchip", &error_abort);
> +    }
> +
> +    if (!irqchip_allowed ||
>          !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
>          return 0;
>      }
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index e555040..ca7e6bc 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -33,6 +33,7 @@
>  #include "exec/ioport.h"
>  #include <asm/hyperv.h>
>  #include "hw/pci/pci.h"
> +#include "hw/boards.h"
>
>  //#define DEBUG_KVM
>
> @@ -853,8 +854,13 @@ int kvm_arch_init(KVMState *s)
>      }
>      qemu_register_reset(kvm_unpoison_all, NULL);
>
> -    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
> -                                   "kvm_shadow_mem", -1);
> +    shadow_mem = -1;
> +    if (object_property_is_set(OBJECT(current_machine),
> +                               "kvm_shadow_mem", &error_abort)) {
> +        shadow_mem = object_property_get_int(OBJECT(current_machine),
> +                                             "kvm_shadow_mem", &error_abort);
> +    }

This can also be simply initialized in the instance_init function.

Paolo

> +
>      if (shadow_mem != -1) {
>          shadow_mem /= 4096;
>          ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
> diff --git a/vl.c b/vl.c
> index dc206e1..007342c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2624,7 +2624,8 @@ static int configure_accelerator(void)
>      bool accel_initialised = false;
>      bool init_failed = false;
>
> -    p = qemu_opt_get(qemu_get_machine_opts(), "accel");
> +    p = object_property_get_str(OBJECT(current_machine),
> +                                "accel", &error_abort);
>      if (p == NULL) {
>          /* Use the default "accelerator", tcg */
>          p = "tcg";
> @@ -4077,6 +4078,12 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>
> +    machine_opts = qemu_get_machine_opts();
> +    if (qemu_opt_foreach(machine_opts, object_set_property, current_machine, 1) < 0) {
> +        object_unref(OBJECT(current_machine));
> +        exit(1);
> +    }
> +
>      configure_accelerator();
>
>      if (qtest_chrdev) {
> @@ -4089,12 +4096,14 @@ int main(int argc, char **argv, char **envp)
>          }
>      }
>
> -    machine_opts = qemu_get_machine_opts();
> -    kernel_filename = qemu_opt_get(machine_opts, "kernel");
> -    initrd_filename = qemu_opt_get(machine_opts, "initrd");
> -    kernel_cmdline = qemu_opt_get(machine_opts, "append");
> -    bios_name = qemu_opt_get(machine_opts, "firmware");
> -
> +    kernel_filename = object_property_get_str(OBJECT(current_machine),
> +                                              "kernel", &error_abort);
> +    initrd_filename = object_property_get_str(OBJECT(current_machine),
> +                                              "initrd", &error_abort);
> +    kernel_cmdline = object_property_get_str(OBJECT(current_machine),
> +                                             "append", &error_abort);
> +    bios_name = object_property_get_str(OBJECT(current_machine),
> +                                        "firmware", &error_abort);
>      boot_order = machine->default_boot_order;
>      opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
>      if (opts) {
> @@ -4135,7 +4144,8 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>
> -    if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> +    if (!linux_boot && object_property_get_str(OBJECT(current_machine),
> +                                               "dtb", &error_abort)) {
>          fprintf(stderr, "-dtb only allowed with -kernel option\n");
>          exit(1);
>      }
>

Re: [Qemu-devel] [PATCH RFC V2 6/9] vl.c: do not set 'type' property in obj_set_property

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> Filter out also 'type' property when setting
> object's properties
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  vl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/vl.c b/vl.c
> index c4939ef..dc206e1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2766,7 +2766,8 @@ static int object_set_property(const char *name, const char *value, void *opaque
>      StringInputVisitor *siv;
>      Error *local_err = NULL;
>
> -    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0) {
> +    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 ||
> +        strcmp(name, "type") == 0) {
>          return 0;
>      }
>
>

Unfortunately, it is quite possible to have a -object invocation where 
the object has a "type" property.

I think you could change the -object implementation to use OptsVisitor, 
similar to hmp_object_add in hmp.c.  Then the -object code can pre-parse 
"qom-type" and "id", while the -machine code can pre-parse "type".

Paolo

Re: [Qemu-devel] [PATCH RFC V2 7/9] qom: add object_property_is_set

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> Sometimes is not enough to get property's value,
> but it is needed to know if the value was actually set.
>
> This is especially useful when querying bool properties
> and having different defaults on different scenarios.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

I suggested some replacements for this in the review to patch 8/9.

The problem is that tristate properties implemented with 
object_property_is_set cannot be "round-trip"-ped out and back in of QEMU.

Paolo

> ---
>  include/qom/object.h | 11 +++++++++++
>  qom/object.c         | 12 ++++++++++++
>  2 files changed, 23 insertions(+)
>
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 9c7c361..4a4f026 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -319,6 +319,7 @@ typedef struct ObjectProperty
>  {
>      gchar *name;
>      gchar *type;
> +    bool is_set;
>      ObjectPropertyAccessor *get;
>      ObjectPropertyAccessor *set;
>      ObjectPropertyRelease *release;
> @@ -931,6 +932,16 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
>                           Error **errp);
>
>  /**
> + * object_property_is_set:
> + * @obj: the object
> + * @name: the name of the property
> + * @errp: returns an error if this function fails
> + *
> + * Returns: true if object's property is set, false otherwise.
> + */
> +bool object_property_is_set(Object *obj, const char *name,
> +                            Error **errp);
> +/**
>   * object_property_parse:
>   * @obj: the object
>   * @string: the string that will be used to parse the property value.
> diff --git a/qom/object.c b/qom/object.c
> index 660859c..5b9d8af 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -817,9 +817,21 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
>          error_set(errp, QERR_PERMISSION_DENIED);
>      } else {
>          prop->set(obj, v, prop->opaque, name, errp);
> +        prop->is_set = true;
>      }
>  }
>
> +bool object_property_is_set(Object *obj, const char *name,
> +                            Error **errp)
> +{
> +    ObjectProperty *prop = object_property_find(obj, name, errp);
> +    if (prop == NULL) {
> +        return false;
> +    }
> +
> +    return prop->is_set;
> +}
> +
>  void object_property_set_str(Object *obj, const char *value,
>                               const char *name, Error **errp)
>  {
>

Re: [Qemu-devel] [PATCH RFC V2 9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> The QOM machine has both field per QemuOpt and an instance of
> QEMUMachineInitArgs. Removed dupplications.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

I think this patch is a bit backwards. :)

You should _start_ with properties that access QEMUMachineInitArgs 
fields, and nothing like the "char *kernel" fields you currently have in 
QemuMachineState.  Then, as part of a patch that does a global 
s/QEMUMachineInitArgs/QemuMachineState/, you move the fields from 
QEMUMachineInitArgs to QemuMachineState, and remove the "init_args." 
indirection in the property accessors.

BTW, it is much simpler if you do not change the field names in the 
process: keep kernel_filename in QemuMachineState, do not change it to 
kernel.

Paolo

> ---
>  hw/core/machine.c   | 18 +++++++++---------
>  include/hw/boards.h |  9 +++------
>  vl.c                |  6 +++---
>  3 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 436829c..73f61bd 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -67,37 +67,37 @@ static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
>  static char *machine_get_kernel(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->kernel);
> +    return g_strdup(machine_state->init_args.kernel_filename);
>  }
>
>  static void machine_set_kernel(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->kernel = g_strdup(value);
> +    machine_state->init_args.kernel_filename = g_strdup(value);
>  }
>
>  static char *machine_get_initrd(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->initrd);
> +    return g_strdup(machine_state->init_args.initrd_filename);
>  }
>
>  static void machine_set_initrd(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->initrd = g_strdup(value);
> +    machine_state->init_args.initrd_filename = g_strdup(value);
>  }
>
>  static char *machine_get_append(Object *obj, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    return g_strdup(machine_state->append);
> +    return g_strdup(machine_state->init_args.kernel_cmdline);
>  }
>
>  static void machine_set_append(Object *obj, const char *value, Error **errp)
>  {
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> -    machine_state->append = g_strdup(value);
> +    machine_state->init_args.kernel_cmdline = g_strdup(value);
>  }
>
>  static char *machine_get_dtb(Object *obj, Error **errp)
> @@ -261,9 +261,9 @@ static void qemu_machine_finalize(Object *obj)
>      QemuMachineState *machine_state = QEMU_MACHINE(obj);
>
>      g_free(machine_state->accel);
> -    g_free(machine_state->kernel);
> -    g_free(machine_state->initrd);
> -    g_free(machine_state->append);
> +    g_free(machine_state->init_args.kernel_filename);
> +    g_free(machine_state->init_args.initrd_filename);
> +    g_free(machine_state->init_args.kernel_cmdline);
>      g_free(machine_state->dtb);
>      g_free(machine_state->dumpdtb);
>      g_free(machine_state->dt_compatible);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 053c113..4b1979e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -13,9 +13,9 @@ typedef struct QEMUMachineInitArgs {
>      const QEMUMachine *machine;
>      ram_addr_t ram_size;
>      const char *boot_order;
> -    const char *kernel_filename;
> -    const char *kernel_cmdline;
> -    const char *initrd_filename;
> +    char *kernel_filename;
> +    char *kernel_cmdline;
> +    char *initrd_filename;
>      const char *cpu_model;
>  } QEMUMachineInitArgs;
>
> @@ -91,9 +91,6 @@ struct QemuMachineState {
>      char *accel;
>      bool kernel_irqchip;
>      int kvm_shadow_mem;
> -    char *kernel;
> -    char *initrd;
> -    char *append;
>      char *dtb;
>      char *dumpdtb;
>      int phandle_start;
> diff --git a/vl.c b/vl.c
> index 007342c..3d7357e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2834,8 +2834,8 @@ int main(int argc, char **argv, char **envp)
>      int i;
>      int snapshot, linux_boot;
>      const char *icount_option = NULL;
> -    const char *initrd_filename;
> -    const char *kernel_filename, *kernel_cmdline;
> +    char *initrd_filename;
> +    char *kernel_filename, *kernel_cmdline;
>      const char *boot_order;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
> @@ -4129,7 +4129,7 @@ int main(int argc, char **argv, char **envp)
>      }
>
>      if (!kernel_cmdline) {
> -        kernel_cmdline = "";
> +        kernel_cmdline = (char *)"";
>      }
>
>      linux_boot = (kernel_filename != NULL);
>

Re: [Qemu-devel] [PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> In order to allow attaching machine options to a machine instance,
> current_machine is converted into QemuMachineState.
> As a first step of deprecating QEMUMachine, some of the functions
> were modified to return QemuMachineCLass.

This is a relatively large change since current_machine is currently 
"class-like", while after your patch it is "object-like".

Not a big problem, since I believe at the end of the conversion there 
shouldn't be a current_machine at all.  Board initialization code will 
get it as an argument where they currently have the QEMUMachineInitArgs. 
  The few that use current_machine before this patch can and should 
access the QOM tree to retrieve the current machine.

... which brings us to what is missing here: you are not adding 
current_machine as /machine in the QOM tree. :)

Paolo

> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  device-hotplug.c    |  4 +++-
>  include/hw/boards.h |  6 ++---
>  qmp.c               |  7 ++++--
>  vl.c                | 69 +++++++++++++++++++++++++++++++----------------------
>  4 files changed, 51 insertions(+), 35 deletions(-)
>
> diff --git a/device-hotplug.c b/device-hotplug.c
> index 103d34a..fb5eb01 100644
> --- a/device-hotplug.c
> +++ b/device-hotplug.c
> @@ -33,12 +33,14 @@ DriveInfo *add_init_drive(const char *optstr)
>  {
>      DriveInfo *dinfo;
>      QemuOpts *opts;
> +    QemuMachineClass *machine_class;
>
>      opts = drive_def(optstr);
>      if (!opts)
>          return NULL;
>
> -    dinfo = drive_init(opts, current_machine->block_default_type);
> +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> +    dinfo = drive_init(opts, machine_class->qemu_machine->block_default_type);
>      if (!dinfo) {
>          qemu_opts_del(opts);
>          return NULL;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 65e1e03..053c113 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -51,9 +51,6 @@ struct QEMUMachine {
>
>  #define TYPE_QEMU_MACHINE_PREFIX "machine-"
>  int qemu_register_machine(QEMUMachine *m);
> -QEMUMachine *find_default_machine(void);
> -
> -extern QEMUMachine *current_machine;
>
>  #define TYPE_QEMU_MACHINE "machine"
>  #define QEMU_MACHINE(obj) \
> @@ -66,6 +63,9 @@ extern QEMUMachine *current_machine;
>  typedef struct QemuMachineState QemuMachineState;
>  typedef struct QemuMachineClass QemuMachineClass;
>
> +QemuMachineClass *find_default_machine(void);
> +extern QemuMachineState *current_machine;
> +
>  /**
>   * @QemuMachineClass
>   *
> diff --git a/qmp.c b/qmp.c
> index d0d98e7..df5d8d9 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -114,8 +114,11 @@ void qmp_cpu(int64_t index, Error **errp)
>
>  void qmp_cpu_add(int64_t id, Error **errp)
>  {
> -    if (current_machine->hot_add_cpu) {
> -        current_machine->hot_add_cpu(id, errp);
> +    QemuMachineClass *machine_class;
> +
> +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> +    if (machine_class->qemu_machine->hot_add_cpu) {
> +        machine_class->qemu_machine->hot_add_cpu(id, errp);
>      } else {
>          error_setg(errp, "Not supported");
>      }
> diff --git a/vl.c b/vl.c
> index 50c880f..c4939ef 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1529,7 +1529,7 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
>  /***********************************************************/
>  /* machine registration */
>
> -QEMUMachine *current_machine = NULL;
> +QemuMachineState *current_machine;
>
>  static void qemu_machine_class_init(ObjectClass *klass, void *data)
>  {
> @@ -1552,44 +1552,45 @@ int qemu_register_machine(QEMUMachine *m)
>      return 0;
>  }
>
> -static QEMUMachine *find_machine(const char *name)
> +static QemuMachineClass *find_machine(const char *name)
>  {
>      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> -    QEMUMachine *m = NULL;
> +    QemuMachineClass *k = NULL;
>
>      for (el = machines; el; el = el->next) {
> -        QemuMachineClass *k = el->data;
> +        QemuMachineClass *temp = el->data;
>
> -        if (!strcmp(k->qemu_machine->name, name)) {
> -            m = k->qemu_machine;
> +        if (!strcmp(temp->qemu_machine->name, name)) {
> +            k = temp;
>              break;
>          }
> -        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> -            m = k->qemu_machine;
> +        if (temp->qemu_machine->alias &&
> +            !strcmp(temp->qemu_machine->alias, name)) {
> +            k = temp;
>              break;
>          }
>      }
>
>      g_slist_free(machines);
> -    return m;
> +    return k;
>  }
>
> -QEMUMachine *find_default_machine(void)
> +QemuMachineClass *find_default_machine(void)
>  {
>      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> -    QEMUMachine *m = NULL;
> +    QemuMachineClass *k = NULL;
>
>      for (el = machines; el; el = el->next) {
> -        QemuMachineClass *k = el->data;
> +        QemuMachineClass *temp = el->data;
>
> -        if (k->qemu_machine->is_default) {
> -            m = k->qemu_machine;
> +        if (temp->qemu_machine->is_default) {
> +            k = temp;
>              break;
>          }
>      }
>
>      g_slist_free(machines);
> -    return m;
> +    return k;
>  }
>
>  MachineInfoList *qmp_query_machines(Error **errp)
> @@ -1818,8 +1819,13 @@ void qemu_devices_reset(void)
>
>  void qemu_system_reset(bool report)
>  {
> -    if (current_machine && current_machine->reset) {
> -        current_machine->reset();
> +    QemuMachineClass *machine_class;
> +
> +    machine_class = current_machine ? QEMU_MACHINE_GET_CLASS(current_machine)
> +                    : NULL;
> +
> +    if (machine_class && machine_class->qemu_machine->reset) {
> +        machine_class->qemu_machine->reset();
>      } else {
>          qemu_devices_reset();
>      }
> @@ -2565,21 +2571,21 @@ static int debugcon_parse(const char *devname)
>      return 0;
>  }
>
> -static QEMUMachine *machine_parse(const char *name)
> +static QemuMachineClass *machine_parse(const char *name)
>  {
> -    QEMUMachine *m, *machine = NULL;
> +    QemuMachineClass *machine_class = NULL;
>      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>
>      if (name) {
> -        machine = find_machine(name);
> +        machine_class = find_machine(name);
>      }
> -    if (machine) {
> -        return machine;
> +    if (machine_class) {
> +        return machine_class;
>      }
>      printf("Supported machines are:\n");
>      for (el = machines; el; el = el->next) {
>          QemuMachineClass *k = el->data;
> -        m = k->qemu_machine;
> +        QEMUMachine *m = k->qemu_machine;
>          if (m->alias) {
>              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>          }
> @@ -2836,6 +2842,7 @@ int main(int argc, char **argv, char **envp)
>      int optind;
>      const char *optarg;
>      const char *loadvm = NULL;
> +    QemuMachineClass *machine_class;
>      QEMUMachine *machine;
>      const char *cpu_model;
>      const char *vga_model = "none";
> @@ -2908,7 +2915,7 @@ int main(int argc, char **argv, char **envp)
>      os_setup_early_signal_handling();
>
>      module_call_init(MODULE_INIT_MACHINE);
> -    machine = find_default_machine();
> +    machine_class = find_default_machine();
>      cpu_model = NULL;
>      ram_size = 0;
>      snapshot = 0;
> @@ -2974,7 +2981,7 @@ int main(int argc, char **argv, char **envp)
>              }
>              switch(popt->index) {
>              case QEMU_OPTION_M:
> -                machine = machine_parse(optarg);
> +                machine_class = machine_parse(optarg);
>                  break;
>              case QEMU_OPTION_no_kvm_irqchip: {
>                  olist = qemu_find_opts("machine");
> @@ -3530,7 +3537,7 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  optarg = qemu_opt_get(opts, "type");
>                  if (optarg) {
> -                    machine = machine_parse(optarg);
> +                    machine_class = machine_parse(optarg);
>                  }
>                  break;
>               case QEMU_OPTION_no_kvm:
> @@ -3844,11 +3851,15 @@ int main(int argc, char **argv, char **envp)
>      }
>  #endif
>
> -    if (machine == NULL) {
> +    if (machine_class == NULL) {
>          fprintf(stderr, "No machine found.\n");
>          exit(1);
>      }
>
> +    current_machine = QEMU_MACHINE(object_new(object_class_get_name(
> +                          OBJECT_CLASS(machine_class))));
> +
> +    machine = machine_class->qemu_machine;
>      if (machine->hw_version) {
>          qemu_set_version(machine->hw_version);
>      }
> @@ -4277,6 +4288,8 @@ int main(int argc, char **argv, char **envp)
>                                   .kernel_cmdline = kernel_cmdline,
>                                   .initrd_filename = initrd_filename,
>                                   .cpu_model = cpu_model };
> +
> +    current_machine->init_args = args;
>      machine->init(&args);
>
>      audio_init();
> @@ -4285,8 +4298,6 @@ int main(int argc, char **argv, char **envp)
>
>      set_numa_modes();
>
> -    current_machine = machine;
> -
>      /* init USB devices */
>      if (usb_enabled(false)) {
>          if (foreach_device_config(DEV_USB, usb_parse) < 0)
>

Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Paolo Bonzini]

Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> Most of the "Cc" list is due to patch 8: (Should I send each patch to a different list?)
>     machine-opts: replace qemu_opt_get by QOM QemuMachine queries.
>
> Status:
>     - machine_opts are mapped into QemuMachineState's properties,
>       which can be queried as regular QOM properties.
>     - Subclassing QemuMachineClass allows to add a command line
>       option specific to a machine type, error mechanism should
>       work if this option is used on another machine. (Not tested, on the todo list.)
>     - Next big step would be to completely remove the qemu machines initialization
>       and replace it by regular QOM type registration.
Having seen all the series, I think we can plan the actual code as follows.

Patches 1-3 are fine (except for the missing object_property_add_child 
in patch 3).  Patch 4 is also fine, but it should refer to the embedded 
QEMUMachineInitArgs.

The .machine field from QEMUMachineArgs can be removed very early.  It 
can be replaced with the class of current_machine.

Another early refactoring should be to pass &current_machine->init_args 
to machine->init, not the "args".

Now here's a possible plan to get rid of QEMUMachineInitArgs:

1) Add three wrappers to QemuMachine for reading kernel_filename, 
kernel_cmdline, initrd_filename.  Unlike object_property_get_str, they 
can skip the strdup of the value.  This way you don't have to add the 
matching free to all uses of the fields.

2) Similarly, add get/set functions (not properties, since these are not 
accessible via -machine) for ram_size, boot_order, cpu_model.

3) Now you can have something like patch 8 in this series.

4) Thanks to the previous change, there should be no usage of 
QEMUMachineInitArgs anymore.  Change the machine init function to take a 
"QemuMachineState *current_machine".

5) Remove the current_machine global.  There will be few users of 
current_machine, and they can look at /machine instead, as mentioned in 
the review of patch 3.

Does it look feasible?  The bulk changes should all be fairly mechanical.

Paolo

Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:50 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > Most of the "Cc" list is due to patch 8: (Should I send each patch to a different list?)
> >     machine-opts: replace qemu_opt_get by QOM QemuMachine queries.
> >
> > Status:
> >     - machine_opts are mapped into QemuMachineState's properties,
> >       which can be queried as regular QOM properties.
> >     - Subclassing QemuMachineClass allows to add a command line
> >       option specific to a machine type, error mechanism should
> >       work if this option is used on another machine. (Not tested, on the todo list.)
> >     - Next big step would be to completely remove the qemu machines initialization
> >       and replace it by regular QOM type registration.
> Having seen all the series, I think we can plan the actual code as follows.
Paolo, I really appreciate you are taking the time to go over this!

> 
> Patches 1-3 are fine (except for the missing object_property_add_child 
> in patch 3).  Patch 4 is also fine, but it should refer to the embedded 
> QEMUMachineInitArgs.
I will, thanks.
> 
> The .machine field from QEMUMachineArgs can be removed very early.  It 
> can be replaced with the class of current_machine.
Sure.
> 
> Another early refactoring should be to pass &current_machine->init_args 
> to machine->init, not the "args".
Problem is that this is a "private field", should I add a getter for it?

> 
> Now here's a possible plan to get rid of QEMUMachineInitArgs:
> 
> 1) Add three wrappers to QemuMachine for reading kernel_filename, 
> kernel_cmdline, initrd_filename.  Unlike object_property_get_str, they 
> can skip the strdup of the value.  This way you don't have to add the 
> matching free to all uses of the fields.
I have nothing against it, but this will break QEMU's unified way to
handle object properties, right?
Meaning I will have a field  of the object state(kernel_filename)
and I will add a method like machine_get_field(QemuMachineState) going
"around" QOM,,, 

> 
> 2) Similarly, add get/set functions (not properties, since these are not 
> accessible via -machine) for ram_size, boot_order, cpu_model.
I am sorry, here you lost me, what do you mean "accessible via -machine"?
Maybe that cannot be queried by QOM tree?
Those getter/setters are not the same wrappers as above, going around "QOM"?


> 
> 3) Now you can have something like patch 8 in this series.
I also need patch 5 that deals with getting a string property with NULL value,
> 
> 4) Thanks to the previous change, there should be no usage of 
> QEMUMachineInitArgs anymore.  Change the machine init function to take a 
> "QemuMachineState *current_machine".
This will touch a lot of files... but needs to be done.
> 
> 5) Remove the current_machine global.  There will be few users of 
> current_machine, and they can look at /machine instead, as mentioned in 
> the review of patch 3.
Agreed
> 
> Does it look feasible?  The bulk changes should all be fairly mechanical.
I see no reason why not, the main problem I see is the use of those wrappers
or setters/getters, I suspect that the usage will be:
1. global QOM query to get the machine
2. Use this wrappers(getters/setters) to do query/alter the machine.
Doesn't QOM have another way to do this? Or I am missing something, of course.

Thanks again for the review!
Marcel

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:49 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > In order to allow attaching machine options to a machine instance,
> > current_machine is converted into QemuMachineState.
> > As a first step of deprecating QEMUMachine, some of the functions
> > were modified to return QemuMachineCLass.
> 
> This is a relatively large change since current_machine is currently 
> "class-like", while after your patch it is "object-like".
> 
> Not a big problem, since I believe at the end of the conversion there 
> shouldn't be a current_machine at all.  Board initialization code will 
> get it as an argument where they currently have the QEMUMachineInitArgs. 
>   The few that use current_machine before this patch can and should 
> access the QOM tree to retrieve the current machine.
This is the main reason I changed from "class like" code to object,
because in the end the users will use the object returned from the query.

> 
> ... which brings us to what is missing here: you are not adding 
> current_machine as /machine in the QOM tree. :)
Is missing in purpose, I am not sure what are the consequences of changing the /machine
from container to an actual object. I am (almost) sure that this container has child
containers and I am looking for the rules: can an object act like a container?

So, I do intend of course to add it, once I have responded to the above issue.
Thanks,
Marcel
> 
> Paolo
> 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  device-hotplug.c    |  4 +++-
> >  include/hw/boards.h |  6 ++---
> >  qmp.c               |  7 ++++--
> >  vl.c                | 69 +++++++++++++++++++++++++++++++----------------------
> >  4 files changed, 51 insertions(+), 35 deletions(-)
> >
> > diff --git a/device-hotplug.c b/device-hotplug.c
> > index 103d34a..fb5eb01 100644
> > --- a/device-hotplug.c
> > +++ b/device-hotplug.c
> > @@ -33,12 +33,14 @@ DriveInfo *add_init_drive(const char *optstr)
> >  {
> >      DriveInfo *dinfo;
> >      QemuOpts *opts;
> > +    QemuMachineClass *machine_class;
> >
> >      opts = drive_def(optstr);
> >      if (!opts)
> >          return NULL;
> >
> > -    dinfo = drive_init(opts, current_machine->block_default_type);
> > +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> > +    dinfo = drive_init(opts, machine_class->qemu_machine->block_default_type);
> >      if (!dinfo) {
> >          qemu_opts_del(opts);
> >          return NULL;
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 65e1e03..053c113 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -51,9 +51,6 @@ struct QEMUMachine {
> >
> >  #define TYPE_QEMU_MACHINE_PREFIX "machine-"
> >  int qemu_register_machine(QEMUMachine *m);
> > -QEMUMachine *find_default_machine(void);
> > -
> > -extern QEMUMachine *current_machine;
> >
> >  #define TYPE_QEMU_MACHINE "machine"
> >  #define QEMU_MACHINE(obj) \
> > @@ -66,6 +63,9 @@ extern QEMUMachine *current_machine;
> >  typedef struct QemuMachineState QemuMachineState;
> >  typedef struct QemuMachineClass QemuMachineClass;
> >
> > +QemuMachineClass *find_default_machine(void);
> > +extern QemuMachineState *current_machine;
> > +
> >  /**
> >   * @QemuMachineClass
> >   *
> > diff --git a/qmp.c b/qmp.c
> > index d0d98e7..df5d8d9 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -114,8 +114,11 @@ void qmp_cpu(int64_t index, Error **errp)
> >
> >  void qmp_cpu_add(int64_t id, Error **errp)
> >  {
> > -    if (current_machine->hot_add_cpu) {
> > -        current_machine->hot_add_cpu(id, errp);
> > +    QemuMachineClass *machine_class;
> > +
> > +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> > +    if (machine_class->qemu_machine->hot_add_cpu) {
> > +        machine_class->qemu_machine->hot_add_cpu(id, errp);
> >      } else {
> >          error_setg(errp, "Not supported");
> >      }
> > diff --git a/vl.c b/vl.c
> > index 50c880f..c4939ef 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1529,7 +1529,7 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
> >  /***********************************************************/
> >  /* machine registration */
> >
> > -QEMUMachine *current_machine = NULL;
> > +QemuMachineState *current_machine;
> >
> >  static void qemu_machine_class_init(ObjectClass *klass, void *data)
> >  {
> > @@ -1552,44 +1552,45 @@ int qemu_register_machine(QEMUMachine *m)
> >      return 0;
> >  }
> >
> > -static QEMUMachine *find_machine(const char *name)
> > +static QemuMachineClass *find_machine(const char *name)
> >  {
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > -    QEMUMachine *m = NULL;
> > +    QemuMachineClass *k = NULL;
> >
> >      for (el = machines; el; el = el->next) {
> > -        QemuMachineClass *k = el->data;
> > +        QemuMachineClass *temp = el->data;
> >
> > -        if (!strcmp(k->qemu_machine->name, name)) {
> > -            m = k->qemu_machine;
> > +        if (!strcmp(temp->qemu_machine->name, name)) {
> > +            k = temp;
> >              break;
> >          }
> > -        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> > -            m = k->qemu_machine;
> > +        if (temp->qemu_machine->alias &&
> > +            !strcmp(temp->qemu_machine->alias, name)) {
> > +            k = temp;
> >              break;
> >          }
> >      }
> >
> >      g_slist_free(machines);
> > -    return m;
> > +    return k;
> >  }
> >
> > -QEMUMachine *find_default_machine(void)
> > +QemuMachineClass *find_default_machine(void)
> >  {
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > -    QEMUMachine *m = NULL;
> > +    QemuMachineClass *k = NULL;
> >
> >      for (el = machines; el; el = el->next) {
> > -        QemuMachineClass *k = el->data;
> > +        QemuMachineClass *temp = el->data;
> >
> > -        if (k->qemu_machine->is_default) {
> > -            m = k->qemu_machine;
> > +        if (temp->qemu_machine->is_default) {
> > +            k = temp;
> >              break;
> >          }
> >      }
> >
> >      g_slist_free(machines);
> > -    return m;
> > +    return k;
> >  }
> >
> >  MachineInfoList *qmp_query_machines(Error **errp)
> > @@ -1818,8 +1819,13 @@ void qemu_devices_reset(void)
> >
> >  void qemu_system_reset(bool report)
> >  {
> > -    if (current_machine && current_machine->reset) {
> > -        current_machine->reset();
> > +    QemuMachineClass *machine_class;
> > +
> > +    machine_class = current_machine ? QEMU_MACHINE_GET_CLASS(current_machine)
> > +                    : NULL;
> > +
> > +    if (machine_class && machine_class->qemu_machine->reset) {
> > +        machine_class->qemu_machine->reset();
> >      } else {
> >          qemu_devices_reset();
> >      }
> > @@ -2565,21 +2571,21 @@ static int debugcon_parse(const char *devname)
> >      return 0;
> >  }
> >
> > -static QEMUMachine *machine_parse(const char *name)
> > +static QemuMachineClass *machine_parse(const char *name)
> >  {
> > -    QEMUMachine *m, *machine = NULL;
> > +    QemuMachineClass *machine_class = NULL;
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> >
> >      if (name) {
> > -        machine = find_machine(name);
> > +        machine_class = find_machine(name);
> >      }
> > -    if (machine) {
> > -        return machine;
> > +    if (machine_class) {
> > +        return machine_class;
> >      }
> >      printf("Supported machines are:\n");
> >      for (el = machines; el; el = el->next) {
> >          QemuMachineClass *k = el->data;
> > -        m = k->qemu_machine;
> > +        QEMUMachine *m = k->qemu_machine;
> >          if (m->alias) {
> >              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
> >          }
> > @@ -2836,6 +2842,7 @@ int main(int argc, char **argv, char **envp)
> >      int optind;
> >      const char *optarg;
> >      const char *loadvm = NULL;
> > +    QemuMachineClass *machine_class;
> >      QEMUMachine *machine;
> >      const char *cpu_model;
> >      const char *vga_model = "none";
> > @@ -2908,7 +2915,7 @@ int main(int argc, char **argv, char **envp)
> >      os_setup_early_signal_handling();
> >
> >      module_call_init(MODULE_INIT_MACHINE);
> > -    machine = find_default_machine();
> > +    machine_class = find_default_machine();
> >      cpu_model = NULL;
> >      ram_size = 0;
> >      snapshot = 0;
> > @@ -2974,7 +2981,7 @@ int main(int argc, char **argv, char **envp)
> >              }
> >              switch(popt->index) {
> >              case QEMU_OPTION_M:
> > -                machine = machine_parse(optarg);
> > +                machine_class = machine_parse(optarg);
> >                  break;
> >              case QEMU_OPTION_no_kvm_irqchip: {
> >                  olist = qemu_find_opts("machine");
> > @@ -3530,7 +3537,7 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  optarg = qemu_opt_get(opts, "type");
> >                  if (optarg) {
> > -                    machine = machine_parse(optarg);
> > +                    machine_class = machine_parse(optarg);
> >                  }
> >                  break;
> >               case QEMU_OPTION_no_kvm:
> > @@ -3844,11 +3851,15 @@ int main(int argc, char **argv, char **envp)
> >      }
> >  #endif
> >
> > -    if (machine == NULL) {
> > +    if (machine_class == NULL) {
> >          fprintf(stderr, "No machine found.\n");
> >          exit(1);
> >      }
> >
> > +    current_machine = QEMU_MACHINE(object_new(object_class_get_name(
> > +                          OBJECT_CLASS(machine_class))));
> > +
> > +    machine = machine_class->qemu_machine;
> >      if (machine->hw_version) {
> >          qemu_set_version(machine->hw_version);
> >      }
> > @@ -4277,6 +4288,8 @@ int main(int argc, char **argv, char **envp)
> >                                   .kernel_cmdline = kernel_cmdline,
> >                                   .initrd_filename = initrd_filename,
> >                                   .cpu_model = cpu_model };
> > +
> > +    current_machine->init_args = args;
> >      machine->init(&args);
> >
> >      audio_init();
> > @@ -4285,8 +4298,6 @@ int main(int argc, char **argv, char **envp)
> >
> >      set_numa_modes();
> >
> > -    current_machine = machine;
> > -
> >      /* init USB devices */
> >      if (usb_enabled(false)) {
> >          if (foreach_device_config(DEV_USB, usb_parse) < 0)
> >
> 

Re: [Qemu-devel] [PATCH RFC V2 7/9] qom: add object_property_is_set

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:13 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > Sometimes is not enough to get property's value,
> > but it is needed to know if the value was actually set.
> >
> > This is especially useful when querying bool properties
> > and having different defaults on different scenarios.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> I suggested some replacements for this in the review to patch 8/9.
> 
> The problem is that tristate properties implemented with 
> object_property_is_set cannot be "round-trip"-ped out and back in of QEMU.

I understand, I will go with required/allowed getters.

Thanks,
Marcel

> 
> Paolo
> 
> > ---
> >  include/qom/object.h | 11 +++++++++++
> >  qom/object.c         | 12 ++++++++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/include/qom/object.h b/include/qom/object.h
> > index 9c7c361..4a4f026 100644
> > --- a/include/qom/object.h
> > +++ b/include/qom/object.h
> > @@ -319,6 +319,7 @@ typedef struct ObjectProperty
> >  {
> >      gchar *name;
> >      gchar *type;
> > +    bool is_set;
> >      ObjectPropertyAccessor *get;
> >      ObjectPropertyAccessor *set;
> >      ObjectPropertyRelease *release;
> > @@ -931,6 +932,16 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name,
> >                           Error **errp);
> >
> >  /**
> > + * object_property_is_set:
> > + * @obj: the object
> > + * @name: the name of the property
> > + * @errp: returns an error if this function fails
> > + *
> > + * Returns: true if object's property is set, false otherwise.
> > + */
> > +bool object_property_is_set(Object *obj, const char *name,
> > +                            Error **errp);
> > +/**
> >   * object_property_parse:
> >   * @obj: the object
> >   * @string: the string that will be used to parse the property value.
> > diff --git a/qom/object.c b/qom/object.c
> > index 660859c..5b9d8af 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -817,9 +817,21 @@ void object_property_set(Object *obj, Visitor *v, const char *name,
> >          error_set(errp, QERR_PERMISSION_DENIED);
> >      } else {
> >          prop->set(obj, v, prop->opaque, name, errp);
> > +        prop->is_set = true;
> >      }
> >  }
> >
> > +bool object_property_is_set(Object *obj, const char *name,
> > +                            Error **errp)
> > +{
> > +    ObjectProperty *prop = object_property_find(obj, name, errp);
> > +    if (prop == NULL) {
> > +        return false;
> > +    }
> > +
> > +    return prop->is_set;
> > +}
> > +
> >  void object_property_set_str(Object *obj, const char *value,
> >                               const char *name, Error **errp)
> >  {
> >
> 

Re: [Qemu-devel] [PATCH RFC V2 6/9] vl.c: do not set 'type' property in obj_set_property

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:11 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > Filter out also 'type' property when setting
> > object's properties
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  vl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/vl.c b/vl.c
> > index c4939ef..dc206e1 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2766,7 +2766,8 @@ static int object_set_property(const char *name, const char *value, void *opaque
> >      StringInputVisitor *siv;
> >      Error *local_err = NULL;
> >
> > -    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0) {
> > +    if (strcmp(name, "qom-type") == 0 || strcmp(name, "id") == 0 ||
> > +        strcmp(name, "type") == 0) {
> >          return 0;
> >      }
> >
> >
> 
> Unfortunately, it is quite possible to have a -object invocation where 
> the object has a "type" property.
Thanks, I was thinking that "type" is a built-in property of the Object,
derived directly from class's type, so no one can "set" this property.
> 
> I think you could change the -object implementation to use OptsVisitor, 
> similar to hmp_object_add in hmp.c.  Then the -object code can pre-parse 
> "qom-type" and "id", while the -machine code can pre-parse "type".
I'll look into it, thanks for the tip.

Thanks,
Marcel

> 
> Paolo

Re: [Qemu-devel] [PATCH RFC V2 9/9] hw/core: mapped QemuOpts into QEMUMachineInitArgs fields to remove duplication

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:13 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > The QOM machine has both field per QemuOpt and an instance of
> > QEMUMachineInitArgs. Removed dupplications.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> I think this patch is a bit backwards. :)
I didn't like it either, but there are still places that use
directly QEMUMachineInitArgs's fields, this is the reason I did
it this way.
 
> 
> You should _start_ with properties that access QEMUMachineInitArgs 
> fields, and nothing like the "char *kernel" fields you currently have in 
> QemuMachineState.  Then, as part of a patch that does a global 
> s/QEMUMachineInitArgs/QemuMachineState/, you move the fields from 
> QEMUMachineInitArgs to QemuMachineState, and remove the "init_args." 
> indirection in the property accessors.
> 
> BTW, it is much simpler if you do not change the field names in the 
> process: keep kernel_filename in QemuMachineState, do not change it to 
> kernel.
My idea was to tackle the machine-opts first, and be able to replace
all the qemu_opt_get calls with QOM syntax.
This is the reason you see "kernel" in QemuMachineState and not "kernel_filename".
It was easier (at least for me) to handle this transition with 1-1 translation
between the machine option and the QemuMachineState field.

I completely agree with you about moving the fields from QEMUMachineInitArgs
to QemuMachineState, but I would like to handle them as second step (meaning now),
of course I can switch back the field names to the more intuitive ones.
 
I'll think what do to with this patch...

Thanks,
Marcel
> 
> Paolo
> 
> > ---
> >  hw/core/machine.c   | 18 +++++++++---------
> >  include/hw/boards.h |  9 +++------
> >  vl.c                |  6 +++---
> >  3 files changed, 15 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 436829c..73f61bd 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -67,37 +67,37 @@ static void machine_set_kvm_shadow_mem(Object *obj, Visitor *v,
> >  static char *machine_get_kernel(Object *obj, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    return g_strdup(machine_state->kernel);
> > +    return g_strdup(machine_state->init_args.kernel_filename);
> >  }
> >
> >  static void machine_set_kernel(Object *obj, const char *value, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    machine_state->kernel = g_strdup(value);
> > +    machine_state->init_args.kernel_filename = g_strdup(value);
> >  }
> >
> >  static char *machine_get_initrd(Object *obj, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    return g_strdup(machine_state->initrd);
> > +    return g_strdup(machine_state->init_args.initrd_filename);
> >  }
> >
> >  static void machine_set_initrd(Object *obj, const char *value, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    machine_state->initrd = g_strdup(value);
> > +    machine_state->init_args.initrd_filename = g_strdup(value);
> >  }
> >
> >  static char *machine_get_append(Object *obj, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    return g_strdup(machine_state->append);
> > +    return g_strdup(machine_state->init_args.kernel_cmdline);
> >  }
> >
> >  static void machine_set_append(Object *obj, const char *value, Error **errp)
> >  {
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> > -    machine_state->append = g_strdup(value);
> > +    machine_state->init_args.kernel_cmdline = g_strdup(value);
> >  }
> >
> >  static char *machine_get_dtb(Object *obj, Error **errp)
> > @@ -261,9 +261,9 @@ static void qemu_machine_finalize(Object *obj)
> >      QemuMachineState *machine_state = QEMU_MACHINE(obj);
> >
> >      g_free(machine_state->accel);
> > -    g_free(machine_state->kernel);
> > -    g_free(machine_state->initrd);
> > -    g_free(machine_state->append);
> > +    g_free(machine_state->init_args.kernel_filename);
> > +    g_free(machine_state->init_args.initrd_filename);
> > +    g_free(machine_state->init_args.kernel_cmdline);
> >      g_free(machine_state->dtb);
> >      g_free(machine_state->dumpdtb);
> >      g_free(machine_state->dt_compatible);
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 053c113..4b1979e 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -13,9 +13,9 @@ typedef struct QEMUMachineInitArgs {
> >      const QEMUMachine *machine;
> >      ram_addr_t ram_size;
> >      const char *boot_order;
> > -    const char *kernel_filename;
> > -    const char *kernel_cmdline;
> > -    const char *initrd_filename;
> > +    char *kernel_filename;
> > +    char *kernel_cmdline;
> > +    char *initrd_filename;
> >      const char *cpu_model;
> >  } QEMUMachineInitArgs;
> >
> > @@ -91,9 +91,6 @@ struct QemuMachineState {
> >      char *accel;
> >      bool kernel_irqchip;
> >      int kvm_shadow_mem;
> > -    char *kernel;
> > -    char *initrd;
> > -    char *append;
> >      char *dtb;
> >      char *dumpdtb;
> >      int phandle_start;
> > diff --git a/vl.c b/vl.c
> > index 007342c..3d7357e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2834,8 +2834,8 @@ int main(int argc, char **argv, char **envp)
> >      int i;
> >      int snapshot, linux_boot;
> >      const char *icount_option = NULL;
> > -    const char *initrd_filename;
> > -    const char *kernel_filename, *kernel_cmdline;
> > +    char *initrd_filename;
> > +    char *kernel_filename, *kernel_cmdline;
> >      const char *boot_order;
> >      DisplayState *ds;
> >      int cyls, heads, secs, translation;
> > @@ -4129,7 +4129,7 @@ int main(int argc, char **argv, char **envp)
> >      }
> >
> >      if (!kernel_cmdline) {
> > -        kernel_cmdline = "";
> > +        kernel_cmdline = (char *)"";
> >      }
> >
> >      linux_boot = (kernel_filename != NULL);
> >
> 

Re: [Qemu-devel] [PATCH RFC V2 8/9] machine-opts: replace qemu_opt_get by QOM QemuMachine queries

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:11 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > @@ -1182,10 +1183,17 @@ ram_addr_t last_ram_offset(void)
> >  static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
> >  {
> >      int ret;
> > +    bool dump_guest_core = true;
> > +
> > +    if (object_property_is_set(OBJECT(current_machine),
> > +                               "dump-guest-core", &error_abort)) {
> > +        dump_guest_core = object_property_get_bool(OBJECT(current_machine),
> > +                                                   "dump-guest-core",
> > +                                                   &error_abort);
> > +    }
> 
> You do not need object_property_is_set.  You can just initialize the 
> field to true in the instance_init function.  Same for mem_merge.
Sure, I can. I was just trying to leave the current functionality "as is",
is some other code will assume "false" as default.
But I can go for required/allowed getters as you suggested, thanks.

> 
> > @@ -563,11 +565,14 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
> >      mpic = g_new(qemu_irq, 256);
> >
> >      if (kvm_enabled()) {
> > -        QemuOpts *machine_opts = qemu_get_machine_opts();
> > -        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> > -                                                "kernel_irqchip", true);
> > -        bool irqchip_required = qemu_opt_get_bool(machine_opts,
> > -                                                  "kernel_irqchip", false);
> > +        bool irqchip_allowed = true;
> > +        bool irqchip_required = false;
> > +        if (object_property_is_set(OBJECT(current_machine),
> > +                                   "kernel_irqchip", &error_abort)) {
> > +            irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
> > +                                                       "kernel_irqchip", &error_abort);
> > +            irqchip_required = irqchip_allowed;
> > +        }
> 
> Alternatively, you could have three properties: kernel_irqchip, 
> write-only (no getter); kernel-irqchip-allowed; kernel-irqchip-required. 
>   Setting irqchip writes to both kernel-irqchip-allowed and 
> kernel-irqchip-required.  kernel-irqchip-allowed and 
> kernel-irqchip-required are initialized to true and false respectively 
> in the instance_init.
The functionality will not be affected if I use your idea,
both implementations are not "clean", but at least yours does not require
an extra QOM method.

Thanks,
Marcel

> 
> 
> > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > index bdb057e..8651167 100644
> > --- a/hw/ppc/virtex_ml507.c
> > +++ b/hw/ppc/virtex_ml507.c
> > @@ -145,7 +145,8 @@ static int xilinx_load_device_tree(hwaddr addr,
> >      int r;
> >      const char *dtb_filename;
> >
> > -    dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> > +    dtb_filename = object_property_get_str(OBJECT(current_machine),
> > +                                           "dtb", &error_abort);
> >      if (dtb_filename) {
> >          fdt = load_device_tree(dtb_filename, &fdt_size);
> >          if (!fdt) {
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 2ca9143..e483c3c 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -22,7 +22,7 @@
> >
> >  #include "qemu-common.h"
> >  #include "qemu/atomic.h"
> > -#include "qemu/option.h"
> > +#include "hw/boards.h"
> >  #include "qemu/config-file.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/hw.h"
> > @@ -1292,8 +1292,15 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
> >  static int kvm_irqchip_create(KVMState *s)
> >  {
> >      int ret;
> > +    bool irqchip_allowed = true;
> >
> > -    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
> > +    if (object_property_is_set(OBJECT(current_machine),
> > +                               "kernel_irqchip", &error_abort)) {
> > +        irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
> > +                                                   "kernel_irqchip", &error_abort);
> > +    }
> > +
> > +    if (!irqchip_allowed ||
> >          !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
> >          return 0;
> >      }
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index e555040..ca7e6bc 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -33,6 +33,7 @@
> >  #include "exec/ioport.h"
> >  #include <asm/hyperv.h>
> >  #include "hw/pci/pci.h"
> > +#include "hw/boards.h"
> >
> >  //#define DEBUG_KVM
> >
> > @@ -853,8 +854,13 @@ int kvm_arch_init(KVMState *s)
> >      }
> >      qemu_register_reset(kvm_unpoison_all, NULL);
> >
> > -    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
> > -                                   "kvm_shadow_mem", -1);
> > +    shadow_mem = -1;
> > +    if (object_property_is_set(OBJECT(current_machine),
> > +                               "kvm_shadow_mem", &error_abort)) {
> > +        shadow_mem = object_property_get_int(OBJECT(current_machine),
> > +                                             "kvm_shadow_mem", &error_abort);
> > +    }
> 
> This can also be simply initialized in the instance_init function.
Sure, thanks

> 
> Paolo
> 
> > +
> >      if (shadow_mem != -1) {
> >          shadow_mem /= 4096;
> >          ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
> > diff --git a/vl.c b/vl.c
> > index dc206e1..007342c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2624,7 +2624,8 @@ static int configure_accelerator(void)
> >      bool accel_initialised = false;
> >      bool init_failed = false;
> >
> > -    p = qemu_opt_get(qemu_get_machine_opts(), "accel");
> > +    p = object_property_get_str(OBJECT(current_machine),
> > +                                "accel", &error_abort);
> >      if (p == NULL) {
> >          /* Use the default "accelerator", tcg */
> >          p = "tcg";
> > @@ -4077,6 +4078,12 @@ int main(int argc, char **argv, char **envp)
> >          exit(0);
> >      }
> >
> > +    machine_opts = qemu_get_machine_opts();
> > +    if (qemu_opt_foreach(machine_opts, object_set_property, current_machine, 1) < 0) {
> > +        object_unref(OBJECT(current_machine));
> > +        exit(1);
> > +    }
> > +
> >      configure_accelerator();
> >
> >      if (qtest_chrdev) {
> > @@ -4089,12 +4096,14 @@ int main(int argc, char **argv, char **envp)
> >          }
> >      }
> >
> > -    machine_opts = qemu_get_machine_opts();
> > -    kernel_filename = qemu_opt_get(machine_opts, "kernel");
> > -    initrd_filename = qemu_opt_get(machine_opts, "initrd");
> > -    kernel_cmdline = qemu_opt_get(machine_opts, "append");
> > -    bios_name = qemu_opt_get(machine_opts, "firmware");
> > -
> > +    kernel_filename = object_property_get_str(OBJECT(current_machine),
> > +                                              "kernel", &error_abort);
> > +    initrd_filename = object_property_get_str(OBJECT(current_machine),
> > +                                              "initrd", &error_abort);
> > +    kernel_cmdline = object_property_get_str(OBJECT(current_machine),
> > +                                             "append", &error_abort);
> > +    bios_name = object_property_get_str(OBJECT(current_machine),
> > +                                        "firmware", &error_abort);
> >      boot_order = machine->default_boot_order;
> >      opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
> >      if (opts) {
> > @@ -4135,7 +4144,8 @@ int main(int argc, char **argv, char **envp)
> >          exit(1);
> >      }
> >
> > -    if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> > +    if (!linux_boot && object_property_get_str(OBJECT(current_machine),
> > +                                               "dtb", &error_abort)) {
> >          fprintf(stderr, "-dtb only allowed with -kernel option\n");
> >          exit(1);
> >      }
> >
> 

Re: [Qemu-devel] [PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 11:49 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > In order to allow attaching machine options to a machine instance,
> > current_machine is converted into QemuMachineState.
> > As a first step of deprecating QEMUMachine, some of the functions
> > were modified to return QemuMachineCLass.
> 
> This is a relatively large change since current_machine is currently 
> "class-like", while after your patch it is "object-like".
> 
> Not a big problem, since I believe at the end of the conversion there 
> shouldn't be a current_machine at all.  Board initialization code will 
> get it as an argument where they currently have the QEMUMachineInitArgs. 
>   The few that use current_machine before this patch can and should 
> access the QOM tree to retrieve the current machine.
This is the main reason I changed from "class like" code to object,
because in the end the users will use the object returned from the query.

> 
> ... which brings us to what is missing here: you are not adding 
> current_machine as /machine in the QOM tree. :)
Is missing on purpose, I am not sure what are the consequences of changing the /machine
from container to an actual object. I am (almost) sure that this container has child
containers and I am looking for the rules: can an object act like a container?

So, I do intend of course to add it, once I have responded to the above issue.
Thanks,
Marcel
> 
> Paolo
> 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  device-hotplug.c    |  4 +++-
> >  include/hw/boards.h |  6 ++---
> >  qmp.c               |  7 ++++--
> >  vl.c                | 69 +++++++++++++++++++++++++++++++----------------------
> >  4 files changed, 51 insertions(+), 35 deletions(-)
> >
> > diff --git a/device-hotplug.c b/device-hotplug.c
> > index 103d34a..fb5eb01 100644
> > --- a/device-hotplug.c
> > +++ b/device-hotplug.c
> > @@ -33,12 +33,14 @@ DriveInfo *add_init_drive(const char *optstr)
> >  {
> >      DriveInfo *dinfo;
> >      QemuOpts *opts;
> > +    QemuMachineClass *machine_class;
> >
> >      opts = drive_def(optstr);
> >      if (!opts)
> >          return NULL;
> >
> > -    dinfo = drive_init(opts, current_machine->block_default_type);
> > +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> > +    dinfo = drive_init(opts, machine_class->qemu_machine->block_default_type);
> >      if (!dinfo) {
> >          qemu_opts_del(opts);
> >          return NULL;
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 65e1e03..053c113 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -51,9 +51,6 @@ struct QEMUMachine {
> >
> >  #define TYPE_QEMU_MACHINE_PREFIX "machine-"
> >  int qemu_register_machine(QEMUMachine *m);
> > -QEMUMachine *find_default_machine(void);
> > -
> > -extern QEMUMachine *current_machine;
> >
> >  #define TYPE_QEMU_MACHINE "machine"
> >  #define QEMU_MACHINE(obj) \
> > @@ -66,6 +63,9 @@ extern QEMUMachine *current_machine;
> >  typedef struct QemuMachineState QemuMachineState;
> >  typedef struct QemuMachineClass QemuMachineClass;
> >
> > +QemuMachineClass *find_default_machine(void);
> > +extern QemuMachineState *current_machine;
> > +
> >  /**
> >   * @QemuMachineClass
> >   *
> > diff --git a/qmp.c b/qmp.c
> > index d0d98e7..df5d8d9 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -114,8 +114,11 @@ void qmp_cpu(int64_t index, Error **errp)
> >
> >  void qmp_cpu_add(int64_t id, Error **errp)
> >  {
> > -    if (current_machine->hot_add_cpu) {
> > -        current_machine->hot_add_cpu(id, errp);
> > +    QemuMachineClass *machine_class;
> > +
> > +    machine_class = QEMU_MACHINE_GET_CLASS(current_machine);
> > +    if (machine_class->qemu_machine->hot_add_cpu) {
> > +        machine_class->qemu_machine->hot_add_cpu(id, errp);
> >      } else {
> >          error_setg(errp, "Not supported");
> >      }
> > diff --git a/vl.c b/vl.c
> > index 50c880f..c4939ef 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1529,7 +1529,7 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
> >  /***********************************************************/
> >  /* machine registration */
> >
> > -QEMUMachine *current_machine = NULL;
> > +QemuMachineState *current_machine;
> >
> >  static void qemu_machine_class_init(ObjectClass *klass, void *data)
> >  {
> > @@ -1552,44 +1552,45 @@ int qemu_register_machine(QEMUMachine *m)
> >      return 0;
> >  }
> >
> > -static QEMUMachine *find_machine(const char *name)
> > +static QemuMachineClass *find_machine(const char *name)
> >  {
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > -    QEMUMachine *m = NULL;
> > +    QemuMachineClass *k = NULL;
> >
> >      for (el = machines; el; el = el->next) {
> > -        QemuMachineClass *k = el->data;
> > +        QemuMachineClass *temp = el->data;
> >
> > -        if (!strcmp(k->qemu_machine->name, name)) {
> > -            m = k->qemu_machine;
> > +        if (!strcmp(temp->qemu_machine->name, name)) {
> > +            k = temp;
> >              break;
> >          }
> > -        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> > -            m = k->qemu_machine;
> > +        if (temp->qemu_machine->alias &&
> > +            !strcmp(temp->qemu_machine->alias, name)) {
> > +            k = temp;
> >              break;
> >          }
> >      }
> >
> >      g_slist_free(machines);
> > -    return m;
> > +    return k;
> >  }
> >
> > -QEMUMachine *find_default_machine(void)
> > +QemuMachineClass *find_default_machine(void)
> >  {
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > -    QEMUMachine *m = NULL;
> > +    QemuMachineClass *k = NULL;
> >
> >      for (el = machines; el; el = el->next) {
> > -        QemuMachineClass *k = el->data;
> > +        QemuMachineClass *temp = el->data;
> >
> > -        if (k->qemu_machine->is_default) {
> > -            m = k->qemu_machine;
> > +        if (temp->qemu_machine->is_default) {
> > +            k = temp;
> >              break;
> >          }
> >      }
> >
> >      g_slist_free(machines);
> > -    return m;
> > +    return k;
> >  }
> >
> >  MachineInfoList *qmp_query_machines(Error **errp)
> > @@ -1818,8 +1819,13 @@ void qemu_devices_reset(void)
> >
> >  void qemu_system_reset(bool report)
> >  {
> > -    if (current_machine && current_machine->reset) {
> > -        current_machine->reset();
> > +    QemuMachineClass *machine_class;
> > +
> > +    machine_class = current_machine ? QEMU_MACHINE_GET_CLASS(current_machine)
> > +                    : NULL;
> > +
> > +    if (machine_class && machine_class->qemu_machine->reset) {
> > +        machine_class->qemu_machine->reset();
> >      } else {
> >          qemu_devices_reset();
> >      }
> > @@ -2565,21 +2571,21 @@ static int debugcon_parse(const char *devname)
> >      return 0;
> >  }
> >
> > -static QEMUMachine *machine_parse(const char *name)
> > +static QemuMachineClass *machine_parse(const char *name)
> >  {
> > -    QEMUMachine *m, *machine = NULL;
> > +    QemuMachineClass *machine_class = NULL;
> >      GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> >
> >      if (name) {
> > -        machine = find_machine(name);
> > +        machine_class = find_machine(name);
> >      }
> > -    if (machine) {
> > -        return machine;
> > +    if (machine_class) {
> > +        return machine_class;
> >      }
> >      printf("Supported machines are:\n");
> >      for (el = machines; el; el = el->next) {
> >          QemuMachineClass *k = el->data;
> > -        m = k->qemu_machine;
> > +        QEMUMachine *m = k->qemu_machine;
> >          if (m->alias) {
> >              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
> >          }
> > @@ -2836,6 +2842,7 @@ int main(int argc, char **argv, char **envp)
> >      int optind;
> >      const char *optarg;
> >      const char *loadvm = NULL;
> > +    QemuMachineClass *machine_class;
> >      QEMUMachine *machine;
> >      const char *cpu_model;
> >      const char *vga_model = "none";
> > @@ -2908,7 +2915,7 @@ int main(int argc, char **argv, char **envp)
> >      os_setup_early_signal_handling();
> >
> >      module_call_init(MODULE_INIT_MACHINE);
> > -    machine = find_default_machine();
> > +    machine_class = find_default_machine();
> >      cpu_model = NULL;
> >      ram_size = 0;
> >      snapshot = 0;
> > @@ -2974,7 +2981,7 @@ int main(int argc, char **argv, char **envp)
> >              }
> >              switch(popt->index) {
> >              case QEMU_OPTION_M:
> > -                machine = machine_parse(optarg);
> > +                machine_class = machine_parse(optarg);
> >                  break;
> >              case QEMU_OPTION_no_kvm_irqchip: {
> >                  olist = qemu_find_opts("machine");
> > @@ -3530,7 +3537,7 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  optarg = qemu_opt_get(opts, "type");
> >                  if (optarg) {
> > -                    machine = machine_parse(optarg);
> > +                    machine_class = machine_parse(optarg);
> >                  }
> >                  break;
> >               case QEMU_OPTION_no_kvm:
> > @@ -3844,11 +3851,15 @@ int main(int argc, char **argv, char **envp)
> >      }
> >  #endif
> >
> > -    if (machine == NULL) {
> > +    if (machine_class == NULL) {
> >          fprintf(stderr, "No machine found.\n");
> >          exit(1);
> >      }
> >
> > +    current_machine = QEMU_MACHINE(object_new(object_class_get_name(
> > +                          OBJECT_CLASS(machine_class))));
> > +
> > +    machine = machine_class->qemu_machine;
> >      if (machine->hw_version) {
> >          qemu_set_version(machine->hw_version);
> >      }
> > @@ -4277,6 +4288,8 @@ int main(int argc, char **argv, char **envp)
> >                                   .kernel_cmdline = kernel_cmdline,
> >                                   .initrd_filename = initrd_filename,
> >                                   .cpu_model = cpu_model };
> > +
> > +    current_machine->init_args = args;
> >      machine->init(&args);
> >
> >      audio_init();
> > @@ -4285,8 +4298,6 @@ int main(int argc, char **argv, char **envp)
> >
> >      set_numa_modes();
> >
> > -    current_machine = machine;
> > -
> >      /* init USB devices */
> >      if (usb_enabled(false)) {
> >          if (foreach_device_config(DEV_USB, usb_parse) < 0)
> >
> 

Re: [Qemu-devel] [PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass

[Paolo Bonzini]

Il 03/03/2014 13:07, Marcel Apfelbaum ha scritto:
>> > ... which brings us to what is missing here: you are not adding
>> > current_machine as /machine in the QOM tree. :)
> Is missing in purpose, I am not sure what are the consequences of changing the /machine
> from container to an actual object. I am (almost) sure that this container has child
> containers and I am looking for the rules: can an object act like a container?

Yes, it can.

"container" is a separate class than "object" only for documentation 
reasons.

Paolo

> So, I do intend of course to add it, once I have responded to the above issue.

Re: [Qemu-devel] [PATCH RFC V2 6/9] vl.c: do not set 'type' property in obj_set_property

[Paolo Bonzini]

Il 03/03/2014 13:09, Marcel Apfelbaum ha scritto:
>> > Unfortunately, it is quite possible to have a -object invocation where
>> > the object has a "type" property.
> Thanks, I was thinking that "type" is a built-in property of the Object,
> derived directly from class's type, so no one can "set" this property.

Actually you're right.  Perhaps we can make -object take "type" instead 
of "qom-type" too.

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

Paolo

Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Paolo Bonzini]

Il 03/03/2014 13:07, Marcel Apfelbaum ha scritto:
>> Another early refactoring should be to pass &current_machine->init_args
>> to machine->init, not the "args".
> Problem is that this is a "private field", should I add a getter for it?

vl.c is already accessing it one line before, isn't it?

So vl.c is already looking at QemuMachineState internals.  I expected 
this to be temporary.

>> Now here's a possible plan to get rid of QEMUMachineInitArgs:
>>
>> 1) Add three wrappers to QemuMachine for reading kernel_filename,
>> kernel_cmdline, initrd_filename.  Unlike object_property_get_str, they
>> can skip the strdup of the value.  This way you don't have to add the
>> matching free to all uses of the fields.
>
> I have nothing against it, but this will break QEMU's unified way to
> handle object properties, right?
> Meaning I will have a field  of the object state(kernel_filename)
> and I will add a method like machine_get_field(QemuMachineState) going
> "around" QOM,,,

True.  On the other hand, I believe there is a purpose in using getters 
and setters: using object_property_get/set all the time is more verbose 
and especially less type-safe.  I prefer adding getters to naming 
properties with #define.  Especially if you need getters but not setters.

GObject uses a similar scheme where you have both regular 
getters/setters and properties.  Static languages use the getters and 
setters, while bindings for dynamic languages will always go through 
properties.

>> 2) Similarly, add get/set functions (not properties, since these are not
>> accessible via -machine) for ram_size, boot_order, cpu_model.
>
> I am sorry, here you lost me, what do you mean "accessible via -machine"?
> Maybe that cannot be queried by QOM tree?

Not yet, at least.  I prefer to keep these fields out of QOM because 
they cannot be set with -machine.  cpu_model is already accessible since 
it's related to the class of the CPU devices.

> Those getter/setters are not the same wrappers as above, going around "QOM"?

Actually they are the same thing.  But I think above you only need 
getters.  Here you also need setters.

>> 3) Now you can have something like patch 8 in this series.
> I also need patch 5 that deals with getting a string property with NULL value,

Ok.

> I see no reason why not, the main problem I see is the use of those wrappers
> or setters/getters, I suspect that the usage will be:
> 1. global QOM query to get the machine
> 2. Use this wrappers(getters/setters) to do query/alter the machine.

I think setters should appear only in vl.c.

> Doesn't QOM have another way to do this? Or I am missing something, of course.

I don't know really.  My main requirement is not to change the -machine 
interface.

Paolo

Re: [Qemu-devel] [PATCH RFC V2 1/9] hw/core: introduced qemu machine as QOM object

[Michael S. Tsirkin]

On Sun, Mar 02, 2014 at 03:07:04PM +0200, Marcel Apfelbaum wrote:
> The main functionality change is to convert QEMUMachine into QemuMachineClass
> and QEMUMachineInitArgs into QemuMachineState, instance of QemuMachineClass.
> 
> As a first step, in order to make possible an incremental developement,
> both QEMUMachine and QEMUMachineInitArgs are being embeded into the
> new types.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/core/Makefile.objs |  2 +-
>  hw/core/machine.c     | 38 +++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h   | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/machine.c
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 9e324be..f80c13c 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -11,4 +11,4 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> -
> +common-obj-$(CONFIG_SOFTMMU) += machine.o
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> new file mode 100644
> index 0000000..2c6e1a3
> --- /dev/null
> +++ b/hw/core/machine.c
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU Machine
> + *
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Authors:
> + *   Marcel Apfelbaum <marcel.a@redhat.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.
> + */
> +
> +#include "hw/boards.h"
> +
> +static void qemu_machine_initfn(Object *obj)
> +{
> +}
> +
> +static void qemu_machine_class_init(ObjectClass *oc, void *data)
> +{
> +}
> +
> +static const TypeInfo qemu_machine_info = {
> +    .name = TYPE_QEMU_MACHINE,
> +    .parent = TYPE_OBJECT,
> +    .abstract = true,
> +    .class_size = sizeof(QemuMachineClass),
> +    .class_init = qemu_machine_class_init,
> +    .instance_size = sizeof(QemuMachineState),
> +    .instance_init = qemu_machine_initfn,
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&qemu_machine_info);
> +}
> +
> +type_init(register_types);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2151460..7b4708d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -5,6 +5,7 @@
>  
>  #include "sysemu/blockdev.h"
>  #include "hw/qdev.h"
> +#include "qom/object.h"
>  
>  typedef struct QEMUMachine QEMUMachine;
>  
> @@ -53,4 +54,55 @@ QEMUMachine *find_default_machine(void);
>  
>  extern QEMUMachine *current_machine;
>  
> +#define TYPE_QEMU_MACHINE "machine"
> +#define QEMU_MACHINE(obj) \
> +    OBJECT_CHECK(QemuMachineState, (obj), TYPE_QEMU_MACHINE)
> +#define QEMU_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(QemuMachineClass, (obj), TYPE_QEMU_MACHINE)
> +#define QEMU_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(QemuMachineClass, (klass), TYPE_QEMU_MACHINE)
> +
> +typedef struct QemuMachineState QemuMachineState;
> +typedef struct QemuMachineClass QemuMachineClass;
> +
> +/**
> + * @QemuMachineClass
> + *
> + * @parent_class: opaque parent class container
> + */
> +struct QemuMachineClass {
> +    ObjectClass parent_class;
> +
> +    QEMUMachine *qemu_machine;
> +};
> +
> +/**
> + * @QemuMachineState
> + *
> + * @parent: opaque parent object container
> + */
> +struct QemuMachineState {
> +    /* private */
> +    Object parent;
> +    /* public */
> +
> +
> +    char *accel;
> +    bool kernel_irqchip;
> +    int kvm_shadow_mem;
> +    char *kernel;
> +    char *initrd;
> +    char *append;
> +    char *dtb;
> +    char *dumpdtb;
> +    int phandle_start;
> +    char *dt_compatible;
> +    bool dump_guest_core;
> +    bool mem_merge;
> +    bool usb;
> +    char *firmware;
> +
> +    QEMUMachineInitArgs init_args;
> +};
> +
>  #endif
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Paolo Bonzini]

Il 03/03/2014 13:58, Michael S. Tsirkin ha scritto:
> On Sun, Mar 02, 2014 at 03:07:05PM +0200, Marcel Apfelbaum wrote:
>> > The machine registration flow is refactored to use the QOM functionality.
>> > Instead of linking the machines into a list, each machine has a type
>> > and the types can be traversed in the QOM way.
>> >
>> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> Nice.
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>
> This patch already looks like a nice cleanup. How about queueing
> this and the preceding patch straight away?
> Which tree is appropriate? I'm guessing QOM?
>

Fine by me.

Paolo

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Michael S. Tsirkin]

On Sun, Mar 02, 2014 at 03:07:05PM +0200, Marcel Apfelbaum wrote:
> The machine registration flow is refactored to use the QOM functionality.
> Instead of linking the machines into a list, each machine has a type
> and the types can be traversed in the QOM way.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Nice.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

This patch already looks like a nice cleanup. How about queueing
this and the preceding patch straight away?
Which tree is appropriate? I'm guessing QOM?

> ---
>  include/hw/boards.h |  1 +
>  vl.c                | 75 ++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7b4708d..65e1e03 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -49,6 +49,7 @@ struct QEMUMachine {
>      const char *hw_version;
>  };
>  
> +#define TYPE_QEMU_MACHINE_PREFIX "machine-"
>  int qemu_register_machine(QEMUMachine *m);
>  QEMUMachine *find_default_machine(void);
>  
> diff --git a/vl.c b/vl.c
> index 9379d33..50c880f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1529,54 +1529,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
>  /***********************************************************/
>  /* machine registration */
>  
> -static QEMUMachine *first_machine = NULL;
>  QEMUMachine *current_machine = NULL;
>  
> +static void qemu_machine_class_init(ObjectClass *klass, void *data)
> +{
> +    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);
> +
> +    k->qemu_machine = data;
> +}
> +
>  int qemu_register_machine(QEMUMachine *m)
>  {
> -    QEMUMachine **pm;
> -    pm = &first_machine;
> -    while (*pm != NULL)
> -        pm = &(*pm)->next;
> -    m->next = NULL;
> -    *pm = m;
> +    TypeInfo ti = {
> +        .name       = g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->name, NULL),
> +        .parent     = TYPE_QEMU_MACHINE,
> +        .class_init = qemu_machine_class_init,
> +        .class_data = (void *)m,
> +    };
> +
> +    type_register(&ti);
> +
>      return 0;
>  }
>  
>  static QEMUMachine *find_machine(const char *name)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
> +
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (!strcmp(m->name, name))
> -            return m;
> -        if (m->alias && !strcmp(m->alias, name))
> -            return m;
> +        if (!strcmp(k->qemu_machine->name, name)) {
> +            m = k->qemu_machine;
> +            break;
> +        }
> +        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> +            m = k->qemu_machine;
> +            break;
> +        }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  QEMUMachine *find_default_machine(void)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (m->is_default) {
> -            return m;
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +
> +        if (k->qemu_machine->is_default) {
> +            m = k->qemu_machine;
> +            break;
>          }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  MachineInfoList *qmp_query_machines(Error **errp)
>  {
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>      MachineInfoList *mach_list = NULL;
>      QEMUMachine *m;
>  
> -    for (m = first_machine; m; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>          MachineInfoList *entry;
>          MachineInfo *info;
>  
> +        m = k->qemu_machine;
>          info = g_malloc0(sizeof(*info));
>          if (m->is_default) {
>              info->has_is_default = true;
> @@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
>          mach_list = entry;
>      }
>  
> +    g_slist_free(machines);
>      return mach_list;
>  }
>  
> @@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname)
>  static QEMUMachine *machine_parse(const char *name)
>  {
>      QEMUMachine *m, *machine = NULL;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>  
>      if (name) {
>          machine = find_machine(name);
> @@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char *name)
>          return machine;
>      }
>      printf("Supported machines are:\n");
> -    for (m = first_machine; m != NULL; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +        m = k->qemu_machine;
>          if (m->alias) {
>              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>          }
>          printf("%-20s %s%s\n", m->name, m->desc,
>                 m->is_default ? " (default)" : "");
>      }
> +
> +    g_slist_free(machines);
>      exit(!name || !is_help_option(name));
>  }
>  
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 13:57 +0100, Paolo Bonzini wrote:
> Il 03/03/2014 13:58, Michael S. Tsirkin ha scritto:
> > On Sun, Mar 02, 2014 at 03:07:05PM +0200, Marcel Apfelbaum wrote:
> >> > The machine registration flow is refactored to use the QOM functionality.
> >> > Instead of linking the machines into a list, each machine has a type
> >> > and the types can be traversed in the QOM way.
> >> >
> >> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > Nice.
Thanks!

> >
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > This patch already looks like a nice cleanup. How about queueing
> > this and the preceding patch straight away?
> > Which tree is appropriate? I'm guessing QOM?
> >
> 
> Fine by me.
Thanks!

Where can I find the QOM tree? I ask because I will rebase the series on this tree...
Marcel
 
> 
> Paolo

Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 13:56 +0100, Paolo Bonzini wrote:
> Il 03/03/2014 13:07, Marcel Apfelbaum ha scritto:
> >> Another early refactoring should be to pass &current_machine->init_args
> >> to machine->init, not the "args".
> > Problem is that this is a "private field", should I add a getter for it?
> 
> vl.c is already accessing it one line before, isn't it?
Yes it is :(

> 
> So vl.c is already looking at QemuMachineState internals.  I expected 
> this to be temporary.
Yes, it will be temporary.

> 
> >> Now here's a possible plan to get rid of QEMUMachineInitArgs:
> >>
> >> 1) Add three wrappers to QemuMachine for reading kernel_filename,
> >> kernel_cmdline, initrd_filename.  Unlike object_property_get_str, they
> >> can skip the strdup of the value.  This way you don't have to add the
> >> matching free to all uses of the fields.
> >
> > I have nothing against it, but this will break QEMU's unified way to
> > handle object properties, right?
> > Meaning I will have a field  of the object state(kernel_filename)
> > and I will add a method like machine_get_field(QemuMachineState) going
> > "around" QOM,,,
> 
> True.  On the other hand, I believe there is a purpose in using getters 
> and setters: using object_property_get/set all the time is more verbose 
> and especially less type-safe.  I prefer adding getters to naming 
> properties with #define.  Especially if you need getters but not setters.
I kind of don't like using the QOM getters/setters too (when I actually know the object type)...
Do we have in Qemu getters to properties declared with #define? I am
going to have a look on GObject too, thanks.
 
> 
> GObject uses a similar scheme where you have both regular 
> getters/setters and properties.  Static languages use the getters and 
> setters, while bindings for dynamic languages will always go through 
> properties.
> 
> >> 2) Similarly, add get/set functions (not properties, since these are not
> >> accessible via -machine) for ram_size, boot_order, cpu_model.
> >
> > I am sorry, here you lost me, what do you mean "accessible via -machine"?
> > Maybe that cannot be queried by QOM tree?
> 
> Not yet, at least.  I prefer to keep these fields out of QOM because 
> they cannot be set with -machine.  cpu_model is already accessible since 
> it's related to the class of the CPU devices.
Got it, thanks.

> 
> > Those getter/setters are not the same wrappers as above, going around "QOM"?
> 
> Actually they are the same thing.  But I think above you only need 
> getters.  Here you also need setters.
OK.

> 
> >> 3) Now you can have something like patch 8 in this series.
> > I also need patch 5 that deals with getting a string property with NULL value,
> 
> Ok.
> 
> > I see no reason why not, the main problem I see is the use of those wrappers
> > or setters/getters, I suspect that the usage will be:
> > 1. global QOM query to get the machine
> > 2. Use this wrappers(getters/setters) to do query/alter the machine.
> 
> I think setters should appear only in vl.c.
Conclusion:
1. It is OK to add getters for QemuMachineState fields in the corresponding
   header file. They can be used when the caller "knows" the object type.
2. In order to populate the fields, static setters that access the internal fields
   are also OK - as long as their scope is very small(e.g: vl.c) and not code wide.

Thanks for the detailed explanations!
Marcel
 
> 
> > Doesn't QOM have another way to do this? Or I am missing something, of course.
> 
> I don't know really.  My main requirement is not to change the -machine 
> interface.
> 
> Paolo

Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object

[Andreas Färber]

Am 03.03.2014 14:17, schrieb Marcel Apfelbaum:
> On Mon, 2014-03-03 at 13:56 +0100, Paolo Bonzini wrote:
>> Il 03/03/2014 13:07, Marcel Apfelbaum ha scritto:
>>> I see no reason why not, the main problem I see is the use of those wrappers
>>> or setters/getters, I suspect that the usage will be:
>>> 1. global QOM query to get the machine
>>> 2. Use this wrappers(getters/setters) to do query/alter the machine.
>>
>> I think setters should appear only in vl.c.
> Conclusion:
> 1. It is OK to add getters for QemuMachineState fields in the corresponding
>    header file. They can be used when the caller "knows" the object type.
> 2. In order to populate the fields, static setters that access the internal fields
>    are also OK - as long as their scope is very small(e.g: vl.c) and not code wide.

+1

QOM does not prohibit writing methods that are in fact getters or
setters. The question we have used as guideline is whether the
information of that field is relevant for QMP-based management tools
(e.g., ACPI) or duplicating QOM getters/setters needed for internal
infrastructure (s390x).

Cheers,
Andreas

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

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Andreas Färber]

Am 03.03.2014 14:03, schrieb Marcel Apfelbaum:
> On Mon, 2014-03-03 at 13:57 +0100, Paolo Bonzini wrote:
>> Il 03/03/2014 13:58, Michael S. Tsirkin ha scritto:
>>> On Sun, Mar 02, 2014 at 03:07:05PM +0200, Marcel Apfelbaum wrote:
>>>>> The machine registration flow is refactored to use the QOM functionality.
>>>>> Instead of linking the machines into a list, each machine has a type
>>>>> and the types can be traversed in the QOM way.
>>>>>
>>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>> Nice.
> Thanks!
> 
>>>
>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> This patch already looks like a nice cleanup. How about queueing
>>> this and the preceding patch straight away?
>>> Which tree is appropriate? I'm guessing QOM?
>>>
>>
>> Fine by me.
> Thanks!
> 
> Where can I find the QOM tree? I ask because I will rebase the series on this tree...

github.com/afaerber/qemu-cpu qom-next

but I hope rebasing won't be necessary, just give me some more time to
look at the series.

Thanks a lot for your efforts already, it sounds very promising!

Cheers,
Andreas

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

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 15:52 +0100, Andreas Färber wrote:
> Am 03.03.2014 14:03, schrieb Marcel Apfelbaum:
> > On Mon, 2014-03-03 at 13:57 +0100, Paolo Bonzini wrote:
> >> Il 03/03/2014 13:58, Michael S. Tsirkin ha scritto:
> >>> On Sun, Mar 02, 2014 at 03:07:05PM +0200, Marcel Apfelbaum wrote:
> >>>>> The machine registration flow is refactored to use the QOM functionality.
> >>>>> Instead of linking the machines into a list, each machine has a type
> >>>>> and the types can be traversed in the QOM way.
> >>>>>
> >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>> Nice.
> > Thanks!
> > 
> >>>
> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> >>>
> >>> This patch already looks like a nice cleanup. How about queueing
> >>> this and the preceding patch straight away?
> >>> Which tree is appropriate? I'm guessing QOM?
> >>>
> >>
> >> Fine by me.
> > Thanks!
> > 
> > Where can I find the QOM tree? I ask because I will rebase the series on this tree...
> 
> github.com/afaerber/qemu-cpu qom-next
> 
> but I hope rebasing won't be necessary, just give me some more time to
> look at the series.
Sure, I appreciate you are going over it.

> 
> Thanks a lot for your efforts already, it sounds very promising!
Thanks! 
Marcel

> 
> Cheers,
> Andreas
> 

Re: [Qemu-devel] [PATCH RFC V2 1/9] hw/core: introduced qemu machine as QOM object

[Andreas Färber]

Am 02.03.2014 14:07, schrieb Marcel Apfelbaum:
> The main functionality change is to convert QEMUMachine into QemuMachineClass
> and QEMUMachineInitArgs into QemuMachineState, instance of QemuMachineClass.

I wonder why go from QEMUMachine* to QemuMachine*? I don't spot a name
clash, and keeping QEMU like we use PCI or USB would make the correct
spelling clearer.

> 
> As a first step, in order to make possible an incremental developement,

"development"

> both QEMUMachine and QEMUMachineInitArgs are being embeded into the

"embedded"

> new types.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/core/Makefile.objs |  2 +-
>  hw/core/machine.c     | 38 +++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h   | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/machine.c
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 9e324be..f80c13c 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -11,4 +11,4 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> -
> +common-obj-$(CONFIG_SOFTMMU) += machine.o

Cleaning up that trailing white line is good. Might it make sense to
move the new line to above null-machine.o though?

> diff --git a/hw/core/machine.c b/hw/core/machine.c
> new file mode 100644
> index 0000000..2c6e1a3
> --- /dev/null
> +++ b/hw/core/machine.c
> @@ -0,0 +1,38 @@
> +/*
> + * QEMU Machine
> + *
> + * Copyright (C) 2013 Red Hat Inc

2014?

> + *
> + * Authors:
> + *   Marcel Apfelbaum <marcel.a@redhat.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.
> + */
> +
> +#include "hw/boards.h"
> +
> +static void qemu_machine_initfn(Object *obj)

My preference would be to just use machine_ prefix.

> +{
> +}
> +
> +static void qemu_machine_class_init(ObjectClass *oc, void *data)
> +{
> +}

No-op functions could be left out here and added once needed.

> +
> +static const TypeInfo qemu_machine_info = {
> +    .name = TYPE_QEMU_MACHINE,

TYPE_MACHINE?

> +    .parent = TYPE_OBJECT,
> +    .abstract = true,
> +    .class_size = sizeof(QemuMachineClass),
> +    .class_init = qemu_machine_class_init,
> +    .instance_size = sizeof(QemuMachineState),
> +    .instance_init = qemu_machine_initfn,
> +};
> +
> +static void register_types(void)

machine_register_types?

> +{
> +    type_register_static(&qemu_machine_info);
> +}
> +
> +type_init(register_types);

No semicolon needed.

> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2151460..7b4708d 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -5,6 +5,7 @@
>  
>  #include "sysemu/blockdev.h"
>  #include "hw/qdev.h"
> +#include "qom/object.h"
>  
>  typedef struct QEMUMachine QEMUMachine;
>  
> @@ -53,4 +54,55 @@ QEMUMachine *find_default_machine(void);
>  
>  extern QEMUMachine *current_machine;
>  
> +#define TYPE_QEMU_MACHINE "machine"
> +#define QEMU_MACHINE(obj) \
> +    OBJECT_CHECK(QemuMachineState, (obj), TYPE_QEMU_MACHINE)
> +#define QEMU_MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(QemuMachineClass, (obj), TYPE_QEMU_MACHINE)
> +#define QEMU_MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(QemuMachineClass, (klass), TYPE_QEMU_MACHINE)
> +
> +typedef struct QemuMachineState QemuMachineState;
> +typedef struct QemuMachineClass QemuMachineClass;
> +
> +/**
> + * @QemuMachineClass
> + *
> + * @parent_class: opaque parent class container
> + */
> +struct QemuMachineClass {

/*< private >*/

> +    ObjectClass parent_class;

/*< public >*/

> +
> +    QEMUMachine *qemu_machine;
> +};
> +
> +/**
> + * @QemuMachineState
> + *
> + * @parent: opaque parent object container
> + */
> +struct QemuMachineState {
> +    /* private */
> +    Object parent;
> +    /* public */

/*< ... >*/ is the gtk-doc syntax, and parent_obj please.

> +
> +

Double white line intentional?

> +    char *accel;
> +    bool kernel_irqchip;
> +    int kvm_shadow_mem;
> +    char *kernel;
> +    char *initrd;
> +    char *append;
> +    char *dtb;
> +    char *dumpdtb;
> +    int phandle_start;
> +    char *dt_compatible;
> +    bool dump_guest_core;
> +    bool mem_merge;
> +    bool usb;
> +    char *firmware;
> +
> +    QEMUMachineInitArgs init_args;
> +};
> +
>  #endif
> 

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Andreas Färber]

Am 02.03.2014 14:07, schrieb Marcel Apfelbaum:
> The machine registration flow is refactored to use the QOM functionality.
> Instead of linking the machines into a list, each machine has a type
> and the types can be traversed in the QOM way.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  include/hw/boards.h |  1 +
>  vl.c                | 75 ++++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 7b4708d..65e1e03 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -49,6 +49,7 @@ struct QEMUMachine {
>      const char *hw_version;
>  };
>  
> +#define TYPE_QEMU_MACHINE_PREFIX "machine-"

Would you mind turning that into a "-machine" suffix?

>  int qemu_register_machine(QEMUMachine *m);
>  QEMUMachine *find_default_machine(void);
>  
> diff --git a/vl.c b/vl.c
> index 9379d33..50c880f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1529,54 +1529,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
>  /***********************************************************/
>  /* machine registration */
>  
> -static QEMUMachine *first_machine = NULL;
>  QEMUMachine *current_machine = NULL;
>  
> +static void qemu_machine_class_init(ObjectClass *klass, void *data)

"oc" like in 1/9 please.

> +{
> +    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);

"mc" for [QEMU]MachineClass maybe? Applies elsewhere as well.

On that matter, might we simply choose MachineState and MachineClass, or
is there some name conflict?

> +
> +    k->qemu_machine = data;
> +}
> +
>  int qemu_register_machine(QEMUMachine *m)
>  {
> -    QEMUMachine **pm;
> -    pm = &first_machine;
> -    while (*pm != NULL)
> -        pm = &(*pm)->next;
> -    m->next = NULL;
> -    *pm = m;
> +    TypeInfo ti = {
> +        .name       = g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->name, NULL),
> +        .parent     = TYPE_QEMU_MACHINE,
> +        .class_init = qemu_machine_class_init,
> +        .class_data = (void *)m,
> +    };
> +
> +    type_register(&ti);

Cute idea as minimally invasive solution!

I do wonder whether doing type_register() as part of machine_init()
callbacks could pose any timing problems - have you checked on that in
vl.c? I.e. in the worst case we might need to sweep
s/machine_init/type_init/ for all machines in this early patch already
while still calling qemu_register_machine().

Otherwise the use of QOM infrastructure looks really nice.

Cheers,
Andreas

> +
>      return 0;
>  }
>  
>  static QEMUMachine *find_machine(const char *name)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
> +
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (!strcmp(m->name, name))
> -            return m;
> -        if (m->alias && !strcmp(m->alias, name))
> -            return m;
> +        if (!strcmp(k->qemu_machine->name, name)) {
> +            m = k->qemu_machine;
> +            break;
> +        }
> +        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> +            m = k->qemu_machine;
> +            break;
> +        }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  QEMUMachine *find_default_machine(void)
>  {
> -    QEMUMachine *m;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> +    QEMUMachine *m = NULL;
>  
> -    for(m = first_machine; m != NULL; m = m->next) {
> -        if (m->is_default) {
> -            return m;
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +
> +        if (k->qemu_machine->is_default) {
> +            m = k->qemu_machine;
> +            break;
>          }
>      }
> -    return NULL;
> +
> +    g_slist_free(machines);
> +    return m;
>  }
>  
>  MachineInfoList *qmp_query_machines(Error **errp)
>  {
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>      MachineInfoList *mach_list = NULL;
>      QEMUMachine *m;
>  
> -    for (m = first_machine; m; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
>          MachineInfoList *entry;
>          MachineInfo *info;
>  
> +        m = k->qemu_machine;
>          info = g_malloc0(sizeof(*info));
>          if (m->is_default) {
>              info->has_is_default = true;
> @@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
>          mach_list = entry;
>      }
>  
> +    g_slist_free(machines);
>      return mach_list;
>  }
>  
> @@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname)
>  static QEMUMachine *machine_parse(const char *name)
>  {
>      QEMUMachine *m, *machine = NULL;
> +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
>  
>      if (name) {
>          machine = find_machine(name);
> @@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char *name)
>          return machine;
>      }
>      printf("Supported machines are:\n");
> -    for (m = first_machine; m != NULL; m = m->next) {
> +    for (el = machines; el; el = el->next) {
> +        QemuMachineClass *k = el->data;
> +        m = k->qemu_machine;
>          if (m->alias) {
>              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
>          }
>          printf("%-20s %s%s\n", m->name, m->desc,
>                 m->is_default ? " (default)" : "");
>      }
> +
> +    g_slist_free(machines);
>      exit(!name || !is_help_option(name));
>  }
>  

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

Re: [Qemu-devel] [PATCH RFC V2 1/9] hw/core: introduced qemu machine as QOM object

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 18:49 +0100, Andreas Färber wrote:
> Am 02.03.2014 14:07, schrieb Marcel Apfelbaum:
> > The main functionality change is to convert QEMUMachine into QemuMachineClass
> > and QEMUMachineInitArgs into QemuMachineState, instance of QemuMachineClass.
> 
> I wonder why go from QEMUMachine* to QemuMachine*? I don't spot a name
> clash, and keeping QEMU like we use PCI or USB would make the correct
> spelling clearer.
Well, it was mostly a "conservative" choice, but MachineClass/MachineState
sounds great to me, I am going to replace them, thanks!
> 
> > 
> > As a first step, in order to make possible an incremental developement,
> 
> "development"
Oops
> 
> > both QEMUMachine and QEMUMachineInitArgs are being embeded into the
> 
> "embedded"
Another oops, I usually run a spelling cycle before sending,
I'll be more careful, thanks.
 
> 
> > new types.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  hw/core/Makefile.objs |  2 +-
> >  hw/core/machine.c     | 38 +++++++++++++++++++++++++++++++++++++
> >  include/hw/boards.h   | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/core/machine.c
> > 
> > diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> > index 9e324be..f80c13c 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -11,4 +11,4 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> >  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> >  common-obj-$(CONFIG_SOFTMMU) += loader.o
> >  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > -
> > +common-obj-$(CONFIG_SOFTMMU) += machine.o
> 
> Cleaning up that trailing white line is good. Might it make sense to
> move the new line to above null-machine.o though?
> 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > new file mode 100644
> > index 0000000..2c6e1a3
> > --- /dev/null
> > +++ b/hw/core/machine.c
> > @@ -0,0 +1,38 @@
> > +/*
> > + * QEMU Machine
> > + *
> > + * Copyright (C) 2013 Red Hat Inc
> 
> 2014?
Yes it is :) , maybe I started a draft on December? I'll update, of course.

> 
> > + *
> > + * Authors:
> > + *   Marcel Apfelbaum <marcel.a@redhat.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.
> > + */
> > +
> > +#include "hw/boards.h"
> > +
> > +static void qemu_machine_initfn(Object *obj)
> 
> My preference would be to just use machine_ prefix.
Removing the "qemu" when not necessary, sure.

> 
> > +{
> > +}
> > +
> > +static void qemu_machine_class_init(ObjectClass *oc, void *data)
> > +{
> > +}
> 
> No-op functions could be left out here and added once needed.
Go it.
> 
> > +
> > +static const TypeInfo qemu_machine_info = {
> > +    .name = TYPE_QEMU_MACHINE,
> 
> TYPE_MACHINE?
Sure, I'll replace.

> 
> > +    .parent = TYPE_OBJECT,
> > +    .abstract = true,
> > +    .class_size = sizeof(QemuMachineClass),
> > +    .class_init = qemu_machine_class_init,
> > +    .instance_size = sizeof(QemuMachineState),
> > +    .instance_init = qemu_machine_initfn,
> > +};
> > +
> > +static void register_types(void)
> 
> machine_register_types?
OK
> 
> > +{
> > +    type_register_static(&qemu_machine_info);
> > +}
> > +
> > +type_init(register_types);
> 
> No semicolon needed.
OK

> 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 2151460..7b4708d 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -5,6 +5,7 @@
> >  
> >  #include "sysemu/blockdev.h"
> >  #include "hw/qdev.h"
> > +#include "qom/object.h"
> >  
> >  typedef struct QEMUMachine QEMUMachine;
> >  
> > @@ -53,4 +54,55 @@ QEMUMachine *find_default_machine(void);
> >  
> >  extern QEMUMachine *current_machine;
> >  
> > +#define TYPE_QEMU_MACHINE "machine"
> > +#define QEMU_MACHINE(obj) \
> > +    OBJECT_CHECK(QemuMachineState, (obj), TYPE_QEMU_MACHINE)
> > +#define QEMU_MACHINE_GET_CLASS(obj) \
> > +    OBJECT_GET_CLASS(QemuMachineClass, (obj), TYPE_QEMU_MACHINE)
> > +#define QEMU_MACHINE_CLASS(klass) \
> > +    OBJECT_CLASS_CHECK(QemuMachineClass, (klass), TYPE_QEMU_MACHINE)
> > +
> > +typedef struct QemuMachineState QemuMachineState;
> > +typedef struct QemuMachineClass QemuMachineClass;
> > +
> > +/**
> > + * @QemuMachineClass
> > + *
> > + * @parent_class: opaque parent class container
> > + */
> > +struct QemuMachineClass {
> 
> /*< private >*/
> 
> > +    ObjectClass parent_class;
> 
> /*< public >*/
OK

> 
> > +
> > +    QEMUMachine *qemu_machine;
> > +};
> > +
> > +/**
> > + * @QemuMachineState
> > + *
> > + * @parent: opaque parent object container
> > + */
> > +struct QemuMachineState {
> > +    /* private */
> > +    Object parent;
> > +    /* public */
> 
> /*< ... >*/ is the gtk-doc syntax, and parent_obj please.
Sure, I've kind of took this from another QOM object, picked 
the wrong one to follow, I'll change, thanks for the tip.
> 
> > +
> > +
> 
> Double white line intentional?
Yes, prefer one? I saw this double line in a few places.

> 
> > +    char *accel;
> > +    bool kernel_irqchip;
> > +    int kvm_shadow_mem;
> > +    char *kernel;
> > +    char *initrd;
> > +    char *append;
> > +    char *dtb;
> > +    char *dumpdtb;
> > +    int phandle_start;
> > +    char *dt_compatible;
> > +    bool dump_guest_core;
> > +    bool mem_merge;
> > +    bool usb;
> > +    char *firmware;
> > +
> > +    QEMUMachineInitArgs init_args;
> > +};
> > +
> >  #endif
> > 
> 
Thanks for the thorough review!
Marcel

> Regards,
> Andreas
> 

Re: [Qemu-devel] [PATCH RFC V2 2/9] vl: use qemu machine QOM class instead of global machines list

[Marcel Apfelbaum]

On Mon, 2014-03-03 at 19:12 +0100, Andreas Färber wrote:
> Am 02.03.2014 14:07, schrieb Marcel Apfelbaum:
> > The machine registration flow is refactored to use the QOM functionality.
> > Instead of linking the machines into a list, each machine has a type
> > and the types can be traversed in the QOM way.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  include/hw/boards.h |  1 +
> >  vl.c                | 75 ++++++++++++++++++++++++++++++++++++++---------------
> >  2 files changed, 55 insertions(+), 21 deletions(-)
> > 
> > diff --git a/include/hw/boards.h b/include/hw/boards.h
> > index 7b4708d..65e1e03 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -49,6 +49,7 @@ struct QEMUMachine {
> >      const char *hw_version;
> >  };
> >  
> > +#define TYPE_QEMU_MACHINE_PREFIX "machine-"
> 
> Would you mind turning that into a "-machine" suffix?
Not at all

> 
> >  int qemu_register_machine(QEMUMachine *m);
> >  QEMUMachine *find_default_machine(void);
> >  
> > diff --git a/vl.c b/vl.c
> > index 9379d33..50c880f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1529,54 +1529,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
> >  /***********************************************************/
> >  /* machine registration */
> >  
> > -static QEMUMachine *first_machine = NULL;
> >  QEMUMachine *current_machine = NULL;
> >  
> > +static void qemu_machine_class_init(ObjectClass *klass, void *data)
> 
> "oc" like in 1/9 please.
Sure
> 
> > +{
> > +    QemuMachineClass *k = QEMU_MACHINE_CLASS(klass);
> 
> "mc" for [QEMU]MachineClass maybe? Applies elsewhere as well.
As above, I couldn't follow a unified naming scheme,
your comment really help, thanks!

> 
> On that matter, might we simply choose MachineState and MachineClass, or
> is there some name conflict?
As stated in your 1/9 patch review, I find your choice great,
indeed, there is no need for the "Qemu" prefix. It mostly helped *me*
to associate old-new names, but there is no naming conflict, I'll change.

> 
> > +
> > +    k->qemu_machine = data;
> > +}
> > +
> >  int qemu_register_machine(QEMUMachine *m)
> >  {
> > -    QEMUMachine **pm;
> > -    pm = &first_machine;
> > -    while (*pm != NULL)
> > -        pm = &(*pm)->next;
> > -    m->next = NULL;
> > -    *pm = m;
> > +    TypeInfo ti = {
> > +        .name       = g_strconcat(TYPE_QEMU_MACHINE_PREFIX, m->name, NULL),
> > +        .parent     = TYPE_QEMU_MACHINE,
> > +        .class_init = qemu_machine_class_init,
> > +        .class_data = (void *)m,
> > +    };
> > +
> > +    type_register(&ti);
> 
> Cute idea as minimally invasive solution!
Thanks!

> 
> I do wonder whether doing type_register() as part of machine_init()
> callbacks could pose any timing problems - have you checked on that in
> vl.c? I.e. in the worst case we might need to sweep
> s/machine_init/type_init/ for all machines in this early patch already
> while still calling qemu_register_machine().
I encountered no problems here (meaning it works), at least I see no race:
The [Qemu]MachineClasses are created dynamically after
'module_call_init(MODULE_INIT_QOM)' is called with no connection with what happened
there, beside the creation of the ObjectClass of course, which we can count it (can we?).
Other than that, we can say that a side effect of 'module_call_init(MODULE_INIT_MACHINE)'
is the creation of the new Machine QOM classes.

Please let me know if I am missing something, in that case I'll switch as you suggested.
For the moment is how I see things: MODULE_INIT_QOM should be before MODULE_INIT_MACHINE.
 

> Otherwise the use of QOM infrastructure looks really nice.
Thanks!

If I am not exaggerating, can you please go over the patch 3/9 ?
([PATCH RFC V2 3/9] hw/boards: converted current_machine to be an instance of QemuMachineCLass)

I have a minor change to do there (add the machine to the QOM tree instead
of the /machine container) and combined with the first two, they can be a
nice feature start :) - and let me concentrate on the next phase.

Thanks again or your time!
Marcel



> 
> Cheers,
> Andreas
> 
> > +
> >      return 0;
> >  }
> >  
> >  static QEMUMachine *find_machine(const char *name)
> >  {
> > -    QEMUMachine *m;
> > +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > +    QEMUMachine *m = NULL;
> > +
> > +    for (el = machines; el; el = el->next) {
> > +        QemuMachineClass *k = el->data;
> >  
> > -    for(m = first_machine; m != NULL; m = m->next) {
> > -        if (!strcmp(m->name, name))
> > -            return m;
> > -        if (m->alias && !strcmp(m->alias, name))
> > -            return m;
> > +        if (!strcmp(k->qemu_machine->name, name)) {
> > +            m = k->qemu_machine;
> > +            break;
> > +        }
> > +        if (k->qemu_machine->alias && !strcmp(k->qemu_machine->alias, name)) {
> > +            m = k->qemu_machine;
> > +            break;
> > +        }
> >      }
> > -    return NULL;
> > +
> > +    g_slist_free(machines);
> > +    return m;
> >  }
> >  
> >  QEMUMachine *find_default_machine(void)
> >  {
> > -    QEMUMachine *m;
> > +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> > +    QEMUMachine *m = NULL;
> >  
> > -    for(m = first_machine; m != NULL; m = m->next) {
> > -        if (m->is_default) {
> > -            return m;
> > +    for (el = machines; el; el = el->next) {
> > +        QemuMachineClass *k = el->data;
> > +
> > +        if (k->qemu_machine->is_default) {
> > +            m = k->qemu_machine;
> > +            break;
> >          }
> >      }
> > -    return NULL;
> > +
> > +    g_slist_free(machines);
> > +    return m;
> >  }
> >  
> >  MachineInfoList *qmp_query_machines(Error **errp)
> >  {
> > +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> >      MachineInfoList *mach_list = NULL;
> >      QEMUMachine *m;
> >  
> > -    for (m = first_machine; m; m = m->next) {
> > +    for (el = machines; el; el = el->next) {
> > +        QemuMachineClass *k = el->data;
> >          MachineInfoList *entry;
> >          MachineInfo *info;
> >  
> > +        m = k->qemu_machine;
> >          info = g_malloc0(sizeof(*info));
> >          if (m->is_default) {
> >              info->has_is_default = true;
> > @@ -1597,6 +1624,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
> >          mach_list = entry;
> >      }
> >  
> > +    g_slist_free(machines);
> >      return mach_list;
> >  }
> >  
> > @@ -2540,6 +2568,7 @@ static int debugcon_parse(const char *devname)
> >  static QEMUMachine *machine_parse(const char *name)
> >  {
> >      QEMUMachine *m, *machine = NULL;
> > +    GSList *el, *machines = object_class_get_list(TYPE_QEMU_MACHINE, false);
> >  
> >      if (name) {
> >          machine = find_machine(name);
> > @@ -2548,13 +2577,17 @@ static QEMUMachine *machine_parse(const char *name)
> >          return machine;
> >      }
> >      printf("Supported machines are:\n");
> > -    for (m = first_machine; m != NULL; m = m->next) {
> > +    for (el = machines; el; el = el->next) {
> > +        QemuMachineClass *k = el->data;
> > +        m = k->qemu_machine;
> >          if (m->alias) {
> >              printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
> >          }
> >          printf("%-20s %s%s\n", m->name, m->desc,
> >                 m->is_default ? " (default)" : "");
> >      }
> > +
> > +    g_slist_free(machines);
> >      exit(!name || !is_help_option(name));
> >  }
> >  
>