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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 16 Dec 2013 16:26:55 -0200
Eduardo Habkost <ehabkost@redhat.com> 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 <afaerber@suse.de> 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?
On contrary, using static properties for feature-bits reduces overall code
complexity and provides much more better self documented properties compared
to current bit arrays and their handling.

So far we do not really need mapping bit2name for x86 CPU, there are 2
places where it's used now:
  1. comparing current CPU model with host CPUID bits.
     we could create temporary CPUhost model for it and compare properties
     instead of bits, something like:
         'compare(current_cpu, host_cpu, "feat-*")' 
  2. listing all feat-* properties for '-cpu ?'.
     that task could be performed by something like:
         object_property_foreach(object_new(x86_cpu), print_prop_if_name("feat-*"))

I guess it is what Andreas suggests instead of iterating over DEVICE->props
array.

[...]
> 
> -- 
> Eduardo


-- 
Regards,
  Igor