[RFC,07/20] target-i386: convert cpuid features into properties

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

On Fri, Aug 10, 2012 at 01:22:23PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 103 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a47cc12..4b22598 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -605,6 +605,103 @@ static int check_features_against_host(x86_def_t *guest_def)
>      return rv;
>  }
>  
> +static bool is_feature_set(const char *name, const uint32_t featbitmap,
> +                                  const char **featureset)
> +{
> +    uint32_t mask;
> +
> +    for (mask = 1; mask; mask <<= 1) {
> +        if (featureset[ffs(mask) - 1] &&
> +            !altcmp(name, NULL, featureset[ffs(mask) - 1])) {
> +            break;
> +    }

Wrong indentation.

> +    }
> +    if (featbitmap & mask) {
> +        return true;
> +    }
> +    return false;

Isn't it simpler to write this as:

    int bit;

    for (bit = 0; bit < 32; bit++) {
        if (featureset[bit] && !altcmp(name, NULL, featureset[bit])) {
            if (featbitmap & (1 << bit)) {
                return true;
        }
    }
    return false;

?

> +}
> +
> +static void x86_cpuid_get_feature(Object *obj, Visitor *v, void *opaque,
> +                                         const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    bool value = true;
> +
> +    if (!is_feature_set(name, env->cpuid_features, feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext_features, ext_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext2_features, ext2_feature_name) &&
> +       !is_feature_set(name, env->cpuid_ext3_features, ext3_feature_name) &&
> +       !is_feature_set(name, env->cpuid_kvm_features, kvm_feature_name) &&
> +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name)) {
> +        value = false;
> +    }
> +
> +    visit_type_bool(v, &value, name, errp);
> +}
> +
> +static void x86_cpuid_set_feature(Object *obj, Visitor *v, void *opaque,
> +                                         const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    uint32_t mask = 0;
> +    uint32_t *dst_features;
> +    bool value;
> +
> +    visit_type_bool(v, &value, name, errp);
> +    if (error_is_set(errp)) {
> +        return;
> +    }
> +
> +    if (lookup_feature(&mask, name, NULL, feature_name)) {
> +        dst_features = &env->cpuid_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext_feature_name)) {
> +        dst_features = &env->cpuid_ext_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext2_feature_name)) {
> +        dst_features = &env->cpuid_ext2_features;
> +    } else if (lookup_feature(&mask, name, NULL, ext3_feature_name)) {
> +        dst_features = &env->cpuid_ext3_features;
> +    } else if (lookup_feature(&mask, name, NULL, kvm_feature_name)) {
> +        dst_features = &env->cpuid_kvm_features;
> +    } else if (lookup_feature(&mask, name, NULL, svm_feature_name)) {
> +        dst_features = &env->cpuid_svm_features;
> +    } else {
> +        error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
> +        return;
> +    }
> +
> +    if (value) {
> +        *dst_features |= mask;
> +    } else {
> +        *dst_features &= ~mask;
> +    }
> +}
> +
> +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> +{
> +    uint32_t mask;
> +
> +    for (mask = 1; mask; mask <<= 1) {
> +        if (featureset[ffs(mask) - 1]) {
> +            char *feature_name, *save_ptr;
> +            char buf[32];
> +            if (strlen(featureset[ffs(mask) - 1]) > sizeof(buf) - 1) {
> +                abort();
> +            }
> +            pstrcpy(buf, sizeof(buf), featureset[ffs(mask) - 1]);
> +            feature_name = strtok_r(buf, "|", &save_ptr);
> +            while (feature_name) {
> +                object_property_add(obj, feature_name, "bool",
> +                                x86_cpuid_get_feature,
> +                                x86_cpuid_set_feature, NULL, NULL, NULL);
> +                feature_name = strtok_r(NULL, "|", &save_ptr);
> +            }
> +        }
> +    }

Same case as above: why the mask/ffs tricks when you could just use
"for (bit = 0; bit < 32; bit++)" and use "bit" instead of
"ffs(mask) - 1"?

> +}
> +
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1801,6 +1898,12 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    x86_register_cpuid_properties(obj, feature_name);
> +    x86_register_cpuid_properties(obj, ext_feature_name);
> +    x86_register_cpuid_properties(obj, ext2_feature_name);
> +    x86_register_cpuid_properties(obj, ext3_feature_name);
> +    x86_register_cpuid_properties(obj, kvm_feature_name);
> +    x86_register_cpuid_properties(obj, svm_feature_name);
>  
>      env->cpuid_apic_id = env->cpu_index;
>  
> -- 
> 1.7.11.2
>

On Fri, Aug 10, 2012 at 01:22:23PM +0200, Igor Mammedov wrote:
[...]
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1801,6 +1898,12 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> +    x86_register_cpuid_properties(obj, feature_name);
> +    x86_register_cpuid_properties(obj, ext_feature_name);
> +    x86_register_cpuid_properties(obj, ext2_feature_name);
> +    x86_register_cpuid_properties(obj, ext3_feature_name);
> +    x86_register_cpuid_properties(obj, kvm_feature_name);
> +    x86_register_cpuid_properties(obj, svm_feature_name);

Stupid question about qdev:

- qdev_prop_set_globals() is called from device_initfn()
- device_initfn() is called before the child class instance_init()
  function (x86_cpu_initfn())
- So, qdev_prop_set_globals() gets called before the CPU class
  properties are registered.

So this would defeat the whole point of all the work we're doing, that
is to allow compatibility bits to be set as machine-type global
properties. But I don't know what's the right solution here.

Should the qdev_prop_set_globals() call be moved to qdev_init() instead?
Should the CPU properties be registered somewhere else?