[Qemu-devel] [RFC PATCH 1/6] qdev: Fix latent bug with compat_props and onboard devices

[Markus Armbruster]

Compatibility properties started life as a qdev property thing: we
supported them only for qdev properties, and implemented them with the
machinery backing command line option -global.

Recent commit fa0cb34d221 put them to use (tacitly) with memory
backend objects (subtypes of TYPE_MEMORY_BACKEND).  To make that
possible, we first moved the work of applying them from the -global
machinery into TYPE_DEVICE's .instance_post_init() method
device_post_init(), in commits ea9ce8934c5 and b66bbee39f6, then made
it available to TYPE_MEMORY_BACKEND's .instance_post_init() method
host_memory_backend_post_init() as object_apply_compat_props(), in
commit 1c3994f6d2a.

Note the code smell: we now have function name starting with object_
in hw/core/qdev.c.  It has to be there rather than in qom/, because it
calls qdev_get_machine() to find the current accelerator's and
machine's compat_props.

Turns out calling qdev_get_machine() there is problematic.  If we
qdev_create() from a machine's .instance_init() method, we call
device_post_init() and thus qdev_get_machine() before main() can
create "/machine" in QOM.  qdev_get_machine() tries to get it with
container_get(), which "helpfully" creates it as "container" object,
and returns that.  object_apply_compat_props() tries to paper over the
problem by doing nothing when the value of qdev_get_machine() isn't a
TYPE_MACHINE.  But the damage is done already: when main() later
attempts to create the real "/machine", it fails with "attempt to add
duplicate property 'machine' to object (type 'container')", and
aborts.

Since no machine .instance_init() calls qdev_create() so far, the bug
is latent.  But since I want to do that, I get to fix the bug first.

Observe that object_apply_compat_props() doesn't actually need the
MachineState, only its the compat_props member of its MachineClass and
AccelClass.  This permits a simple fix: register MachineClass and
AccelClass compat_props with the object_apply_compat_props() machinery
right after these classes get selected.

This is actually similar to how things worked before commits
ea9ce8934c5 and b66bbee39f6, except we now register much earlier.  The
old code registered them only after the machine's .instance_init()
ran, which would've broken compatibility properties for any devices
created there.

Cc: Marc-André Lureau <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>
---
 accel/accel.c          |  1 +
 hw/core/qdev.c         | 48 ++++++++++++++++++++++++++++++++----------
 include/hw/qdev-core.h |  2 ++
 vl.c                   |  1 +
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index 68b6d56323..4a1670e404 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -66,6 +66,7 @@ static int accel_init_machine(AccelClass *acc, MachineState 
*ms)
         *(acc->allowed) = false;
         object_unref(OBJECT(accel));
     }
+    object_set_accelerator_compat_props(acc->compat_props);
     return ret;
 }
 
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index d59071b8ed..1a86c7990a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -970,25 +970,51 @@ static void device_initfn(Object *obj)
     QLIST_INIT(&dev->gpios);
 }
 
+/*
+ * Global property defaults
+ * Slot 0: accelerator's global property defaults
+ * Slot 1: machine's global property defaults
+ * Each is a GPtrArray of of GlobalProperty.
+ * Applied in order, later entries override earlier ones.
+ */
+static GPtrArray *object_compat_props[2];
+
+/*
+ * Set machine's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_machine_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[1]);
+    object_compat_props[1] = compat_props;
+}
+
+/*
+ * Set accelerator's global property defaults to @compat_props.
+ * May be called at most once.
+ */
+void object_set_accelerator_compat_props(GPtrArray *compat_props)
+{
+    assert(!object_compat_props[0]);
+    object_compat_props[0] = compat_props;
+}
+
 void object_apply_compat_props(Object *obj)
 {
-    if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
-        MachineState *m = MACHINE(qdev_get_machine());
-        MachineClass *mc = MACHINE_GET_CLASS(m);
+    int i;
 
-        if (m->accelerator) {
-            AccelClass *ac = ACCEL_GET_CLASS(m->accelerator);
-
-            if (ac->compat_props) {
-                object_apply_global_props(obj, ac->compat_props, &error_abort);
-            }
-        }
-        object_apply_global_props(obj, mc->compat_props, &error_abort);
+    for (i = 0; i < ARRAY_SIZE(object_compat_props); i++) {
+        object_apply_global_props(obj, object_compat_props[i],
+                                  &error_abort);
     }
 }
 
 static void device_post_init(Object *obj)
 {
+    /*
+     * Note: ordered so that the user's global properties take
+     * precedence.
+     */
     object_apply_compat_props(obj);
     qdev_prop_set_globals(DEVICE(obj));
 }
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 0a84c42756..bced1f2666 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -418,6 +418,8 @@ const char *qdev_fw_name(DeviceState *dev);
 
 Object *qdev_get_machine(void);
 
+void object_set_machine_compat_props(GPtrArray *compat_props);
+void object_set_accelerator_compat_props(GPtrArray *compat_props);
 void object_apply_compat_props(Object *obj);
 
 /* FIXME: make this a link<> */
diff --git a/vl.c b/vl.c
index 502857a176..c50c2d6178 100644
--- a/vl.c
+++ b/vl.c
@@ -3954,6 +3954,7 @@ int main(int argc, char **argv, char **envp)
     configure_rtc(qemu_find_opts_singleton("rtc"));
 
     machine_class = select_machine();
+    object_set_machine_compat_props(machine_class->compat_props);
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
 
-- 
2.17.2