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

[Eduardo Habkost]

On Thu, Oct 04, 2012 at 03:01:19PM +0200, Igor Mammedov wrote:
> On Thu, 4 Oct 2012 09:43:41 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden> 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 <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.
> > > > 
> > > 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.

-- 
Eduardo