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

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

Changes v1 -> v2:
* Use "cpuid-" prefix
* Register release function for properties
* Don't remove underscores from feature names on the feature array yet
  * Remove underscores on x86_cpu_register_feature_prop() instead,
    so we don't need to touch x86_cpu_parse_featurestr() by now
* Make x86-cpu-model-dump return a reasonable exit code
* Add dev->realized check to property setter

Eduardo Habkost (4):
  target-i386: Make "level" and "xlevel" properties static
  target-i386: X86CPU::xlevel2 QOM property
  target-i386: Register QOM properties for feature flags
  scripts: x86-cpu-model-dump script

 scripts/x86-cpu-model-dump          | 221 ++++++++++++++++++++++++++++++++++++
 scripts/x86-cpu-model-dump-selftest |  41 +++++++
 target-i386/cpu.c                   | 156 ++++++++++++++++++-------
 3 files changed, 379 insertions(+), 39 deletions(-)
 create mode 100755 scripts/x86-cpu-model-dump
 create mode 100755 scripts/x86-cpu-model-dump-selftest

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] 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 03b33cf..2bbf01d 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);
@@ -2900,12 +2868,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);
@@ -2998,6 +2960,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 v2 2/4] 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>
Reviewed-by: Paolo Bonzini <pbonzini@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 2bbf01d..e657f10 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2077,7 +2077,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++) {
@@ -2962,6 +2962,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 v2 3/4] 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>
---
Changes v1 -> v2:
* Use "cpuid-" prefix instead of "feat-"
* Register release function for property
* Convert '_' to '-' on feature name before registering property
* Add dev->realized check to property setter
---
 target-i386/cpu.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e657f10..7099027 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2848,12 +2848,118 @@ 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);
+    DeviceState *dev = DEVICE(obj);
+    CPUX86State *env = &cpu->env;
+    FeatureProperty *fp = opaque;
+    bool value;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_bool(v, &value, name, errp);
+    if (value) {
+        env->features[fp->word] |= fp->mask;
+    } else {
+        env->features[fp->word] &= ~fp->mask;
+    }
+}
+
+static void x86_cpu_release_feature_prop(Object *obj, const char *name,
+                                         void *opaque)
+{
+    FeatureProperty *prop = opaque;
+    g_free(prop);
+}
+
+/* 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,
+                            x86_cpu_release_feature_prop, 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];
+        feat2prop(feat_name);
+        char *prop_name = g_strdup_printf("cpuid-%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;
@@ -2894,6 +3000,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 v2 4/4] 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>
---
Changes v1 -> v2:
* Use "cpuid-" prefix instead of "feat-"
* Exit earlier if QEMU fails
* Exit code of the script will match QEMU or diff exit code
---
 scripts/x86-cpu-model-dump          | 221 ++++++++++++++++++++++++++++++++++++
 scripts/x86-cpu-model-dump-selftest |  41 +++++++
 2 files changed, 262 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..38812ab
--- /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|cpuid-.*|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 = 'cpuid-%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..3c62bca
--- /dev/null
+++ b/scripts/x86-cpu-model-dump-selftest
@@ -0,0 +1,41 @@
+#!/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"
+E="$?"
+
+rm -f "$dump1" "$dump2"
+exit "$E"
-- 
2.1.0

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

[Igor Mammedov]

On Wed,  8 Apr 2015 16:02:40 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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>
Reviewed-by: Igor Mammedov <imammedo@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 03b33cf..2bbf01d 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);
> @@ -2900,12 +2868,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);
> @@ -2998,6 +2960,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()
>  };
>  

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

[Eduardo Habkost]

On Wed, Apr 08, 2015 at 04:02:42PM -0300, Eduardo Habkost wrote:
[...]
> +    names = g_strsplit(fi->feat_names[bit], "|", 0);

I forgot to implement the property aliases Igor asked for. I will submit
v3 of this patch later.

> +    for (i = 0; names[i]; i++) {
> +        char *feat_name = names[i];
> +        feat2prop(feat_name);
> +        char *prop_name = g_strdup_printf("cpuid-%s", feat_name);
> +        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL << bit));
> +        g_free(prop_name);
> +    }
> +    g_strfreev(names);
> +}
> +
-- 
Eduardo

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

[Igor Mammedov]

On Thu, 9 Apr 2015 15:48:30 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 08, 2015 at 04:02:42PM -0300, Eduardo Habkost wrote:
> [...]
> > +    names = g_strsplit(fi->feat_names[bit], "|", 0);
> 
> I forgot to implement the property aliases Igor asked for. I will
> submit v3 of this patch later.
> 
> > +    for (i = 0; names[i]; i++) {
> > +        char *feat_name = names[i];
> > +        feat2prop(feat_name);
> > +        char *prop_name = g_strdup_printf("cpuid-%s", feat_name);
BTW: I've remembered why we've chosen feat- vs. cpuid- prefix
it was to make CPU features platform neutral so that libvirt
would use the same prefix for x86, arm other targets.

> > +        x86_cpu_register_feature_prop(cpu, prop_name, w, (1UL <<
> > bit));
> > +        g_free(prop_name);
> > +    }
> > +    g_strfreev(names);
> > +}
> > +

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

[Paolo Bonzini]

On 10/04/2015 09:27, Igor Mammedov wrote:
> > +    for (i = 0; names[i]; i++) {
> > +        char *feat_name = names[i];
> > +        feat2prop(feat_name);
> > +        char *prop_name = g_strdup_printf("cpuid-%s", feat_name);
> 
> BTW: I've remembered why we've chosen feat- vs. cpuid- prefix
> it was to make CPU features platform neutral so that libvirt
> would use the same prefix for x86, arm other targets.

Ok, that make sense, but if we want to make it platform-neutral, let's
spell it "feature-" or remove the prefix altogether.

If we remove it, perhaps we could add a QOM property with the list of
features?

But I don't want to bikeshed too much.

Paolo

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

[Andreas Färber]

Am 10.04.2015 um 10:21 schrieb Paolo Bonzini:
> On 10/04/2015 09:27, Igor Mammedov wrote:
>>> +    for (i = 0; names[i]; i++) {
>>> +        char *feat_name = names[i];
>>> +        feat2prop(feat_name);
>>> +        char *prop_name = g_strdup_printf("cpuid-%s", feat_name);
>>
>> BTW: I've remembered why we've chosen feat- vs. cpuid- prefix
>> it was to make CPU features platform neutral so that libvirt
>> would use the same prefix for x86, arm other targets.
> 
> Ok, that make sense, but if we want to make it platform-neutral, let's
> spell it "feature-" or remove the prefix altogether.
> 
> If we remove it, perhaps we could add a QOM property with the list of
> features?
> 
> But I don't want to bikeshed too much.

I had suggested a container sub-object for property grouping but Anthony
preferred a prefix.

Btw a suffix could work as well and would read more natural. QMP don't
sort alphabetically anyway.

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 v2 3/4] target-i386: Register QOM properties for feature flags

[Paolo Bonzini]

On 10/04/2015 10:53, Andreas Färber wrote:
> > Ok, that make sense, but if we want to make it platform-neutral, let's
> > spell it "feature-" or remove the prefix altogether.
> > 
> > If we remove it, perhaps we could add a QOM property with the list of
> > features?
> > 
> > But I don't want to bikeshed too much.
> 
> I had suggested a container sub-object for property grouping but Anthony
> preferred a prefix.

And I agree. :)  However, it seems to me that properties such as
feature-words and filtered-features are a good precedent so we might as
well add all-features.

> Btw a suffix could work as well and would read more natural. QMP don't
> sort alphabetically anyway.

QMP doesn't, but the front-end (scripts/qom-list for example) could.

Paolo

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

[Andreas Färber]

Am 10.04.2015 um 10:59 schrieb Paolo Bonzini:
> On 10/04/2015 10:53, Andreas Färber wrote:
>>> Ok, that make sense, but if we want to make it platform-neutral, let's
>>> spell it "feature-" or remove the prefix altogether.
>>>
>>> If we remove it, perhaps we could add a QOM property with the list of
>>> features?
>>>
>>> But I don't want to bikeshed too much.
[...]
>> Btw a suffix could work as well and would read more natural. QMP don't
>> sort alphabetically anyway.
> 
> QMP doesn't, but the front-end (scripts/qom-list for example) could.

I don't think our scripts should modify QMP responses. Some commands
transmit things in reverse and we should find out why and fix them
instead of papering over it. In particular for scripts/qom-tree's
sibling nodes (qom-list) I find that very annoying.

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 v2 3/4] target-i386: Register QOM properties for feature flags

[Eduardo Habkost]

On Fri, Apr 10, 2015 at 10:21:49AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/04/2015 09:27, Igor Mammedov wrote:
> > > +    for (i = 0; names[i]; i++) {
> > > +        char *feat_name = names[i];
> > > +        feat2prop(feat_name);
> > > +        char *prop_name = g_strdup_printf("cpuid-%s", feat_name);
> > 
> > BTW: I've remembered why we've chosen feat- vs. cpuid- prefix
> > it was to make CPU features platform neutral so that libvirt
> > would use the same prefix for x86, arm other targets.
> 
> Ok, that make sense, but if we want to make it platform-neutral, let's
> spell it "feature-" or remove the prefix altogether.

I am liking the idea of removing the prefix. I can't find a compelling
reason for making boolean properties that affect CPUID special and
different from other boolean properties in X86CPU.

> 
> If we remove it, perhaps we could add a QOM property with the list of
> features?

I am not even sure this will be necessary. libvirt, for example, is
already aware of the list of CPU feature flags it can set/unset, so it
probably wouldn't need the list. If it becomes necessary, we can add it.

I will send v4 without the prefix.

-- 
Eduardo