| Message ID | 20121003182411.7075c0ef@nial.usersys.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > 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? > 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)
On Wed, 3 Oct 2012 13:54:34 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > 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? > > > > 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. > After irc discussion, Anthony suggested to use static properties instead of dynamic ones that we use now. But qdev_prop_set_globals() in device_initfn() is still causes problems even with static properties. For x86 CPU classes we were going dynamically generate CPU classes and store pointer to appropriate cpudef from builtin_x86_defs in class field for each CPU class and then init default feature words values from this field int x86_cpu_initfn(). However with qdev_prop_set_globals() in device_initfn() that is called before x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will overwrite whatever was set by qdev_prop_set_globals(). IMHO from general POV it's not correct to set properties before object is completely created. But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could we move it from device_initfn() to qdev_init() or some other place then?
Il 04/10/2012 08:53, Igor Mammedov ha scritto: > IMHO from general POV it's not correct to set properties before object is > completely created. > But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could > we move it from device_initfn() to qdev_init() or some other place then? Yes, moving it qdev_init would work. I don't like it particularly but hey... Paolo
On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > On Wed, 3 Oct 2012 13:54:34 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > 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? > > > > > > > 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. > > > After irc discussion, Anthony suggested to use static properties instead of > dynamic ones that we use now. > > But qdev_prop_set_globals() in device_initfn() is still causes problems even > with static properties. > > For x86 CPU classes we were going dynamically generate CPU classes and store > pointer to appropriate cpudef from builtin_x86_defs in class field for each > CPU class and then init default feature words values from this field int > x86_cpu_initfn(). > > However with qdev_prop_set_globals() in device_initfn() that is called before > x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will > overwrite whatever was set by qdev_prop_set_globals(). We can set the default values on class_init, instead. The class_init function for each CPU model can get the x86_def_t struct as the data pointer. I still think that the interface to build the DeviceClass.props array on class_init is really painful to use, but it's still doable. > > IMHO from general POV it's not correct to set properties before object is > completely created. If I understood it correctly, the point of all this is to allow properties (and their defaults) to be introspected by just looking at the class, without having to create any object. IMO the problem is that we don't have any decent mechanism to implement machine-type compatibility behavior that doesn't involve having to deal with the strict rules of global properties. All this work is blocking us from implementing many necessary bug fixes. It's OK if the long-term goal is to move the code to this generic, flexible, fully-instropectable, complex framework, but we need to have a way to implement bug fixes without waiting for all this work to be finished. > But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so could > we move it from device_initfn() to qdev_init() or some other place then?
Am 04.10.2012 14:43, schrieb Eduardo Habkost: > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: >> For x86 CPU classes we were going dynamically generate CPU classes and store >> pointer to appropriate cpudef from builtin_x86_defs in class field for each >> CPU class and then init default feature words values from this field int >> x86_cpu_initfn(). >> >> However with qdev_prop_set_globals() in device_initfn() that is called before >> x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will >> overwrite whatever was set by qdev_prop_set_globals(). > > We can set the default values on class_init, instead. The class_init > function for each CPU model can get the x86_def_t struct as the data > pointer. Let's avoid going backwards here, the plan was to have imperative initfns, so x86_def_t would go away, no? I'm catching up my mail on multiple fronts and will continue review, IIUC Blue already applied the CPU feature deduplification series so according to your roadmap this series is next. >> IMHO from general POV it's not correct to set properties before object is >> completely created. > > If I understood it correctly, the point of all this is to allow > properties (and their defaults) to be introspected by just looking at > the class, without having to create any object. It would be news to me that that was Anthony's plan... Static properties are being assigned to the class but populated at instantiation time. We do not have class properties as such. Andreas
On Thu, 4 Oct 2012 09:43:41 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > On Wed, 3 Oct 2012 13:54:34 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > 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? > > > > > > > > > > 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. > > > > > After irc discussion, Anthony suggested to use static properties instead > > of dynamic ones that we use now. > > > > But qdev_prop_set_globals() in device_initfn() is still causes problems > > even with static properties. > > > > For x86 CPU classes we were going dynamically generate CPU classes and > > store pointer to appropriate cpudef from builtin_x86_defs in class field > > for each CPU class and then init default feature words values from this > > field int x86_cpu_initfn(). > > > > However with qdev_prop_set_globals() in device_initfn() that is called > > before x86_cpu_initfn() it won't work because defaults in > > x86_cpu_initfn() will overwrite whatever was set by > > qdev_prop_set_globals(). > > We can set the default values on class_init, instead. The class_init > function for each CPU model can get the x86_def_t struct as the data > pointer. > > I still think that the interface to build the DeviceClass.props array on > class_init is really painful to use, but it's still doable. You mean dynamic building of DeviceClass.props arrays for each CPU sub-class? > > > > > IMHO from general POV it's not correct to set properties before object is > > completely created. > > If I understood it correctly, the point of all this is to allow > properties (and their defaults) to be introspected by just looking at > the class, without having to create any object. > > IMO the problem is that we don't have any decent mechanism to implement > machine-type compatibility behavior that doesn't involve having to deal > with the strict rules of global properties. All this work is blocking us > from implementing many necessary bug fixes. > > It's OK if the long-term goal is to move the code to this generic, > flexible, fully-instropectable, complex framework, but we need to have a > way to implement bug fixes without waiting for all this work to be > finished. > > > > But Anthony wants to keep qdev_prop_set_globals() qdev only thing, so > > could we move it from device_initfn() to qdev_init() or some other place > > then? >
On Thu, Oct 04, 2012 at 02:57:29PM +0200, Andreas Färber wrote: > Am 04.10.2012 14:43, schrieb Eduardo Habkost: > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > >> For x86 CPU classes we were going dynamically generate CPU classes and store > >> pointer to appropriate cpudef from builtin_x86_defs in class field for each > >> CPU class and then init default feature words values from this field int > >> x86_cpu_initfn(). > >> > >> However with qdev_prop_set_globals() in device_initfn() that is called before > >> x86_cpu_initfn() it won't work because defaults in x86_cpu_initfn() will > >> overwrite whatever was set by qdev_prop_set_globals(). > > > > We can set the default values on class_init, instead. The class_init > > function for each CPU model can get the x86_def_t struct as the data > > pointer. > > Let's avoid going backwards here, the plan was to have imperative > initfns, so x86_def_t would go away, no? It was never my plan, and I still don't see why we would want to do that. > > I'm catching up my mail on multiple fronts and will continue review, > IIUC Blue already applied the CPU feature deduplification series so > according to your roadmap this series is next. It would be the next series on the queue, yes, but now Anthony wants the CPU properties to be all static properties (set on the DeviceClass.props array). (We were discussing this yesterday on #qemu.) I suggested including this series anyway, and then move the properties to be static later (after converting CPU to DeviceState), but Anthony didn't like the idea. > > >> IMHO from general POV it's not correct to set properties before object is > >> completely created. > > > > If I understood it correctly, the point of all this is to allow > > properties (and their defaults) to be introspected by just looking at > > the class, without having to create any object. > > It would be news to me that that was Anthony's plan... Static properties > are being assigned to the class but populated at instantiation time. We > do not have class properties as such. That was the explanation (if I understood it correctly) for why we won't global properties can't be used to set "dynamic" properties (properties registered and set on class->instance_init()).
On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote: > On Thu, 4 Oct 2012 09:43:41 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > > On Wed, 3 Oct 2012 13:54:34 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > > 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? > > > > > > > > > > > > > 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. > > > > > > > After irc discussion, Anthony suggested to use static properties instead > > > of dynamic ones that we use now. > > > > > > But qdev_prop_set_globals() in device_initfn() is still causes problems > > > even with static properties. > > > > > > For x86 CPU classes we were going dynamically generate CPU classes and > > > store pointer to appropriate cpudef from builtin_x86_defs in class field > > > for each CPU class and then init default feature words values from this > > > field int x86_cpu_initfn(). > > > > > > However with qdev_prop_set_globals() in device_initfn() that is called > > > before x86_cpu_initfn() it won't work because defaults in > > > x86_cpu_initfn() will overwrite whatever was set by > > > qdev_prop_set_globals(). > > > > We can set the default values on class_init, instead. The class_init > > function for each CPU model can get the x86_def_t struct as the data > > pointer. > > > > I still think that the interface to build the DeviceClass.props array on > > class_init is really painful to use, but it's still doable. > > You mean dynamic building of DeviceClass.props arrays for each CPU sub-class? That's the only solution I see if we want to make all the CPU properties static, yes. I'm still not convinced we really need to do that, though. Maybe we can make static only the ones we really need to be able to implement machine-type-compatibility global properties? Machine-type compatibility global properties were the initial reason for the static-properties requirement. We don't really need to allow _all_ CPU features to be controlled by global properties, only the ones we need for machine-type compatibility.
On Thu, 04 Oct 2012 14:57:29 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 04.10.2012 14:43, schrieb Eduardo Habkost: > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > >> For x86 CPU classes we were going dynamically generate CPU classes and > >> store pointer to appropriate cpudef from builtin_x86_defs in class field > >> for each CPU class and then init default feature words values from this > >> field int x86_cpu_initfn(). > >> > >> However with qdev_prop_set_globals() in device_initfn() that is called > >> before x86_cpu_initfn() it won't work because defaults in > >> x86_cpu_initfn() will overwrite whatever was set by > >> qdev_prop_set_globals(). > > > > We can set the default values on class_init, instead. The class_init > > function for each CPU model can get the x86_def_t struct as the data > > pointer. > > Let's avoid going backwards here, the plan was to have imperative > initfns, so x86_def_t would go away, no? > > I'm catching up my mail on multiple fronts and will continue review, > IIUC Blue already applied the CPU feature deduplification series so > according to your roadmap this series is next. > I guess no, it's now cpu-as-device that should be next on a queue, since I'm rewriting this series to use static properties instead so CPU must be a device. Though work would be easier if static props we written&applied on top of this, it's not like we need working for CPU qdev_prop_set_globals() after this series.
On Thu, Oct 04, 2012 at 03:10:53PM +0200, Igor Mammedov wrote: > On Thu, 04 Oct 2012 14:57:29 +0200 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 04.10.2012 14:43, schrieb Eduardo Habkost: > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > >> For x86 CPU classes we were going dynamically generate CPU classes and > > >> store pointer to appropriate cpudef from builtin_x86_defs in class field > > >> for each CPU class and then init default feature words values from this > > >> field int x86_cpu_initfn(). > > >> > > >> However with qdev_prop_set_globals() in device_initfn() that is called > > >> before x86_cpu_initfn() it won't work because defaults in > > >> x86_cpu_initfn() will overwrite whatever was set by > > >> qdev_prop_set_globals(). > > > > > > We can set the default values on class_init, instead. The class_init > > > function for each CPU model can get the x86_def_t struct as the data > > > pointer. > > > > Let's avoid going backwards here, the plan was to have imperative > > initfns, so x86_def_t would go away, no? > > > > I'm catching up my mail on multiple fronts and will continue review, > > IIUC Blue already applied the CPU feature deduplification series so > > according to your roadmap this series is next. > > > I guess no, it's now cpu-as-device that should be next on a queue, since I'm > rewriting this series to use static properties instead so CPU must be a > device. Whatever we decide on this thread about CPU properties, cpu-as-device seems to be the obvious candidate for "next-in-the-queue", or at least it can be written/discussed in parallel. I plan to submit a new RFC soon. > > Though work would be easier if static props we written&applied on top of > this, it's not like we need working for CPU qdev_prop_set_globals() after this > series. > > >
On Thu, 4 Oct 2012 10:10:27 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote: > > On Thu, 4 Oct 2012 09:43:41 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > > > On Wed, 3 Oct 2012 13:54:34 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > > > 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? > > > > > > > > > > > > > > > > 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. > > > > > > > > > After irc discussion, Anthony suggested to use static properties > > > > instead of dynamic ones that we use now. > > > > > > > > But qdev_prop_set_globals() in device_initfn() is still causes > > > > problems even with static properties. > > > > > > > > For x86 CPU classes we were going dynamically generate CPU classes and > > > > store pointer to appropriate cpudef from builtin_x86_defs in class > > > > field for each CPU class and then init default feature words values > > > > from this field int x86_cpu_initfn(). > > > > > > > > However with qdev_prop_set_globals() in device_initfn() that is called > > > > before x86_cpu_initfn() it won't work because defaults in > > > > x86_cpu_initfn() will overwrite whatever was set by > > > > qdev_prop_set_globals(). > > > > > > We can set the default values on class_init, instead. The class_init > > > function for each CPU model can get the x86_def_t struct as the data > > > pointer. > > > > > > I still think that the interface to build the DeviceClass.props array on > > > class_init is really painful to use, but it's still doable. > > > > You mean dynamic building of DeviceClass.props arrays for each CPU > > sub-class? > > That's the only solution I see if we want to make all the CPU properties > static, yes. Well I could generate compile time arrays for every built-in cpu model and we can remove then x86_def_t struct & builtins altogether. Only 'host' would be left for dynamic generation then. > > I'm still not convinced we really need to do that, though. Maybe we can > make static only the ones we really need to be able to implement > machine-type-compatibility global properties? > > Machine-type compatibility global properties were the initial reason for > the static-properties requirement. We don't really need to allow _all_ > CPU features to be controlled by global properties, only the ones we > need for machine-type compatibility. >
On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote: > On Thu, 4 Oct 2012 10:10:27 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote: > > > On Thu, 4 Oct 2012 09:43:41 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > > > > On Wed, 3 Oct 2012 13:54:34 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > > > > 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? > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > After irc discussion, Anthony suggested to use static properties > > > > > instead of dynamic ones that we use now. > > > > > > > > > > But qdev_prop_set_globals() in device_initfn() is still causes > > > > > problems even with static properties. > > > > > > > > > > For x86 CPU classes we were going dynamically generate CPU classes and > > > > > store pointer to appropriate cpudef from builtin_x86_defs in class > > > > > field for each CPU class and then init default feature words values > > > > > from this field int x86_cpu_initfn(). > > > > > > > > > > However with qdev_prop_set_globals() in device_initfn() that is called > > > > > before x86_cpu_initfn() it won't work because defaults in > > > > > x86_cpu_initfn() will overwrite whatever was set by > > > > > qdev_prop_set_globals(). > > > > > > > > We can set the default values on class_init, instead. The class_init > > > > function for each CPU model can get the x86_def_t struct as the data > > > > pointer. > > > > > > > > I still think that the interface to build the DeviceClass.props array on > > > > class_init is really painful to use, but it's still doable. > > > > > > You mean dynamic building of DeviceClass.props arrays for each CPU > > > sub-class? > > > > That's the only solution I see if we want to make all the CPU properties > > static, yes. > > Well I could generate compile time arrays for every built-in cpu model > and we can remove then x86_def_t struct & builtins altogether. > Only 'host' would be left for dynamic generation then. You mean duplicating the property list in the code? Then the feature-name -> CPUID-bit mapping information would be duplicated on all those arrays, and adding support to a new CPU feature would require adding entries to all the arrays. > > > > > I'm still not convinced we really need to do that, though. Maybe we can > > make static only the ones we really need to be able to implement > > machine-type-compatibility global properties? > > > > Machine-type compatibility global properties were the initial reason for > > the static-properties requirement. We don't really need to allow _all_ > > CPU features to be controlled by global properties, only the ones we > > need for machine-type compatibility. > > >
On Thu, 4 Oct 2012 10:33:30 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote: > > On Thu, 4 Oct 2012 10:10:27 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote: > > > > On Thu, 4 Oct 2012 09:43:41 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > > > > > On Wed, 3 Oct 2012 13:54:34 -0300 > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > > > > > 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? > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > After irc discussion, Anthony suggested to use static properties > > > > > > instead of dynamic ones that we use now. > > > > > > > > > > > > But qdev_prop_set_globals() in device_initfn() is still causes > > > > > > problems even with static properties. > > > > > > > > > > > > For x86 CPU classes we were going dynamically generate CPU > > > > > > classes and store pointer to appropriate cpudef from > > > > > > builtin_x86_defs in class field for each CPU class and then init > > > > > > default feature words values from this field int x86_cpu_initfn(). > > > > > > > > > > > > However with qdev_prop_set_globals() in device_initfn() that is > > > > > > called before x86_cpu_initfn() it won't work because defaults in > > > > > > x86_cpu_initfn() will overwrite whatever was set by > > > > > > qdev_prop_set_globals(). > > > > > > > > > > We can set the default values on class_init, instead. The class_init > > > > > function for each CPU model can get the x86_def_t struct as the data > > > > > pointer. > > > > > > > > > > I still think that the interface to build the DeviceClass.props > > > > > array on class_init is really painful to use, but it's still doable. > > > > > > > > You mean dynamic building of DeviceClass.props arrays for each CPU > > > > sub-class? > > > > > > That's the only solution I see if we want to make all the CPU properties > > > static, yes. > > > > Well I could generate compile time arrays for every built-in cpu model > > and we can remove then x86_def_t struct & builtins altogether. > > Only 'host' would be left for dynamic generation then. > > You mean duplicating the property list in the code? Then the > feature-name -> CPUID-bit mapping information would be duplicated on all > those arrays, and adding support to a new CPU feature would require > adding entries to all the arrays. Not to all arrays, but only to ones which cpumodel-s support specific feature. Here is possible ups&downs this: + full introspection, including default cpu features values. -/+ it would be possible to represent cpumodel more faithfully, i.e. include only features that specific cpu supports. (so no AVX=on in 486 model), not sure if it is plus but it would more like real hw. - a lot of lines of code , but it could be dealt with extra macros, so resulting arrays could look like built-in now (if we add feature there we anyway should replicate it other relevant builtins). + maybe generating only "host"'s DeviceClass.props could be simplier. And then for builtin models I could make a series static CPU classes that would use this arrays. > > > > > > > > > I'm still not convinced we really need to do that, though. Maybe we can > > > make static only the ones we really need to be able to implement > > > machine-type-compatibility global properties? > > > > > > Machine-type compatibility global properties were the initial reason for > > > the static-properties requirement. We don't really need to allow _all_ > > > CPU features to be controlled by global properties, only the ones we > > > need for machine-type compatibility. > > > > > >
On Thu, Oct 04, 2012 at 03:50:34PM +0200, Igor Mammedov wrote: > On Thu, 4 Oct 2012 10:33:30 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Oct 04, 2012 at 03:25:59PM +0200, Igor Mammedov wrote: > > > On Thu, 4 Oct 2012 10:10:27 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote: > > > > > On Thu, 4 Oct 2012 09:43:41 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > On Thu, Oct 04, 2012 at 08:53:22AM +0200, Igor Mammedov wrote: > > > > > > > On Wed, 3 Oct 2012 13:54:34 -0300 > > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > > > > > On Wed, Oct 03, 2012 at 06:24:11PM +0200, Igor Mammedov wrote: > > > > > > > > > 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? > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > After irc discussion, Anthony suggested to use static properties > > > > > > > instead of dynamic ones that we use now. > > > > > > > > > > > > > > But qdev_prop_set_globals() in device_initfn() is still causes > > > > > > > problems even with static properties. > > > > > > > > > > > > > > For x86 CPU classes we were going dynamically generate CPU > > > > > > > classes and store pointer to appropriate cpudef from > > > > > > > builtin_x86_defs in class field for each CPU class and then init > > > > > > > default feature words values from this field int x86_cpu_initfn(). > > > > > > > > > > > > > > However with qdev_prop_set_globals() in device_initfn() that is > > > > > > > called before x86_cpu_initfn() it won't work because defaults in > > > > > > > x86_cpu_initfn() will overwrite whatever was set by > > > > > > > qdev_prop_set_globals(). > > > > > > > > > > > > We can set the default values on class_init, instead. The class_init > > > > > > function for each CPU model can get the x86_def_t struct as the data > > > > > > pointer. > > > > > > > > > > > > I still think that the interface to build the DeviceClass.props > > > > > > array on class_init is really painful to use, but it's still doable. > > > > > > > > > > You mean dynamic building of DeviceClass.props arrays for each CPU > > > > > sub-class? > > > > > > > > That's the only solution I see if we want to make all the CPU properties > > > > static, yes. > > > > > > Well I could generate compile time arrays for every built-in cpu model > > > and we can remove then x86_def_t struct & builtins altogether. > > > Only 'host' would be left for dynamic generation then. > > > > You mean duplicating the property list in the code? Then the > > feature-name -> CPUID-bit mapping information would be duplicated on all > > those arrays, and adding support to a new CPU feature would require > > adding entries to all the arrays. > Not to all arrays, but only to ones which cpumodel-s support specific feature. > > Here is possible ups&downs this: > + full introspection, including default cpu features values. > -/+ it would be possible to represent cpumodel more faithfully, i.e. include > only features that specific cpu supports. (so no AVX=on in 486 model), not > sure if it is plus but it would more like real hw. It looks like a downside, as it would break configurations that could have worked previously. I mean: a 486 with AVX may not exist, but that doesn't mean people don't have existing (and working) VMs in their systems with those settings, and they would break on a QEMU upgrade. > - a lot of lines of code , but it could be dealt with extra macros, so > resulting arrays could look like built-in now (if we add feature there we > anyway should replicate it other relevant builtins). The main problem I see is that the macros that define the property have to define its default value, too. For example: Westmere supports the tsc-deadline flag, but it default=false, and SandyBride would support the same flag, but with default=true. That means that somewhere in the code we will have a line saying: DEFINE_PROP_BIT("tsc-deadline", CPUX86State, ext_features, 24, false) and somewhere else, we will have a line saying: DEFINE_PROP_BIT("tsc-deadline", CPUX86State, ext_features, 24, true) I don't see how we could let different classes have different defaults without duplicating the list of properties, even with help of macros. > + maybe generating only "host"'s DeviceClass.props could be simplier. True. But we can still make the "host" class simpler, even if we don't do any of the above. (It could use a different instance init function, for example). Anyway, I don't see why so much effort to generate those property lists at compile time. The list is set on DeviceClass.props at runtime (at class_init) so we can generate the property list at runtime inside class_init, already. > > And then for builtin models I could make a series static CPU classes that > would use this arrays. > > > > > > > > > > > > > > > I'm still not convinced we really need to do that, though. Maybe we can > > > > make static only the ones we really need to be able to implement > > > > machine-type-compatibility global properties? > > > > > > > > Machine-type compatibility global properties were the initial reason for > > > > the static-properties requirement. We don't really need to allow _all_ > > > > CPU features to be controlled by global properties, only the ones we > > > > need for machine-type compatibility. > > > > > > > > > >
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)