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

[Eduardo Habkost]

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.

> 
> 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?

-- 
Eduardo