Re: [Qemu-devel] [PATCH 20/23] target-i386: use properties to set/unset user specified features on CPU

[Eduardo Habkost]

On Tue, Oct 02, 2012 at 05:37:12PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <address@hidden>
> Signed-off-by: Eduardo Habkost <address@hidden>
> ---
> v2:
>   * fix error of initializing from incorrect cpu model definition
>     use x86_cpu_def instead of def. spotted-by: Eduardo Habkost
>   * add missing env in cpu_x86_find_by_name()
>   * added cpu_x86_set_props() to make following code movement more
>     clean. suggested-by: Eduardo Habkost
>   * init name and feature to NULL, to avoid freeing uninitialized mem
> v3:
>   * rebase on top of "x86: Implement SMEP and SMAP"
> ---
>  target-i386/cpu.c |  199 +++++++++++-----------------------------------------
>  1 files changed, 42 insertions(+), 157 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9b3cffd..22a1ded 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -221,25 +221,6 @@ static bool lookup_feature(uint32_t *pval, const char 
> *s, const char *e,
>      return found;
>  }
>  
> -static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features,
> -                                    uint32_t *ext_features,
> -                                    uint32_t *ext2_features,
> -                                    uint32_t *ext3_features,
> -                                    uint32_t *kvm_features,
> -                                    uint32_t *svm_features,
> -                                    uint32_t *cpuid_7_0_ebx_features)
> -{
> -    if (!lookup_feature(features, flagname, NULL, feature_name) &&
> -        !lookup_feature(ext_features, flagname, NULL, ext_feature_name) &&
> -        !lookup_feature(ext2_features, flagname, NULL, ext2_feature_name) &&
> -        !lookup_feature(ext3_features, flagname, NULL, ext3_feature_name) &&
> -        !lookup_feature(kvm_features, flagname, NULL, kvm_feature_name) &&
> -        !lookup_feature(svm_features, flagname, NULL, svm_feature_name) &&
> -        !lookup_feature(cpuid_7_0_ebx_features, flagname, NULL,
> -                        cpuid_7_0_ebx_feature_name))
> -            fprintf(stderr, "CPU feature %s not found\n", flagname);
> -}
> -
>  typedef struct x86_def_t {
>      struct x86_def_t *next;
>      const char *name;
> @@ -847,7 +828,9 @@ static void x86_cpuid_get_feature(Object *obj, Visitor 
> *v, void *opaque,
>         !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)) {
> +       !is_feature_set(name, env->cpuid_svm_features, svm_feature_name) &&
> +       !is_feature_set(name, env->cpuid_7_0_ebx_features,
> +                       cpuid_7_0_ebx_feature_name)) {

If you ever need to respin this series, it would be nice to do this at
patch 07/23 instead.

>          value = false;
>      }
>  
[...]
> @@ -2208,6 +2092,7 @@ static void x86_cpu_initfn(Object *obj)
>      x86_register_cpuid_properties(obj, ext3_feature_name);
>      x86_register_cpuid_properties(obj, kvm_feature_name);
>      x86_register_cpuid_properties(obj, svm_feature_name);
> +    x86_register_cpuid_properties(obj, cpuid_7_0_ebx_feature_name);

This could go to patch 07/23 as well.

It's not a problem to keep it here, however, because the new properties
don't get actually used until this patch is applied.

>  
>      env->cpuid_apic_id = env->cpu_index;
>  
> -- 
> 1.7.1
> 

-- 
Eduardo