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

[Igor Mammedov]

On Mon, 16 Dec 2013 16:26:55 -0200
Eduardo Habkost <address@hidden> wrote:

> On Mon, Dec 16, 2013 at 04:01:05PM +0100, Igor Mammedov wrote:
> > 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.
> 
> I agree with Igor, here. We don't strictly need to make those properties
> "static" anymore, but it is still useful to do it, because it makes the
> instrospection information visible to other code (inside and eventually
> outside QEMU) before a CPU object gets created, and to me it really
> looks simpler than registering the properties at instance_init().
> 
> > 
> > > 
> > > >   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.
> 
> In the specific case of the feature-bit properties, I am more inclined
> to agree with Andreas: is making the properties "static" worth the extra
> code complexity?
qdev_prop_find_bit() is the only complexity with static bit properties,
otherwise feature bits as static props are much more maintainable and
self descriptive then feature name arrays.

> 
> We still don't have a QMP command to list all the properties supported
> by a DeviceClass, do we? If we had it, having static properties would be
> much more useful. Today they are not much better than simple "dynamic"
> properties registered at instance_init().
with static properties there won't be need to rewrite this twice, once
command becomes available.

> 
> > 
> > > 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 need to iterate over feature properties was something that I
> considered "okay" only because we were forced to use static properties.
> Now we moved global property handling to instance_post_init and we don't
> really need it anymore.
> 
> One more simple way to do it is to simply keep the existing
> feature_name[] arrays and use them to register/lookup feature property
> names. I would prefer that to the convoluted way this new code do
> feature bit->name lookup.
> 
> > 
> > > 
> > > 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?
> 
> Maybe Andreas is suggesting the same I suggest above: keep the
> feature_names arrays on feature_word_info and do something this on
> instance_init:
> 
>     for (w = 0; w < FEATURE_WORDS; w++) {
>         FeatureWordInfo *wi = &feature_word_info[w];
>         char **feat_names = wi->feat_names;
>         if (!feat_names)
>             continue;
>         for (bit = 0; bit < 32; bit++) {
>             if (!feat_names[bit])
>                 continue;
>             char *prop_name = g_strdup_printf("feat-%s", feat_names[bit]); is 
>             object_property_add_bit(OBJECT(cpu), prop_name, 
> &cpu->features[w], bit);
>         }
>     }
> 
> This way feature name->bit lookup/set/unset code would use the QOM
> property infrastructure, but feature bit->name lookup could still look
> exactly the same.
feature_name[] array is harder to maintain compared to proposed static props,
and bit->name lookup is not less convoluted than qdev_prop_find_bit(),
albeit the people are more used to it.

qdev_prop_find_bit() might be reused by other bit properties if bit->name
mapping would be needed while feature_name[] just hides problem.

> 
> 
> > 
> > > Am I the only one that finds the approach backwards? o.O
> 
> You are not alone. As I said above, I found it acceptable because it was
> the only way to make global properties work. But it is not really
> necessary anymore.
Anyway if majority are for using feature_name[] for bit->name lookup and
registering feature bits as dynamic properties, I'm not against it
considering we gave up on class introspection in favor of instance
introspection, there is not much difference currently between them,
except of maintainability of result.

> 
> > > 
> > > Regards,
> > > Andreas
> > > 
> > > >   target-i386: remove unused *_feature_name arrays
> > > >   target-i386: cpu: fix invalid use of error_is_set(errp) if errp ==
> > > >     NULL
> > > 
> > 
>