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

[Andreas Färber]

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.

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.

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

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.

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.

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg