[Qemu-devel] [PATCH 00/10] cpus: make "-cpu cpux, features" global properties

[Qemu-devel] [PATCH 00/10] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

Changelog:
  since RFC:
    - include "target-i386: Remove xlevel & hv-spinlocks option fixups"
    - use Eduardo's version of:
        target-i386: cpu: consolidate calls of object_property_parse...
    - fix error handling of +-feat
    - add comment why +-feat is special
    - add comment to remove +-feat static vars
    - reuse x86_cpu_class_get_model_name()

Current CLI option -cpu cpux,features serves as template
for all created cpus of type: cpux. However QEMU parses
"features" every time it creates a cpu instance and applies
them to it while doing parsing.

That doesn't work well with -device/device_add infrastructure
as it has no idea about cpu specific hooks that's used for
parsing "features".
In order to make -device/device_add utilize "-cpu features"
template convert it into a set of global properties, so that
every new CPU created will have them applied automatically.

That also allows to parse features only once, instread of
doing it for every CPU instance being created.

Git tree for testing:
  https://github.com/imammedo/qemu.git cpu_parse_into_global_props_V1 
web view:
  https://github.com/imammedo/qemu/commits/cpu_parse_into_global_props_V1

Eduardo Habkost (2):
  target-i386: Remove xlevel & hv-spinlocks option fixups
  target-i386: cpu: consolidate calls of object_property_parse() in
    x86_cpu_parse_featurestr

Igor Mammedov (8):
  target-i386: cpu: move features logic that requires CPUState to
    realize time
  target-i386: cpu: move xcc->kvm_required check to realize time
  target-i386: cpu: use cpu_generic_init() in cpu_x86_init()
  target-i386: print obsolete warnings if +-features are used
  target-sparc: cpu: use sparc_cpu_parse_features() directly
  cpu: use CPUClass->parse_features() as convertor to global properties
  arm: virt: parse cpu_model only once
  pc: parse cpu features only once

 hw/arm/virt.c      |  41 ++++-----
 hw/i386/pc.c       |  37 ++++++--
 include/qom/cpu.h  |   2 +-
 qom/cpu.c          |  29 +++---
 target-i386/cpu.c  | 252 ++++++++++++++++++++++-------------------------------
 target-i386/cpu.h  |   1 -
 target-sparc/cpu.c |   7 +-
 7 files changed, 176 insertions(+), 193 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/10] target-i386: Remove xlevel & hv-spinlocks option fixups

[Igor Mammedov]

From: Eduardo Habkost <ehabkost@redhat.com>

The "fixup will be removed in future versions" warnings are
present since QEMU 1.7.0, at least, so users should have fixed
their scripts and configurations, already.

In the case of libvirt users, libvirt doesn't use the "xlevel"
option, and already rejects HyperV spinlock retry count < 0xFFF.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 36 +-----------------------------------
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3fbc6f3..07bbab7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1944,7 +1944,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     FeatureWordArray plus_features = { 0 };
     /* Features to be removed */
     FeatureWordArray minus_features = { 0 };
-    uint32_t numvalue;
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
 
@@ -1959,23 +1958,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             feat2prop(featurestr);
-            if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                char num[32];
-
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                if (numvalue < 0x80000000) {
-                    error_report("xlevel value shall always be >= 0x80000000"
-                                 ", fixup will be removed in future versions");
-                    numvalue += 0x80000000;
-                }
-                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
-            } else if (!strcmp(featurestr, "tsc-freq")) {
+            if (!strcmp(featurestr, "tsc-freq")) {
                 int64_t tsc_freq;
                 char *err;
                 char num[32];
@@ -1989,23 +1972,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
                 object_property_parse(OBJECT(cpu), num, "tsc-frequency",
                                       &local_err);
-            } else if (!strcmp(featurestr, "hv-spinlocks")) {
-                char *err;
-                const int min = 0xFFF;
-                char num[32];
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                if (numvalue < min) {
-                    error_report("hv-spinlocks value shall always be >= 0x%x"
-                                 ", fixup will be removed in future versions",
-                                 min);
-                    numvalue = min;
-                }
-                snprintf(num, sizeof(num), "%" PRId32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
             } else {
                 object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
             }
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/10] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

Making x86_cpu_parse_featurestr() a pure convertor
of legacy feature string into global properties, needs
it to be called before a CPU instance is created so
parser shouldn't modify CPUState directly or access
it at all. Hence move current hack that directly pokes
into CPUState, to set/unset +-feats, from parser to
CPU's realize method.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
 - rewrite commit message
 - add comment why +-feat is special
 - add TODO comment to remove plus_features & minus_features static vars
---
 target-i386/cpu.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 07bbab7..79a9b61 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1932,6 +1932,14 @@ static inline void feat2prop(char *s)
     }
 }
 
+/* Compatibily hack to maintain legacy +-feat semantic,
+ * where +-feat overwrites any feature set by
+ * feat=on|feat even if the later is parsed after +-feat
+ * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
+ */
+static FeatureWordArray plus_features = { 0 };
+static FeatureWordArray minus_features = { 0 };
+
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
 static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
@@ -1939,12 +1947,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
 {
     X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
-    FeatureWord w;
-    /* Features to be added */
-    FeatureWordArray plus_features = { 0 };
-    /* Features to be removed */
-    FeatureWordArray minus_features = { 0 };
-    CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
 
     featurestr = features ? strtok(features, ",") : NULL;
@@ -1985,18 +1987,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         }
         featurestr = strtok(NULL, ",");
     }
-
-    if (cpu->host_features) {
-        for (w = 0; w < FEATURE_WORDS; w++) {
-            env->features[w] =
-                x86_cpu_get_supported_feature_word(w, cpu->migratable);
-        }
-    }
-
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        env->features[w] |= plus_features[w];
-        env->features[w] &= ~minus_features[w];
-    }
 }
 
 /* Print all cpuid feature names in featureset
@@ -2878,12 +2868,28 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
     static bool ht_warned;
+    FeatureWord w;
 
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
     }
 
+    /* TODO: convert plus_features & minus_features static vars
+     * to global properties, once broken host_features is fixed
+     */
+    if (cpu->host_features) {
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            env->features[w] =
+                x86_cpu_get_supported_feature_word(w, cpu->migratable);
+        }
+    }
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        cpu->env.features[w] |= plus_features[w];
+        cpu->env.features[w] &= ~minus_features[w];
+    }
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/10] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:44PM +0200, Igor Mammedov wrote:
> Making x86_cpu_parse_featurestr() a pure convertor
> of legacy feature string into global properties, needs
> it to be called before a CPU instance is created so
> parser shouldn't modify CPUState directly or access
> it at all. Hence move current hack that directly pokes
> into CPUState, to set/unset +-feats, from parser to
> CPU's realize method.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

I will just edit a comment below, when applying, for clarity:

[...]
> +    /* TODO: convert plus_features & minus_features static vars
> +     * to global properties, once broken host_features is fixed
> +     */

I will rewrite this to:

    /*TODO: cpu->host_features inclurrectly overwrites features
     * set using "feat=on|off". Once we fix this, we can convert
     * plus_features & minus_features to global properties
     * inside x86_cpu_parse_featurestr() too.
     */

> +    if (cpu->host_features) {
> +        for (w = 0; w < FEATURE_WORDS; w++) {
> +            env->features[w] =
> +                x86_cpu_get_supported_feature_word(w, cpu->migratable);
> +        }
> +    }
> +
> +    for (w = 0; w < FEATURE_WORDS; w++) {
> +        cpu->env.features[w] |= plus_features[w];
> +        cpu->env.features[w] &= ~minus_features[w];
> +    }
> +
>      if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
>          env->cpuid_level = 7;
>      }
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 02/10] target-i386: cpu: move features logic that requires CPUState to realize time

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On 06/07/2016 02:25 PM, Eduardo Habkost wrote:

> [...]
>> +    /* TODO: convert plus_features & minus_features static vars
>> +     * to global properties, once broken host_features is fixed
>> +     */
> 
> I will rewrite this to:
> 
>     /*TODO: cpu->host_features inclurrectly overwrites features

Was that supposed to be "incorrectly" or "currently"?

>      * set using "feat=on|off". Once we fix this, we can convert
>      * plus_features & minus_features to global properties
>      * inside x86_cpu_parse_featurestr() too.
>      */


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/10] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Tue, Jun 07, 2016 at 03:07:33PM -0600, Eric Blake wrote:
> On 06/07/2016 02:25 PM, Eduardo Habkost wrote:
> 
> > [...]
> >> +    /* TODO: convert plus_features & minus_features static vars
> >> +     * to global properties, once broken host_features is fixed
> >> +     */
> > 
> > I will rewrite this to:
> > 
> >     /*TODO: cpu->host_features inclurrectly overwrites features
> 
> Was that supposed to be "incorrectly" or "currently"?

Wow, that's an weird typo. I wrote "incorrectly" inclurrectly.

-- 
Eduardo

[Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Igor Mammedov]

it will allow to drop custom cpu_x86_init() and use
cpu_generic_init() insteadi, reducing cpu_x86_create()
to a simple 3-liner.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Eduardo Habkost <ehabkost@redhat.com>
---
v1:
  - use x86_cpu_class_get_model_name() for getting cpu model name
---
 target-i386/cpu.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 79a9b61..ae67a15 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -677,6 +677,14 @@ static ObjectClass *x86_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
+{
+    const char *class_name = object_class_get_name(OBJECT_CLASS(cc));
+    assert(g_str_has_suffix(class_name, X86_CPU_TYPE_SUFFIX));
+    return g_strndup(class_name,
+                     strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
+}
+
 struct X86CPUDefinition {
     const char *name;
     uint32_t level;
@@ -2169,7 +2177,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
-    X86CPUClass *xcc;
     ObjectClass *oc;
     gchar **model_pieces;
     char *name, *features;
@@ -2188,12 +2195,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
         error_setg(&error, "Unable to find CPU definition: %s", name);
         goto out;
     }
-    xcc = X86_CPU_CLASS(oc);
-
-    if (xcc->kvm_required && !kvm_enabled()) {
-        error_setg(&error, "CPU model '%s' requires KVM", name);
-        goto out;
-    }
 
     cpu = X86_CPU(object_new(object_class_get_name(oc)));
 
@@ -2870,6 +2871,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     static bool ht_warned;
     FeatureWord w;
 
+    if (xcc->kvm_required && !kvm_enabled()) {
+        char *name = x86_cpu_class_get_model_name(xcc);
+        error_setg(&local_err, "CPU model '%s' requires KVM", name);
+        g_free(name);
+        goto out;
+    }
+
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:45PM +0200, Igor Mammedov wrote:
> it will allow to drop custom cpu_x86_init() and use
> cpu_generic_init() insteadi, reducing cpu_x86_create()
> to a simple 3-liner.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Eduardo Habkost <ehabkost@redhat.com>

The "Reviewed-by: " prefix is missing, but this can be fixed when
applying the patch.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:45PM +0200, Igor Mammedov wrote:
> it will allow to drop custom cpu_x86_init() and use
> cpu_generic_init() insteadi, reducing cpu_x86_create()
> to a simple 3-liner.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Eduardo Habkost <ehabkost@redhat.com>

Applied to x86-next. Thanks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:45PM +0200, Igor Mammedov wrote:
> it will allow to drop custom cpu_x86_init() and use
> cpu_generic_init() insteadi, reducing cpu_x86_create()
> to a simple 3-liner.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Eduardo Habkost <ehabkost@redhat.com>

This triggers an assert when trying to use -cpu host with TCG:

  # ./x86_64-softmmu/qemu-system-x86_64 -machine accel=tcg -cpu host -nographic
  qemu-system-x86_64: /root/qemu/target-i386/cpu.c:1558: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
  Aborted

I will squash the following fix in it:

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3f886a5..dcbfa0b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1555,16 +1555,17 @@ static void host_x86_cpu_initfn(Object *obj)
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
 
-    assert(kvm_enabled());
-
     /* We can't fill the features array here because we don't know yet if
      * "migratable" is true or false.
      */
     cpu->host_features = true;
 
-    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
-    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    /* If KVM is disabled, x86_cpu_realizefn() will report an error later */
+    if (kvm_enabled()) {
+        env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+        env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+        env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    }
 
     object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
 }

-- 
Eduardo

Re: [Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Igor Mammedov]

On Thu, 9 Jun 2016 15:34:30 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 06, 2016 at 05:16:45PM +0200, Igor Mammedov wrote:
> > it will allow to drop custom cpu_x86_init() and use
> > cpu_generic_init() insteadi, reducing cpu_x86_create()
> > to a simple 3-liner.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Eduardo Habkost <ehabkost@redhat.com>  
> 
> This triggers an assert when trying to use -cpu host with TCG:
> 
>   # ./x86_64-softmmu/qemu-system-x86_64 -machine accel=tcg -cpu host -nographic
>   qemu-system-x86_64: /root/qemu/target-i386/cpu.c:1558: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
>   Aborted
This is not related to this patch and it was there before.

Maybe a separate patch saying that it replaces assert() in
host_x86_cpu_initfn() with error check and user friendly
error message at later stage.

> 
> I will squash the following fix in it:
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3f886a5..dcbfa0b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1555,16 +1555,17 @@ static void host_x86_cpu_initfn(Object *obj)
>      CPUX86State *env = &cpu->env;
>      KVMState *s = kvm_state;
>  
> -    assert(kvm_enabled());
> -
>      /* We can't fill the features array here because we don't know yet if
>       * "migratable" is true or false.
>       */
>      cpu->host_features = true;
>  
> -    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> -    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> -    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    /* If KVM is disabled, x86_cpu_realizefn() will report an error later */
> +    if (kvm_enabled()) {
> +        env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> +        env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> +        env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    }
>  
>      object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
>  }
> 

Re: [Qemu-devel] [PATCH 03/10] target-i386: cpu: move xcc->kvm_required check to realize time

[Eduardo Habkost]

On Fri, Jun 10, 2016 at 09:15:44AM +0200, Igor Mammedov wrote:
> On Thu, 9 Jun 2016 15:34:30 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 06, 2016 at 05:16:45PM +0200, Igor Mammedov wrote:
> > > it will allow to drop custom cpu_x86_init() and use
> > > cpu_generic_init() insteadi, reducing cpu_x86_create()
> > > to a simple 3-liner.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Eduardo Habkost <ehabkost@redhat.com>  
> > 
> > This triggers an assert when trying to use -cpu host with TCG:
> > 
> >   # ./x86_64-softmmu/qemu-system-x86_64 -machine accel=tcg -cpu host -nographic
> >   qemu-system-x86_64: /root/qemu/target-i386/cpu.c:1558: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
> >   Aborted
> This is not related to this patch and it was there before.

It is caused by this patch. The assert() was there, but you
removed the check on cpu_x86_create() that prevented it from
being triggered.

> 
> Maybe a separate patch saying that it replaces assert() in
> host_x86_cpu_initfn() with error check and user friendly
> error message at later stage.

We could apply the fix below as a separate patch before 03/10, or
just squash the fix in it. What do you think? (We can't do it as
a follow-up patch.)

If you prefer to suggest another fix, I will remove the series
from x86-next and wait for v3.

> 
> > 
> > I will squash the following fix in it:
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3f886a5..dcbfa0b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1555,16 +1555,17 @@ static void host_x86_cpu_initfn(Object *obj)
> >      CPUX86State *env = &cpu->env;
> >      KVMState *s = kvm_state;
> >  
> > -    assert(kvm_enabled());
> > -
> >      /* We can't fill the features array here because we don't know yet if
> >       * "migratable" is true or false.
> >       */
> >      cpu->host_features = true;
> >  
> > -    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> > -    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> > -    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> > +    /* If KVM is disabled, x86_cpu_realizefn() will report an error later */
> > +    if (kvm_enabled()) {
> > +        env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> > +        env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> > +        env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> > +    }
> >  
> >      object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
> >  }
> > 
> 

-- 
Eduardo

[Qemu-devel] [PATCH] target-i386: Remove assert(kvm_enabled()) from host_x86_cpu_initfn()

[Eduardo Habkost]

The code will be changed to allow creation of the CPU object and
report kvm_required errors only at realizefn, so we need to make
the instance_init function more flexible.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
I am applying this before "Move xcc->kvm_required check to
realize time" to avoid triggering the assert().
---
 target-i386/cpu.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 647cd30..c91902f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1547,16 +1547,17 @@ static void host_x86_cpu_initfn(Object *obj)
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
 
-    assert(kvm_enabled());
-
     /* We can't fill the features array here because we don't know yet if
      * "migratable" is true or false.
      */
     cpu->host_features = true;
 
-    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
-    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    /* If KVM is disabled, cpu_x86_create() will already report an error */
+    if (kvm_enabled()) {
+        env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+        env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+        env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    }
 
     object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
 }
-- 
2.5.5

Re: [Qemu-devel] [PATCH] target-i386: Remove assert(kvm_enabled()) from host_x86_cpu_initfn()

[Igor Mammedov]

On Fri, 10 Jun 2016 08:48:33 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The code will be changed to allow creation of the CPU object and
> report kvm_required errors only at realizefn, so we need to make
> the instance_init function more flexible.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> I am applying this before "Move xcc->kvm_required check to
> realize time" to avoid triggering the assert().
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target-i386/cpu.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 647cd30..c91902f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1547,16 +1547,17 @@ static void host_x86_cpu_initfn(Object *obj)
>      CPUX86State *env = &cpu->env;
>      KVMState *s = kvm_state;
>  
> -    assert(kvm_enabled());
> -
>      /* We can't fill the features array here because we don't know yet if
>       * "migratable" is true or false.
>       */
>      cpu->host_features = true;
>  
> -    env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> -    env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> -    env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    /* If KVM is disabled, cpu_x86_create() will already report an error */
> +    if (kvm_enabled()) {
> +        env->cpuid_level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> +        env->cpuid_xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
> +        env->cpuid_xlevel2 = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    }
>  
>      object_property_set_bool(OBJECT(cpu), true, "pmu", &error_abort);
>  }

[Qemu-devel] [PATCH 04/10] target-i386: cpu: use cpu_generic_init() in cpu_x86_init()

[Igor Mammedov]

now cpu_x86_init() does nothing more or less
than duplicating cpu_generic_init() logic.
So simplify it by using cpu_generic_init().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ae67a15..349b971 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2217,25 +2217,7 @@ out:
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-    Error *error = NULL;
-    X86CPU *cpu;
-
-    cpu = cpu_x86_create(cpu_model, &error);
-    if (error) {
-        goto out;
-    }
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
-
-out:
-    if (error) {
-        error_report_err(error);
-        if (cpu != NULL) {
-            object_unref(OBJECT(cpu));
-            cpu = NULL;
-        }
-    }
-    return cpu;
+    return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/10] target-i386: cpu: use cpu_generic_init() in cpu_x86_init()

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:46PM +0200, Igor Mammedov wrote:
> now cpu_x86_init() does nothing more or less
> than duplicating cpu_generic_init() logic.
> So simplify it by using cpu_generic_init().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Applied to x86-next. Thanks.

-- 
Eduardo

[Qemu-devel] [PATCH 05/10] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

From: Eduardo Habkost <ehabkost@redhat.com>

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v1:
 - fix error handling in of +-feat, Igor Mammedov <imammedo@redhat.com>
 - rebase on top of
    "target-i386: Remove xlevel & hv-spinlocks option fixups"
---
 target-i386/cpu.c | 74 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 349b971..f791a06 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1957,43 +1957,67 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     char *featurestr; /* Single 'key=value" string being parsed */
     Error *local_err = NULL;
 
-    featurestr = features ? strtok(features, ",") : NULL;
+    if (!features) {
+        return;
+    }
 
-    while (featurestr) {
-        char *val;
+    for (featurestr = strtok(features, ",");
+         featurestr;
+         featurestr = strtok(NULL, ",")) {
+        const char *name;
+        const char *val = NULL;
+        char *eq = NULL;
+
+        /* Compatibility syntax: */
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+            continue;
         } else if (featurestr[0] == '-') {
             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            feat2prop(featurestr);
-            if (!strcmp(featurestr, "tsc-freq")) {
-                int64_t tsc_freq;
-                char *err;
-                char num[32];
-
-                tsc_freq = qemu_strtosz_suffix_unit(val, &err,
-                                               QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
-                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
-                                      &local_err);
-            } else {
-                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
             }
+            continue;
+        }
+
+        eq = strchr(featurestr, '=');
+        if (eq) {
+            *eq++ = 0;
+            val = eq;
         } else {
-            feat2prop(featurestr);
-            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
+            val = "on";
+        }
+
+        feat2prop(featurestr);
+        name = featurestr;
+
+        /* Special case: */
+        if (!strcmp(name, "tsc-freq")) {
+            int64_t tsc_freq;
+            char *err;
+            char num[32];
+
+            tsc_freq = qemu_strtosz_suffix_unit(val, &err,
+                                           QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
+            if (tsc_freq < 0 || *err) {
+                error_setg(errp, "bad numerical value %s", val);
+                return;
+            }
+            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
+            val = num;
+            name = "tsc-frequency";
         }
+
+        object_property_parse(OBJECT(cpu), val, name, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
         }
-        featurestr = strtok(NULL, ",");
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/10] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:47PM +0200, Igor Mammedov wrote:
> From: Eduardo Habkost <ehabkost@redhat.com>
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

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

A small suggestion in case we are going to need a new version of
this series, below:

> ---
> v1:
>  - fix error handling in of +-feat, Igor Mammedov <imammedo@redhat.com>
>  - rebase on top of
>     "target-i386: Remove xlevel & hv-spinlocks option fixups"
> ---
>  target-i386/cpu.c | 74 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 349b971..f791a06 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1957,43 +1957,67 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      char *featurestr; /* Single 'key=value" string being parsed */
>      Error *local_err = NULL;
>  
> -    featurestr = features ? strtok(features, ",") : NULL;
> +    if (!features) {
> +        return;
> +    }
>  
> -    while (featurestr) {
> -        char *val;
> +    for (featurestr = strtok(features, ",");
> +         featurestr;
> +         featurestr = strtok(NULL, ",")) {
> +        const char *name;
> +        const char *val = NULL;
> +        char *eq = NULL;
> +
> +        /* Compatibility syntax: */
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +            continue;

If you add an error_propagate() call to the end of the function,
this can be shortened to:

    if (local_err) {
        break;
    }

Or maybe the loop could be simply written as:

    for (featurestr = strtok(features, ",");
         featurestr && !local_err;
         featurestr = strtok(NULL, ","))

and we could avoid all the if (local_err) checks inside the loop
body.


>          } else if (featurestr[0] == '-') {
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            feat2prop(featurestr);
> -            if (!strcmp(featurestr, "tsc-freq")) {
> -                int64_t tsc_freq;
> -                char *err;
> -                char num[32];
> -
> -                tsc_freq = qemu_strtosz_suffix_unit(val, &err,
> -                                               QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> -                if (tsc_freq < 0 || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> -                    return;
> -                }
> -                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> -                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> -                                      &local_err);
> -            } else {
> -                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                return;
>              }
> +            continue;
> +        }
> +
> +        eq = strchr(featurestr, '=');
> +        if (eq) {
> +            *eq++ = 0;
> +            val = eq;
>          } else {
> -            feat2prop(featurestr);
> -            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
> +            val = "on";
> +        }
> +
> +        feat2prop(featurestr);
> +        name = featurestr;
> +
> +        /* Special case: */
> +        if (!strcmp(name, "tsc-freq")) {
> +            int64_t tsc_freq;
> +            char *err;
> +            char num[32];
> +
> +            tsc_freq = qemu_strtosz_suffix_unit(val, &err,
> +                                           QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> +            if (tsc_freq < 0 || *err) {
> +                error_setg(errp, "bad numerical value %s", val);
> +                return;
> +            }
> +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> +            val = num;
> +            name = "tsc-frequency";
>          }
> +
> +        object_property_parse(OBJECT(cpu), val, name, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
>          }
> -        featurestr = strtok(NULL, ",");
>      }
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Igor Mammedov]

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f791a06..31e5e6f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 error_propagate(errp, local_err);
                 return;
             }
+            fprintf(stderr,
+                "'+%s' is obsolete and will be removed in future, use '%s=on'",
+                featurestr + 1, featurestr + 1);
             continue;
         } else if (featurestr[0] == '-') {
             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
@@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 error_propagate(errp, local_err);
                 return;
             }
+            fprintf(stderr,
+                "'-%s' is obsolete and will be removed in future, use '%s=off'",
+                featurestr + 1, featurestr + 1);
             continue;
         }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Paolo Bonzini]

On 06/06/2016 17:16, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index f791a06..31e5e6f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                  error_propagate(errp, local_err);
>                  return;
>              }
> +            fprintf(stderr,
> +                "'+%s' is obsolete and will be removed in future, use '%s=on'",
> +                featurestr + 1, featurestr + 1);

Could you detect using +foo together with foo=off, and -foo together
with foo=on?  Those are the really problematic cases, without them +foo
and -foo can become synonyms for =on and =off.

Paolo

>              continue;
>          } else if (featurestr[0] == '-') {
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> @@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                  error_propagate(errp, local_err);
>                  return;
>              }
> +            fprintf(stderr,
> +                "'-%s' is obsolete and will be removed in future, use '%s=off'",
> +                featurestr + 1, featurestr + 1);
>              continue;
>          }
>  
> 

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Igor Mammedov]

On Tue, 7 Jun 2016 13:44:58 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 06/06/2016 17:16, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index f791a06..31e5e6f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1975,6 +1975,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                  error_propagate(errp, local_err);
> >                  return;
> >              }
> > +            fprintf(stderr,
> > +                "'+%s' is obsolete and will be removed in future, use '%s=on'",
> > +                featurestr + 1, featurestr + 1);  
> 
> Could you detect using +foo together with foo=off, and -foo together
> with foo=on?  Those are the really problematic cases, without them +foo
> and -foo can become synonyms for =on and =off.
That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
potentially it's possible to track foo=x locally in parser
and then compare with +-foo both ways.
But all we can do currently is to print warning about such use case.

I think Eduardo's suggestion to just warn that +-foo is obsolete for now
and drop support for it in several releases is sufficient(good) enough.

> Paolo
> 
> >              continue;
> >          } else if (featurestr[0] == '-') {
> >              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> > @@ -1982,6 +1985,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                  error_propagate(errp, local_err);
> >                  return;
> >              }
> > +            fprintf(stderr,
> > +                "'-%s' is obsolete and will be removed in future, use '%s=off'",
> > +                featurestr + 1, featurestr + 1);
> >              continue;
> >          }
> >  
> >   

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Paolo Bonzini]

On 07/06/2016 14:32, Igor Mammedov wrote:
>> > Could you detect using +foo together with foo=off, and -foo together
>> > with foo=on?  Those are the really problematic cases, without them +foo
>> > and -foo can become synonyms for =on and =off.
> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
> potentially it's possible to track foo=x locally in parser
> and then compare with +-foo both ways.
> But all we can do currently is to print warning about such use case.
> 
> I think Eduardo's suggestion to just warn that +-foo is obsolete for now
> and drop support for it in several releases is sufficient(good) enough.

kvm-unit-tests and libvirt both use it, especially because =on and =off
are relatively new I think?  It seems like it's really widespread.

Paolo

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Igor Mammedov]

On Tue, 7 Jun 2016 14:36:51 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 07/06/2016 14:32, Igor Mammedov wrote:
> >> > Could you detect using +foo together with foo=off, and -foo together
> >> > with foo=on?  Those are the really problematic cases, without them +foo
> >> > and -foo can become synonyms for =on and =off.  
> > That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
> > potentially it's possible to track foo=x locally in parser
> > and then compare with +-foo both ways.
> > But all we can do currently is to print warning about such use case.
> > 
> > I think Eduardo's suggestion to just warn that +-foo is obsolete for now
> > and drop support for it in several releases is sufficient(good) enough.  
> 
> kvm-unit-tests and libvirt both use it, especially because =on and =off
> are relatively new I think?  It seems like it's really widespread.
Yep, that's why it's not removed now.
Looks like libvirt would be able to switch to foo=x syntax,
I can take a look at kvm-unit-tests and make it use foo=x too.

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Paolo Bonzini]

On 07/06/2016 14:54, Igor Mammedov wrote:
> On Tue, 7 Jun 2016 14:36:51 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 07/06/2016 14:32, Igor Mammedov wrote:
>>>>> Could you detect using +foo together with foo=off, and -foo together
>>>>> with foo=on?  Those are the really problematic cases, without them +foo
>>>>> and -foo can become synonyms for =on and =off.  
>>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
>>> potentially it's possible to track foo=x locally in parser
>>> and then compare with +-foo both ways.
>>> But all we can do currently is to print warning about such use case.
>>>
>>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now
>>> and drop support for it in several releases is sufficient(good) enough.  
>>
>> kvm-unit-tests and libvirt both use it, especially because =on and =off
>> are relatively new I think?  It seems like it's really widespread.
> 
> Yep, that's why it's not removed now.
> Looks like libvirt would be able to switch to foo=x syntax,
> I can take a look at kvm-unit-tests and make it use foo=x too.

And all tutorials, and all scripts.  It's really too hard.

I'd really prefer to make an incompatible change straight in 2.7 for the
case of mixed foo=x and [+-]foo.

Paolo

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Igor Mammedov]

On Tue, 7 Jun 2016 15:00:04 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 07/06/2016 14:54, Igor Mammedov wrote:
> > On Tue, 7 Jun 2016 14:36:51 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> On 07/06/2016 14:32, Igor Mammedov wrote:  
> >>>>> Could you detect using +foo together with foo=off, and -foo together
> >>>>> with foo=on?  Those are the really problematic cases, without them +foo
> >>>>> and -foo can become synonyms for =on and =off.    
> >>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
> >>> potentially it's possible to track foo=x locally in parser
> >>> and then compare with +-foo both ways.
> >>> But all we can do currently is to print warning about such use case.
> >>>
> >>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now
> >>> and drop support for it in several releases is sufficient(good) enough.    
> >>
> >> kvm-unit-tests and libvirt both use it, especially because =on and =off
> >> are relatively new I think?  It seems like it's really widespread.  
> > 
> > Yep, that's why it's not removed now.
> > Looks like libvirt would be able to switch to foo=x syntax,
> > I can take a look at kvm-unit-tests and make it use foo=x too.  
> 
> And all tutorials, and all scripts.  It's really too hard.
> 
> I'd really prefer to make an incompatible change straight in 2.7 for the
> case of mixed foo=x and [+-]foo.
I've tried to make a bit more extreme incompatible change starting from 2.7
machine type in RFC (i.e. allow only foo=x syntax).
But Eduardo prefers to keep current +-foo, just marking it obsolete
so that users would have time to adapt before support for legacy +-foo
is dropped.

Anyways, we can introduce "mixed foo=x and [+-]foo" check on top of
this series with warning and probably switch to fail later so not
break existing users without giving them time to adapt.

> Paolo

Re: [Qemu-devel] [PATCH 06/10] target-i386: print obsolete warnings if +-features are used

[Eduardo Habkost]

On Tue, Jun 07, 2016 at 03:26:58PM +0200, Igor Mammedov wrote:
> On Tue, 7 Jun 2016 15:00:04 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > On 07/06/2016 14:54, Igor Mammedov wrote:
> > > On Tue, 7 Jun 2016 14:36:51 +0200
> > > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >   
> > >> On 07/06/2016 14:32, Igor Mammedov wrote:  
> > >>>>> Could you detect using +foo together with foo=off, and -foo together
> > >>>>> with foo=on?  Those are the really problematic cases, without them +foo
> > >>>>> and -foo can become synonyms for =on and =off.    
> > >>> That's (legacy)current semantic of -cpu +-foo where it overrides any foo=x,
> > >>> potentially it's possible to track foo=x locally in parser
> > >>> and then compare with +-foo both ways.
> > >>> But all we can do currently is to print warning about such use case.
> > >>>
> > >>> I think Eduardo's suggestion to just warn that +-foo is obsolete for now
> > >>> and drop support for it in several releases is sufficient(good) enough.    
> > >>
> > >> kvm-unit-tests and libvirt both use it, especially because =on and =off
> > >> are relatively new I think?  It seems like it's really widespread.  
> > > 
> > > Yep, that's why it's not removed now.
> > > Looks like libvirt would be able to switch to foo=x syntax,
> > > I can take a look at kvm-unit-tests and make it use foo=x too.  
> > 
> > And all tutorials, and all scripts.  It's really too hard.
> > 
> > I'd really prefer to make an incompatible change straight in 2.7 for the
> > case of mixed foo=x and [+-]foo.
> I've tried to make a bit more extreme incompatible change starting from 2.7
> machine type in RFC (i.e. allow only foo=x syntax).

kvm-unit-tests would be another reason to not disable it
unconditionally on pc-2.7, as it uses the default machine-type.

> But Eduardo prefers to keep current +-foo, just marking it obsolete
> so that users would have time to adapt before support for legacy +-foo
> is dropped.
> 
> Anyways, we can introduce "mixed foo=x and [+-]foo" check on top of
> this series with warning and probably switch to fail later so not
> break existing users without giving them time to adapt.

We could make it fail in the case of mixing +foo and foo=x, but
we would need to be careful to not break existing valid cases
(like "vendor=AuthenticAMD,+foo", or "+foo,pmu=on". The code that
handles "foo=x" doesn't know if the property is a feature flag or
not. I'm not sure the extra code complexity is worth it.

If we plan to to be stuck with the "[+-]foo" code forever (I am
not completely against keeping it), why not just let people mix
it with the new format? I don't see the point of making the code
more convoluted just to prevent people from mixing both.

If we don't plan to keep the [+-]foo code forever, then let's
start printing a warning as soon as possible.

(Note that I am not talking about the weird "-foo,+foo" ordering
semantics. That's something else we need to warn about and
eventually obsolete.)

-- 
Eduardo

[Qemu-devel] [PATCH 07/10] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Igor Mammedov]

make SPARC target use sparc_cpu_parse_features() directly
so it won't get in the way of switching other propertified
targets to handling features as global properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
SPARC target could be switched to features properties
later but that would need quite a bit of refactoring
in generating necessary CPU types and adding appropriate
properties.
---
 target-sparc/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 5b74cfc..e4089f2 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -101,9 +101,11 @@ static void cpu_sparc_disas_set_info(CPUState *cpu, disassemble_info *info)
 #endif
 }
 
+static void sparc_cpu_parse_features(CPUState *cs, char *features,
+                                     Error **errp);
+
 static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUSPARCState *env = &cpu->env;
     char *s = g_strdup(cpu_model);
     char *featurestr, *name = strtok(s, ",");
@@ -119,7 +121,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
     memcpy(env->def, def, sizeof(*def));
 
     featurestr = strtok(NULL, ",");
-    cc->parse_features(CPU(cpu), featurestr, &err);
+    sparc_cpu_parse_features(CPU(cpu), featurestr, &err);
     g_free(s);
     if (err) {
         error_report_err(err);
@@ -840,7 +842,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     scc->parent_reset = cc->reset;
     cc->reset = sparc_cpu_reset;
 
-    cc->parse_features = sparc_cpu_parse_features;
     cc->has_work = sparc_cpu_has_work;
     cc->do_interrupt = sparc_cpu_do_interrupt;
     cc->cpu_exec_interrupt = sparc_cpu_exec_interrupt;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/10] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:49PM +0200, Igor Mammedov wrote:
> make SPARC target use sparc_cpu_parse_features() directly
> so it won't get in the way of switching other propertified
> targets to handling features as global properties.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I would like to apply this to the x86 tree, to allow the
remaining patches to be applied. May I get an Acked-by from the
SPARC maintainers?

> ---
> SPARC target could be switched to features properties
> later but that would need quite a bit of refactoring
> in generating necessary CPU types and adding appropriate
> properties.
> ---
>  target-sparc/cpu.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
> index 5b74cfc..e4089f2 100644
> --- a/target-sparc/cpu.c
> +++ b/target-sparc/cpu.c
> @@ -101,9 +101,11 @@ static void cpu_sparc_disas_set_info(CPUState *cpu, disassemble_info *info)
>  #endif
>  }
>  
> +static void sparc_cpu_parse_features(CPUState *cs, char *features,
> +                                     Error **errp);
> +
>  static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUSPARCState *env = &cpu->env;
>      char *s = g_strdup(cpu_model);
>      char *featurestr, *name = strtok(s, ",");
> @@ -119,7 +121,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
>      memcpy(env->def, def, sizeof(*def));
>  
>      featurestr = strtok(NULL, ",");
> -    cc->parse_features(CPU(cpu), featurestr, &err);
> +    sparc_cpu_parse_features(CPU(cpu), featurestr, &err);
>      g_free(s);
>      if (err) {
>          error_report_err(err);
> @@ -840,7 +842,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>      scc->parent_reset = cc->reset;
>      cc->reset = sparc_cpu_reset;
>  
> -    cc->parse_features = sparc_cpu_parse_features;
>      cc->has_work = sparc_cpu_has_work;
>      cc->do_interrupt = sparc_cpu_do_interrupt;
>      cc->cpu_exec_interrupt = sparc_cpu_exec_interrupt;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/10] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Eduardo Habkost]

On Wed, Jun 08, 2016 at 01:30:11PM -0300, Eduardo Habkost wrote:
> On Mon, Jun 06, 2016 at 05:16:49PM +0200, Igor Mammedov wrote:
> > make SPARC target use sparc_cpu_parse_features() directly
> > so it won't get in the way of switching other propertified
> > targets to handling features as global properties.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I would like to apply this to the x86 tree, to allow the
> remaining patches to be applied. May I get an Acked-by from the
> SPARC maintainers?

I hear no objections, I will queue it on x86-next.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/10] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Mark Cave-Ayland]

On 10/06/16 12:51, Eduardo Habkost wrote:

> On Wed, Jun 08, 2016 at 01:30:11PM -0300, Eduardo Habkost wrote:
>> On Mon, Jun 06, 2016 at 05:16:49PM +0200, Igor Mammedov wrote:
>>> make SPARC target use sparc_cpu_parse_features() directly
>>> so it won't get in the way of switching other propertified
>>> targets to handling features as global properties.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>
>> I would like to apply this to the x86 tree, to allow the
>> remaining patches to be applied. May I get an Acked-by from the
>> SPARC maintainers?
> 
> I hear no objections, I will queue it on x86-next.

Given that I've never used CPU options for SPARC, this is probably okay
as long the standard sun4m/sun4u guests fire up with the same command lines.

Apologies for the delay on reviewing, my QEMU development is relegated
to time outside of work and the recent breakage on PPC/SPARC has eaten
huge amounts of my available time over the past week :/


ATB,

Mark.

[Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

Currently CPUClass->parse_features() is used to parse
-cpu features string and set properties on created CPU
instances.

But considering that features specified by -cpu apply to
every created CPU instance, it doesn't make sence to
parse the same features string for every CPU created.
It also makes every target that cares about parsing
features string explicitly call CPUClass->parse_features()
parser, which gets in a way if we consider using
generic device_add for CPU hotplug as device_add
has not a clue about CPU specific hooks.

Turns out we can use global properties mechanism to set
properties on every created CPU instance for a given
type. That way it's possible to convert CPU features
into a set of global properties for CPU type specified
by -cpu cpu_model and common Device.device_post_init()
will apply them to CPU of given type automatically
regardless whether it's manually created CPU or CPU
created with help of device_add.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
This patch only make CPUClass->parse_features()
a global properties convertor and follow up patches
will switch individual users to new behaviour
---
 hw/arm/virt.c     |  7 ++++---
 include/qom/cpu.h |  2 +-
 qom/cpu.c         | 29 +++++++++++++++++------------
 target-i386/cpu.c | 30 ++++++++++++++++++++----------
 4 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e77ed88..473e439 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState *machine)
 
     for (n = 0; n < smp_cpus; n++) {
         ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
+        const char *typename = object_class_get_name(oc);
         CPUClass *cc = CPU_CLASS(oc);
         Object *cpuobj;
         Error *err = NULL;
@@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState *machine)
             error_report("Unable to find CPU definition");
             exit(1);
         }
-        cpuobj = object_new(object_class_get_name(oc));
+        /* convert -smp CPU options specified by the user into global props */
+        cc->parse_features(typename, cpuopts, &err);
+        cpuobj = object_new(typename);
 
-        /* Handle any CPU options specified by the user */
-        cc->parse_features(CPU(cpuobj), cpuopts, &err);
         g_free(cpuopts);
         if (err) {
             error_report_err(err);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..cacb100 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -134,7 +134,7 @@ typedef struct CPUClass {
     /*< public >*/
 
     ObjectClass *(*class_by_name)(const char *cpu_model);
-    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
+    void (*parse_features)(const char *typename, char *str, Error **errp);
 
     void (*reset)(CPUState *cpu);
     int reset_dump_flags;
diff --git a/qom/cpu.c b/qom/cpu.c
index 751e992..f3e3c02 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -28,6 +28,7 @@
 #include "exec/log.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -46,7 +47,7 @@ bool cpu_exists(int64_t id)
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 {
     char *str, *name, *featurestr;
-    CPUState *cpu;
+    CPUState *cpu = NULL;
     ObjectClass *oc;
     CPUClass *cc;
     Error *err = NULL;
@@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
         return NULL;
     }
 
-    cpu = CPU(object_new(object_class_get_name(oc)));
-    cc = CPU_GET_CLASS(cpu);
-
+    cc = CPU_CLASS(oc);
     featurestr = strtok(NULL, ",");
-    cc->parse_features(cpu, featurestr, &err);
+    cc->parse_features(object_class_get_name(oc), featurestr, &err);
     g_free(str);
     if (err != NULL) {
         goto out;
     }
 
+    cpu = CPU(object_new(object_class_get_name(oc)));
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
 out:
@@ -282,25 +282,29 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
     return NULL;
 }
 
-static void cpu_common_parse_features(CPUState *cpu, char *features,
+static void cpu_common_parse_features(const char *typename, char *features,
                                       Error **errp)
 {
     char *featurestr; /* Single "key=value" string being parsed */
     char *val;
-    Error *err = NULL;
+    static bool cpu_globals_initialized;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
 
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
         val = strchr(featurestr, '=');
         if (val) {
+            GlobalProperty *prop = g_new0(typeof(*prop), 1);
             *val = 0;
             val++;
-            object_property_parse(OBJECT(cpu), val, featurestr, &err);
-            if (err) {
-                error_propagate(errp, err);
-                return;
-            }
+            prop->driver = typename;
+            prop->property = g_strdup(featurestr);
+            prop->value = g_strdup(val);
+            qdev_prop_register_global(prop);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
                        featurestr);
@@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
         }
         featurestr = strtok(NULL, ",");
     }
+    cpu_globals_initialized = true;
 }
 
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 31e5e6f..43b22e6 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features = { 0 };
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
+static void x86_cpu_parse_featurestr(const char *typename, char *features,
                                      Error **errp)
 {
-    X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
     Error *local_err = NULL;
+    static bool cpu_globals_initialized;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
 
     if (!features) {
         return;
@@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         const char *name;
         const char *val = NULL;
         char *eq = NULL;
+        GlobalProperty *prop;
 
         /* Compatibility syntax: */
         if (featurestr[0] == '+') {
@@ -2019,12 +2024,14 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
             name = "tsc-frequency";
         }
 
-        object_property_parse(OBJECT(cpu), val, name, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
+        prop = g_new0(typeof(*prop), 1);
+        prop->driver = typename;
+        prop->property = g_strdup(name);
+        prop->value = g_strdup(val);
+        qdev_prop_register_global(prop);
     }
+
+    cpu_globals_initialized = true;
 }
 
 /* Print all cpuid feature names in featureset
@@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
     ObjectClass *oc;
+    CPUClass *cc;
     gchar **model_pieces;
     char *name, *features;
     Error *error = NULL;
+    const char *typename;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
@@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
         error_setg(&error, "Unable to find CPU definition: %s", name);
         goto out;
     }
+    cc = CPU_CLASS(oc);
+    typename = object_class_get_name(oc);
 
-    cpu = X86_CPU(object_new(object_class_get_name(oc)));
-
-    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
+    cc->parse_features(typename, features, &error);
+    cpu = X86_CPU(object_new(typename));
     if (error) {
         goto out;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote:
> Currently CPUClass->parse_features() is used to parse
> -cpu features string and set properties on created CPU
> instances.
> 
> But considering that features specified by -cpu apply to
> every created CPU instance, it doesn't make sence to
> parse the same features string for every CPU created.
> It also makes every target that cares about parsing
> features string explicitly call CPUClass->parse_features()
> parser, which gets in a way if we consider using
> generic device_add for CPU hotplug as device_add
> has not a clue about CPU specific hooks.
> 
> Turns out we can use global properties mechanism to set
> properties on every created CPU instance for a given
> type. That way it's possible to convert CPU features
> into a set of global properties for CPU type specified
> by -cpu cpu_model and common Device.device_post_init()
> will apply them to CPU of given type automatically
> regardless whether it's manually created CPU or CPU
> created with help of device_add.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> This patch only make CPUClass->parse_features()
> a global properties convertor and follow up patches
> will switch individual users to new behaviour

Considering that we won't fix all callers to not call it multiple
times in the same series, can we add TODO notes to the
->parse_features() callers that are still need to be fixed?

Additional comments (and TODO notes suggestions) below:

> ---
>  hw/arm/virt.c     |  7 ++++---
>  include/qom/cpu.h |  2 +-
>  qom/cpu.c         | 29 +++++++++++++++++------------
>  target-i386/cpu.c | 30 ++++++++++++++++++++----------
>  4 files changed, 42 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index e77ed88..473e439 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState *machine)
>  
>      for (n = 0; n < smp_cpus; n++) {
>          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> +        const char *typename = object_class_get_name(oc);
>          CPUClass *cc = CPU_CLASS(oc);
>          Object *cpuobj;
>          Error *err = NULL;
> @@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState *machine)
>              error_report("Unable to find CPU definition");
>              exit(1);
>          }
> -        cpuobj = object_new(object_class_get_name(oc));
> +        /* convert -smp CPU options specified by the user into global props */
> +        cc->parse_features(typename, cpuopts, &err);

/*TODO: call cc->parse_features() only once */

> +        cpuobj = object_new(typename);
>  
> -        /* Handle any CPU options specified by the user */
> -        cc->parse_features(CPU(cpuobj), cpuopts, &err);
>          g_free(cpuopts);
>          if (err) {
>              error_report_err(err);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..cacb100 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -134,7 +134,7 @@ typedef struct CPUClass {
>      /*< public >*/
>  
>      ObjectClass *(*class_by_name)(const char *cpu_model);
> -    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
> +    void (*parse_features)(const char *typename, char *str, Error **errp);
>  
>      void (*reset)(CPUState *cpu);
>      int reset_dump_flags;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..f3e3c02 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -28,6 +28,7 @@
>  #include "exec/log.h"
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/qdev-properties.h"
>  
>  bool cpu_exists(int64_t id)
>  {
> @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id)
>  CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
>  {
>      char *str, *name, *featurestr;
> -    CPUState *cpu;
> +    CPUState *cpu = NULL;
>      ObjectClass *oc;
>      CPUClass *cc;
>      Error *err = NULL;
> @@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
>          return NULL;
>      }
>  
> -    cpu = CPU(object_new(object_class_get_name(oc)));
> -    cc = CPU_GET_CLASS(cpu);
> -
> +    cc = CPU_CLASS(oc);
>      featurestr = strtok(NULL, ",");
> -    cc->parse_features(cpu, featurestr, &err);
> +    cc->parse_features(object_class_get_name(oc), featurestr, &err);

/*TODO: all callers of cpu_generic_init() need to be converted to
 * call parse_features() only once, before calling cpu_generic_init().
 */

>      g_free(str);
>      if (err != NULL) {
>          goto out;
>      }
>  
> +    cpu = CPU(object_new(object_class_get_name(oc)));
>      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>  
>  out:
> @@ -282,25 +282,29 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
>      return NULL;
>  }
>  
> -static void cpu_common_parse_features(CPUState *cpu, char *features,
> +static void cpu_common_parse_features(const char *typename, char *features,
>                                        Error **errp)
>  {
>      char *featurestr; /* Single "key=value" string being parsed */
>      char *val;
> -    Error *err = NULL;
> +    static bool cpu_globals_initialized;
> +

/*TODO: all callers of ->parse_features() need to be changed to
 * call it only once, so we can remove this check (or change it
 * to assert(!cpu_globals_initialized).
 * Current callers of ->parse_features() are:
 * - machvirt_init()
 * - cpu_generic_init()
 * - cpu_x86_create()
 */

As far as I can see, after applying the whole series, only
cpu_x86_create() will remain.

> +    if (cpu_globals_initialized) {
> +        return;
> +    }
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
>          val = strchr(featurestr, '=');
>          if (val) {
> +            GlobalProperty *prop = g_new0(typeof(*prop), 1);
>              *val = 0;
>              val++;
> -            object_property_parse(OBJECT(cpu), val, featurestr, &err);
> -            if (err) {
> -                error_propagate(errp, err);
> -                return;
> -            }
> +            prop->driver = typename;
> +            prop->property = g_strdup(featurestr);
> +            prop->value = g_strdup(val);
> +            qdev_prop_register_global(prop);
>          } else {
>              error_setg(errp, "Expected key=value format, found %s.",
>                         featurestr);
> @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
>          }
>          featurestr = strtok(NULL, ",");
>      }
> +    cpu_globals_initialized = true;

This will register globals multiple times if called with
"foo=x,bar". Easier to just set cpu_globals_initialized=true
earlier, and report errors only on the first ->parse_features()
call?

(The same comment applies to x86_cpu_parse_featurestr() below)

>  }
>  
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 31e5e6f..43b22e6 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features = { 0 };
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> +static void x86_cpu_parse_featurestr(const char *typename, char *features,
>                                       Error **errp)
>  {
> -    X86CPU *cpu = X86_CPU(cs);
>      char *featurestr; /* Single 'key=value" string being parsed */
>      Error *local_err = NULL;
> +    static bool cpu_globals_initialized;
> +
> +    if (cpu_globals_initialized) {
> +        return;
> +    }
>  
>      if (!features) {
>          return;
> @@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>          const char *name;
>          const char *val = NULL;
>          char *eq = NULL;
> +        GlobalProperty *prop;
>  
>          /* Compatibility syntax: */
>          if (featurestr[0] == '+') {
> @@ -2019,12 +2024,14 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>              name = "tsc-frequency";
>          }
>  
> -        object_property_parse(OBJECT(cpu), val, name, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> -        }
> +        prop = g_new0(typeof(*prop), 1);
> +        prop->driver = typename;
> +        prop->property = g_strdup(name);
> +        prop->value = g_strdup(val);
> +        qdev_prop_register_global(prop);
>      }
> +
> +    cpu_globals_initialized = true;
>  }
>  
>  /* Print all cpuid feature names in featureset
> @@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu = NULL;
>      ObjectClass *oc;
> +    CPUClass *cc;
>      gchar **model_pieces;
>      char *name, *features;
>      Error *error = NULL;
> +    const char *typename;
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> @@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>          error_setg(&error, "Unable to find CPU definition: %s", name);
>          goto out;
>      }
> +    cc = CPU_CLASS(oc);
> +    typename = object_class_get_name(oc);
>  
> -    cpu = X86_CPU(object_new(object_class_get_name(oc)));
> -
> -    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
> +    cc->parse_features(typename, features, &error);

/*TODO: call cc->parse_features() only once, before calling cpu_x86_create() */

> +    cpu = X86_CPU(object_new(typename));
>      if (error) {
>          goto out;
>      }
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

On Wed, 8 Jun 2016 13:47:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote:
> > Currently CPUClass->parse_features() is used to parse
> > -cpu features string and set properties on created CPU
> > instances.
> > 
> > But considering that features specified by -cpu apply to
> > every created CPU instance, it doesn't make sence to
> > parse the same features string for every CPU created.
> > It also makes every target that cares about parsing
> > features string explicitly call CPUClass->parse_features()
> > parser, which gets in a way if we consider using
> > generic device_add for CPU hotplug as device_add
> > has not a clue about CPU specific hooks.
> > 
> > Turns out we can use global properties mechanism to set
> > properties on every created CPU instance for a given
> > type. That way it's possible to convert CPU features
> > into a set of global properties for CPU type specified
> > by -cpu cpu_model and common Device.device_post_init()
> > will apply them to CPU of given type automatically
> > regardless whether it's manually created CPU or CPU
> > created with help of device_add.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > This patch only make CPUClass->parse_features()
> > a global properties convertor and follow up patches
> > will switch individual users to new behaviour
> 
> Considering that we won't fix all callers to not call it multiple
> times in the same series, can we add TODO notes to the
> ->parse_features() callers that are still need to be fixed?
the only callers left that aren't fixed after this series are
cpu_init() callers.
The rest are taken care of by the last 2 patches.

> 
> Additional comments (and TODO notes suggestions) below:
> 
> > ---
> >  hw/arm/virt.c     |  7 ++++---
> >  include/qom/cpu.h |  2 +-
> >  qom/cpu.c         | 29 +++++++++++++++++------------
> >  target-i386/cpu.c | 30 ++++++++++++++++++++----------
> >  4 files changed, 42 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index e77ed88..473e439 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState
> > *machine) 
> >      for (n = 0; n < smp_cpus; n++) {
> >          ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU,
> > cpustr[0]);
> > +        const char *typename = object_class_get_name(oc);
> >          CPUClass *cc = CPU_CLASS(oc);
> >          Object *cpuobj;
> >          Error *err = NULL;
> > @@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState
> > *machine) error_report("Unable to find CPU definition");
> >              exit(1);
> >          }
> > -        cpuobj = object_new(object_class_get_name(oc));
> > +        /* convert -smp CPU options specified by the user into
> > global props */
> > +        cc->parse_features(typename, cpuopts, &err);
> 
> /*TODO: call cc->parse_features() only once */
I'd not add TODO here as it's done by the next patch.

> 
> > +        cpuobj = object_new(typename);
> >  
> > -        /* Handle any CPU options specified by the user */
> > -        cc->parse_features(CPU(cpuobj), cpuopts, &err);
> >          g_free(cpuopts);
> >          if (err) {
> >              error_report_err(err);
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 32f3af3..cacb100 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -134,7 +134,7 @@ typedef struct CPUClass {
> >      /*< public >*/
> >  
> >      ObjectClass *(*class_by_name)(const char *cpu_model);
> > -    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
> > +    void (*parse_features)(const char *typename, char *str, Error
> > **errp); 
> >      void (*reset)(CPUState *cpu);
> >      int reset_dump_flags;
> > diff --git a/qom/cpu.c b/qom/cpu.c
> > index 751e992..f3e3c02 100644
> > --- a/qom/cpu.c
> > +++ b/qom/cpu.c
> > @@ -28,6 +28,7 @@
> >  #include "exec/log.h"
> >  #include "qemu/error-report.h"
> >  #include "sysemu/sysemu.h"
> > +#include "hw/qdev-properties.h"
> >  
> >  bool cpu_exists(int64_t id)
> >  {
> > @@ -46,7 +47,7 @@ bool cpu_exists(int64_t id)
> >  CPUState *cpu_generic_init(const char *typename, const char
> > *cpu_model) {
> >      char *str, *name, *featurestr;
> > -    CPUState *cpu;
> > +    CPUState *cpu = NULL;
> >      ObjectClass *oc;
> >      CPUClass *cc;
> >      Error *err = NULL;
> > @@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char
> > *typename, const char *cpu_model) return NULL;
> >      }
> >  
> > -    cpu = CPU(object_new(object_class_get_name(oc)));
> > -    cc = CPU_GET_CLASS(cpu);
> > -
> > +    cc = CPU_CLASS(oc);
> >      featurestr = strtok(NULL, ",");
> > -    cc->parse_features(cpu, featurestr, &err);
> > +    cc->parse_features(object_class_get_name(oc), featurestr,
> > &err);
> 
> /*TODO: all callers of cpu_generic_init() need to be converted to
>  * call parse_features() only once, before calling cpu_generic_init().
>  */
> 
> >      g_free(str);
> >      if (err != NULL) {
> >          goto out;
> >      }
> >  
> > +    cpu = CPU(object_new(object_class_get_name(oc)));
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> >  
> >  out:
> > @@ -282,25 +282,29 @@ static ObjectClass
> > *cpu_common_class_by_name(const char *cpu_model) return NULL;
> >  }
> >  
> > -static void cpu_common_parse_features(CPUState *cpu, char
> > *features, +static void cpu_common_parse_features(const char
> > *typename, char *features, Error **errp)
> >  {
> >      char *featurestr; /* Single "key=value" string being parsed */
> >      char *val;
> > -    Error *err = NULL;
> > +    static bool cpu_globals_initialized;
> > +
> 
> /*TODO: all callers of ->parse_features() need to be changed to
>  * call it only once, so we can remove this check (or change it
>  * to assert(!cpu_globals_initialized).
>  * Current callers of ->parse_features() are:
>  * - machvirt_init()
>  * - cpu_generic_init()
>  * - cpu_x86_create()
>  */
> 
> As far as I can see, after applying the whole series, only
> cpu_x86_create() will remain.
Have you meant cpu_generic_init() ?  cpu_x86_create is removed
in the last patch.

So I'd drop cpu_x86_create() and machvirt_init() from suggested
comment.

> 
> > +    if (cpu_globals_initialized) {
> > +        return;
> > +    }
> >  
> >      featurestr = features ? strtok(features, ",") : NULL;
> >  
> >      while (featurestr) {
> >          val = strchr(featurestr, '=');
> >          if (val) {
> > +            GlobalProperty *prop = g_new0(typeof(*prop), 1);
> >              *val = 0;
> >              val++;
> > -            object_property_parse(OBJECT(cpu), val, featurestr,
> > &err);
> > -            if (err) {
> > -                error_propagate(errp, err);
> > -                return;
> > -            }
> > +            prop->driver = typename;
> > +            prop->property = g_strdup(featurestr);
> > +            prop->value = g_strdup(val);
> > +            qdev_prop_register_global(prop);
> >          } else {
> >              error_setg(errp, "Expected key=value format, found
> > %s.", featurestr);
> > @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState
> > *cpu, char *features, }
> >          featurestr = strtok(NULL, ",");
> >      }
> > +    cpu_globals_initialized = true;
> 
> This will register globals multiple times if called with
> "foo=x,bar".
I fail to see how it could happen here.

> Easier to just set cpu_globals_initialized=true
> earlier, and report errors only on the first ->parse_features()
> call?
Agreed, I'll make it like this:

    if (cpu_globals_initialized) {
        return;
    }
    cpu_globals_initialized = true;
 
> (The same comment applies to x86_cpu_parse_featurestr() below)
> 
> >  }
> >  
> >  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 31e5e6f..43b22e6 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1950,12 +1950,16 @@ static FeatureWordArray minus_features =
> > { 0 }; 
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> > -static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > +static void x86_cpu_parse_featurestr(const char *typename, char
> > *features, Error **errp)
> >  {
> > -    X86CPU *cpu = X86_CPU(cs);
> >      char *featurestr; /* Single 'key=value" string being parsed */
> >      Error *local_err = NULL;
> > +    static bool cpu_globals_initialized;
> > +
> > +    if (cpu_globals_initialized) {
> > +        return;
> > +    }
> >  
> >      if (!features) {
> >          return;
> > @@ -1967,6 +1971,7 @@ static void x86_cpu_parse_featurestr(CPUState
> > *cs, char *features, const char *name;
> >          const char *val = NULL;
> >          char *eq = NULL;
> > +        GlobalProperty *prop;
> >  
> >          /* Compatibility syntax: */
> >          if (featurestr[0] == '+') {
> > @@ -2019,12 +2024,14 @@ static void
> > x86_cpu_parse_featurestr(CPUState *cs, char *features, name =
> > "tsc-frequency"; }
> >  
> > -        object_property_parse(OBJECT(cpu), val, name, &local_err);
> > -        if (local_err) {
> > -            error_propagate(errp, local_err);
> > -            return;
> > -        }
> > +        prop = g_new0(typeof(*prop), 1);
> > +        prop->driver = typename;
> > +        prop->property = g_strdup(name);
> > +        prop->value = g_strdup(val);
> > +        qdev_prop_register_global(prop);
> >      }
> > +
> > +    cpu_globals_initialized = true;
> >  }
> >  
> >  /* Print all cpuid feature names in featureset
> > @@ -2208,9 +2215,11 @@ X86CPU *cpu_x86_create(const char
> > *cpu_model, Error **errp) {
> >      X86CPU *cpu = NULL;
> >      ObjectClass *oc;
> > +    CPUClass *cc;
> >      gchar **model_pieces;
> >      char *name, *features;
> >      Error *error = NULL;
> > +    const char *typename;
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > @@ -2225,10 +2234,11 @@ X86CPU *cpu_x86_create(const char
> > *cpu_model, Error **errp) error_setg(&error, "Unable to find CPU
> > definition: %s", name); goto out;
> >      }
> > +    cc = CPU_CLASS(oc);
> > +    typename = object_class_get_name(oc);
> >  
> > -    cpu = X86_CPU(object_new(object_class_get_name(oc)));
> > -
> > -    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
> > +    cc->parse_features(typename, features, &error);
> 
> /*TODO: call cc->parse_features() only once, before calling
> cpu_x86_create() */
ditto, I'd not add TODO here as it's done by a following patch.

> 
> > +    cpu = X86_CPU(object_new(typename));
> >      if (error) {
> >          goto out;
> >      }
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:38:49PM +0200, Igor Mammedov wrote:
> On Wed, 8 Jun 2016 13:47:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote:
> > > Currently CPUClass->parse_features() is used to parse
> > > -cpu features string and set properties on created CPU
> > > instances.
> > > 
> > > But considering that features specified by -cpu apply to
> > > every created CPU instance, it doesn't make sence to
> > > parse the same features string for every CPU created.
> > > It also makes every target that cares about parsing
> > > features string explicitly call CPUClass->parse_features()
> > > parser, which gets in a way if we consider using
> > > generic device_add for CPU hotplug as device_add
> > > has not a clue about CPU specific hooks.
> > > 
> > > Turns out we can use global properties mechanism to set
> > > properties on every created CPU instance for a given
> > > type. That way it's possible to convert CPU features
> > > into a set of global properties for CPU type specified
> > > by -cpu cpu_model and common Device.device_post_init()
> > > will apply them to CPU of given type automatically
> > > regardless whether it's manually created CPU or CPU
> > > created with help of device_add.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > This patch only make CPUClass->parse_features()
> > > a global properties convertor and follow up patches
> > > will switch individual users to new behaviour
> > 
> > Considering that we won't fix all callers to not call it multiple
> > times in the same series, can we add TODO notes to the
> > ->parse_features() callers that are still need to be fixed?
> the only callers left that aren't fixed after this series are
> cpu_init() callers.
> The rest are taken care of by the last 2 patches.

I just miss some documentation in the patch saying why exactly we
still need cpu_globals_initialized.

I like to keep the comments consistent in the intermediate steps,
as in case this patch is considered good for inclusion but the
other two need a respin for some reason. But if you want to add a
comment just for cpu_init()/cpu_generic_init(), that's OK.

> 
> > 
> > Additional comments (and TODO notes suggestions) below:
> > 
[...]
> > 
> > /*TODO: all callers of ->parse_features() need to be changed to
> >  * call it only once, so we can remove this check (or change it
> >  * to assert(!cpu_globals_initialized).
> >  * Current callers of ->parse_features() are:

I guess this needs to be changed to "current callers of
->parse_features() that may call it multiple times".

> >  * - machvirt_init()
> >  * - cpu_generic_init()
> >  * - cpu_x86_create()
> >  */
> > 
> > As far as I can see, after applying the whole series, only
> > cpu_x86_create() will remain.
> Have you meant cpu_generic_init() ?  cpu_x86_create is removed
> in the last patch.

Oops, yes, I meant cpu_generic_init().

> 
> So I'd drop cpu_x86_create() and machvirt_init() from suggested
> comment.

Works for me. Although I prefer when patches can be
reviewed/applied on their own, without depending on the patches
that come after them.

> 
> > 
> > > +    if (cpu_globals_initialized) {
> > > +        return;
> > > +    }
> > >  
> > >      featurestr = features ? strtok(features, ",") : NULL;
> > >  
> > >      while (featurestr) {
> > >          val = strchr(featurestr, '=');
> > >          if (val) {
> > > +            GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > >              *val = 0;
> > >              val++;
> > > -            object_property_parse(OBJECT(cpu), val, featurestr,
> > > &err);
> > > -            if (err) {
> > > -                error_propagate(errp, err);
> > > -                return;
> > > -            }
> > > +            prop->driver = typename;
> > > +            prop->property = g_strdup(featurestr);
> > > +            prop->value = g_strdup(val);
> > > +            qdev_prop_register_global(prop);
> > >          } else {
> > >              error_setg(errp, "Expected key=value format, found
> > > %s.", featurestr);
> > > @@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState
> > > *cpu, char *features, }
> > >          featurestr = strtok(NULL, ",");
> > >      }
> > > +    cpu_globals_initialized = true;
> > 
> > This will register globals multiple times if called with
> > "foo=x,bar".
> I fail to see how it could happen here.

I mean it will register globals multiple times if the function is
called multiple times. "foo=x" will be registered before the
error at "bar" is detected and reported.

> 
> > Easier to just set cpu_globals_initialized=true
> > earlier, and report errors only on the first ->parse_features()
> > call?
> Agreed, I'll make it like this:
> 
>     if (cpu_globals_initialized) {
>         return;
>     }
>     cpu_globals_initialized = true;

OK.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 08/10] cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

On Thu, 9 Jun 2016 10:23:57 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 09, 2016 at 02:38:49PM +0200, Igor Mammedov wrote:
> > On Wed, 8 Jun 2016 13:47:46 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Mon, Jun 06, 2016 at 05:16:50PM +0200, Igor Mammedov wrote:
> > > > Currently CPUClass->parse_features() is used to parse
> > > > -cpu features string and set properties on created CPU
> > > > instances.
> > > > 
> > > > But considering that features specified by -cpu apply to
> > > > every created CPU instance, it doesn't make sence to
> > > > parse the same features string for every CPU created.
> > > > It also makes every target that cares about parsing
> > > > features string explicitly call CPUClass->parse_features()
> > > > parser, which gets in a way if we consider using
> > > > generic device_add for CPU hotplug as device_add
> > > > has not a clue about CPU specific hooks.
> > > > 
> > > > Turns out we can use global properties mechanism to set
> > > > properties on every created CPU instance for a given
> > > > type. That way it's possible to convert CPU features
> > > > into a set of global properties for CPU type specified
> > > > by -cpu cpu_model and common Device.device_post_init()
> > > > will apply them to CPU of given type automatically
> > > > regardless whether it's manually created CPU or CPU
> > > > created with help of device_add.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > This patch only make CPUClass->parse_features()
> > > > a global properties convertor and follow up patches
> > > > will switch individual users to new behaviour
> > > 
> > > Considering that we won't fix all callers to not call it multiple
> > > times in the same series, can we add TODO notes to the
> > > ->parse_features() callers that are still need to be fixed?
> > the only callers left that aren't fixed after this series are
> > cpu_init() callers.
> > The rest are taken care of by the last 2 patches.
> 
> I just miss some documentation in the patch saying why exactly we
> still need cpu_globals_initialized.
> 
> I like to keep the comments consistent in the intermediate steps,
> as in case this patch is considered good for inclusion but the
> other two need a respin for some reason. But if you want to add a
> comment just for cpu_init()/cpu_generic_init(), that's OK.
> 
Ok, I'll post v2 for this patch as reply here as 2 following
patches look ok and don't need respining.

> > 
> > > 
> > > Additional comments (and TODO notes suggestions) below:
> > > 
> [...]
> > > 
> > > /*TODO: all callers of ->parse_features() need to be changed to
> > >  * call it only once, so we can remove this check (or change it
> > >  * to assert(!cpu_globals_initialized).
> > >  * Current callers of ->parse_features() are:
> 
> I guess this needs to be changed to "current callers of
> ->parse_features() that may call it multiple times".
> 
> > >  * - machvirt_init()
> > >  * - cpu_generic_init()
> > >  * - cpu_x86_create()
> > >  */
> > > 
> > > As far as I can see, after applying the whole series, only
> > > cpu_x86_create() will remain.
> > Have you meant cpu_generic_init() ?  cpu_x86_create is removed
> > in the last patch.
> 
> Oops, yes, I meant cpu_generic_init().
> 
> > 
> > So I'd drop cpu_x86_create() and machvirt_init() from suggested
> > comment.
> 
> Works for me. Although I prefer when patches can be
> reviewed/applied on their own, without depending on the patches
> that come after them.
> 
> > 
> > > 
> > > > +    if (cpu_globals_initialized) {
> > > > +        return;
> > > > +    }
> > > >  
> > > >      featurestr = features ? strtok(features, ",") : NULL;
> > > >  
> > > >      while (featurestr) {
> > > >          val = strchr(featurestr, '=');
> > > >          if (val) {
> > > > +            GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > >              *val = 0;
> > > >              val++;
> > > > -            object_property_parse(OBJECT(cpu), val, featurestr,
> > > > &err);
> > > > -            if (err) {
> > > > -                error_propagate(errp, err);
> > > > -                return;
> > > > -            }
> > > > +            prop->driver = typename;
> > > > +            prop->property = g_strdup(featurestr);
> > > > +            prop->value = g_strdup(val);
> > > > +            qdev_prop_register_global(prop);
> > > >          } else {
> > > >              error_setg(errp, "Expected key=value format, found
> > > > %s.", featurestr);
> > > > @@ -308,6 +312,7 @@ static void
> > > > cpu_common_parse_features(CPUState *cpu, char *features, }
> > > >          featurestr = strtok(NULL, ",");
> > > >      }
> > > > +    cpu_globals_initialized = true;
> > > 
> > > This will register globals multiple times if called with
> > > "foo=x,bar".
> > I fail to see how it could happen here.
> 
> I mean it will register globals multiple times if the function is
> called multiple times. "foo=x" will be registered before the
> error at "bar" is detected and reported.
That's true, however I haven't considered it as a caller of
parse_features() will not call it second time if error occurred.

> 
> > 
> > > Easier to just set cpu_globals_initialized=true
> > > earlier, and report errors only on the first ->parse_features()
> > > call?
> > Agreed, I'll make it like this:
> > 
> >     if (cpu_globals_initialized) {
> >         return;
> >     }
> >     cpu_globals_initialized = true;
> 
> OK.
> 

[Qemu-devel] [PATCH 09/10] arm: virt: parse cpu_model only once

[Igor Mammedov]

considering that features are converted to
global properties and global properties are
automatically applied to every new instance
of created CPU (at object_new() time), there
is no point in parsing cpu_model string every
time a CPU created.
So move parsing outside CPU creation loop and
do it only once.
Parsing also should be done before any CPU is
created so that features would affect the first
CPU a well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/arm/virt.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 473e439..0dbee47 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1112,6 +1112,10 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    ObjectClass *oc;
+    const char *typename;
+    CPUClass *cc;
+    Error *err = NULL;
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
 
     if (!cpu_model) {
@@ -1191,27 +1195,24 @@ static void machvirt_init(MachineState *machine)
 
     create_fdt(vbi);
 
-    for (n = 0; n < smp_cpus; n++) {
-        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
-        const char *typename = object_class_get_name(oc);
-        CPUClass *cc = CPU_CLASS(oc);
-        Object *cpuobj;
-        Error *err = NULL;
-        char *cpuopts = g_strdup(cpustr[1]);
-
-        if (!oc) {
-            error_report("Unable to find CPU definition");
-            exit(1);
-        }
-        /* convert -smp CPU options specified by the user into global props */
-        cc->parse_features(typename, cpuopts, &err);
-        cpuobj = object_new(typename);
+    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
+    if (!oc) {
+        error_report("Unable to find CPU definition");
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
 
-        g_free(cpuopts);
-        if (err) {
-            error_report_err(err);
-            exit(1);
-        }
+    /* convert -smp CPU options specified by the user into global props */
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, cpustr[1], &err);
+    g_strfreev(cpustr);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
+
+    for (n = 0; n < smp_cpus; n++) {
+        Object *cpuobj = object_new(typename);
 
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
@@ -1242,7 +1243,6 @@ static void machvirt_init(MachineState *machine)
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
-    g_strfreev(cpustr);
     fdt_add_timer_nodes(vbi, gic_version);
     fdt_add_cpu_nodes(vbi);
     fdt_add_psci_node(vbi);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/10] arm: virt: parse cpu_model only once

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:51PM +0200, Igor Mammedov wrote:
> considering that features are converted to
> global properties and global properties are
> automatically applied to every new instance
> of created CPU (at object_new() time), there
> is no point in parsing cpu_model string every
> time a CPU created.
> So move parsing outside CPU creation loop and
> do it only once.
> Parsing also should be done before any CPU is
> created so that features would affect the first
> CPU a well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Peter, do you prefer to get this included through my tree with
the rest of the series, or wait to included it in your tree after
the other patches get merged to master?

> ---
>  hw/arm/virt.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 473e439..0dbee47 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1112,6 +1112,10 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    ObjectClass *oc;
> +    const char *typename;
> +    CPUClass *cc;
> +    Error *err = NULL;
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>  
>      if (!cpu_model) {
> @@ -1191,27 +1195,24 @@ static void machvirt_init(MachineState *machine)
>  
>      create_fdt(vbi);
>  
> -    for (n = 0; n < smp_cpus; n++) {
> -        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> -        const char *typename = object_class_get_name(oc);
> -        CPUClass *cc = CPU_CLASS(oc);
> -        Object *cpuobj;
> -        Error *err = NULL;
> -        char *cpuopts = g_strdup(cpustr[1]);
> -
> -        if (!oc) {
> -            error_report("Unable to find CPU definition");
> -            exit(1);
> -        }
> -        /* convert -smp CPU options specified by the user into global props */
> -        cc->parse_features(typename, cpuopts, &err);
> -        cpuobj = object_new(typename);
> +    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
> +    if (!oc) {
> +        error_report("Unable to find CPU definition");
> +        exit(1);
> +    }
> +    typename = object_class_get_name(oc);
>  
> -        g_free(cpuopts);
> -        if (err) {
> -            error_report_err(err);
> -            exit(1);
> -        }
> +    /* convert -smp CPU options specified by the user into global props */
> +    cc = CPU_CLASS(oc);
> +    cc->parse_features(typename, cpustr[1], &err);
> +    g_strfreev(cpustr);
> +    if (err) {
> +        error_report_err(err);
> +        exit(1);
> +    }
> +
> +    for (n = 0; n < smp_cpus; n++) {
> +        Object *cpuobj = object_new(typename);
>  
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> @@ -1242,7 +1243,6 @@ static void machvirt_init(MachineState *machine)
>  
>          object_property_set_bool(cpuobj, true, "realized", NULL);
>      }
> -    g_strfreev(cpustr);
>      fdt_add_timer_nodes(vbi, gic_version);
>      fdt_add_cpu_nodes(vbi);
>      fdt_add_psci_node(vbi);
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 09/10] arm: virt: parse cpu_model only once

[Peter Maydell]

On 8 June 2016 at 17:55, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jun 06, 2016 at 05:16:51PM +0200, Igor Mammedov wrote:
>> considering that features are converted to
>> global properties and global properties are
>> automatically applied to every new instance
>> of created CPU (at object_new() time), there
>> is no point in parsing cpu_model string every
>> time a CPU created.
>> So move parsing outside CPU creation loop and
>> do it only once.
>> Parsing also should be done before any CPU is
>> created so that features would affect the first
>> CPU a well.
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Peter, do you prefer to get this included through my tree with
> the rest of the series, or wait to included it in your tree after
> the other patches get merged to master?

Seems easiest for you to take it.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH 10/10] pc: parse cpu features only once

[Igor Mammedov]

considering that features are converted to
global properties and global properties are
automatically applied to every new instance
of created CPU (at object_new() time), there
is no point in parsing cpu_model string every
time a CPU created.
So move parsing outside CPU creation loop and
do it only once.
Parsing also should be done before any CPU is
created so that features would affect the first
CPU a well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      | 37 ++++++++++++++++++++++++++++---------
 target-i386/cpu.c | 44 --------------------------------------------
 target-i386/cpu.h |  1 -
 3 files changed, 28 insertions(+), 54 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c48322b..0331e6d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1041,21 +1041,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
+static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
     X86CPU *cpu = NULL;
     Error *local_err = NULL;
 
-    cpu = cpu_x86_create(cpu_model, &local_err);
-    if (local_err != NULL) {
-        goto out;
-    }
+    cpu = X86_CPU(object_new(typename));
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
-out:
     if (local_err) {
         error_propagate(errp, local_err);
         object_unref(OBJECT(cpu));
@@ -1067,7 +1063,8 @@ out:
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     X86CPU *cpu;
-    MachineState *machine = MACHINE(qdev_get_machine());
+    ObjectClass *oc;
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
     Error *local_err = NULL;
 
@@ -1095,7 +1092,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
+    assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */
+    oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
+    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1106,6 +1105,10 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 void pc_cpus_init(PCMachineState *pcms)
 {
     int i, j;
+    CPUClass *cc;
+    ObjectClass *oc;
+    const char *typename;
+    gchar **model_pieces;
     X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
 
@@ -1118,6 +1121,22 @@ void pc_cpus_init(PCMachineState *pcms)
 #endif
     }
 
+    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+
     /* Calculates the limit to CPU APIC ID values
      *
      * Limit for the APIC ID value, so that all
@@ -1148,7 +1167,7 @@ void pc_cpus_init(PCMachineState *pcms)
         }
 
         if (i < smp_cpus) {
-            cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
+            cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
                              &error_fatal);
             pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 43b22e6..c633579 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2211,50 +2211,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
 }
 
-X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
-{
-    X86CPU *cpu = NULL;
-    ObjectClass *oc;
-    CPUClass *cc;
-    gchar **model_pieces;
-    char *name, *features;
-    Error *error = NULL;
-    const char *typename;
-
-    model_pieces = g_strsplit(cpu_model, ",", 2);
-    if (!model_pieces[0]) {
-        error_setg(&error, "Invalid/empty CPU model name");
-        goto out;
-    }
-    name = model_pieces[0];
-    features = model_pieces[1];
-
-    oc = x86_cpu_class_by_name(name);
-    if (oc == NULL) {
-        error_setg(&error, "Unable to find CPU definition: %s", name);
-        goto out;
-    }
-    cc = CPU_CLASS(oc);
-    typename = object_class_get_name(oc);
-
-    cc->parse_features(typename, features, &error);
-    cpu = X86_CPU(object_new(typename));
-    if (error) {
-        goto out;
-    }
-
-out:
-    if (error != NULL) {
-        error_propagate(errp, error);
-        if (cpu) {
-            object_unref(OBJECT(cpu));
-            cpu = NULL;
-        }
-    }
-    g_strfreev(model_pieces);
-    return cpu;
-}
-
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a257c53..a9a3b87 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1227,7 +1227,6 @@ void x86_cpu_exec_enter(CPUState *cpu);
 void x86_cpu_exec_exit(CPUState *cpu);
 
 X86CPU *cpu_x86_init(const char *cpu_model);
-X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 int cpu_x86_exec(CPUState *cpu);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/10] pc: parse cpu features only once

[Eduardo Habkost]

On Mon, Jun 06, 2016 at 05:16:52PM +0200, Igor Mammedov wrote:
> considering that features are converted to
> global properties and global properties are
> automatically applied to every new instance
> of created CPU (at object_new() time), there
> is no point in parsing cpu_model string every
> time a CPU created.
> So move parsing outside CPU creation loop and
> do it only once.
> Parsing also should be done before any CPU is
> created so that features would affect the first
> CPU a well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/pc.c      | 37 ++++++++++++++++++++++++++++---------
>  target-i386/cpu.c | 44 --------------------------------------------
>  target-i386/cpu.h |  1 -
>  3 files changed, 28 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index c48322b..0331e6d 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1041,21 +1041,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
>      }
>  }
>  
> -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> +static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
>                            Error **errp)
>  {
>      X86CPU *cpu = NULL;
>      Error *local_err = NULL;
>  
> -    cpu = cpu_x86_create(cpu_model, &local_err);
> -    if (local_err != NULL) {
> -        goto out;
> -    }
> +    cpu = X86_CPU(object_new(typename));

Nice. :)

>  
>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>  
> -out:
>      if (local_err) {
>          error_propagate(errp, local_err);
>          object_unref(OBJECT(cpu));
> @@ -1067,7 +1063,8 @@ out:
>  void pc_hot_add_cpu(const int64_t id, Error **errp)
>  {
>      X86CPU *cpu;
> -    MachineState *machine = MACHINE(qdev_get_machine());
> +    ObjectClass *oc;
> +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
>      int64_t apic_id = x86_cpu_apic_id_from_index(id);
>      Error *local_err = NULL;
>  
> @@ -1095,7 +1092,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>          return;
>      }
>  
> -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> +    assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */
> +    oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));

The same pattern will probably repeat in other machines. I
wouldn't mind adding a new MachineState::cpu_type field, as we
already have MachineState::cpu_model.

MachineState::cpu_model could eventually go away if we move all
parse_features() calls to generic code.

> +    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -1106,6 +1105,10 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>  void pc_cpus_init(PCMachineState *pcms)
>  {
>      int i, j;
> +    CPUClass *cc;
> +    ObjectClass *oc;
> +    const char *typename;
> +    gchar **model_pieces;
>      X86CPU *cpu = NULL;
>      MachineState *machine = MACHINE(pcms);
>  
> @@ -1118,6 +1121,22 @@ void pc_cpus_init(PCMachineState *pcms)
>  #endif
>      }
>  
> +    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
> +    if (!model_pieces[0]) {
> +        error_report("Invalid/empty CPU model name");
> +        exit(1);
> +    }
> +
> +    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
> +    if (oc == NULL) {
> +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> +        exit(1);
> +    }
> +    typename = object_class_get_name(oc);
> +    cc = CPU_CLASS(oc);
> +    cc->parse_features(typename, model_pieces[1], &error_fatal);
> +    g_strfreev(model_pieces);

Can we move this to a generic function to be reused by other
machines?

> +
>      /* Calculates the limit to CPU APIC ID values
>       *
>       * Limit for the APIC ID value, so that all
> @@ -1148,7 +1167,7 @@ void pc_cpus_init(PCMachineState *pcms)
>          }
>  
>          if (i < smp_cpus) {
> -            cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
> +            cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
>                               &error_fatal);
>              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
>              object_unref(OBJECT(cpu));
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 43b22e6..c633579 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2211,50 +2211,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  
>  }
>  
> -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> -{
> -    X86CPU *cpu = NULL;
> -    ObjectClass *oc;
> -    CPUClass *cc;
> -    gchar **model_pieces;
> -    char *name, *features;
> -    Error *error = NULL;
> -    const char *typename;
> -
> -    model_pieces = g_strsplit(cpu_model, ",", 2);
> -    if (!model_pieces[0]) {
> -        error_setg(&error, "Invalid/empty CPU model name");
> -        goto out;
> -    }
> -    name = model_pieces[0];
> -    features = model_pieces[1];
> -
> -    oc = x86_cpu_class_by_name(name);
> -    if (oc == NULL) {
> -        error_setg(&error, "Unable to find CPU definition: %s", name);
> -        goto out;
> -    }
> -    cc = CPU_CLASS(oc);
> -    typename = object_class_get_name(oc);
> -
> -    cc->parse_features(typename, features, &error);
> -    cpu = X86_CPU(object_new(typename));
> -    if (error) {
> -        goto out;
> -    }
> -
> -out:
> -    if (error != NULL) {
> -        error_propagate(errp, error);
> -        if (cpu) {
> -            object_unref(OBJECT(cpu));
> -            cpu = NULL;
> -        }
> -    }
> -    g_strfreev(model_pieces);
> -    return cpu;
> -}

Nice. :)

> -
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index a257c53..a9a3b87 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1227,7 +1227,6 @@ void x86_cpu_exec_enter(CPUState *cpu);
>  void x86_cpu_exec_exit(CPUState *cpu);
>  
>  X86CPU *cpu_x86_init(const char *cpu_model);
> -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
>  int cpu_x86_exec(CPUState *cpu);
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>  int cpu_x86_support_mca_broadcast(CPUX86State *env);
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 10/10] pc: parse cpu features only once

[Igor Mammedov]

On Wed, 8 Jun 2016 14:03:15 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 06, 2016 at 05:16:52PM +0200, Igor Mammedov wrote:
> > considering that features are converted to
> > global properties and global properties are
> > automatically applied to every new instance
> > of created CPU (at object_new() time), there
> > is no point in parsing cpu_model string every
> > time a CPU created.
> > So move parsing outside CPU creation loop and
> > do it only once.
> > Parsing also should be done before any CPU is
> > created so that features would affect the first
> > CPU a well.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/pc.c      | 37 ++++++++++++++++++++++++++++---------
> >  target-i386/cpu.c | 44 --------------------------------------------
> >  target-i386/cpu.h |  1 -
> >  3 files changed, 28 insertions(+), 54 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index c48322b..0331e6d 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1041,21 +1041,17 @@ void pc_acpi_smi_interrupt(void *opaque,
> > int irq, int level) }
> >  }
> >  
> > -static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> > +static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
> >                            Error **errp)
> >  {
> >      X86CPU *cpu = NULL;
> >      Error *local_err = NULL;
> >  
> > -    cpu = cpu_x86_create(cpu_model, &local_err);
> > -    if (local_err != NULL) {
> > -        goto out;
> > -    }
> > +    cpu = X86_CPU(object_new(typename));
> 
> Nice. :)
> 
> >  
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id",
> > &local_err); object_property_set_bool(OBJECT(cpu), true,
> > "realized", &local_err); 
> > -out:
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          object_unref(OBJECT(cpu));
> > @@ -1067,7 +1063,8 @@ out:
> >  void pc_hot_add_cpu(const int64_t id, Error **errp)
> >  {
> >      X86CPU *cpu;
> > -    MachineState *machine = MACHINE(qdev_get_machine());
> > +    ObjectClass *oc;
> > +    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> >      int64_t apic_id = x86_cpu_apic_id_from_index(id);
> >      Error *local_err = NULL;
> >  
> > @@ -1095,7 +1092,9 @@ void pc_hot_add_cpu(const int64_t id, Error
> > **errp) return;
> >      }
> >  
> > -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> > +    assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always
> > present */
> > +    oc =
> > OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
> 
> The same pattern will probably repeat in other machines. I
> wouldn't mind adding a new MachineState::cpu_type field, as we
> already have MachineState::cpu_model.
> 
> MachineState::cpu_model could eventually go away if we move all
> parse_features() calls to generic code.
All of above should be done as one step i.e.
 add cpu_type + drop cpu_model
When we are ready to call parsing from generic code but not now.

For calling parsing from generic place, the only blocker is sparc
target. 
It needs to be converted to CPU subtypes + features=>properties,
like hat you've done with x86. 

> 
> > +    cpu = pc_new_cpu(object_class_get_name(oc), apic_id,
> > &local_err); if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -1106,6 +1105,10 @@ void pc_hot_add_cpu(const int64_t id, Error
> > **errp) void pc_cpus_init(PCMachineState *pcms)
> >  {
> >      int i, j;
> > +    CPUClass *cc;
> > +    ObjectClass *oc;
> > +    const char *typename;
> > +    gchar **model_pieces;
> >      X86CPU *cpu = NULL;
> >      MachineState *machine = MACHINE(pcms);
> >  
> > @@ -1118,6 +1121,22 @@ void pc_cpus_init(PCMachineState *pcms)
> >  #endif
> >      }
> >  
> > +    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
> > +    if (!model_pieces[0]) {
> > +        error_report("Invalid/empty CPU model name");
> > +        exit(1);
> > +    }
> > +
> > +    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
> > +    if (oc == NULL) {
> > +        error_report("Unable to find CPU definition: %s",
> > model_pieces[0]);
> > +        exit(1);
> > +    }
> > +    typename = object_class_get_name(oc);
> > +    cc = CPU_CLASS(oc);
> > +    cc->parse_features(typename, model_pieces[1], &error_fatal);
> > +    g_strfreev(model_pieces);
> 
> Can we move this to a generic function to be reused by other
> machines?
It could be generalized and reduce similar site in virt-arm 
to 1 function. I'll do it on top of this series.

> > +
> >      /* Calculates the limit to CPU APIC ID values
> >       *
> >       * Limit for the APIC ID value, so that all
> > @@ -1148,7 +1167,7 @@ void pc_cpus_init(PCMachineState *pcms)
> >          }
> >  
> >          if (i < smp_cpus) {
> > -            cpu = pc_new_cpu(machine->cpu_model,
> > x86_cpu_apic_id_from_index(i),
> > +            cpu = pc_new_cpu(typename,
> > x86_cpu_apic_id_from_index(i), &error_fatal);
> >              pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
> >              object_unref(OBJECT(cpu));
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 43b22e6..c633579 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2211,50 +2211,6 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > X86CPUDefinition *def, Error **errp) 
> >  }
> >  
> > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> > -{
> > -    X86CPU *cpu = NULL;
> > -    ObjectClass *oc;
> > -    CPUClass *cc;
> > -    gchar **model_pieces;
> > -    char *name, *features;
> > -    Error *error = NULL;
> > -    const char *typename;
> > -
> > -    model_pieces = g_strsplit(cpu_model, ",", 2);
> > -    if (!model_pieces[0]) {
> > -        error_setg(&error, "Invalid/empty CPU model name");
> > -        goto out;
> > -    }
> > -    name = model_pieces[0];
> > -    features = model_pieces[1];
> > -
> > -    oc = x86_cpu_class_by_name(name);
> > -    if (oc == NULL) {
> > -        error_setg(&error, "Unable to find CPU definition: %s",
> > name);
> > -        goto out;
> > -    }
> > -    cc = CPU_CLASS(oc);
> > -    typename = object_class_get_name(oc);
> > -
> > -    cc->parse_features(typename, features, &error);
> > -    cpu = X86_CPU(object_new(typename));
> > -    if (error) {
> > -        goto out;
> > -    }
> > -
> > -out:
> > -    if (error != NULL) {
> > -        error_propagate(errp, error);
> > -        if (cpu) {
> > -            object_unref(OBJECT(cpu));
> > -            cpu = NULL;
> > -        }
> > -    }
> > -    g_strfreev(model_pieces);
> > -    return cpu;
> > -}
> 
> Nice. :)
> 
> > -
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> >      return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index a257c53..a9a3b87 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1227,7 +1227,6 @@ void x86_cpu_exec_enter(CPUState *cpu);
> >  void x86_cpu_exec_exit(CPUState *cpu);
> >  
> >  X86CPU *cpu_x86_init(const char *cpu_model);
> > -X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
> >  int cpu_x86_exec(CPUState *cpu);
> >  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> >  int cpu_x86_support_mca_broadcast(CPUX86State *env);
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH 10/10] pc: parse cpu features only once

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 02:07:35PM +0200, Igor Mammedov wrote:
> On Wed, 8 Jun 2016 14:03:15 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > -    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
> > > +    assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always
> > > present */
> > > +    oc =
> > > OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
> > 
> > The same pattern will probably repeat in other machines. I
> > wouldn't mind adding a new MachineState::cpu_type field, as we
> > already have MachineState::cpu_model.
> > 
> > MachineState::cpu_model could eventually go away if we move all
> > parse_features() calls to generic code.
> All of above should be done as one step i.e.
>  add cpu_type + drop cpu_model
> When we are ready to call parsing from generic code but not now.
> 
> For calling parsing from generic place, the only blocker is sparc
> target. 
> It needs to be converted to CPU subtypes + features=>properties,
> like hat you've done with x86. 

OK. We just need to keep that in mind and not forget to do that
later.

> 
[...]
> > > +    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
> > > +    if (!model_pieces[0]) {
> > > +        error_report("Invalid/empty CPU model name");
> > > +        exit(1);
> > > +    }
> > > +
> > > +    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
> > > +    if (oc == NULL) {
> > > +        error_report("Unable to find CPU definition: %s",
> > > model_pieces[0]);
> > > +        exit(1);
> > > +    }
> > > +    typename = object_class_get_name(oc);
> > > +    cc = CPU_CLASS(oc);
> > > +    cc->parse_features(typename, model_pieces[1], &error_fatal);
> > > +    g_strfreev(model_pieces);
> > 
> > Can we move this to a generic function to be reused by other
> > machines?
> It could be generalized and reduce similar site in virt-arm 
> to 1 function. I'll do it on top of this series.

Thanks!

-- 
Eduardo