[Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Andreas Färber]

Hello,

Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
Peter's wish for ARMCPU properties, this series sets out to align cpu_model
parsing across targets.

QemuOpts would've been nice to use, but on the one hand x86 and sparc use
QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.

The x86 implementation is closest to the proposed API, save for some laziness.
SPARC is brought in line. And as fallback for the remaining targets a new
implementation, derived from x86 but supporting only key=value format, is added.

To facilitate using this infrastructure, a generic CPU init function is created.

Only lightly tested. Available at:
git://github.com/afaerber/qemu-cpu.git qom-cpu-features.v1
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-features.v1

Regards,
Andreas

Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Anthony Liguori <anthony@codemonkey.ws>

Andreas Färber (6):
  cpu: Introduce CPUClass::parse_features() hook
  target-sparc: Use error_report() for CPU error reporting
  target-sparc: Implement CPUClass::parse_features() for SPARCCPU
  target-sparc: Defer SPARCCPU feature inference to QOM realize
  cpu: Implement CPUClass::parse_features() for the rest of CPUs
  cpu: Factor out cpu_generic_init()

 include/qom/cpu.h           | 14 +++++++
 qom/cpu.c                   | 72 +++++++++++++++++++++++++++++++++-
 target-arm/helper.c         | 14 +------
 target-cris/cpu.c           | 13 +-----
 target-i386/cpu.c           | 36 ++++++++++-------
 target-lm32/helper.c        | 13 +-----
 target-moxie/cpu.c          | 13 +-----
 target-openrisc/cpu.c       | 13 +-----
 target-ppc/translate_init.c | 21 +---------
 target-sh4/cpu.c            | 13 +-----
 target-sparc/cpu.c          | 96 +++++++++++++++++++++++++++------------------
 target-unicore32/helper.c   | 13 ++----
 12 files changed, 174 insertions(+), 157 deletions(-)

-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Andreas Färber]

Adapt the X86CPU implementation to suit the generic hook.
This involves a cleanup of error handling to cope with NULL errp.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h |  3 +++
 target-i386/cpu.c | 36 +++++++++++++++++++++---------------
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 64ebfa5..43d253a 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -67,6 +67,8 @@ struct TranslationBlock;
  * CPUClass:
  * @class_by_name: Callback to map -cpu command line model name to an
  * instantiatable CPU type.
+ * @parse_features: Callback to parse command line arguments.
+ * The argument may be modified by the callback.
  * @reset: Callback to reset the #CPUState to its initial state.
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.
@@ -96,6 +98,7 @@ typedef struct CPUClass {
     /*< public >*/
 
     ObjectClass *(*class_by_name)(const char *cpu_model);
+    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
 
     void (*reset)(CPUState *cpu);
     int reset_dump_flags;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index cbc8cd6..653840a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1626,8 +1626,10 @@ static inline void feat2prop(char *s)
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
+static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
+                                     Error **errp)
 {
+    X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
     /* Features to be added */
     FeatureWordArray plus_features = { 0 };
@@ -1635,6 +1637,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
     FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
     CPUX86State *env = &cpu->env;
+    Error *local_err = NULL;
 
     featurestr = features ? strtok(features, ",") : NULL;
 
@@ -1653,16 +1656,16 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
 
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
+                    error_setg(&local_err, "bad numerical value %s", val);
                     goto out;
                 }
                 if (numvalue < 0x80000000) {
-                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
-                            ", fixup will be removed in future versions\n");
+                    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, errp);
+                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
             } else if (!strcmp(featurestr, "tsc-freq")) {
                 int64_t tsc_freq;
                 char *err;
@@ -1671,36 +1674,38 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
                 tsc_freq = strtosz_suffix_unit(val, &err,
                                                STRTOSZ_DEFSUFFIX_B, 1000);
                 if (tsc_freq < 0 || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
+                    error_setg(&local_err, "bad numerical value %s", val);
                     goto out;
                 }
                 snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
-                object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp);
+                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);
+                    error_setg(&local_err, "bad numerical value %s", val);
                     goto out;
                 }
                 if (numvalue < min) {
-                    fprintf(stderr, "hv-spinlocks value shall always be >= 0x%x"
-                            ", fixup will be removed in future versions\n",
+                    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, errp);
+                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
             } else {
-                object_property_parse(OBJECT(cpu), val, featurestr, errp);
+                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
             }
         } else {
             feat2prop(featurestr);
-            object_property_parse(OBJECT(cpu), "on", featurestr, errp);
+            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
         }
-        if (error_is_set(errp)) {
+        if (local_err) {
+            error_propagate(errp, local_err);
             goto out;
         }
         featurestr = strtok(NULL, ",");
@@ -1926,7 +1931,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
         goto out;
     }
 
-    cpu_x86_parse_featurestr(cpu, features, &error);
+    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
     if (error) {
         goto out;
     }
@@ -2748,6 +2753,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->reset = x86_cpu_reset;
     cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
 
+    cc->parse_features = x86_cpu_parse_featurestr;
     cc->has_work = x86_cpu_has_work;
     cc->do_interrupt = x86_cpu_do_interrupt;
     cc->dump_state = x86_cpu_dump_state;
-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 2/6] target-sparc: Use error_report() for CPU error reporting

[Andreas Färber]

Replace non-debug fprintf() with error_report().

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-sparc/cpu.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index e91a2f3..ed005b2 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -18,6 +18,7 @@
  */
 
 #include "cpu.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_FEATURES
 
@@ -505,7 +506,7 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features)
             return;
         }
     }
-    fprintf(stderr, "CPU feature %s not found\n", flagname);
+    error_report("CPU feature %s not found", flagname);
 }
 
 static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
@@ -544,7 +545,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 iu_version = strtoll(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
+                    error_report("bad numerical value %s", val);
                     goto error;
                 }
                 cpu_def->iu_version = iu_version;
@@ -556,7 +557,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 fpu_version = strtol(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
+                    error_report("bad numerical value %s", val);
                     goto error;
                 }
                 cpu_def->fpu_version = fpu_version;
@@ -568,7 +569,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 mmu_version = strtol(val, &err, 0);
                 if (!*val || *err) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
+                    error_report("bad numerical value %s", val);
                     goto error;
                 }
                 cpu_def->mmu_version = mmu_version;
@@ -581,7 +582,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
                 nwindows = strtol(val, &err, 0);
                 if (!*val || *err || nwindows > MAX_NWINDOWS ||
                     nwindows < MIN_NWINDOWS) {
-                    fprintf(stderr, "bad numerical value %s\n", val);
+                    error_report("bad numerical value %s", val);
                     goto error;
                 }
                 cpu_def->nwindows = nwindows;
@@ -589,12 +590,12 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
                 fprintf(stderr, "nwindows %d\n", nwindows);
 #endif
             } else {
-                fprintf(stderr, "unrecognized feature %s\n", featurestr);
+                error_report("unrecognized feature %s", featurestr);
                 goto error;
             }
         } else {
-            fprintf(stderr, "feature string `%s' not in format "
-                    "(+feature|-feature|feature=xyz)\n", featurestr);
+            error_report("feature string `%s' not in format "
+                         "(+feature|-feature|feature=xyz)", featurestr);
             goto error;
         }
         featurestr = strtok(NULL, ",");
-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 3/6] target-sparc: Implement CPUClass::parse_features() for SPARCCPU

[Andreas Färber]

Factor cpu_model parsing out of cpu_sparc_find_by_name() by passing
cpu_sparc_find_by_name() the name portion only and calling
CPUClass::parse_features() from cpu_sparc_register() afterwards.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-sparc/cpu.c | 82 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 33 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index ed005b2..c948b49 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -70,16 +70,32 @@ static void sparc_cpu_reset(CPUState *s)
     env->cache_control = 0;
 }
 
-static int cpu_sparc_register(CPUSPARCState *env, const char *cpu_model)
+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, ",");
     sparc_def_t def1, *def = &def1;
+    Error *err = NULL;
 
-    if (cpu_sparc_find_by_name(def, cpu_model) < 0) {
+    if (cpu_sparc_find_by_name(def, name) < 0) {
+        g_free(s);
         return -1;
     }
 
     env->def = g_new0(sparc_def_t, 1);
     memcpy(env->def, def, sizeof(*def));
+
+    featurestr = strtok(NULL, ",");
+    cc->parse_features(CPU(cpu), featurestr, &err);
+    g_free(s);
+    if (err) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+        return -1;
+    }
+
 #if defined(CONFIG_USER_ONLY)
     if ((env->def->features & CPU_FEATURE_FLOAT)) {
         env->def->features |= CPU_FEATURE_FLOAT128;
@@ -104,12 +120,10 @@ static int cpu_sparc_register(CPUSPARCState *env, const char *cpu_model)
 SPARCCPU *cpu_sparc_init(const char *cpu_model)
 {
     SPARCCPU *cpu;
-    CPUSPARCState *env;
 
     cpu = SPARC_CPU(object_new(TYPE_SPARC_CPU));
-    env = &cpu->env;
 
-    if (cpu_sparc_register(env, cpu_model) < 0) {
+    if (cpu_sparc_register(cpu, cpu_model) < 0) {
         object_unref(OBJECT(cpu));
         return NULL;
     }
@@ -509,16 +523,10 @@ static void add_flagname_to_bitmaps(const char *flagname, uint32_t *features)
     error_report("CPU feature %s not found", flagname);
 }
 
-static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
+static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *name)
 {
     unsigned int i;
     const sparc_def_t *def = NULL;
-    char *s = g_strdup(cpu_model);
-    char *featurestr, *name = strtok(s, ",");
-    uint32_t plus_features = 0;
-    uint32_t minus_features = 0;
-    uint64_t iu_version;
-    uint32_t fpu_version, mmu_version, nwindows;
 
     for (i = 0; i < ARRAY_SIZE(sparc_defs); i++) {
         if (strcasecmp(name, sparc_defs[i].name) == 0) {
@@ -526,11 +534,24 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
         }
     }
     if (!def) {
-        goto error;
+        return -1;
     }
     memcpy(cpu_def, def, sizeof(*def));
+    return 0;
+}
 
-    featurestr = strtok(NULL, ",");
+static void sparc_cpu_parse_features(CPUState *cs, char *features,
+                                     Error **errp)
+{
+    SPARCCPU *cpu = SPARC_CPU(cs);
+    sparc_def_t *cpu_def = cpu->env.def;
+    char *featurestr;
+    uint32_t plus_features = 0;
+    uint32_t minus_features = 0;
+    uint64_t iu_version;
+    uint32_t fpu_version, mmu_version, nwindows;
+
+    featurestr = features ? strtok(features, ",") : NULL;
     while (featurestr) {
         char *val;
 
@@ -545,8 +566,8 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 iu_version = strtoll(val, &err, 0);
                 if (!*val || *err) {
-                    error_report("bad numerical value %s", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s", val);
+                    return;
                 }
                 cpu_def->iu_version = iu_version;
 #ifdef DEBUG_FEATURES
@@ -557,8 +578,8 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 fpu_version = strtol(val, &err, 0);
                 if (!*val || *err) {
-                    error_report("bad numerical value %s", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s", val);
+                    return;
                 }
                 cpu_def->fpu_version = fpu_version;
 #ifdef DEBUG_FEATURES
@@ -569,8 +590,8 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 
                 mmu_version = strtol(val, &err, 0);
                 if (!*val || *err) {
-                    error_report("bad numerical value %s", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s", val);
+                    return;
                 }
                 cpu_def->mmu_version = mmu_version;
 #ifdef DEBUG_FEATURES
@@ -582,21 +603,21 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
                 nwindows = strtol(val, &err, 0);
                 if (!*val || *err || nwindows > MAX_NWINDOWS ||
                     nwindows < MIN_NWINDOWS) {
-                    error_report("bad numerical value %s", val);
-                    goto error;
+                    error_setg(errp, "bad numerical value %s", val);
+                    return;
                 }
                 cpu_def->nwindows = nwindows;
 #ifdef DEBUG_FEATURES
                 fprintf(stderr, "nwindows %d\n", nwindows);
 #endif
             } else {
-                error_report("unrecognized feature %s", featurestr);
-                goto error;
+                error_setg(errp, "unrecognized feature %s", featurestr);
+                return;
             }
         } else {
-            error_report("feature string `%s' not in format "
-                         "(+feature|-feature|feature=xyz)", featurestr);
-            goto error;
+            error_setg(errp, "feature string `%s' not in format "
+                             "(+feature|-feature|feature=xyz)", featurestr);
+            return;
         }
         featurestr = strtok(NULL, ",");
     }
@@ -605,12 +626,6 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
 #ifdef DEBUG_FEATURES
     print_features(stderr, fprintf, cpu_def->features, NULL);
 #endif
-    g_free(s);
-    return 0;
-
- error:
-    g_free(s);
-    return -1;
 }
 
 void sparc_cpu_list(FILE *f, fprintf_function cpu_fprintf)
@@ -791,6 +806,7 @@ 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->dump_state = sparc_cpu_dump_state;
-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 4/6] target-sparc: Defer SPARCCPU feature inference to QOM realize

[Andreas Färber]

Gets it out of cpu_sparc_register() and aligns with target-arm.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-sparc/cpu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index c948b49..42c6de9 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -96,11 +96,6 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
         return -1;
     }
 
-#if defined(CONFIG_USER_ONLY)
-    if ((env->def->features & CPU_FEATURE_FLOAT)) {
-        env->def->features |= CPU_FEATURE_FLOAT128;
-    }
-#endif
     env->version = def->iu_version;
     env->fsr = def->fpu_version;
     env->nwindows = def->nwindows;
@@ -766,6 +761,14 @@ static bool sparc_cpu_has_work(CPUState *cs)
 static void sparc_cpu_realizefn(DeviceState *dev, Error **errp)
 {
     SPARCCPUClass *scc = SPARC_CPU_GET_CLASS(dev);
+#if defined(CONFIG_USER_ONLY)
+    SPARCCPU *cpu = SPARC_CPU(dev);
+    CPUSPARCState *env = &cpu->env;
+
+    if ((env->def->features & CPU_FEATURE_FLOAT)) {
+        env->def->features |= CPU_FEATURE_FLOAT128;
+    }
+#endif
 
     qemu_init_vcpu(CPU(dev));
 
-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 5/6] cpu: Implement CPUClass::parse_features() for the rest of CPUs

[Andreas Färber]

CPUs who do not provide their own implementation of feature parsing
will treat each option as a QOM property and set it to the supplied
value.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 qom/cpu.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/qom/cpu.c b/qom/cpu.c
index 84c51e5..70fea7b 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -1,7 +1,7 @@
 /*
  * QEMU CPU model
  *
- * Copyright (c) 2012 SUSE LINUX Products GmbH
+ * Copyright (c) 2012-2014 SUSE LINUX Products GmbH
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -218,6 +218,34 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
     return NULL;
 }
 
+static void cpu_common_parse_features(CPUState *cpu, char *features,
+                                      Error **errp)
+{
+    char *featurestr; /* Single "key=value" string being parsed */
+    char *val;
+    Error *err = NULL;
+
+    featurestr = features ? strtok(features, ",") : NULL;
+
+    while (featurestr) {
+        val = strchr(featurestr, '=');
+        if (val) {
+            *val = 0;
+            val++;
+            object_property_parse(OBJECT(cpu), val, featurestr, &err);
+            if (err) {
+                error_propagate(errp, err);
+                return;
+            }
+        } else {
+            error_setg(errp, "Expected key=value format, found %s.",
+                       featurestr);
+            return;
+        }
+        featurestr = strtok(NULL, ",");
+    }
+}
+
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
@@ -248,6 +276,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     CPUClass *k = CPU_CLASS(klass);
 
     k->class_by_name = cpu_common_class_by_name;
+    k->parse_features = cpu_common_parse_features;
     k->reset = cpu_common_reset;
     k->get_arch_id = cpu_common_get_arch_id;
     k->has_work = cpu_common_has_work;
-- 
1.8.4.5

[Qemu-devel] [PATCH qom-cpu 6/6] cpu: Factor out cpu_generic_init()

[Andreas Färber]

All targets using it gain the ability to set -cpu name,key=value,...
options via the default TYPE_CPU CPUClass::parse_features() implementation.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h           | 11 +++++++++++
 qom/cpu.c                   | 41 +++++++++++++++++++++++++++++++++++++++++
 target-arm/helper.c         | 14 +-------------
 target-cris/cpu.c           | 13 +------------
 target-lm32/helper.c        | 13 +------------
 target-moxie/cpu.c          | 13 +------------
 target-openrisc/cpu.c       | 13 +------------
 target-ppc/translate_init.c | 21 +--------------------
 target-sh4/cpu.c            | 13 +------------
 target-unicore32/helper.c   | 13 +++----------
 10 files changed, 62 insertions(+), 103 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 43d253a..29bab93 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -352,6 +352,17 @@ void cpu_reset(CPUState *cpu);
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model);
 
 /**
+ * cpu_generic_init:
+ * @typename: The CPU base type.
+ * @cpu_model: The model string including optional parameters.
+ *
+ * Instantiates a CPU, processes optional parameters and realizes the CPU.
+ *
+ * Returns: A #CPUState or %NULL if an error occurred.
+ */
+CPUState *cpu_generic_init(const char *typename, const char *cpu_model);
+
+/**
  * cpu_has_work:
  * @cpu: The vCPU to check.
  *
diff --git a/qom/cpu.c b/qom/cpu.c
index 70fea7b..4e5446a 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -23,6 +23,7 @@
 #include "sysemu/kvm.h"
 #include "qemu/notify.h"
 #include "qemu/log.h"
+#include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 
 bool cpu_exists(int64_t id)
@@ -39,6 +40,46 @@ bool cpu_exists(int64_t id)
     return false;
 }
 
+CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
+{
+    char *str, *name, *featurestr;
+    CPUState *cpu;
+    ObjectClass *oc;
+    CPUClass *cc;
+    Error *err = NULL;
+
+    str = g_strdup(cpu_model);
+    name = strtok(str, ",");
+
+    oc = cpu_class_by_name(typename, name);
+    if (oc == NULL) {
+        g_free(str);
+        return NULL;
+    }
+
+    cpu = CPU(object_new(object_class_get_name(oc)));
+    cc = CPU_GET_CLASS(cpu);
+
+    featurestr = strtok(NULL, ",");
+    cc->parse_features(cpu, featurestr, &err);
+    g_free(str);
+    if (err != NULL) {
+        goto out;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+
+out:
+    if (err != NULL) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    }
+
+    return cpu;
+}
+
 bool cpu_paging_enabled(const CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 90f85f1..c67d680 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2105,19 +2105,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
 
 ARMCPU *cpu_arm_init(const char *cpu_model)
 {
-    ARMCPU *cpu;
-    ObjectClass *oc;
-
-    oc = cpu_class_by_name(TYPE_ARM_CPU, cpu_model);
-    if (!oc) {
-        return NULL;
-    }
-    cpu = ARM_CPU(object_new(object_class_get_name(oc)));
-
-    /* TODO this should be set centrally, once possible */
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return ARM_CPU(cpu_generic_init(TYPE_ARM_CPU, cpu_model));
 }
 
 void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
diff --git a/target-cris/cpu.c b/target-cris/cpu.c
index 07da845..12c90ee 100644
--- a/target-cris/cpu.c
+++ b/target-cris/cpu.c
@@ -89,18 +89,7 @@ static ObjectClass *cris_cpu_class_by_name(const char *cpu_model)
 
 CRISCPU *cpu_cris_init(const char *cpu_model)
 {
-    CRISCPU *cpu;
-    ObjectClass *oc;
-
-    oc = cris_cpu_class_by_name(cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-    cpu = CRIS_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return CRIS_CPU(cpu_generic_init(TYPE_CRIS_CPU, cpu_model));
 }
 
 /* Sort alphabetically by VR. */
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index eecb9f6..e813e7d 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -182,18 +182,7 @@ void lm32_cpu_do_interrupt(CPUState *cs)
 
 LM32CPU *cpu_lm32_init(const char *cpu_model)
 {
-    LM32CPU *cpu;
-    ObjectClass *oc;
-
-    oc = cpu_class_by_name(TYPE_LM32_CPU, cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-    cpu = LM32_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return LM32_CPU(cpu_generic_init(TYPE_LM32_CPU, cpu_model));
 }
 
 /* Some soc ignores the MSB on the address bus. Thus creating a shadow memory
diff --git a/target-moxie/cpu.c b/target-moxie/cpu.c
index 88b0d35..32c6104 100644
--- a/target-moxie/cpu.c
+++ b/target-moxie/cpu.c
@@ -136,18 +136,7 @@ static const MoxieCPUInfo moxie_cpus[] = {
 
 MoxieCPU *cpu_moxie_init(const char *cpu_model)
 {
-    MoxieCPU *cpu;
-    ObjectClass *oc;
-
-    oc = moxie_cpu_class_by_name(cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-    cpu = MOXIE_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return MOXIE_CPU(cpu_generic_init(TYPE_MOXIE_CPU, cpu_model));
 }
 
 static void cpu_register(const MoxieCPUInfo *info)
diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c
index 83fed5e..0b2ffb2 100644
--- a/target-openrisc/cpu.c
+++ b/target-openrisc/cpu.c
@@ -208,18 +208,7 @@ static void openrisc_cpu_register_types(void)
 
 OpenRISCCPU *cpu_openrisc_init(const char *cpu_model)
 {
-    OpenRISCCPU *cpu;
-    ObjectClass *oc;
-
-    oc = openrisc_cpu_class_by_name(cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-    cpu = OPENRISC_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return OPENRISC_CPU(cpu_generic_init(TYPE_OPENRISC_CPU, cpu_model));
 }
 
 /* Sort alphabetically by type name, except for "any". */
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index f598c24..c5e29a6 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8361,26 +8361,7 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
 
 PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 {
-    PowerPCCPU *cpu;
-    ObjectClass *oc;
-    Error *err = NULL;
-
-    oc = ppc_cpu_class_by_name(cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-
-    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
-    if (err != NULL) {
-        error_report("%s", error_get_pretty(err));
-        error_free(err);
-        object_unref(OBJECT(cpu));
-        return NULL;
-    }
-
-    return cpu;
+    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
 }
 
 /* Sort by PVR, ordering special case "host" last. */
diff --git a/target-sh4/cpu.c b/target-sh4/cpu.c
index 61b82f5..8e7bcd2 100644
--- a/target-sh4/cpu.c
+++ b/target-sh4/cpu.c
@@ -148,18 +148,7 @@ static ObjectClass *superh_cpu_class_by_name(const char *cpu_model)
 
 SuperHCPU *cpu_sh4_init(const char *cpu_model)
 {
-    SuperHCPU *cpu;
-    ObjectClass *oc;
-
-    oc = superh_cpu_class_by_name(cpu_model);
-    if (oc == NULL) {
-        return NULL;
-    }
-    cpu = SUPERH_CPU(object_new(object_class_get_name(oc)));
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return cpu;
+    return SUPERH_CPU(cpu_generic_init(TYPE_SUPERH_CPU, cpu_model));
 }
 
 static void sh7750r_cpu_initfn(Object *obj)
diff --git a/target-unicore32/helper.c b/target-unicore32/helper.c
index 9bf4fea..a1f86b0 100644
--- a/target-unicore32/helper.c
+++ b/target-unicore32/helper.c
@@ -28,19 +28,12 @@
 CPUUniCore32State *uc32_cpu_init(const char *cpu_model)
 {
     UniCore32CPU *cpu;
-    CPUUniCore32State *env;
-    ObjectClass *oc;
 
-    oc = cpu_class_by_name(TYPE_UNICORE32_CPU, cpu_model);
-    if (oc == NULL) {
+    cpu = UNICORE32_CPU(cpu_generic_init(TYPE_UNICORE32_CPU, cpu_model));
+    if (cpu == NULL) {
         return NULL;
     }
-    cpu = UNICORE32_CPU(object_new(object_class_get_name(oc)));
-    env = &cpu->env;
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
-
-    return env;
+    return &cpu->env;
 }
 
 uint32_t HELPER(clo)(uint32_t x)
-- 
1.8.4.5

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Andreas Färber]

Am 04.03.2014 03:55, schrieb Andreas Färber:
> Hello,
> 
> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
> parsing across targets.
> 
> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
> 
> The x86 implementation is closest to the proposed API, save for some laziness.
> SPARC is brought in line. And as fallback for the remaining targets a new
> implementation, derived from x86 but supporting only key=value format, is added.
> 
> To facilitate using this infrastructure, a generic CPU init function is created.
> 
> Only lightly tested. Available at:
> git://github.com/afaerber/qemu-cpu.git qom-cpu-features.v1
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-features.v1
> 
> Regards,
> Andreas
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Anthony Liguori <anthony@codemonkey.ws>
> 
> Andreas Färber (6):
>   cpu: Introduce CPUClass::parse_features() hook
>   target-sparc: Use error_report() for CPU error reporting
>   target-sparc: Implement CPUClass::parse_features() for SPARCCPU
>   target-sparc: Defer SPARCCPU feature inference to QOM realize

Mark and Fabien, forgot to CC you: Could you take a look at the sparc
parts and give them some testing please?

The very latest version can be found on qom-cpu-ppc branch if necessary.

Thanks,
Andreas

>   cpu: Implement CPUClass::parse_features() for the rest of CPUs
>   cpu: Factor out cpu_generic_init()
> 
>  include/qom/cpu.h           | 14 +++++++
>  qom/cpu.c                   | 72 +++++++++++++++++++++++++++++++++-
>  target-arm/helper.c         | 14 +------
>  target-cris/cpu.c           | 13 +-----
>  target-i386/cpu.c           | 36 ++++++++++-------
>  target-lm32/helper.c        | 13 +-----
>  target-moxie/cpu.c          | 13 +-----
>  target-openrisc/cpu.c       | 13 +-----
>  target-ppc/translate_init.c | 21 +---------
>  target-sh4/cpu.c            | 13 +-----
>  target-sparc/cpu.c          | 96 +++++++++++++++++++++++++++------------------
>  target-unicore32/helper.c   | 13 ++----
>  12 files changed, 174 insertions(+), 157 deletions(-)

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

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Alexey Kardashevskiy]

On 03/04/2014 01:55 PM, Andreas Färber wrote:
> Hello,
> 
> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
> parsing across targets.
> 
> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
> 
> The x86 implementation is closest to the proposed API, save for some laziness.
> SPARC is brought in line. And as fallback for the remaining targets a new
> implementation, derived from x86 but supporting only key=value format, is added.
> 
> To facilitate using this infrastructure, a generic CPU init function is created.


Besides the fact that this patchset does not support dynamic properties
(added by object_property_add(), and I used it in my initial patchset),
that works for SPAPR, just need to implement property statically (tested).




-- 
Alexey

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Andreas Färber]

Am 05.03.2014 03:50, schrieb Alexey Kardashevskiy:
> On 03/04/2014 01:55 PM, Andreas Färber wrote:
>> Hello,
>>
>> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
>> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
>> parsing across targets.
>>
>> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
>> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
>> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
>>
>> The x86 implementation is closest to the proposed API, save for some laziness.
>> SPARC is brought in line. And as fallback for the remaining targets a new
>> implementation, derived from x86 but supporting only key=value format, is added.
>>
>> To facilitate using this infrastructure, a generic CPU init function is created.
> 
> 
> Besides the fact that this patchset does not support dynamic properties
> (added by object_property_add(), and I used it in my initial patchset),

Why would that be? I am using QOM object_property_parse() just like on
x86 where we do have a mix of static and dynamic properties. Maybe you
are using object_property_add() in the wrong place? It should be used in
the instance_init function of the CPU - be it PowerPCCPU or a derived
family/model - i.e. before cpu_ppc_init() returns. The same is necessary
to support -global.

> that works for SPAPR, just need to implement property statically (tested).

Thanks,
Andreas

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

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Alexey Kardashevskiy]

On 03/05/2014 07:30 PM, Andreas Färber wrote:
> Am 05.03.2014 03:50, schrieb Alexey Kardashevskiy:
>> On 03/04/2014 01:55 PM, Andreas Färber wrote:
>>> Hello,
>>>
>>> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
>>> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
>>> parsing across targets.
>>>
>>> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
>>> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
>>> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
>>>
>>> The x86 implementation is closest to the proposed API, save for some laziness.
>>> SPARC is brought in line. And as fallback for the remaining targets a new
>>> implementation, derived from x86 but supporting only key=value format, is added.
>>>
>>> To facilitate using this infrastructure, a generic CPU init function is created.
>>
>>
>> Besides the fact that this patchset does not support dynamic properties
>> (added by object_property_add(), and I used it in my initial patchset),
> 
> Why would that be? I am using QOM object_property_parse() just like on
> x86 where we do have a mix of static and dynamic properties. Maybe you
> are using object_property_add() in the wrong place? It should be used in
> the instance_init function of the CPU - be it PowerPCCPU or a derived
> family/model - i.e. before cpu_ppc_init() returns. The same is necessary
> to support -global.


cpu_ppc_init() calls cpu_generic_init() which does parsing before setting
"realized" to "true". The only way to add a dynamic property here is to put
object_property_add() in ppc_cpu_initfn() but it does not have CPU family
hooks (unlike realizefn). Adding "compat" for every PPC CPU is ... wrong?

Where I tried adding dynamic property before is init_proc_POWER7
(PowerPCCPUClass::init_proc) which is called from init_ppc_proc() which is
called from ppc_cpu_realizefn() and this is too late.


> 
>> that works for SPAPR, just need to implement property statically (tested).
> 
> Thanks,
> Andreas
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Igor Mammedov]

On Tue,  4 Mar 2014 03:55:44 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Adapt the X86CPU implementation to suit the generic hook.
> This involves a cleanup of error handling to cope with NULL errp.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qom/cpu.h |  3 +++
>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 64ebfa5..43d253a 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -67,6 +67,8 @@ struct TranslationBlock;
>   * CPUClass:
>   * @class_by_name: Callback to map -cpu command line model name to an
>   * instantiatable CPU type.
> + * @parse_features: Callback to parse command line arguments.
> + * The argument may be modified by the callback.
Could you specify which argument is expected to be modified?

>   * @reset: Callback to reset the #CPUState to its initial state.
>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
>   * @has_work: Callback for checking if there is work to do.
> @@ -96,6 +98,7 @@ typedef struct CPUClass {
>      /*< public >*/
>  
>      ObjectClass *(*class_by_name)(const char *cpu_model);
> +    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
>  
>      void (*reset)(CPUState *cpu);
>      int reset_dump_flags;
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index cbc8cd6..653840a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1626,8 +1626,10 @@ static inline void feat2prop(char *s)
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
> -static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
> +static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> +                                     Error **errp)
>  {
> +    X86CPU *cpu = X86_CPU(cs);
>      char *featurestr; /* Single 'key=value" string being parsed */
>      /* Features to be added */
>      FeatureWordArray plus_features = { 0 };
> @@ -1635,6 +1637,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>      FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
>      CPUX86State *env = &cpu->env;
> +    Error *local_err = NULL;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
> @@ -1653,16 +1656,16 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>  
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  if (numvalue < 0x80000000) {
> -                    fprintf(stderr, "xlevel value shall always be >= 0x80000000"
> -                            ", fixup will be removed in future versions\n");
> +                    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, errp);
> +                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
>              } else if (!strcmp(featurestr, "tsc-freq")) {
>                  int64_t tsc_freq;
>                  char *err;
> @@ -1671,36 +1674,38 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp)
>                  tsc_freq = strtosz_suffix_unit(val, &err,
>                                                 STRTOSZ_DEFSUFFIX_B, 1000);
>                  if (tsc_freq < 0 || *err) {
> -                    error_setg(errp, "bad numerical value %s", val);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> -                object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp);
> +                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);
> +                    error_setg(&local_err, "bad numerical value %s", val);
>                      goto out;
>                  }
>                  if (numvalue < min) {
> -                    fprintf(stderr, "hv-spinlocks value shall always be >= 0x%x"
> -                            ", fixup will be removed in future versions\n",
> +                    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, errp);
> +                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
>              } else {
> -                object_property_parse(OBJECT(cpu), val, featurestr, errp);
> +                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
>              }
>          } else {
>              feat2prop(featurestr);
> -            object_property_parse(OBJECT(cpu), "on", featurestr, errp);
> +            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
>          }
> -        if (error_is_set(errp)) {
> +        if (local_err) {
> +            error_propagate(errp, local_err);
>              goto out;
>          }
>          featurestr = strtok(NULL, ",");
> @@ -1926,7 +1931,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>          goto out;
>      }
>  
> -    cpu_x86_parse_featurestr(cpu, features, &error);
> +    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
>      if (error) {
>          goto out;
>      }
> @@ -2748,6 +2753,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      cc->reset = x86_cpu_reset;
>      cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
>  
> +    cc->parse_features = x86_cpu_parse_featurestr;
>      cc->has_work = x86_cpu_has_work;
>      cc->do_interrupt = x86_cpu_do_interrupt;
>      cc->dump_state = x86_cpu_dump_state;

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Andreas Färber]

Am 05.03.2014 16:04, schrieb Igor Mammedov:
> On Tue,  4 Mar 2014 03:55:44 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Adapt the X86CPU implementation to suit the generic hook.
>> This involves a cleanup of error handling to cope with NULL errp.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  include/qom/cpu.h |  3 +++
>>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
>>  2 files changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 64ebfa5..43d253a 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -67,6 +67,8 @@ struct TranslationBlock;
>>   * CPUClass:
>>   * @class_by_name: Callback to map -cpu command line model name to an
>>   * instantiatable CPU type.
>> + * @parse_features: Callback to parse command line arguments.
>> + * The argument may be modified by the callback.
> Could you specify which argument is expected to be modified?

Like so? "The arguments (%str) may be modified by the callback."

Alternatively I could drop that line, given that it's not const char *.
Or add a typedef for the callback and document it there using @str syntax.

Thanks,
Andreas

> 
>>   * @reset: Callback to reset the #CPUState to its initial state.
>>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
>>   * @has_work: Callback for checking if there is work to do.
>> @@ -96,6 +98,7 @@ typedef struct CPUClass {
>>      /*< public >*/
>>  
>>      ObjectClass *(*class_by_name)(const char *cpu_model);
>> +    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
>>  
>>      void (*reset)(CPUState *cpu);
>>      int reset_dump_flags;
[snip]

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

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Igor Mammedov]

On Wed, 05 Mar 2014 17:06:15 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 05.03.2014 16:04, schrieb Igor Mammedov:
> > On Tue,  4 Mar 2014 03:55:44 +0100
> > Andreas Färber <afaerber@suse.de> wrote:
> > 
> >> Adapt the X86CPU implementation to suit the generic hook.
> >> This involves a cleanup of error handling to cope with NULL errp.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >>  include/qom/cpu.h |  3 +++
> >>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
> >>  2 files changed, 24 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> >> index 64ebfa5..43d253a 100644
> >> --- a/include/qom/cpu.h
> >> +++ b/include/qom/cpu.h
> >> @@ -67,6 +67,8 @@ struct TranslationBlock;
> >>   * CPUClass:
> >>   * @class_by_name: Callback to map -cpu command line model name to an
> >>   * instantiatable CPU type.
> >> + * @parse_features: Callback to parse command line arguments.
> >> + * The argument may be modified by the callback.
> > Could you specify which argument is expected to be modified?
> 
> Like so? "The arguments (%str) may be modified by the callback."
> 
> Alternatively I could drop that line, given that it's not const char *.
> Or add a typedef for the callback and document it there using @str syntax.
I'd prefer to drop it.

BTW: why is 'str' modified by callback?

> Thanks,
> Andreas
> 
> > 
> >>   * @reset: Callback to reset the #CPUState to its initial state.
> >>   * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
> >>   * @has_work: Callback for checking if there is work to do.
> >> @@ -96,6 +98,7 @@ typedef struct CPUClass {
> >>      /*< public >*/
> >>  
> >>      ObjectClass *(*class_by_name)(const char *cpu_model);
> >> +    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
> >>  
> >>      void (*reset)(CPUState *cpu);
> >>      int reset_dump_flags;
> [snip]
> 

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Eduardo Habkost]

On Wed, Mar 05, 2014 at 05:57:10PM +0100, Igor Mammedov wrote:
> On Wed, 05 Mar 2014 17:06:15 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
> > Am 05.03.2014 16:04, schrieb Igor Mammedov:
> > > On Tue,  4 Mar 2014 03:55:44 +0100
> > > Andreas Färber <afaerber@suse.de> wrote:
> > > 
> > >> Adapt the X86CPU implementation to suit the generic hook.
> > >> This involves a cleanup of error handling to cope with NULL errp.
> > >>
> > >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> > >> ---
> > >>  include/qom/cpu.h |  3 +++
> > >>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
> > >>  2 files changed, 24 insertions(+), 15 deletions(-)
> > >>
> > >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > >> index 64ebfa5..43d253a 100644
> > >> --- a/include/qom/cpu.h
> > >> +++ b/include/qom/cpu.h
> > >> @@ -67,6 +67,8 @@ struct TranslationBlock;
> > >>   * CPUClass:
> > >>   * @class_by_name: Callback to map -cpu command line model name to an
> > >>   * instantiatable CPU type.
> > >> + * @parse_features: Callback to parse command line arguments.
> > >> + * The argument may be modified by the callback.
> > > Could you specify which argument is expected to be modified?
> > 
> > Like so? "The arguments (%str) may be modified by the callback."
> > 
> > Alternatively I could drop that line, given that it's not const char *.
> > Or add a typedef for the callback and document it there using @str syntax.
> I'd prefer to drop it.
> 
> BTW: why is 'str' modified by callback?

Allowing it to be modified allows (for example) strtok() to be used
(like we do on the i386 code today). So I don't see a reason to forbid
it.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Mark Cave-Ayland]

On 04/03/14 20:32, Andreas Färber wrote:

> Am 04.03.2014 03:55, schrieb Andreas Färber:
>> Hello,
>>
>> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
>> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
>> parsing across targets.
>>
>> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
>> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
>> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
>>
>> The x86 implementation is closest to the proposed API, save for some laziness.
>> SPARC is brought in line. And as fallback for the remaining targets a new
>> implementation, derived from x86 but supporting only key=value format, is added.
>>
>> To facilitate using this infrastructure, a generic CPU init function is created.
>>
>> Only lightly tested. Available at:
>> git://github.com/afaerber/qemu-cpu.git qom-cpu-features.v1
>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-features.v1
>>
>> Regards,
>> Andreas
>>
>> Cc: Alexey Kardashevskiy<aik@ozlabs.ru>
>> Cc: Peter Maydell<peter.maydell@linaro.org>
>> Cc: Anthony Liguori<anthony@codemonkey.ws>
>>
>> Andreas Färber (6):
>>    cpu: Introduce CPUClass::parse_features() hook
>>    target-sparc: Use error_report() for CPU error reporting
>>    target-sparc: Implement CPUClass::parse_features() for SPARCCPU
>>    target-sparc: Defer SPARCCPU feature inference to QOM realize
>
> Mark and Fabien, forgot to CC you: Could you take a look at the sparc
> parts and give them some testing please?
>
> The very latest version can be found on qom-cpu-ppc branch if necessary.
>
> Thanks,
> Andreas

Hi Andreas,

I've had a quick test of this branch, and while I don't tend to use CPU 
options that much, the parsing seems to work as I might expect from 
looking at the changes. I think any other snags if they exist can be 
picked up during pre-release testing so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Andreas Färber]

Am 04.03.2014 03:55, schrieb Andreas Färber:
> Adapt the X86CPU implementation to suit the generic hook.
> This involves a cleanup of error handling to cope with NULL errp.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qom/cpu.h |  3 +++
>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
>  2 files changed, 24 insertions(+), 15 deletions(-)

X86CPU subclasses require the following trivial rebase:

diff --cc target-i386/cpu.c
index 3c3a987,653840a..0000000
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@@ -1941,7 -1923,15 +1946,7 @@@ X86CPU *cpu_x86_create(const char *cpu_
      object_unref(OBJECT(cpu));
  #endif

-     cpu_x86_parse_featurestr(cpu, features, &error);
 -    /* Emulate per-model subclasses for global properties */
 -    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
 -    qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error);
 -    g_free(typename);
 -    if (error) {
 -        goto out;
 -    }
 -
+     x86_cpu_parse_featurestr(CPU(cpu), features, &error);
      if (error) {
          goto out;
      }
@@@ -2790,7 -2753,7 +2795,8 @@@ static void x86_cpu_common_class_init(O
      cc->reset = x86_cpu_reset;
      cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;

 +    cc->class_by_name = x86_cpu_class_by_name;
+     cc->parse_features = x86_cpu_parse_featurestr;
      cc->has_work = x86_cpu_has_work;
      cc->do_interrupt = x86_cpu_do_interrupt;
      cc->dump_state = x86_cpu_dump_state;

Andreas

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

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Andreas Färber]

Am 05.03.2014 23:31, schrieb Eduardo Habkost:
> On Wed, Mar 05, 2014 at 05:57:10PM +0100, Igor Mammedov wrote:
>> On Wed, 05 Mar 2014 17:06:15 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>>
>>> Am 05.03.2014 16:04, schrieb Igor Mammedov:
>>>> On Tue,  4 Mar 2014 03:55:44 +0100
>>>> Andreas Färber <afaerber@suse.de> wrote:
>>>>
>>>>> Adapt the X86CPU implementation to suit the generic hook.
>>>>> This involves a cleanup of error handling to cope with NULL errp.
>>>>>
>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>> ---
>>>>>  include/qom/cpu.h |  3 +++
>>>>>  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
>>>>>  2 files changed, 24 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>>>> index 64ebfa5..43d253a 100644
>>>>> --- a/include/qom/cpu.h
>>>>> +++ b/include/qom/cpu.h
>>>>> @@ -67,6 +67,8 @@ struct TranslationBlock;
>>>>>   * CPUClass:
>>>>>   * @class_by_name: Callback to map -cpu command line model name to an
>>>>>   * instantiatable CPU type.
>>>>> + * @parse_features: Callback to parse command line arguments.
>>>>> + * The argument may be modified by the callback.
>>>> Could you specify which argument is expected to be modified?
>>>
>>> Like so? "The arguments (%str) may be modified by the callback."
>>>
>>> Alternatively I could drop that line, given that it's not const char *.
>>> Or add a typedef for the callback and document it there using @str syntax.
>> I'd prefer to drop it.
>>
>> BTW: why is 'str' modified by callback?
> 
> Allowing it to be modified allows (for example) strtok() to be used
> (like we do on the i386 code today). So I don't see a reason to forbid
> it.

The second user apart from strtok (which NUL-terminates tokens) is
feat2prop() replacing '_' -> '-' in target-i386.

Dropping:

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 61134af..3703b68 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -69,7 +69,6 @@ struct TranslationBlock;
  * @class_by_name: Callback to map -cpu command line model name to an
  * instantiatable CPU type.
  * @parse_features: Callback to parse command line arguments.
- * The argument may be modified by the callback.
  * @reset: Callback to reset the #CPUState to its initial state.
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
  * @has_work: Callback for checking if there is work to do.

Andreas

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

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Andreas Färber]

Am 05.03.2014 12:27, schrieb Alexey Kardashevskiy:
> On 03/05/2014 07:30 PM, Andreas Färber wrote:
>> Am 05.03.2014 03:50, schrieb Alexey Kardashevskiy:
>>> On 03/04/2014 01:55 PM, Andreas Färber wrote:
>>>> Hello,
>>>>
>>>> Prompted by Alexey's desire for tweakable PowerPCCPU properties but also by
>>>> Peter's wish for ARMCPU properties, this series sets out to align cpu_model
>>>> parsing across targets.
>>>>
>>>> QemuOpts would've been nice to use, but on the one hand x86 and sparc use
>>>> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather than apply
>>>> immediately) and on the other linux-user and bsd-user don't use QemuOpts at all.
>>>>
>>>> The x86 implementation is closest to the proposed API, save for some laziness.
>>>> SPARC is brought in line. And as fallback for the remaining targets a new
>>>> implementation, derived from x86 but supporting only key=value format, is added.
>>>>
>>>> To facilitate using this infrastructure, a generic CPU init function is created.
>>>
>>>
>>> Besides the fact that this patchset does not support dynamic properties
>>> (added by object_property_add(), and I used it in my initial patchset),
>>
>> Why would that be? I am using QOM object_property_parse() just like on
>> x86 where we do have a mix of static and dynamic properties. Maybe you
>> are using object_property_add() in the wrong place? It should be used in
>> the instance_init function of the CPU - be it PowerPCCPU or a derived
>> family/model - i.e. before cpu_ppc_init() returns. The same is necessary
>> to support -global.
> 
> 
> cpu_ppc_init() calls cpu_generic_init() which does parsing before setting
> "realized" to "true". The only way to add a dynamic property here is to put
> object_property_add() in ppc_cpu_initfn() but it does not have CPU family
> hooks (unlike realizefn). Adding "compat" for every PPC CPU is ... wrong?
> 
> Where I tried adding dynamic property before is init_proc_POWER7
> (PowerPCCPUClass::init_proc) which is called from init_ppc_proc() which is
> called from ppc_cpu_realizefn() and this is too late.

I see now that the family's TypeInfo only specifies a class_init but not
an instance_init.

>>> that works for SPAPR, just need to implement property statically (tested).

It's been really long ago... could you please point me to what exactly
the property needs to be doing so that I get a feeling of whether a
static property is a solution after all or whether I need to somehow
tweak the family macro to allow for an instance_init? Like, does the
property need to be settable after realize?

Thanks,
Andreas

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

Re: [Qemu-devel] [PATCH qom-cpu 0/6] cpu: Unifying features parsing

[Andreas Färber]

Am 08.03.2014 21:50, schrieb Mark Cave-Ayland:
> On 04/03/14 20:32, Andreas Färber wrote:
> 
>> Am 04.03.2014 03:55, schrieb Andreas Färber:
>>> Hello,
>>>
>>> Prompted by Alexey's desire for tweakable PowerPCCPU properties but
>>> also by
>>> Peter's wish for ARMCPU properties, this series sets out to align
>>> cpu_model
>>> parsing across targets.
>>>
>>> QemuOpts would've been nice to use, but on the one hand x86 and sparc
>>> use
>>> QemuOpts-incompatible +foo and -foo syntax (which accumulate rather
>>> than apply
>>> immediately) and on the other linux-user and bsd-user don't use
>>> QemuOpts at all.
>>>
>>> The x86 implementation is closest to the proposed API, save for some
>>> laziness.
>>> SPARC is brought in line. And as fallback for the remaining targets a
>>> new
>>> implementation, derived from x86 but supporting only key=value
>>> format, is added.
>>>
>>> To facilitate using this infrastructure, a generic CPU init function
>>> is created.
>>>
>>> Only lightly tested. Available at:
>>> git://github.com/afaerber/qemu-cpu.git qom-cpu-features.v1
>>> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-features.v1
>>>
>>> Regards,
>>> Andreas
>>>
>>> Cc: Alexey Kardashevskiy<aik@ozlabs.ru>
>>> Cc: Peter Maydell<peter.maydell@linaro.org>
>>> Cc: Anthony Liguori<anthony@codemonkey.ws>
>>>
>>> Andreas Färber (6):
>>>    cpu: Introduce CPUClass::parse_features() hook
>>>    target-sparc: Use error_report() for CPU error reporting
>>>    target-sparc: Implement CPUClass::parse_features() for SPARCCPU
>>>    target-sparc: Defer SPARCCPU feature inference to QOM realize
>>
>> Mark and Fabien, forgot to CC you: Could you take a look at the sparc
>> parts and give them some testing please?
>>
>> The very latest version can be found on qom-cpu-ppc branch if necessary.
>>
>> Thanks,
>> Andreas
> 
> Hi Andreas,
> 
> I've had a quick test of this branch, and while I don't tend to use CPU
> options that much, the parsing seems to work as I might expect from
> looking at the changes. I think any other snags if they exist can be
> picked up during pre-release testing so:
> 
> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks a lot, proceeding to staging it on qom-cpu-next:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu-next

Andreas

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

Re: [Qemu-devel] [PATCH qom-cpu 1/6] cpu: Introduce CPUClass::parse_features() hook

[Igor Mammedov]

On Sun, 09 Mar 2014 16:45:20 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 04.03.2014 03:55, schrieb Andreas Färber:
> > Adapt the X86CPU implementation to suit the generic hook.
> > This involves a cleanup of error handling to cope with NULL errp.
> > 
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > ---
> >  include/qom/cpu.h |  3 +++
> >  target-i386/cpu.c | 36 +++++++++++++++++++++---------------
> >  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> X86CPU subclasses require the following trivial rebase:
> 
> diff --cc target-i386/cpu.c
> index 3c3a987,653840a..0000000
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@@ -1941,7 -1923,15 +1946,7 @@@ X86CPU *cpu_x86_create(const char *cpu_
>       object_unref(OBJECT(cpu));
>   #endif
> 
> -     cpu_x86_parse_featurestr(cpu, features, &error);
>  -    /* Emulate per-model subclasses for global properties */
>  -    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
>  -    qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error);
>  -    g_free(typename);
>  -    if (error) {
>  -        goto out;
>  -    }
>  -
> +     x86_cpu_parse_featurestr(CPU(cpu), features, &error);
>       if (error) {
>           goto out;
>       }
> @@@ -2790,7 -2753,7 +2795,8 @@@ static void x86_cpu_common_class_init(O
>       cc->reset = x86_cpu_reset;
>       cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
> 
>  +    cc->class_by_name = x86_cpu_class_by_name;
> +     cc->parse_features = x86_cpu_parse_featurestr;
>       cc->has_work = x86_cpu_has_work;
>       cc->do_interrupt = x86_cpu_do_interrupt;
>       cc->dump_state = x86_cpu_dump_state;
> 
> Andreas
> 

looks good,

Reviewed-by: Igor Mammedov <imammedo@redhat.com>