[Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

Changelog:
  since v1:
    - rebase on top of x86-next tree, which shaves off already applied several patches
      and fixed deps on wrong tree (v1 was against my device_add_cpu tree)
    - add TODO comments
    - set cpu_globals_initialized = true early
    - move error_propagate() to the end of x86_cpu_parse_featurestr()
  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_V2 
web view:
  https://github.com/imammedo/qemu/commits/cpu_parse_into_global_props_V2


Eduardo Habkost (1):
  target-i386: cpu: consolidate calls of object_property_parse() in
    x86_cpu_parse_featurestr

Igor Mammedov (5):
  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          |  38 ++++++++++-----
 target-i386/cpu.c  | 135 +++++++++++++++++++++++++----------------------------
 target-i386/cpu.h  |   1 -
 target-sparc/cpu.c |   7 +--
 7 files changed, 143 insertions(+), 118 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH v2 1/6] 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>
Reviewed-by: Eduardo Habkost <ehabkost@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"
v2:
 - move error_propagate() out of loop.
       Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 74 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e6d342..947cf18 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1965,43 +1965,59 @@ 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;
+    }
+
+    for (featurestr = strtok(features, ",");
+         featurestr  && !local_err;
+         featurestr = strtok(NULL, ",")) {
+        const char *name;
+        const char *val = NULL;
+        char *eq = NULL;
 
-    while (featurestr) {
-        char *val;
+        /* Compatibility syntax: */
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
+            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);
-            }
+            continue;
+        }
+
+        eq = strchr(featurestr, '=');
+        if (eq) {
+            *eq++ = 0;
+            val = eq;
         } else {
-            feat2prop(featurestr);
-            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
+            val = "on";
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+
+        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";
         }
-        featurestr = strtok(NULL, ",");
+
+        object_property_parse(OBJECT(cpu), val, name, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
     }
 }
 
-- 
2.7.0

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

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
[...]
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
>      }

error_propagate() already ignores local_err==NULL so you don't
need to check it first.

I can change this while applying the patch, if you're OK.

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

-- 
Eduardo

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

[Eric Blake]

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

On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> [...]
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>      }
> 
> error_propagate() already ignores local_err==NULL so you don't
> need to check it first.

In fact, if the ONLY reason you are doing an 'if (local_err)'
conditional is to decide if an error was set, then you don't care about
the error locally, and could have passed errp instead of &local_err in
the first place.

-- 
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 v2 1/6] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 01:39:34PM -0600, Eric Blake wrote:
> On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> > [...]
> >> +
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >>      }
> > 
> > error_propagate() already ignores local_err==NULL so you don't
> > need to check it first.
> 
> In fact, if the ONLY reason you are doing an 'if (local_err)'
> conditional is to decide if an error was set, then you don't care about
> the error locally, and could have passed errp instead of &local_err in
> the first place.

In this case, we stop the parsing loop if an error is detected.

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 9 Jun 2016 13:39:34 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> > [...]  
> >> +
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >>      }  
> > 
> > error_propagate() already ignores local_err==NULL so you don't
> > need to check it first.  
> 
> In fact, if the ONLY reason you are doing an 'if (local_err)'
> conditional is to decide if an error was set, then you don't care about
> the error locally, and could have passed errp instead of &local_err in
> the first place.
errp might be NULL, so we won't do parsing at all.
considering it's more or less generic API hook it's safer
to follow error handling pattern used in properties
i.e. use local_error internally and then propagate error
if caller cares about it.

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

[Igor Mammedov]

On Thu, 9 Jun 2016 14:29:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> [...]
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >      }  
> 
> error_propagate() already ignores local_err==NULL so you don't
> need to check it first.
> 
> I can change this while applying the patch, if you're OK.
sounds good to me.

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

[Qemu-devel] [PATCH v2 2/6] 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 947cf18..3f886a5 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1979,9 +1979,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         /* Compatibility syntax: */
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
+            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);
+            fprintf(stderr,
+                "'-%s' is obsolete and will be removed in future, use '%s=off'",
+                featurestr + 1, featurestr + 1);
             continue;
         }
 
-- 
2.7.0

Re: [Qemu-devel] [PATCH v2 2/6] target-i386: print obsolete warnings if +-features are used

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 07:10:59PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Applied to x86-next after changing it to use error_report().

> ---
>  target-i386/cpu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 947cf18..3f886a5 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1979,9 +1979,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>          /* Compatibility syntax: */
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> +            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);
> +            fprintf(stderr,
> +                "'-%s' is obsolete and will be removed in future, use '%s=off'",
> +                featurestr + 1, featurestr + 1);
>              continue;
>          }
>  
> -- 
> 2.7.0
> 

-- 
Eduardo

[Qemu-devel] [PATCH v2 3/6] 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;
-- 
2.7.0

Re: [Qemu-devel] [PATCH v2 3/6] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 07:11:00PM +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>

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

Applied to x86-next.

-- 
Eduardo

[Qemu-devel] [PATCH v2 4/6] 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

v2:
  - add todo comments in cpu_generic_init/cpu_common_parse_features
  - set cpu_globals_initialized = true early
---
 hw/arm/virt.c     |  7 ++++---
 include/qom/cpu.h |  2 +-
 qom/cpu.c         | 40 ++++++++++++++++++++++++++++------------
 target-i386/cpu.c | 25 +++++++++++++++++++------
 4 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 73113cf..3519c9f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1198,6 +1198,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;
@@ -1207,10 +1208,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..eee23db 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,18 @@ 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);
+    /* TODO: all callers of cpu_generic_init() need to be converted to
+     * call parse_features() only once, before calling cpu_generic_init().
+     */
+    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 +285,38 @@ 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()
+     */
+    if (cpu_globals_initialized) {
+        return;
+    }
+    cpu_globals_initialized = true;
 
     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);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3f886a5..279f656 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1958,12 +1958,17 @@ 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;
+    }
+    cpu_globals_initialized = true;
 
     if (!features) {
         return;
@@ -1975,6 +1980,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,7 +2025,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
             name = "tsc-frequency";
         }
 
-        object_property_parse(OBJECT(cpu), val, name, &local_err);
+        prop = g_new0(typeof(*prop), 1);
+        prop->driver = typename;
+        prop->property = g_strdup(name);
+        prop->value = g_strdup(val);
+        qdev_prop_register_global(prop);
     }
 
     if (local_err) {
@@ -2208,9 +2218,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 +2237,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;
     }
-- 
2.7.0

[Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/6] cpu: use CPUClass->parse_features() as convertor to global properties)

[Eduardo Habkost]

On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote:
[...]
> -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()
> +     */
> +    if (cpu_globals_initialized) {
> +        return;
> +    }
> +    cpu_globals_initialized = true;
>  
>      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);

This allows the user to trigger an assert:

  $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
  qemu-system-x86_64: hw/core/qdev-properties.c:1087: qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
  Aborted (core dumped)

but even if we fix the assert by setting
prop->user_provided=true, we have a problem. Previous behavior
was:

  $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
  qemu-system-x86_64: Property '.INVALID' not found
  $ 

after this patch, and setting prop->user_provided=true, we have:

  $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
  qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored: Property '.INVALID' not found
  [QEMU keeps running]

QEMU needs to refuse to run if an invalid property is specified
on -cpu. It is an important mechanism to prevent VMs from running
if the user is requesting for a unsupported feature that requires
newer QEMU.

Any suggestions on how to fix that?

Maybe qdev_prop_set_globals() can collect errors in a list in
DeviceState, and we can check for them in code that creates
device objects (like cpu_generic_init(), qdev_device_add()), or
in the beginning of device_set_realized().

-- 
Eduardo

Re: [Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/6] cpu: use CPUClass->parse_features() as convertor to global properties)

[Paolo Bonzini]

----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: qemu-devel@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, "mark cave-ayland"
> <mark.cave-ayland@ilande.co.uk>, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net,
> "Markus Armbruster" <armbru@redhat.com>
> Sent: Tuesday, June 14, 2016 9:47:18 PM
> Subject: Handling errors caused by -global (was Re: [Qemu-devel] [PATCH v2 4/6] cpu: use CPUClass->parse_features()
> as convertor to global properties)
> 
> On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote:
> [...]
> > -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()
> > +     */
> > +    if (cpu_globals_initialized) {
> > +        return;
> > +    }
> > +    cpu_globals_initialized = true;
> >  
> >      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);
> 
> This allows the user to trigger an assert:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
>   qemu-system-x86_64: hw/core/qdev-properties.c:1087:
>   qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
>   Aborted (core dumped)
> 
> but even if we fix the assert by setting
> prop->user_provided=true, we have a problem. Previous behavior
> was:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
>   qemu-system-x86_64: Property '.INVALID' not found
>   $
> 
> after this patch, and setting prop->user_provided=true, we have:
> 
>   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
>   qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored:
>   Property '.INVALID' not found
>   [QEMU keeps running]
> 
> QEMU needs to refuse to run if an invalid property is specified
> on -cpu. It is an important mechanism to prevent VMs from running
> if the user is requesting for a unsupported feature that requires
> newer QEMU.
> 
> Any suggestions on how to fix that?

Replace user_provided with a Error* argument to qdev_prop_register_global.
It can be &error_abort and NULL for the current users of the function,
while -cpu can use &error_fatal.

Paolo

> Maybe qdev_prop_set_globals() can collect errors in a list in
> DeviceState, and we can check for them in code that creates
> device objects (like cpu_generic_init(), qdev_device_add()), or
> in the beginning of device_set_realized().
> 
> --
> Eduardo
> 

Re: [Qemu-devel] Handling errors caused by -global (was Re: [PATCH v2 4/6] cpu: use CPUClass->parse_features() as convertor to global properties)

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 05:41:03PM -0400, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Eduardo Habkost" <ehabkost@redhat.com>
> > To: "Igor Mammedov" <imammedo@redhat.com>
> > Cc: qemu-devel@nongnu.org, "peter maydell" <peter.maydell@linaro.org>, "mark cave-ayland"
> > <mark.cave-ayland@ilande.co.uk>, blauwirbel@gmail.com, qemu-arm@nongnu.org, pbonzini@redhat.com, rth@twiddle.net,
> > "Markus Armbruster" <armbru@redhat.com>
> > Sent: Tuesday, June 14, 2016 9:47:18 PM
> > Subject: Handling errors caused by -global (was Re: [Qemu-devel] [PATCH v2 4/6] cpu: use CPUClass->parse_features()
> > as convertor to global properties)
> > 
> > On Thu, Jun 09, 2016 at 07:11:01PM +0200, Igor Mammedov wrote:
> > [...]
> > > -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()
> > > +     */
> > > +    if (cpu_globals_initialized) {
> > > +        return;
> > > +    }
> > > +    cpu_globals_initialized = true;
> > >  
> > >      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);
> > 
> > This allows the user to trigger an assert:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
> >   qemu-system-x86_64: hw/core/qdev-properties.c:1087:
> >   qdev_prop_set_globals_for_type: Assertion `prop->user_provided' failed.
> >   Aborted (core dumped)
> > 
> > but even if we fix the assert by setting
> > prop->user_provided=true, we have a problem. Previous behavior
> > was:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
> >   qemu-system-x86_64: Property '.INVALID' not found
> >   $
> > 
> > after this patch, and setting prop->user_provided=true, we have:
> > 
> >   $ ./x86_64-softmmu/qemu-system-x86_64 -cpu qemu64,INVALID=on
> >   qemu-system-x86_64: Warning: global qemu64-x86_64-cpu.INVALID=on ignored:
> >   Property '.INVALID' not found
> >   [QEMU keeps running]
> > 
> > QEMU needs to refuse to run if an invalid property is specified
> > on -cpu. It is an important mechanism to prevent VMs from running
> > if the user is requesting for a unsupported feature that requires
> > newer QEMU.
> > 
> > Any suggestions on how to fix that?
> 
> Replace user_provided with a Error* argument to qdev_prop_register_global.
> It can be &error_abort and NULL for the current users of the function,
> while -cpu can use &error_fatal.

Brilliant. This should work.

-- 
Eduardo

[Qemu-devel] [PATCH v2 4/6] fixup! cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

make sure that -cpu features converted to global properties
will not be ignored and QEMU exits with error as it used to
in case feature couldn't be applied.

fixup is on top of:
 [PATCH v2 00/10] globals: Clean up validation and error checking

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

diff --git a/qom/cpu.c b/qom/cpu.c
index b34c4a0..a9727a1 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -314,6 +314,7 @@ static void cpu_common_parse_features(const char *typename, char *features,
             prop->driver = typename;
             prop->property = g_strdup(featurestr);
             prop->value = g_strdup(val);
+            prop->errp = &error_fatal;
             qdev_prop_register_global(prop);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 212d552..0f0a3f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2024,6 +2024,7 @@ static void x86_cpu_parse_featurestr(const char *typename, char *features,
         prop->driver = typename;
         prop->property = g_strdup(name);
         prop->value = g_strdup(val);
+        prop->errp = &error_fatal;
         qdev_prop_register_global(prop);
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 4/6] fixup! cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Tue, Jun 21, 2016 at 03:24:17PM +0200, Igor Mammedov wrote:
> make sure that -cpu features converted to global properties
> will not be ignored and QEMU exits with error as it used to
> in case feature couldn't be applied.
> 
> fixup is on top of:
>  [PATCH v2 00/10] globals: Clean up validation and error checking
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

To original patch + fixup:

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

-- 
Eduardo

[Qemu-devel] [PATCH v2 5/6] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 42 +++++++++++++++++++++---------------------
 qom/cpu.c     |  1 -
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3519c9f..790680f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1113,6 +1113,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) {
@@ -1196,27 +1200,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);
@@ -1247,7 +1248,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);
diff --git a/qom/cpu.c b/qom/cpu.c
index eee23db..1295f5b 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -296,7 +296,6 @@ static void cpu_common_parse_features(const char *typename, char *features,
      * 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()
      */
-- 
2.7.0

[Qemu-devel] [PATCH v2 6/6] 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 ++++++++++++++++++++++++++++---------
 qom/cpu.c         |  1 -
 target-i386/cpu.c | 44 --------------------------------------------
 target-i386/cpu.h |  1 -
 4 files changed, 28 insertions(+), 55 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7198ed5..a3f94b1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1039,21 +1039,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));
@@ -1065,7 +1061,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;
 
@@ -1093,7 +1090,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;
@@ -1104,6 +1103,10 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 void pc_cpus_init(PCMachineState *pcms)
 {
     int i;
+    CPUClass *cc;
+    ObjectClass *oc;
+    const char *typename;
+    gchar **model_pieces;
     X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
 
@@ -1116,6 +1119,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
@@ -1136,7 +1155,7 @@ void pc_cpus_init(PCMachineState *pcms)
         pcms->possible_cpus->cpus[i].arch_id = x86_cpu_apic_id_from_index(i);
         pcms->possible_cpus->len++;
         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/qom/cpu.c b/qom/cpu.c
index 1295f5b..b34c4a0 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -297,7 +297,6 @@ static void cpu_common_parse_features(const char *typename, char *features,
      * to assert(!cpu_globals_initialized).
      * Current callers of ->parse_features() are:
      * - cpu_generic_init()
-     * - cpu_x86_create()
      */
     if (cpu_globals_initialized) {
         return;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 279f656..4d36623 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2214,50 +2214,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 d9ab884..f4f65ce 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1234,7 +1234,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);
-- 
2.7.0

Re: [Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

On Thu,  9 Jun 2016 19:10:57 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

Eduardo,

is anything more left to review/fix so remaining patches of this series
could be merged?

There is PPC series[1] that would introduce a conflict (using old style
feature parsing code), it would be better if this series are merged
and so that [1] could be rebased on top of it if there aren't any issues
left.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg383073.html

> Changelog:
>   since v1:
>     - rebase on top of x86-next tree, which shaves off already
> applied several patches and fixed deps on wrong tree (v1 was against
> my device_add_cpu tree)
>     - add TODO comments
>     - set cpu_globals_initialized = true early
>     - move error_propagate() to the end of x86_cpu_parse_featurestr()
>   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_V2 
> web view:
>   https://github.com/imammedo/qemu/commits/cpu_parse_into_global_props_V2
> 
> 
> Eduardo Habkost (1):
>   target-i386: cpu: consolidate calls of object_property_parse() in
>     x86_cpu_parse_featurestr
> 
> Igor Mammedov (5):
>   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          |  38 ++++++++++-----
>  target-i386/cpu.c  | 135
> +++++++++++++++++++++++++----------------------------
> target-i386/cpu.h  |   1 - target-sparc/cpu.c |   7 +--
>  7 files changed, 143 insertions(+), 118 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Mon, Jul 04, 2016 at 10:04:31AM +0200, Igor Mammedov wrote:
> On Thu,  9 Jun 2016 19:10:57 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> Eduardo,
> 
> is anything more left to review/fix so remaining patches of this series
> could be merged?

The only thing left is that I run some basic testing and submit a
pull request. I couldn't do that last week (sorry!), but I plan
to submit a pull request this week.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

On Mon, 4 Jul 2016 11:01:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jul 04, 2016 at 10:04:31AM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jun 2016 19:10:57 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > Eduardo,
> > 
> > is anything more left to review/fix so remaining patches of this
> > series could be merged?
> 
> The only thing left is that I run some basic testing and submit a
> pull request. I couldn't do that last week (sorry!), but I plan
> to submit a pull request this week.
Is there a tree for Greg to rebase his work upon? 

Re: [Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Mon, Jul 04, 2016 at 04:32:19PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jul 2016 11:01:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jul 04, 2016 at 10:04:31AM +0200, Igor Mammedov wrote:
> > > On Thu,  9 Jun 2016 19:10:57 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > > Eduardo,
> > > 
> > > is anything more left to review/fix so remaining patches of this
> > > series could be merged?
> > 
> > The only thing left is that I run some basic testing and submit a
> > pull request. I couldn't do that last week (sorry!), but I plan
> > to submit a pull request this week.
> Is there a tree for Greg to rebase his work upon? 

git://github.com/ehabkost/qemu.git x86-next

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 0/6] cpus: make "-cpu cpux, features" global properties

[Greg Kurz]

On Mon, 4 Jul 2016 13:21:21 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jul 04, 2016 at 04:32:19PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jul 2016 11:01:46 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Mon, Jul 04, 2016 at 10:04:31AM +0200, Igor Mammedov wrote:  
> > > > On Thu,  9 Jun 2016 19:10:57 +0200
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > > 
> > > > Eduardo,
> > > > 
> > > > is anything more left to review/fix so remaining patches of this
> > > > series could be merged?  
> > > 
> > > The only thing left is that I run some basic testing and submit a
> > > pull request. I couldn't do that last week (sorry!), but I plan
> > > to submit a pull request this week.  
> > Is there a tree for Greg to rebase his work upon?   
> 
> git://github.com/ehabkost/qemu.git x86-next
> 

Thanks Eduardo !

--
Greg