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

[Eduardo Habkost]

On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote:
> On Wed, 03 Oct 2012 17:20:46 +0200
> Paolo Bonzini <address@hidden> 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