On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> On Wed, 03 Oct 2012 17:20:46 +0200
> Paolo Bonzini <pbonz...@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?
>
That's almost exactly the same code I wrote here. :-)
The only difference is that I added post_init to the struct Object
documentation comments, and added a unit test. The unit test required
the qdev-core/qdev split, so we could compile it without bringing too
many dependencies. I will submit it soon.
> 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)
--
Eduardo
