[Qemu-devel] [PATCH v3 0/3] qemu-machine as a QOM object

[Qemu-devel] [PATCH v3 0/3] qemu-machine as a QOM object

[Marcel Apfelbaum]

RFC V2 -> V3
    Split the series into two parts, because the first part
    can be submitted as was already accepted (after this review).
    - The second part involves the replacement of machine opts
      queries with QOM queries and I will resend it separately.
    Addressed Andreas Färber's comments:
    - Dropped the Qemu prefix everywhere
    - Coding style issues (I really hope I got them all!)
    - machine class:' -machine' sufix instead of 'machine-' prefix
    - Regarding replacement of the init order machine_init/type_init:
      - I answered on the mail thread that I saw no issues.
      - I also stated there that I think that type_init should be before
        machine_init because the later is using the QOM subsystem.
   Addressed Paolo Bonzini's comments:
   - Added the current_machine to the QOM tree as '/machine'.
   - Passed current_machine->init_args to machine->init.
   - I will continue the series following his suggestions (Thanks!).

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 -> MachineClass
    QEMUMachineInitArgs -> MachineState.
As a first step, in order to make possible an incremental development,
both QEMUMachine and QEMUMachineInitArgs are being embedded into the
new types.


Marcel Apfelbaum (3):
  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 MachineCLass

 device-hotplug.c      |   4 +-
 hw/core/Makefile.objs |   2 +-
 hw/core/machine.c     |  28 ++++++++++++
 include/hw/boards.h   |  58 +++++++++++++++++++++++-
 qmp.c                 |   7 ++-
 vl.c                  | 121 ++++++++++++++++++++++++++++++++++----------------
 6 files changed, 176 insertions(+), 44 deletions(-)
 create mode 100644 hw/core/machine.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] hw/core: introduced qemu machine as QOM object

[Marcel Apfelbaum]

The main functionality change is to convert QEMUMachine into MachineClass
and QEMUMachineInitArgs into MachineState, instance of MachineClass.

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

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 hw/core/Makefile.objs |  2 +-
 hw/core/machine.c     | 28 +++++++++++++++++++++++++++
 include/hw/boards.h   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+), 1 deletion(-)
 create mode 100644 hw/core/machine.c

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 9e324be..981593c 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -8,7 +8,7 @@ common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 common-obj-$(CONFIG_XILINX_AXI) += stream.o
 common-obj-$(CONFIG_PTIMER) += ptimer.o
 common-obj-$(CONFIG_SOFTMMU) += sysbus.o
+common-obj-$(CONFIG_SOFTMMU) += machine.o
 common-obj-$(CONFIG_SOFTMMU) += null-machine.o
 common-obj-$(CONFIG_SOFTMMU) += loader.o
 common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
-
diff --git a/hw/core/machine.c b/hw/core/machine.c
new file mode 100644
index 0000000..d3ffef7
--- /dev/null
+++ b/hw/core/machine.c
@@ -0,0 +1,28 @@
+/*
+ * QEMU Machine
+ *
+ * Copyright (C) 2014 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 const TypeInfo machine_info = {
+    .name = TYPE_MACHINE,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(MachineClass),
+    .instance_size = sizeof(MachineState),
+};
+
+static void machine_register_types(void)
+{
+    type_register_static(&machine_info);
+}
+
+type_init(machine_register_types)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2151460..15df78e 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,56 @@ QEMUMachine *find_default_machine(void);
 
 extern QEMUMachine *current_machine;
 
+#define TYPE_MACHINE "machine"
+#define MACHINE(obj) \
+    OBJECT_CHECK(MachineState, (obj), TYPE_MACHINE)
+#define MACHINE_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MachineClass, (obj), TYPE_MACHINE)
+#define MACHINE_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
+
+typedef struct MachineState MachineState;
+typedef struct MachineClass MachineClass;
+
+/**
+ * @MachineClass
+ *
+ * @parent_class: opaque parent class container
+ */
+struct MachineClass {
+    /*< private >*/
+    ObjectClass parent_class;
+    /*< public >*/
+
+    QEMUMachine *qemu_machine;
+};
+
+/**
+ * @MachineState
+ *
+ * @parent_obj: opaque parent object container
+ */
+struct MachineState {
+    /*< private >*/
+    Object parent_obj;
+    /*< 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 v3 2/3] 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.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
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 15df78e..7c93384 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -49,6 +49,7 @@ struct QEMUMachine {
     const char *hw_version;
 };
 
+#define TYPE_MACHINE_SUFFIX "-machine"
 int qemu_register_machine(QEMUMachine *m);
 QEMUMachine *find_default_machine(void);
 
diff --git a/vl.c b/vl.c
index 685a7a9..9fff2eb 100644
--- a/vl.c
+++ b/vl.c
@@ -1525,54 +1525,81 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
 /***********************************************************/
 /* machine registration */
 
-static QEMUMachine *first_machine = NULL;
 QEMUMachine *current_machine = NULL;
 
+static void machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->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(m->name, TYPE_MACHINE_SUFFIX, NULL),
+        .parent     = TYPE_MACHINE,
+        .class_init = 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_MACHINE, false);
+    QEMUMachine *m = NULL;
+
+    for (el = machines; el; el = el->next) {
+        MachineClass *mc = 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(mc->qemu_machine->name, name)) {
+            m = mc->qemu_machine;
+            break;
+        }
+        if (mc->qemu_machine->alias && !strcmp(mc->qemu_machine->alias, name)) {
+            m = mc->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_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) {
+        MachineClass *mc = el->data;
+
+        if (mc->qemu_machine->is_default) {
+            m = mc->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_MACHINE, false);
     MachineInfoList *mach_list = NULL;
     QEMUMachine *m;
 
-    for (m = first_machine; m; m = m->next) {
+    for (el = machines; el; el = el->next) {
+        MachineClass *mc = el->data;
         MachineInfoList *entry;
         MachineInfo *info;
 
+        m = mc->qemu_machine;
         info = g_malloc0(sizeof(*info));
         if (m->is_default) {
             info->has_is_default = true;
@@ -1593,6 +1620,7 @@ MachineInfoList *qmp_query_machines(Error **errp)
         mach_list = entry;
     }
 
+    g_slist_free(machines);
     return mach_list;
 }
 
@@ -2560,6 +2588,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_MACHINE, false);
 
     if (name) {
         machine = find_machine(name);
@@ -2568,13 +2597,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) {
+        MachineClass *mc = el->data;
+        m = mc->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 v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

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

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

diff --git a/device-hotplug.c b/device-hotplug.c
index 103d34a..ebfa6b1 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -33,12 +33,14 @@ DriveInfo *add_init_drive(const char *optstr)
 {
     DriveInfo *dinfo;
     QemuOpts *opts;
+    MachineClass *mc;
 
     opts = drive_def(optstr);
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->block_default_type);
+    mc = MACHINE_GET_CLASS(current_machine);
+    dinfo = drive_init(opts, mc->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 7c93384..33debc2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -51,9 +51,6 @@ struct QEMUMachine {
 
 #define TYPE_MACHINE_SUFFIX "-machine"
 int qemu_register_machine(QEMUMachine *m);
-QEMUMachine *find_default_machine(void);
-
-extern QEMUMachine *current_machine;
 
 #define TYPE_MACHINE "machine"
 #define MACHINE(obj) \
@@ -66,6 +63,9 @@ extern QEMUMachine *current_machine;
 typedef struct MachineState MachineState;
 typedef struct MachineClass MachineClass;
 
+MachineClass *find_default_machine(void);
+extern MachineState *current_machine;
+
 /**
  * @MachineClass
  *
diff --git a/qmp.c b/qmp.c
index f556a04..87a28f7 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);
+    MachineClass *mc;
+
+    mc = MACHINE_GET_CLASS(current_machine);
+    if (mc->qemu_machine->hot_add_cpu) {
+        mc->qemu_machine->hot_add_cpu(id, errp);
     } else {
         error_setg(errp, "Not supported");
     }
diff --git a/vl.c b/vl.c
index 9fff2eb..ed1bb19 100644
--- a/vl.c
+++ b/vl.c
@@ -1525,7 +1525,7 @@ void pcmcia_info(Monitor *mon, const QDict *qdict)
 /***********************************************************/
 /* machine registration */
 
-QEMUMachine *current_machine = NULL;
+MachineState *current_machine;
 
 static void machine_class_init(ObjectClass *oc, void *data)
 {
@@ -1548,44 +1548,45 @@ int qemu_register_machine(QEMUMachine *m)
     return 0;
 }
 
-static QEMUMachine *find_machine(const char *name)
+static MachineClass *find_machine(const char *name)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
-    QEMUMachine *m = NULL;
+    MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
-        MachineClass *mc = el->data;
+        MachineClass *temp = el->data;
 
-        if (!strcmp(mc->qemu_machine->name, name)) {
-            m = mc->qemu_machine;
+        if (!strcmp(temp->qemu_machine->name, name)) {
+            mc = temp;
             break;
         }
-        if (mc->qemu_machine->alias && !strcmp(mc->qemu_machine->alias, name)) {
-            m = mc->qemu_machine;
+        if (temp->qemu_machine->alias &&
+            !strcmp(temp->qemu_machine->alias, name)) {
+            mc = temp;
             break;
         }
     }
 
     g_slist_free(machines);
-    return m;
+    return mc;
 }
 
-QEMUMachine *find_default_machine(void)
+MachineClass *find_default_machine(void)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
-    QEMUMachine *m = NULL;
+    MachineClass *mc = NULL;
 
     for (el = machines; el; el = el->next) {
-        MachineClass *mc = el->data;
+        MachineClass *temp = el->data;
 
-        if (mc->qemu_machine->is_default) {
-            m = mc->qemu_machine;
+        if (temp->qemu_machine->is_default) {
+            mc = temp;
             break;
         }
     }
 
     g_slist_free(machines);
-    return m;
+    return mc;
 }
 
 MachineInfoList *qmp_query_machines(Error **errp)
@@ -1814,8 +1815,12 @@ void qemu_devices_reset(void)
 
 void qemu_system_reset(bool report)
 {
-    if (current_machine && current_machine->reset) {
-        current_machine->reset();
+    MachineClass *mc;
+
+    mc = current_machine ? MACHINE_GET_CLASS(current_machine) : NULL;
+
+    if (mc && mc->qemu_machine->reset) {
+        mc->qemu_machine->reset();
     } else {
         qemu_devices_reset();
     }
@@ -2585,21 +2590,21 @@ static int debugcon_parse(const char *devname)
     return 0;
 }
 
-static QEMUMachine *machine_parse(const char *name)
+static MachineClass *machine_parse(const char *name)
 {
-    QEMUMachine *m, *machine = NULL;
+    MachineClass *mc = NULL;
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
 
     if (name) {
-        machine = find_machine(name);
+        mc = find_machine(name);
     }
-    if (machine) {
-        return machine;
+    if (mc) {
+        return mc;
     }
     printf("Supported machines are:\n");
     for (el = machines; el; el = el->next) {
         MachineClass *mc = el->data;
-        m = mc->qemu_machine;
+        QEMUMachine *m = mc->qemu_machine;
         if (m->alias) {
             printf("%-20s %s (alias of %s)\n", m->alias, m->desc, m->name);
         }
@@ -2856,6 +2861,7 @@ int main(int argc, char **argv, char **envp)
     int optind;
     const char *optarg;
     const char *loadvm = NULL;
+    MachineClass *machine_class;
     QEMUMachine *machine;
     const char *cpu_model;
     const char *vga_model = "none";
@@ -2929,7 +2935,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;
@@ -2995,7 +3001,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");
@@ -3551,7 +3557,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:
@@ -3865,11 +3871,17 @@ 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 = MACHINE(object_new(object_class_get_name(
+                          OBJECT_CLASS(machine_class))));
+    object_property_add_child(object_get_root(), "machine",
+                              OBJECT(current_machine), &error_abort);
+
+    machine = machine_class->qemu_machine;
     if (machine->hw_version) {
         qemu_set_version(machine->hw_version);
     }
@@ -4298,7 +4310,9 @@ int main(int argc, char **argv, char **envp)
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
                                  .cpu_model = cpu_model };
-    machine->init(&args);
+
+    current_machine->init_args = args;
+    machine->init(&current_machine->init_args);
 
     audio_init();
 
@@ -4306,8 +4320,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

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Eric Blake]

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

On 03/05/2014 10:30 AM, Marcel Apfelbaum wrote:
> In order to allow attaching machine options to a machine instance,
> current_machine is converted into MachineState.
> As a first step of deprecating QEMUMachine, some of the functions
> were modified to return MachineCLass.

s/MachineCLass/MachineClass/ here and in subject

> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---

I'll leave the actual review to others, just pointing out the typo.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Wed, 2014-03-05 at 10:36 -0700, Eric Blake wrote:
> On 03/05/2014 10:30 AM, Marcel Apfelbaum wrote:
> > In order to allow attaching machine options to a machine instance,
> > current_machine is converted into MachineState.
> > As a first step of deprecating QEMUMachine, some of the functions
> > were modified to return MachineCLass.
> 
> s/MachineCLass/MachineClass/ here and in subject
Thanks, I'll take care of it
Marcel

> 
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> 
> I'll leave the actual review to others, just pointing out the typo.
> 

Re: [Qemu-devel] [PATCH v3 1/3] hw/core: introduced qemu machine as QOM object

[Andreas Färber]

Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> The main functionality change is to convert QEMUMachine into MachineClass
> and QEMUMachineInitArgs into MachineState, instance of MachineClass.
> 
> As a first step, in order to make possible an incremental development,
> both QEMUMachine and QEMUMachineInitArgs are being embedded into the
> new types.
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  hw/core/Makefile.objs |  2 +-
>  hw/core/machine.c     | 28 +++++++++++++++++++++++++++
>  include/hw/boards.h   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 1 deletion(-)
>  create mode 100644 hw/core/machine.c
> 
> diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
> index 9e324be..981593c 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -8,7 +8,7 @@ common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
>  common-obj-$(CONFIG_XILINX_AXI) += stream.o
>  common-obj-$(CONFIG_PTIMER) += ptimer.o
>  common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> +common-obj-$(CONFIG_SOFTMMU) += machine.o
>  common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>  common-obj-$(CONFIG_SOFTMMU) += loader.o
>  common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> -
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> new file mode 100644
> index 0000000..d3ffef7
> --- /dev/null
> +++ b/hw/core/machine.c
> @@ -0,0 +1,28 @@
> +/*
> + * QEMU Machine
> + *
> + * Copyright (C) 2014 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 const TypeInfo machine_info = {
> +    .name = TYPE_MACHINE,
> +    .parent = TYPE_OBJECT,
> +    .abstract = true,
> +    .class_size = sizeof(MachineClass),
> +    .instance_size = sizeof(MachineState),
> +};
> +
> +static void machine_register_types(void)
> +{
> +    type_register_static(&machine_info);
> +}
> +
> +type_init(machine_register_types)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2151460..15df78e 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,56 @@ QEMUMachine *find_default_machine(void);
>  
>  extern QEMUMachine *current_machine;
>  
> +#define TYPE_MACHINE "machine"
> +#define MACHINE(obj) \
> +    OBJECT_CHECK(MachineState, (obj), TYPE_MACHINE)
> +#define MACHINE_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(MachineClass, (obj), TYPE_MACHINE)
> +#define MACHINE_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
> +
> +typedef struct MachineState MachineState;
> +typedef struct MachineClass MachineClass;
> +
> +/**
> + * @MachineClass

"MachineClass:"

> + *
> + * @parent_class: opaque parent class container
> + */
> +struct MachineClass {
> +    /*< private >*/
> +    ObjectClass parent_class;

The very purpose of < private > is to suppress this field from
documentation, so you don't need to document it.

> +    /*< public >*/
> +
> +    QEMUMachine *qemu_machine;
> +};
> +
> +/**
> + * @MachineState
> + *
> + * @parent_obj: opaque parent object container
> + */
> +struct MachineState {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< 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

Thanks, applied to qom-next with gtk-doc changes:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 15df78e..cb94db1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -66,9 +66,8 @@ typedef struct MachineState MachineState;
 typedef struct MachineClass MachineClass;

 /**
- * @MachineClass
- *
- * @parent_class: opaque parent class container
+ * MachineClass:
+ * @qemu_machine: #QEMUMachine
  */
 struct MachineClass {
     /*< private >*/
@@ -79,9 +78,7 @@ struct MachineClass {
 };

 /**
- * @MachineState
- *
- * @parent_obj: opaque parent object container
+ * MachineState:
  */
 struct MachineState {
     /*< private >*/



-- 
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 v3 2/3] vl: use qemu machine QOM class instead of global machines list

[Andreas Färber]

Am 05.03.2014 18:30, 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.
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Thanks, applied to qom-next:
https://github.com/afaerber/qemu-cpu/commits/qom-next

Andreas

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

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Andreas Färber]

Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> In order to allow attaching machine options to a machine instance,
> current_machine is converted into MachineState.
> As a first step of deprecating QEMUMachine, some of the functions
> were modified to return MachineCLass.
> 
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>

Looks mostly good, but same issue as Alexey's patch: We are risking
qdev_get_machine() creating a Container-typed /machine node.

What about the following on top?

Alexey, if we reach agreement here, this means for you that we can just
use type_register_static() in place of qemu_machine_register() to
register your custom machine type with interface added.

Regards,
Andreas

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index b6deebd..749c83a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -861,7 +861,8 @@ Object *qdev_get_machine(void)
     static Object *dev;

     if (dev == NULL) {
-        dev = container_get(object_get_root(), "/machine");
+        dev = object_resolve_path("/machine", NULL);
+        g_assert(dev);
     }

     return dev;


-- 
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 v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Alexey Kardashevskiy]

On 03/07/2014 10:44 AM, Andreas Färber wrote:
> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
>> In order to allow attaching machine options to a machine instance,
>> current_machine is converted into MachineState.
>> As a first step of deprecating QEMUMachine, some of the functions
>> were modified to return MachineCLass.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> Looks mostly good, but same issue as Alexey's patch: We are risking
> qdev_get_machine() creating a Container-typed /machine node.

Sorry, I am not following you here. object_resolve_path() can create objects?


> What about the following on top?
> 
> Alexey, if we reach agreement here, this means for you that we can just
> use type_register_static() in place of qemu_machine_register() to
> register your custom machine type with interface added.

I am perfectly fine with that, I just do not see what difference does it
make and why do you still keep qemu_machine_register() (or this is in the
plan already?)?



> Regards,
> Andreas
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b6deebd..749c83a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -861,7 +861,8 @@ Object *qdev_get_machine(void)
>      static Object *dev;
> 
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = object_resolve_path("/machine", NULL);
> +        g_assert(dev);
>      }
> 
>      return dev;
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> > In order to allow attaching machine options to a machine instance,
> > current_machine is converted into MachineState.
> > As a first step of deprecating QEMUMachine, some of the functions
> > were modified to return MachineCLass.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> 
> Looks mostly good, but same issue as Alexey's patch: We are risking
> qdev_get_machine() creating a Container-typed /machine node.
> 
> What about the following on top?
Hi Andreas,

I checked with the debugger and qdev_get_machine is called
long after we add the machine to the QOM tree.
However, the race still exists as someone can call qdev_get_machine
before the machine is added to the tree, not being aware of that.

Your change solves the problem, thank you!
Do you want me to add this diff and resend,
or I will send yours separately?

Thanks,
Marcel

> 
> Alexey, if we reach agreement here, this means for you that we can just
> use type_register_static() in place of qemu_machine_register() to
> register your custom machine type with interface added.
> 
> Regards,
> Andreas
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b6deebd..749c83a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -861,7 +861,8 @@ Object *qdev_get_machine(void)
>      static Object *dev;
> 
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = object_resolve_path("/machine", NULL);
> +        g_assert(dev);
>      }
> 
>      return dev;
> 
> 

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Fri, 2014-03-07 at 12:16 +1100, Alexey Kardashevskiy wrote:
> On 03/07/2014 10:44 AM, Andreas Färber wrote:
> > Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> >> In order to allow attaching machine options to a machine instance,
> >> current_machine is converted into MachineState.
> >> As a first step of deprecating QEMUMachine, some of the functions
> >> were modified to return MachineCLass.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > 
> > Looks mostly good, but same issue as Alexey's patch: We are risking
> > qdev_get_machine() creating a Container-typed /machine node.
> 
> Sorry, I am not following you here. object_resolve_path() can create objects?
Hi Alexey,
No, object_resolve_path() does not create objects, the point is that the machine
is created before, you only need to get it. You don't want to risk creating
a container. 

> 
> 
> > What about the following on top?
> > 
> > Alexey, if we reach agreement here, this means for you that we can just
> > use type_register_static() in place of qemu_machine_register() to
> > register your custom machine type with interface added.
> 
> I am perfectly fine with that, I just do not see what difference does it
> make and why do you still keep qemu_machine_register() (or this is in the
> plan already?)?
It is in my plan to eliminate qemu_machine_register(), however it will
take some time as it includes making changes to lots of files.
For the moment, subclassing MachineClass and registering it to QOM
will be exactly like calling qemu_machine_register().

Thanks,
Marcel

> 
> 
> 
> > Regards,
> > Andreas
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index b6deebd..749c83a 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -861,7 +861,8 @@ Object *qdev_get_machine(void)
> >      static Object *dev;
> > 
> >      if (dev == NULL) {
> > -        dev = container_get(object_get_root(), "/machine");
> > +        dev = object_resolve_path("/machine", NULL);
> > +        g_assert(dev);
> >      }
> > 
> >      return dev;
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Paolo Bonzini]

Il 07/03/2014 00:44, Andreas Färber ha scritto:
> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
>> In order to allow attaching machine options to a machine instance,
>> current_machine is converted into MachineState.
>> As a first step of deprecating QEMUMachine, some of the functions
>> were modified to return MachineCLass.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>
> Looks mostly good, but same issue as Alexey's patch: We are risking
> qdev_get_machine() creating a Container-typed /machine node.
>
> What about the following on top?

Good idea!  The smallest patches are (almost) always the best. :)

Paolo

> Alexey, if we reach agreement here, this means for you that we can just
> use type_register_static() in place of qemu_machine_register() to
> register your custom machine type with interface added.
>
> Regards,
> Andreas
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b6deebd..749c83a 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -861,7 +861,8 @@ Object *qdev_get_machine(void)
>      static Object *dev;
>
>      if (dev == NULL) {
> -        dev = container_get(object_get_root(), "/machine");
> +        dev = object_resolve_path("/machine", NULL);
> +        g_assert(dev);
>      }
>
>      return dev;
>
>

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Andreas Färber]

Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
>>> In order to allow attaching machine options to a machine instance,
>>> current_machine is converted into MachineState.
>>> As a first step of deprecating QEMUMachine, some of the functions
>>> were modified to return MachineCLass.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>
>> Looks mostly good, but same issue as Alexey's patch: We are risking
>> qdev_get_machine() creating a Container-typed /machine node.
>>
>> What about the following on top?
> Hi Andreas,
> 
> I checked with the debugger and qdev_get_machine is called
> long after we add the machine to the QOM tree.
> However, the race still exists as someone can call qdev_get_machine
> before the machine is added to the tree, not being aware of that.
> 
> Your change solves the problem, thank you!
> Do you want me to add this diff and resend,
> or I will send yours separately?

My preference would be to avoid another round of review on my part by
simply squashing into your 3/3.

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 v3 1/3] hw/core: introduced qemu machine as QOM object

[Andreas Färber]

Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> +struct MachineState {
> +    /*< private >*/
> +    Object parent_obj;
> +    /*< 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;
> +};

If I'm reading directly, init_args is the only field used in this
3-patch series. Should we drop the unused ones for now? Or do you think
we should get the whole conversion done for 2.0 and therefore keep them?

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 v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
> > On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
> >> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> >>> In order to allow attaching machine options to a machine instance,
> >>> current_machine is converted into MachineState.
> >>> As a first step of deprecating QEMUMachine, some of the functions
> >>> were modified to return MachineCLass.
> >>>
> >>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>
> >> Looks mostly good, but same issue as Alexey's patch: We are risking
> >> qdev_get_machine() creating a Container-typed /machine node.
> >>
> >> What about the following on top?
> > Hi Andreas,
> > 
> > I checked with the debugger and qdev_get_machine is called
> > long after we add the machine to the QOM tree.
> > However, the race still exists as someone can call qdev_get_machine
> > before the machine is added to the tree, not being aware of that.
> > 
> > Your change solves the problem, thank you!
> > Do you want me to add this diff and resend,
> > or I will send yours separately?
> 
> My preference would be to avoid another round of review on my part by
> simply squashing into your 3/3.
There is a problem with it: 'make check fails' on test-qdev-global-props.
- 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
  has TYPE_DEVICE as parent.
- The machine is added to the QOM tree *only in vl's main* and this test does not
  reach it, but assumes that always will be a machine in the QOM tree.
  This is no longer true.

Possible solution would be making existing 'null machine' a subclass of MachineClass
and add it manually to QOM on this test(and other places as necessary). The risk here is
that there are other places where the machine needs to be added manually to the QOM tree.
(I am trying this option, but make check gets stuck !!!, debugging)

Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
and use this in qdev_get_machine() implementation. (ugly?)

Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)

Any thoughts?
Thanks, 
Marcel

 

> 
> Cheers,
> Andreas
> 

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Paolo Bonzini]

Il 07/03/2014 17:22, Marcel Apfelbaum ha scritto:
> There is a problem with it: 'make check fails' on test-qdev-global-props.
> - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
>   has TYPE_DEVICE as parent.
> - The machine is added to the QOM tree *only in vl's main* and this test does not
>   reach it, but assumes that always will be a machine in the QOM tree.
>   This is no longer true.
>
> Possible solution would be making existing 'null machine' a subclass of MachineClass
> and add it manually to QOM on this test(and other places as necessary). The risk here is
> that there are other places where the machine needs to be added manually to the QOM tree.
> (I am trying this option, but make check gets stuck !!!, debugging)

This is probably the right thing to do, but I guess it means it's better 
to leave this series to 2.1.

Paolo

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Andreas Färber]

Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
>> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
>>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
>>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
>>>>> In order to allow attaching machine options to a machine instance,
>>>>> current_machine is converted into MachineState.
>>>>> As a first step of deprecating QEMUMachine, some of the functions
>>>>> were modified to return MachineCLass.
>>>>>
>>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>>
>>>> Looks mostly good, but same issue as Alexey's patch: We are risking
>>>> qdev_get_machine() creating a Container-typed /machine node.
>>>>
>>>> What about the following on top?
>>> Hi Andreas,
>>>
>>> I checked with the debugger and qdev_get_machine is called
>>> long after we add the machine to the QOM tree.
>>> However, the race still exists as someone can call qdev_get_machine
>>> before the machine is added to the tree, not being aware of that.
>>>
>>> Your change solves the problem, thank you!
>>> Do you want me to add this diff and resend,
>>> or I will send yours separately?
>>
>> My preference would be to avoid another round of review on my part by
>> simply squashing into your 3/3.
> There is a problem with it: 'make check fails' on test-qdev-global-props.
> - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
>   has TYPE_DEVICE as parent.
> - The machine is added to the QOM tree *only in vl's main* and this test does not
>   reach it, but assumes that always will be a machine in the QOM tree.
>   This is no longer true.
> 
> Possible solution would be making existing 'null machine' a subclass of MachineClass
> and add it manually to QOM on this test(and other places as necessary).

The following hack fixes this particular failure for me (ran into it
while trying to generate the HTML report):

diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
index e4ad173..31bac15 100644
--- a/tests/test-qdev-global-props.c
+++ b/tests/test-qdev-global-props.c
@@ -167,6 +167,9 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);

     module_call_init(MODULE_INIT_QOM);
+
+    object_property_add_child(object_get_root(), "machine",
object_new("container"), NULL);
+
     type_register_static(&static_prop_type);
     type_register_static(&dynamic_prop_type);


Not yet suitable for squashing obviously.

Andreas

> The risk here is
> that there are other places where the machine needs to be added manually to the QOM tree.
> (I am trying this option, but make check gets stuck !!!, debugging)
> 
> Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
> and use this in qdev_get_machine() implementation. (ugly?)
> 
> Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)
> 
> Any thoughts?
> Thanks, 
> Marcel
> 
>  
> 
>>
>> 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 v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Fri, 2014-03-07 at 18:30 +0100, Andreas Färber wrote:
> Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> > On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
> >> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
> >>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
> >>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> >>>>> In order to allow attaching machine options to a machine instance,
> >>>>> current_machine is converted into MachineState.
> >>>>> As a first step of deprecating QEMUMachine, some of the functions
> >>>>> were modified to return MachineCLass.
> >>>>>
> >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>>>
> >>>> Looks mostly good, but same issue as Alexey's patch: We are risking
> >>>> qdev_get_machine() creating a Container-typed /machine node.
> >>>>
> >>>> What about the following on top?
> >>> Hi Andreas,
> >>>
> >>> I checked with the debugger and qdev_get_machine is called
> >>> long after we add the machine to the QOM tree.
> >>> However, the race still exists as someone can call qdev_get_machine
> >>> before the machine is added to the tree, not being aware of that.
> >>>
> >>> Your change solves the problem, thank you!
> >>> Do you want me to add this diff and resend,
> >>> or I will send yours separately?
> >>
> >> My preference would be to avoid another round of review on my part by
> >> simply squashing into your 3/3.
> > There is a problem with it: 'make check fails' on test-qdev-global-props.
> > - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
> >   has TYPE_DEVICE as parent.
> > - The machine is added to the QOM tree *only in vl's main* and this test does not
> >   reach it, but assumes that always will be a machine in the QOM tree.
> >   This is no longer true.
> > 
> > Possible solution would be making existing 'null machine' a subclass of MachineClass
> > and add it manually to QOM on this test(and other places as necessary).
> 
> The following hack fixes this particular failure for me (ran into it
> while trying to generate the HTML report):
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index e4ad173..31bac15 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -167,6 +167,9 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
> 
>      module_call_init(MODULE_INIT_QOM);
> +
> +    object_property_add_child(object_get_root(), "machine",
> object_new("container"), NULL);
> +
Great! Thanks! I was trying to create a null-machine, but for this scenario
a container is more than enough.

>      type_register_static(&static_prop_type);
>      type_register_static(&dynamic_prop_type);
> 
> 
> Not yet suitable for squashing obviously.
I tested it and make check passes for all architectures, so why not?
It seems elegant and not a hack (this scenario does not require an actual machine).

Thanks,
Marcel
> 
> Andreas
> 
> > The risk here is
> > that there are other places where the machine needs to be added manually to the QOM tree.
> > (I am trying this option, but make check gets stuck !!!, debugging)
> > 
> > Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
> > and use this in qdev_get_machine() implementation. (ugly?)
> > 
> > Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)
> > 
> > Any thoughts?
> > Thanks, 
> > Marcel
> > 
> >  
> > 
> >>
> >> Cheers,
> >> Andreas
> >>
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Fri, 2014-03-07 at 18:30 +0100, Andreas Färber wrote:
> Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> > On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
> >> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
> >>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
> >>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> >>>>> In order to allow attaching machine options to a machine instance,
> >>>>> current_machine is converted into MachineState.
> >>>>> As a first step of deprecating QEMUMachine, some of the functions
> >>>>> were modified to return MachineCLass.
> >>>>>
> >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>>>
> >>>> Looks mostly good, but same issue as Alexey's patch: We are risking
> >>>> qdev_get_machine() creating a Container-typed /machine node.
> >>>>
> >>>> What about the following on top?
> >>> Hi Andreas,
> >>>
> >>> I checked with the debugger and qdev_get_machine is called
> >>> long after we add the machine to the QOM tree.
> >>> However, the race still exists as someone can call qdev_get_machine
> >>> before the machine is added to the tree, not being aware of that.
> >>>
> >>> Your change solves the problem, thank you!
> >>> Do you want me to add this diff and resend,
> >>> or I will send yours separately?
> >>
> >> My preference would be to avoid another round of review on my part by
> >> simply squashing into your 3/3.
> > There is a problem with it: 'make check fails' on test-qdev-global-props.
> > - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
> >   has TYPE_DEVICE as parent.
> > - The machine is added to the QOM tree *only in vl's main* and this test does not
> >   reach it, but assumes that always will be a machine in the QOM tree.
> >   This is no longer true.
> > 
> > Possible solution would be making existing 'null machine' a subclass of MachineClass
> > and add it manually to QOM on this test(and other places as necessary).
> 
> The following hack fixes this particular failure for me (ran into it
> while trying to generate the HTML report):
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index e4ad173..31bac15 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -167,6 +167,9 @@ int main(int argc, char **argv)
>      g_test_init(&argc, &argv, NULL);
> 
>      module_call_init(MODULE_INIT_QOM);
> +
> +    object_property_add_child(object_get_root(), "machine",
> object_new("container"), NULL);
> +
>      type_register_static(&static_prop_type);
>      type_register_static(&dynamic_prop_type);
> 
> 
> Not yet suitable for squashing obviously.
Hi Andreas,

I found the reason why the 'make check' is getting stuck, it was
related to a bug in qtest library. I posted a patch that fixes this:
    [PATCH V4] tests/libqtest: Fix possible deadlock in qtest initialization

I saw that you already squashed "object_resolve_path" into qdev_get_machine.
I just sent
    [PATCH 0/3] tests/qdev-global-props: fixes due to machine conversion to QOM
which takes care of the 'make check' failures.

I think is OK now.

Thanks,
Marcel
 
> 
> Andreas
> 
> > The risk here is
> > that there are other places where the machine needs to be added manually to the QOM tree.
> > (I am trying this option, but make check gets stuck !!!, debugging)
> > 
> > Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
> > and use this in qdev_get_machine() implementation. (ugly?)
> > 
> > Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)
> > 
> > Any thoughts?
> > Thanks, 
> > Marcel
> > 
> >  
> > 
> >>
> >> Cheers,
> >> Andreas
> >>
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Andreas Färber]

Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
>> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
>>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
>>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
>>>>> In order to allow attaching machine options to a machine instance,
>>>>> current_machine is converted into MachineState.
>>>>> As a first step of deprecating QEMUMachine, some of the functions
>>>>> were modified to return MachineCLass.
>>>>>
>>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
>>>>
>>>> Looks mostly good, but same issue as Alexey's patch: We are risking
>>>> qdev_get_machine() creating a Container-typed /machine node.
>>>>
>>>> What about the following on top?
>>> Hi Andreas,
>>>
>>> I checked with the debugger and qdev_get_machine is called
>>> long after we add the machine to the QOM tree.
>>> However, the race still exists as someone can call qdev_get_machine
>>> before the machine is added to the tree, not being aware of that.
>>>
>>> Your change solves the problem, thank you!
>>> Do you want me to add this diff and resend,
>>> or I will send yours separately?
>>
>> My preference would be to avoid another round of review on my part by
>> simply squashing into your 3/3.
> There is a problem with it: 'make check fails' on test-qdev-global-props.
> - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
>   has TYPE_DEVICE as parent.
> - The machine is added to the QOM tree *only in vl's main* and this test does not
>   reach it, but assumes that always will be a machine in the QOM tree.
>   This is no longer true.

My simple solution here is to revert my own patch addition. If /machine
exists, container.c:container_get() will use object_resolve_path(), just
like my diff, to obtain the pre-existing object. And your addition of
the /machine child<> in vl.c actually uses error_abort, so would error
out if already added by qdev_get_machine(). This means that vl.c
actually works as intended and the unit test would continue to
implicitly create the /machine code without us needing to add more
workarounds.

Regards,
Andreas

> 
> Possible solution would be making existing 'null machine' a subclass of MachineClass
> and add it manually to QOM on this test(and other places as necessary). The risk here is
> that there are other places where the machine needs to be added manually to the QOM tree.
> (I am trying this option, but make check gets stuck !!!, debugging)
> 
> Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
> and use this in qdev_get_machine() implementation. (ugly?)
> 
> Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)
> 
> Any thoughts?
> Thanks, 
> Marcel

-- 
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 v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Marcel Apfelbaum]

On Tue, 2014-03-11 at 17:48 +0100, Andreas Färber wrote:
> Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> > On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
> >> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
> >>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber wrote:
> >>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum:
> >>>>> In order to allow attaching machine options to a machine instance,
> >>>>> current_machine is converted into MachineState.
> >>>>> As a first step of deprecating QEMUMachine, some of the functions
> >>>>> were modified to return MachineCLass.
> >>>>>
> >>>>> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> >>>>
> >>>> Looks mostly good, but same issue as Alexey's patch: We are risking
> >>>> qdev_get_machine() creating a Container-typed /machine node.
> >>>>
> >>>> What about the following on top?
> >>> Hi Andreas,
> >>>
> >>> I checked with the debugger and qdev_get_machine is called
> >>> long after we add the machine to the QOM tree.
> >>> However, the race still exists as someone can call qdev_get_machine
> >>> before the machine is added to the tree, not being aware of that.
> >>>
> >>> Your change solves the problem, thank you!
> >>> Do you want me to add this diff and resend,
> >>> or I will send yours separately?
> >>
> >> My preference would be to avoid another round of review on my part by
> >> simply squashing into your 3/3.
> > There is a problem with it: 'make check fails' on test-qdev-global-props.
> > - 'qdev_get_machine()' is called by 'device_set_realized()' because static_prop_type
> >   has TYPE_DEVICE as parent.
> > - The machine is added to the QOM tree *only in vl's main* and this test does not
> >   reach it, but assumes that always will be a machine in the QOM tree.
> >   This is no longer true.
> 
> My simple solution here is to revert my own patch addition. If /machine
> exists, container.c:container_get() will use object_resolve_path(), just
> like my diff, to obtain the pre-existing object. And your addition of
> the /machine child<> in vl.c actually uses error_abort, so would error
> out if already added by qdev_get_machine(). This means that vl.c
> actually works as intended and the unit test would continue to
> implicitly create the /machine code without us needing to add more
> workarounds.
I completely agree, I liked the idea of "object_resolve_path" because
the current code hides the *subtle* and *not straight forward* idea you
just mentioned above.
Meaning, it would be much easier to understand the code if it would just work,
but maybe it does not worth all the workarounds.

So, no problem, just "un-squash" this,

Thanks for the help,
Marcel

> 
> Regards,
> Andreas
> 
> > 
> > Possible solution would be making existing 'null machine' a subclass of MachineClass
> > and add it manually to QOM on this test(and other places as necessary). The risk here is
> > that there are other places where the machine needs to be added manually to the QOM tree.
> > (I am trying this option, but make check gets stuck !!!, debugging)
> > 
> > Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" define
> > and use this in qdev_get_machine() implementation. (ugly?)
> > 
> > Finally, is possible to be aware that may be a race when doing code review. ("dangerous"?)
> > 
> > Any thoughts?
> > Thanks, 
> > Marcel
>