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

[Andreas Färber]

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.

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

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

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