Re: [Qemu-devel] [PATCH qom-cpu 00/15 v8] target-i386: convert CPU features into properties, part 1

[Igor Mammedov]

On Wed, 05 Jun 2013 19:04:59 +0200
Andreas Färber <address@hidden> wrote:

> Am 05.06.2013 16:39, schrieb Igor Mammedov:
> > On Wed, 05 Jun 2013 15:29:08 +0200
> > Andreas Färber <address@hidden> wrote:
> > 
> >> Am 05.06.2013 15:18, schrieb Igor Mammedov:
> >>> It's a rebase of v7 on current qom-cpu tree, since then some patches from 
> >>> it
> >>> were applied to master. Convertion of feature bits is left for part 2
> >>> since it's not ready yet.
> >>>
> >>> v7-v8:
> >>> * split out dynamic properties convertion patch into per property patches
> >>>   to simplify review
> >>> * drop feature bits convertion
> >>
> >> Why is conversion of dynamic properties to static properties still
> >> needed after I applied a solution to override values of dynamic
> >> properties with -global for 1.5?
> > Do you mean qdev_prop_set_globals_for_type() & co?
> 
> Yes.
> 
> > If yes, then I recall it was acceptable hack to permit more clean
> > approach for compat props fixes to work. And we promised Anthony to
> > get rid of it when possible.
> 
> Indeed, but no one talked about reverting to static properties as the
> solution. :) Instead I was talking about solving this very general
> problem at DeviceState / QOM level.
> 
> > Now more to the point,
> > 
> > 1. if CPU are to be created with device_add(wich is still  a goal)
> >    cpu_x86_create() won't be created, so it leaves rules out compat
> >    properties set by qdev_prop_set_globals_for_type().
> 
> It does not rule it out, but I think we all agree that we do not want
> calls of it cluttered over subclasses.
> 
> Instead we have a very generic problem: instance_init is called
> recursively, parents first, so a parent class cannot do any instance
> initialization *after* its derived classes initialized the instance.
> That's contrary to how realize and other QOM methods work but in
> exchange for the flexibility put the burden of saving and calling the
> parent's implementation onto subclasses.

So far all efforts to do it failed. But I agree that it should be solved
at this level since setting defaults/globals on properties before instance
is completely created (i.e all instance_init()s called) is just wrong.
But it's orthogonal to static vs dynamic properties question.

> That's what I would like to change in some way, possibly a
> instance_post_init hook or the like, similar to how DeviceState got its
> own base class initialization hook to handle static props.
> That would not only keep the work low in this case but may also solve
> the virtio-net initialization problem reported elsewhere.
> 
> I'm pretty sure if we moved instance_init into ObjectClass, we'd not
> only get an insurge of *Class structs but also people forgetting to
> save&call the parent type's implementation, resulting in all kinds of
> weird errors, so I don't consider this a real option.
> 
> >    Having properties converted by this series as static would open
> >    road for:
> >        - making cpu_x86_parse_featurestr() target specific hook that
> >          deals with legacy cpu_model parsing and converts provided
> >          features into global properties.
> 
> Such a hook could be implemented today, no dependency on static props, I
> just didn't see the need yet. sparc is the only other target where I
> stumbled over this so far and haven't found time to prepare that yet.
It could be done by board specific code before creating CPUs, I do not
have a preference here (but class hook looks a bit cleaner and less likely
to break).

> 
> >            as result during hot-add device_add can work without
> >            specifying +-foo1,foo2,level=XXX, it will be applied from
> >            globals.
> 
> Using globals for that is fine with me. That code is currently per-CPU
> whereas -cpu does not allow for differently configured CPUs. Mixed CPU
> configurations would be either instantiated from the board and via -device.
> 
> >        - above will allow to replace create_x86_cpu() with plain
> >          device_add when subclasses are implemented.
> > 
> > 2. I'd like to utilize default values of static properties for defining
> >    subclasses like here: 
> > https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4f9517d0a25e5c
> >    - that would automatically provide benefit of class introspection
> >      without creating CPU instance
> >    - replace current CPU definitions array to a set of class_init_xxx
> >      for each subclass.
> 
> How and when do you actually want to inspect them? What's the use case?
> If we hardcode object_property_set_*() in instance_init then surely it's
> only inspectable in instances, much like the properties added for
> libvirt by Eduardo which didn't seem to disturb anyone. Peter actively
> chose the instance_init option for ARM CPUs, meaning they can only be
> inspected once created.
Yes, currently there is no way to do introspect property defaults at
class level, but it doesn't mean that we shouldn't try to move towards it.
Even if there was an interface to do so, it would still not be much useful
for i386 target since a lot of fixups at realize() stage, that needs to be
fixed as well for introspection at class level to work.

> However any field or pointer assignment in class_init will allow for
> some form of inspection after class creation (thinking of -cpu ? and
> query-cpus), including although not limited to today's x86_def_t.
Drawbacks in putting pointer to x86_def_t in class definition, is that
it could be beginning of bloating out class structure and would mean
implementation specific ways to inspect it.

If it's done with static properties, than needed pointer already
exists in DeviceClass and there could be a generic way to inspect it
for Device derived classes.

> 
> The only thing that seems to require static properties today is
> `qemu-system-x86_64 -device x86_64-cpu,?`, which happens to terminate
> right after, so might as well create an instance of the type to list all
> its properties.[*]
> I expect `qemu-system-x86_64 -device Haswell-x86_64-cpu,level=4` to work
> the same with dynamic and static properties.
> 
> `info qtree` by comparison works only at runtime even though operating
> on static properties and can easily be replaced with qom-list / qom-get.
> 
> What I dislike about static properties is that they seem a relic from
> qdev times that add yet another layer of abstraction above QOM
> properties. The only practical differences are:
> * QOM properties are added in instance_init and added to Object whereas
> qdev properties are assigned in class_init and use external storage, and
> * qdev properties specify a default value at property level whereas QOM
> properties set the default value in a implementation-dependent way,
> usually field assignment from either class field or a hardcoded value.
They might be relic but convenient and widely used one among "Devices".
Wouldn't it be better to have more unified property handling code vs
impl. specific?
It might be possible that if we come to having generic class properties
some day, that static properties would be much easier to convert to them.

Is there other reason not to use static properties besides "qdev relic"?
Current series simplifies x86_cpu_initfn() and makes property handling
more alike with other devices. And static properties will simplify
target-i386/cpu.c even more after feature_bits are converted to it.
And if static properties are extended to support value validation, a number
of custom property setters could be replaced by on-liners like it's done
for basic types now, reducing code base even more.


> 
> To me that poses the question of whether we may want to have a
> QTAILQ_HEAD(, ObjectProperty) properties;
> not only in Object but also in ObjectClass for inspection of name and
> type fields. We probably don't want to rule out dynamically added
> properties except for Peter's array properties in ARM devices.
> 
> [*] PowerPCCPU is known to leak memory on finalization, I have a patch
> pending. But I carefully employed QOM realize in CPUState to allow for
> creating an instance without creating vCPU threads so that it can easily
> be destroyed again (mostly thinking of property assignment errors back
> then), any problems with that should get fixed, ideally with test cases;
> QOM unrealize by comparison is not yet (widely) supported by CPUs I fear.
> 
> Regards,
> Andreas
>