[Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[Eduardo Habkost]

Add QMP command to allow management software to query for
CPU information for the running host.

The data returned by the command is in the form of a dictionary
of QOM properties.

This series depends on the "Add runnability info to
query-cpu-definitions" series I sent 2 weeks ago.

Git tree:
  https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu

Example output for x86:

    {
        "qom-properties": {
            "pfthreshold": false,
            "pku": false,
            "rtm": false,
            "tsc-deadline": true,
            "xstore-en": false,
            "cpuid-0xb": true,
            "abm": true,
            "ia64": false,
            "kvm-mmu": false,
            "xsaveopt": true,
            "hv-spinlocks": -1,
            "hotpluggable": true,
            "tce": false,
            "smep": true,
            "nx": true,
            "xcrypt": false,
            "sse4_2": true,
            "clflush": true,
            "sse4_1": true,
            "flushbyasid": false,
            "kvm-steal-time": true,
            "host-cache-info": false,
            "tsc": true,
            "adx": false,
            "ds": false,
            "fxsr": true,
            "tm": false,
            "hv-relaxed": false,
            "pclmuldq": true,
            "xgetbv1": false,
            "xstore": false,
            "migratable": true,
            "vme": true,
            "vendor": "GenuineIntel",
            "arat": true,
            "ffxsr": false,
            "de": true,
            "aes": true,
            "pse": true,
            "ds-cpl": false,
            "tbm": false,
            "sse": true,
            "phe-en": false,
            "f16c": true,
            "hotplugged": true,
            "mpx": false,
            "tsc-adjust": true,
            "avx512f": false,
            "avx2": true,
            "misalignsse": false,
            "level": 13,
            "pbe": false,
            "cx16": true,
            "avx512pf": false,
            "movbe": true,
            "perfctr-nb": false,
            "enforce": false,
            "ospke": false,
            "pmu": true,
            "fma4": false,
            "stepping": 1,
            "feature-words": [
                {
                    "cpuid-register": "EAX",
                    "cpuid-input-eax": 6,
                    "features": 4
                },
                {
                    "cpuid-register": "EAX",
                    "cpuid-input-eax": 13,
                    "features": 1,
                    "cpuid-input-ecx": 1
                },
                {
                    "cpuid-register": "EDX",
                    "cpuid-input-eax": 2147483658,
                    "features": 0
                },
                {
                    "cpuid-register": "EAX",
                    "cpuid-input-eax": 1073741825,
                    "features": 16777467
                },
                {
                    "cpuid-register": "EDX",
                    "cpuid-input-eax": 3221225473,
                    "features": 0
                },
                {
                    "cpuid-register": "EDX",
                    "cpuid-input-eax": 2147483655,
                    "features": 0
                },
                {
                    "cpuid-register": "ECX",
                    "cpuid-input-eax": 2147483649,
                    "features": 33
                },
                {
                    "cpuid-register": "EDX",
                    "cpuid-input-eax": 2147483649,
                    "features": 739248128
                },
                {
                    "cpuid-register": "ECX",
                    "cpuid-input-eax": 7,
                    "features": 0,
                    "cpuid-input-ecx": 0
                },
                {
                    "cpuid-register": "EBX",
                    "cpuid-input-eax": 7,
                    "features": 1963,
                    "cpuid-input-ecx": 0
                },
                {
                    "cpuid-register": "ECX",
                    "cpuid-input-eax": 1,
                    "features": 4160369155
                },
                {
                    "cpuid-register": "EDX",
                    "cpuid-input-eax": 1,
                    "features": 260832255
                }
            ],
            "sse4a": false,
            "i64": true,
            "xsave": true,
            "hv-runtime": false,
            "pmm": false,
            "hle": false,
            "hv-crash": false,
            "est": false,
            "xop": false,
            "smx": false,
            "tsc-scale": false,
            "monitor": false,
            "avx512er": false,
            "apic": true,
            "sse4.1": true,
            "sse4.2": true,
            "hv-vapic": false,
            "pause-filter": false,
            "lahf-lm": true,
            "kvm-nopiodelay": true,
            "acpi": false,
            "mmx": true,
            "osxsave": false,
            "pcommit": false,
            "clwb": false,
            "dca": false,
            "pdcm": false,
            "xcrypt-en": false,
            "3dnow": false,
            "invtsc": false,
            "tm2": false,
            "hv-time": false,
            "hypervisor": true,
            "kvmclock-stable-bit": true,
            "xlevel": 2147483656,
            "fxsr-opt": false,
            "pcid": true,
            "lbrv": false,
            "wdt": false,
            "svm-lock": false,
            "popcnt": true,
            "nrip-save": false,
            "x2apic": true,
            "kvmclock": true,
            "smap": false,
            "pdpe1gb": true,
            "family": 6,
            "xlevel2": 0,
            "dtes64": false,
            "xd": true,
            "ace2": false,
            "xtpr": false,
            "lwp": false,
            "sep": true,
            "msr": true,
            "syscall": true,
            "decodeassists": false,
            "perfctr-core": false,
            "pge": true,
            "pn": false,
            "fma": true,
            "nodeid-msr": false,
            "hv-vpindex": false,
            "cx8": true,
            "mce": true,
            "avx512cd": false,
            "cr8legacy": false,
            "mca": true,
            "pni": true,
            "hv-vendor-id": "",
            "rdseed": false,
            "osvw": false,
            "fsgsbase": true,
            "model-id": "Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz",
            "cmp-legacy": false,
            "kvm-pv-unhalt": true,
            "rdtscp": true,
            "mmxext": false,
            "cid": false,
            "type": "host-x86_64-cpu",
            "vmx": false,
            "ssse3": true,
            "extapic": false,
            "pse36": true,
            "mtrr": true,
            "ibs": false,
            "avx": true,
            "ace2-en": false,
            "invpcid": true,
            "bmi2": true,
            "vmcb-clean": false,
            "erms": true,
            "cmov": true,
            "check": true,
            "parent_bus": "",
            "clflushopt": false,
            "pat": true,
            "hv-synic": false,
            "hv-stimer": false,
            "3dnowprefetch": false,
            "fpu": true,
            "pae": true,
            "hv-reset": false,
            "skinit": false,
            "kvm": true,
            "pmm-en": false,
            "phe": false,
            "3dnowext": false,
            "ht": false,
            "tsc-frequency": 0,
            "kvm-pv-eoi": true,
            "npt": false,
            "apic-id": -1,
            "xsavec": false,
            "lm": true,
            "pclmulqdq": true,
            "svm": false,
            "sse3": true,
            "sse2": true,
            "ss": true,
            "topoext": false,
            "rdrand": true,
            "bmi1": true,
            "kvm-asyncpf": true,
            "xsaves": false,
            "model": 69
        }
    }

Eduardo Habkost (3):
  qmp: Add query-host-cpu command
  target-i386: Introduce x86_cpu_load_host_data() function
  target-i386: Implement arch_query_host_cpu_info()

 include/sysemu/arch_init.h       |  1 +
 qapi-schema.json                 | 36 +++++++++++++++++
 qmp-commands.hx                  |  6 +++
 qmp.c                            | 13 ++++++
 stubs/Makefile.objs              |  1 +
 stubs/arch-query-host-cpu-info.c |  8 ++++
 target-i386/cpu.c                | 87 ++++++++++++++++++++++++++++++++++++----
 7 files changed, 145 insertions(+), 7 deletions(-)
 create mode 100644 stubs/arch-query-host-cpu-info.c

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] qmp: Add query-host-cpu command

[Eduardo Habkost]

The command can be used to return host-specific CPU capabilities
information.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/sysemu/arch_init.h       |  1 +
 qapi-schema.json                 | 36 ++++++++++++++++++++++++++++++++++++
 qmp-commands.hx                  |  6 ++++++
 qmp.c                            | 13 +++++++++++++
 stubs/Makefile.objs              |  1 +
 stubs/arch-query-host-cpu-info.c |  8 ++++++++
 6 files changed, 65 insertions(+)
 create mode 100644 stubs/arch-query-host-cpu-info.c

diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index d690dfa..54215ab 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -35,5 +35,6 @@ int kvm_available(void);
 int xen_available(void);
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
+void arch_query_host_cpu_info(HostCPUInfo *r, bool migratable, Error **errp);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 19e3ef2..d2f4879 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3047,6 +3047,42 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
 
+
+##
+# @HostCPUInfo:
+#
+# Information on CPU capabilities supported by the current host.
+#
+# @qom-properties: #optional Values of CPU QOM properties corresponding
+#                  to CPU capabilities supported by the host.
+#
+# Most properties returned in qom-properties are boolean properties
+# indicating if a feature can be enabled in the current host. Other
+# non-boolean properties may be returned, the semantics of each property
+# depend on the architecture-specific code that handle them.
+#
+# Since: 2.7.0
+##
+{ 'struct': 'HostCPUInfo',
+  'data': { '*qom-properties': 'any' } }
+
+##
+# @query-host-cpu:
+#
+# @migratable: #optional If false, unmigratable features will be
+#              returned as well. If true, only migratable features
+#              will be returned. Defaults to true.
+#
+# Return information about CPU capabilities in the current host.
+# The returned data may depend on machine and accelerator configuration.
+#
+# Returns: A HostCPUInfo object.
+#
+# Since: 2.7.0
+##
+{ 'command': 'query-host-cpu', 'data': { '*migratable': 'bool' },
+  'returns': 'HostCPUInfo' }
+
 # @AddfdInfo:
 #
 # Information about a file descriptor that was added to an fd set.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b444c20..d4c2ccd 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3930,6 +3930,12 @@ EQMP
     },
 
     {
+        .name       = "query-host-cpu",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_query_host_cpu,
+    },
+
+    {
         .name       = "query-target",
         .args_type  = "",
         .mhandler.cmd_new = qmp_marshal_query_target,
diff --git a/qmp.c b/qmp.c
index 7df6543..aec24d5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -607,6 +607,19 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
     return arch_query_cpu_definitions(errp);
 }
 
+HostCPUInfo *qmp_query_host_cpu(bool has_migratable, bool migratable,
+                                Error **errp)
+{
+    HostCPUInfo *r = g_new0(HostCPUInfo, 1);
+
+    if (!has_migratable) {
+        migratable = true;
+    }
+
+    arch_query_host_cpu_info(r, migratable, errp);
+    return r;
+}
+
 void qmp_add_client(const char *protocol, const char *fdname,
                     bool has_skipauth, bool skipauth, bool has_tls, bool tls,
                     Error **errp)
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 4b258a6..eae0e89 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,4 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
+stub-obj-y += arch-query-host-cpu-info.o
 stub-obj-y += bdrv-next-monitor-owned.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
diff --git a/stubs/arch-query-host-cpu-info.c b/stubs/arch-query-host-cpu-info.c
new file mode 100644
index 0000000..b0c455c
--- /dev/null
+++ b/stubs/arch-query-host-cpu-info.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "sysemu/arch_init.h"
+#include "qapi/qmp/qerror.h"
+
+void arch_query_host_cpu_info(HostCPUInfo *r, bool migratable, Error **errp)
+{
+}
-- 
2.5.5

[Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

[Eduardo Habkost]

The code that loads host-specific information inside
x86_cpu_realizefn() will be reused by the implementation of
query-host-cpu, so move it to a separate function.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index aadd0b9..3d3635d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char *prop, const char *value)
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only);
 
+/* Load host-dependent CPU information, when applicable */
+static void x86_cpu_load_host_data(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+    FeatureWord w;
+
+    if (cpu->host_features) {
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            env->features[w] =
+                x86_cpu_get_supported_feature_word(w, cpu->migratable);
+        }
+    }
+}
+
 #ifdef CONFIG_KVM
 
 static int cpu_x86_fill_model_id(char *str)
@@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
+    x86_cpu_load_host_data(cpu);
+
     /*TODO: cpu->host_features incorrectly overwrites features
      * set using "feat=on|off". Once we fix this, we can convert
      * plus_features & minus_features to global properties
      * inside x86_cpu_parse_featurestr() too.
      */
-    if (cpu->host_features) {
-        for (w = 0; w < FEATURE_WORDS; w++) {
-            env->features[w] =
-                x86_cpu_get_supported_feature_word(w, cpu->migratable);
-        }
-    }
-
     for (w = 0; w < FEATURE_WORDS; w++) {
         cpu->env.features[w] |= plus_features[w];
         cpu->env.features[w] &= ~minus_features[w];
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] target-i386: Implement arch_query_host_cpu_info()

[Eduardo Habkost]

Return information on the host CPU using the "host" CPU model.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3d3635d..06b0b99 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -30,6 +30,7 @@
 #include "qemu/config-file.h"
 #include "qapi/qmp/qerror.h"
 
+#include "qom/qom-qobject.h"
 #include "qapi-types.h"
 #include "qapi-visit.h"
 #include "qapi/visitor.h"
@@ -2222,6 +2223,69 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
+/* Return host CPU info by instantiating a "host" CPU object,
+ * and returning all the default values for QOM properties
+ */
+void arch_query_host_cpu_info(HostCPUInfo *r, bool migratable, Error **errp)
+{
+    Object *obj = NULL;
+    ObjectPropertyIterator iter;
+    ObjectProperty *prop;
+    QDict *propdict = NULL;
+    Error *err = NULL;
+
+    /* Host CPU information is returned only in KVM mode, by now */
+    if (!kvm_enabled()) {
+        goto out;
+    }
+
+    obj = object_new(X86_CPU_TYPE_NAME("host"));
+
+    object_property_set_bool(obj, migratable, "migratable", &err);
+    if (err) {
+        goto out;
+    }
+
+    x86_cpu_load_host_data(X86_CPU(obj));
+
+    propdict = qdict_new();
+
+    object_property_iter_init(&iter, obj);
+    while ((prop = object_property_iter_next(&iter))) {
+        QObject *v;
+
+        /* Skip properties that aren't useful for the query because
+         * they don't make sense here:
+         * - "realized" doesn't make sense because we never realize
+         *   the CPU object above.
+         * - "filtered-features" doesn't make sense because we
+         *   never filter the feature list (as it is done inside
+         *   realizefn).
+         */
+        if (!strcmp(prop->name, "realized") ||
+            !strcmp(prop->name, "filtered-features")) {
+            continue;
+        }
+
+        v = object_property_get_qobject(obj, prop->name, &err);
+        if (err) {
+            goto out;
+        }
+        qdict_put_obj(propdict, prop->name, v);
+    }
+
+    r->has_qom_properties = true;
+    r->qom_properties = QOBJECT(propdict);
+
+out:
+    object_unref(obj);
+    if (err) {
+        qobject_decref(QOBJECT(propdict));
+        error_propagate(errp, err);
+    }
+    return;
+}
+
 static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only)
 {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> Add QMP command to allow management software to query for
> CPU information for the running host.
> 
> The data returned by the command is in the form of a dictionary
> of QOM properties.
> 
> This series depends on the "Add runnability info to
> query-cpu-definitions" series I sent 2 weeks ago.
> 
> Git tree:
>   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> 

I like that interface, I'm going to post (maybe today? :) ) a similar interface
that allows to also expand other cpu models, not just the host model.

Maybe we can then decide which one makes sense for all of us. But in general,
this interface is much better compared to what we had before. 

David

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[Eduardo Habkost]

On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:
> > Add QMP command to allow management software to query for
> > CPU information for the running host.
> > 
> > The data returned by the command is in the form of a dictionary
> > of QOM properties.
> > 
> > This series depends on the "Add runnability info to
> > query-cpu-definitions" series I sent 2 weeks ago.
> > 
> > Git tree:
> >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > 
> 
> I like that interface, I'm going to post (maybe today? :) ) a similar interface
> that allows to also expand other cpu models, not just the host model.

In x86 I want to avoid exposing the details of other CPU models
to libvirt because the details depend on machine-type.

But if it is useful for you, I believe the same "qom-properties"
dict could be returned in query-cpu-definitions.

> 
> Maybe we can then decide which one makes sense for all of us. But in general,
> this interface is much better compared to what we had before. 

Maybe both? I think it's better to have a separate interface for
querying "what exactly this host supports" and another one for
querying for "what happens if I use -cpu host". In the case of
x86, both are equivalent, but we can't guarantee this on all
architectures.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:
> > > Add QMP command to allow management software to query for
> > > CPU information for the running host.
> > > 
> > > The data returned by the command is in the form of a dictionary
> > > of QOM properties.
> > > 
> > > This series depends on the "Add runnability info to
> > > query-cpu-definitions" series I sent 2 weeks ago.
> > > 
> > > Git tree:
> > >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > >   
> > 
> > I like that interface, I'm going to post (maybe today? :) ) a similar interface
> > that allows to also expand other cpu models, not just the host model.  
> 
> In x86 I want to avoid exposing the details of other CPU models
> to libvirt because the details depend on machine-type.
> 
> But if it is useful for you, I believe the same "qom-properties"
> dict could be returned in query-cpu-definitions.
> 
> > 
> > Maybe we can then decide which one makes sense for all of us. But in general,
> > this interface is much better compared to what we had before.   
> 
> Maybe both? I think it's better to have a separate interface for
> querying "what exactly this host supports" and another one for
> querying for "what happens if I use -cpu host". In the case of
> x86, both are equivalent, but we can't guarantee this on all
> architectures.
> 

I'll post my patches in a couple of minutes, let's discuss it then.

We might want to avoid having multiple interfaces carrying out the same task.

David

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[Eduardo Habkost]

On Tue, Jun 21, 2016 at 02:52:54PM +0200, David Hildenbrand wrote:
> > On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:
> > > > Add QMP command to allow management software to query for
> > > > CPU information for the running host.
> > > > 
> > > > The data returned by the command is in the form of a dictionary
> > > > of QOM properties.
> > > > 
> > > > This series depends on the "Add runnability info to
> > > > query-cpu-definitions" series I sent 2 weeks ago.
> > > > 
> > > > Git tree:
> > > >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > > >   
> > > 
> > > I like that interface, I'm going to post (maybe today? :) ) a similar interface
> > > that allows to also expand other cpu models, not just the host model.  
> > 
> > In x86 I want to avoid exposing the details of other CPU models
> > to libvirt because the details depend on machine-type.
> > 
> > But if it is useful for you, I believe the same "qom-properties"
> > dict could be returned in query-cpu-definitions.
> > 
> > > 
> > > Maybe we can then decide which one makes sense for all of us. But in general,
> > > this interface is much better compared to what we had before.   
> > 
> > Maybe both? I think it's better to have a separate interface for
> > querying "what exactly this host supports" and another one for
> > querying for "what happens if I use -cpu host". In the case of
> > x86, both are equivalent, but we can't guarantee this on all
> > architectures.
> > 
> 
> I'll post my patches in a couple of minutes, let's discuss it
> then.
> 
> We might want to avoid having multiple interfaces carrying out
> the same task.

OK, I will wait for the patches before discussing it. My
assumption is that both look similar, but are actually different
tasks.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/3] qmp: query-host-cpu command

[David Hildenbrand]

> On Tue, Jun 21, 2016 at 02:52:54PM +0200, David Hildenbrand wrote:
> > > On Tue, Jun 21, 2016 at 08:20:40AM +0200, David Hildenbrand wrote:  
> > > > > Add QMP command to allow management software to query for
> > > > > CPU information for the running host.
> > > > > 
> > > > > The data returned by the command is in the form of a dictionary
> > > > > of QOM properties.
> > > > > 
> > > > > This series depends on the "Add runnability info to
> > > > > query-cpu-definitions" series I sent 2 weeks ago.
> > > > > 
> > > > > Git tree:
> > > > >   https://github.com/ehabkost/qemu-hacks.git work/query-host-cpu
> > > > >     
> > > > 
> > > > I like that interface, I'm going to post (maybe today? :) ) a similar interface
> > > > that allows to also expand other cpu models, not just the host model.    
> > > 
> > > In x86 I want to avoid exposing the details of other CPU models
> > > to libvirt because the details depend on machine-type.
> > > 
> > > But if it is useful for you, I believe the same "qom-properties"
> > > dict could be returned in query-cpu-definitions.
> > >   
> > > > 
> > > > Maybe we can then decide which one makes sense for all of us. But in general,
> > > > this interface is much better compared to what we had before.     
> > > 
> > > Maybe both? I think it's better to have a separate interface for
> > > querying "what exactly this host supports" and another one for
> > > querying for "what happens if I use -cpu host". In the case of
> > > x86, both are equivalent, but we can't guarantee this on all
> > > architectures.
> > >   
> > 
> > I'll post my patches in a couple of minutes, let's discuss it
> > then.
> > 
> > We might want to avoid having multiple interfaces carrying out
> > the same task.  
> 
> OK, I will wait for the patches before discussing it. My
> assumption is that both look similar, but are actually different
> tasks.
> 

For us, also "-cpu host" and querying the host model is identical. Not sure
if there would for now really be a requirement for some other architecture
to handle this differently. Do you have anything specific already in mind?

But let's see if it indeed makes sense to split this up after looking at
our interface proposal.

David

Re: [Qemu-devel] [PATCH 1/3] qmp: Add query-host-cpu command

[Eric Blake]

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

On 06/20/2016 02:12 PM, Eduardo Habkost wrote:
> The command can be used to return host-specific CPU capabilities
> information.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  include/sysemu/arch_init.h       |  1 +
>  qapi-schema.json                 | 36 ++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx                  |  6 ++++++
>  qmp.c                            | 13 +++++++++++++
>  stubs/Makefile.objs              |  1 +
>  stubs/arch-query-host-cpu-info.c |  8 ++++++++
>  6 files changed, 65 insertions(+)
>  create mode 100644 stubs/arch-query-host-cpu-info.c
> 
> diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
> index d690dfa..54215ab 100644
> --- a/include/sysemu/arch_init.h
> +++ b/include/sysemu/arch_init.h
> @@ -35,5 +35,6 @@ int kvm_available(void);
>  int xen_available(void);
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
> +void arch_query_host_cpu_info(HostCPUInfo *r, bool migratable, Error **errp);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 19e3ef2..d2f4879 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3047,6 +3047,42 @@
>  ##
>  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
>  
> +
> +##
> +# @HostCPUInfo:
> +#
> +# Information on CPU capabilities supported by the current host.
> +#
> +# @qom-properties: #optional Values of CPU QOM properties corresponding
> +#                  to CPU capabilities supported by the host.
> +#
> +# Most properties returned in qom-properties are boolean properties
> +# indicating if a feature can be enabled in the current host. Other
> +# non-boolean properties may be returned, the semantics of each property
> +# depend on the architecture-specific code that handle them.
> +#
> +# Since: 2.7.0

Most places in .json files list just 'Since: x.y' rather than 'x.y.z',
but we aren't consistent enough to insist either way on including or
excluding a micro release number.

> +##
> +{ 'struct': 'HostCPUInfo',
> +  'data': { '*qom-properties': 'any' } }

This is a big hammer that makes the properties non-introspectible - a
client can tell that properties will be returned, but cannot tell which
properties to expect nor what format to expect for a given property
name.  I don't know that the interface could be made easily
introspectible or not (it would probably require some QAPI unions, and a
LOT more generated code).  So it would be nice if we could explore how
hard it would be to use a type-safe representation instead of 'any',
before declaring that this is the best we can do.  Or, it may be the
sign of a bigger issue that we have no good way to introspect what qom
properties to expect, in general (and that solving that would also solve
this).

> +
> +##
> +# @query-host-cpu:
> +#
> +# @migratable: #optional If false, unmigratable features will be
> +#              returned as well. If true, only migratable features
> +#              will be returned. Defaults to true.
> +#
> +# Return information about CPU capabilities in the current host.
> +# The returned data may depend on machine and accelerator configuration.
> +#
> +# Returns: A HostCPUInfo object.
> +#
> +# Since: 2.7.0
> +##
> +{ 'command': 'query-host-cpu', 'data': { '*migratable': 'bool' },
> +  'returns': 'HostCPUInfo' }
> +
>  # @AddfdInfo:

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


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

Re: [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

[Igor Mammedov]

On Mon, 20 Jun 2016 17:12:43 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The code that loads host-specific information inside
> x86_cpu_realizefn() will be reused by the implementation of
> query-host-cpu, so move it to a separate function.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index aadd0b9..3d3635d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char
> *prop, const char *value) static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w, bool
> migratable_only); 
> +/* Load host-dependent CPU information, when applicable */
> +static void x86_cpu_load_host_data(X86CPU *cpu)
> +{
> +    CPUX86State *env = &cpu->env;
> +    FeatureWord w;
> +
> +    if (cpu->host_features) {
> +        for (w = 0; w < FEATURE_WORDS; w++) {
> +            env->features[w] =
> +                x86_cpu_get_supported_feature_word(w,
> cpu->migratable);
> +        }
> +    }
> +}
> +
>  #ifdef CONFIG_KVM
>  
>  static int cpu_x86_fill_model_id(char *str)
> @@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState
> *dev, Error **errp) return;
>      }
>  
> +    x86_cpu_load_host_data(cpu);
this function should be below TODO comment as it applies to moved
code.

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

> +
>      /*TODO: cpu->host_features incorrectly overwrites features
>       * set using "feat=on|off". Once we fix this, we can convert
>       * plus_features & minus_features to global properties
>       * inside x86_cpu_parse_featurestr() too.
>       */
> -    if (cpu->host_features) {
> -        for (w = 0; w < FEATURE_WORDS; w++) {
> -            env->features[w] =
> -                x86_cpu_get_supported_feature_word(w,
> cpu->migratable);
> -        }
> -    }
> -
>      for (w = 0; w < FEATURE_WORDS; w++) {
>          cpu->env.features[w] |= plus_features[w];
>          cpu->env.features[w] &= ~minus_features[w];

Re: [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 04:59:28PM +0200, Igor Mammedov wrote:
> On Mon, 20 Jun 2016 17:12:43 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > The code that loads host-specific information inside
> > x86_cpu_realizefn() will be reused by the implementation of
> > query-host-cpu, so move it to a separate function.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index aadd0b9..3d3635d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char
> > *prop, const char *value) static uint32_t
> > x86_cpu_get_supported_feature_word(FeatureWord w, bool
> > migratable_only); 
> > +/* Load host-dependent CPU information, when applicable */
> > +static void x86_cpu_load_host_data(X86CPU *cpu)
> > +{
> > +    CPUX86State *env = &cpu->env;
> > +    FeatureWord w;
> > +
> > +    if (cpu->host_features) {
> > +        for (w = 0; w < FEATURE_WORDS; w++) {
> > +            env->features[w] =
> > +                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > +        }
> > +    }
> > +}
> > +
> >  #ifdef CONFIG_KVM
> >  
> >  static int cpu_x86_fill_model_id(char *str)
> > @@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState
> > *dev, Error **errp) return;
> >      }
> >  
> > +    x86_cpu_load_host_data(cpu);
> this function should be below TODO comment as it applies to moved
> code.

It was on purpose. The comment is actually about the
plus_features/minus_features code, that is the hack we want to
remove after cpu->host_features is fixed.

Placing the comment before the x86_cpu_load_host_data() call
wouldn't make sense, as the host_features code is now hidden
inside the function.

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

Considering the above explanation, do you prefer that I keep the
patch as-is, or move the comment inside x86_cpu_load_host_data()?

(I will not move it before the x86_cpu_load_host_data() call)


> 
> > +
> >      /*TODO: cpu->host_features incorrectly overwrites features
> >       * set using "feat=on|off". Once we fix this, we can convert
> >       * plus_features & minus_features to global properties
> >       * inside x86_cpu_parse_featurestr() too.
> >       */
> > -    if (cpu->host_features) {
> > -        for (w = 0; w < FEATURE_WORDS; w++) {
> > -            env->features[w] =
> > -                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > -        }
> > -    }
> > -
> >      for (w = 0; w < FEATURE_WORDS; w++) {
> >          cpu->env.features[w] |= plus_features[w];
> >          cpu->env.features[w] &= ~minus_features[w];
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] qmp: Add query-host-cpu command

[Eduardo Habkost]

On Tue, Jun 21, 2016 at 03:48:58PM -0600, Eric Blake wrote:
> On 06/20/2016 02:12 PM, Eduardo Habkost wrote:
> > The command can be used to return host-specific CPU capabilities
> > information.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  include/sysemu/arch_init.h       |  1 +
> >  qapi-schema.json                 | 36 ++++++++++++++++++++++++++++++++++++
> >  qmp-commands.hx                  |  6 ++++++
> >  qmp.c                            | 13 +++++++++++++
> >  stubs/Makefile.objs              |  1 +
> >  stubs/arch-query-host-cpu-info.c |  8 ++++++++
> >  6 files changed, 65 insertions(+)
> >  create mode 100644 stubs/arch-query-host-cpu-info.c
> > 
> > diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
> > index d690dfa..54215ab 100644
> > --- a/include/sysemu/arch_init.h
> > +++ b/include/sysemu/arch_init.h
> > @@ -35,5 +35,6 @@ int kvm_available(void);
> >  int xen_available(void);
> >  
> >  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp);
> > +void arch_query_host_cpu_info(HostCPUInfo *r, bool migratable, Error **errp);
> >  
> >  #endif
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 19e3ef2..d2f4879 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3047,6 +3047,42 @@
> >  ##
> >  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
> >  
> > +
> > +##
> > +# @HostCPUInfo:
> > +#
> > +# Information on CPU capabilities supported by the current host.
> > +#
> > +# @qom-properties: #optional Values of CPU QOM properties corresponding
> > +#                  to CPU capabilities supported by the host.
> > +#
> > +# Most properties returned in qom-properties are boolean properties
> > +# indicating if a feature can be enabled in the current host. Other
> > +# non-boolean properties may be returned, the semantics of each property
> > +# depend on the architecture-specific code that handle them.
> > +#
> > +# Since: 2.7.0
> 
> Most places in .json files list just 'Since: x.y' rather than 'x.y.z',
> but we aren't consistent enough to insist either way on including or
> excluding a micro release number.
> 
> > +##
> > +{ 'struct': 'HostCPUInfo',
> > +  'data': { '*qom-properties': 'any' } }
> 
> This is a big hammer that makes the properties non-introspectible - a
> client can tell that properties will be returned, but cannot tell which
> properties to expect nor what format to expect for a given property
> name.  I don't know that the interface could be made easily
> introspectible or not (it would probably require some QAPI unions, and a
> LOT more generated code).  So it would be nice if we could explore how
> hard it would be to use a type-safe representation instead of 'any',
> before declaring that this is the best we can do.  Or, it may be the
> sign of a bigger issue that we have no good way to introspect what qom
> properties to expect, in general (and that solving that would also solve
> this).

What I thought libvirt needed is different from what I though, so
this series will be dropped by now (see the "s390x CPU models:
exposing features" thread). But your comments may still apply
when we look at the alternative "query-cpu-model-expansion"
proposal.

I believe QOM introspection is really the issue here. The CPU
configuration is already based on QOM properties. Manually
duplicating the existing QOM properties into the QAPI
representation would be a waste of time, IMO.

But I agree that the interface could be improved: we should
document very clearly what can be done with the QOM property list
being returned, and return only useful data. For example: we
could only return properties that really makes sense when used
with "-cpu" or "-global" (not every single QOM property), and
document that.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

[Igor Mammedov]

On Thu, 23 Jun 2016 13:04:53 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 23, 2016 at 04:59:28PM +0200, Igor Mammedov wrote:
> > On Mon, 20 Jun 2016 17:12:43 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > The code that loads host-specific information inside
> > > x86_cpu_realizefn() will be reused by the implementation of
> > > query-host-cpu, so move it to a separate function.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 23 ++++++++++++++++-------
> > >  1 file changed, 16 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index aadd0b9..3d3635d 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char
> > > *prop, const char *value) static uint32_t
> > > x86_cpu_get_supported_feature_word(FeatureWord w, bool
> > > migratable_only); 
> > > +/* Load host-dependent CPU information, when applicable */
> > > +static void x86_cpu_load_host_data(X86CPU *cpu)
> > > +{
> > > +    CPUX86State *env = &cpu->env;
> > > +    FeatureWord w;
> > > +
> > > +    if (cpu->host_features) {
> > > +        for (w = 0; w < FEATURE_WORDS; w++) {
> > > +            env->features[w] =
> > > +                x86_cpu_get_supported_feature_word(w,
> > > cpu->migratable);
> > > +        }
> > > +    }
> > > +}
> > > +
> > >  #ifdef CONFIG_KVM
> > >  
> > >  static int cpu_x86_fill_model_id(char *str)
> > > @@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState
> > > *dev, Error **errp) return;
> > >      }
> > >  
> > > +    x86_cpu_load_host_data(cpu);
> > this function should be below TODO comment as it applies to moved
> > code.
> 
> It was on purpose. The comment is actually about the
> plus_features/minus_features code, that is the hack we want to
> remove after cpu->host_features is fixed.
> 
> Placing the comment before the x86_cpu_load_host_data() call
> wouldn't make sense, as the host_features code is now hidden
> inside the function.
> 
> > 
> > with this fixed
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> Considering the above explanation, do you prefer that I keep the
> patch as-is, or move the comment inside x86_cpu_load_host_data()?
I prefer comment inside call as it is related to bug introduced by
moving

    env->features[w] = x86_cpu_get_supported_feature_word(w, cpu->migratable);

into x86_cpu_parse_featurestr() for initfn().

plus_features/minus_features code in realize are side affect of above
otherwise they could be converted at x86_cpu_parse_featurestr() time.

> 
> (I will not move it before the x86_cpu_load_host_data() call)
> 
> 
> > 
> > > +
> > >      /*TODO: cpu->host_features incorrectly overwrites features
> > >       * set using "feat=on|off". Once we fix this, we can convert
> > >       * plus_features & minus_features to global properties
> > >       * inside x86_cpu_parse_featurestr() too.
> > >       */
> > > -    if (cpu->host_features) {
> > > -        for (w = 0; w < FEATURE_WORDS; w++) {
> > > -            env->features[w] =
> > > -                x86_cpu_get_supported_feature_word(w,
> > > cpu->migratable);
> > > -        }
> > > -    }
> > > -
> > >      for (w = 0; w < FEATURE_WORDS; w++) {
> > >          cpu->env.features[w] |= plus_features[w];
> > >          cpu->env.features[w] &= ~minus_features[w];
> > 
> 

Re: [Qemu-devel] [PATCH 2/3] target-i386: Introduce x86_cpu_load_host_data() function

[Eduardo Habkost]

On Thu, Jun 23, 2016 at 06:56:12PM +0200, Igor Mammedov wrote:
> On Thu, 23 Jun 2016 13:04:53 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 23, 2016 at 04:59:28PM +0200, Igor Mammedov wrote:
> > > On Mon, 20 Jun 2016 17:12:43 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > The code that loads host-specific information inside
> > > > x86_cpu_realizefn() will be reused by the implementation of
> > > > query-host-cpu, so move it to a separate function.
> > > > 
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > >  target-i386/cpu.c | 23 ++++++++++++++++-------
> > > >  1 file changed, 16 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index aadd0b9..3d3635d 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1491,6 +1491,20 @@ void x86_cpu_change_kvm_default(const char
> > > > *prop, const char *value) static uint32_t
> > > > x86_cpu_get_supported_feature_word(FeatureWord w, bool
> > > > migratable_only); 
> > > > +/* Load host-dependent CPU information, when applicable */
> > > > +static void x86_cpu_load_host_data(X86CPU *cpu)
> > > > +{
> > > > +    CPUX86State *env = &cpu->env;
> > > > +    FeatureWord w;
> > > > +
> > > > +    if (cpu->host_features) {
> > > > +        for (w = 0; w < FEATURE_WORDS; w++) {
> > > > +            env->features[w] =
> > > > +                x86_cpu_get_supported_feature_word(w,
> > > > cpu->migratable);
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > >  #ifdef CONFIG_KVM
> > > >  
> > > >  static int cpu_x86_fill_model_id(char *str)
> > > > @@ -3012,18 +3026,13 @@ static void x86_cpu_realizefn(DeviceState
> > > > *dev, Error **errp) return;
> > > >      }
> > > >  
> > > > +    x86_cpu_load_host_data(cpu);
> > > this function should be below TODO comment as it applies to moved
> > > code.
> > 
> > It was on purpose. The comment is actually about the
> > plus_features/minus_features code, that is the hack we want to
> > remove after cpu->host_features is fixed.
> > 
> > Placing the comment before the x86_cpu_load_host_data() call
> > wouldn't make sense, as the host_features code is now hidden
> > inside the function.
> > 
> > > 
> > > with this fixed
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Considering the above explanation, do you prefer that I keep the
> > patch as-is, or move the comment inside x86_cpu_load_host_data()?
> I prefer comment inside call as it is related to bug introduced by
> moving
> 
>     env->features[w] = x86_cpu_get_supported_feature_word(w, cpu->migratable);
> 
> into x86_cpu_parse_featurestr() for initfn().
> 
> plus_features/minus_features code in realize are side affect of above
> otherwise they could be converted at x86_cpu_parse_featurestr() time.

OK, I will move it inside x86_cpu_load_host_data().

-- 
Eduardo