[Qemu-devel] [PATCH 00/20 v2] x86 CPU cleanup (wave 2)

[Qemu-devel] [PATCH 00/20 v2] x86 CPU cleanup (wave 2)

[Igor Mammedov]

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

Series doesn't depend on cpu as device or any other series, and applies
to current master.

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

v2:
 - cleanup commit message  and style fixes in
     [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time
 - extracted more patches [07-20] from cpu properties series, that were
   more cleanups and code reorganizing than conversion to static properties.

Igor Mammedov (20):
  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_x86_register() consolidate freeing resources
  target-i386: compile kvm only functions if CONFIG_KVM is defined
  target-i386: move kvm_check_features_against_host() check to realize
    time
  target-i386: prepare cpu_x86_parse_featurestr() to return a set of
    key,value property pairs
  target-i386: do not set vendor_override in x86_cpuid_set_vendor()
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: convert [cpuid_]vendor_override to bool
  target-i386: set custom 'vendor' without intermediate x86_def_t
  target-i386: set custom 'xlevel' without intermediate x86_def_t
  target-i386: set custom 'level' without intermediate x86_def_t
  target-i386: set custom 'model-id' without intermediate x86_def_t
  target-i386: set custom 'stepping' without intermediate x86_def_t
  target-i386: set custom 'model' without intermediate x86_def_t
  target-i386: set custom 'family' without intermediate x86_def_t

 target-i386/cpu.c |  338 ++++++++++++++++++++++++-----------------------------
 target-i386/cpu.h |    8 +-
 2 files changed, 158 insertions(+), 188 deletions(-)

[Qemu-devel] [PATCH 01/20] 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

[Qemu-devel] [PATCH 02/20] 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>
---
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..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

[Qemu-devel] [PATCH 03/20] 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>
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

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

[Igor Mammedov]

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

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

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

[Igor Mammedov]

No functional change, needed for simplifying conversion 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

[Qemu-devel] [PATCH 06/20] 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>
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 & split cpu_x86_find_by_name()
  - AMD's ext2_features filtering is 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

[Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..418c899 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1567,19 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.1

[Qemu-devel] [PATCH 08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 418c899..24bfd95 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -862,7 +862,6 @@ static int cpu_x86_fill_model_id(char *str)
     }
     return 0;
 }
-#endif
 
 /* Fill a x86_def_t struct with information about the host CPU, and
  * the CPU features supported by the host hardware + host kernel
@@ -871,7 +870,6 @@ static int cpu_x86_fill_model_id(char *str)
  */
 static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 {
-#ifdef CONFIG_KVM
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
@@ -930,7 +928,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
      * unsupported ones later.
      */
     x86_cpu_def->svm_features = -1;
-#endif /* CONFIG_KVM */
 }
 
 static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
@@ -981,6 +978,7 @@ static int kvm_check_features_against_host(x86_def_t *guest_def)
                 }
     return rv;
 }
+#endif /* CONFIG_KVM */
 
 static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
                                          const char *name, Error **errp)
@@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         }
     }
     if (kvm_enabled() && name && strcmp(name, "host") == 0) {
+#ifdef CONFIG_KVM
         kvm_cpu_fill_host(x86_cpu_def);
+#endif
     } else if (!def) {
         return -1;
     } else {
@@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
+#ifdef CONFIG_KVM
     if (check_cpuid && kvm_enabled()) {
         if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
             goto error;
     }
+#endif
     return 0;
 
 error:
-- 
1.7.1

[Qemu-devel] [PATCH 09/20] target-i386: move kvm_check_features_against_host() check to realize time

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 24bfd95..e075b59 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -951,19 +951,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
  *
  * This function may be called only if KVM is enabled.
  */
-static int kvm_check_features_against_host(x86_def_t *guest_def)
+static int kvm_check_features_against_host(X86CPU *cpu)
 {
+    CPUX86State *env = &cpu->env;
     x86_def_t host_def;
     uint32_t mask;
     int rv, i;
     struct model_features_t ft[] = {
-        {&guest_def->features, &host_def.features,
+        {&env->cpuid_features, &host_def.features,
             ~0, feature_name, 0x00000000},
-        {&guest_def->ext_features, &host_def.ext_features,
+        {&env->cpuid_ext_features, &host_def.ext_features,
             ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
-        {&guest_def->ext2_features, &host_def.ext2_features,
+        {&env->cpuid_ext2_features, &host_def.ext2_features,
             ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
-        {&guest_def->ext3_features, &host_def.ext3_features,
+        {&env->cpuid_ext3_features, &host_def.ext3_features,
             ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
 
     assert(kvm_enabled());
@@ -1428,12 +1429,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
-#ifdef CONFIG_KVM
-    if (check_cpuid && kvm_enabled()) {
-        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
-            goto error;
-    }
-#endif
     return 0;
 
 error:
@@ -2106,6 +2101,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     } else {
 #ifdef CONFIG_KVM
+        if (check_cpuid && kvm_check_features_against_host(cpu)
+            && enforce_cpuid) {
+            error_setg(errp, "Host's CPU doesn't support requested features");
+            return;
+        }
+
         filter_features_for_kvm(cpu);
 #endif
     }
-- 
1.7.1

[Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

It prepares for converting "+feature,-feature,feature=foo,feature" into
a set of key,value property pairs that will be applied to CPU by
cpu_x86_set_props().

Each feature handled by cpu_x86_parse_featurestr() will be converted into
foo,val pair and a corresponding property setter by following patches.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e075b59..a74d74b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
     return 0;
 }
 
+/* Set features on X86CPU object based on a provide key,value list */
+static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
+{
+    const QDictEntry *ent;
+
+    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
+        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
+        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
+                              qdict_entry_key(ent), errp);
+        if (error_is_set(errp)) {
+            return;
+        }
+    }
+}
+
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
+static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
+                                    QDict **props)
 {
     unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
@@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     uint32_t minus_kvm_features = 0, minus_svm_features = 0;
     uint32_t minus_7_0_ebx_features = 0;
     uint32_t numvalue;
+    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
+    *props = qdict_new();
+    int j = 0;
 
-    featurestr = features ? strtok(features, ",") : NULL;
-
-    while (featurestr) {
+    while ((featurestr = feat_array[j++])) {
         char *val;
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, &plus_features,
@@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
-        featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
     x86_cpu_def->ext_features |= plus_ext_features;
@@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
     x86_cpu_def->kvm_features &= ~minus_kvm_features;
     x86_cpu_def->svm_features &= ~minus_svm_features;
     x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
+    g_strfreev(feat_array);
     return 0;
 
 error:
+    g_strfreev(feat_array);
     return -1;
 }
 
@@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     x86_def_t def1, *def = &def1;
+    QDict *props = NULL;
     Error *error = NULL;
     char *name, *features;
     gchar **model_pieces;
@@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->ext3_features, &def->kvm_features,
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
-    if (cpu_x86_parse_featurestr(def, features) < 0) {
+    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
         error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
         goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
+    cpu_x86_set_props(cpu, props, &error);
 
 out:
+    QDECREF(props);
     g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
-- 
1.7.1

[Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Igor Mammedov]

commit d480e1af which introduced vendor property was setting
env->cpuid_vendor_override = 1, which prevents using vendor property
on its own without triggering vendor override.
Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
to allow to use vendor property in other places that doesn't require
cpuid_vendor_override to be set to 1.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a74d74b..c6c074f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1163,7 +1163,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
         env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
         env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
     }
-    env->cpuid_vendor_override = 1;
 }
 
 static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
-- 
1.7.1

[Qemu-devel] [PATCH 12/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

Vendor property setter takes string as vendor value but cpudefs
use uint32_t vendor[123] fields to define vendor value. It makes it
difficult to unify and use property setter for values from cpudefs.

Simplify code by using vendor property setter, vendor[123] fields
are converted into vendor[13] array to keep its value. And vendor
property setter is used to access/set value on CPU.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
  v2:
    - fix vendor field convertion for a new Haswell & Opteron_G5 cpu models
    - fix  vendor field convertion for cpu models that had implicit vendor via
      default value.
---
 target-i386/cpu.c |  131 ++++++++++++++---------------------------------------
 target-i386/cpu.h |    6 +-
 2 files changed, 38 insertions(+), 99 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c6c074f..ed67de7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -275,7 +275,7 @@ typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
     uint32_t level;
-    uint32_t vendor1, vendor2, vendor3;
+    char vendor[CPUID_VENDOR_SZ + 1];
     int family;
     int model;
     int stepping;
@@ -340,9 +340,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -359,9 +357,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "phenom",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 16,
         .model = 2,
         .stepping = 3,
@@ -387,9 +383,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -408,9 +402,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm64",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -434,9 +426,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -447,9 +437,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -464,9 +452,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -482,9 +468,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -494,9 +478,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -506,9 +488,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -518,9 +498,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -530,9 +508,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "athlon",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -546,9 +522,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 28,
         .stepping = 2,
@@ -567,9 +541,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Conroe",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -587,9 +559,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Penryn",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -608,9 +578,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Nehalem",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -629,9 +597,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Westmere",
         .level = 11,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 44,
         .stepping = 1,
@@ -651,9 +617,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "SandyBridge",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 42,
         .stepping = 1,
@@ -676,9 +640,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Haswell",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 60,
         .stepping = 1,
@@ -706,9 +668,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G1",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -730,9 +690,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G2",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -756,9 +714,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G3",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -784,9 +740,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G4",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 1,
         .stepping = 2,
@@ -816,9 +770,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G5",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 2,
         .stepping = 0,
@@ -872,14 +824,17 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 {
     KVMState *s = kvm_state;
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+    int i;
 
     assert(kvm_enabled());
 
     x86_cpu_def->name = "host";
     host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_def->vendor1 = ebx;
-    x86_cpu_def->vendor2 = edx;
-    x86_cpu_def->vendor3 = ecx;
+    for (i = 0; i < 4; i++) {
+        x86_cpu_def->vendor[i] = ebx >> (8 * i);
+        x86_cpu_def->vendor[i + 4] = edx >> (8 * i);
+        x86_cpu_def->vendor[i + 8] = ecx >> (8 * i);
+    }
 
     host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
@@ -907,9 +862,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
-    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
-        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
-        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
+    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
         host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
@@ -1237,10 +1190,8 @@ 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;
+    assert(def->vendor[0]);
+    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     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);
@@ -1303,7 +1254,6 @@ static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
 static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                                     QDict **props)
 {
-    unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
     uint32_t plus_features = 0, plus_ext_features = 0;
@@ -1378,18 +1328,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                if (strlen(val) != 12) {
-                    fprintf(stderr, "vendor string must be 12 chars long\n");
-                    goto error;
-                }
-                x86_cpu_def->vendor1 = 0;
-                x86_cpu_def->vendor2 = 0;
-                x86_cpu_def->vendor3 = 0;
-                for(i = 0; i < 4; i++) {
-                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
-                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
-                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
-                }
+                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
                 x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..fbbe730 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -515,14 +515,14 @@
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
 #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
 #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
 #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
 
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.1

[Qemu-devel] [PATCH 13/20] target-i386: convert [cpuid_]vendor_override to bool

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ed67de7..2b7272a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -284,7 +284,7 @@ typedef struct x86_def_t {
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
-    int vendor_override;
+    bool vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -859,7 +859,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
                 kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
-    x86_cpu_def->vendor_override = 0;
+    x86_cpu_def->vendor_override = false;
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
@@ -1329,7 +1329,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
+                x86_cpu_def->vendor_override = true;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
                         val);
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fbbe730..0247253 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -812,7 +812,7 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
+    bool cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.1

[Qemu-devel] [PATCH 14/20] target-i386: set custom 'vendor' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 2b7272a..0cb0931 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1328,7 +1328,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
+                qdict_put(*props, featurestr, qstring_from_str(val));
                 x86_cpu_def->vendor_override = true;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-- 
1.7.1

[Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0cb0931..714ae79 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 if (numvalue < 0x80000000) {
                     numvalue += 0x80000000;
                 }
-                x86_cpu_def->xlevel = numvalue;
+                val = g_strdup_printf("%u", numvalue);
+                qdict_put(*props, featurestr, qstring_from_str(val));
+                g_free(val);
             } else if (!strcmp(featurestr, "vendor")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
                 x86_cpu_def->vendor_override = true;
-- 
1.7.1

[Qemu-devel] [PATCH 16/20] target-i386: set custom 'level' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 714ae79..a66bdd8 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1309,13 +1309,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->stepping = numvalue ;
             } else if (!strcmp(featurestr, "level")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->level = numvalue;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "xlevel")) {
                 char *err;
                 numvalue = strtoul(val, &err, 0);
-- 
1.7.1

[Qemu-devel] [PATCH 17/20] target-i386: set custom 'model-id' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a66bdd8..b4b1e70 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1327,8 +1327,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 qdict_put(*props, featurestr, qstring_from_str(val));
                 x86_cpu_def->vendor_override = true;
             } else if (!strcmp(featurestr, "model_id")) {
-                pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
-                        val);
+                qdict_put(*props, "model-id", qstring_from_str(val));
             } else if (!strcmp(featurestr, "tsc_freq")) {
                 int64_t tsc_freq;
                 char *err;
-- 
1.7.1

[Qemu-devel] [PATCH 18/20] target-i386: set custom 'stepping' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b4b1e70..eb01bb0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1301,13 +1301,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->model = numvalue;
             } else if (!strcmp(featurestr, "stepping")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->stepping = numvalue ;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "level")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "xlevel")) {
-- 
1.7.1

[Qemu-devel] [PATCH 19/20] target-i386: set custom 'model' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index eb01bb0..59e2491 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1293,13 +1293,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->family = numvalue;
             } else if (!strcmp(featurestr, "model")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->model = numvalue;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "stepping")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "level")) {
-- 
1.7.1

[Qemu-devel] [PATCH 20/20] target-i386: set custom 'family' without intermediate x86_def_t

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 59e2491..de7f4e5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1285,13 +1285,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             if (!strcmp(featurestr, "family")) {
-                char *err;
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err || numvalue > 0xff + 0xf) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
-                    goto error;
-                }
-                x86_cpu_def->family = numvalue;
+                qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "model")) {
                 qdict_put(*props, featurestr, qstring_from_str(val));
             } else if (!strcmp(featurestr, "stepping")) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH 00/20 v2] x86 CPU cleanup (wave 2)

[Andreas Färber]

Am 17.12.2012 17:01, schrieb Igor Mammedov:
> This series is several cleanups, moved out from CPU properties series,
> since they do not really depend on CPU properties re-factoring and could
> simplify CPU subclasses work as well.

The initial cleanups (most of which I had already reviewed) look
promising. But some others are working around issues that subclasses
solve. And I would like to keep functional changes visible.

If we go and explicitly set the vendor everywhere as you suggest, do we
even still need the vendor override flag at all?

Andreas

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

Re: [Qemu-devel] [PATCH 00/20 v2] x86 CPU cleanup (wave 2)

[Igor Mammedov]

On Mon, 17 Dec 2012 21:43:33 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 17.12.2012 17:01, schrieb Igor Mammedov:
> > This series is several cleanups, moved out from CPU properties series,
> > since they do not really depend on CPU properties re-factoring and could
> > simplify CPU subclasses work as well.
> 
> The initial cleanups (most of which I had already reviewed) look
> promising. But some others are working around issues that subclasses
> solve. And I would like to keep functional changes visible.
Could you be more specific, please?

This series is aimed to separate as much as possible (without converting
everything to properties first) initial CPU initialization from a found
cpu-model /will become classes/ and user supplied values for features which
will be gradually transformed to canonical form of key,value pairs and applied
through properties.

Following on cpu properties series is going property-fy remaining features
that are not properties yet and convert current dynamic properties into 
static properties. It's practically complete, I've just haven't completed
patch-wise testing yet, you can see it here:
https://github.com/imammedo/qemu/tree/x86-cpu-properties.WIP

> If we go and explicitly set the vendor everywhere as you suggest, do we
> even still need the vendor override flag at all?
vendor_override is used on kvm only when user wishes to force
custom vendor value on the guest, built-in models don't do it now, otherwise
guest always gets host's vendor value (see get_cpuid_vendor()).


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


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:19PM +0100, Igor Mammedov wrote:
> freeing resources in one place would require setting 'error'
> to not NULL, so add some more error reporting before jumping to
> exit branch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[...]
> +out:
> +    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -        goto error;
>      }
> -
> -    g_strfreev(model_pieces);


You are making the function return 0 on errors. Is it on purpose?


>      return 0;
> -error:
> -    g_strfreev(model_pieces);
> -    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

On Tue, 18 Dec 2012 13:28:29 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:19PM +0100, Igor Mammedov wrote:
> > freeing resources in one place would require setting 'error'
> > to not NULL, so add some more error reporting before jumping to
> > exit branch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [...]
> > +out:
> > +    g_strfreev(model_pieces);
> >      if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> >          error_free(error);
> > -        goto error;
> >      }
> > -
> > -    g_strfreev(model_pieces);
> 
> 
> You are making the function return 0 on errors. Is it on purpose?
Nope, I'm sorry it seems I've lost return -1; in error branch on the way.

> 
> 
> >      return 0;
> > -error:
> > -    g_strfreev(model_pieces);
> > -    return -1;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -- 
> > 1.7.1
> > 
> > 
> 

[Qemu-devel] [PATCH 07/20 v2] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
   - add missing 'return -1' on exit if error is not NULL,
           Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..fe8b76c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1567,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
+        return -1;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/20 v2] target-i386: cpu_x86_register() consolidate freeing resources

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 05:30:43PM +0100, Igor Mammedov wrote:
> freeing resources in one place would require setting 'error'
> to not NULL, so add some more error reporting before jumping to
> exit branch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>    - add missing 'return -1' on exit if error is not NULL,
>            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3b9bbfe..fe8b76c 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> -        goto error;
> +        goto out;

Missing error_set*() call here.

The rest of the patch looks good, to me. I liked this style of handling
errors & freeing resources.


>      }
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
>      if (cpu_x86_find_by_name(def, name) < 0) {
> -        goto error;
> +        error_setg(&error, "Unable to find CPU definition: %s", name);
> +        goto out;
>      }
>  
>      def->kvm_features |= kvm_default_features;
> @@ -1566,22 +1567,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        goto error;
> +        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> +        goto out;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
>  
> +out:
> +    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -        goto error;
> +        return -1;
>      }
> -
> -    g_strfreev(model_pieces);
>      return 0;
> -error:
> -    g_strfreev(model_pieces);
> -    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote:
[...]
>  
>  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void *opaque,
>                                           const char *name, Error **errp)
> @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          }
>      }
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> +#ifdef CONFIG_KVM
>          kvm_cpu_fill_host(x86_cpu_def);
> +#endif

Is this really better than the existing code that generates an empty
stub function (that will never be called anyway)?

I am not strongly inclined either way, but I prefer the existing style.


>      } else if (!def) {
>          return -1;
>      } else {
> @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      x86_cpu_def->kvm_features &= ~minus_kvm_features;
>      x86_cpu_def->svm_features &= ~minus_svm_features;
>      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> +#ifdef CONFIG_KVM
>      if (check_cpuid && kvm_enabled()) {
>          if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
>              goto error;
>      }
> +#endif
>      return 0;
>  
>  error:
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 09/20] target-i386: move kvm_check_features_against_host() check to realize time

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:21PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

I really like this one, because it makes it easy to unify
filter_features_for_kvm() and kvm_check_features_against_host() later.


> ---
>  target-i386/cpu.c |   23 ++++++++++++-----------
>  1 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 24bfd95..e075b59 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -951,19 +951,20 @@ static int unavailable_host_feature(struct model_features_t *f, uint32_t mask)
>   *
>   * This function may be called only if KVM is enabled.
>   */
> -static int kvm_check_features_against_host(x86_def_t *guest_def)
> +static int kvm_check_features_against_host(X86CPU *cpu)
>  {
> +    CPUX86State *env = &cpu->env;
>      x86_def_t host_def;
>      uint32_t mask;
>      int rv, i;
>      struct model_features_t ft[] = {
> -        {&guest_def->features, &host_def.features,
> +        {&env->cpuid_features, &host_def.features,
>              ~0, feature_name, 0x00000000},
> -        {&guest_def->ext_features, &host_def.ext_features,
> +        {&env->cpuid_ext_features, &host_def.ext_features,
>              ~CPUID_EXT_HYPERVISOR, ext_feature_name, 0x00000001},
> -        {&guest_def->ext2_features, &host_def.ext2_features,
> +        {&env->cpuid_ext2_features, &host_def.ext2_features,
>              ~PPRO_FEATURES, ext2_feature_name, 0x80000000},
> -        {&guest_def->ext3_features, &host_def.ext3_features,
> +        {&env->cpuid_ext3_features, &host_def.ext3_features,
>              ~CPUID_EXT3_SVM, ext3_feature_name, 0x80000001}};
>  
>      assert(kvm_enabled());
> @@ -1428,12 +1429,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      x86_cpu_def->kvm_features &= ~minus_kvm_features;
>      x86_cpu_def->svm_features &= ~minus_svm_features;
>      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> -#ifdef CONFIG_KVM
> -    if (check_cpuid && kvm_enabled()) {
> -        if (kvm_check_features_against_host(x86_cpu_def) && enforce_cpuid)
> -            goto error;
> -    }
> -#endif
>      return 0;
>  
>  error:
> @@ -2106,6 +2101,12 @@ void x86_cpu_realize(Object *obj, Error **errp)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      } else {
>  #ifdef CONFIG_KVM
> +        if (check_cpuid && kvm_check_features_against_host(cpu)
> +            && enforce_cpuid) {
> +            error_setg(errp, "Host's CPU doesn't support requested features");
> +            return;
> +        }
> +
>          filter_features_for_kvm(cpu);
>  #endif
>      }
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20 v2] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

On Wed, 19 Dec 2012 14:36:28 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Dec 18, 2012 at 05:30:43PM +0100, Igor Mammedov wrote:
> > freeing resources in one place would require setting 'error'
> > to not NULL, so add some more error reporting before jumping to
> > exit branch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >    - add missing 'return -1' on exit if error is not NULL,
> >            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |   17 ++++++++---------
> >  1 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3b9bbfe..fe8b76c 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) 
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > -        goto error;
> > +        goto out;
> 
> Missing error_set*() call here.
Thinking about this error path, it looks like it's unreachable, because of
every caller of cpu_init() passes in not empty cpu_model.
Perhaps it would make sense to just assert(cpu_model) at the beginning of
function and delete this error check.

> 
> The rest of the patch looks good, to me. I liked this style of handling
> errors & freeing resources.
> 
> 
[snip]

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> It prepares for converting "+feature,-feature,feature=foo,feature" into
> a set of key,value property pairs that will be applied to CPU by
> cpu_x86_set_props().
> 
> Each feature handled by cpu_x86_parse_featurestr() will be converted into
> foo,val pair and a corresponding property setter by following patches.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
object as parameter and set the properties directly? Or you can see some
use case where saving the property-setting dictionary for later use will
be useful?

(This will probably require calling cpudef_2_x86_cpu() before
cpu_x86_parse_featurestr(), and changing the existing
cpu_x86_parse_featurestr() code to change the cpu object, not a
x86_def_t struct, but I think this will simplify the logic a lot)

> ---
>  target-i386/cpu.c |   33 +++++++++++++++++++++++++++------
>  1 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index e075b59..a74d74b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>      return 0;
>  }
>  
> +/* Set features on X86CPU object based on a provide key,value list */
> +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> +{
> +    const QDictEntry *ent;
> +
> +    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
> +        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> +        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> +                              qdict_entry_key(ent), errp);
> +        if (error_is_set(errp)) {
> +            return;
> +        }
> +    }
> +}
> +
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> +                                    QDict **props)
>  {
>      unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
> @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      uint32_t minus_kvm_features = 0, minus_svm_features = 0;
>      uint32_t minus_7_0_ebx_features = 0;
>      uint32_t numvalue;
> +    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
> +    *props = qdict_new();
> +    int j = 0;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> -
> -    while (featurestr) {
> +    while ((featurestr = feat_array[j++])) {
>          char *val;
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>              goto error;
>          }
> -        featurestr = strtok(NULL, ",");
>      }
>      x86_cpu_def->features |= plus_features;
>      x86_cpu_def->ext_features |= plus_ext_features;
> @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
>      x86_cpu_def->kvm_features &= ~minus_kvm_features;
>      x86_cpu_def->svm_features &= ~minus_svm_features;
>      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> +    g_strfreev(feat_array);
>      return 0;
>  
>  error:
> +    g_strfreev(feat_array);
>      return -1;
>  }
>  
> @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  {
>      x86_def_t def1, *def = &def1;
> +    QDict *props = NULL;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
> @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              &def->ext3_features, &def->kvm_features,
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
> -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> +    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
>          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
>          goto out;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
> +    cpu_x86_set_props(cpu, props, &error);
>  
>  out:
> +    QDECREF(props);
>      g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20 v2] target-i386: cpu_x86_register() consolidate freeing resources

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 05:49:22PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 14:36:28 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Dec 18, 2012 at 05:30:43PM +0100, Igor Mammedov wrote:
> > > freeing resources in one place would require setting 'error'
> > > to not NULL, so add some more error reporting before jumping to
> > > exit branch.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >   v2:
> > >    - add missing 'return -1' on exit if error is not NULL,
> > >            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target-i386/cpu.c |   17 ++++++++---------
> > >  1 files changed, 8 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 3b9bbfe..fe8b76c 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char
> > > *cpu_model) 
> > >      model_pieces = g_strsplit(cpu_model, ",", 2);
> > >      if (!model_pieces[0]) {
> > > -        goto error;
> > > +        goto out;
> > 
> > Missing error_set*() call here.
> Thinking about this error path, it looks like it's unreachable, because of
> every caller of cpu_init() passes in not empty cpu_model.

Not empty, or just non-NULL? I just hit the check above using -cpu "".

Note that the above check catches both NULL and empty cpu_model strings,
because g_strsplit("", ...) returns an empty array.


> Perhaps it would make sense to just assert(cpu_model) at the beginning of
> function and delete this error check.
> 
> > 
> > The rest of the patch looks good, to me. I liked this style of handling
> > errors & freeing resources.
> > 
> > 
> [snip]
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

[Igor Mammedov]

On Wed, 19 Dec 2012 14:42:31 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote:
> [...]
> >  
> >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
> > *opaque, const char *name, Error **errp)
> > @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name) }
> >      }
> >      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> > +#ifdef CONFIG_KVM
> >          kvm_cpu_fill_host(x86_cpu_def);
> > +#endif
> 
> Is this really better than the existing code that generates an empty
> stub function (that will never be called anyway)?
Following patch that moves kvm_check_features_against_host() inside
of #ifdef CONFIG_KVM in realize_fn(), makes build failure *-user with
warning that kvm_check_features_against_host() is unused and if
we make stub from kvm_check_features_against_host() as well then we will have
to ifdef unavailable_host_feature() as well. As result it makes +2 more
ifdefs one of which excludes whole function.

This patch makes one big ifdef block of kvm specific functions and the next
one keep amount of ifdef the same as before these patches.

And as bonus we get cleaner build and won't get unused symbols & calls to
empty functions on debug builds.

> 
> I am not strongly inclined either way, but I prefer the existing style.
> 
> 
> >      } else if (!def) {
> >          return -1;
> >      } else {
> > @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > *x86_cpu_def, char *features) x86_cpu_def->kvm_features &=
> > ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features;
> >      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > +#ifdef CONFIG_KVM
> >      if (check_cpuid && kvm_enabled()) {
> >          if (kvm_check_features_against_host(x86_cpu_def) &&
> > enforce_cpuid) goto error;
> >      }
> > +#endif
> >      return 0;
> >  
> >  error:
> > -- 
> > 1.7.1
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 07/20 v2] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

On Wed, 19 Dec 2012 15:04:37 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Dec 19, 2012 at 05:49:22PM +0100, Igor Mammedov wrote:
> > On Wed, 19 Dec 2012 14:36:28 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Tue, Dec 18, 2012 at 05:30:43PM +0100, Igor Mammedov wrote:
> > > > freeing resources in one place would require setting 'error'
> > > > to not NULL, so add some more error reporting before jumping to
> > > > exit branch.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >   v2:
> > > >    - add missing 'return -1' on exit if error is not NULL,
> > > >            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > >  target-i386/cpu.c |   17 ++++++++---------
> > > >  1 files changed, 8 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 3b9bbfe..fe8b76c 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1550,13 +1550,14 @@ int cpu_x86_register(X86CPU *cpu, const char
> > > > *cpu_model) 
> > > >      model_pieces = g_strsplit(cpu_model, ",", 2);
> > > >      if (!model_pieces[0]) {
> > > > -        goto error;
> > > > +        goto out;
> > > 
> > > Missing error_set*() call here.
> > Thinking about this error path, it looks like it's unreachable, because of
> > every caller of cpu_init() passes in not empty cpu_model.
> 
> Not empty, or just non-NULL? I just hit the check above using -cpu "".
> 
> Note that the above check catches both NULL and empty cpu_model strings,
> because g_strsplit("", ...) returns an empty array.
I see, I'll add error message here then.


Thanks!

> 
> 
> > Perhaps it would make sense to just assert(cpu_model) at the beginning of
> > function and delete this error check.
> > 
> > > 
> > > The rest of the patch looks good, to me. I liked this style of handling
> > > errors & freeing resources.
> > > 
> > > 
> > [snip]
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:23PM +0100, Igor Mammedov wrote:
> commit d480e1af which introduced vendor property was setting
> env->cpuid_vendor_override = 1, which prevents using vendor property
> on its own without triggering vendor override.
> Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
> to allow to use vendor property in other places that doesn't require
> cpuid_vendor_override to be set to 1.

By making "vendor" not force override, you are making "-cpu vendor=xxx"
behave differently from setting "vendor" using all other interfaces
(e.g. -device, -global, QMP commands).

What about taking the opposite approach? Setting "vendor" could always
force vendor override, but the code that initialize the defaults would
take care of not overriding the vendor ID if unsafe. e.g.: it could just
do this:

 if (!kvm_enabled() || def->vendor_override) {
   object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
 } /* else, leave the "vendor" property untouched" */

(something equivalent could be done inside class_init() when we
introduce subclasses)

On all I cases I can think of somebody setting the "vendor" property
(e.g. using -cpu, QMP, -device, or -global), it means they want vendor
override (otherwise, what's the point of setting the property?). Setting
vendor in no-override mode is the special case, not the other way
around.

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a74d74b..c6c074f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1163,7 +1163,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
>          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
>      }
> -    env->cpuid_vendor_override = 1;
>  }
>  
>  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t

[Eduardo Habkost]

On Mon, Dec 17, 2012 at 05:01:27PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 0cb0931..714ae79 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  if (numvalue < 0x80000000) {
>                      numvalue += 0x80000000;
>                  }
> -                x86_cpu_def->xlevel = numvalue;
> +                val = g_strdup_printf("%u", numvalue);

Why not just eliminate the string->integer conversion entirely, and move
it inside x86_cpuid_set_xlevel()?

> +                qdict_put(*props, featurestr, qstring_from_str(val));

If you made cpu_x86_parse_featurestr() set the properties directly on
the object instead of creating an intermediate directionary, it could
simply call object_property_set_int() (if for some reason it's
impossible to eliminate the string->int conversion above).

> +                g_free(val);
>              } else if (!strcmp(featurestr, "vendor")) {
>                  qdict_put(*props, featurestr, qstring_from_str(val));
>                  x86_cpu_def->vendor_override = true;
> -- 
> 1.7.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Andreas Färber]

Am 19.12.2012 18:38, schrieb Eduardo Habkost:
> On Mon, Dec 17, 2012 at 05:01:23PM +0100, Igor Mammedov wrote:
>> commit d480e1af which introduced vendor property was setting
>> env->cpuid_vendor_override = 1, which prevents using vendor property
>> on its own without triggering vendor override.
>> Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
>> to allow to use vendor property in other places that doesn't require
>> cpuid_vendor_override to be set to 1.
> 
> By making "vendor" not force override, you are making "-cpu vendor=xxx"
> behave differently from setting "vendor" using all other interfaces
> (e.g. -device, -global, QMP commands).
> 
> What about taking the opposite approach? Setting "vendor" could always
> force vendor override, but the code that initialize the defaults would
> take care of not overriding the vendor ID if unsafe. e.g.: it could just
> do this:
> 
>  if (!kvm_enabled() || def->vendor_override) {
>    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>  } /* else, leave the "vendor" property untouched" */
> 
> (something equivalent could be done inside class_init() when we
> introduce subclasses)
> 
> On all I cases I can think of somebody setting the "vendor" property
> (e.g. using -cpu, QMP, -device, or -global), it means they want vendor
> override (otherwise, what's the point of setting the property?). Setting
> vendor in no-override mode is the special case, not the other way
> around.

+1

I was thinking it might be possible to just reset vendor_override to
false when set internally via property - I didn't get to trying out
whether there is a second place affected though.

Andreas

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

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

On Wed, 19 Dec 2012 14:54:30 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > It prepares for converting "+feature,-feature,feature=foo,feature" into
> > a set of key,value property pairs that will be applied to CPU by
> > cpu_x86_set_props().
> > 
> > Each feature handled by cpu_x86_parse_featurestr() will be converted into
> > foo,val pair and a corresponding property setter by following patches.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> object as parameter and set the properties directly? Or you can see some
> use case where saving the property-setting dictionary for later use will
> be useful?

I plan to use cpu_x86_parse_featurestr() + list of properties later when we
have properties and subclasses in place. Then it would be possible to
transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of global
properties.

In the end the option handler for -cpu XXX,... could be changed into:

cpu_opt_parser() {
    // hides legacy target specific ugliness 
    target_XXX_cpu_compat_parser_callback() {
        cpu_class_name = get_class_name(optval)

        // dumb compatibility parser, which converts old format into
        // canonical form feature,val property list
        prop_list = parse_featurestr(optval)

        // with classes and global properties we could get rid of the field
        // cpu_model_str in CPUxxxState
        return prop_list, cpu_class_name        
    }

    foreach (prop_list)
        add_global_prop(cpu_class_name,prop,val)

    // could be later transformed to property of board object and
    // we could get rid of one more global var
    cpu_model = cpu_class_name
}

> 
> (This will probably require calling cpudef_2_x86_cpu() before
> cpu_x86_parse_featurestr(), and changing the existing
> cpu_x86_parse_featurestr() code to change the cpu object, not a
> x86_def_t struct, but I think this will simplify the logic a lot)
You cannot set/uset +-feat directly on CPU without breaking current
behavior where -feat overrides all +feat no matter in what order they are
specified. That's why dictionary is used to easily maintain "if(not feat)
ignore" logic and avoid duplication. Pls, look at commit
https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
that converts +-feat into canonical feat=on/off form.

And if features are applied directly to CPU, it would require to another
rewrite if global properties are to be used. Which IMHO should be eventually
done since -cpu ... looks like global parameters for a specific cpu type.

> 
> > ---
> >  target-i386/cpu.c |   33 +++++++++++++++++++++++++++------
> >  1 files changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e075b59..a74d74b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> >      return 0;
> >  }
> >  
> > +/* Set features on X86CPU object based on a provide key,value list */
> > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> > +{
> > +    const QDictEntry *ent;
> > +
> > +    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
> > +        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> > +        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> > +                              qdict_entry_key(ent), errp);
> > +        if (error_is_set(errp)) {
> > +            return;
> > +        }
> > +    }
> > +}
> > +
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> > +                                    QDict **props)
> >  {
> >      unsigned int i;
> >      char *featurestr; /* Single 'key=value" string being parsed */
> > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> >      uint32_t minus_7_0_ebx_features = 0;
> >      uint32_t numvalue;
> > +    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
> > +    *props = qdict_new();
> > +    int j = 0;
> >  
> > -    featurestr = features ? strtok(features, ",") : NULL;
> > -
> > -    while (featurestr) {
> > +    while ((featurestr = feat_array[j++])) {
> >          char *val;
> >          if (featurestr[0] == '+') {
> >              add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> >              goto error;
> >          }
> > -        featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> >      x86_cpu_def->ext_features |= plus_ext_features;
> > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> >      x86_cpu_def->kvm_features &= ~minus_kvm_features;
> >      x86_cpu_def->svm_features &= ~minus_svm_features;
> >      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > +    g_strfreev(feat_array);
> >      return 0;
> >  
> >  error:
> > +    g_strfreev(feat_array);
> >      return -1;
> >  }
> >  
> > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  {
> >      x86_def_t def1, *def = &def1;
> > +    QDict *props = NULL;
> >      Error *error = NULL;
> >      char *name, *features;
> >      gchar **model_pieces;
> > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >                              &def->ext3_features, &def->kvm_features,
> >                              &def->svm_features, &def->cpuid_7_0_ebx_features);
> >  
> > -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> > +    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
> >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> >          goto out;
> >      }
> >  
> >      cpudef_2_x86_cpu(cpu, def, &error);
> > +    cpu_x86_set_props(cpu, props, &error);
> >  
> >  out:
> > +    QDECREF(props);
> >      g_strfreev(model_pieces);
> >      if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > -- 
> > 1.7.1
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 15/20] target-i386: set custom 'xlevel' without intermediate x86_def_t

[Igor Mammedov]

On Wed, 19 Dec 2012 15:58:40 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:27PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 0cb0931..714ae79 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> >                  if (numvalue < 0x80000000) {
> >                      numvalue += 0x80000000;
> >                  }
> > -                x86_cpu_def->xlevel = numvalue;
> > +                val = g_strdup_printf("%u", numvalue);
> 
> Why not just eliminate the string->integer conversion entirely, and move
> it inside x86_cpuid_set_xlevel()?
Silent fixup of an incorrect input value doesn't look like a candidate for
making into property setter, more correct way is bailing out. But for
compatibility sake we can't just remove it without giving userbase a warning.
Perhaps candidate for next release notes.
And when it's removed, this branch could be collapsed in catch-all(feat=val)
branch + adding input value check to xlevel property setter.
 
> 
> > +                qdict_put(*props, featurestr, qstring_from_str(val));
> 
> If you made cpu_x86_parse_featurestr() set the properties directly on
> the object instead of creating an intermediate directionary, it could
> simply call object_property_set_int() (if for some reason it's
> impossible to eliminate the string->int conversion above).
I've explained in answer to [10/20] why.
Besides normalizing input first to uniform set and then applying it
to object using only one approach whether it via cpu_x86_set_props() or
global properties seems to me more straightforward and reduces chance of error.
and might allow completely get rid of cpu_x86_parse_featurestr() call in
cpu_init() call chain. cpu_x86_parse_featurestr() should/might be called only
once to create a set of properties that are applied later to each created CPU.

> 
> > +                g_free(val);
> >              } else if (!strcmp(featurestr, "vendor")) {
> >                  qdict_put(*props, featurestr, qstring_from_str(val));
> >                  x86_cpu_def->vendor_override = true;
> > -- 
> > 1.7.1
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Igor Mammedov]

On Wed, 19 Dec 2012 15:38:09 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:23PM +0100, Igor Mammedov wrote:
> > commit d480e1af which introduced vendor property was setting
> > env->cpuid_vendor_override = 1, which prevents using vendor property
> > on its own without triggering vendor override.
> > Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
> > to allow to use vendor property in other places that doesn't require
> > cpuid_vendor_override to be set to 1.
> 
> By making "vendor" not force override, you are making "-cpu vendor=xxx"
                                                         ^^^^^^^^^^^^^^^
old behavior is taken care in cpu_x86_parse_featurestr()
> behave differently from setting "vendor" using all other interfaces
> (e.g. -device, -global, QMP commands).
all other users do not exits for|use CPU yet, so we have a chance to new
behavior there.
 
> 
> What about taking the opposite approach? Setting "vendor" could always
> force vendor override, but the code that initialize the defaults would
> take care of not overriding the vendor ID if unsafe. e.g.: it could just
> do this:
> 
>  if (!kvm_enabled() || def->vendor_override) {
>    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>  } /* else, leave the "vendor" property untouched" */
Unless it's placed in some class_init() I would strongly object, because
it introduces extra hardcoded initialization step between
object_new()..realize_fn().
> 
> (something equivalent could be done inside class_init() when we
> introduce subclasses)
> 
> On all I cases I can think of somebody setting the "vendor" property
> (e.g. using -cpu, QMP, -device, or -global), it means they want vendor
> override (otherwise, what's the point of setting the property?). Setting
> vendor in no-override mode is the special case, not the other way
> around.
Partly it's true,
currently vendor_override has meaning only for kvm guests and default vendor
value guest see changes as following:

1. tcg mode: guest always sees built-in or user provided vendor value,
             vendor_override has no effect here, we could assume it's true
        * and then vendor property setting it always to true is fine.
2. kvm mode: by default guest doesn't see built-in vendor value (it sees
             host's value instead), setting custom vendor value from command
             line currently makes guest to see vendor value that are kept env.
        * this is not OK with vendor property setting it always to true.

Perhaps we could in class_x86xxx_init() use host's vendor value as default
instead of built-in cpu_def's one if kvm_enabled()==true and remove
vendor_override field altogether.
It will keep default behavior the same as before and provide a real picture
of what guest will see by default on class introspection.

I'll post patch in several minutes.

> 
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index a74d74b..c6c074f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1163,7 +1163,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> >          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> >          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> >      }
> > -    env->cpuid_vendor_override = 1;
> >  }
> >  
> >  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > -- 
> > 1.7.1
> > 
> > 
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

[Qemu-devel] target-i386: Remove *vendor_override fields from x86_def_t and CPUX86State

[Igor Mammedov]

Makes code cleaner and avoids headache with 'vendor' property.

replaces previous patches:
  [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()
  [PATCH 13/20] target-i386: convert [cpuid_]vendor_override to bool
with:
  [PATCH 11/20] target-i386: add x86cpu_vendor_words2str()
  [PATCH 13/20] target-i386: remove vendor_override field from CPUX86State
ammends:
  [PATCH 12/20] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Qemu-devel] [PATCH 11/20] target-i386: add x86cpu_vendor_words2str()

[Igor Mammedov]

Make for() cycle reusable for the next patch

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a74d74b..06b991e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -847,6 +847,18 @@ static x86_def_t builtin_x86_defs[] = {
     },
 };
 
+static void x86cpu_vendor_words2str(char *dst, uint32_t ebx, uint32_t ecx,
+                                    uint32_t edx)
+{
+    int i;
+    for (i = 0; i < 4; i++) {
+        dst[i] = ebx >> (8 * i);
+        dst[i + 4] = edx >> (8 * i);
+        dst[i + 8] = ecx >> (8 * i);
+    }
+    dst[CPUID_VENDOR_SZ] = '\0';
+}
+
 #ifdef CONFIG_KVM
 static int cpu_x86_fill_model_id(char *str)
 {
@@ -1130,15 +1142,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
     char *value;
-    int i;
 
     value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
-    for (i = 0; i < 4; i++) {
-        value[i    ] = env->cpuid_vendor1 >> (8 * i);
-        value[i + 4] = env->cpuid_vendor2 >> (8 * i);
-        value[i + 8] = env->cpuid_vendor3 >> (8 * i);
-    }
-    value[CPUID_VENDOR_SZ] = '\0';
+    x86cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
+                            env->cpuid_vendor3);
     return value;
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 12/20 v2] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Igor Mammedov]

Vendor property setter takes string as vendor value but cpudefs
use uint32_t vendor[123] fields to define vendor value. It makes it
difficult to unify and use property setter for values from cpudefs.

Simplify code by using vendor property setter, vendor[123] fields
are converted into vendor[13] array to keep its value. And vendor
property setter is used to access/set value on CPU.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
  v3:
    - no functional change, instead of duplicating for() cycle reuse
      x86cpu_vendor_words2str()
  v2:
    - fix vendor field convertion for a new Haswell & Opteron_G5 cpu models
    - fix  vendor field convertion for cpu models that had implicit vendor via
      default value.
---
 target-i386/cpu.c | 127 +++++++++++++-----------------------------------------
 target-i386/cpu.h |   6 +--
 2 files changed, 33 insertions(+), 100 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 06b991e..a96aa33 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -275,7 +275,7 @@ typedef struct x86_def_t {
     struct x86_def_t *next;
     const char *name;
     uint32_t level;
-    uint32_t vendor1, vendor2, vendor3;
+    char vendor[CPUID_VENDOR_SZ + 1];
     int family;
     int model;
     int stepping;
@@ -340,9 +340,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "qemu64",
         .level = 4,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -359,9 +357,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "phenom",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 16,
         .model = 2,
         .stepping = 3,
@@ -387,9 +383,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 15,
         .stepping = 11,
@@ -408,9 +402,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "kvm64",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -434,9 +426,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 3,
         .stepping = 3,
@@ -447,9 +437,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -464,9 +452,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 14,
         .stepping = 8,
@@ -482,9 +468,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 4,
         .model = 0,
         .stepping = 0,
@@ -494,9 +478,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 5,
         .model = 4,
         .stepping = 3,
@@ -506,9 +488,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 5,
         .stepping = 2,
@@ -518,9 +498,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 7,
         .stepping = 3,
@@ -530,9 +508,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "athlon",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -546,9 +522,7 @@ 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,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 28,
         .stepping = 2,
@@ -567,9 +541,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Conroe",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -587,9 +559,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Penryn",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -608,9 +578,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Nehalem",
         .level = 2,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 2,
         .stepping = 3,
@@ -629,9 +597,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Westmere",
         .level = 11,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 44,
         .stepping = 1,
@@ -651,9 +617,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "SandyBridge",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 42,
         .stepping = 1,
@@ -676,9 +640,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Haswell",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_INTEL_1,
-        .vendor2 = CPUID_VENDOR_INTEL_2,
-        .vendor3 = CPUID_VENDOR_INTEL_3,
+        .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 60,
         .stepping = 1,
@@ -706,9 +668,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G1",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -730,9 +690,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G2",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -756,9 +714,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G3",
         .level = 5,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 15,
         .model = 6,
         .stepping = 1,
@@ -784,9 +740,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G4",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 1,
         .stepping = 2,
@@ -816,9 +770,7 @@ static x86_def_t builtin_x86_defs[] = {
     {
         .name = "Opteron_G5",
         .level = 0xd,
-        .vendor1 = CPUID_VENDOR_AMD_1,
-        .vendor2 = CPUID_VENDOR_AMD_2,
-        .vendor3 = CPUID_VENDOR_AMD_3,
+        .vendor = CPUID_VENDOR_AMD,
         .family = 21,
         .model = 2,
         .stepping = 0,
@@ -888,10 +840,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     assert(kvm_enabled());
 
     x86_cpu_def->name = "host";
-    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-    x86_cpu_def->vendor1 = ebx;
-    x86_cpu_def->vendor2 = edx;
-    x86_cpu_def->vendor3 = ecx;
+    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
 
     host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
     x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
@@ -919,9 +868,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
-    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
-        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
-        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
+    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
         host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
@@ -1245,10 +1192,8 @@ 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;
+    assert(def->vendor[0]);
+    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
     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);
@@ -1311,7 +1256,6 @@ static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
 static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                                     QDict **props)
 {
-    unsigned int i;
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
     uint32_t plus_features = 0, plus_ext_features = 0;
@@ -1386,18 +1330,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 }
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
-                if (strlen(val) != 12) {
-                    fprintf(stderr, "vendor string must be 12 chars long\n");
-                    goto error;
-                }
-                x86_cpu_def->vendor1 = 0;
-                x86_cpu_def->vendor2 = 0;
-                x86_cpu_def->vendor3 = 0;
-                for(i = 0; i < 4; i++) {
-                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
-                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
-                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
-                }
+                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
                 x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..fbbe730 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -515,14 +515,14 @@
 #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
 #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
 #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
+#define CPUID_VENDOR_INTEL "GenuineIntel"
 
 #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
 #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
 #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
+#define CPUID_VENDOR_AMD   "AuthenticAMD"
 
-#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
-#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
-#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
+#define CPUID_VENDOR_VIA   "CentaurHauls"
 
 #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
 #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
-- 
1.7.11.7

[Qemu-devel] [PATCH 13/20] target-i386: remove vendor_override field from CPUX86State

[Igor Mammedov]

commit 8935499831312 makes cpuid return to guest host's vendor value
instead of built-in one by default if kvm_enabled() == true and allows
to override this behavior if 'vendor' is specified on -cpu command line.

But every time guest calls cpuid to get 'vendor' value, host's value is
read again and again in default case.

It complicates semantic of vendor property and makes it harder to use,
due to split brain syndrome, lets simplify it.

Instead of reading 'vendor' value from host every time cpuid[vendor] is
called, override 'vendor' value only once in cpu_x86_find_by_name(), when
built-in CPU model is found and if(kvm_enabled() == true).

It provides the same default semantic
 if (kvm_enabled() == true)  vendor = host's vendor
 else vendor = built-in vendor

and then later:
 if (custom vendor) vendor = custom vendor

'vendor' value is overridden when user provides it on -cpu command line,
and there isn't need in vendor_override field anymore, remove it.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a96aa33..a12d938 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -284,7 +284,6 @@ typedef struct x86_def_t {
     uint32_t kvm_features, svm_features;
     uint32_t xlevel;
     char model_id[48];
-    int vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t ext4_features;
     uint32_t xlevel2;
@@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
                 kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
-    x86_cpu_def->vendor_override = 0;
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
@@ -1117,7 +1115,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
         env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
         env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
     }
-    env->cpuid_vendor_override = 1;
 }
 
 static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
@@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
 
     assert(def->vendor[0]);
     object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
-    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);
@@ -1231,6 +1227,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
         return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
+        if (kvm_enabled()) {
+            uint32_t  ebx = 0, ecx = 0, edx = 0;
+            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
+            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
+        }
     }
 
     return 0;
@@ -1331,7 +1332,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
                 x86_cpu_def->xlevel = numvalue;
             } else if (!strcmp(featurestr, "vendor")) {
                 pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
-                x86_cpu_def->vendor_override = 1;
             } else if (!strcmp(featurestr, "model_id")) {
                 pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
                         val);
@@ -1582,16 +1582,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
     *ebx = env->cpuid_vendor1;
     *edx = env->cpuid_vendor2;
     *ecx = env->cpuid_vendor3;
-
-    /* sysenter isn't supported on compatibility mode on AMD, syscall
-     * isn't supported in compatibility mode on Intel.
-     * Normally we advertise the actual cpu vendor, but you can override
-     * this if you want to use KVM's sysenter/syscall emulation
-     * in compatibility mode and when doing cross vendor migration
-     */
-    if (kvm_enabled() && ! env->cpuid_vendor_override) {
-        host_cpuid(0, 0, NULL, ebx, ecx, edx);
-    }
 }
 
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fbbe730..a15a09e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -812,7 +812,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_ext2_features;
     uint32_t cpuid_ext3_features;
     uint32_t cpuid_apic_id;
-    int cpuid_vendor_override;
     /* Store the results of Centaur's CPUID instructions */
     uint32_t cpuid_xlevel2;
     uint32_t cpuid_ext4_features;
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 11/20] target-i386: do not set vendor_override in x86_cpuid_set_vendor()

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 11:47:00PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 15:38:09 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Dec 17, 2012 at 05:01:23PM +0100, Igor Mammedov wrote:
> > > commit d480e1af which introduced vendor property was setting
> > > env->cpuid_vendor_override = 1, which prevents using vendor property
> > > on its own without triggering vendor override.
> > > Fix it by removing setting cpuid_vendor_override in x86_cpuid_set_vendor()
> > > to allow to use vendor property in other places that doesn't require
> > > cpuid_vendor_override to be set to 1.
> > 
> > By making "vendor" not force override, you are making "-cpu vendor=xxx"
>                                                          ^^^^^^^^^^^^^^^
> old behavior is taken care in cpu_x86_parse_featurestr()
> > behave differently from setting "vendor" using all other interfaces
> > (e.g. -device, -global, QMP commands).
> all other users do not exits for|use CPU yet, so we have a chance to new
> behavior there.

The point is that the new behavior wouldn't make much sense: what's the
point of setting the vendor property and not getting the vendor ID
actually exposed to the guest?


>  
> > 
> > What about taking the opposite approach? Setting "vendor" could always
> > force vendor override, but the code that initialize the defaults would
> > take care of not overriding the vendor ID if unsafe. e.g.: it could just
> > do this:
> > 
> >  if (!kvm_enabled() || def->vendor_override) {
> >    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
> >  } /* else, leave the "vendor" property untouched" */
> Unless it's placed in some class_init() I would strongly object, because
> it introduces extra hardcoded initialization step between
> object_new()..realize_fn().

It wouldn't be "hardcoded initialization", it would be just code inside
instance_init(), that's supposed to have code inside it, too (but,
anyway, we probably can put that inside class_init).

> > 
> > (something equivalent could be done inside class_init() when we
> > introduce subclasses)
> > 
> > On all I cases I can think of somebody setting the "vendor" property
> > (e.g. using -cpu, QMP, -device, or -global), it means they want vendor
> > override (otherwise, what's the point of setting the property?). Setting
> > vendor in no-override mode is the special case, not the other way
> > around.
> Partly it's true,
> currently vendor_override has meaning only for kvm guests and default vendor
> value guest see changes as following:
> 
> 1. tcg mode: guest always sees built-in or user provided vendor value,
>              vendor_override has no effect here, we could assume it's true
>         * and then vendor property setting it always to true is fine.
> 2. kvm mode: by default guest doesn't see built-in vendor value (it sees
>              host's value instead), setting custom vendor value from command
>              line currently makes guest to see vendor value that are kept env.
>         * this is not OK with vendor property setting it always to true.
> 
> Perhaps we could in class_x86xxx_init() use host's vendor value as default
> instead of built-in cpu_def's one if kvm_enabled()==true and remove
> vendor_override field altogether.

This exactly what I suggested above, if you remove the
def->vendor_override check (that won't be necessary if all predefined
CPU models have vendor_override=false).


> It will keep default behavior the same as before and provide a real picture
> of what guest will see by default on class introspection.

Exactly.

> 
> I'll post patch in several minutes.
> 
> > 
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c |    1 -
> > >  1 files changed, 0 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index a74d74b..c6c074f 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1163,7 +1163,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> > >          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> > >          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> > >      }
> > > -    env->cpuid_vendor_override = 1;
> > >  }
> > >  
> > >  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 
> > -- 
> > Eduardo
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [PATCH 13/20] target-i386: remove vendor_override field from CPUX86State

[Eduardo Habkost]

On Thu, Dec 20, 2012 at 01:16:31AM +0100, Igor Mammedov wrote:
> commit 8935499831312 makes cpuid return to guest host's vendor value
> instead of built-in one by default if kvm_enabled() == true and allows
> to override this behavior if 'vendor' is specified on -cpu command line.
> 
> But every time guest calls cpuid to get 'vendor' value, host's value is
> read again and again in default case.
> 
> It complicates semantic of vendor property and makes it harder to use,
> due to split brain syndrome, lets simplify it.
> 
> Instead of reading 'vendor' value from host every time cpuid[vendor] is
> called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> built-in CPU model is found and if(kvm_enabled() == true).
> 
> It provides the same default semantic
>  if (kvm_enabled() == true)  vendor = host's vendor
>  else vendor = built-in vendor
> 
> and then later:
>  if (custom vendor) vendor = custom vendor
> 
> 'vendor' value is overridden when user provides it on -cpu command line,
> and there isn't need in vendor_override field anymore, remove it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 20 +++++---------------
>  target-i386/cpu.h |  1 -
>  2 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a96aa33..a12d938 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -284,7 +284,6 @@ typedef struct x86_def_t {
>      uint32_t kvm_features, svm_features;
>      uint32_t xlevel;
>      char model_id[48];
> -    int vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t ext4_features;
>      uint32_t xlevel2;


> @@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
>  
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> -    x86_cpu_def->vendor_override = 0;
>  

It's funny how x86_def_t _never_ has vendor_override set to true. The
field was completely pointless.


>      /* Call Centaur's CPUID instruction. */
>      if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> @@ -1117,7 +1115,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>          env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
>          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
>      }
> -    env->cpuid_vendor_override = 1;
>  }
>  
>  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> @@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t *def, Error **errp)
>  
>      assert(def->vendor[0]);
>      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
> -    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);
> @@ -1231,6 +1227,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>          return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));

Could you keep the old comment explaining the reason for the KVM
behavior, here?

/* sysenter isn't supported on compatibility mode on AMD, syscall
 * isn't supported in compatibility mode on Intel.
 * Normally we advertise the actual cpu vendor, but you can override
 * this if you want to use KVM's sysenter/syscall emulation
 * in compatibility mode and when doing cross vendor migration
 */

(I suggest replacing "you can override this" with "you can override this
using the 'vendor' property").

The rest of the patch looks good to me.


> +        if (kvm_enabled()) {
> +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> +            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> +        }

I like the fact that you are doing this as early as possible (even
before cpudef_2_x86_cpu() is called), making it easy to move the logic
to class_init.


>      }
>  
>      return 0;
> @@ -1331,7 +1332,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
>                  pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
> -                x86_cpu_def->vendor_override = 1;
>              } else if (!strcmp(featurestr, "model_id")) {
>                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
>                          val);
> @@ -1582,16 +1582,6 @@ static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
>      *ebx = env->cpuid_vendor1;
>      *edx = env->cpuid_vendor2;
>      *ecx = env->cpuid_vendor3;
> -
> -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> -     * isn't supported in compatibility mode on Intel.
> -     * Normally we advertise the actual cpu vendor, but you can override
> -     * this if you want to use KVM's sysenter/syscall emulation
> -     * in compatibility mode and when doing cross vendor migration
> -     */
> -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> -    }
>  }
>  
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index fbbe730..a15a09e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -812,7 +812,6 @@ typedef struct CPUX86State {
>      uint32_t cpuid_ext2_features;
>      uint32_t cpuid_ext3_features;
>      uint32_t cpuid_apic_id;
> -    int cpuid_vendor_override;
>      /* Store the results of Centaur's CPUID instructions */
>      uint32_t cpuid_xlevel2;
>      uint32_t cpuid_ext4_features;
> -- 
> 1.7.11.7
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 13/20] target-i386: remove vendor_override field from CPUX86State

[Igor Mammedov]

On Thu, 20 Dec 2012 10:48:08 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 20, 2012 at 01:16:31AM +0100, Igor Mammedov wrote:
> > commit 8935499831312 makes cpuid return to guest host's vendor value
> > instead of built-in one by default if kvm_enabled() == true and allows
> > to override this behavior if 'vendor' is specified on -cpu command line.
> > 
> > But every time guest calls cpuid to get 'vendor' value, host's value is
> > read again and again in default case.
> > 
> > It complicates semantic of vendor property and makes it harder to use,
> > due to split brain syndrome, lets simplify it.
> > 
> > Instead of reading 'vendor' value from host every time cpuid[vendor] is
> > called, override 'vendor' value only once in cpu_x86_find_by_name(), when
> > built-in CPU model is found and if(kvm_enabled() == true).
> > 
> > It provides the same default semantic
> >  if (kvm_enabled() == true)  vendor = host's vendor
> >  else vendor = built-in vendor
> > 
> > and then later:
> >  if (custom vendor) vendor = custom vendor
> > 
> > 'vendor' value is overridden when user provides it on -cpu command line,
> > and there isn't need in vendor_override field anymore, remove it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 20 +++++---------------
> >  target-i386/cpu.h |  1 -
> >  2 files changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index a96aa33..a12d938 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -284,7 +284,6 @@ typedef struct x86_def_t {
> >      uint32_t kvm_features, svm_features;
> >      uint32_t xlevel;
> >      char model_id[48];
> > -    int vendor_override;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t ext4_features;
> >      uint32_t xlevel2;
> 
> 
> > @@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
> >                  kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> >  
> >      cpu_x86_fill_model_id(x86_cpu_def->model_id);
> > -    x86_cpu_def->vendor_override = 0;
> >  
> 
> It's funny how x86_def_t _never_ has vendor_override set to true. The
> field was completely pointless.
> 
> 
> >      /* Call Centaur's CPUID instruction. */
> >      if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> > @@ -1117,7 +1115,6 @@ static void x86_cpuid_set_vendor(Object *obj, const
> > char *value, env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i);
> >          env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i);
> >      }
> > -    env->cpuid_vendor_override = 1;
> >  }
> >  
> >  static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > @@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t
> > *def, Error **errp) 
> >      assert(def->vendor[0]);
> >      object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
> > -    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);
> > @@ -1231,6 +1227,11 @@ static int cpu_x86_find_by_name(x86_def_t
> > *x86_cpu_def, const char *name) return -1;
> >      } else {
> >          memcpy(x86_cpu_def, def, sizeof(*def));
> 
> Could you keep the old comment explaining the reason for the KVM
> behavior, here?
> 
> /* sysenter isn't supported on compatibility mode on AMD, syscall
>  * isn't supported in compatibility mode on Intel.
>  * Normally we advertise the actual cpu vendor, but you can override
>  * this if you want to use KVM's sysenter/syscall emulation
>  * in compatibility mode and when doing cross vendor migration
>  */
> 
> (I suggest replacing "you can override this" with "you can override this
> using the 'vendor' property").
sure, I'll do it on series respin.


> 
> The rest of the patch looks good to me.
> 
> 
> > +        if (kvm_enabled()) {
> > +            uint32_t  ebx = 0, ecx = 0, edx = 0;
> > +            host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> > +            x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
> > +        }
> 
> I like the fact that you are doing this as early as possible (even
> before cpudef_2_x86_cpu() is called), making it easy to move the logic
> to class_init.
> 
> 
> >      }
> >  
> >      return 0;
> > @@ -1331,7 +1332,6 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > *x86_cpu_def, char *features, x86_cpu_def->xlevel = numvalue;
> >              } else if (!strcmp(featurestr, "vendor")) {
> >                  pstrcpy(x86_cpu_def->vendor,
> > sizeof(x86_cpu_def->vendor), val);
> > -                x86_cpu_def->vendor_override = 1;
> >              } else if (!strcmp(featurestr, "model_id")) {
> >                  pstrcpy(x86_cpu_def->model_id,
> > sizeof(x86_cpu_def->model_id), val);
> > @@ -1582,16 +1582,6 @@ static void get_cpuid_vendor(CPUX86State *env,
> > uint32_t *ebx, *ebx = env->cpuid_vendor1;
> >      *edx = env->cpuid_vendor2;
> >      *ecx = env->cpuid_vendor3;
> > -
> > -    /* sysenter isn't supported on compatibility mode on AMD, syscall
> > -     * isn't supported in compatibility mode on Intel.
> > -     * Normally we advertise the actual cpu vendor, but you can override
> > -     * this if you want to use KVM's sysenter/syscall emulation
> > -     * in compatibility mode and when doing cross vendor migration
> > -     */
> > -    if (kvm_enabled() && ! env->cpuid_vendor_override) {
> > -        host_cpuid(0, 0, NULL, ebx, ecx, edx);
> > -    }
> >  }
> >  
> >  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index fbbe730..a15a09e 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -812,7 +812,6 @@ typedef struct CPUX86State {
> >      uint32_t cpuid_ext2_features;
> >      uint32_t cpuid_ext3_features;
> >      uint32_t cpuid_apic_id;
> > -    int cpuid_vendor_override;
> >      /* Store the results of Centaur's CPUID instructions */
> >      uint32_t cpuid_xlevel2;
> >      uint32_t cpuid_ext4_features;
> > -- 
> > 1.7.11.7
> > 
> 

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 14:54:30 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > > It prepares for converting "+feature,-feature,feature=foo,feature" into
> > > a set of key,value property pairs that will be applied to CPU by
> > > cpu_x86_set_props().
> > > 
> > > Each feature handled by cpu_x86_parse_featurestr() will be converted into
> > > foo,val pair and a corresponding property setter by following patches.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> > object as parameter and set the properties directly? Or you can see some
> > use case where saving the property-setting dictionary for later use will
> > be useful?
> 
> I plan to use cpu_x86_parse_featurestr() + list of properties later when we
> have properties and subclasses in place. Then it would be possible to
> transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of global
> properties.

Is it really worth it to use global properties when you can simply set
the properties at the code that handles the "-cpu" string (and already
has to create the CPU object)?

(more comments about globals are below)


> 
> In the end the option handler for -cpu XXX,... could be changed into:
> 
> cpu_opt_parser() {
>     // hides legacy target specific ugliness 
>     target_XXX_cpu_compat_parser_callback() {
>         cpu_class_name = get_class_name(optval)
> 
>         // dumb compatibility parser, which converts old format into
>         // canonical form feature,val property list
>         prop_list = parse_featurestr(optval)

I'm expecting it to look different: instead of a target-specific parsing
function, you just need target-specific legacy properties on the CPU
object, that would be set by the (generic) featurestr parsing code if
present.

(e.g. "legacy-xlevel" could be used for the legacy "1 becomes
0x80000001" behavior, and "xlevel" would be the more sane xlevel
property without any magic).


> 
>         // with classes and global properties we could get rid of the field
>         // cpu_model_str in CPUxxxState
>         return prop_list, cpu_class_name        
>     }
> 
>     foreach (prop_list)
>         add_global_prop(cpu_class_name,prop,val)
> 
>     // could be later transformed to property of board object and
>     // we could get rid of one more global var
>     cpu_model = cpu_class_name

The cpu_model string will be immediately used to create the CPU object
just after this code runs. So do we really need to do the parsing before
creating the CPU object and use global properties for that? It looks
like unnecessary added complexity.

> }
> 
> > 
> > (This will probably require calling cpudef_2_x86_cpu() before
> > cpu_x86_parse_featurestr(), and changing the existing
> > cpu_x86_parse_featurestr() code to change the cpu object, not a
> > x86_def_t struct, but I think this will simplify the logic a lot)
> You cannot set/uset +-feat directly on CPU without breaking current
> behavior where -feat overrides all +feat no matter in what order they are
> specified. That's why dictionary is used to easily maintain "if(not feat)
> ignore" logic and avoid duplication. Pls, look at commit
> https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
> that converts +-feat into canonical feat=on/off form.

Well, you can make feature parsing code could simply save the
+feat/-feat results internally, and set/unset the properties directly at
the CPU in the end. You can even use a dictionary internally, the point
is that you don't need to expose the dictionary-based interface to the
outside if not necessary (making the API much simpler, IMO).

> 
> And if features are applied directly to CPU, it would require to another
> rewrite if global properties are to be used. Which IMHO should be eventually
> done since -cpu ... looks like global parameters for a specific cpu type.

If we really are going to use global properties for the featurestring
result, we will need to parse the featurestring before creating the CPU
object, then you are correct.

I'm questioning the need to use global properties for the
featurestring parsing, if we can simply set the properties after
creating the CPU object. I expect "-cpu" to be easily translatable to
"-device" options, not to "-global" options.

In other words, I expect this:

  -cpu foo,+xxx,-yyy

to be translated in the future into something like:

  -device foo-x86-cpu,xxx=on,yyy=off

and not to:

  -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu

In other words, I disagree about "-cpu ..." looking like global
parameters for a specific CPU type.

> 
> > 
> > > ---
> > >  target-i386/cpu.c |   33 +++++++++++++++++++++++++++------
> > >  1 files changed, 27 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index e075b59..a74d74b 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1284,9 +1284,25 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
> > >      return 0;
> > >  }
> > >  
> > > +/* Set features on X86CPU object based on a provide key,value list */
> > > +static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
> > > +{
> > > +    const QDictEntry *ent;
> > > +
> > > +    for (ent = qdict_first(features); ent; ent = qdict_next(features, ent)) {
> > > +        const QString *qval = qobject_to_qstring(qdict_entry_value(ent));
> > > +        object_property_parse(OBJECT(cpu), qstring_get_str(qval),
> > > +                              qdict_entry_key(ent), errp);
> > > +        if (error_is_set(errp)) {
> > > +            return;
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> > >   */
> > > -static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > > +static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
> > > +                                    QDict **props)
> > >  {
> > >      unsigned int i;
> > >      char *featurestr; /* Single 'key=value" string being parsed */
> > > @@ -1301,10 +1317,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > >      uint32_t minus_kvm_features = 0, minus_svm_features = 0;
> > >      uint32_t minus_7_0_ebx_features = 0;
> > >      uint32_t numvalue;
> > > +    gchar **feat_array = g_strsplit(features ? features : "", ",", 0);
> > > +    *props = qdict_new();
> > > +    int j = 0;
> > >  
> > > -    featurestr = features ? strtok(features, ",") : NULL;
> > > -
> > > -    while (featurestr) {
> > > +    while ((featurestr = feat_array[j++])) {
> > >          char *val;
> > >          if (featurestr[0] == '+') {
> > >              add_flagname_to_bitmaps(featurestr + 1, &plus_features,
> > > @@ -1413,7 +1430,6 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > >              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> > >              goto error;
> > >          }
> > > -        featurestr = strtok(NULL, ",");
> > >      }
> > >      x86_cpu_def->features |= plus_features;
> > >      x86_cpu_def->ext_features |= plus_ext_features;
> > > @@ -1429,9 +1445,11 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features)
> > >      x86_cpu_def->kvm_features &= ~minus_kvm_features;
> > >      x86_cpu_def->svm_features &= ~minus_svm_features;
> > >      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > > +    g_strfreev(feat_array);
> > >      return 0;
> > >  
> > >  error:
> > > +    g_strfreev(feat_array);
> > >      return -1;
> > >  }
> > >  
> > > @@ -1539,6 +1557,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
> > >  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> > >  {
> > >      x86_def_t def1, *def = &def1;
> > > +    QDict *props = NULL;
> > >      Error *error = NULL;
> > >      char *name, *features;
> > >      gchar **model_pieces;
> > > @@ -1563,14 +1582,16 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> > >                              &def->ext3_features, &def->kvm_features,
> > >                              &def->svm_features, &def->cpuid_7_0_ebx_features);
> > >  
> > > -    if (cpu_x86_parse_featurestr(def, features) < 0) {
> > > +    if (cpu_x86_parse_featurestr(def, features, &props) < 0) {
> > >          error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> > >          goto out;
> > >      }
> > >  
> > >      cpudef_2_x86_cpu(cpu, def, &error);
> > > +    cpu_x86_set_props(cpu, props, &error);
> > >  
> > >  out:
> > > +    QDECREF(props);
> > >      g_strfreev(model_pieces);
> > >      if (error) {
> > >          fprintf(stderr, "%s\n", error_get_pretty(error));
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/20] target-i386: compile kvm only functions if CONFIG_KVM is defined

[Eduardo Habkost]

On Wed, Dec 19, 2012 at 06:16:08PM +0100, Igor Mammedov wrote:
> On Wed, 19 Dec 2012 14:42:31 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Dec 17, 2012 at 05:01:20PM +0100, Igor Mammedov wrote:
> > [...]
> > >  
> > >  static void x86_cpuid_version_get_family(Object *obj, Visitor *v, void
> > > *opaque, const char *name, Error **errp)
> > > @@ -1273,7 +1271,9 @@ static int cpu_x86_find_by_name(x86_def_t
> > > *x86_cpu_def, const char *name) }
> > >      }
> > >      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> > > +#ifdef CONFIG_KVM
> > >          kvm_cpu_fill_host(x86_cpu_def);
> > > +#endif
> > 
> > Is this really better than the existing code that generates an empty
> > stub function (that will never be called anyway)?
> Following patch that moves kvm_check_features_against_host() inside
> of #ifdef CONFIG_KVM in realize_fn(), makes build failure *-user with
> warning that kvm_check_features_against_host() is unused and if
> we make stub from kvm_check_features_against_host() as well then we will have
> to ifdef unavailable_host_feature() as well. As result it makes +2 more
> ifdefs one of which excludes whole function.

I see. So my question above doesn't apply anymore once patch 09/20 is
applied (and the rest of the changes in this patch make sense).

I think you could just squash both patches together, and it should be
obvious that you are adding/moving #ifdefs around the functions just
because the function call is now inside an #ifdef.


> 
> This patch makes one big ifdef block of kvm specific functions and the next
> one keep amount of ifdef the same as before these patches.
> 
> And as bonus we get cleaner build and won't get unused symbols & calls to
> empty functions on debug builds.
> 
> > 
> > I am not strongly inclined either way, but I prefer the existing style.
> > 
> > 
> > >      } else if (!def) {
> > >          return -1;
> > >      } else {
> > > @@ -1428,10 +1428,12 @@ static int cpu_x86_parse_featurestr(x86_def_t
> > > *x86_cpu_def, char *features) x86_cpu_def->kvm_features &=
> > > ~minus_kvm_features; x86_cpu_def->svm_features &= ~minus_svm_features;
> > >      x86_cpu_def->cpuid_7_0_ebx_features &= ~minus_7_0_ebx_features;
> > > +#ifdef CONFIG_KVM
> > >      if (check_cpuid && kvm_enabled()) {
> > >          if (kvm_check_features_against_host(x86_cpu_def) &&
> > > enforce_cpuid) goto error;
> > >      }
> > > +#endif
> > >      return 0;
> > >  
> > >  error:
> > > -- 
> > > 1.7.1
> > > 
> > > 
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 11/20] target-i386: add x86cpu_vendor_words2str()

[Eduardo Habkost]

On Thu, Dec 20, 2012 at 01:16:13AM +0100, Igor Mammedov wrote:
> Make for() cycle reusable for the next patch
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>  target-i386/cpu.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a74d74b..06b991e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -847,6 +847,18 @@ static x86_def_t builtin_x86_defs[] = {
>      },
>  };
>  
> +static void x86cpu_vendor_words2str(char *dst, uint32_t ebx, uint32_t ecx,
> +                                    uint32_t edx)
> +{
> +    int i;
> +    for (i = 0; i < 4; i++) {
> +        dst[i] = ebx >> (8 * i);
> +        dst[i + 4] = edx >> (8 * i);
> +        dst[i + 8] = ecx >> (8 * i);
> +    }
> +    dst[CPUID_VENDOR_SZ] = '\0';
> +}
> +
>  #ifdef CONFIG_KVM
>  static int cpu_x86_fill_model_id(char *str)
>  {
> @@ -1130,15 +1142,10 @@ static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
>      char *value;
> -    int i;
>  
>      value = (char *)g_malloc(CPUID_VENDOR_SZ + 1);
> -    for (i = 0; i < 4; i++) {
> -        value[i    ] = env->cpuid_vendor1 >> (8 * i);
> -        value[i + 4] = env->cpuid_vendor2 >> (8 * i);
> -        value[i + 8] = env->cpuid_vendor3 >> (8 * i);
> -    }
> -    value[CPUID_VENDOR_SZ] = '\0';
> +    x86cpu_vendor_words2str(value, env->cpuid_vendor1, env->cpuid_vendor2,
> +                            env->cpuid_vendor3);
>      return value;
>  }
>  
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 12/20 v2] target-i386: replace uint32_t vendor fields by vendor string in x86_def_t

[Eduardo Habkost]

On Thu, Dec 20, 2012 at 01:16:22AM +0100, Igor Mammedov wrote:
> Vendor property setter takes string as vendor value but cpudefs
> use uint32_t vendor[123] fields to define vendor value. It makes it
> difficult to unify and use property setter for values from cpudefs.
> 
> Simplify code by using vendor property setter, vendor[123] fields
> are converted into vendor[13] array to keep its value. And vendor
> property setter is used to access/set value on CPU.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

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

> ---
>   v3:
>     - no functional change, instead of duplicating for() cycle reuse
>       x86cpu_vendor_words2str()
>   v2:
>     - fix vendor field convertion for a new Haswell & Opteron_G5 cpu models
>     - fix  vendor field convertion for cpu models that had implicit vendor via
>       default value.
> ---
>  target-i386/cpu.c | 127 +++++++++++++-----------------------------------------
>  target-i386/cpu.h |   6 +--
>  2 files changed, 33 insertions(+), 100 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 06b991e..a96aa33 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -275,7 +275,7 @@ typedef struct x86_def_t {
>      struct x86_def_t *next;
>      const char *name;
>      uint32_t level;
> -    uint32_t vendor1, vendor2, vendor3;
> +    char vendor[CPUID_VENDOR_SZ + 1];
>      int family;
>      int model;
>      int stepping;
> @@ -340,9 +340,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "qemu64",
>          .level = 4,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 6,
>          .model = 2,
>          .stepping = 3,
> @@ -359,9 +357,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "phenom",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 16,
>          .model = 2,
>          .stepping = 3,
> @@ -387,9 +383,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 15,
>          .stepping = 11,
> @@ -408,9 +402,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "kvm64",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -434,9 +426,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 3,
>          .stepping = 3,
> @@ -447,9 +437,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -464,9 +452,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 14,
>          .stepping = 8,
> @@ -482,9 +468,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 4,
>          .model = 0,
>          .stepping = 0,
> @@ -494,9 +478,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 5,
>          .model = 4,
>          .stepping = 3,
> @@ -506,9 +488,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 5,
>          .stepping = 2,
> @@ -518,9 +498,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 7,
>          .stepping = 3,
> @@ -530,9 +508,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "athlon",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 6,
>          .model = 2,
>          .stepping = 3,
> @@ -546,9 +522,7 @@ 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,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 28,
>          .stepping = 2,
> @@ -567,9 +541,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Conroe",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 2,
>          .stepping = 3,
> @@ -587,9 +559,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Penryn",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 2,
>          .stepping = 3,
> @@ -608,9 +578,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Nehalem",
>          .level = 2,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 2,
>          .stepping = 3,
> @@ -629,9 +597,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Westmere",
>          .level = 11,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 44,
>          .stepping = 1,
> @@ -651,9 +617,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "SandyBridge",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 42,
>          .stepping = 1,
> @@ -676,9 +640,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Haswell",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_INTEL_1,
> -        .vendor2 = CPUID_VENDOR_INTEL_2,
> -        .vendor3 = CPUID_VENDOR_INTEL_3,
> +        .vendor = CPUID_VENDOR_INTEL,
>          .family = 6,
>          .model = 60,
>          .stepping = 1,
> @@ -706,9 +668,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G1",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -730,9 +690,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G2",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -756,9 +714,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G3",
>          .level = 5,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 15,
>          .model = 6,
>          .stepping = 1,
> @@ -784,9 +740,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G4",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 21,
>          .model = 1,
>          .stepping = 2,
> @@ -816,9 +770,7 @@ static x86_def_t builtin_x86_defs[] = {
>      {
>          .name = "Opteron_G5",
>          .level = 0xd,
> -        .vendor1 = CPUID_VENDOR_AMD_1,
> -        .vendor2 = CPUID_VENDOR_AMD_2,
> -        .vendor3 = CPUID_VENDOR_AMD_3,
> +        .vendor = CPUID_VENDOR_AMD,
>          .family = 21,
>          .model = 2,
>          .stepping = 0,
> @@ -888,10 +840,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      assert(kvm_enabled());
>  
>      x86_cpu_def->name = "host";
> -    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -    x86_cpu_def->vendor1 = ebx;
> -    x86_cpu_def->vendor2 = edx;
> -    x86_cpu_def->vendor3 = ecx;
> +    x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx);
>  
>      host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
>      x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> @@ -919,9 +868,7 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      x86_cpu_def->vendor_override = 0;
>  
>      /* Call Centaur's CPUID instruction. */
> -    if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 &&
> -        x86_cpu_def->vendor2 == CPUID_VENDOR_VIA_2 &&
> -        x86_cpu_def->vendor3 == CPUID_VENDOR_VIA_3) {
> +    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
>          host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
>          eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
>          if (eax >= 0xC0000001) {
> @@ -1245,10 +1192,8 @@ 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;
> +    assert(def->vendor[0]);
> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp);
>      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);
> @@ -1311,7 +1256,6 @@ static void cpu_x86_set_props(X86CPU *cpu, QDict *features, Error **errp)
>  static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                                      QDict **props)
>  {
> -    unsigned int i;
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
>      uint32_t plus_features = 0, plus_ext_features = 0;
> @@ -1386,18 +1330,7 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features,
>                  }
>                  x86_cpu_def->xlevel = numvalue;
>              } else if (!strcmp(featurestr, "vendor")) {
> -                if (strlen(val) != 12) {
> -                    fprintf(stderr, "vendor string must be 12 chars long\n");
> -                    goto error;
> -                }
> -                x86_cpu_def->vendor1 = 0;
> -                x86_cpu_def->vendor2 = 0;
> -                x86_cpu_def->vendor3 = 0;
> -                for(i = 0; i < 4; i++) {
> -                    x86_cpu_def->vendor1 |= ((uint8_t)val[i    ]) << (8 * i);
> -                    x86_cpu_def->vendor2 |= ((uint8_t)val[i + 4]) << (8 * i);
> -                    x86_cpu_def->vendor3 |= ((uint8_t)val[i + 8]) << (8 * i);
> -                }
> +                pstrcpy(x86_cpu_def->vendor, sizeof(x86_cpu_def->vendor), val);
>                  x86_cpu_def->vendor_override = 1;
>              } else if (!strcmp(featurestr, "model_id")) {
>                  pstrcpy(x86_cpu_def->model_id, sizeof(x86_cpu_def->model_id),
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 386c4f6..fbbe730 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -515,14 +515,14 @@
>  #define CPUID_VENDOR_INTEL_1 0x756e6547 /* "Genu" */
>  #define CPUID_VENDOR_INTEL_2 0x49656e69 /* "ineI" */
>  #define CPUID_VENDOR_INTEL_3 0x6c65746e /* "ntel" */
> +#define CPUID_VENDOR_INTEL "GenuineIntel"
>  
>  #define CPUID_VENDOR_AMD_1   0x68747541 /* "Auth" */
>  #define CPUID_VENDOR_AMD_2   0x69746e65 /* "enti" */
>  #define CPUID_VENDOR_AMD_3   0x444d4163 /* "cAMD" */
> +#define CPUID_VENDOR_AMD   "AuthenticAMD"
>  
> -#define CPUID_VENDOR_VIA_1   0x746e6543 /* "Cent" */
> -#define CPUID_VENDOR_VIA_2   0x48727561 /* "aurH" */
> -#define CPUID_VENDOR_VIA_3   0x736c7561 /* "auls" */
> +#define CPUID_VENDOR_VIA   "CentaurHauls"
>  
>  #define CPUID_MWAIT_IBE     (1 << 1) /* Interrupts can exit capability */
>  #define CPUID_MWAIT_EMX     (1 << 0) /* enumeration supported */
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

On Thu, 20 Dec 2012 12:10:25 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

It's a log list of answers. please look through them all to the end.
> On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> > On Wed, 19 Dec 2012 14:54:30 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > > > It prepares for converting "+feature,-feature,feature=foo,feature"
> > > > into a set of key,value property pairs that will be applied to CPU by
> > > > cpu_x86_set_props().
> > > > 
> > > > Each feature handled by cpu_x86_parse_featurestr() will be converted
> > > > into foo,val pair and a corresponding property setter by following
> > > > patches.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> > > object as parameter and set the properties directly? Or you can see some
> > > use case where saving the property-setting dictionary for later use will
> > > be useful?
> > 
> > I plan to use cpu_x86_parse_featurestr() + list of properties later when
> > we have properties and subclasses in place. Then it would be possible to
> > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of
> > global properties.
> 
> Is it really worth it to use global properties when you can simply set
> the properties at the code that handles the "-cpu" string (and already
> has to create the CPU object)?
> 
> (more comments about globals are below)
> 
> 
> > 
> > In the end the option handler for -cpu XXX,... could be changed into:
> > 
> > cpu_opt_parser() {
> >     // hides legacy target specific ugliness 
> >     target_XXX_cpu_compat_parser_callback() {
> >         cpu_class_name = get_class_name(optval)
> > 
> >         // dumb compatibility parser, which converts old format into
> >         // canonical form feature,val property list
> >         prop_list = parse_featurestr(optval)
> 
> I'm expecting it to look different: instead of a target-specific parsing
> function, you just need target-specific legacy properties on the CPU
> object, that would be set by the (generic) featurestr parsing code if
> present.
> 
> (e.g. "legacy-xlevel" could be used for the legacy "1 becomes
> 0x80000001" behavior, and "xlevel" would be the more sane xlevel
> property without any magic).
Introducing 'legacy-xlevel' doesn't solve problem, because with new name
compatibility will be broken and user has to fix qemu invocation anyway.
The same effect could be achieved if qemu exits with invalid value error and
forces user to fix value on qemu invocation without introducing new
properties.

An idea of creating legacy properties would be a negative for following
reasons:
 - new legacy properties with new names could break current users, and force
   them to fix their code
 + converting old feature names into the same property names with same
   behavior without adding new ones. But sometimes old behavior is wrong or
   hard to convert into property, so not good for cpu model eventually.
 - it burdens CPU object with properties that do almost the same as its
   'correct' counterpart but not quite so. That complicates API of CPU
   object a lot.
 - more properties lead to more code, more invariants, resulting in
   more complex behavior.
 - increased maintenance (fixing/rewriting/testing/name it)
 - maintenance  will multiply by target amount that has legacy features that
   do not map 1:1 into properties

Legacy format converter allows to mitigate or reduce above mention problems:
 + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and
   expected not to behave in legacy way.
 + targets provide simple converter from command line format (could be
   painlessly deprecated in future)
 + allows to centralize point of conversion in one place, using utilities
   that target provides if it cares.
 + 'using global properties' and 'cpu_name=>class_name conversion in
    converter' opens easy road to removing CPUxxxState.cpu_model_str and as
    result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init()
 + cpu_init() would be easier to unify/simplify and could be reduced to
   object_new() + set instance specific properties() + realize()
 - practically every QOMified CPU that would use unified cpu_init, should
   provide as minimum converter cpu_name=>class_name, but it could be fixed
   gradually per target.
 - there will be ugly converter callback for a while, but it's temporary,
   after it's deprecation users would be expected to use
   foo-cpu-class,foo=xxx format


In summary I think it would be better to keep CPU object as simple as possible
with a stable API(a property set) for the every interface and convert legacy
command-line format into this API.
It will keep legacy code contained inside one function, which eventually
should disappear as legacy features are deprecated.

> 
> 
> > 
> >         // with classes and global properties we could get rid of the
> > field // cpu_model_str in CPUxxxState
> >         return prop_list, cpu_class_name        
> >     }
> > 
> >     foreach (prop_list)
> >         add_global_prop(cpu_class_name,prop,val)
> > 
> >     // could be later transformed to property of board object and
> >     // we could get rid of one more global var
> >     cpu_model = cpu_class_name
> 
> The cpu_model string will be immediately used to create the CPU object
> just after this code runs. So do we really need to do the parsing before
> creating the CPU object and use global properties for that? It looks
> like unnecessary added complexity.
It might be used immediately or might be not in case of hot-plug or
copy_cpu() or depending where converter is called. Ideally there won't be
cpu_model string at all.

Presently all created CPUs use the same cpu_model string so parsing it
only once instead of N times would be already an improvement.

Pushing common features for type into global properties also makes sense,
isn't global properties made specifically for this?

If common features are pushed in global properties, cpu hot-plug could look
like:
   device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
and all common features that user has overridden on command line would
applied to a new cpu without extra work from user.

> 
> > }
> > 
> > > 
> > > (This will probably require calling cpudef_2_x86_cpu() before
> > > cpu_x86_parse_featurestr(), and changing the existing
> > > cpu_x86_parse_featurestr() code to change the cpu object, not a
> > > x86_def_t struct, but I think this will simplify the logic a lot)
> > You cannot set/uset +-feat directly on CPU without breaking current
> > behavior where -feat overrides all +feat no matter in what order they are
> > specified. That's why dictionary is used to easily maintain "if(not feat)
> > ignore" logic and avoid duplication. Pls, look at commit
> > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
> > that converts +-feat into canonical feat=on/off form.
> 
> Well, you can make feature parsing code could simply save the
> +feat/-feat results internally, and set/unset the properties directly at
> the CPU in the end. You can even use a dictionary internally, the point
> is that you don't need to expose the dictionary-based interface to the
> outside if not necessary (making the API much simpler, IMO).
dictionary outside of cpu_x86_parse_featurestr() is temporary,
until all features is converted into static properties, after that it could
be moved inside cpu_x86_parse_featurestr(), which would push custom properties
into global properties list and be called only once.
Additionally it would be possible to remove introduced here
cpu_x86_set_props(), simplifying cpu_x86_(init|create)().
I'd like to keep legacy handling in one compact place and as much as possible
separated from CPU object itself.

> 
> > 
> > And if features are applied directly to CPU, it would require to another
> > rewrite if global properties are to be used. Which IMHO should be
> > eventually done since -cpu ... looks like global parameters for a
> > specific cpu type.
> 
> If we really are going to use global properties for the featurestring
> result, we will need to parse the featurestring before creating the CPU
> object, then you are correct.
> 
> I'm questioning the need to use global properties for the
> featurestring parsing, if we can simply set the properties after
> creating the CPU object. I expect "-cpu" to be easily translatable to
> "-device" options, not to "-global" options.
> 
> In other words, I expect this:
> 
>   -cpu foo,+xxx,-yyy
> 
> to be translated in the future into something like:
> 
>   -device foo-x86-cpu,xxx=on,yyy=off
> 
> and not to:
> 
>   -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu
> 
> In other words, I disagree about "-cpu ..." looking like global
> parameters for a specific CPU type.
I can't agree with you here, on the contrary second option makes more sense
for me.
But both ways are lead to the same result and both ways will be possible to
use.

However if you look at current -cpu cpu_name,foo=xxx,... it is template for
all CPUs of type cpu_name, qemu is creating. That's maps perfectly into
global properties for a specific type.

If you are concerned with command line length the for single/several cpus
case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.."
will be shorter, but with many cpus syntax:
 -global foo-cpu.yyy=off -device foo-cpu ... 
will be more compact.

And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
of global properties as well.

I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
translated into something like:
   -global foo-cpu.foo=x
   -device foo-cpu,id=a,node=nodeA
   -device foo-cpu,id=b,node=nodeB
   ...
   or
   -device foo-cpu,id=f(cpuN,nodeN)
   ...
where common options are pushed into global properties and only instance
specific ones are specified per instance.

Do you see any issues ahead that global properties would introduce?

> > 
[snip]
> > > > -- 
> > > > 1.7.1
> > > > 
> > > > 
> > > 
> > > -- 
> > > Eduardo
> > 
> > 
> > -- 
> > Regards,
> >   Igor
> 

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Thu, Dec 20, 2012 at 09:22:49PM +0100, Igor Mammedov wrote:
> On Thu, 20 Dec 2012 12:10:25 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> It's a log list of answers. please look through them all to the end.
> > On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> > > On Wed, 19 Dec 2012 14:54:30 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > > > > It prepares for converting "+feature,-feature,feature=foo,feature"
> > > > > into a set of key,value property pairs that will be applied to CPU by
> > > > > cpu_x86_set_props().
> > > > > 
> > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted
> > > > > into foo,val pair and a corresponding property setter by following
> > > > > patches.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> > > > object as parameter and set the properties directly? Or you can see some
> > > > use case where saving the property-setting dictionary for later use will
> > > > be useful?
> > > 
> > > I plan to use cpu_x86_parse_featurestr() + list of properties later when
> > > we have properties and subclasses in place. Then it would be possible to
> > > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of
> > > global properties.
> > 
> > Is it really worth it to use global properties when you can simply set
> > the properties at the code that handles the "-cpu" string (and already
> > has to create the CPU object)?
> > 
> > (more comments about globals are below)
> > 
> > 
> > > 
> > > In the end the option handler for -cpu XXX,... could be changed into:
> > > 
> > > cpu_opt_parser() {
> > >     // hides legacy target specific ugliness 
> > >     target_XXX_cpu_compat_parser_callback() {
> > >         cpu_class_name = get_class_name(optval)
> > > 
> > >         // dumb compatibility parser, which converts old format into
> > >         // canonical form feature,val property list
> > >         prop_list = parse_featurestr(optval)
> > 
> > I'm expecting it to look different: instead of a target-specific parsing
> > function, you just need target-specific legacy properties on the CPU
> > object, that would be set by the (generic) featurestr parsing code if
> > present.
> > 
> > (e.g. "legacy-xlevel" could be used for the legacy "1 becomes
> > 0x80000001" behavior, and "xlevel" would be the more sane xlevel
> > property without any magic).
> Introducing 'legacy-xlevel' doesn't solve problem, because with new name
> compatibility will be broken and user has to fix qemu invocation anyway.
> The same effect could be achieved if qemu exits with invalid value error and
> forces user to fix value on qemu invocation without introducing new
> properties.
> 
> An idea of creating legacy properties would be a negative for following
> reasons:
>  - new legacy properties with new names could break current users, and force
>    them to fix their code

No, the point is to keep compatibility (just like your per-target
parse_featurestr() approach), the only difference is that compatibility
details would be encoded on the legacy properties, not inside
parse_featurestr().

>  + converting old feature names into the same property names with same
>    behavior without adding new ones. But sometimes old behavior is wrong or
>    hard to convert into property, so not good for cpu model eventually.

That's why the legacy properties would be "legacy" one. The only code
that would be supposed to use it would be the (legacy) "-cpu" parsing
code.

>  - it burdens CPU object with properties that do almost the same as its
>    'correct' counterpart but not quite so. That complicates API of CPU
>    object a lot.

I believe the legacy property is as heavy a burden as a legacy
featurestr-parsing code. It's no burden to the CPU object to have
property setters that are almost never used. They would be just sitting
there, unused.

>  - more properties lead to more code, more invariants, resulting in
>    more complex behavior.

I don't think it's more code. It's less code: instead of writing
multiple parsers, you have a target-independent parser, and the
compatibility hacks would be isolated on the legacy property-setters.

>  - increased maintenance (fixing/rewriting/testing/name it)

It looks like as much maintenance as having a target-specific
parse_featurestr() function. Maybe even less, because now we can reuse
the same parser for the other targets.

>  - maintenance  will multiply by target amount that has legacy features that
>    do not map 1:1 into properties

If a legacy feature do not map 1:1 into a property, you just add a
legacy property-setter that will translate it into the "true"
properties. It would contain exactly the same code you would put inside
an "else if (!strcmp(...)) { ... }" block inside parse_featurestr().

> 
> Legacy format converter allows to mitigate or reduce above mention problems:
>  + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and
>    expected not to behave in legacy way.

The legacy property doesn't even need to be a "static" one, and doesn't
even need to have a getter. Other code would just never use it. It would
be similar to the legacy properties registered by
qdev_property_add_legacy() (maybe we could simply reuse that existing
mechanism?).


>  + targets provide simple converter from command line format (could be
>    painlessly deprecated in future)

This would have exactly the same functionality: targets provide simple
list of legacy property-setters to translate old options. But now with a
target-independent parser.


>  + allows to centralize point of conversion in one place, using utilities
>    that target provides if it cares.

This is the main difference, I guess. But I think not having to
duplicate parsing code and just have to provide a bunch of
property-setter functions would be better than having target-specific
parsing code just to "keep things in one place". You can easily put all
the legacy setter functions in the same place inside the code, if you
want to group them into a visible place.

By the way, I don't think they _must_ be properties. All I want to avoid
is having to duplicate the full featurestring parser for each target,
when the only difference is that the targets will handle a few specific
options differently from -device and QMP. Then featurestr parser could
be a generic parser that simply set properties. But I would be happy
with an approch based with something like a:
  parse_single_feature(const char *key, const char *value)
callback in CPUClass, called by the generic parser.


>  + 'using global properties' and 'cpu_name=>class_name conversion in
>     converter' opens easy road to removing CPUxxxState.cpu_model_str and as
>     result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init()
>  + cpu_init() would be easier to unify/simplify and could be reduced to
>    object_new() + set instance specific properties() + realize()
>  - practically every QOMified CPU that would use unified cpu_init, should
>    provide as minimum converter cpu_name=>class_name, but it could be fixed
>    gradually per target.


I don't think this has anything to do with the legacy-parse_featurestr
handling. You can simplify/generalize cpu_init() in exactly the same
way, the difference is that (wherever you call it) you can call a
generic featurestr parser, instead of a target-specific one.

IIRC, other target maintainers expressed interest in allowing options to
be set in "-cpu". And we already have a feature string parser in x86, we
just need to make it target-independent so other targets could use it.


>  - there will be ugly converter callback for a while, but it's temporary,
>    after it's deprecation users would be expected to use
>    foo-cpu-class,foo=xxx format

The same can be said by the legacy property-setter (or a
parse_single_feature() callback). They could be temporary and would be
used only if a "-cpu" option that required compatibility translation was
used.


> 
> 
> In summary I think it would be better to keep CPU object as simple as possible
> with a stable API(a property set) for the every interface and convert legacy
> command-line format into this API.
> It will keep legacy code contained inside one function, which eventually
> should disappear as legacy features are deprecated.

All above said, I am not strongly against your approach, but I believe
we could try to make the feature string parsing code generic and
reusable (with target-specific properties or callbacks), instead of
having to write a new generic feature string parsing function.

Anyway, the above is just bikeshedding to me, and your approach is an
improvement, already, even if we try to make the parser more generic
later.

But the questions below about global properties seem important/interesting:

> 
> > 
> > 
> > > 
> > >         // with classes and global properties we could get rid of the
> > > field // cpu_model_str in CPUxxxState
> > >         return prop_list, cpu_class_name        
> > >     }
> > > 
> > >     foreach (prop_list)
> > >         add_global_prop(cpu_class_name,prop,val)
> > > 
> > >     // could be later transformed to property of board object and
> > >     // we could get rid of one more global var
> > >     cpu_model = cpu_class_name
> > 
> > The cpu_model string will be immediately used to create the CPU object
> > just after this code runs. So do we really need to do the parsing before
> > creating the CPU object and use global properties for that? It looks
> > like unnecessary added complexity.
> It might be used immediately or might be not in case of hot-plug or
> copy_cpu() or depending where converter is called. Ideally there won't be
> cpu_model string at all.

I don't see the difference between storing a cpu_model string and
storing a dictionary to be used later. It would just be a
micro-optimization for the code that creates new CPUs.


> 
> Presently all created CPUs use the same cpu_model string so parsing it
> only once instead of N times would be already an improvement.

I don't believe we would want to make the code more complex just to
optimize string parsing that is run a few times during QEMU
initialization. That would be unnecessary optimization.

> 
> Pushing common features for type into global properties also makes sense,
> isn't global properties made specifically for this?

Yes, but do we want to make "-cpu" affect every single
-device/device_add call for CPUs, or only the ones created on
initialization? That's the main question, I believe.

> 
> If common features are pushed in global properties, cpu hot-plug could look
> like:
>    device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
> and all common features that user has overridden on command line would
> applied to a new cpu without extra work from user.

CPU hotplug is an interesting case: if thinking only about the CPUs
created from the command-line the "-global vs -device" question wouldn't
make such a big difference. But in the case of device_add for CPU
hotplug, it would be very different. But I am not sure which option is
less surprising for users.

What others think? Andreas? Should "-cpu" affect only the CPUs created
on initialization, or all CPUs created by -device/device_add for the
lifetime of the QEMU process?  (in other words, should the options to
"-cpu" be translated to "-device" arguments, or to "-global" arguments?)

(BTW about the device_add example above: I believe we will want the
hotplug interface to be based on a "cpu_index" parameter/property, to
abstract out the complexity of the APIC ID calculation)



> 
> > 
> > > }
> > > 
> > > > 
> > > > (This will probably require calling cpudef_2_x86_cpu() before
> > > > cpu_x86_parse_featurestr(), and changing the existing
> > > > cpu_x86_parse_featurestr() code to change the cpu object, not a
> > > > x86_def_t struct, but I think this will simplify the logic a lot)
> > > You cannot set/uset +-feat directly on CPU without breaking current
> > > behavior where -feat overrides all +feat no matter in what order they are
> > > specified. That's why dictionary is used to easily maintain "if(not feat)
> > > ignore" logic and avoid duplication. Pls, look at commit
> > > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
> > > that converts +-feat into canonical feat=on/off form.
> > 
> > Well, you can make feature parsing code could simply save the
> > +feat/-feat results internally, and set/unset the properties directly at
> > the CPU in the end. You can even use a dictionary internally, the point
> > is that you don't need to expose the dictionary-based interface to the
> > outside if not necessary (making the API much simpler, IMO).
> dictionary outside of cpu_x86_parse_featurestr() is temporary,
> until all features is converted into static properties, after that it could
> be moved inside cpu_x86_parse_featurestr(), which would push custom properties
> into global properties list and be called only once.
> Additionally it would be possible to remove introduced here
> cpu_x86_set_props(), simplifying cpu_x86_(init|create)().
> I'd like to keep legacy handling in one compact place and as much as possible
> separated from CPU object itself.
> 

I see. If you are going to use global properties, the dictionary
approach makes a lot of sense.


> > 
> > > 
> > > And if features are applied directly to CPU, it would require to another
> > > rewrite if global properties are to be used. Which IMHO should be
> > > eventually done since -cpu ... looks like global parameters for a
> > > specific cpu type.
> > 
> > If we really are going to use global properties for the featurestring
> > result, we will need to parse the featurestring before creating the CPU
> > object, then you are correct.
> > 
> > I'm questioning the need to use global properties for the
> > featurestring parsing, if we can simply set the properties after
> > creating the CPU object. I expect "-cpu" to be easily translatable to
> > "-device" options, not to "-global" options.
> > 
> > In other words, I expect this:
> > 
> >   -cpu foo,+xxx,-yyy
> > 
> > to be translated in the future into something like:
> > 
> >   -device foo-x86-cpu,xxx=on,yyy=off
> > 
> > and not to:
> > 
> >   -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu
> > 
> > In other words, I disagree about "-cpu ..." looking like global
> > parameters for a specific CPU type.
> I can't agree with you here, on the contrary second option makes more sense
> for me.
> But both ways are lead to the same result and both ways will be possible to
> use.

Both ways have different results if we think about using device_add for
CPU hotplug, though.

> 
> However if you look at current -cpu cpu_name,foo=xxx,... it is template for
> all CPUs of type cpu_name, qemu is creating. That's maps perfectly into
> global properties for a specific type.

I see it as a template of the CPUs QEMU is creating _at initialization_,
but I understand that seeing it as a template for all CPUs QEMU will
ever create makes sense, too. We just have to decide which way to go.


> 
> If you are concerned with command line length the for single/several cpus
> case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.."
> will be shorter, but with many cpus syntax:
>  -global foo-cpu.yyy=off -device foo-cpu ... 
> will be more compact.

command-line length won't be an issue if users use a config file.  :-)

> 
> And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
> of global properties as well.
> 
> I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
> translated into something like:
>    -global foo-cpu.foo=x
>    -device foo-cpu,id=a,node=nodeA
>    -device foo-cpu,id=b,node=nodeB
>    ...
>    or
>    -device foo-cpu,id=f(cpuN,nodeN)
>    ...
> where common options are pushed into global properties and only instance
> specific ones are specified per instance.
> 
> Do you see any issues ahead that global properties would introduce?

I see no issue, except that it was not what I was expecting at first. I
just don't know which one is less surprising for users (including
libvirt).

Summary: I can see that translating "-cpu" to global properties may make
sense, too. Now I'm divided. I hope to hear what Andreas (and other
maintainers) think about this.

[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

On Thu, 20 Dec 2012 20:13:19 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 20, 2012 at 09:22:49PM +0100, Igor Mammedov wrote:
> > On Thu, 20 Dec 2012 12:10:25 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > It's a log list of answers. please look through them all to the end.
> > > On Wed, Dec 19, 2012 at 09:18:09PM +0100, Igor Mammedov wrote:
> > > > On Wed, 19 Dec 2012 14:54:30 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Mon, Dec 17, 2012 at 05:01:22PM +0100, Igor Mammedov wrote:
> > > > > > It prepares for converting "+feature,-feature,feature=foo,feature"
> > > > > > into a set of key,value property pairs that will be applied to CPU by
> > > > > > cpu_x86_set_props().
> > > > > > 
> > > > > > Each feature handled by cpu_x86_parse_featurestr() will be converted
> > > > > > into foo,val pair and a corresponding property setter by following
> > > > > > patches.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > Isn't it much simpler to make cpu_x86_parse_featurestr() get the CPU
> > > > > object as parameter and set the properties directly? Or you can see some
> > > > > use case where saving the property-setting dictionary for later use will
> > > > > be useful?
> > > > 
> > > > I plan to use cpu_x86_parse_featurestr() + list of properties later when
> > > > we have properties and subclasses in place. Then it would be possible to
> > > > transform cpu_x86_parse_featurestr() + cpu_x86_set_props() into set of
> > > > global properties.
> > > 
> > > Is it really worth it to use global properties when you can simply set
> > > the properties at the code that handles the "-cpu" string (and already
> > > has to create the CPU object)?
> > > 
> > > (more comments about globals are below)
> > > 
> > > 
> > > > 
> > > > In the end the option handler for -cpu XXX,... could be changed into:
> > > > 
> > > > cpu_opt_parser() {
> > > >     // hides legacy target specific ugliness 
> > > >     target_XXX_cpu_compat_parser_callback() {
> > > >         cpu_class_name = get_class_name(optval)
> > > > 
> > > >         // dumb compatibility parser, which converts old format into
> > > >         // canonical form feature,val property list
> > > >         prop_list = parse_featurestr(optval)
> > > 
> > > I'm expecting it to look different: instead of a target-specific parsing
> > > function, you just need target-specific legacy properties on the CPU
> > > object, that would be set by the (generic) featurestr parsing code if
> > > present.
> > > 
> > > (e.g. "legacy-xlevel" could be used for the legacy "1 becomes
> > > 0x80000001" behavior, and "xlevel" would be the more sane xlevel
> > > property without any magic).
> > Introducing 'legacy-xlevel' doesn't solve problem, because with new name
> > compatibility will be broken and user has to fix qemu invocation anyway.
> > The same effect could be achieved if qemu exits with invalid value error and
> > forces user to fix value on qemu invocation without introducing new
> > properties.
> > 
> > An idea of creating legacy properties would be a negative for following
> > reasons:
> >  - new legacy properties with new names could break current users, and force
> >    them to fix their code
> 
> No, the point is to keep compatibility (just like your per-target
> parse_featurestr() approach), the only difference is that compatibility
> details would be encoded on the legacy properties, not inside
> parse_featurestr().
> 
> >  + converting old feature names into the same property names with same
> >    behavior without adding new ones. But sometimes old behavior is wrong or
> >    hard to convert into property, so not good for cpu model eventually.
> 
> That's why the legacy properties would be "legacy" one. The only code
> that would be supposed to use it would be the (legacy) "-cpu" parsing
> code.
> 
> >  - it burdens CPU object with properties that do almost the same as its
> >    'correct' counterpart but not quite so. That complicates API of CPU
> >    object a lot.
> 
> I believe the legacy property is as heavy a burden as a legacy
> featurestr-parsing code. It's no burden to the CPU object to have
> property setters that are almost never used. They would be just sitting
> there, unused.
> 
> >  - more properties lead to more code, more invariants, resulting in
> >    more complex behavior.
> 
> I don't think it's more code. It's less code: instead of writing
> multiple parsers, you have a target-independent parser, and the
> compatibility hacks would be isolated on the legacy property-setters.
> 
> >  - increased maintenance (fixing/rewriting/testing/name it)
> 
> It looks like as much maintenance as having a target-specific
> parse_featurestr() function. Maybe even less, because now we can reuse
> the same parser for the other targets.
> 
> >  - maintenance  will multiply by target amount that has legacy features that
> >    do not map 1:1 into properties
> 
> If a legacy feature do not map 1:1 into a property, you just add a
> legacy property-setter that will translate it into the "true"
> properties. It would contain exactly the same code you would put inside
> an "else if (!strcmp(...)) { ... }" block inside parse_featurestr().
> 
> > 
> > Legacy format converter allows to mitigate or reduce above mention problems:
> >  + keeps CPU API simpler, new interfaces /QMP .../ see only CPU API and
> >    expected not to behave in legacy way.
> 
> The legacy property doesn't even need to be a "static" one, and doesn't
> even need to have a getter. Other code would just never use it. It would
> be similar to the legacy properties registered by
> qdev_property_add_legacy() (maybe we could simply reuse that existing
> mechanism?).
> 
> 
> >  + targets provide simple converter from command line format (could be
> >    painlessly deprecated in future)
> 
> This would have exactly the same functionality: targets provide simple
> list of legacy property-setters to translate old options. But now with a
> target-independent parser.
> 
> 
> >  + allows to centralize point of conversion in one place, using utilities
> >    that target provides if it cares.
> 
> This is the main difference, I guess. But I think not having to
> duplicate parsing code and just have to provide a bunch of
> property-setter functions would be better than having target-specific
> parsing code just to "keep things in one place". You can easily put all
> the legacy setter functions in the same place inside the code, if you
> want to group them into a visible place.
> 
> By the way, I don't think they _must_ be properties. All I want to avoid
> is having to duplicate the full featurestring parser for each target,
> when the only difference is that the targets will handle a few specific
> options differently from -device and QMP. Then featurestr parser could
> be a generic parser that simply set properties. But I would be happy
> with an approch based with something like a:
>   parse_single_feature(const char *key, const char *value)
> callback in CPUClass, called by the generic parser.
> 
> 
> >  + 'using global properties' and 'cpu_name=>class_name conversion in
> >     converter' opens easy road to removing CPUxxxState.cpu_model_str and as
> >     result simplifying CPUxxxState, copy_cpu() and cpu_xxx_init()
> >  + cpu_init() would be easier to unify/simplify and could be reduced to
> >    object_new() + set instance specific properties() + realize()
> >  - practically every QOMified CPU that would use unified cpu_init, should
> >    provide as minimum converter cpu_name=>class_name, but it could be fixed
> >    gradually per target.
> 
> 
> I don't think this has anything to do with the legacy-parse_featurestr
> handling. You can simplify/generalize cpu_init() in exactly the same
> way, the difference is that (wherever you call it) you can call a
> generic featurestr parser, instead of a target-specific one.
> 
> IIRC, other target maintainers expressed interest in allowing options to
> be set in "-cpu". And we already have a feature string parser in x86, we
> just need to make it target-independent so other targets could use it.
> 
> 
> >  - there will be ugly converter callback for a while, but it's temporary,
> >    after it's deprecation users would be expected to use
> >    foo-cpu-class,foo=xxx format
> 
> The same can be said by the legacy property-setter (or a
> parse_single_feature() callback). They could be temporary and would be
> used only if a "-cpu" option that required compatibility translation was
> used.
> 
> 
> > 
> > 
> > In summary I think it would be better to keep CPU object as simple as possible
> > with a stable API(a property set) for the every interface and convert legacy
> > command-line format into this API.
> > It will keep legacy code contained inside one function, which eventually
> > should disappear as legacy features are deprecated.
> 
> All above said, I am not strongly against your approach, but I believe
> we could try to make the feature string parsing code generic and
> reusable (with target-specific properties or callbacks), instead of
> having to write a new generic feature string parsing function.
> 
> Anyway, the above is just bikeshedding to me, and your approach is an
> improvement, already, even if we try to make the parser more generic
> later.
>
feature string parser in this series isn't meant to be generic, it's just
a simplification of the current function and splitting parsing
features & setting properties into separate steps. kind of what you did with
cpu_name+feature_string split.

It's not clear to me what you mean under legacy properties, could you
throw in a hack using as example xlevel feature to demonstrate what you mean,
please?

> But the questions below about global properties seem important/interesting:
> 
> > 
> > > 
> > > 
> > > > 
> > > >         // with classes and global properties we could get rid of the
> > > > field // cpu_model_str in CPUxxxState
> > > >         return prop_list, cpu_class_name        
> > > >     }
> > > > 
> > > >     foreach (prop_list)
> > > >         add_global_prop(cpu_class_name,prop,val)
> > > > 
> > > >     // could be later transformed to property of board object and
> > > >     // we could get rid of one more global var
> > > >     cpu_model = cpu_class_name
> > > 
> > > The cpu_model string will be immediately used to create the CPU object
> > > just after this code runs. So do we really need to do the parsing before
> > > creating the CPU object and use global properties for that? It looks
> > > like unnecessary added complexity.
> > It might be used immediately or might be not in case of hot-plug or
> > copy_cpu() or depending where converter is called. Ideally there won't be
> > cpu_model string at all.
> 
> I don't see the difference between storing a cpu_model string and
> storing a dictionary to be used later. It would just be a
> micro-optimization for the code that creates new CPUs.
Dictionary is an implementation detail, a temporary list of properties that
would be applied to global properties and then list destroyed. It is there now
because:
 1. it's just a convenient data structure to implement current weird +-foo
    semantic
 2. a necessary intermediate step to hold list of properties because
    properties/features is not yet converted into static properties. Properties
    could/should be pushed directly into global properties if possible (i.e.
    when property is static), then there won't be any need in intermediate
    property list at all.

There is not compelling reason to use dictionary in generic code, it could be
removed or be hidden inside current featurestr parser.

> 
> 
> > 
> > Presently all created CPUs use the same cpu_model string so parsing it
> > only once instead of N times would be already an improvement.
> 
> I don't believe we would want to make the code more complex just to
> optimize string parsing that is run a few times during QEMU
> initialization. That would be unnecessary optimization.
If we move generalized parsing out into vl.c & *-user/main.c option parsing
code, it would simplify every target. I would be better to do option parsing
at the time when options are parsed in a single place and let boards/targets to
concentrate on creating cpus (without caring about options at all, not really
their job).

> 
> > 
> > Pushing common features for type into global properties also makes sense,
> > isn't global properties made specifically for this?
> 
> Yes, but do we want to make "-cpu" affect every single
> -device/device_add call for CPUs, or only the ones created on
> initialization? That's the main question, I believe.
If user overrides default cpu model features with -cpu option it would be only
logical for hot-plugged cpus to use that overrides as well, instead of creating
cpu that would be different (it may confuse guests, to the point of crashing).
IMHO, It would be surprising for user to get a different cpu on hot-plug than
already existing ones, in most cases.

And if user likes to shoot in its own foot, it will be possible to override
global features by adding extra foo=xxx to a specific device_add/-device
command.

> 
> > 
> > If common features are pushed in global properties, cpu hot-plug could look
> > like:
> >    device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
> > and all common features that user has overridden on command line would
> > applied to a new cpu without extra work from user.
> 
> CPU hotplug is an interesting case: if thinking only about the CPUs
> created from the command-line the "-global vs -device" question wouldn't
> make such a big difference. But in the case of device_add for CPU
> hotplug, it would be very different. But I am not sure which option is
> less surprising for users.
> 
> What others think? Andreas? Should "-cpu" affect only the CPUs created
> on initialization, or all CPUs created by -device/device_add for the
> lifetime of the QEMU process?  (in other words, should the options to
> "-cpu" be translated to "-device" arguments, or to "-global" arguments?)
> 
> (BTW about the device_add example above: I believe we will want the
> hotplug interface to be based on a "cpu_index" parameter/property, to
> abstract out the complexity of the APIC ID calculation)
there was other opinion about cpu_index
http://permalink.gmane.org/gmane.comp.emulators.qemu/151626
 
However if board will allocate sockets (Anthony suggested to try links for
this) then user won't have to calculate ID, instead of user will have to
enumerate existing cpu links and use IDs embedded in it for managing
cpus.

> 
> 
> 
> > 
> > > 
> > > > }
> > > > 
> > > > > 
> > > > > (This will probably require calling cpudef_2_x86_cpu() before
> > > > > cpu_x86_parse_featurestr(), and changing the existing
> > > > > cpu_x86_parse_featurestr() code to change the cpu object, not a
> > > > > x86_def_t struct, but I think this will simplify the logic a lot)
> > > > You cannot set/uset +-feat directly on CPU without breaking current
> > > > behavior where -feat overrides all +feat no matter in what order they are
> > > > specified. That's why dictionary is used to easily maintain "if(not feat)
> > > > ignore" logic and avoid duplication. Pls, look at commit
> > > > https://github.com/imammedo/qemu/commit/ea0573ded2f637844f02142437f4a21ed74ec7f3
> > > > that converts +-feat into canonical feat=on/off form.
> > > 
> > > Well, you can make feature parsing code could simply save the
> > > +feat/-feat results internally, and set/unset the properties directly at
> > > the CPU in the end. You can even use a dictionary internally, the point
> > > is that you don't need to expose the dictionary-based interface to the
> > > outside if not necessary (making the API much simpler, IMO).
> > dictionary outside of cpu_x86_parse_featurestr() is temporary,
> > until all features is converted into static properties, after that it could
> > be moved inside cpu_x86_parse_featurestr(), which would push custom properties
> > into global properties list and be called only once.
> > Additionally it would be possible to remove introduced here
> > cpu_x86_set_props(), simplifying cpu_x86_(init|create)().
> > I'd like to keep legacy handling in one compact place and as much as possible
> > separated from CPU object itself.
> > 
> 
> I see. If you are going to use global properties, the dictionary
> approach makes a lot of sense.
> 
> 
> > > 
> > > > 
> > > > And if features are applied directly to CPU, it would require to another
> > > > rewrite if global properties are to be used. Which IMHO should be
> > > > eventually done since -cpu ... looks like global parameters for a
> > > > specific cpu type.
> > > 
> > > If we really are going to use global properties for the featurestring
> > > result, we will need to parse the featurestring before creating the CPU
> > > object, then you are correct.
> > > 
> > > I'm questioning the need to use global properties for the
> > > featurestring parsing, if we can simply set the properties after
> > > creating the CPU object. I expect "-cpu" to be easily translatable to
> > > "-device" options, not to "-global" options.
> > > 
> > > In other words, I expect this:
> > > 
> > >   -cpu foo,+xxx,-yyy
> > > 
> > > to be translated in the future into something like:
> > > 
> > >   -device foo-x86-cpu,xxx=on,yyy=off
> > > 
> > > and not to:
> > > 
> > >   -global foo-x86-cpu.xxx=on -global foo-x86-cpu.yyy=off -device foo-x86-cpu
> > > 
> > > In other words, I disagree about "-cpu ..." looking like global
> > > parameters for a specific CPU type.
> > I can't agree with you here, on the contrary second option makes more sense
> > for me.
> > But both ways are lead to the same result and both ways will be possible to
> > use.
> 
> Both ways have different results if we think about using device_add for
> CPU hotplug, though.
> 
> > 
> > However if you look at current -cpu cpu_name,foo=xxx,... it is template for
> > all CPUs of type cpu_name, qemu is creating. That's maps perfectly into
> > global properties for a specific type.
> 
> I see it as a template of the CPUs QEMU is creating _at initialization_,
> but I understand that seeing it as a template for all CPUs QEMU will
> ever create makes sense, too. We just have to decide which way to go.
That's what I was trying to explain, I'm sorry my poor English that it took so
many words to get here.


> 
> 
> > 
> > If you are concerned with command line length the for single/several cpus
> > case then syntax "-device foo-cpu,xxx=on,yyy=off -device foo-cpu,.."
> > will be shorter, but with many cpus syntax:
> >  -global foo-cpu.yyy=off -device foo-cpu ... 
> > will be more compact.
> 
> command-line length won't be an issue if users use a config file.  :-)
> 
> > 
> > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
> > of global properties as well.
> > 
> > I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
> > translated into something like:
> >    -global foo-cpu.foo=x
> >    -device foo-cpu,id=a,node=nodeA
> >    -device foo-cpu,id=b,node=nodeB
> >    ...
> >    or
> >    -device foo-cpu,id=f(cpuN,nodeN)
> >    ...
> > where common options are pushed into global properties and only instance
> > specific ones are specified per instance.
> > 
> > Do you see any issues ahead that global properties would introduce?
> 
> I see no issue, except that it was not what I was expecting at first. I
> just don't know which one is less surprising for users (including
> libvirt).
users shouldn't be affected, -cpu will continue to works the same way until we
start deprecating legacy behavior.

> 
> Summary: I can see that translating "-cpu" to global properties may make
> sense, too. Now I'm divided. I hope to hear what Andreas (and other
> maintainers) think about this.
> 
> [...]
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote:
[...]
> > All above said, I am not strongly against your approach, but I believe
> > we could try to make the feature string parsing code generic and
> > reusable (with target-specific properties or callbacks), instead of
> > having to write a new generic feature string parsing function.
> > 
> > Anyway, the above is just bikeshedding to me, and your approach is an
> > improvement, already, even if we try to make the parser more generic
> > later.
> >
> feature string parser in this series isn't meant to be generic, it's just
> a simplification of the current function and splitting parsing
> features & setting properties into separate steps. kind of what you did with
> cpu_name+feature_string split.
> 
> It's not clear to me what you mean under legacy properties, could you
> throw in a hack using as example xlevel feature to demonstrate what you mean,
> please?

I will try to show it below, but note that we can try to do that _after_
we introduce the true/final/clean properties that won't have those
hacks, and after changing the current parse_featurestr() implementation
to contain the compatibility hacks (that's the work you are already
doing).

I was thinking about using the "legacy-%s" feature inside
qdev_prop_parse(), this way:

/* Generic featurestr parsing function */
void generic_parse_featurestr(CPUClass *cc, const char *featurestr)
{
    opts = g_strsplit(featurestr, ",")
    for (each opt in opts) {
        if (opt[0] == '+') {
            dict[opt+1] = 'true';
        } else if (opt[0] == '
            dict[opt+1] = 'false ;
        } else if (opt contains '=) {
            key, value = g_strsplit(opt, "=");
            dict[key] = value;
        }
    }
    for (each key in dict) {
        /* qdev_prop_set_globals() uses qdev_prop_parse(), that already
         * checks for a "legacy-<name>" property.
         */
        qdev_add_one_global(key, dict[key]);
    }
}


/* target-specific handling of legacy values for the -cpu option */
void cpu_set_legacy_xlevel(X86CPU *cpu, Visitor *v, ...)
{
    int64_t i;
    visit_type_int(v, &i, ...);
    if (i < 0x80000000)
        i += 0x80000000;
    object_property_set_int(cpu, i, "xlevel");
}

void cpu_set_legacy_foobar(...)
{
    ...
}

void cpu_x86_class_init(CPUClass *cc, ...)
{
    ...
    qdev_class_add_property("legacy-xlevel", cpu_set_legacy_xlevel, NULL, ...); /* only a setter, no getter */
    qdev_class_add_property("legacy-foobar", cpu_set_legacy_foobar, NULL, ...); /* only a setter, no getter */
}


Note:

- We don't have a qdev_class_add_property() function, so we have to add
  the legacy property to the ->props array;
- The props array is based on name/offset/type, so we would need to
  define (for example) a "PropertyInfo cpu_prop_xlevel" struct;
  - On the other hand, setting a ->parse function on PropertyInfo gives
    us the legacy-* property registration for free when registering
    "xlevel" (see device_initfn()/qdev_property_add_legacy())
  - But I would prefer a qdev_class_add_property() interface, as it
    would be much simpler...

So, in the end it could be too complex due to the interfaces provided by
qdev.


> 
> > But the questions below about global properties seem important/interesting:
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > >         // with classes and global properties we could get rid of the
> > > > > field // cpu_model_str in CPUxxxState
> > > > >         return prop_list, cpu_class_name        
> > > > >     }
> > > > > 
> > > > >     foreach (prop_list)
> > > > >         add_global_prop(cpu_class_name,prop,val)
> > > > > 
> > > > >     // could be later transformed to property of board object and
> > > > >     // we could get rid of one more global var
> > > > >     cpu_model = cpu_class_name
> > > > 
> > > > The cpu_model string will be immediately used to create the CPU object
> > > > just after this code runs. So do we really need to do the parsing before
> > > > creating the CPU object and use global properties for that? It looks
> > > > like unnecessary added complexity.
> > > It might be used immediately or might be not in case of hot-plug or
> > > copy_cpu() or depending where converter is called. Ideally there won't be
> > > cpu_model string at all.
> > 
> > I don't see the difference between storing a cpu_model string and
> > storing a dictionary to be used later. It would just be a
> > micro-optimization for the code that creates new CPUs.
> Dictionary is an implementation detail, a temporary list of properties that
> would be applied to global properties and then list destroyed. It is there now
> because:
>  1. it's just a convenient data structure to implement current weird +-foo
>     semantic
>  2. a necessary intermediate step to hold list of properties because
>     properties/features is not yet converted into static properties. Properties
>     could/should be pushed directly into global properties if possible (i.e.
>     when property is static), then there won't be any need in intermediate
>     property list at all.
> 
> There is not compelling reason to use dictionary in generic code, it could be
> removed or be hidden inside current featurestr parser.

I wasn't taking into account that the dictionary would be simply used to
feed the global property list, so nevermind.

> 
> > 
> > 
> > > 
> > > Presently all created CPUs use the same cpu_model string so parsing it
> > > only once instead of N times would be already an improvement.
> > 
> > I don't believe we would want to make the code more complex just to
> > optimize string parsing that is run a few times during QEMU
> > initialization. That would be unnecessary optimization.
> If we move generalized parsing out into vl.c & *-user/main.c option parsing
> code, it would simplify every target. I would be better to do option parsing
> at the time when options are parsed in a single place and let boards/targets to
> concentrate on creating cpus (without caring about options at all, not really
> their job).

Even if we did it the "inefficient" way (parsing it multiple times), it
could be moved to generic CPU creation code.

Anyway, my assumption about "making the code more complex" doesn't apply
if the dictionary is used only internally and we use global properties.


> 
> > 
> > > 
> > > Pushing common features for type into global properties also makes sense,
> > > isn't global properties made specifically for this?
> > 
> > Yes, but do we want to make "-cpu" affect every single
> > -device/device_add call for CPUs, or only the ones created on
> > initialization? That's the main question, I believe.
> If user overrides default cpu model features with -cpu option it would be only
> logical for hot-plugged cpus to use that overrides as well, instead of creating
> cpu that would be different (it may confuse guests, to the point of crashing).
> IMHO, It would be surprising for user to get a different cpu on hot-plug than
> already existing ones, in most cases.
> 
> And if user likes to shoot in its own foot, it will be possible to override
> global features by adding extra foo=xxx to a specific device_add/-device
> command.

This is a good argument, and I am now convinced that translating "-cpu"
to global properties would be a good idea.  :)


> > > 
> > > If common features are pushed in global properties, cpu hot-plug could look
> > > like:
> > >    device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
> > > and all common features that user has overridden on command line would
> > > applied to a new cpu without extra work from user.
> > 
> > CPU hotplug is an interesting case: if thinking only about the CPUs
> > created from the command-line the "-global vs -device" question wouldn't
> > make such a big difference. But in the case of device_add for CPU
> > hotplug, it would be very different. But I am not sure which option is
> > less surprising for users.
> > 
> > What others think? Andreas? Should "-cpu" affect only the CPUs created
> > on initialization, or all CPUs created by -device/device_add for the
> > lifetime of the QEMU process?  (in other words, should the options to
> > "-cpu" be translated to "-device" arguments, or to "-global" arguments?)
> > 
> > (BTW about the device_add example above: I believe we will want the
> > hotplug interface to be based on a "cpu_index" parameter/property, to
> > abstract out the complexity of the APIC ID calculation)
> there was other opinion about cpu_index
> http://permalink.gmane.org/gmane.comp.emulators.qemu/151626
>  
> However if board will allocate sockets (Anthony suggested to try links for
> this) then user won't have to calculate ID, instead of user will have to
> enumerate existing cpu links and use IDs embedded in it for managing
> cpus.

I don't care if we use "IDs", "links", a "cpu_index" property, socket
objects, socket IDs, or whatever. I just strongly recommend we don't
force users/management-tools to calculate the APIC ID themselves.

We could even kill the cpu_index field (as suggested at the URL above).
But to allow the APIC ID to be calculated, we need to let the CPU know
where it is "located" in the system (its
cpu_index/socket_index/whatever), and the CPU topology of the system,
somehow.

(I believe asked multiple times why devices can't know their parents by
design [so they can't just ask the sockets/motherboard for the
topology], and I never got an answer...)


[...]
> > > 
> > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
> > > of global properties as well.
> > > 
> > > I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
> > > translated into something like:
> > >    -global foo-cpu.foo=x
> > >    -device foo-cpu,id=a,node=nodeA
> > >    -device foo-cpu,id=b,node=nodeB
> > >    ...
> > >    or
> > >    -device foo-cpu,id=f(cpuN,nodeN)
> > >    ...
> > > where common options are pushed into global properties and only instance
> > > specific ones are specified per instance.
> > > 
> > > Do you see any issues ahead that global properties would introduce?
> > 
> > I see no issue, except that it was not what I was expecting at first. I
> > just don't know which one is less surprising for users (including
> > libvirt).
> users shouldn't be affected, -cpu will continue to works the same way until we
> start deprecating legacy behavior.

I mean: users will be affected by our decision once they start using
device_add for CPU hotplug (then both approaches would have different
results).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Igor Mammedov]

On Fri, 21 Dec 2012 11:50:26 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote:
> [...]
> > > All above said, I am not strongly against your approach, but I believe
> > > we could try to make the feature string parsing code generic and
> > > reusable (with target-specific properties or callbacks), instead of
> > > having to write a new generic feature string parsing function.
> > > 
> > > Anyway, the above is just bikeshedding to me, and your approach is an
> > > improvement, already, even if we try to make the parser more generic
> > > later.
> > >
> > feature string parser in this series isn't meant to be generic, it's just
> > a simplification of the current function and splitting parsing
> > features & setting properties into separate steps. kind of what you did with
> > cpu_name+feature_string split.
> > 
> > It's not clear to me what you mean under legacy properties, could you
> > throw in a hack using as example xlevel feature to demonstrate what you mean,
> > please?
> 
> I will try to show it below, but note that we can try to do that _after_
> we introduce the true/final/clean properties that won't have those
> hacks, and after changing the current parse_featurestr() implementation
> to contain the compatibility hacks (that's the work you are already
> doing).
> 
> I was thinking about using the "legacy-%s" feature inside
> qdev_prop_parse(), this way:
> 
> /* Generic featurestr parsing function */
> void generic_parse_featurestr(CPUClass *cc, const char *featurestr)
> {
>     opts = g_strsplit(featurestr, ",")
>     for (each opt in opts) {
>         if (opt[0] == '+') {
>             dict[opt+1] = 'true';
>         } else if (opt[0] == '
>             dict[opt+1] = 'false ;
>         } else if (opt contains '=) {
>             key, value = g_strsplit(opt, "=");
>             dict[key] = value;
>         }
>     }
It's only sparc and i386 targets that use +- format for features.
I'd rather avoid exposing +- format parsing to other targets.
BTW it's a bit more complex for i386 target due to f-* name conversion of
features into properties.

We should work towards deprecation of this format and not introducing it to
other targets. When all this legacy stuff is deprecated we should have only
foo=xxx format left as the other devices, then there won't be need in
dedicated parser.

 

>     for (each key in dict) {
>         /* qdev_prop_set_globals() uses qdev_prop_parse(), that already
>          * checks for a "legacy-<name>" property.
>          */
>         qdev_add_one_global(key, dict[key]);
>     }
> }
>
In addition, having per target converter makes deprecation easier and possible
to do on per target basis instead of fixing everything at once.

> 
> /* target-specific handling of legacy values for the -cpu option */
> void cpu_set_legacy_xlevel(X86CPU *cpu, Visitor *v, ...)
> {
>     int64_t i;
>     visit_type_int(v, &i, ...);
>     if (i < 0x80000000)
>         i += 0x80000000;
>     object_property_set_int(cpu, i, "xlevel");
> }
> 
> void cpu_set_legacy_foobar(...)
> {
>     ...
> }
> 
> void cpu_x86_class_init(CPUClass *cc, ...)
> {
>     ...
>     qdev_class_add_property("legacy-xlevel", cpu_set_legacy_xlevel, NULL, ...); /* only a setter, no getter */
>     qdev_class_add_property("legacy-foobar", cpu_set_legacy_foobar, NULL, ...); /* only a setter, no getter */
> }
> 
> 
> Note:
> 
> - We don't have a qdev_class_add_property() function, so we have to add
>   the legacy property to the ->props array;
> - The props array is based on name/offset/type, so we would need to
>   define (for example) a "PropertyInfo cpu_prop_xlevel" struct;
>   - On the other hand, setting a ->parse function on PropertyInfo gives
>     us the legacy-* property registration for free when registering
>     "xlevel" (see device_initfn()/qdev_property_add_legacy())
>   - But I would prefer a qdev_class_add_property() interface, as it
>     would be much simpler...
> 
> So, in the end it could be too complex due to the interfaces provided by
> qdev.
Concentrating legacy conversion to one function /especially if function
already exists/ and gradually srinking it to NOP looks a bit simpler 
than introducing new legacy-* code and/or qdev interfaces.

> 
> > 
> > > But the questions below about global properties seem important/interesting:
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > >         // with classes and global properties we could get rid of the
> > > > > > field // cpu_model_str in CPUxxxState
> > > > > >         return prop_list, cpu_class_name        
> > > > > >     }
> > > > > > 
> > > > > >     foreach (prop_list)
> > > > > >         add_global_prop(cpu_class_name,prop,val)
> > > > > > 
> > > > > >     // could be later transformed to property of board object and
> > > > > >     // we could get rid of one more global var
> > > > > >     cpu_model = cpu_class_name
> > > > > 
> > > > > The cpu_model string will be immediately used to create the CPU object
> > > > > just after this code runs. So do we really need to do the parsing before
> > > > > creating the CPU object and use global properties for that? It looks
> > > > > like unnecessary added complexity.
> > > > It might be used immediately or might be not in case of hot-plug or
> > > > copy_cpu() or depending where converter is called. Ideally there won't be
> > > > cpu_model string at all.
> > > 
> > > I don't see the difference between storing a cpu_model string and
> > > storing a dictionary to be used later. It would just be a
> > > micro-optimization for the code that creates new CPUs.
> > Dictionary is an implementation detail, a temporary list of properties that
> > would be applied to global properties and then list destroyed. It is there now
> > because:
> >  1. it's just a convenient data structure to implement current weird +-foo
> >     semantic
> >  2. a necessary intermediate step to hold list of properties because
> >     properties/features is not yet converted into static properties. Properties
> >     could/should be pushed directly into global properties if possible (i.e.
> >     when property is static), then there won't be any need in intermediate
> >     property list at all.
> > 
> > There is not compelling reason to use dictionary in generic code, it could be
> > removed or be hidden inside current featurestr parser.
> 
> I wasn't taking into account that the dictionary would be simply used to
> feed the global property list, so nevermind.
> 
> > 
> > > 
> > > 
> > > > 
> > > > Presently all created CPUs use the same cpu_model string so parsing it
> > > > only once instead of N times would be already an improvement.
> > > 
> > > I don't believe we would want to make the code more complex just to
> > > optimize string parsing that is run a few times during QEMU
> > > initialization. That would be unnecessary optimization.
> > If we move generalized parsing out into vl.c & *-user/main.c option parsing
> > code, it would simplify every target. I would be better to do option parsing
> > at the time when options are parsed in a single place and let boards/targets to
> > concentrate on creating cpus (without caring about options at all, not really
> > their job).
> 
> Even if we did it the "inefficient" way (parsing it multiple times), it
> could be moved to generic CPU creation code.
> 
> Anyway, my assumption about "making the code more complex" doesn't apply
> if the dictionary is used only internally and we use global properties.
> 
> 
> > 
> > > 
> > > > 
> > > > Pushing common features for type into global properties also makes sense,
> > > > isn't global properties made specifically for this?
> > > 
> > > Yes, but do we want to make "-cpu" affect every single
> > > -device/device_add call for CPUs, or only the ones created on
> > > initialization? That's the main question, I believe.
> > If user overrides default cpu model features with -cpu option it would be only
> > logical for hot-plugged cpus to use that overrides as well, instead of creating
> > cpu that would be different (it may confuse guests, to the point of crashing).
> > IMHO, It would be surprising for user to get a different cpu on hot-plug than
> > already existing ones, in most cases.
> > 
> > And if user likes to shoot in its own foot, it will be possible to override
> > global features by adding extra foo=xxx to a specific device_add/-device
> > command.
> 
> This is a good argument, and I am now convinced that translating "-cpu"
> to global properties would be a good idea.  :)
> 
> 
> > > > 
> > > > If common features are pushed in global properties, cpu hot-plug could look
> > > > like:
> > > >    device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
> > > > and all common features that user has overridden on command line would
> > > > applied to a new cpu without extra work from user.
> > > 
> > > CPU hotplug is an interesting case: if thinking only about the CPUs
> > > created from the command-line the "-global vs -device" question wouldn't
> > > make such a big difference. But in the case of device_add for CPU
> > > hotplug, it would be very different. But I am not sure which option is
> > > less surprising for users.
> > > 
> > > What others think? Andreas? Should "-cpu" affect only the CPUs created
> > > on initialization, or all CPUs created by -device/device_add for the
> > > lifetime of the QEMU process?  (in other words, should the options to
> > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?)
> > > 
> > > (BTW about the device_add example above: I believe we will want the
> > > hotplug interface to be based on a "cpu_index" parameter/property, to
> > > abstract out the complexity of the APIC ID calculation)
> > there was other opinion about cpu_index
> > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626
> >  
> > However if board will allocate sockets (Anthony suggested to try links for
> > this) then user won't have to calculate ID, instead of user will have to
> > enumerate existing cpu links and use IDs embedded in it for managing
> > cpus.
> 
> I don't care if we use "IDs", "links", a "cpu_index" property, socket
> objects, socket IDs, or whatever. I just strongly recommend we don't
> force users/management-tools to calculate the APIC ID themselves.
> 
> We could even kill the cpu_index field (as suggested at the URL above).
> But to allow the APIC ID to be calculated, we need to let the CPU know
> where it is "located" in the system (its
> cpu_index/socket_index/whatever), and the CPU topology of the system,
> somehow.
> 
> (I believe asked multiple times why devices can't know their parents by
> design [so they can't just ask the sockets/motherboard for the
> topology], and I never got an answer...)
Can we create core_id/package_id/node_id properties for CPU and use them when
creating/hot-plugging CPUs for APICID calculation?


> 
> 
> [...]
> > > > 
> > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
> > > > of global properties as well.
> > > > 
> > > > I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
> > > > translated into something like:
> > > >    -global foo-cpu.foo=x
> > > >    -device foo-cpu,id=a,node=nodeA
> > > >    -device foo-cpu,id=b,node=nodeB
> > > >    ...
> > > >    or
> > > >    -device foo-cpu,id=f(cpuN,nodeN)
> > > >    ...
> > > > where common options are pushed into global properties and only instance
> > > > specific ones are specified per instance.
> > > > 
> > > > Do you see any issues ahead that global properties would introduce?
> > > 
> > > I see no issue, except that it was not what I was expecting at first. I
> > > just don't know which one is less surprising for users (including
> > > libvirt).
> > users shouldn't be affected, -cpu will continue to works the same way until we
> > start deprecating legacy behavior.
> 
> I mean: users will be affected by our decision once they start using
> device_add for CPU hotplug (then both approaches would have different
> results).
One more reason to make them global /i.e. to have the same behavior/

> 
> -- 
> Eduardo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 10/20] target-i386: prepare cpu_x86_parse_featurestr() to return a set of key, value property pairs

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:33:04PM +0100, Igor Mammedov wrote:
> On Fri, 21 Dec 2012 11:50:26 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 21, 2012 at 01:56:56AM +0100, Igor Mammedov wrote:
> > [...]
> > > > All above said, I am not strongly against your approach, but I believe
> > > > we could try to make the feature string parsing code generic and
> > > > reusable (with target-specific properties or callbacks), instead of
> > > > having to write a new generic feature string parsing function.
> > > > 
> > > > Anyway, the above is just bikeshedding to me, and your approach is an
> > > > improvement, already, even if we try to make the parser more generic
> > > > later.
> > > >
> > > feature string parser in this series isn't meant to be generic, it's just
> > > a simplification of the current function and splitting parsing
> > > features & setting properties into separate steps. kind of what you did with
> > > cpu_name+feature_string split.
> > > 
> > > It's not clear to me what you mean under legacy properties, could you
> > > throw in a hack using as example xlevel feature to demonstrate what you mean,
> > > please?
> > 
> > I will try to show it below, but note that we can try to do that _after_
> > we introduce the true/final/clean properties that won't have those
> > hacks, and after changing the current parse_featurestr() implementation
> > to contain the compatibility hacks (that's the work you are already
> > doing).
> > 
> > I was thinking about using the "legacy-%s" feature inside
> > qdev_prop_parse(), this way:
> > 
> > /* Generic featurestr parsing function */
> > void generic_parse_featurestr(CPUClass *cc, const char *featurestr)
> > {
> >     opts = g_strsplit(featurestr, ",")
> >     for (each opt in opts) {
> >         if (opt[0] == '+') {
> >             dict[opt+1] = 'true';
> >         } else if (opt[0] == '
> >             dict[opt+1] = 'false ;
> >         } else if (opt contains '=) {
> >             key, value = g_strsplit(opt, "=");
> >             dict[key] = value;
> >         }
> >     }
> It's only sparc and i386 targets that use +- format for features.
> I'd rather avoid exposing +- format parsing to other targets.
> BTW it's a bit more complex for i386 target due to f-* name conversion of
> features into properties.
> 
> We should work towards deprecation of this format and not introducing it to
> other targets. When all this legacy stuff is deprecated we should have only
> foo=xxx format left as the other devices, then there won't be need in
> dedicated parser.

OK, that would be a good reason to have a separate parser function.

[...]
> > > > > If common features are pushed in global properties, cpu hot-plug could look
> > > > > like:
> > > > >    device_add driver=cpu_x86_foo_class,[apicid|id]=xxx
> > > > > and all common features that user has overridden on command line would
> > > > > applied to a new cpu without extra work from user.
> > > > 
> > > > CPU hotplug is an interesting case: if thinking only about the CPUs
> > > > created from the command-line the "-global vs -device" question wouldn't
> > > > make such a big difference. But in the case of device_add for CPU
> > > > hotplug, it would be very different. But I am not sure which option is
> > > > less surprising for users.
> > > > 
> > > > What others think? Andreas? Should "-cpu" affect only the CPUs created
> > > > on initialization, or all CPUs created by -device/device_add for the
> > > > lifetime of the QEMU process?  (in other words, should the options to
> > > > "-cpu" be translated to "-device" arguments, or to "-global" arguments?)
> > > > 
> > > > (BTW about the device_add example above: I believe we will want the
> > > > hotplug interface to be based on a "cpu_index" parameter/property, to
> > > > abstract out the complexity of the APIC ID calculation)
> > > there was other opinion about cpu_index
> > > http://permalink.gmane.org/gmane.comp.emulators.qemu/151626
> > >  
> > > However if board will allocate sockets (Anthony suggested to try links for
> > > this) then user won't have to calculate ID, instead of user will have to
> > > enumerate existing cpu links and use IDs embedded in it for managing
> > > cpus.
> > 
> > I don't care if we use "IDs", "links", a "cpu_index" property, socket
> > objects, socket IDs, or whatever. I just strongly recommend we don't
> > force users/management-tools to calculate the APIC ID themselves.
> > 
> > We could even kill the cpu_index field (as suggested at the URL above).
> > But to allow the APIC ID to be calculated, we need to let the CPU know
> > where it is "located" in the system (its
> > cpu_index/socket_index/whatever), and the CPU topology of the system,
> > somehow.
> > 
> > (I believe asked multiple times why devices can't know their parents by
> > design [so they can't just ask the sockets/motherboard for the
> > topology], and I never got an answer...)
> Can we create core_id/package_id/node_id properties for CPU and use them when
> creating/hot-plugging CPUs for APICID calculation?

That's how I think we have to do it, but It would be interesting to not
require the caller to keep track of all those IDs and explicitly set
them on the device_add arguments. I believe this is similar to the
allocation of addresses in many device busses, so there may be an
existing solution for this that I don't know about.


> > [...]
> > > > > 
> > > > > And as I explained before, cpu hot-plug and copy_cpu() will benefit from usage
> > > > > of global properties as well.
> > > > > 
> > > > > I think "-cpu foo-cpu,foo=x -smp n  -numa node,cpus=..." could be
> > > > > translated into something like:
> > > > >    -global foo-cpu.foo=x
> > > > >    -device foo-cpu,id=a,node=nodeA
> > > > >    -device foo-cpu,id=b,node=nodeB
> > > > >    ...
> > > > >    or
> > > > >    -device foo-cpu,id=f(cpuN,nodeN)
> > > > >    ...
> > > > > where common options are pushed into global properties and only instance
> > > > > specific ones are specified per instance.
> > > > > 
> > > > > Do you see any issues ahead that global properties would introduce?
> > > > 
> > > > I see no issue, except that it was not what I was expecting at first. I
> > > > just don't know which one is less surprising for users (including
> > > > libvirt).
> > > users shouldn't be affected, -cpu will continue to works the same way until we
> > > start deprecating legacy behavior.
> > 
> > I mean: users will be affected by our decision once they start using
> > device_add for CPU hotplug (then both approaches would have different
> > results).
> One more reason to make them global /i.e. to have the same behavior/

True.

(That said, maybe we could even save the CPU model name and topology in
global properties so we could just use "device_add cpu-package" [with no
arguments] and have everything magically set [including the CPU model]
depending on the globals?)

-- 
Eduardo

[Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
  v2:
   - add missing 'return -1' on exit if error is not NULL,
           Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v3:
   - set error if cpu_model is empty
---
 target-i386/cpu.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..97cce89 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        error_setg(&error, "Invalid/empty CPU model name");
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1568,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
+        return -1;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Eduardo Habkost]

On Thu, Dec 27, 2012 at 03:59:23PM +0100, Igor Mammedov wrote:
> freeing resources in one place would require setting 'error'
> to not NULL, so add some more error reporting before jumping to
> exit branch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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


> ---
>   v2:
>    - add missing 'return -1' on exit if error is not NULL,
>            Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
>   v3:
>    - set error if cpu_model is empty
> ---
>  target-i386/cpu.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3b9bbfe..97cce89 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> -        goto error;
> +        error_setg(&error, "Invalid/empty CPU model name");
> +        goto out;
>      }
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
>      if (cpu_x86_find_by_name(def, name) < 0) {
> -        goto error;
> +        error_setg(&error, "Unable to find CPU definition: %s", name);
> +        goto out;
>      }
>  
>      def->kvm_features |= kvm_default_features;
> @@ -1566,22 +1568,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> -        goto error;
> +        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
> +        goto out;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
>  
> +out:
> +    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -        goto error;
> +        return -1;
>      }
> -
> -    g_strfreev(model_pieces);
>      return 0;
> -error:
> -    g_strfreev(model_pieces);
> -    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

[Igor Mammedov]

freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
   - add missing 'return -1' on exit if error is not NULL,
           Spotted-By: Eduardo Habkost <ehabkost@redhat.com>
  v3:
   - set error if cpu_model is empty
---
 target-i386/cpu.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..97cce89 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        error_setg(&error, "Invalid/empty CPU model name");
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1568,20 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
+        return -1;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.1