Re: [Qemu-devel] [PATCH qom-cpu 00/16 v10] target-i386: convert CPU features into properties

[Igor Mammedov]

On Sun, 15 Dec 2013 23:50:47 +0100
Andreas Färber <address@hidden> wrote:

> Am 27.11.2013 23:28, schrieb Igor Mammedov:
> > Igor Mammedov (16):
> >   target-i386: cleanup 'foo' feature handling'
> >   target-i386: cleanup 'foo=val' feature handling
> 
> Thanks, I've queued these on qom-cpu-next:
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next
> 
> >   target-i386: cpu: convert 'level' to static property
> >   target-i386: cpu: convert 'xlevel' to static property
> >   target-i386: cpu: convert 'family' to static property
> >   target-i386: cpu: convert 'model' to static property
> >   target-i386: cpu: convert 'stepping' to static property
> >   target-i386: cpu: convert 'vendor' to static property
> >   target-i386: cpu: convert 'model-id' to static property
> >   target-i386: cpu: convert 'tsc-frequency' to static property
> 
> But I still don't see the utility of this conversion after all the
> discussions we've had... :( The below patches seem to only operate on
> CPUID bits, which get added as properties in the following patch.
Above patches are there to simplify/unify current codebase. For example,
level & xlevel replace custom setters/getters with static property onliners.
The rest are making initfn more readable, not to mention that they
become visible in HMP along with the rest features wich is nice for
consistent behavior even is we do not care about HMP.
Otherwise there is not much difference between dynamic vs static anymore,
so this patches could be dropped, however with them ,I think,
code is a bit cleaner.

> 
> >   target-i386: set [+-]feature using static properties
> >   qdev: introduce qdev_prop_find_bit()
> >   target-i386: use static properties in check_features_against_host() to
> >     print CPUID feature names
> >   target-i386: use static properties to list CPUID features
> 
> I am reading too many occurrences of "static properties" above that
> should IMO just be "properties". You got permission to use a name-based
in current code base static properties are almost the same as dynamic ones,
it's just a more abbreviated version of dynamic ones with static defaults,
range checking, bit handling ...
So I don't see why more verbose dynamic properties SHOULD be used,
where the same code could be written more compact with static properties.

> scheme to iterate over feat-* properties, so why are you still iterating
> over static properties with a helper searching for offsets rather than
> QOM properties with feat- prefix? Either we need that scheme for
> automated processing as I understood you, then we should be consequent
> in using it, or we don't. And I would prefer to keep these mappings in
> x86 code rather than messing in generic device infrastructure and
> iterating over *all* properties in your qdev_prop_find_bit() and making
> generally available new QDEV_* macros QDEV_PROP_FOREACH() and
> QDEV_CLASS_FOREACH().
this patch was, was on list for more than half a year without any
complaints/reviews. I don't have ICR for that time already, but if I recall
correctly something like this was suggested by Anthony to resolve problem
of mapping bit fields to names. If there is a more simple elegant way to do
it I'd like to hear more concrete suggestion(s) how it should be vs just
"messing" verdict with which I don't agree btw.

> 
> The utility of the feat- prefix AIUI is to go from +foo to feat-foo=on;
> going from bit position to name should work just as before and could
> even be consolidated into a single array by using dynamic properties.
Could you elaborate more on what you are proposing please?

> Am I the only one that finds the approach backwards? o.O
> 
> Regards,
> Andreas
> 
> >   target-i386: remove unused *_feature_name arrays
> >   target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> >     NULL
>