[Qemu-devel] [PATCH 0/6] x86 CPU cleanup (wave 2)

[Qemu-devel] [PATCH 0/6] x86 CPU cleanup (wave 2)

[Igor Mammedov]

This series is a several cleanups, moved out from CPU properties series,
since they do not really depend on CPU properties refactoring and could
simplify CPU subclasses work as well.

git tree for testing:
    https://github.com/imammedo/qemu/tree/x86_cpu_cleanup.wave2

Igor Mammedov (6):
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: sanitize AMD's ext2_features at realize time
  target-i386: explicitly set vendor for each built-in cpudef
  target-i386: setting default 'vendor' is obsolete, remove it
  target-i386: move setting defaults out of cpu_x86_parse_featurestr()
  target-i386: move out CPU features initialization in separate func

 target-i386/cpu.c |  147 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 90 insertions(+), 57 deletions(-)

[Qemu-devel] [PATCH 1/6] target-i386: filter out not TCG features if running without kvm at realize time

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   31 ++++++++++++++++---------------
 1 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7be3ad8..63aae86 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1549,21 +1549,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
     }
 
-    if (!kvm_enabled()) {
-        env->cpuid_features &= TCG_FEATURES;
-        env->cpuid_ext_features &= TCG_EXT_FEATURES;
-        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
-#ifdef TARGET_X86_64
-            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
-#endif
-            );
-        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
-        env->cpuid_svm_features &= TCG_SVM_FEATURES;
-    } else {
-#ifdef CONFIG_KVM
-        filter_features_for_kvm(cpu);
-#endif
-    }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_level = 7;
     }
 
+    if (!kvm_enabled()) {
+        env->cpuid_features &= TCG_FEATURES;
+        env->cpuid_ext_features &= TCG_EXT_FEATURES;
+        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
+#ifdef TARGET_X86_64
+            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
+#endif
+            );
+        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
+        env->cpuid_svm_features &= TCG_SVM_FEATURES;
+    } else {
+#ifdef CONFIG_KVM
+        filter_features_for_kvm(cpu);
+#endif
+    }
+
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/6] target-i386: filter out not TCG features if running without kvm at realize time

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:01AM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Just to confirm that this submission still looks good to me:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c |   31 ++++++++++++++++---------------
>  1 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7be3ad8..63aae86 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1549,21 +1549,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
>      }
>  
> -    if (!kvm_enabled()) {
> -        env->cpuid_features &= TCG_FEATURES;
> -        env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
> -#ifdef TARGET_X86_64
> -            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
> -#endif
> -            );
> -        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> -        env->cpuid_svm_features &= TCG_SVM_FEATURES;
> -    } else {
> -#ifdef CONFIG_KVM
> -        filter_features_for_kvm(cpu);
> -#endif
> -    }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> @@ -2077,6 +2062,22 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_level = 7;
>      }
>  
> +    if (!kvm_enabled()) {
> +        env->cpuid_features &= TCG_FEATURES;
> +        env->cpuid_ext_features &= TCG_EXT_FEATURES;
> +        env->cpuid_ext2_features &= (TCG_EXT2_FEATURES
> +#ifdef TARGET_X86_64
> +            | CPUID_EXT2_SYSCALL | CPUID_EXT2_LM
> +#endif
> +            );
> +        env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
> +        env->cpuid_svm_features &= TCG_SVM_FEATURES;
> +    } else {
> +#ifdef CONFIG_KVM
> +        filter_features_for_kvm(cpu);
> +#endif
> +    }
> +
>  #ifndef CONFIG_USER_ONLY
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>  
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov]

when CPU properties are implemented, ext2_features may change
between object_new(CPU) and cpu_realize_fn(). Sanitizing
ext2_features for AMD based CPU at realize() time will keep
current behavior after CPU features are converted to properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - style fix, make line shorter than 80 characters

amd san
---
 target-i386/cpu.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 63aae86..64b7637 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", &error);
 
-    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-     * CPUID[1].EDX.
-     */
-    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
-            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
-            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
-        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
-        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
-    }
-
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_level = 7;
     }
 
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= (env->cpuid_features
+           &  CPUID_EXT2_AMD_ALIASES);
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:02AM +0100, Igor Mammedov wrote:
> when CPU properties are implemented, ext2_features may change
> between object_new(CPU) and cpu_realize_fn(). Sanitizing
> ext2_features for AMD based CPU at realize() time will keep
> current behavior after CPU features are converted to properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   - style fix, make line shorter than 80 characters
> 
> amd san

Incomplete sentence?

> ---
>  target-i386/cpu.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 63aae86..64b7637 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                              "tsc-frequency", &error);
>  
> -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> -     * CPUID[1].EDX.
> -     */
> -    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> -            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> -            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> -        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> -        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
> -    }
> -
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_level = 7;
>      }
>  
> +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> +     * CPUID[1].EDX.
> +     */
> +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {

I would add extra indentation space here, to make it not align with the
statements below, making the condition visually distinct from the body,
like in the original code you are moving.

> +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> +        env->cpuid_ext2_features |= (env->cpuid_features
> +           &  CPUID_EXT2_AMD_ALIASES);

Weird spacing around the "&" above (3 spaces indent, 2 spaces after the
"&").

I would align this as:

        env->cpuid_ext2_features |= (env->cpuid_features &
                                     CPUID_EXT2_AMD_ALIASES);


As the above issues are only cosmetic:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> +    }
> +
>      if (!kvm_enabled()) {
>          env->cpuid_features &= TCG_FEATURES;
>          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -- 
> 1.7.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov]

On Tue, 11 Dec 2012 11:31:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Dec 11, 2012 at 11:11:02AM +0100, Igor Mammedov wrote:
> > when CPU properties are implemented, ext2_features may change
> > between object_new(CPU) and cpu_realize_fn(). Sanitizing
> > ext2_features for AMD based CPU at realize() time will keep
> > current behavior after CPU features are converted to properties.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   - style fix, make line shorter than 80 characters
> > 
> > amd san
> 
> Incomplete sentence?
I forgot to remove comment from squashed in commit with alignment changes,
which I got wrong anyway. I'll fix it and resubmit this patch + update git
tree on github.
Thanks!

> 
> > ---
> >  target-i386/cpu.c |   21 +++++++++++----------
> >  1 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 63aae86..64b7637 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz *
> > 1000, "tsc-frequency", &error);
> >  
> > -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > -     * CPUID[1].EDX.
> > -     */
> > -    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > -            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > -            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> > -        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > -        env->cpuid_ext2_features |= (def->features &
> > CPUID_EXT2_AMD_ALIASES);
> > -    }
> > -
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> > &error); if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >          env->cpuid_level = 7;
> >      }
> >  
> > +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > +     * CPUID[1].EDX.
> > +     */
> > +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> 
> I would add extra indentation space here, to make it not align with the
> statements below, making the condition visually distinct from the body,
> like in the original code you are moving.
> 
> > +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > +        env->cpuid_ext2_features |= (env->cpuid_features
> > +           &  CPUID_EXT2_AMD_ALIASES);
> 
> Weird spacing around the "&" above (3 spaces indent, 2 spaces after the
> "&").
> 
> I would align this as:
> 
>         env->cpuid_ext2_features |= (env->cpuid_features &
>                                      CPUID_EXT2_AMD_ALIASES);
> 
> 
> As the above issues are only cosmetic:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> > +    }
> > +
> >      if (!kvm_enabled()) {
> >          env->cpuid_features &= TCG_FEATURES;
> >          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> > -- 
> > 1.7.1
> > 
> 

Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov]

On Tue, 11 Dec 2012 11:31:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

[...]
> > ---
> >  target-i386/cpu.c |   21 +++++++++++----------
> >  1 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 63aae86..64b7637 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz *
> > 1000, "tsc-frequency", &error);
> >  
> > -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > -     * CPUID[1].EDX.
> > -     */
> > -    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > -            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > -            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> > -        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > -        env->cpuid_ext2_features |= (def->features &
> > CPUID_EXT2_AMD_ALIASES);
> > -    }
> > -
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> > &error); if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
> >          env->cpuid_level = 7;
> >      }
> >  
> > +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > +     * CPUID[1].EDX.
> > +     */
> > +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> 
> I would add extra indentation space here, to make it not align with the
> statements below, making the condition visually distinct from the body,
> like in the original code you are moving.
I've thought people would object to ident here, that's why I've changed
original indentation to a more consistent with rules.

BTW: git grep -A 3 "if (.*[^{)]$"
shows that many places use this style including fairly recent ones:
aio-posix.c first hit

and we have in target-i386/cpu.c
if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &
...
with the same alignment style and at least one more similar.
Lets leave it this way to be consistent with the rest of the code.

> 
> > +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> > +        env->cpuid_ext2_features |= (env->cpuid_features
> > +           &  CPUID_EXT2_AMD_ALIASES);
> 
> Weird spacing around the "&" above (3 spaces indent, 2 spaces after the
> "&").
> 
> I would align this as:
> 
>         env->cpuid_ext2_features |= (env->cpuid_features &
>                                      CPUID_EXT2_AMD_ALIASES);
Thanks, fixed.
> 
> 
> As the above issues are only cosmetic:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> > +    }
> > +
> >      if (!kvm_enabled()) {
> >          env->cpuid_features &= TCG_FEATURES;
> >          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> > -- 
> > 1.7.1
> > 
> 

Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 03:08:56PM +0100, Igor Mammedov wrote:
[...]
> > > +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> > > +     * CPUID[1].EDX.
> > > +     */
> > > +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> > > +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> > > +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> > 
> > I would add extra indentation space here, to make it not align with the
> > statements below, making the condition visually distinct from the body,
> > like in the original code you are moving.
> I've thought people would object to ident here, that's why I've changed
> original indentation to a more consistent with rules.
> 
> BTW: git grep -A 3 "if (.*[^{)]$"
> shows that many places use this style including fairly recent ones:
> aio-posix.c first hit
> 
> and we have in target-i386/cpu.c
> if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &
> ...
> with the same alignment style and at least one more similar.
> Lets leave it this way to be consistent with the rest of the code.

No problem. I was expressing personal preference, not knowing what was
the preferred/usual style in QEMU. My only data point was the original
code you moved.  :-)

-- 
Eduardo

[Qemu-devel] [PATCH 2/6 v3] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov]

when CPU properties are implemented, ext2_features may change
between object_new(CPU) and cpu_realize_fn(). Sanitizing
ext2_features for AMD based CPU at realize() time will keep
current behavior after CPU features are converted to properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v3:
  - commit msg cleanup
  - ident fix
v2:
  - style fix, make line shorter than 80 characters
---
 target-i386/cpu.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 63aae86..bc10cfa 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
                             "tsc-frequency", &error);
 
-    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-     * CPUID[1].EDX.
-     */
-    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
-            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
-            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
-        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
-        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
-    }
-
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
@@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_level = 7;
     }
 
+    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
+     * CPUID[1].EDX.
+     */
+    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
+        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
+        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
+        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
+        env->cpuid_ext2_features |= (env->cpuid_features &
+                                     CPUID_EXT2_AMD_ALIASES);
+    }
+
     if (!kvm_enabled()) {
         env->cpuid_features &= TCG_FEATURES;
         env->cpuid_ext_features &= TCG_EXT_FEATURES;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/6 v3] target-i386: sanitize AMD's ext2_features at realize time

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 03:18:25PM +0100, Igor Mammedov wrote:
> when CPU properties are implemented, ext2_features may change
> between object_new(CPU) and cpu_realize_fn(). Sanitizing
> ext2_features for AMD based CPU at realize() time will keep
> current behavior after CPU features are converted to properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

To confirm that this submission is OK to me as well:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
> v3:
>   - commit msg cleanup
>   - ident fix
> v2:
>   - style fix, make line shorter than 80 characters
> ---
>  target-i386/cpu.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 63aae86..bc10cfa 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
>                              "tsc-frequency", &error);
>  
> -    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> -     * CPUID[1].EDX.
> -     */
> -    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> -            env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> -            env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> -        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> -        env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES);
> -    }
> -
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_level = 7;
>      }
>  
> +    /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
> +     * CPUID[1].EDX.
> +     */
> +    if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 &&
> +        env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 &&
> +        env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) {
> +        env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES;
> +        env->cpuid_ext2_features |= (env->cpuid_features &
> +                                     CPUID_EXT2_AMD_ALIASES);
> +    }
> +
>      if (!kvm_enabled()) {
>          env->cpuid_features &= TCG_FEATURES;
>          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 3/6] target-i386: explicitly set vendor for each built-in cpudef

[Igor Mammedov]

it will help to get rid of setting default.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 64b7637..1497980 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -387,6 +387,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "core2duo",
         .level = 10,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -431,6 +434,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu32",
         .level = 4,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -441,6 +447,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm32",
         .level = 5,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -455,6 +464,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "coreduo",
         .level = 10,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -470,6 +482,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "486",
         .level = 1,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -479,6 +494,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium",
         .level = 1,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -488,6 +506,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium2",
         .level = 2,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -497,6 +518,9 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "pentium3",
         .level = 2,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -522,6 +546,9 @@ static x86_def_t builtin_x86_defs[] = {
         .name = "n270",
         /* original is on level 10 */
         .level = 5,
+        .vendor1 = CPUID_VENDOR_INTEL_1,
+        .vendor2 = CPUID_VENDOR_INTEL_2,
+        .vendor3 = CPUID_VENDOR_INTEL_3,
         .family = 6,
         .model = 28,
         .stepping = 2,
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/6] target-i386: explicitly set vendor for each built-in cpudef

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:03AM +0100, Igor Mammedov wrote:
> it will help to get rid of setting default.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 64b7637..1497980 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -387,6 +387,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "core2duo",
>          .level = 10,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 15,
>          .stepping = 11,
> @@ -431,6 +434,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "qemu32",
>          .level = 4,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 3,
>          .stepping = 3,
> @@ -441,6 +447,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "kvm32",
>          .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -455,6 +464,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "coreduo",
>          .level = 10,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 14,
>          .stepping = 8,
> @@ -470,6 +482,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "486",
>          .level = 1,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 4,
>          .model = 0,
>          .stepping = 0,
> @@ -479,6 +494,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium",
>          .level = 1,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 5,
>          .model = 4,
>          .stepping = 3,
> @@ -488,6 +506,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium2",
>          .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 5,
>          .stepping = 2,
> @@ -497,6 +518,9 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "pentium3",
>          .level = 2,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 7,
>          .stepping = 3,
> @@ -522,6 +546,9 @@ static x86_def_t builtin_x86_defs[] = {
>          .name = "n270",
>          /* original is on level 10 */
>          .level = 5,
> +        .vendor1 = CPUID_VENDOR_INTEL_1,
> +        .vendor2 = CPUID_VENDOR_INTEL_2,
> +        .vendor3 = CPUID_VENDOR_INTEL_3,
>          .family = 6,
>          .model = 28,
>          .stepping = 2,
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 4/6] target-i386: setting default 'vendor' is obsolete, remove it

[Igor Mammedov]

since cpu_def config is not supported anymore and all remainig sources now
always set x86_def_t.vendor[123] fields remove setting default vendor to
simplify future refactoring.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1497980..99fd3f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,15 +1539,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-    if (def->vendor1) {
-        env->cpuid_vendor1 = def->vendor1;
-        env->cpuid_vendor2 = def->vendor2;
-        env->cpuid_vendor3 = def->vendor3;
-    } else {
-        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
-        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
-        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
-    }
+    assert(def->vendor1);
+    env->cpuid_vendor1 = def->vendor1;
+    env->cpuid_vendor2 = def->vendor2;
+    env->cpuid_vendor3 = def->vendor3;
     env->cpuid_vendor_override = def->vendor_override;
     object_property_set_int(OBJECT(cpu), def->level, "level", &error);
     object_property_set_int(OBJECT(cpu), def->family, "family", &error);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/6] target-i386: setting default 'vendor' is obsolete, remove it

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:04AM +0100, Igor Mammedov wrote:
> since cpu_def config is not supported anymore and all remainig sources now
> always set x86_def_t.vendor[123] fields remove setting default vendor to
> simplify future refactoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1497980..99fd3f3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,15 +1539,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -    if (def->vendor1) {
> -        env->cpuid_vendor1 = def->vendor1;
> -        env->cpuid_vendor2 = def->vendor2;
> -        env->cpuid_vendor3 = def->vendor3;
> -    } else {
> -        env->cpuid_vendor1 = CPUID_VENDOR_INTEL_1;
> -        env->cpuid_vendor2 = CPUID_VENDOR_INTEL_2;
> -        env->cpuid_vendor3 = CPUID_VENDOR_INTEL_3;
> -    }
> +    assert(def->vendor1);
> +    env->cpuid_vendor1 = def->vendor1;
> +    env->cpuid_vendor2 = def->vendor2;
> +    env->cpuid_vendor3 = def->vendor3;
>      env->cpuid_vendor_override = def->vendor_override;
>      object_property_set_int(OBJECT(cpu), def->level, "level", &error);
>      object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 5/6] target-i386: move setting defaults out of cpu_x86_parse_featurestr()

[Igor Mammedov]

No functional change, needed for simplifying convertion to properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 99fd3f3..e534254 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1264,7 +1264,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     /* Features to be added */
     uint32_t plus_features = 0, plus_ext_features = 0;
     uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
-    uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
+    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
     uint32_t plus_7_0_ebx_features = 0;
     /* Features to be removed */
     uint32_t minus_features = 0, minus_ext_features = 0;
@@ -1273,10 +1273,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
 
-    add_flagname_to_bitmaps("hypervisor", &plus_features,
-            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
-            &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
-
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
@@ -1536,6 +1532,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         goto error;
     }
 
+    def->kvm_features |= kvm_default_features;
+    add_flagname_to_bitmaps("hypervisor", &def->features,
+                            &def->ext_features, &def->ext2_features,
+                            &def->ext3_features, &def->kvm_features,
+                            &def->svm_features, &def->cpuid_7_0_ebx_features);
+
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/6] target-i386: move setting defaults out of cpu_x86_parse_featurestr()

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:05AM +0100, Igor Mammedov wrote:
> No functional change, needed for simplifying convertion to properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 99fd3f3..e534254 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1264,7 +1264,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      /* Features to be added */
>      uint32_t plus_features = 0, plus_ext_features = 0;
>      uint32_t plus_ext2_features = 0, plus_ext3_features = 0;
> -    uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0;
> +    uint32_t plus_kvm_features = 0, plus_svm_features = 0;
>      uint32_t plus_7_0_ebx_features = 0;
>      /* Features to be removed */
>      uint32_t minus_features = 0, minus_ext_features = 0;
> @@ -1273,10 +1273,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      uint32_t minus_7_0_ebx_features = 0;
>      uint32_t numvalue;
>  
> -    add_flagname_to_bitmaps("hypervisor", &plus_features,
> -            &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
> -            &plus_kvm_features, &plus_svm_features,  &plus_7_0_ebx_features);
> -
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
> @@ -1536,6 +1532,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          goto error;
>      }
>  
> +    def->kvm_features |= kvm_default_features;
> +    add_flagname_to_bitmaps("hypervisor", &def->features,
> +                            &def->ext_features, &def->ext2_features,
> +                            &def->ext3_features, &def->kvm_features,
> +                            &def->svm_features, &def->cpuid_7_0_ebx_features);
> +
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 6/6] target-i386: move out CPU features initialization in separate func

[Igor Mammedov]

No functional change, a simple code movement to simplify following refactoring.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 v2:
  - rebased on top of  "i386: cpu: remove duplicate feature names"
      http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
 v3:
  - rebased on top of 1.3-rc0 & splitted cpu_x86_find_by_name()
  - AMD's ext2_features filtering are moved into separate patch
---
 target-i386/cpu.c |   53 ++++++++++++++++++++++++++++++-----------------------
 1 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e534254..3b9bbfe 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1235,6 +1235,34 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
+static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
+{
+    CPUX86State *env = &cpu->env;
+
+    assert(def->vendor1);
+    env->cpuid_vendor1 = def->vendor1;
+    env->cpuid_vendor2 = def->vendor2;
+    env->cpuid_vendor3 = def->vendor3;
+    env->cpuid_vendor_override = def->vendor_override;
+    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
+    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
+    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
+    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
+    env->cpuid_features = def->features;
+    env->cpuid_ext_features = def->ext_features;
+    env->cpuid_ext2_features = def->ext2_features;
+    env->cpuid_ext3_features = def->ext3_features;
+    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
+    env->cpuid_kvm_features = def->kvm_features;
+    env->cpuid_svm_features = def->svm_features;
+    env->cpuid_ext4_features = def->ext4_features;
+    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
+    env->cpuid_xlevel2 = def->xlevel2;
+    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
+                            "tsc-frequency", errp);
+    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
+}
+
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
     x86_def_t *def;
@@ -1513,7 +1541,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
 
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
-    CPUX86State *env = &cpu->env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
     char *name, *features;
@@ -1541,29 +1568,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     if (cpu_x86_parse_featurestr(def, features) < 0) {
         goto error;
     }
-    assert(def->vendor1);
-    env->cpuid_vendor1 = def->vendor1;
-    env->cpuid_vendor2 = def->vendor2;
-    env->cpuid_vendor3 = def->vendor3;
-    env->cpuid_vendor_override = def->vendor_override;
-    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
-    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
-    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
-    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
-    env->cpuid_features = def->features;
-    env->cpuid_ext_features = def->ext_features;
-    env->cpuid_ext2_features = def->ext2_features;
-    env->cpuid_ext3_features = def->ext3_features;
-    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
-    env->cpuid_kvm_features = def->kvm_features;
-    env->cpuid_svm_features = def->svm_features;
-    env->cpuid_ext4_features = def->ext4_features;
-    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
-    env->cpuid_xlevel2 = def->xlevel2;
-    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
-                            "tsc-frequency", &error);
 
-    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+    cpudef_2_x86_cpu(cpu, def, &error);
+
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 6/6] target-i386: move out CPU features initialization in separate func

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 11:11:06AM +0100, Igor Mammedov wrote:
> No functional change, a simple code movement to simplify following refactoring.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  v2:
>   - rebased on top of  "i386: cpu: remove duplicate feature names"
>       http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html
>  v3:
>   - rebased on top of 1.3-rc0 & splitted cpu_x86_find_by_name()
>   - AMD's ext2_features filtering are moved into separate patch
> ---
>  target-i386/cpu.c |   53 ++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e534254..3b9bbfe 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1235,6 +1235,34 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> +static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
> +{
> +    CPUX86State *env = &cpu->env;
> +
> +    assert(def->vendor1);
> +    env->cpuid_vendor1 = def->vendor1;
> +    env->cpuid_vendor2 = def->vendor2;
> +    env->cpuid_vendor3 = def->vendor3;
> +    env->cpuid_vendor_override = def->vendor_override;
> +    object_property_set_int(OBJECT(cpu), def->level, "level", errp);
> +    object_property_set_int(OBJECT(cpu), def->family, "family", errp);
> +    object_property_set_int(OBJECT(cpu), def->model, "model", errp);
> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
> +    env->cpuid_features = def->features;
> +    env->cpuid_ext_features = def->ext_features;
> +    env->cpuid_ext2_features = def->ext2_features;
> +    env->cpuid_ext3_features = def->ext3_features;
> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
> +    env->cpuid_kvm_features = def->kvm_features;
> +    env->cpuid_svm_features = def->svm_features;
> +    env->cpuid_ext4_features = def->ext4_features;
> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> +    env->cpuid_xlevel2 = def->xlevel2;
> +    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> +                            "tsc-frequency", errp);
> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
> +}
> +
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
>      x86_def_t *def;
> @@ -1513,7 +1541,6 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>      char *name, *features;
> @@ -1541,29 +1568,9 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
>          goto error;
>      }
> -    assert(def->vendor1);
> -    env->cpuid_vendor1 = def->vendor1;
> -    env->cpuid_vendor2 = def->vendor2;
> -    env->cpuid_vendor3 = def->vendor3;
> -    env->cpuid_vendor_override = def->vendor_override;
> -    object_property_set_int(OBJECT(cpu), def->level, "level", &error);
> -    object_property_set_int(OBJECT(cpu), def->family, "family", &error);
> -    object_property_set_int(OBJECT(cpu), def->model, "model", &error);
> -    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", &error);
> -    env->cpuid_features = def->features;
> -    env->cpuid_ext_features = def->ext_features;
> -    env->cpuid_ext2_features = def->ext2_features;
> -    env->cpuid_ext3_features = def->ext3_features;
> -    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", &error);
> -    env->cpuid_kvm_features = def->kvm_features;
> -    env->cpuid_svm_features = def->svm_features;
> -    env->cpuid_ext4_features = def->ext4_features;
> -    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> -    env->cpuid_xlevel2 = def->xlevel2;
> -    object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000,
> -                            "tsc-frequency", &error);
>  
> -    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +    cpudef_2_x86_cpu(cpu, def, &error);
> +
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -- 
> 1.7.1
> 

-- 
Eduardo