Patchwork [07/21] target-i386: convert cpuid features into properties

login
register
mail settings
Submitter Igor Mammedov
Date Aug. 15, 2012, 4:13 p.m.
Message ID <1345047221-26898-8-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/177740/
State New
Headers show

Comments

Igor Mammedov - Aug. 15, 2012, 4:13 p.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
v2:
  * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
    as suggested by Eduardo Habkost
---
 target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
Eduardo Habkost - Aug. 16, 2012, 6:10 p.m.
On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>   * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
>     as suggested by Eduardo Habkost
> ---
>  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 37ba5ef..440e724 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -606,6 +606,101 @@ 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 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;
> +    }

Some feature names are duplicated on feature_names and
ext_feature_names. On AMD CPU models, we have to set both, on Intel
models we need to set the bits only on cpuid_features.

Maybe it's better to:

1) eliminate the duplication and set the names only on feature_name
   array;
2) At the end of CPU initialization, set the features on
   cpuid_ext2_features as copies of the corresponding cpuid_features
   bits if CPU vendor == AMD (or, maybe, if some boolean
   "ext2_features_aliases" flag is set, to make it not
   vendor-dependent).


> +
> +    if (value) {
> +        *dst_features |= mask;
> +    } else {
> +        *dst_features &= ~mask;
> +    }
> +}
> +
> +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> +{
> +    uint32_t bit;
> +
> +    for (bit = 0; bit < 32; ++bit) {
> +        if (featureset[bit]) {
> +            char *feature_name, *save_ptr;
> +            char buf[32];
> +            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
> +                abort();
> +            }
> +            pstrcpy(buf, sizeof(buf), featureset[bit]);
> +            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);

What happens when object_property_add() is called twice with the same
feature name?

> +            }
> +        }
> +    }
> +}
> +
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
>  {
> @@ -1824,6 +1919,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
> 
>
Eduardo Habkost - Aug. 17, 2012, 5:32 p.m.
On Thu, Aug 16, 2012 at 03:10:50PM -0300, Eduardo Habkost wrote:
> On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > --
> > v2:
> >   * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
> >     as suggested by Eduardo Habkost
> > ---
> >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 37ba5ef..440e724 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -606,6 +606,101 @@ 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 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;
> > +    }
> 
> Some feature names are duplicated on feature_names and
> ext_feature_names. On AMD CPU models, we have to set both, on Intel
> models we need to set the bits only on cpuid_features.
> 
> Maybe it's better to:
> 
> 1) eliminate the duplication and set the names only on feature_name
>    array;
> 2) At the end of CPU initialization, set the features on
>    cpuid_ext2_features as copies of the corresponding cpuid_features
>    bits if CPU vendor == AMD (or, maybe, if some boolean
>    "ext2_features_aliases" flag is set, to make it not
>    vendor-dependent).

I implemented the above (after killing "cpudef" support), and pushed to:
https://github.com/ehabkost/qemu-hacks/tree/work/unduplicate-feature-names-v0.4-2012-08-17

I will submit it as an RFC soon.

I also made an experimental rebase of your series on top of that branch,
at:
https://github.com/ehabkost/qemu-hacks/tree/work/cpu-properties-igor-rebase-v3.4-2012-08-17
and also rebased my CPUID code work-in-progress, at:
https://github.com/ehabkost/qemu-hacks/tree/work/cpuid-refactor-v0.9-2012-08-15

> 
> 
> > +
> > +    if (value) {
> > +        *dst_features |= mask;
> > +    } else {
> > +        *dst_features &= ~mask;
> > +    }
> > +}
> > +
> > +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> > +{
> > +    uint32_t bit;
> > +
> > +    for (bit = 0; bit < 32; ++bit) {
> > +        if (featureset[bit]) {
> > +            char *feature_name, *save_ptr;
> > +            char buf[32];
> > +            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
> > +                abort();
> > +            }
> > +            pstrcpy(buf, sizeof(buf), featureset[bit]);
> > +            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);
> 
> What happens when object_property_add() is called twice with the same
> feature name?
> 
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> >                                           const char *name, Error **errp)
> >  {
> > @@ -1824,6 +1919,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
> > 
> > 
> 
> -- 
> Eduardo
>
Igor Mammedov - Aug. 20, 2012, 12:16 p.m.
On Thu, 16 Aug 2012 15:10:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > --
> > v2:
> >   * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
> >     as suggested by Eduardo Habkost
> > ---
> >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 101 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 37ba5ef..440e724 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -606,6 +606,101 @@ 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 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;
> > +    }
> 
> Some feature names are duplicated on feature_names and
> ext_feature_names. On AMD CPU models, we have to set both, on Intel
> models we need to set the bits only on cpuid_features.
> 
> Maybe it's better to:
> 
> 1) eliminate the duplication and set the names only on feature_name
>    array;
> 2) At the end of CPU initialization, set the features on
>    cpuid_ext2_features as copies of the corresponding cpuid_features
>    bits if CPU vendor == AMD (or, maybe, if some boolean
>    "ext2_features_aliases" flag is set, to make it not
>    vendor-dependent).

So far I was trying to keep original behavior and keep scope of this series
only on moving CPU features into properties. I'll rebase this series on top
of what you proposed in a week when I'm back from vacation.
 
> 
> > +
> > +    if (value) {
> > +        *dst_features |= mask;
> > +    } else {
> > +        *dst_features &= ~mask;
> > +    }
> > +}
> > +
> > +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> > +{
> > +    uint32_t bit;
> > +
> > +    for (bit = 0; bit < 32; ++bit) {
> > +        if (featureset[bit]) {
> > +            char *feature_name, *save_ptr;
> > +            char buf[32];
> > +            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
> > +                abort();
> > +            }
> > +            pstrcpy(buf, sizeof(buf), featureset[bit]);
> > +            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);
> 
> What happens when object_property_add() is called twice with the same
> feature name?
Just extra item in list (pure waste). Thanks for pointing out.
I've fixed it (not yet pushed) by checking if property already exists before
adding it.

> 
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> >                                           const char *name, Error **errp)
> >  {
> > @@ -1824,6 +1919,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
> > 
> > 
> 
> -- 
> Eduardo
>
Eduardo Habkost - Aug. 20, 2012, 12:34 p.m.
On Mon, Aug 20, 2012 at 02:16:26PM +0200, Igor Mammedov wrote:
> On Thu, 16 Aug 2012 15:10:50 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Aug 15, 2012 at 06:13:27PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > --
> > > v2:
> > >   * replaced mask/ffs tricks by plain 'for (bit = 0; bit < 32; bit++)'
> > >     as suggested by Eduardo Habkost
> > > ---
> > >  target-i386/cpu.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 101 insertions(+)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 37ba5ef..440e724 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -606,6 +606,101 @@ 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 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;
> > > +    }
> > 
> > Some feature names are duplicated on feature_names and
> > ext_feature_names. On AMD CPU models, we have to set both, on Intel
> > models we need to set the bits only on cpuid_features.
> > 
> > Maybe it's better to:
> > 
> > 1) eliminate the duplication and set the names only on feature_name
> >    array;
> > 2) At the end of CPU initialization, set the features on
> >    cpuid_ext2_features as copies of the corresponding cpuid_features
> >    bits if CPU vendor == AMD (or, maybe, if some boolean
> >    "ext2_features_aliases" flag is set, to make it not
> >    vendor-dependent).
> 
> So far I was trying to keep original behavior and keep scope of this series
> only on moving CPU features into properties. I'll rebase this series on top
> of what you proposed in a week when I'm back from vacation.

Sure, keeping the behavior while changing the code to use properties
makes perfect sense. The only problem is that the current behavior is
incorrect and makes the property handling more complicated, so it makes
sense to fix it first.

>  
> > 
> > > +
> > > +    if (value) {
> > > +        *dst_features |= mask;
> > > +    } else {
> > > +        *dst_features &= ~mask;
> > > +    }
> > > +}
> > > +
> > > +static void x86_register_cpuid_properties(Object *obj, const char **featureset)
> > > +{
> > > +    uint32_t bit;
> > > +
> > > +    for (bit = 0; bit < 32; ++bit) {
> > > +        if (featureset[bit]) {
> > > +            char *feature_name, *save_ptr;
> > > +            char buf[32];
> > > +            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
> > > +                abort();
> > > +            }
> > > +            pstrcpy(buf, sizeof(buf), featureset[bit]);
> > > +            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);
> > 
> > What happens when object_property_add() is called twice with the same
> > feature name?
> Just extra item in list (pure waste). Thanks for pointing out.
> I've fixed it (not yet pushed) by checking if property already exists before
> adding it.
> 
> > 
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
> > >                                           const char *name, Error **errp)
> > >  {
> > > @@ -1824,6 +1919,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
> > > 
> > > 
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 37ba5ef..440e724 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -606,6 +606,101 @@  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 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 bit;
+
+    for (bit = 0; bit < 32; ++bit) {
+        if (featureset[bit]) {
+            char *feature_name, *save_ptr;
+            char buf[32];
+            if (strlen(featureset[bit]) > sizeof(buf) - 1) {
+                abort();
+            }
+            pstrcpy(buf, sizeof(buf), featureset[bit]);
+            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);
+            }
+        }
+    }
+}
+
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
 {
@@ -1824,6 +1919,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;