[Qemu-devel] [PATCH 0/6] target-i386: Feature properties, sample script for -global/-readconfig

[Qemu-devel] [PATCH 0/6] target-i386: Feature properties, sample script for -global/-readconfig

[Eduardo Habkost]

This series adds feature flag properties to X86CPU, and adds a sample script to
demonstrate how the new properties can be used to give management software more
flexibility to control CPU features using -global and -readconfig.

While at it, simplify the code for "level" and "xlevel" properties, and add a
"xlevel2" property, that was still missing.

This series doesn't change x86_cpu_parse_featurestr() to use the new properties
yet, but that will be possible in the future (we just need to be careful about
the ordering requirements of the current host_features code).

Eduardo Habkost (6):
  target-i386: Move error handling to end of x86_cpu_parse_featurestr()
  target-i386: Remove underscores from feature names
  target-i386: Register QOM properties for feature flags
  target-i386: Make "level" and "xlevel" properties static
  target-i386: X86CPU::xlevel2 QOM property
  scripts: x86-cpu-model-dump script

 scripts/x86-cpu-model-dump          | 221 ++++++++++++++++++++++++++++++++++++
 scripts/x86-cpu-model-dump-selftest |  38 +++++++
 target-i386/cpu.c                   | 180 +++++++++++++++++++----------
 3 files changed, 382 insertions(+), 57 deletions(-)
 create mode 100755 scripts/x86-cpu-model-dump
 create mode 100755 scripts/x86-cpu-model-dump-selftest

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] target-i386: Move error handling to end of x86_cpu_parse_featurestr()

[Eduardo Habkost]

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 03b33cf..f83d586 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1961,8 +1961,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
             object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
         }
         if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+            goto out;
         }
         featurestr = strtok(NULL, ",");
     }
@@ -1978,6 +1977,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         env->features[w] |= plus_features[w];
         env->features[w] &= ~minus_features[w];
     }
+
+out:
+    if (local_err) {
+        error_propagate(errp, local_err);
+    }
 }
 
 /* Print all cpuid feature names in featureset
-- 
2.1.0

[Qemu-devel] [PATCH 2/6] target-i386: Remove underscores from feature names

[Eduardo Habkost]

The underscores will be translated by x86_cpu_parse_featurestr().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f83d586..099ed03 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -188,11 +188,11 @@ static const char *feature_name[] = {
 };
 static const char *ext_feature_name[] = {
     "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor",
-    "ds_cpl", "vmx", "smx", "est",
+    "ds-cpl", "vmx", "smx", "est",
     "tm2", "ssse3", "cid", NULL,
     "fma", "cx16", "xtpr", "pdcm",
-    NULL, "pcid", "dca", "sse4.1|sse4_1",
-    "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
+    NULL, "pcid", "dca", "sse4.1|sse4-1",
+    "sse4.2|sse4-2", "x2apic", "movbe", "popcnt",
     "tsc-deadline", "aes", "xsave", "osxsave",
     "avx", "f16c", "rdrand", "hypervisor",
 };
@@ -208,17 +208,17 @@ static const char *ext2_feature_name[] = {
     NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
     NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
     "nx|xd", NULL, "mmxext", NULL /* mmx */,
-    NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
+    NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
     NULL, "lm|i64", "3dnowext", "3dnow",
 };
 static const char *ext3_feature_name[] = {
-    "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */,
+    "lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD ExtApicSpace */,
     "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
     "3dnowprefetch", "osvw", "ibs", "xop",
     "skinit", "wdt", NULL, "lwp",
-    "fma4", "tce", NULL, "nodeid_msr",
-    NULL, "tbm", "topoext", "perfctr_core",
-    "perfctr_nb", NULL, NULL, NULL,
+    "fma4", "tce", NULL, "nodeid-msr",
+    NULL, "tbm", "topoext", "perfctr-core",
+    "perfctr-nb", NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
 };
 
@@ -234,8 +234,8 @@ static const char *ext4_feature_name[] = {
 };
 
 static const char *kvm_feature_name[] = {
-    "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-    "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+    "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+    "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
@@ -245,9 +245,9 @@ static const char *kvm_feature_name[] = {
 };
 
 static const char *svm_feature_name[] = {
-    "npt", "lbrv", "svm_lock", "nrip_save",
-    "tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-    NULL, NULL, "pause_filter", NULL,
+    "npt", "lbrv", "svm-lock", "nrip-save",
+    "tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+    NULL, NULL, "pause-filter", NULL,
     "pfthreshold", NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
     NULL, NULL, NULL, NULL,
@@ -256,7 +256,7 @@ static const char *svm_feature_name[] = {
 };
 
 static const char *cpuid_7_0_ebx_feature_name[] = {
-    "fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
+    "fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
     "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
     "avx512f", NULL, "rdseed", "adx", "smap", NULL, NULL, NULL,
     NULL, NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
@@ -1899,13 +1899,13 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
 
     while (featurestr) {
         char *val;
+        feat2prop(featurestr);
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
         } 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, "xlevel")) {
                 char *err;
                 char num[32];
@@ -1957,7 +1957,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
             }
         } else {
-            feat2prop(featurestr);
             object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
         }
         if (local_err) {
-- 
2.1.0

[Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

This uses the feature name arrays to register "feat-*" QOM properties
for feature flags. This simply adds the properties so they can be
configured using -global, but doesn't change x86_cpu_parse_featurestr()
to use them yet.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 099ed03..f29e55e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2883,12 +2883,103 @@ out:
     }
 }
 
+typedef struct FeatureProperty {
+    FeatureWord word;
+    uint32_t mask;
+} FeatureProperty;
+
+
+static void x86_cpu_get_feature_prop(Object *obj,
+                                     struct Visitor *v,
+                                     void *opaque,
+                                     const char *name,
+                                     Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    FeatureProperty *fp = opaque;
+    bool value = (env->features[fp->word] & fp->mask) == fp->mask;
+    visit_type_bool(v, &value, name, errp);
+}
+
+static void x86_cpu_set_feature_prop(Object *obj,
+                                     struct Visitor *v,
+                                     void *opaque,
+                                     const char *name,
+                                     Error **errp)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    CPUX86State *env = &cpu->env;
+    FeatureProperty *fp = opaque;
+    bool value;
+    visit_type_bool(v, &value, name, errp);
+    if (value) {
+        env->features[fp->word] |= fp->mask;
+    } else {
+        env->features[fp->word] &= ~fp->mask;
+    }
+}
+
+/* Register a boolean feature-bits property.
+ * If mask has multiple bits, all must be set for the property to return true.
+ * The same property name can be registered multiple times to make it affect
+ * multiple bits in the same FeatureWord.
+ */
+static void x86_cpu_register_feature_prop(X86CPU *cpu,
+                                          const char *prop_name,
+                                          FeatureWord w,
+                                          uint32_t mask)
+{
+    FeatureProperty *fp;
+    ObjectProperty *op;
+    op = object_property_find(OBJECT(cpu), prop_name, NULL);
+    if (op) {
+        fp = op->opaque;
+        assert(fp->word == w);
+        fp->mask |= mask;
+    } else {
+        fp = g_new0(FeatureProperty, 1);
+        fp->word = w;
+        fp->mask = mask;
+        object_property_add(OBJECT(cpu), prop_name, "bool",
+                            x86_cpu_get_feature_prop,
+                            x86_cpu_set_feature_prop,
+                            NULL, fp, &error_abort);
+    }
+}
+
+static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
+                                               FeatureWord w,
+                                               int bit)
+{
+    int i;
+    char **names;
+    FeatureWordInfo *fi = &feature_word_info[w];
+
+    if (!fi->feat_names) {
+        return;
+    }
+    if (!fi->feat_names[bit]) {
+        return;
+    }
+
+    names = g_strsplit(fi->feat_names[bit], "|", 0);
+    for (i = 0; names[i]; i++) {
+        char *feat_name = names[i];
+        char *prop_name = g_strdup_printf("feat-%s", feat_name);
+        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit));
+        g_free(prop_name);
+    }
+    g_strfreev(names);
+}
+
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
     X86CPU *cpu = X86_CPU(obj);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
+    FeatureWord w;
     static int inited;
 
     cs->env_ptr = env;
@@ -2935,6 +3026,13 @@ static void x86_cpu_initfn(Object *obj)
     cpu->apic_id = -1;
 #endif
 
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        int bit;
+        for (bit = 0; bit < 32; bit++) {
+            x86_cpu_register_feature_bit_props(cpu, w, bit);
+        }
+    }
+
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
     /* init various static tables used in TCG mode */
-- 
2.1.0

[Qemu-devel] [PATCH 4/6] target-i386: Make "level" and "xlevel" properties static

[Eduardo Habkost]

Static properties require only 1 line of code, much simpler than the
existing code that requires writing new getters/setters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 40 ++--------------------------------------
 1 file changed, 2 insertions(+), 38 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f29e55e..6ccea14 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1618,38 +1618,6 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
     env->cpuid_version |= value & 0xf;
 }
 
-static void x86_cpuid_get_level(Object *obj, Visitor *v, void *opaque,
-                                const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
-
-static void x86_cpuid_set_level(Object *obj, Visitor *v, void *opaque,
-                                const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_level, name, errp);
-}
-
-static void x86_cpuid_get_xlevel(Object *obj, Visitor *v, void *opaque,
-                                 const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
-static void x86_cpuid_set_xlevel(Object *obj, Visitor *v, void *opaque,
-                                 const char *name, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-
-    visit_type_uint32(v, &cpu->env.cpuid_xlevel, name, errp);
-}
-
 static char *x86_cpuid_get_vendor(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
@@ -2994,12 +2962,6 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add(obj, "stepping", "int",
                         x86_cpuid_version_get_stepping,
                         x86_cpuid_version_set_stepping, NULL, NULL, NULL);
-    object_property_add(obj, "level", "int",
-                        x86_cpuid_get_level,
-                        x86_cpuid_set_level, NULL, NULL, NULL);
-    object_property_add(obj, "xlevel", "int",
-                        x86_cpuid_get_xlevel,
-                        x86_cpuid_set_xlevel, NULL, NULL, NULL);
     object_property_add_str(obj, "vendor",
                             x86_cpuid_get_vendor,
                             x86_cpuid_set_vendor, NULL);
@@ -3099,6 +3061,8 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, false),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
+    DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
+    DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.1.0

[Qemu-devel] [PATCH 5/6] target-i386: X86CPU::xlevel2 QOM property

[Eduardo Habkost]

We already have "level" and "xlevel", only "xlevel2" is missing.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6ccea14..ae07af4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2080,7 +2080,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
     object_property_set_int(OBJECT(cpu), def->model, "model", errp);
     object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
     object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
-    env->cpuid_xlevel2 = def->xlevel2;
+    object_property_set_int(OBJECT(cpu), def->xlevel2, "xlevel2", errp);
     cpu->cache_info_passthrough = def->cache_info_passthrough;
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
     for (w = 0; w < FEATURE_WORDS; w++) {
@@ -3063,6 +3063,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
+    DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] scripts: x86-cpu-model-dump script

[Eduardo Habkost]

This is an example script that can be used to help generate a config
file that will reproduce a given CPU model from QEMU. The generated
config file can be loaded using "-readconfig" to make QEMU create CPUs
that will look exactly like the one used when cpu-model-dump was run.

A cpu-model-dump-selftest script is also provided, to help ensure that
the output of cpu-model-dump will produce the same config when run under
cpu-model-dump again.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 scripts/x86-cpu-model-dump          | 221 ++++++++++++++++++++++++++++++++++++
 scripts/x86-cpu-model-dump-selftest |  38 +++++++
 2 files changed, 259 insertions(+)
 create mode 100755 scripts/x86-cpu-model-dump
 create mode 100755 scripts/x86-cpu-model-dump-selftest

diff --git a/scripts/x86-cpu-model-dump b/scripts/x86-cpu-model-dump
new file mode 100755
index 0000000..b3b556f
--- /dev/null
+++ b/scripts/x86-cpu-model-dump
@@ -0,0 +1,221 @@
+#!/usr/bin/env python2.7
+#
+# Script to dump CPU model information as a QEMU config file that can be loaded
+# using -readconfig
+#
+# Author: Eduardo Habkost <ehabkost@redhat.com>
+#
+# Copyright (c) 2015 Red Hat Inc.
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+# THE SOFTWARE.
+
+
+import sys, os, signal, tempfile, re
+import xml.etree.ElementTree
+
+# Allow us to load the qmp/qmp.py module:
+sys.path.append(os.path.join(os.path.dirname(sys.argv[0]), 'qmp'))
+import qmp
+
+CPU_PATH = '/machine/icc-bridge/icc/child[0]'
+RE_PROPS = re.compile('x?level2?|vendor|family|model|stepping|feat-.*|model-id')
+CPU_MAP = '/usr/share/libvirt/cpu_map.xml'
+
+# features that may not be on cpu_map.xml:
+KNOWN_FEAT_NAMES = [
+    (0x40000001,0,'eax', [
+        "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+        "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
+        None, None, None, None,
+        None, None, None, None,
+        None, None, None, None,
+        None, None, None, None,
+        "kvmclock-stable-bit", None, None, None,
+        None, None, None, None,
+    ]),
+    (0xd,1,'eax',[
+        "xsaveopt", "xsavec", "xgetbv1", "xsaves",
+    ]),
+    # CPU feature aliases don't have properties, add some special feature
+    # names telling the script to ignore them:
+    (0x80000001,0,'edx',[
+        "fpu-ALIAS", "vme-ALIAS", "de-ALIAS", "pse-ALIAS",
+        "tsc-ALIAS", "msr-ALIAS", "pae-ALIAS", "mce-ALIAS",
+        "cx8-ALIAS", "apic-ALIAS", None, None,
+        "mtrr-ALIAS", "pge-ALIAS", "mca-ALIAS", "cmov-ALIAS",
+        "pat-ALIAS", "pse36-ALIAS", None, None,
+        None, None, None, "mmx-ALIAS",
+        "fxsr-ALIAS", None, None, None,
+        None, None, None, None,
+    ])
+]
+
+def value_to_string(ptype, v):
+    """Convert property value to string parseable by -global"""
+    if ptype == "bool":
+        return v and "on" or "off"
+    elif ptype == 'string':
+        return v
+    elif ptype.startswith('int') or ptype.startswith('uint'):
+        return str(v)
+    else:
+        raise Exception("Unsupported property type: %s", ptype)
+
+def load_feat_names(cpu_map):
+    """Load feature names from libvirt cpu_map.xml"""
+    cpumap = xml.etree.ElementTree.parse(cpu_map)
+    feat_names = {}
+
+    for function,index,reg,names in KNOWN_FEAT_NAMES:
+        for bitnr,name in enumerate(names):
+            if name:
+                feat_names[(function,index,reg,bitnr)] = name
+
+    for f in cpumap.getroot().findall("./arch[@name='x86']/feature"):
+        fname = f.attrib['name']
+        for cpuid in f.findall('cpuid'):
+            function=int(cpuid.attrib['function'], 0)
+            index = 0
+            for reg in 'abcd':
+                regname = 'e%sx' % (reg)
+                if regname in cpuid.attrib:
+                    v = int(cpuid.attrib[regname], 0)
+                    for bitnr in range(32):
+                        bitval = (1 << bitnr)
+                        if v & bitval:
+                            feat_names[(function,index,regname,bitnr)] = fname
+
+    return feat_names
+
+def dump_cpu_data(qmp, cpu_path):
+
+    feat_names = load_feat_names(CPU_MAP)
+
+    props = qmp.command('qom-list', path=cpu_path)
+    props = sorted([(prop['name'], prop['type']) for prop in props])
+
+    propdict = {}
+    for pname,ptype in props:
+        if RE_PROPS.match(pname):
+            value = qmp.command('qom-get', path=cpu_path, property=pname)
+            propdict[pname] = value_to_string(ptype, value)
+            #print >>sys.stderr, pname, propdict[pname]
+
+    # sanity-check feature-words and fix filtered-features:
+    for prop in ('feature-words', 'filtered-features'):
+        reply = qmp.command('qom-get', path=cpu_path, property=prop)
+        for fw in reply:
+            function = fw['cpuid-input-eax']
+            index = fw.get('cpuid-input-ecx', 0)
+            regname = fw['cpuid-register'].lower()
+            value = fw['features']
+            for bitnr in range(32):
+                bitval = (1 << bitnr)
+                is_set = (value & bitval) != 0
+                key = (function,index,regname,bitnr)
+                keystr = "0x%x,0x%x,%s,%d" % (function, index, regname, bitnr)
+                feat_name = feat_names.get(key)
+
+                if feat_name is None:
+                    if is_set:
+                        raise Exception("Unknown feature is set: %s" % (keystr))
+                    else:
+                        continue
+
+                # special case for alias bits: ignore them
+                if feat_name.endswith('-ALIAS'):
+                    continue
+
+                pname = 'feat-%s' % (feat_name.replace('_', '-'))
+                if not propdict.has_key(pname):
+                    if is_set:
+                        raise Exception("Enabled feature with no property: %s" % pname)
+                    else:
+                        continue
+
+                # feature-word bits must match property:
+                if prop == 'feature-words':
+                    propvalue = value_to_string('bool', is_set)
+                    assert propdict[pname] == propvalue
+                # bits set on filtered-features needs property fixup:
+                elif prop == 'filtered-features' and is_set:
+                    assert propdict[pname] == 'off'
+                    propdict[pname] = 'on'
+
+    for pname in sorted(propdict.keys()):
+        pvalue = propdict[pname]
+        print '[global]'
+        print 'driver = "cpu"'
+        print 'property = "%s"' % (pname)
+        print 'value = "%s"' % (pvalue)
+        print ''
+
+def main(argv):
+    args = argv[1:]
+    if len(args) < 1:
+        print >>sys.stderr, "Usage: %s <qemu> [<arguments>...]" % (argv[0])
+        return 1
+
+    qemu = args.pop(0)
+
+    sockdir = tempfile.mkdtemp()
+    sockpath = os.path.join(sockdir, 'monitor.sock')
+    pidfile = os.path.join(sockdir, 'pidfile')
+
+    try:
+        qemu_cmd = [qemu]
+        qemu_cmd.extend(args)
+        qemu_cmd.append('-chardev')
+        qemu_cmd.append('socket,id=qmp0,path=%s,server,nowait' % (sockpath))
+        qemu_cmd.append('-qmp')
+        qemu_cmd.append('chardev:qmp0')
+        qemu_cmd.append('-daemonize')
+        qemu_cmd.append('-pidfile')
+        qemu_cmd.append(pidfile)
+
+        ret = os.spawnvp(os.P_WAIT, qemu, qemu_cmd)
+        if ret != 0:
+            print >>sys.stderr, "Failed to start QEMU"
+            return 1
+
+        srv = qmp.QEMUMonitorProtocol(sockpath)
+        srv.connect()
+
+        dump_cpu_data(srv, CPU_PATH)
+    finally:
+        try:
+            pid = int(open(pidfile, 'r').read())
+            #print >>sys.stderr, 'Killing QEMU, pid: %d' % (pid)
+            os.kill(pid, signal.SIGTERM)
+            os.waitpid(pid, 0)
+        except:
+            pass
+        try:
+            os.unlink(pidfile)
+        except:
+            pass
+        try:
+            os.unlink(sockpath)
+        except:
+            pass
+        os.rmdir(sockdir)
+
+
+if __name__ == '__main__':
+    sys.exit(main(sys.argv))
diff --git a/scripts/x86-cpu-model-dump-selftest b/scripts/x86-cpu-model-dump-selftest
new file mode 100755
index 0000000..eb6c9e7
--- /dev/null
+++ b/scripts/x86-cpu-model-dump-selftest
@@ -0,0 +1,38 @@
+#!/bin/bash
+# self-test script for cpu-model-dump: check if the generated config file
+# will make the script generate a similar config file
+#
+# Author: Eduardo Habkost <ehabkost@redhat.com>
+#
+# Copyright (c) 2015 Red Hat Inc.
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+# THE SOFTWARE.
+
+DUMP_SCRIPT="$(dirname $0)/x86-cpu-model-dump"
+
+QEMU="$1"
+shift
+
+dump1="$(mktemp)"
+dump2="$(mktemp)"
+"$DUMP_SCRIPT" "$QEMU" "$@" > "$dump1"
+"$DUMP_SCRIPT" "$QEMU" -readconfig "$dump1" > "$dump2"
+
+diff -u "$dump1" "$dump2"
+rm -f "$dump1" "$dump2"
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Paolo Bonzini]

On 07/04/2015 22:46, Eduardo Habkost wrote:
> This uses the feature name arrays to register "feat-*" QOM properties
> for feature flags. This simply adds the properties so they can be
> configured using -global, but doesn't change x86_cpu_parse_featurestr()
> to use them yet.

Out of curiosity, why the prefix?  (Also, perhaps a prefix such as
"cpuid-*" would be better since the property often only affects the
cpuid leaves, rather than the availability of the feature itself).

Paolo

> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 099ed03..f29e55e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2883,12 +2883,103 @@ out:
>      }
>  }
>  
> +typedef struct FeatureProperty {
> +    FeatureWord word;
> +    uint32_t mask;
> +} FeatureProperty;
> +
> +
> +static void x86_cpu_get_feature_prop(Object *obj,
> +                                     struct Visitor *v,
> +                                     void *opaque,
> +                                     const char *name,
> +                                     Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    FeatureProperty *fp = opaque;
> +    bool value = (env->features[fp->word] & fp->mask) == fp->mask;
> +    visit_type_bool(v, &value, name, errp);
> +}
> +
> +static void x86_cpu_set_feature_prop(Object *obj,
> +                                     struct Visitor *v,
> +                                     void *opaque,
> +                                     const char *name,
> +                                     Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    FeatureProperty *fp = opaque;
> +    bool value;
> +    visit_type_bool(v, &value, name, errp);
> +    if (value) {
> +        env->features[fp->word] |= fp->mask;
> +    } else {
> +        env->features[fp->word] &= ~fp->mask;
> +    }
> +}
> +
> +/* Register a boolean feature-bits property.
> + * If mask has multiple bits, all must be set for the property to return true.
> + * The same property name can be registered multiple times to make it affect
> + * multiple bits in the same FeatureWord.
> + */
> +static void x86_cpu_register_feature_prop(X86CPU *cpu,
> +                                          const char *prop_name,
> +                                          FeatureWord w,
> +                                          uint32_t mask)
> +{
> +    FeatureProperty *fp;
> +    ObjectProperty *op;
> +    op = object_property_find(OBJECT(cpu), prop_name, NULL);
> +    if (op) {
> +        fp = op->opaque;
> +        assert(fp->word == w);
> +        fp->mask |= mask;
> +    } else {
> +        fp = g_new0(FeatureProperty, 1);
> +        fp->word = w;
> +        fp->mask = mask;
> +        object_property_add(OBJECT(cpu), prop_name, "bool",
> +                            x86_cpu_get_feature_prop,
> +                            x86_cpu_set_feature_prop,
> +                            NULL, fp, &error_abort);
> +    }
> +}
> +
> +static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
> +                                               FeatureWord w,
> +                                               int bit)
> +{
> +    int i;
> +    char **names;
> +    FeatureWordInfo *fi = &feature_word_info[w];
> +
> +    if (!fi->feat_names) {
> +        return;
> +    }
> +    if (!fi->feat_names[bit]) {
> +        return;
> +    }
> +
> +    names = g_strsplit(fi->feat_names[bit], "|", 0);
> +    for (i = 0; names[i]; i++) {
> +        char *feat_name = names[i];
> +        char *prop_name = g_strdup_printf("feat-%s", feat_name);
> +        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit));
> +        g_free(prop_name);
> +    }
> +    g_strfreev(names);
> +}
> +
>  static void x86_cpu_initfn(Object *obj)
>  {
>      CPUState *cs = CPU(obj);
>      X86CPU *cpu = X86_CPU(obj);
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>      CPUX86State *env = &cpu->env;
> +    FeatureWord w;
>      static int inited;
>  
>      cs->env_ptr = env;
> @@ -2935,6 +3026,13 @@ static void x86_cpu_initfn(Object *obj)
>      cpu->apic_id = -1;
>  #endif
>  
> +    for (w = 0; w < FEATURE_WORDS; w++) {
> +        int bit;
> +        for (bit = 0; bit < 32; bit++) {
> +            x86_cpu_register_feature_bit_props(cpu, w, bit);
> +        }
> +    }
> +
>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
>  
>      /* init various static tables used in TCG mode */
> 

Re: [Qemu-devel] [PATCH 1/6] target-i386: Move error handling to end of x86_cpu_parse_featurestr()

[Paolo Bonzini]

On 07/04/2015 22:46, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 03b33cf..f83d586 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1961,8 +1961,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>              object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
>          }
>          if (local_err) {
> -            error_propagate(errp, local_err);
> -            return;
> +            goto out;
>          }
>          featurestr = strtok(NULL, ",");
>      }
> @@ -1978,6 +1977,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>          env->features[w] |= plus_features[w];
>          env->features[w] &= ~minus_features[w];
>      }
> +
> +out:
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +    }
>  }
>  
>  /* Print all cpuid feature names in featureset
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 5/6] target-i386: X86CPU::xlevel2 QOM property

[Paolo Bonzini]

On 07/04/2015 22:46, Eduardo Habkost wrote:
> We already have "level" and "xlevel", only "xlevel2" is missing.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 6ccea14..ae07af4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2080,7 +2080,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>      object_property_set_int(OBJECT(cpu), def->model, "model", errp);
>      object_property_set_int(OBJECT(cpu), def->stepping, "stepping", errp);
>      object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", errp);
> -    env->cpuid_xlevel2 = def->xlevel2;
> +    object_property_set_int(OBJECT(cpu), def->xlevel2, "xlevel2", errp);
>      cpu->cache_info_passthrough = def->cache_info_passthrough;
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp);
>      for (w = 0; w < FEATURE_WORDS; w++) {
> @@ -3063,6 +3063,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, 0),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, 0),
> +    DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, 0),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 10:30:52AM +0200, Paolo Bonzini wrote:
> 
> 
> On 07/04/2015 22:46, Eduardo Habkost wrote:
> > This uses the feature name arrays to register "feat-*" QOM properties
> > for feature flags. This simply adds the properties so they can be
> > configured using -global, but doesn't change x86_cpu_parse_featurestr()
> > to use them yet.
> 
> Out of curiosity, why the prefix?  (Also, perhaps a prefix such as
> "cpuid-*" would be better since the property often only affects the
> cpuid leaves, rather than the availability of the feature itself).

The prefix exists to allow those properties to be easily identified by
software that doesn't know the full list of feature names (I even took
advantage of that on the x86-cpu-model-dump script).

About the reason for using the "feat-" prefix, the short answer is "it's
the prefix used in the last patch that implemented this (by Igor)". I
think the first suggestion was to use "f-", then we changed to
"feature-" or "feat-", and simply stayed using "feat-" in the last few
versions.

But I like the "cpuid-" suggestion and plan to use it on v2. Any
objections?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Igor Mammedov]

On Tue,  7 Apr 2015 17:46:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This uses the feature name arrays to register "feat-*" QOM properties
> for feature flags. This simply adds the properties so they can be
> configured using -global, but doesn't change x86_cpu_parse_featurestr()
> to use them yet.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 099ed03..f29e55e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2883,12 +2883,103 @@ out:
>      }
>  }
>  
> +typedef struct FeatureProperty {
> +    FeatureWord word;
> +    uint32_t mask;
> +} FeatureProperty;
> +
> +
> +static void x86_cpu_get_feature_prop(Object *obj,
> +                                     struct Visitor *v,
> +                                     void *opaque,
> +                                     const char *name,
> +                                     Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    FeatureProperty *fp = opaque;
> +    bool value = (env->features[fp->word] & fp->mask) == fp->mask;
> +    visit_type_bool(v, &value, name, errp);
> +}
> +
> +static void x86_cpu_set_feature_prop(Object *obj,
> +                                     struct Visitor *v,
> +                                     void *opaque,
> +                                     const char *name,
> +                                     Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    FeatureProperty *fp = opaque;
> +    bool value;
> +    visit_type_bool(v, &value, name, errp);
> +    if (value) {
> +        env->features[fp->word] |= fp->mask;
> +    } else {
> +        env->features[fp->word] &= ~fp->mask;
> +    }
> +}
> +
> +/* Register a boolean feature-bits property.
> + * If mask has multiple bits, all must be set for the property to return true.
> + * The same property name can be registered multiple times to make it affect
> + * multiple bits in the same FeatureWord.
> + */
> +static void x86_cpu_register_feature_prop(X86CPU *cpu,
> +                                          const char *prop_name,
> +                                          FeatureWord w,
> +                                          uint32_t mask)
> +{
> +    FeatureProperty *fp;
> +    ObjectProperty *op;
> +    op = object_property_find(OBJECT(cpu), prop_name, NULL);
> +    if (op) {
> +        fp = op->opaque;
> +        assert(fp->word == w);
> +        fp->mask |= mask;
> +    } else {
> +        fp = g_new0(FeatureProperty, 1);
> +        fp->word = w;
> +        fp->mask = mask;
> +        object_property_add(OBJECT(cpu), prop_name, "bool",
> +                            x86_cpu_get_feature_prop,
> +                            x86_cpu_set_feature_prop,
> +                            NULL, fp, &error_abort);
> +    }
> +}
it would be better to create generic bit property and replace above code with it
something similar to object_property_add_uint32_ptr()


> +
> +static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
> +                                               FeatureWord w,
> +                                               int bit)
> +{
> +    int i;
> +    char **names;
> +    FeatureWordInfo *fi = &feature_word_info[w];
> +
> +    if (!fi->feat_names) {
> +        return;
> +    }
> +    if (!fi->feat_names[bit]) {
> +        return;
> +    }
> +
> +    names = g_strsplit(fi->feat_names[bit], "|", 0);
> +    for (i = 0; names[i]; i++) {
> +        char *feat_name = names[i];
> +        char *prop_name = g_strdup_printf("feat-%s", feat_name);
> +        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit));
it might be better instead of creating duplicate property to make an alias

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 01:36:29PM +0200, Igor Mammedov wrote:
> On Tue,  7 Apr 2015 17:46:40 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This uses the feature name arrays to register "feat-*" QOM properties
> > for feature flags. This simply adds the properties so they can be
> > configured using -global, but doesn't change x86_cpu_parse_featurestr()
> > to use them yet.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 099ed03..f29e55e 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2883,12 +2883,103 @@ out:
> >      }
> >  }
> >  
> > +typedef struct FeatureProperty {
> > +    FeatureWord word;
> > +    uint32_t mask;
> > +} FeatureProperty;
> > +
> > +
> > +static void x86_cpu_get_feature_prop(Object *obj,
> > +                                     struct Visitor *v,
> > +                                     void *opaque,
> > +                                     const char *name,
> > +                                     Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env = &cpu->env;
> > +    FeatureProperty *fp = opaque;
> > +    bool value = (env->features[fp->word] & fp->mask) == fp->mask;
> > +    visit_type_bool(v, &value, name, errp);
> > +}
> > +
> > +static void x86_cpu_set_feature_prop(Object *obj,
> > +                                     struct Visitor *v,
> > +                                     void *opaque,
> > +                                     const char *name,
> > +                                     Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env = &cpu->env;
> > +    FeatureProperty *fp = opaque;
> > +    bool value;
> > +    visit_type_bool(v, &value, name, errp);
> > +    if (value) {
> > +        env->features[fp->word] |= fp->mask;
> > +    } else {
> > +        env->features[fp->word] &= ~fp->mask;
> > +    }
> > +}
> > +
> > +/* Register a boolean feature-bits property.
> > + * If mask has multiple bits, all must be set for the property to return true.
> > + * The same property name can be registered multiple times to make it affect
> > + * multiple bits in the same FeatureWord.
> > + */
> > +static void x86_cpu_register_feature_prop(X86CPU *cpu,
> > +                                          const char *prop_name,
> > +                                          FeatureWord w,
> > +                                          uint32_t mask)
> > +{
> > +    FeatureProperty *fp;
> > +    ObjectProperty *op;
> > +    op = object_property_find(OBJECT(cpu), prop_name, NULL);
> > +    if (op) {
> > +        fp = op->opaque;
> > +        assert(fp->word == w);
> > +        fp->mask |= mask;
> > +    } else {
> > +        fp = g_new0(FeatureProperty, 1);
> > +        fp->word = w;
> > +        fp->mask = mask;
> > +        object_property_add(OBJECT(cpu), prop_name, "bool",
> > +                            x86_cpu_get_feature_prop,
> > +                            x86_cpu_set_feature_prop,
> > +                            NULL, fp, &error_abort);
> > +    }
> > +}
> it would be better to create generic bit property and replace above code with it
> something similar to object_property_add_uint32_ptr()

object_property_add_*_ptr() adds read-only properties, and I didn't want
to make object_property_add_bit_ptr() inconsistent with the other
functions. But maybe it is better to have an inconsistent but reusable
API than making the new code non-reusable by keeping it inside
target-i386/cpu.c. I will give it a try.

BTW, it is on my wishlist to remove the existing duplication in
DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to
support the same data types without duplicating code, but this may take
a while.



> 
> 
> > +
> > +static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
> > +                                               FeatureWord w,
> > +                                               int bit)
> > +{
> > +    int i;
> > +    char **names;
> > +    FeatureWordInfo *fi = &feature_word_info[w];
> > +
> > +    if (!fi->feat_names) {
> > +        return;
> > +    }
> > +    if (!fi->feat_names[bit]) {
> > +        return;
> > +    }
> > +
> > +    names = g_strsplit(fi->feat_names[bit], "|", 0);
> > +    for (i = 0; names[i]; i++) {
> > +        char *feat_name = names[i];
> > +        char *prop_name = g_strdup_printf("feat-%s", feat_name);
> > +        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit));
> it might be better instead of creating duplicate property to make an alias

I wasn't aware of property aliases. I will take a look. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Paolo Bonzini]

On 08/04/2015 14:20, Eduardo Habkost wrote:
>> > it would be better to create generic bit property and replace above code with it
>> > something similar to object_property_add_uint32_ptr()
> object_property_add_*_ptr() adds read-only properties, and I didn't want
> to make object_property_add_bit_ptr() inconsistent with the other
> functions. But maybe it is better to have an inconsistent but reusable
> API than making the new code non-reusable by keeping it inside
> target-i386/cpu.c. I will give it a try.

add_*_ptr() is read-only because read-only properties do not require
validations (at least that's the common case).  So I think the
inconsistent API is worse than a local one.

> BTW, it is on my wishlist to remove the existing duplication in
> DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to
> support the same data types without duplicating code, but this may take
> a while.

Yeah, that would be nice...

Paolo

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Andreas Färber]

Am 08.04.2015 um 13:06 schrieb Eduardo Habkost:
> On Wed, Apr 08, 2015 at 10:30:52AM +0200, Paolo Bonzini wrote:
>> On 07/04/2015 22:46, Eduardo Habkost wrote:
>>> This uses the feature name arrays to register "feat-*" QOM properties
>>> for feature flags. This simply adds the properties so they can be
>>> configured using -global, but doesn't change x86_cpu_parse_featurestr()
>>> to use them yet.
>>
>> Out of curiosity, why the prefix?  (Also, perhaps a prefix such as
>> "cpuid-*" would be better since the property often only affects the
>> cpuid leaves, rather than the availability of the feature itself).
> 
> The prefix exists to allow those properties to be easily identified by
> software that doesn't know the full list of feature names (I even took
> advantage of that on the x86-cpu-model-dump script).
> 
> About the reason for using the "feat-" prefix, the short answer is "it's
> the prefix used in the last patch that implemented this (by Igor)". I
> think the first suggestion was to use "f-", then we changed to
> "feature-" or "feat-", and simply stayed using "feat-" in the last few
> versions.

Fair to mention that they did not all get equal review. ;)

> But I like the "cpuid-" suggestion and plan to use it on v2. Any
> objections?

Assuming it's technically correct, +1 for cpuid-.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Igor Mammedov]

On Wed, 08 Apr 2015 14:24:35 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 08/04/2015 14:20, Eduardo Habkost wrote:
> >> > it would be better to create generic bit property and replace above code with it
> >> > something similar to object_property_add_uint32_ptr()
> > object_property_add_*_ptr() adds read-only properties, and I didn't want
> > to make object_property_add_bit_ptr() inconsistent with the other
> > functions. But maybe it is better to have an inconsistent but reusable
> > API than making the new code non-reusable by keeping it inside
> > target-i386/cpu.c. I will give it a try.
> 
> add_*_ptr() is read-only because read-only properties do not require
> validations (at least that's the common case).  So I think the
> inconsistent API is worse than a local one.
I've had on my todo list adding add_*_ptr_RW() variants
because in common case there isn't any need in validation and
with current API we are forced to duplicate custom setter/getter
every time for trivial type.
For example properties like "level" or "xlevel".

The same goes for generic bitmap setter (i.e.) without validation,
which is what we need in this case. We [could] do validation at realize
time when it's possible to analyze combination of feature flags.

> 
> > BTW, it is on my wishlist to remove the existing duplication in
> > DEFINE_PROP_*(), QAPI, and object_property_add_*(), that are supposed to
> > support the same data types without duplicating code, but this may take
> > a while.
> 
> Yeah, that would be nice...
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 04:09:32PM +0200, Igor Mammedov wrote:
> On Wed, 08 Apr 2015 14:24:35 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > 
> > 
> > On 08/04/2015 14:20, Eduardo Habkost wrote:
> > >> > it would be better to create generic bit property and replace above code with it
> > >> > something similar to object_property_add_uint32_ptr()
> > > object_property_add_*_ptr() adds read-only properties, and I didn't want
> > > to make object_property_add_bit_ptr() inconsistent with the other
> > > functions. But maybe it is better to have an inconsistent but reusable
> > > API than making the new code non-reusable by keeping it inside
> > > target-i386/cpu.c. I will give it a try.
> > 
> > add_*_ptr() is read-only because read-only properties do not require
> > validations (at least that's the common case).  So I think the
> > inconsistent API is worse than a local one.
> I've had on my todo list adding add_*_ptr_RW() variants
> because in common case there isn't any need in validation and
> with current API we are forced to duplicate custom setter/getter
> every time for trivial type.
> For example properties like "level" or "xlevel".
> 
> The same goes for generic bitmap setter (i.e.) without validation,
> which is what we need in this case. We [could] do validation at realize
> time when it's possible to analyze combination of feature flags.

Even if no property value validation is done at the setter, we still
want to prevent the property from being changed after realize (which is
something I forgot to do in this patch, and we forgot to do on most
X86CPU setters).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/6] target-i386: Register QOM properties for feature flags

[Igor Mammedov]

On Wed, 8 Apr 2015 12:01:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 08, 2015 at 04:09:32PM +0200, Igor Mammedov wrote:
> > On Wed, 08 Apr 2015 14:24:35 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > 
> > > 
> > > On 08/04/2015 14:20, Eduardo Habkost wrote:
> > > >> > it would be better to create generic bit property and replace above code with it
> > > >> > something similar to object_property_add_uint32_ptr()
> > > > object_property_add_*_ptr() adds read-only properties, and I didn't want
> > > > to make object_property_add_bit_ptr() inconsistent with the other
> > > > functions. But maybe it is better to have an inconsistent but reusable
> > > > API than making the new code non-reusable by keeping it inside
> > > > target-i386/cpu.c. I will give it a try.
> > > 
> > > add_*_ptr() is read-only because read-only properties do not require
> > > validations (at least that's the common case).  So I think the
> > > inconsistent API is worse than a local one.
> > I've had on my todo list adding add_*_ptr_RW() variants
> > because in common case there isn't any need in validation and
> > with current API we are forced to duplicate custom setter/getter
> > every time for trivial type.
> > For example properties like "level" or "xlevel".
> > 
> > The same goes for generic bitmap setter (i.e.) without validation,
> > which is what we need in this case. We [could] do validation at realize
> > time when it's possible to analyze combination of feature flags.
> 
> Even if no property value validation is done at the setter, we still
> want to prevent the property from being changed after realize (which is
> something I forgot to do in this patch, and we forgot to do on most
> X86CPU setters).
Yep, the same goes for every generic setters in current API
since realize is device specific thingy that's why it's supported by
static properties only now.
We should have something like Stefan's object_property_add_link(...,*check,...)
but for generic properties.