Re: [Qemu-devel] [PATCH 07/23] target-i386: convert cpuid features into properties

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 03 Oct 2012 17:20:46 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 03/10/2012 17:03, Eduardo Habkost ha scritto:
> > On Tue, Oct 02, 2012 at 05:38:45PM -0300, Eduardo Habkost wrote:
> >> (Now replying on the right thread, to keep the discussion in the right
> >> place. I don't know how I ended up replying to a pre-historic version of
> >> the patch, sorry.)
> >>
> >> On Tue, Oct 02, 2012 at 05:36:59PM +0200, Igor Mammedov wrote:
> >> [...]
> >>> @@ -1938,6 +2043,12 @@ static void x86_cpu_initfn(Object *obj)
> >>>      object_property_add(obj, "tsc-frequency", "int",
> >>>                          x86_cpuid_get_tsc_freq,
> >>>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> >>> +    x86_register_cpuid_properties(obj, feature_name);
> >>> +    x86_register_cpuid_properties(obj, ext_feature_name);
> >>> +    x86_register_cpuid_properties(obj, ext2_feature_name);
> >>> +    x86_register_cpuid_properties(obj, ext3_feature_name);
> >>> +    x86_register_cpuid_properties(obj, kvm_feature_name);
> >>> +    x86_register_cpuid_properties(obj, svm_feature_name);
> >>
> >> Stupid question about qdev:
> >>
> >> - qdev_prop_set_globals() is called from device_initfn()
> >> - device_initfn() is called before the child class instance_init()
> >>   function (x86_cpu_initfn())
> >> - So, qdev_prop_set_globals() gets called before the CPU class
> >>   properties are registered.
> >>
> >> So this would defeat the whole point of all the work we're doing, that
> >> is to allow compatibility bits to be set as machine-type global
> >> properties. But I don't know what's the right solution here.
> >>
> >> Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
> >> Should the CPU properties be registered somewhere else?
> 
> Properties should be registered (for all objects, not just CPUs) in the
> instance_init function.  This is device_initfn.
> 
> I would add an instance_postinit function that is called at the end of
> object_initialize_with_type, that is after instance_init, and in the
> opposite order (i.e. from the leaf to the root).

You've meant something like that?

diff --git a/hw/qdev.c b/hw/qdev.c
index b5a52ac..4eb5f44 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -682,12 +682,17 @@ static void device_initfn(Object *obj)
         }
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
-    qdev_prop_set_globals(dev);
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
                              (Object **)&dev->parent_bus, NULL);
 }
 
+static void device_postinitfn(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    qdev_prop_set_globals(dev);
+}
+
 /* Unlink device from bus and free the structure.  */
 static void device_finalize(Object *obj)
 {
@@ -750,6 +755,7 @@ static TypeInfo device_type_info = {
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(DeviceState),
     .instance_init = device_initfn,
+    .instance_postinit = device_postinitfn,
     .instance_finalize = device_finalize,
     .class_base_init = device_class_base_init,
     .abstract = true,
diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..dfb5d8a 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -311,6 +311,7 @@ struct TypeInfo
 
     size_t instance_size;
     void (*instance_init)(Object *obj);
+    void (*instance_postinit)(Object *obj);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
diff --git a/qom/object.c b/qom/object.c
index e3e9242..979b410 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -49,6 +49,7 @@ struct TypeImpl
     void *class_data;
 
     void (*instance_init)(Object *obj);
+    void (*instance_postinit)(Object *obj);
     void (*instance_finalize)(Object *obj);
 
     bool abstract;
@@ -295,6 +296,17 @@ static void object_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
+static void object_postinit_with_type(Object *obj, TypeImpl *ti)
+{
+    if (ti->instance_postinit) {
+        ti->instance_postinit(obj);
+    }
+
+    if (type_has_parent(ti)) {
+        object_postinit_with_type(obj, type_get_parent(ti));
+    }
+}
+
 void object_initialize_with_type(void *data, TypeImpl *type)
 {
     Object *obj = data;
@@ -309,6 +321,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     obj->class = type->class;
     QTAILQ_INIT(&obj->properties);
     object_init_with_type(obj, type);
+    object_postinit_with_type(obj, type);
 }
 
 void object_initialize(void *data, const char *typename)