[Qemu-devel] [PATCH 0/9] Add runnability info to query-cpu-definitions

[Qemu-devel] [PATCH 0/9] Add runnability info to query-cpu-definitions

[Eduardo Habkost]

This series extends query-cpu-definitions to include two extra
fields: "runnable", and "unavailable-features".

This will return information based on the current machine and
accelerator only. In the future we may extend these mechanisms to
allow querying other machines and other accelerators without
restarting QEMU, but it will require some reorganization of
QEMU's main code.

This series is based on my 'x86-next' branch, at:
  git://github.com/ehabkost/qemu.git x86-next

Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: libvir-list@redhat.com

Eduardo Habkost (9):
  target-i386: Move TCG initialization check to tcg_x86_init()
  target-i386: Move TCG initialization to realize time
  target-i386: Call cpu_exec_init() on realize
  target-i386: List CPU models using subclass list
  target-i386: Move warning code outside x86_cpu_filter_features()
  target-i386: Define CPUID filtering functions before x86_cpu_list()
  qmp: Add runnability information to query-cpu-definitions
  target-i386: Use "-" instead of "_" on all feature names
  target-i386: Return runnability information on query-cpu-definitions

 qapi-schema.json        |  10 +-
 target-i386/cpu-qom.h   |   4 +
 target-i386/cpu.c       | 275 +++++++++++++++++++++++++++++++++---------------
 target-i386/translate.c |   6 ++
 4 files changed, 207 insertions(+), 88 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/9] target-i386: Move TCG initialization check to tcg_x86_init()

[Eduardo Habkost]

Instead of requiring cpu.c to check if TCG was already initialized,
simply let the function be called multiple times.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c       | 4 +---
 target-i386/translate.c | 6 ++++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 4856cd4..a689fec 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3087,7 +3087,6 @@ static void x86_cpu_initfn(Object *obj)
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
     FeatureWord w;
-    static int inited;
 
     cs->env_ptr = env;
     cpu_exec_init(cs, &error_abort);
@@ -3138,8 +3137,7 @@ static void x86_cpu_initfn(Object *obj)
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
     /* init various static tables used in TCG mode */
-    if (tcg_enabled() && !inited) {
-        inited = 1;
+    if (tcg_enabled()) {
         tcg_x86_init();
     }
 }
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 1a1214d..92570b4 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -8133,6 +8133,12 @@ void tcg_x86_init(void)
         "bnd0_ub", "bnd1_ub", "bnd2_ub", "bnd3_ub"
     };
     int i;
+    static bool initialized = false;
+
+    if (initialized) {
+        return;
+    }
+    initialized = true;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
     cpu_cc_op = tcg_global_mem_new_i32(cpu_env,
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/9] target-i386: Move TCG initialization check to tcg_x86_init()

[Igor Mammedov]

On Fri,  6 May 2016 15:11:24 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of requiring cpu.c to check if TCG was already initialized,
> simply let the function be called multiple times.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target-i386/cpu.c       | 4 +---
>  target-i386/translate.c | 6 ++++++
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 4856cd4..a689fec 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -3087,7 +3087,6 @@ static void x86_cpu_initfn(Object *obj)
>      X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>      CPUX86State *env = &cpu->env;
>      FeatureWord w;
> -    static int inited;
>  
>      cs->env_ptr = env;
>      cpu_exec_init(cs, &error_abort);
> @@ -3138,8 +3137,7 @@ static void x86_cpu_initfn(Object *obj)
>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
>  
>      /* init various static tables used in TCG mode */
> -    if (tcg_enabled() && !inited) {
> -        inited = 1;
> +    if (tcg_enabled()) {
>          tcg_x86_init();
>      }
>  }
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 1a1214d..92570b4 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -8133,6 +8133,12 @@ void tcg_x86_init(void)
>          "bnd0_ub", "bnd1_ub", "bnd2_ub", "bnd3_ub"
>      };
>      int i;
> +    static bool initialized = false;
> +
> +    if (initialized) {
> +        return;
> +    }
> +    initialized = true;
>  
>      cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
>      cpu_cc_op = tcg_global_mem_new_i32(cpu_env,

[Qemu-devel] [PATCH 2/9] target-i386: Move TCG initialization to realize time

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

Move TCG initialization to realize time so it won't be called when just
doing object_new() on a X86CPU subclass.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 * Now the inited/tcg_initialized variable doesn't exist anymore
 * Move tcg_x86_init() call after basic parameter validation inside
   realizefn
---
 target-i386/cpu.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a689fec..bde649a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,6 +2901,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 
+    if (tcg_enabled()) {
+        tcg_x86_init();
+    }
+
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
@@ -3135,11 +3139,6 @@ static void x86_cpu_initfn(Object *obj)
     }
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
-
-    /* init various static tables used in TCG mode */
-    if (tcg_enabled()) {
-        tcg_x86_init();
-    }
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.5.5

Re: [Qemu-devel] [PATCH 2/9] target-i386: Move TCG initialization to realize time

[Igor Mammedov]

On Fri,  6 May 2016 15:11:25 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> QOM instance_init functions are not supposed to have any side-effects,
> as new objects may be created at any moment for querying property
> information (see qmp_device_list_properties()).
> 
> Move TCG initialization to realize time so it won't be called when just
> doing object_new() on a X86CPU subclass.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes v1 -> v2:
>  * Now the inited/tcg_initialized variable doesn't exist anymore
>  * Move tcg_x86_init() call after basic parameter validation inside
>    realizefn
> ---
>  target-i386/cpu.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a689fec..bde649a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2901,6 +2901,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>  
> +    if (tcg_enabled()) {
> +        tcg_x86_init();
> +    }
> +
>  #ifndef CONFIG_USER_ONLY
>      qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
>  
> @@ -3135,11 +3139,6 @@ static void x86_cpu_initfn(Object *obj)
>      }
>  
>      x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
> -
> -    /* init various static tables used in TCG mode */
> -    if (tcg_enabled()) {
> -        tcg_x86_init();
> -    }
>  }
>  
>  static int64_t x86_cpu_get_arch_id(CPUState *cs)

[Qemu-devel] [PATCH 3/9] target-i386: Call cpu_exec_init() on realize

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

Calling cpu_exec_init() also affects QEMU's ability to handle errors
during CPU creation, as some actions done by cpu_exec_init() can't be
reverted.

Move cpu_exec_init() call to realize so a simple object_new() won't
trigger it, and so that it is called after some basic validation of CPU
parameters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 * Call cpu_exec_init() after basic parameter validation
---
 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 bde649a..26a5a6b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2901,6 +2901,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
 
+    cpu_exec_init(cs, &error_abort);
+
     if (tcg_enabled()) {
         tcg_x86_init();
     }
@@ -3093,7 +3095,6 @@ static void x86_cpu_initfn(Object *obj)
     FeatureWord w;
 
     cs->env_ptr = env;
-    cpu_exec_init(cs, &error_abort);
 
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
-- 
2.5.5

Re: [Qemu-devel] [PATCH 3/9] target-i386: Call cpu_exec_init() on realize

[Igor Mammedov]

On Fri,  6 May 2016 15:11:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> QOM instance_init functions are not supposed to have any side-effects,
> as new objects may be created at any moment for querying property
> information (see qmp_device_list_properties()).
> 
> Calling cpu_exec_init() also affects QEMU's ability to handle errors
> during CPU creation, as some actions done by cpu_exec_init() can't be
> reverted.
> 
> Move cpu_exec_init() call to realize so a simple object_new() won't
> trigger it, and so that it is called after some basic validation of CPU
> parameters.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes v1 -> v2:
>  * Call cpu_exec_init() after basic parameter validation
> ---
>  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 bde649a..26a5a6b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2901,6 +2901,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      }
>  
>  
> +    cpu_exec_init(cs, &error_abort);
> +
>      if (tcg_enabled()) {
>          tcg_x86_init();
>      }
> @@ -3093,7 +3095,6 @@ static void x86_cpu_initfn(Object *obj)
>      FeatureWord w;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(cs, &error_abort);
>  
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,

[Qemu-devel] [PATCH 4/9] target-i386: List CPU models using subclass list

[Eduardo Habkost]

Instead of using the builtin_x86_defs array, use the QOM subclass list
to list CPU models on "-cpu ?" and "query-cpu-definitions".

Signed-off-by: Andreas Färber <afaerber@suse.de>
[ehabkost: copied code from a patch by Andreas:
 "target-i386: QOM'ify CPU", from March 2012]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |   4 ++
 target-i386/cpu.c     | 111 +++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 86 insertions(+), 29 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index cb75017..c23b634 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -64,6 +64,10 @@ typedef struct X86CPUClass {
 
     bool kvm_required;
 
+    /* Optional description of CPU model.
+     * If unavailable, cpu_def->model_id is used */
+    const char *model_description;
+
     DeviceRealize parent_realize;
     void (*parent_reset)(CPUState *cpu);
 } X86CPUClass;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 26a5a6b..037b602 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -676,6 +676,14 @@ static ObjectClass *x86_cpu_class_by_name(const char *cpu_model)
     return oc;
 }
 
+static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
+{
+    const char *class_name = object_class_get_name(OBJECT_CLASS(cc));
+    assert(g_str_has_suffix(class_name, X86_CPU_TYPE_SUFFIX));
+    return g_strndup(class_name,
+                     strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
+}
+
 struct X86CPUDefinition {
     const char *name;
     uint32_t level;
@@ -1484,6 +1492,9 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
     cpu_x86_fill_model_id(host_cpudef.model_id);
 
     xcc->cpu_def = &host_cpudef;
+    xcc->model_description =
+        "KVM processor with all supported host features "
+        "(only available in KVM mode)";
 
     /* level, xlevel, xlevel2, and the feature words are initialized on
      * instance_init, because they require KVM to be initialized.
@@ -2009,23 +2020,62 @@ static void listflags(FILE *f, fprintf_function print, const char **featureset)
     }
 }
 
-/* generate CPU information. */
+/* Sort alphabetically by type name, listing kvm_required models last. */
+static gint x86_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *class_a = (ObjectClass *)a;
+    ObjectClass *class_b = (ObjectClass *)b;
+    X86CPUClass *cc_a = X86_CPU_CLASS(class_a);
+    X86CPUClass *cc_b = X86_CPU_CLASS(class_b);
+    const char *name_a, *name_b;
+
+    if (cc_a->kvm_required != cc_b->kvm_required) {
+        /* kvm_required items go last */
+        return cc_a->kvm_required ? 1 : -1;
+    } else {
+        name_a = object_class_get_name(class_a);
+        name_b = object_class_get_name(class_b);
+        return strcmp(name_a, name_b);
+    }
+}
+
+static GSList *get_sorted_cpu_model_list(void)
+{
+    GSList *list = object_class_get_list(TYPE_X86_CPU, false);
+    list = g_slist_sort(list, x86_cpu_list_compare);
+    return list;
+}
+
+static void x86_cpu_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    X86CPUClass *cc = X86_CPU_CLASS(oc);
+    CPUListState *s = user_data;
+    char *name = x86_cpu_class_get_model_name(cc);
+    const char *desc = cc->model_description;
+    if (!desc) {
+        desc = cc->cpu_def->model_id;
+    }
+
+    (*s->cpu_fprintf)(s->file, "x86 %16s  %-48s\n",
+                      name, desc);
+    g_free(name);
+}
+
+/* list available CPU models and flags */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    X86CPUDefinition *def;
-    char buf[256];
     int i;
+    CPUListState s = {
+        .file = f,
+        .cpu_fprintf = cpu_fprintf,
+    };
+    GSList *list;
 
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        def = &builtin_x86_defs[i];
-        snprintf(buf, sizeof(buf), "%s", def->name);
-        (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
-    }
-#ifdef CONFIG_KVM
-    (*cpu_fprintf)(f, "x86 %16s  %-48s\n", "host",
-                   "KVM processor with all supported host features "
-                   "(only available in KVM mode)");
-#endif
+    (*cpu_fprintf)(f, "Available CPUs:\n");
+    list = get_sorted_cpu_model_list();
+    g_slist_foreach(list, x86_cpu_list_entry, &s);
+    g_slist_free(list);
 
     (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
@@ -2037,26 +2087,29 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     }
 }
 
-CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
-    CpuDefinitionInfoList *cpu_list = NULL;
-    X86CPUDefinition *def;
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        CpuDefinitionInfoList *entry;
-        CpuDefinitionInfo *info;
+    ObjectClass *oc = data;
+    X86CPUClass *cc = X86_CPU_CLASS(oc);
+    CpuDefinitionInfoList **cpu_list = user_data;
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
 
-        def = &builtin_x86_defs[i];
-        info = g_malloc0(sizeof(*info));
-        info->name = g_strdup(def->name);
+    info = g_malloc0(sizeof(*info));
+    info->name = x86_cpu_class_get_model_name(cc);
 
-        entry = g_malloc0(sizeof(*entry));
-        entry->value = info;
-        entry->next = cpu_list;
-        cpu_list = entry;
-    }
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+    entry->next = *cpu_list;
+    *cpu_list = entry;
+}
 
+CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+{
+    CpuDefinitionInfoList *cpu_list = NULL;
+    GSList *list = get_sorted_cpu_model_list();
+    g_slist_foreach(list, x86_cpu_definition_entry, &cpu_list);
+    g_slist_free(list);
     return cpu_list;
 }
 
-- 
2.5.5

[Qemu-devel] [PATCH 5/9] target-i386: Move warning code outside x86_cpu_filter_features()

[Eduardo Habkost]

x86_cpu_filter_features() will be reused by code that shouldn't
print any warning. Move the warning code to a new
x86_cpu_report_filtered_features() function, and call it from
x86_cpu_realizefn().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 037b602..465c125 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2152,9 +2152,6 @@ static int x86_cpu_filter_features(X86CPU *cpu)
         env->features[w] &= host_feat;
         cpu->filtered_features[w] = requested_features & ~env->features[w];
         if (cpu->filtered_features[w]) {
-            if (cpu->check_cpuid || cpu->enforce_cpuid) {
-                report_unavailable_features(w, cpu->filtered_features[w]);
-            }
             rv = 1;
         }
     }
@@ -2162,6 +2159,15 @@ static int x86_cpu_filter_features(X86CPU *cpu)
     return rv;
 }
 
+static void x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+    FeatureWord w;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        report_unavailable_features(w, cpu->filtered_features[w]);
+    }
+}
+
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 {
     PropValue *pv;
@@ -2936,12 +2942,16 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->cpuid_level = 7;
     }
 
-    if (x86_cpu_filter_features(cpu) && cpu->enforce_cpuid) {
-        error_setg(&local_err,
-                   kvm_enabled() ?
-                       "Host doesn't support requested features" :
-                       "TCG doesn't support requested features");
-        goto out;
+    if (x86_cpu_filter_features(cpu) &&
+        (cpu->check_cpuid || cpu->enforce_cpuid)) {
+        x86_cpu_report_filtered_features(cpu);
+        if (cpu->enforce_cpuid) {
+            error_setg(&local_err,
+                       kvm_enabled() ?
+                           "Host doesn't support requested features" :
+                           "TCG doesn't support requested features");
+            goto out;
+        }
     }
 
     /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on
-- 
2.5.5

[Qemu-devel] [PATCH 6/9] target-i386: Define CPUID filtering functions before x86_cpu_list()

[Eduardo Habkost]

Just move code to another place so the it can be reused by the
query-cpu-definitions code.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 465c125..309ef55 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2005,6 +2005,40 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     }
 }
 
+/*
+ * Filters CPU feature words based on host availability of each feature.
+ *
+ * Returns: 0 if all flags are supported by the host, non-zero otherwise.
+ */
+static int x86_cpu_filter_features(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+    FeatureWord w;
+    int rv = 0;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        uint32_t host_feat =
+            x86_cpu_get_supported_feature_word(w, cpu->migratable);
+        uint32_t requested_features = env->features[w];
+        env->features[w] &= host_feat;
+        cpu->filtered_features[w] = requested_features & ~env->features[w];
+        if (cpu->filtered_features[w]) {
+            rv = 1;
+        }
+    }
+
+    return rv;
+}
+
+static void x86_cpu_report_filtered_features(X86CPU *cpu)
+{
+    FeatureWord w;
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        report_unavailable_features(w, cpu->filtered_features[w]);
+    }
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2134,40 +2168,6 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w,
     return r;
 }
 
-/*
- * Filters CPU feature words based on host availability of each feature.
- *
- * Returns: 0 if all flags are supported by the host, non-zero otherwise.
- */
-static int x86_cpu_filter_features(X86CPU *cpu)
-{
-    CPUX86State *env = &cpu->env;
-    FeatureWord w;
-    int rv = 0;
-
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        uint32_t host_feat =
-            x86_cpu_get_supported_feature_word(w, cpu->migratable);
-        uint32_t requested_features = env->features[w];
-        env->features[w] &= host_feat;
-        cpu->filtered_features[w] = requested_features & ~env->features[w];
-        if (cpu->filtered_features[w]) {
-            rv = 1;
-        }
-    }
-
-    return rv;
-}
-
-static void x86_cpu_report_filtered_features(X86CPU *cpu)
-{
-    FeatureWord w;
-
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        report_unavailable_features(w, cpu->filtered_features[w]);
-    }
-}
-
 static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 {
     PropValue *pv;
-- 
2.5.5

[Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Extend query-cpu-definitions schema to allow it to return two new
optional fields: "runnable" and "unavailable-features".
"runnable" will tell if the CPU model can be run in the current
host. "unavailable-features" will contain a list of CPU
properties that are preventing the CPU model from running in the
current host.

Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qapi-schema.json | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 54634c4..450e6e7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2948,11 +2948,19 @@
 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
+# @runnable: true if the CPU model is runnable using the current
+#            machine and accelerator. Optional. Since 2.6.
+# @unavailable-features: List of properties that prevent the CPU
+#                        model from running in the current host,
+#                        if @runnable is false. Optional.
+#                        Since 2.6.
 #
 # Since: 1.2.0
 ##
 { 'struct': 'CpuDefinitionInfo',
-  'data': { 'name': 'str' } }
+  'data': { 'name': 'str',
+            '*runnable': 'bool',
+            '*unavailable-features': [ 'str' ] } }
 
 ##
 # @query-cpu-definitions:
-- 
2.5.5

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
>
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qapi-schema.json | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#            machine and accelerator. Optional. Since 2.6.
> +# @unavailable-features: List of properties that prevent the CPU
> +#                        model from running in the current host,
> +#                        if @runnable is false. Optional.
> +#                        Since 2.6.
>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str',
> +            '*runnable': 'bool',
> +            '*unavailable-features': [ 'str' ] } }
>  
>  ##
>  # @query-cpu-definitions:

Can @unavailable-features be empty or missing when @runnable is true?

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
> 
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qapi-schema.json | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#            machine and accelerator. Optional. Since 2.6.
> +# @unavailable-features: List of properties that prevent the CPU
> +#                        model from running in the current host,
> +#                        if @runnable is false. Optional.
> +#                        Since 2.6.

Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
generation) also define if a CPU model is runnable, so the pure availability of
features does not mean that a cpu model is runnable.

We could have runnable=false and unavailable-features being an empty list.


>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str',
> +            '*runnable': 'bool',
> +            '*unavailable-features': [ 'str' ] } }
> 
>  ##
>  # @query-cpu-definitions:



David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote:
> > Extend query-cpu-definitions schema to allow it to return two new
> > optional fields: "runnable" and "unavailable-features".
> > "runnable" will tell if the CPU model can be run in the current
> > host. "unavailable-features" will contain a list of CPU
> > properties that are preventing the CPU model from running in the
> > current host.
> > 
> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Jiri Denemark <jdenemar@redhat.com>
> > Cc: libvir-list@redhat.com
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qapi-schema.json | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 54634c4..450e6e7 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2948,11 +2948,19 @@
> >  # Virtual CPU definition.
> >  #
> >  # @name: the name of the CPU definition
> > +# @runnable: true if the CPU model is runnable using the current
> > +#            machine and accelerator. Optional. Since 2.6.
> > +# @unavailable-features: List of properties that prevent the CPU
> > +#                        model from running in the current host,
> > +#                        if @runnable is false. Optional.
> > +#                        Since 2.6.
> 
> Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> generation) also define if a CPU model is runnable, so the pure availability of
> features does not mean that a cpu model is runnable.
> 
> We could have runnable=false and unavailable-features being an empty list.

Even on those cases, can't the root cause be mapped to a QOM
property name (e.g. "cpu-generation"), even if it's property that
can't be changed by the user?

We could replace this with something more generic, like:

@runnability-blockers: List of attributes that prevent the CPU
  model from running in the current host.
  
  A list of QOM property names that represent CPU model
  attributes that prevent the CPU from running. If the QOM
  property is read-only, that means the CPU model can never run
  in the current host. If the property is read-write, it means
  that it MAY be possible to run the CPU model in the current
  host if that property is changed.
  
  Management software can use it as hints to suggest or choose an
  alternative for the user, or just to generate meaningful error
  messages explaining why the CPU model can't be used.

(I am looking for a better name than "runnability-blockers").

-- 
Eduardo

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote:
> > > Extend query-cpu-definitions schema to allow it to return two new
> > > optional fields: "runnable" and "unavailable-features".
> > > "runnable" will tell if the CPU model can be run in the current
> > > host. "unavailable-features" will contain a list of CPU
> > > properties that are preventing the CPU model from running in the
> > > current host.
> > > 
> > > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > Cc: Jiri Denemark <jdenemar@redhat.com>
> > > Cc: libvir-list@redhat.com
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  qapi-schema.json | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > index 54634c4..450e6e7 100644
> > > --- a/qapi-schema.json
> > > +++ b/qapi-schema.json
> > > @@ -2948,11 +2948,19 @@
> > >  # Virtual CPU definition.
> > >  #
> > >  # @name: the name of the CPU definition
> > > +# @runnable: true if the CPU model is runnable using the current
> > > +#            machine and accelerator. Optional. Since 2.6.
> > > +# @unavailable-features: List of properties that prevent the CPU
> > > +#                        model from running in the current host,
> > > +#                        if @runnable is false. Optional.
> > > +#                        Since 2.6.  
> > 
> > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> > generation) also define if a CPU model is runnable, so the pure availability of
> > features does not mean that a cpu model is runnable.
> > 
> > We could have runnable=false and unavailable-features being an empty list.  
> 
> Even on those cases, can't the root cause be mapped to a QOM
> property name (e.g. "cpu-generation"), even if it's property that
> can't be changed by the user?

In the current state, no.

So I think for runnable=false:
a) unavailable-features set -> can be made runnable
b) unavailable-features not set -> cannot be made runnable

would be enough.

> 
> We could replace this with something more generic, like:
> 
> @runnability-blockers: List of attributes that prevent the CPU
>   model from running in the current host.
>   
>   A list of QOM property names that represent CPU model
>   attributes that prevent the CPU from running. If the QOM
>   property is read-only, that means the CPU model can never run
>   in the current host. If the property is read-write, it means
>   that it MAY be possible to run the CPU model in the current
>   host if that property is changed.
>   
>   Management software can use it as hints to suggest or choose an
>   alternative for the user, or just to generate meaningful error
>   messages explaining why the CPU model can't be used.
> 
> (I am looking for a better name than "runnability-blockers").
> 



David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Mon, May 09, 2016 at 02:05:05PM +0200, David Hildenbrand wrote:
> > On Mon, May 09, 2016 at 10:54:53AM +0200, David Hildenbrand wrote:
> > > > Extend query-cpu-definitions schema to allow it to return two new
> > > > optional fields: "runnable" and "unavailable-features".
> > > > "runnable" will tell if the CPU model can be run in the current
> > > > host. "unavailable-features" will contain a list of CPU
> > > > properties that are preventing the CPU model from running in the
> > > > current host.
> > > > 
> > > > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > > > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > > > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > Cc: Jiri Denemark <jdenemar@redhat.com>
> > > > Cc: libvir-list@redhat.com
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > >  qapi-schema.json | 10 +++++++++-
> > > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > index 54634c4..450e6e7 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -2948,11 +2948,19 @@
> > > >  # Virtual CPU definition.
> > > >  #
> > > >  # @name: the name of the CPU definition
> > > > +# @runnable: true if the CPU model is runnable using the current
> > > > +#            machine and accelerator. Optional. Since 2.6.
> > > > +# @unavailable-features: List of properties that prevent the CPU
> > > > +#                        model from running in the current host,
> > > > +#                        if @runnable is false. Optional.
> > > > +#                        Since 2.6.  
> > > 
> > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> > > generation) also define if a CPU model is runnable, so the pure availability of
> > > features does not mean that a cpu model is runnable.
> > > 
> > > We could have runnable=false and unavailable-features being an empty list.  
> > 
> > Even on those cases, can't the root cause be mapped to a QOM
> > property name (e.g. "cpu-generation"), even if it's property that
> > can't be changed by the user?
> 
> In the current state, no.

But it could be implemented by s390x in the future, if it's
considered useful, right?

> 
> So I think for runnable=false:
> a) unavailable-features set -> can be made runnable
> b) unavailable-features not set -> cannot be made runnable
> 
> would be enough.

I understand it would be enough, but I would like to at least
define semantics that would make sense for all architectures in
case it gets implemented in the future. Would the proposal below
make sense?

> > 
> > We could replace this with something more generic, like:
> > 
> > @runnability-blockers: List of attributes that prevent the CPU
> >   model from running in the current host.
> >   
> >   A list of QOM property names that represent CPU model
> >   attributes that prevent the CPU from running. If the QOM
> >   property is read-only, that means the CPU model can never run
> >   in the current host. If the property is read-write, it means
> >   that it MAY be possible to run the CPU model in the current
> >   host if that property is changed.
> >   
> >   Management software can use it as hints to suggest or choose an
> >   alternative for the user, or just to generate meaningful error
> >   messages explaining why the CPU model can't be used.
> > 
> > (I am looking for a better name than "runnability-blockers").
> > 
> 
> 
> 
> David
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> > > > 
> > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> > > > generation) also define if a CPU model is runnable, so the pure availability of
> > > > features does not mean that a cpu model is runnable.
> > > > 
> > > > We could have runnable=false and unavailable-features being an empty list.    
> > > 
> > > Even on those cases, can't the root cause be mapped to a QOM
> > > property name (e.g. "cpu-generation"), even if it's property that
> > > can't be changed by the user?  
> > 
> > In the current state, no.  
> 
> But it could be implemented by s390x in the future, if it's
> considered useful, right?

Yes, we could fit that into read-only properties if we would ever need it
(like cpu-generation you mentioned and cpu-ga-level - both will always be
read-only).

However we could come up with more optional fields for that in the future.
(like unsupported-values or sth. like that). I actually prefer
unavailable-features over runnability-blockers.

> 
> > 
> > So I think for runnable=false:
> > a) unavailable-features set -> can be made runnable
> > b) unavailable-features not set -> cannot be made runnable
> > 
> > would be enough.  
> 
> I understand it would be enough, but I would like to at least
> define semantics that would make sense for all architectures in
> case it gets implemented in the future. Would the proposal below
> make sense?
> 

Yes, I think so. However to really make good hints, upper layers would most
likely need more information about the exact problem with a property -
maybe something like an enum value per problematic property.
(UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ...

> > > 
> > > We could replace this with something more generic, like:
> > > 
> > > @runnability-blockers: List of attributes that prevent the CPU
> > >   model from running in the current host.
> > >   
> > >   A list of QOM property names that represent CPU model
> > >   attributes that prevent the CPU from running. If the QOM
> > >   property is read-only, that means the CPU model can never run
> > >   in the current host. If the property is read-write, it means
> > >   that it MAY be possible to run the CPU model in the current
> > >   host if that property is changed.
> > >   
> > >   Management software can use it as hints to suggest or choose an
> > >   alternative for the user, or just to generate meaningful error
> > >   messages explaining why the CPU model can't be used.
> > > 
> > > (I am looking for a better name than "runnability-blockers").
> > >   

Not sure which approach would be better.

David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Mon, May 09, 2016 at 03:06:18PM +0200, David Hildenbrand wrote:
> > > > > 
> > > > > Just FYI, on other architectures (e.g. s390x), other conditions (e.g. cpu
> > > > > generation) also define if a CPU model is runnable, so the pure availability of
> > > > > features does not mean that a cpu model is runnable.
> > > > > 
> > > > > We could have runnable=false and unavailable-features being an empty list.    
> > > > 
> > > > Even on those cases, can't the root cause be mapped to a QOM
> > > > property name (e.g. "cpu-generation"), even if it's property that
> > > > can't be changed by the user?  
> > > 
> > > In the current state, no.  
> > 
> > But it could be implemented by s390x in the future, if it's
> > considered useful, right?
> 
> Yes, we could fit that into read-only properties if we would ever need it
> (like cpu-generation you mentioned and cpu-ga-level - both will always be
> read-only).
> 
> However we could come up with more optional fields for that in the future.
> (like unsupported-values or sth. like that). I actually prefer
> unavailable-features over runnability-blockers.
> 
> > 
> > > 
> > > So I think for runnable=false:
> > > a) unavailable-features set -> can be made runnable
> > > b) unavailable-features not set -> cannot be made runnable
> > > 
> > > would be enough.  
> > 
> > I understand it would be enough, but I would like to at least
> > define semantics that would make sense for all architectures in
> > case it gets implemented in the future. Would the proposal below
> > make sense?
> > 
> 
> Yes, I think so. However to really make good hints, upper layers would most
> likely need more information about the exact problem with a property -
> maybe something like an enum value per problematic property.
> (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ...

If a more powerful interface is needed, we can add extra fields,
yes. Although I'm not sure we really start providing that level
of detail in a generic way (I guess it will depend on how much
arch-independent code libvirt will use to handle runnability
information).

> 
> > > > 
> > > > We could replace this with something more generic, like:
> > > > 
> > > > @runnability-blockers: List of attributes that prevent the CPU
> > > >   model from running in the current host.
> > > >   
> > > >   A list of QOM property names that represent CPU model
> > > >   attributes that prevent the CPU from running. If the QOM
> > > >   property is read-only, that means the CPU model can never run
> > > >   in the current host. If the property is read-write, it means
> > > >   that it MAY be possible to run the CPU model in the current
> > > >   host if that property is changed.
> > > >   
> > > >   Management software can use it as hints to suggest or choose an
> > > >   alternative for the user, or just to generate meaningful error
> > > >   messages explaining why the CPU model can't be used.
> > > > 
> > > > (I am looking for a better name than "runnability-blockers").
> > > >   
> 
> Not sure which approach would be better.

Note that this is only a change in documentation of the new
field, to allow it to cover extra cases without any changes in
the interface.

I also don't like the "runnability-blockers" name, but I prefer
the new documentation I wrote above because it is more explicit
about what exactly the field means. I plan to keep the
"unavailable-features" name but use the above documentation text
in the next version. Does that sound OK?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> > Yes, I think so. However to really make good hints, upper layers would most
> > likely need more information about the exact problem with a property -
> > maybe something like an enum value per problematic property.
> > (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ...  
> 
> If a more powerful interface is needed, we can add extra fields,
> yes. Although I'm not sure we really start providing that level
> of detail in a generic way (I guess it will depend on how much
> arch-independent code libvirt will use to handle runnability
> information).

And I would actually (later) prefer to have something like
bool too_new; (name tbd)

to cover the cpu-generation problem instead of having to expose read-only
properties just for the sake of communicating that a cpu model is simply too new
and cannot be made runnable toggling features.

> 
> >   
> > > > > 
> > > > > We could replace this with something more generic, like:
> > > > > 
> > > > > @runnability-blockers: List of attributes that prevent the CPU
> > > > >   model from running in the current host.
> > > > >   
> > > > >   A list of QOM property names that represent CPU model
> > > > >   attributes that prevent the CPU from running. If the QOM
> > > > >   property is read-only, that means the CPU model can never run
> > > > >   in the current host. If the property is read-write, it means
> > > > >   that it MAY be possible to run the CPU model in the current
> > > > >   host if that property is changed.
> > > > >   
> > > > >   Management software can use it as hints to suggest or choose an
> > > > >   alternative for the user, or just to generate meaningful error
> > > > >   messages explaining why the CPU model can't be used.
> > > > > 
> > > > > (I am looking for a better name than "runnability-blockers").
> > > > >     
> > 
> > Not sure which approach would be better.  
> 
> Note that this is only a change in documentation of the new
> field, to allow it to cover extra cases without any changes in
> the interface.
> 
> I also don't like the "runnability-blockers" name, but I prefer
> the new documentation I wrote above because it is more explicit
> about what exactly the field means. I plan to keep the
> "unavailable-features" name but use the above documentation text
> in the next version. Does that sound OK?
> 

I like the read-only part about that, but still you should somehow clarify that
we are dealing with boolean properties a.k.a features. At least that's my
opinion.

Apart from that, this is fine with me!

David

Re: [Qemu-devel] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Tue, May 10, 2016 at 08:32:53AM +0200, David Hildenbrand wrote:
> 
> > > Yes, I think so. However to really make good hints, upper layers would most
> > > likely need more information about the exact problem with a property -
> > > maybe something like an enum value per problematic property.
> > > (UNAVAILABLE_FEATURE, VALUE_TOO_BIG, VALUE_TOO_SMALL, UNSUPPORTED_VALUE) ...  
> > 
> > If a more powerful interface is needed, we can add extra fields,
> > yes. Although I'm not sure we really start providing that level
> > of detail in a generic way (I guess it will depend on how much
> > arch-independent code libvirt will use to handle runnability
> > information).
> 
> And I would actually (later) prefer to have something like
> bool too_new; (name tbd)
> 
> to cover the cpu-generation problem instead of having to expose read-only
> properties just for the sake of communicating that a cpu model is simply too new
> and cannot be made runnable toggling features.

That could work. Or a "CPUNotRunnableReason" enum with values
like MISSING_FEATURES, TOO_NEW, etc.

> 
> > 
> > >   
> > > > > > 
> > > > > > We could replace this with something more generic, like:
> > > > > > 
> > > > > > @runnability-blockers: List of attributes that prevent the CPU
> > > > > >   model from running in the current host.
> > > > > >   
> > > > > >   A list of QOM property names that represent CPU model
> > > > > >   attributes that prevent the CPU from running. If the QOM
> > > > > >   property is read-only, that means the CPU model can never run
> > > > > >   in the current host. If the property is read-write, it means
> > > > > >   that it MAY be possible to run the CPU model in the current
> > > > > >   host if that property is changed.
> > > > > >   
> > > > > >   Management software can use it as hints to suggest or choose an
> > > > > >   alternative for the user, or just to generate meaningful error
> > > > > >   messages explaining why the CPU model can't be used.
> > > > > > 
> > > > > > (I am looking for a better name than "runnability-blockers").
> > > > > >     
> > > 
> > > Not sure which approach would be better.  
> > 
> > Note that this is only a change in documentation of the new
> > field, to allow it to cover extra cases without any changes in
> > the interface.
> > 
> > I also don't like the "runnability-blockers" name, but I prefer
> > the new documentation I wrote above because it is more explicit
> > about what exactly the field means. I plan to keep the
> > "unavailable-features" name but use the above documentation text
> > in the next version. Does that sound OK?
> > 
> 
> I like the read-only part about that, but still you should somehow clarify that
> we are dealing with boolean properties a.k.a features. At least that's my
> opinion.

The point is to not restrict it to boolean properties, if the
architecture code wants to return other properties. The
information is less useful for them, but they would still be
allowed.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eric Blake]

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

On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> Extend query-cpu-definitions schema to allow it to return two new
> optional fields: "runnable" and "unavailable-features".
> "runnable" will tell if the CPU model can be run in the current
> host. "unavailable-features" will contain a list of CPU
> properties that are preventing the CPU model from running in the
> current host.
> 
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qapi-schema.json | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 54634c4..450e6e7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2948,11 +2948,19 @@
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> +# @runnable: true if the CPU model is runnable using the current
> +#            machine and accelerator. Optional. Since 2.6.

You've missed 2.6.  Also, the typical spelling for a per-member
designation is '(since 2.7)', not 'Since 2.7'

Why is it optional? Would it hurt to always be present in qemu new
enough to understand why it is needed?

> +# @unavailable-features: List of properties that prevent the CPU
> +#                        model from running in the current host,
> +#                        if @runnable is false. Optional.
> +#                        Since 2.6.
>  #
>  # Since: 1.2.0
>  ##
>  { 'struct': 'CpuDefinitionInfo',
> -  'data': { 'name': 'str' } }
> +  'data': { 'name': 'str',
> +            '*runnable': 'bool',
> +            '*unavailable-features': [ 'str' ] } }
>  
>  ##
>  # @query-cpu-definitions:
> 

-- 
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] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> > Extend query-cpu-definitions schema to allow it to return two new
> > optional fields: "runnable" and "unavailable-features".
> > "runnable" will tell if the CPU model can be run in the current
> > host. "unavailable-features" will contain a list of CPU
> > properties that are preventing the CPU model from running in the
> > current host.
> > 
> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> > Cc: Jiri Denemark <jdenemar@redhat.com>
> > Cc: libvir-list@redhat.com
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  qapi-schema.json | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 54634c4..450e6e7 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -2948,11 +2948,19 @@
> >  # Virtual CPU definition.
> >  #
> >  # @name: the name of the CPU definition
> > +# @runnable: true if the CPU model is runnable using the current
> > +#            machine and accelerator. Optional. Since 2.6.
> 
> You've missed 2.6.  Also, the typical spelling for a per-member
> designation is '(since 2.7)', not 'Since 2.7'

Oops! I meant 2.7, and I didn't notice that it was not using the
typical format. I will fix it, thanks.

> 
> Why is it optional? Would it hurt to always be present in qemu new
> enough to understand why it is needed?

It is optional because not all architectures will return the
field. This series implements it only for x86.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
>> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
>> > Extend query-cpu-definitions schema to allow it to return two new
>> > optional fields: "runnable" and "unavailable-features".
>> > "runnable" will tell if the CPU model can be run in the current
>> > host. "unavailable-features" will contain a list of CPU
>> > properties that are preventing the CPU model from running in the
>> > current host.
>> > 
>> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
>> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> > Cc: Jiri Denemark <jdenemar@redhat.com>
>> > Cc: libvir-list@redhat.com
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> >  qapi-schema.json | 10 +++++++++-
>> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/qapi-schema.json b/qapi-schema.json
>> > index 54634c4..450e6e7 100644
>> > --- a/qapi-schema.json
>> > +++ b/qapi-schema.json
>> > @@ -2948,11 +2948,19 @@
>> >  # Virtual CPU definition.
>> >  #
>> >  # @name: the name of the CPU definition
>> > +# @runnable: true if the CPU model is runnable using the current
>> > +#            machine and accelerator. Optional. Since 2.6.
>> 
>> You've missed 2.6.  Also, the typical spelling for a per-member
>> designation is '(since 2.7)', not 'Since 2.7'
>
> Oops! I meant 2.7, and I didn't notice that it was not using the
> typical format. I will fix it, thanks.
>
>> 
>> Why is it optional? Would it hurt to always be present in qemu new
>> enough to understand why it is needed?
>
> It is optional because not all architectures will return the
> field. This series implements it only for x86.

Its documentation seems to suggest missing runnable has the same meaning
as runnable: false.  Is that correct?

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Jiri Denemark]

On Tue, May 10, 2016 at 10:23:16 +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> >> > Extend query-cpu-definitions schema to allow it to return two new
> >> > optional fields: "runnable" and "unavailable-features".
> >> > "runnable" will tell if the CPU model can be run in the current
> >> > host. "unavailable-features" will contain a list of CPU
> >> > properties that are preventing the CPU model from running in the
> >> > current host.
> >> > 
> >> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> >> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> > Cc: Jiri Denemark <jdenemar@redhat.com>
> >> > Cc: libvir-list@redhat.com
> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > ---
> >> >  qapi-schema.json | 10 +++++++++-
> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> > index 54634c4..450e6e7 100644
> >> > --- a/qapi-schema.json
> >> > +++ b/qapi-schema.json
> >> > @@ -2948,11 +2948,19 @@
> >> >  # Virtual CPU definition.
> >> >  #
> >> >  # @name: the name of the CPU definition
> >> > +# @runnable: true if the CPU model is runnable using the current
> >> > +#            machine and accelerator. Optional. Since 2.6.
> >> 
> >> You've missed 2.6.  Also, the typical spelling for a per-member
> >> designation is '(since 2.7)', not 'Since 2.7'
> >
> > Oops! I meant 2.7, and I didn't notice that it was not using the
> > typical format. I will fix it, thanks.
> >
> >> 
> >> Why is it optional? Would it hurt to always be present in qemu new
> >> enough to understand why it is needed?
> >
> > It is optional because not all architectures will return the
> > field. This series implements it only for x86.
> 
> Its documentation seems to suggest missing runnable has the same meaning
> as runnable: false.  Is that correct?

I think it would be a bug if missing runnable had the same meaning as
runnable=false. To maintain backward compatibility with older QEMU
missing runnable should mean the CPU may or may not be runnable, i.e.,
we don't know which and the user has to try to figure it out.

Jirka

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> >> > Extend query-cpu-definitions schema to allow it to return two new
> >> > optional fields: "runnable" and "unavailable-features".
> >> > "runnable" will tell if the CPU model can be run in the current
> >> > host. "unavailable-features" will contain a list of CPU
> >> > properties that are preventing the CPU model from running in the
> >> > current host.
> >> > 
> >> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> >> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> > Cc: Jiri Denemark <jdenemar@redhat.com>
> >> > Cc: libvir-list@redhat.com
> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > ---
> >> >  qapi-schema.json | 10 +++++++++-
> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> > index 54634c4..450e6e7 100644
> >> > --- a/qapi-schema.json
> >> > +++ b/qapi-schema.json
> >> > @@ -2948,11 +2948,19 @@
> >> >  # Virtual CPU definition.
> >> >  #
> >> >  # @name: the name of the CPU definition
> >> > +# @runnable: true if the CPU model is runnable using the current
> >> > +#            machine and accelerator. Optional. Since 2.6.
> >> 
> >> You've missed 2.6.  Also, the typical spelling for a per-member
> >> designation is '(since 2.7)', not 'Since 2.7'
> >
> > Oops! I meant 2.7, and I didn't notice that it was not using the
> > typical format. I will fix it, thanks.
> >
> >> 
> >> Why is it optional? Would it hurt to always be present in qemu new
> >> enough to understand why it is needed?
> >
> > It is optional because not all architectures will return the
> > field. This series implements it only for x86.
> 
> Its documentation seems to suggest missing runnable has the same meaning
> as runnable: false.  Is that correct?

No, it means the architecture code doesn't implement the feature
yet and we don't know if the CPU model is runnable or not.

The day we implement the new field in all architectures, we can
stop making it optional.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
>> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
>> >> > Extend query-cpu-definitions schema to allow it to return two new
>> >> > optional fields: "runnable" and "unavailable-features".
>> >> > "runnable" will tell if the CPU model can be run in the current
>> >> > host. "unavailable-features" will contain a list of CPU
>> >> > properties that are preventing the CPU model from running in the
>> >> > current host.
>> >> > 
>> >> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> >> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
>> >> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> >> > Cc: Jiri Denemark <jdenemar@redhat.com>
>> >> > Cc: libvir-list@redhat.com
>> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> > ---
>> >> >  qapi-schema.json | 10 +++++++++-
>> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >> > 
>> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> > index 54634c4..450e6e7 100644
>> >> > --- a/qapi-schema.json
>> >> > +++ b/qapi-schema.json
>> >> > @@ -2948,11 +2948,19 @@
>> >> >  # Virtual CPU definition.
>> >> >  #
>> >> >  # @name: the name of the CPU definition
>> >> > +# @runnable: true if the CPU model is runnable using the current
>> >> > +#            machine and accelerator. Optional. Since 2.6.
>> >> 
>> >> You've missed 2.6.  Also, the typical spelling for a per-member
>> >> designation is '(since 2.7)', not 'Since 2.7'
>> >
>> > Oops! I meant 2.7, and I didn't notice that it was not using the
>> > typical format. I will fix it, thanks.
>> >
>> >> 
>> >> Why is it optional? Would it hurt to always be present in qemu new
>> >> enough to understand why it is needed?
>> >
>> > It is optional because not all architectures will return the
>> > field. This series implements it only for x86.
>> 
>> Its documentation seems to suggest missing runnable has the same meaning
>> as runnable: false.  Is that correct?
>
> No, it means the architecture code doesn't implement the feature
> yet and we don't know if the CPU model is runnable or not.
>
> The day we implement the new field in all architectures, we can
> stop making it optional.

Please clarify that in the docs.  Here's my try:

# @runnable: #optional whether the CPU model us usable with the current
# machine and accelerator, only present if we know (since 2.6)

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Wed, May 11, 2016 at 09:11:33AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> >> 
> >> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
> >> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
> >> >> > Extend query-cpu-definitions schema to allow it to return two new
> >> >> > optional fields: "runnable" and "unavailable-features".
> >> >> > "runnable" will tell if the CPU model can be run in the current
> >> >> > host. "unavailable-features" will contain a list of CPU
> >> >> > properties that are preventing the CPU model from running in the
> >> >> > current host.
> >> >> > 
> >> >> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> >> >> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
> >> >> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> >> >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> >> > Cc: Jiri Denemark <jdenemar@redhat.com>
> >> >> > Cc: libvir-list@redhat.com
> >> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >> >> > ---
> >> >> >  qapi-schema.json | 10 +++++++++-
> >> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >> >> > 
> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
> >> >> > index 54634c4..450e6e7 100644
> >> >> > --- a/qapi-schema.json
> >> >> > +++ b/qapi-schema.json
> >> >> > @@ -2948,11 +2948,19 @@
> >> >> >  # Virtual CPU definition.
> >> >> >  #
> >> >> >  # @name: the name of the CPU definition
> >> >> > +# @runnable: true if the CPU model is runnable using the current
> >> >> > +#            machine and accelerator. Optional. Since 2.6.
> >> >> 
> >> >> You've missed 2.6.  Also, the typical spelling for a per-member
> >> >> designation is '(since 2.7)', not 'Since 2.7'
> >> >
> >> > Oops! I meant 2.7, and I didn't notice that it was not using the
> >> > typical format. I will fix it, thanks.
> >> >
> >> >> 
> >> >> Why is it optional? Would it hurt to always be present in qemu new
> >> >> enough to understand why it is needed?
> >> >
> >> > It is optional because not all architectures will return the
> >> > field. This series implements it only for x86.
> >> 
> >> Its documentation seems to suggest missing runnable has the same meaning
> >> as runnable: false.  Is that correct?
> >
> > No, it means the architecture code doesn't implement the feature
> > yet and we don't know if the CPU model is runnable or not.
> >
> > The day we implement the new field in all architectures, we can
> > stop making it optional.
> 
> Please clarify that in the docs.  Here's my try:
> 
> # @runnable: #optional whether the CPU model us usable with the current
> # machine and accelerator, only present if we know (since 2.6)

Updated to:

##
# @CpuDefinitionInfo:
#
# Virtual CPU definition.
#
# @name: the name of the CPU definition
# @runnable: #optional. whether the CPU model us usable with the
#            current machine and accelerator. Omitted if we don't
#            know the answer. (since 2.7)
# @unavailable-features: List of attributes that prevent the CPU
#                        model from running in the current host.
#                        (since 2.7)
#
# @unavailable-features is a list of QOM property names that
# represent CPU model attributes that prevent the CPU from running.
# If the QOM property is read-only, that means the CPU model can
# never run in the current host. If the property is read-write, it
# means that it MAY be possible to run the CPU model in the current
# host if that property is changed. Management software can use it
# as hints to suggest or choose an alternative for the user, or
# just to generate meaningful error messages explaining why the CPU
# model can't be used.
#
# Since: 1.2.0
##

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> Updated to:
> 
> ##
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the
> #            current machine and accelerator. Omitted if we don't
> #            know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU

"List of properties" ?

> #                        model from running in the current host.
> #                        (since 2.7)
> #
> # @unavailable-features is a list of QOM property names that
> # represent CPU model attributes that prevent the CPU from running.
> # If the QOM property is read-only, that means the CPU model can
> # never run in the current host. If the property is read-write, it
> # means that it MAY be possible to run the CPU model in the current
> # host if that property is changed. Management software can use it
> # as hints to suggest or choose an alternative for the user, or
> # just to generate meaningful error messages explaining why the CPU
> # model can't be used.
> #
> # Since: 1.2.0
> ##
> 

what about changing unavailable-features to

problematic-properties
responsible-properties

... anything else making clear that we are dealing with properties, not
only features?

Apart from that, sounds good to me.

David

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Jiri Denemark]

On Wed, May 11, 2016 at 16:35:50 -0300, Eduardo Habkost wrote:
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the

s/ us / is /

> #            current machine and accelerator. Omitted if we don't
> #            know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU
> #                        model from running in the current host.
> #                        (since 2.7)
> #
> # @unavailable-features is a list of QOM property names that
> # represent CPU model attributes that prevent the CPU from running.
> # If the QOM property is read-only, that means the CPU model can
> # never run in the current host. If the property is read-write, it
> # means that it MAY be possible to run the CPU model in the current
> # host if that property is changed. Management software can use it
> # as hints to suggest or choose an alternative for the user, or
> # just to generate meaningful error messages explaining why the CPU
> # model can't be used.

Any chance this could be extended to provide data about every single
machine type rather than just the current one?

Jirka

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Thu, May 12, 2016 at 09:19:48AM +0200, Jiri Denemark wrote:
> On Wed, May 11, 2016 at 16:35:50 -0300, Eduardo Habkost wrote:
> > # @CpuDefinitionInfo:
> > #
> > # Virtual CPU definition.
> > #
> > # @name: the name of the CPU definition
> > # @runnable: #optional. whether the CPU model us usable with the
> 
> s/ us / is /

Thanks!

> 
> > #            current machine and accelerator. Omitted if we don't
> > #            know the answer. (since 2.7)
> > # @unavailable-features: List of attributes that prevent the CPU
> > #                        model from running in the current host.
> > #                        (since 2.7)
> > #
> > # @unavailable-features is a list of QOM property names that
> > # represent CPU model attributes that prevent the CPU from running.
> > # If the QOM property is read-only, that means the CPU model can
> > # never run in the current host. If the property is read-write, it
> > # means that it MAY be possible to run the CPU model in the current
> > # host if that property is changed. Management software can use it
> > # as hints to suggest or choose an alternative for the user, or
> > # just to generate meaningful error messages explaining why the CPU
> > # model can't be used.
> 
> Any chance this could be extended to provide data about every single
> machine type rather than just the current one?

I want to do that, but it would require reorganizing
CPU/machine/accelerator code to allow probing to happen without
depending on global data (including: machine state, accelerator
state, global properties). This will probably take a while to be
implemented.

But: at least for x86, we now guarantee that runnability
shouldn't change depending on the machine-type. If a CPU model is
runnable with a machine version, it should be runnable with other
versions of the same family.

That guarantee is not documented above because I still don't know
if we can enforce it on all other architectures.

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, May 11, 2016 at 09:11:33AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Tue, May 10, 2016 at 10:23:16AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >> 
>> >> > On Mon, May 09, 2016 at 09:20:15AM -0600, Eric Blake wrote:
>> >> >> On 05/06/2016 12:11 PM, Eduardo Habkost wrote:
>> >> >> > Extend query-cpu-definitions schema to allow it to return two new
>> >> >> > optional fields: "runnable" and "unavailable-features".
>> >> >> > "runnable" will tell if the CPU model can be run in the current
>> >> >> > host. "unavailable-features" will contain a list of CPU
>> >> >> > properties that are preventing the CPU model from running in the
>> >> >> > current host.
>> >> >> > 
>> >> >> > Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> >> >> > Cc: Michael Mueller <mimu@linux.vnet.ibm.com>
>> >> >> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
>> >> >> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> >> >> > Cc: Jiri Denemark <jdenemar@redhat.com>
>> >> >> > Cc: libvir-list@redhat.com
>> >> >> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> >> >> > ---
>> >> >> >  qapi-schema.json | 10 +++++++++-
>> >> >> >  1 file changed, 9 insertions(+), 1 deletion(-)
>> >> >> > 
>> >> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> >> > index 54634c4..450e6e7 100644
>> >> >> > --- a/qapi-schema.json
>> >> >> > +++ b/qapi-schema.json
>> >> >> > @@ -2948,11 +2948,19 @@
>> >> >> >  # Virtual CPU definition.
>> >> >> >  #
>> >> >> >  # @name: the name of the CPU definition
>> >> >> > +# @runnable: true if the CPU model is runnable using the current
>> >> >> > +#            machine and accelerator. Optional. Since 2.6.
>> >> >> 
>> >> >> You've missed 2.6.  Also, the typical spelling for a per-member
>> >> >> designation is '(since 2.7)', not 'Since 2.7'
>> >> >
>> >> > Oops! I meant 2.7, and I didn't notice that it was not using the
>> >> > typical format. I will fix it, thanks.
>> >> >
>> >> >> 
>> >> >> Why is it optional? Would it hurt to always be present in qemu new
>> >> >> enough to understand why it is needed?
>> >> >
>> >> > It is optional because not all architectures will return the
>> >> > field. This series implements it only for x86.
>> >> 
>> >> Its documentation seems to suggest missing runnable has the same meaning
>> >> as runnable: false.  Is that correct?
>> >
>> > No, it means the architecture code doesn't implement the feature
>> > yet and we don't know if the CPU model is runnable or not.
>> >
>> > The day we implement the new field in all architectures, we can
>> > stop making it optional.
>> 
>> Please clarify that in the docs.  Here's my try:
>> 
>> # @runnable: #optional whether the CPU model us usable with the current
>> # machine and accelerator, only present if we know (since 2.6)
>
> Updated to:
>
> ##
> # @CpuDefinitionInfo:
> #
> # Virtual CPU definition.
> #
> # @name: the name of the CPU definition
> # @runnable: #optional. whether the CPU model us usable with the
> #            current machine and accelerator. Omitted if we don't
> #            know the answer. (since 2.7)
> # @unavailable-features: List of attributes that prevent the CPU

Unless you drop the * sigil from '*unavailable-features', you need to
insert #optional after the colon.

> #                        model from running in the current host.
> #                        (since 2.7)
> #
> # @unavailable-features is a list of QOM property names that
> # represent CPU model attributes that prevent the CPU from running.
> # If the QOM property is read-only, that means the CPU model can
> # never run in the current host. If the property is read-write, it
> # means that it MAY be possible to run the CPU model in the current
> # host if that property is changed. Management software can use it
> # as hints to suggest or choose an alternative for the user, or
> # just to generate meaningful error messages explaining why the CPU
> # model can't be used.
> #
> # Since: 1.2.0
> ##

Better.

Next issue: how @runnable and @unavailable-features are related isn't
fully documented.  Here's my guess:

Combinations possible?            @runnable
@unavailable-features       absent  false  true
absent                         yes      ?     ?
present, empty                   ?      ?     ?
present, non-empty               ?    yes    no

The '?' need to be answered, too.

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

Just noticed that I hadn't replied to this yet. Sorry for the
long delay!

On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
[...]
> > ##
> > # @CpuDefinitionInfo:
> > #
> > # Virtual CPU definition.
> > #
> > # @name: the name of the CPU definition
> > # @runnable: #optional. whether the CPU model us usable with the
> > #            current machine and accelerator. Omitted if we don't
> > #            know the answer. (since 2.7)
> > # @unavailable-features: List of attributes that prevent the CPU
> 
> Unless you drop the * sigil from '*unavailable-features', you need to
> insert #optional after the colon.

Fixed.

> 
> > #                        model from running in the current host.
> > #                        (since 2.7)
> > #
> > # @unavailable-features is a list of QOM property names that
> > # represent CPU model attributes that prevent the CPU from running.
> > # If the QOM property is read-only, that means the CPU model can
> > # never run in the current host. If the property is read-write, it
> > # means that it MAY be possible to run the CPU model in the current
> > # host if that property is changed. Management software can use it
> > # as hints to suggest or choose an alternative for the user, or
> > # just to generate meaningful error messages explaining why the CPU
> > # model can't be used.
> > #
> > # Since: 1.2.0
> > ##
> 
> Better.
> 
> Next issue: how @runnable and @unavailable-features are related isn't
> fully documented.  Here's my guess:
> 
> Combinations possible?            @runnable
> @unavailable-features       absent  false  true
> absent                         yes      ?     ?
> present, empty                   ?      ?     ?
> present, non-empty               ?    yes    no

unavailable-features should be present only if runnable is false.
It may be absent or empty if the architecture code still doesn't
provide detailed info.

Once we have additional architectures implementing the new
fields, we can consider requiring unavailable-features to be
always present (and non-empty) if runnable is false.

In other words:

Combinations possible?            @runnable
@unavailable-features       absent  false  true
absent                         yes  yes[1]  yes
present, empty                  no  yes[1]   no
present, non-empty              no   yes     no

[1] I would like it to be "no", but I prefer to make it mandatory
    only after we get some experience with other architectures.


I'm making the following changes to the documentation:

 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
-# @runnable: #optional. whether the CPU model us usable with the
+# @runnable: #optional Whether the CPU model us usable with the
 #            current machine and accelerator. Omitted if we don't
 #            know the answer. (since 2.7)
-# @unavailable-features: List of attributes that prevent the CPU
-#                        model from running in the current host.
+# @unavailable-features: #optional List of attributes that prevent
+#                        the CPU model from running in the current
+#                        host. Present only if @runnable is false.
 #                        (since 2.7)
 #
 # @unavailable-features is a list of QOM property names that

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> Just noticed that I hadn't replied to this yet. Sorry for the
> long delay!
>
> On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
> [...]
>> > ##
>> > # @CpuDefinitionInfo:
>> > #
>> > # Virtual CPU definition.
>> > #
>> > # @name: the name of the CPU definition
>> > # @runnable: #optional. whether the CPU model us usable with the
>> > #            current machine and accelerator. Omitted if we don't
>> > #            know the answer. (since 2.7)
>> > # @unavailable-features: List of attributes that prevent the CPU
>> 
>> Unless you drop the * sigil from '*unavailable-features', you need to
>> insert #optional after the colon.
>
> Fixed.
>
>> 
>> > #                        model from running in the current host.
>> > #                        (since 2.7)
>> > #
>> > # @unavailable-features is a list of QOM property names that
>> > # represent CPU model attributes that prevent the CPU from running.
>> > # If the QOM property is read-only, that means the CPU model can
>> > # never run in the current host. If the property is read-write, it
>> > # means that it MAY be possible to run the CPU model in the current
>> > # host if that property is changed. Management software can use it
>> > # as hints to suggest or choose an alternative for the user, or
>> > # just to generate meaningful error messages explaining why the CPU
>> > # model can't be used.
>> > #
>> > # Since: 1.2.0
>> > ##
>> 
>> Better.
>> 
>> Next issue: how @runnable and @unavailable-features are related isn't
>> fully documented.  Here's my guess:
>> 
>> Combinations possible?            @runnable
>> @unavailable-features       absent  false  true
>> absent                         yes      ?     ?
>> present, empty                   ?      ?     ?
>> present, non-empty               ?    yes    no
>
> unavailable-features should be present only if runnable is false.
> It may be absent or empty if the architecture code still doesn't
> provide detailed info.
>
> Once we have additional architectures implementing the new
> fields, we can consider requiring unavailable-features to be
> always present (and non-empty) if runnable is false.
>
> In other words:
>
> Combinations possible?            @runnable
> @unavailable-features       absent  false  true
> absent                         yes  yes[1]  yes
> present, empty                  no  yes[1]   no
> present, non-empty              no   yes     no
>
> [1] I would like it to be "no", but I prefer to make it mandatory
>     only after we get some experience with other architectures.
>
>
> I'm making the following changes to the documentation:
>
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> -# @runnable: #optional. whether the CPU model us usable with the
> +# @runnable: #optional Whether the CPU model us usable with the
>  #            current machine and accelerator. Omitted if we don't
>  #            know the answer. (since 2.7)
> -# @unavailable-features: List of attributes that prevent the CPU
> -#                        model from running in the current host.
> +# @unavailable-features: #optional List of attributes that prevent
> +#                        the CPU model from running in the current
> +#                        host. Present only if @runnable is false.
>  #                        (since 2.7)
>  #
>  # @unavailable-features is a list of QOM property names that

"Present only if @runnable is false" makes me wonder why we need two
separate optional members tied together with constraints.  I dislike
such constraints, and avoid them whenever practical.

The new members encode an answer to the question whether a certain CPU
usable with the current machine an accelerator, and if no, why.
The possible answers are:

(1) Don't know.
(2) Yes.
(3) No, but we can't say why.
(4) No, and here's a list of reasons.

The two "dunno" answers (1) and (3) exist so we don't have to boil the
CPU ocean now.

Without them, the natural solution is a single member, where (4) is
encoded as nonempty list, and (2) could be encoded as empty list or
absent.

Now let me try to fit in (1) and (3).

The obvious way to do (1) is absent.  So let's use empty list for (2).

That leaves (3).  I think the simplest solution that could possibly work
is to treat it as a special "dunno" reason: encode it just like (4), but
with a special "dunno" list element.  I'd use the empty string.

Could even be used if we need to distinguish

(4a) No, and here's the *complete* list of reasons.
(4b) No, and here's a possibly incomplete list of reasons.

For (4b), include the "dunno" element with the others.

Unlike the proposed solution, this one doesn't leave interface crud
behind if we succeed in getting rid of (1) and (3):

* When (1) goes away, the single member becomes mandatory.

* When (3) goes away, the special "dunno" list element no longer occurs.

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > Just noticed that I hadn't replied to this yet. Sorry for the
> > long delay!
> >
> > On Thu, May 12, 2016 at 09:46:25AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost <ehabkost@redhat.com> writes:
> > [...]
> >> > ##
> >> > # @CpuDefinitionInfo:
> >> > #
> >> > # Virtual CPU definition.
> >> > #
> >> > # @name: the name of the CPU definition
> >> > # @runnable: #optional. whether the CPU model us usable with the
> >> > #            current machine and accelerator. Omitted if we don't
> >> > #            know the answer. (since 2.7)
> >> > # @unavailable-features: List of attributes that prevent the CPU
> >> 
> >> Unless you drop the * sigil from '*unavailable-features', you need to
> >> insert #optional after the colon.
> >
> > Fixed.
> >
> >> 
> >> > #                        model from running in the current host.
> >> > #                        (since 2.7)
> >> > #
> >> > # @unavailable-features is a list of QOM property names that
> >> > # represent CPU model attributes that prevent the CPU from running.
> >> > # If the QOM property is read-only, that means the CPU model can
> >> > # never run in the current host. If the property is read-write, it
> >> > # means that it MAY be possible to run the CPU model in the current
> >> > # host if that property is changed. Management software can use it
> >> > # as hints to suggest or choose an alternative for the user, or
> >> > # just to generate meaningful error messages explaining why the CPU
> >> > # model can't be used.
> >> > #
> >> > # Since: 1.2.0
> >> > ##
> >> 
> >> Better.
> >> 
> >> Next issue: how @runnable and @unavailable-features are related isn't
> >> fully documented.  Here's my guess:
> >> 
> >> Combinations possible?            @runnable
> >> @unavailable-features       absent  false  true
> >> absent                         yes      ?     ?
> >> present, empty                   ?      ?     ?
> >> present, non-empty               ?    yes    no
> >
> > unavailable-features should be present only if runnable is false.
> > It may be absent or empty if the architecture code still doesn't
> > provide detailed info.
> >
> > Once we have additional architectures implementing the new
> > fields, we can consider requiring unavailable-features to be
> > always present (and non-empty) if runnable is false.
> >
> > In other words:
> >
> > Combinations possible?            @runnable
> > @unavailable-features       absent  false  true
> > absent                         yes  yes[1]  yes
> > present, empty                  no  yes[1]   no
> > present, non-empty              no   yes     no
> >
> > [1] I would like it to be "no", but I prefer to make it mandatory
> >     only after we get some experience with other architectures.
> >
> >
> > I'm making the following changes to the documentation:
> >
> >  # Virtual CPU definition.
> >  #
> >  # @name: the name of the CPU definition
> > -# @runnable: #optional. whether the CPU model us usable with the
> > +# @runnable: #optional Whether the CPU model us usable with the
> >  #            current machine and accelerator. Omitted if we don't
> >  #            know the answer. (since 2.7)
> > -# @unavailable-features: List of attributes that prevent the CPU
> > -#                        model from running in the current host.
> > +# @unavailable-features: #optional List of attributes that prevent
> > +#                        the CPU model from running in the current
> > +#                        host. Present only if @runnable is false.
> >  #                        (since 2.7)
> >  #
> >  # @unavailable-features is a list of QOM property names that
> 
> "Present only if @runnable is false" makes me wonder why we need two
> separate optional members tied together with constraints.  I dislike
> such constraints, and avoid them whenever practical.
> 
> The new members encode an answer to the question whether a certain CPU
> usable with the current machine an accelerator, and if no, why.
> The possible answers are:
> 
> (1) Don't know.
> (2) Yes.
> (3) No, but we can't say why.
> (4) No, and here's a list of reasons.
> 
> The two "dunno" answers (1) and (3) exist so we don't have to boil the
> CPU ocean now.
> 
> Without them, the natural solution is a single member, where (4) is
> encoded as nonempty list, and (2) could be encoded as empty list or
> absent.
> 
> Now let me try to fit in (1) and (3).
> 
> The obvious way to do (1) is absent.  So let's use empty list for (2).
> 
> That leaves (3).  I think the simplest solution that could possibly work
> is to treat it as a special "dunno" reason: encode it just like (4), but
> with a special "dunno" list element.  I'd use the empty string.
> 
> Could even be used if we need to distinguish
> 
> (4a) No, and here's the *complete* list of reasons.
> (4b) No, and here's a possibly incomplete list of reasons.
> 
> For (4b), include the "dunno" element with the others.
> 
> Unlike the proposed solution, this one doesn't leave interface crud
> behind if we succeed in getting rid of (1) and (3):
> 
> * When (1) goes away, the single member becomes mandatory.
> 
> * When (3) goes away, the special "dunno" list element no longer occurs.

I like your suggestion.

I suggest "type" as the "dunno" element. It would keep the
existing "QOM property name" semantics, and it would just mean
"sorry, the only advice we can currently give you is to choose a
different CPU type". It even matches the previous documentation I
sent describing the meaning of read-only property names.

Rewriting the docs again:

 # Virtual CPU definition.
 #
 # @name: the name of the CPU definition
-# @runnable: #optional Whether the CPU model us usable with the
-#            current machine and accelerator. Omitted if we don't
-#            know the answer. (since 2.7)
-# @unavailable-features: #optional List of attributes that prevent
+# @unavailable-features: #optional List of properties that prevent
 #                        the CPU model from running in the current
-#                        host. Present only if @runnable is false.
-#                        (since 2.7)
+#                        host. (since 2.7)
 #
 # @unavailable-features is a list of QOM property names that
 # represent CPU model attributes that prevent the CPU from running.
-# If the QOM property is read-only, that means the CPU model can
-# never run in the current host. If the property is read-write, it
+# If the QOM property is read-only, that means there's no known
+# way to make the CPU model run in the current host.
+# If the property is read-write, it
 # means that it MAY be possible to run the CPU model in the current
 # host if that property is changed. Management software can use it
 # as hints to suggest or choose an alternative for the user, or
 # just to generate meaningful error messages explaining why the CPU
 # model can't be used.
+# If @unavailable-features is an empty list, the CPU model is
+# runnable using the current host and machine-type.
+# If @unavailable-features is not present, runnability
+# information for the CPU model is not available.
 #
 # Since: 1.2.0
 ##


-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
[...]
>> The new members encode an answer to the question whether a certain CPU
>> usable with the current machine an accelerator, and if no, why.
>> The possible answers are:
>> 
>> (1) Don't know.
>> (2) Yes.
>> (3) No, but we can't say why.
>> (4) No, and here's a list of reasons.
>> 
>> The two "dunno" answers (1) and (3) exist so we don't have to boil the
>> CPU ocean now.
>> 
>> Without them, the natural solution is a single member, where (4) is
>> encoded as nonempty list, and (2) could be encoded as empty list or
>> absent.
>> 
>> Now let me try to fit in (1) and (3).
>> 
>> The obvious way to do (1) is absent.  So let's use empty list for (2).
>> 
>> That leaves (3).  I think the simplest solution that could possibly work
>> is to treat it as a special "dunno" reason: encode it just like (4), but
>> with a special "dunno" list element.  I'd use the empty string.
>> 
>> Could even be used if we need to distinguish
>> 
>> (4a) No, and here's the *complete* list of reasons.
>> (4b) No, and here's a possibly incomplete list of reasons.
>> 
>> For (4b), include the "dunno" element with the others.
>> 
>> Unlike the proposed solution, this one doesn't leave interface crud
>> behind if we succeed in getting rid of (1) and (3):
>> 
>> * When (1) goes away, the single member becomes mandatory.
>> 
>> * When (3) goes away, the special "dunno" list element no longer occurs.
>
> I like your suggestion.
>
> I suggest "type" as the "dunno" element. It would keep the
> existing "QOM property name" semantics, and it would just mean
> "sorry, the only advice we can currently give you is to choose a
> different CPU type". It even matches the previous documentation I
> sent describing the meaning of read-only property names.
>
> Rewriting the docs again:
>
>  # Virtual CPU definition.
>  #
>  # @name: the name of the CPU definition
> -# @runnable: #optional Whether the CPU model us usable with the
> -#            current machine and accelerator. Omitted if we don't
> -#            know the answer. (since 2.7)
> -# @unavailable-features: #optional List of attributes that prevent
> +# @unavailable-features: #optional List of properties that prevent
>  #                        the CPU model from running in the current
> -#                        host. Present only if @runnable is false.
> -#                        (since 2.7)
> +#                        host. (since 2.7)
>  #
>  # @unavailable-features is a list of QOM property names that
>  # represent CPU model attributes that prevent the CPU from running.
> -# If the QOM property is read-only, that means the CPU model can
> -# never run in the current host. If the property is read-write, it
> +# If the QOM property is read-only, that means there's no known
> +# way to make the CPU model run in the current host.
> +# If the property is read-write, it
>  # means that it MAY be possible to run the CPU model in the current
>  # host if that property is changed. Management software can use it
>  # as hints to suggest or choose an alternative for the user, or
>  # just to generate meaningful error messages explaining why the CPU
>  # model can't be used.

Should we spell out the special case "type"?

> +# If @unavailable-features is an empty list, the CPU model is
> +# runnable using the current host and machine-type.
> +# If @unavailable-features is not present, runnability
> +# information for the CPU model is not available.
>  #
>  # Since: 1.2.0
>  ##

I'm happy with this interface.  Thanks!

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[Eduardo Habkost]

On Tue, May 31, 2016 at 03:24:50PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, May 30, 2016 at 11:33:38AM +0200, Markus Armbruster wrote:
> [...]
> >> The new members encode an answer to the question whether a certain CPU
> >> usable with the current machine an accelerator, and if no, why.
> >> The possible answers are:
> >> 
> >> (1) Don't know.
> >> (2) Yes.
> >> (3) No, but we can't say why.
> >> (4) No, and here's a list of reasons.
> >> 
> >> The two "dunno" answers (1) and (3) exist so we don't have to boil the
> >> CPU ocean now.
> >> 
> >> Without them, the natural solution is a single member, where (4) is
> >> encoded as nonempty list, and (2) could be encoded as empty list or
> >> absent.
> >> 
> >> Now let me try to fit in (1) and (3).
> >> 
> >> The obvious way to do (1) is absent.  So let's use empty list for (2).
> >> 
> >> That leaves (3).  I think the simplest solution that could possibly work
> >> is to treat it as a special "dunno" reason: encode it just like (4), but
> >> with a special "dunno" list element.  I'd use the empty string.
> >> 
> >> Could even be used if we need to distinguish
> >> 
> >> (4a) No, and here's the *complete* list of reasons.
> >> (4b) No, and here's a possibly incomplete list of reasons.
> >> 
> >> For (4b), include the "dunno" element with the others.
> >> 
> >> Unlike the proposed solution, this one doesn't leave interface crud
> >> behind if we succeed in getting rid of (1) and (3):
> >> 
> >> * When (1) goes away, the single member becomes mandatory.
> >> 
> >> * When (3) goes away, the special "dunno" list element no longer occurs.
> >
> > I like your suggestion.
> >
> > I suggest "type" as the "dunno" element. It would keep the
> > existing "QOM property name" semantics, and it would just mean
> > "sorry, the only advice we can currently give you is to choose a
> > different CPU type". It even matches the previous documentation I
> > sent describing the meaning of read-only property names.
> >
> > Rewriting the docs again:
> >
> >  # Virtual CPU definition.
> >  #
> >  # @name: the name of the CPU definition
> > -# @runnable: #optional Whether the CPU model us usable with the
> > -#            current machine and accelerator. Omitted if we don't
> > -#            know the answer. (since 2.7)
> > -# @unavailable-features: #optional List of attributes that prevent
> > +# @unavailable-features: #optional List of properties that prevent
> >  #                        the CPU model from running in the current
> > -#                        host. Present only if @runnable is false.
> > -#                        (since 2.7)
> > +#                        host. (since 2.7)
> >  #
> >  # @unavailable-features is a list of QOM property names that
> >  # represent CPU model attributes that prevent the CPU from running.
> > -# If the QOM property is read-only, that means the CPU model can
> > -# never run in the current host. If the property is read-write, it
> > +# If the QOM property is read-only, that means there's no known
> > +# way to make the CPU model run in the current host.
> > +# If the property is read-write, it
> >  # means that it MAY be possible to run the CPU model in the current
> >  # host if that property is changed. Management software can use it
> >  # as hints to suggest or choose an alternative for the user, or
> >  # just to generate meaningful error messages explaining why the CPU
> >  # model can't be used.
> 
> Should we spell out the special case "type"?

It is not exactly a special case (it is a read-only property like
any other), but it's worth mentioning. I will change it to:

# If the QOM property is read-only, that means there's no known
# way to make the CPU model run in the current host. If
# absolutely no extra information will be returned to explain why
# the CPU model is not runnable, implementations may simply
# return "type" as the property name.

> 
> > +# If @unavailable-features is an empty list, the CPU model is
> > +# runnable using the current host and machine-type.
> > +# If @unavailable-features is not present, runnability
> > +# information for the CPU model is not available.
> >  #
> >  # Since: 1.2.0
> >  ##
> 
> I'm happy with this interface.  Thanks!

Thanks!

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH 7/9] qmp: Add runnability information to query-cpu-definitions

[David Hildenbrand]

> It is not exactly a special case (it is a read-only property like
> any other), but it's worth mentioning. I will change it to:
> 
> # If the QOM property is read-only, that means there's no known
> # way to make the CPU model run in the current host. If
> # absolutely no extra information will be returned to explain why
> # the CPU model is not runnable, implementations may simply
> # return "type" as the property name.
> 
> >   
> > > +# If @unavailable-features is an empty list, the CPU model is
> > > +# runnable using the current host and machine-type.
> > > +# If @unavailable-features is not present, runnability
> > > +# information for the CPU model is not available.
> > >  #
> > >  # Since: 1.2.0
> > >  ##  
> > 
> > I'm happy with this interface.  Thanks!  
> 
> Thanks!
> 

Yes, sounds also good to me. For "hw generation too new" and similar errors
we will simply return "type". Thanks.

David

[Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Eduardo Habkost]

This makes the feature name tables in feature_word_info all match
the actual QOM property names we use.

This will make the command-line interface more consistent,
allowing the QOM property names to be used as "-cpu" arguments
directly.

Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
compatibility with the old that had underscores.

Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 309ef55..e7365d1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -187,7 +187,7 @@ 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",
@@ -207,17 +207,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,
 };
 
@@ -233,8 +233,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,
@@ -244,9 +244,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,
@@ -255,7 +255,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, "pcommit", "clflushopt",
     "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
@@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
     .set   = x86_set_hv_spinlocks,
 };
 
-/* Convert all '_' in a feature string option name to '-', to make feature
- * name conform to QOM property naming rule, which uses '-' instead of '_'.
+/* Convert all '_' in a feature string option name to '-', to keep compatibility
+ * with old feature names that used "_" instead of "-".
  */
 static inline void feat2prop(char *s)
 {
@@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     while (featurestr) {
         char *val;
         if (featurestr[0] == '+') {
+            feat2prop(featurestr);
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
         } else if (featurestr[0] == '-') {
+            feat2prop(featurestr);
             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
@@ -3137,11 +3139,9 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
 
     names = g_strsplit(fi->feat_names[bitnr], "|", 0);
 
-    feat2prop(names[0]);
     x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr);
 
     for (i = 1; names[i]; i++) {
-        feat2prop(names[i]);
         object_property_add_alias(obj, names[i], obj, names[0],
                                   &error_abort);
     }
-- 
2.5.5

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Igor Mammedov]

On Fri,  6 May 2016 15:11:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This makes the feature name tables in feature_word_info all match
> the actual QOM property names we use.
> 
> This will make the command-line interface more consistent,
> allowing the QOM property names to be used as "-cpu" arguments
> directly.

wouldn't that change output of '-cpu help',
can it break libvirt?


> Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> compatibility with the old that had underscores.
> 
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 309ef55..e7365d1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -187,7 +187,7 @@ 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",
> @@ -207,17 +207,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,
>  };
>  
> @@ -233,8 +233,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,
> @@ -244,9 +244,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,
> @@ -255,7 +255,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, "pcommit", "clflushopt",
>      "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
>      .set   = x86_set_hv_spinlocks,
>  };
>  
> -/* Convert all '_' in a feature string option name to '-', to make feature
> - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> + * with old feature names that used "_" instead of "-".
>   */
>  static inline void feat2prop(char *s)
>  {
> @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      while (featurestr) {
>          char *val;
>          if (featurestr[0] == '+') {
> +            feat2prop(featurestr);
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
>          } else if (featurestr[0] == '-') {
> +            feat2prop(featurestr);
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
>          } else if ((val = strchr(featurestr, '='))) {
>              *val = 0; val++;
> @@ -3137,11 +3139,9 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
>  
>      names = g_strsplit(fi->feat_names[bitnr], "|", 0);
>  
> -    feat2prop(names[0]);
>      x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr);
>  
>      for (i = 1; names[i]; i++) {
> -        feat2prop(names[i]);
>          object_property_add_alias(obj, names[i], obj, names[0],
>                                    &error_abort);
>      }

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Jiri Denemark]

On Tue, May 10, 2016 at 17:19:51 +0200, Igor Mammedov wrote:
> On Fri,  6 May 2016 15:11:31 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This makes the feature name tables in feature_word_info all match
> > the actual QOM property names we use.
> > 
> > This will make the command-line interface more consistent,
> > allowing the QOM property names to be used as "-cpu" arguments
> > directly.
> 
> wouldn't that change output of '-cpu help',
> can it break libvirt?

Don't worry, for any QEMU >= 1.2.0 libvirt uses only QMP commands when
probing capabilities. And currently we don't even parse feature names
anywhere, we only use feature names when constructing -cpu command line
and that parts seems to be covered by this patch.

Jirka

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Igor Mammedov]

On Fri,  6 May 2016 15:11:31 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This makes the feature name tables in feature_word_info all match
> the actual QOM property names we use.
> 
> This will make the command-line interface more consistent,
> allowing the QOM property names to be used as "-cpu" arguments
> directly.
> 
> Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> compatibility with the old that had underscores.
> 
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: libvir-list@redhat.com
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 309ef55..e7365d1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -187,7 +187,7 @@ 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",
> @@ -207,17 +207,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,
>  };
>  
> @@ -233,8 +233,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,
> @@ -244,9 +244,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,
> @@ -255,7 +255,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, "pcommit", "clflushopt",
>      "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
>      .set   = x86_set_hv_spinlocks,
>  };
>  
> -/* Convert all '_' in a feature string option name to '-', to make feature
> - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> + * with old feature names that used "_" instead of "-".
>   */
>  static inline void feat2prop(char *s)
>  {
> @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      while (featurestr) {
>          char *val;
I'd place a single feat2prop() here
and delete it from other call sites in this function.

>          if (featurestr[0] == '+') {
> +            feat2prop(featurestr);
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
>          } else if (featurestr[0] == '-') {
> +            feat2prop(featurestr);
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
>          } else if ((val = strchr(featurestr, '='))) {
>              *val = 0; val++;
> @@ -3137,11 +3139,9 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu,
>  
>      names = g_strsplit(fi->feat_names[bitnr], "|", 0);
>  
> -    feat2prop(names[0]);
>      x86_cpu_register_bit_prop(cpu, names[0], &cpu->env.features[w], bitnr);
>  
>      for (i = 1; names[i]; i++) {
> -        feat2prop(names[i]);
>          object_property_add_alias(obj, names[i], obj, names[0],
>                                    &error_abort);
>      }

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Eduardo Habkost]

On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> On Fri,  6 May 2016 15:11:31 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This makes the feature name tables in feature_word_info all match
> > the actual QOM property names we use.
> > 
> > This will make the command-line interface more consistent,
> > allowing the QOM property names to be used as "-cpu" arguments
> > directly.
> > 
> > Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> > compatibility with the old that had underscores.
> > 
> > Cc: Jiri Denemark <jdenemar@redhat.com>
> > Cc: libvir-list@redhat.com
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 32 ++++++++++++++++----------------
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 309ef55..e7365d1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -187,7 +187,7 @@ 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",
> > @@ -207,17 +207,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,
> >  };
> >  
> > @@ -233,8 +233,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,
> > @@ -244,9 +244,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,
> > @@ -255,7 +255,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, "pcommit", "clflushopt",
> >      "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> > @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
> >      .set   = x86_set_hv_spinlocks,
> >  };
> >  
> > -/* Convert all '_' in a feature string option name to '-', to make feature
> > - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> > +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> > + * with old feature names that used "_" instead of "-".
> >   */
> >  static inline void feat2prop(char *s)
> >  {
> > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >      while (featurestr) {
> >          char *val;
> I'd place a single feat2prop() here
> and delete it from other call sites in this function.

A previous version of this patch had it. But it would change the
property value too, not just the property name (breaking stuff
like "model-id=some_string").

-- 
Eduardo

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Igor Mammedov]

On Tue, 24 May 2016 09:34:05 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> > On Fri,  6 May 2016 15:11:31 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > This makes the feature name tables in feature_word_info all match
> > > the actual QOM property names we use.
> > > 
> > > This will make the command-line interface more consistent,
> > > allowing the QOM property names to be used as "-cpu" arguments
> > > directly.
> > > 
> > > Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> > > compatibility with the old that had underscores.
> > > 
> > > Cc: Jiri Denemark <jdenemar@redhat.com>
> > > Cc: libvir-list@redhat.com
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 32 ++++++++++++++++----------------
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 309ef55..e7365d1 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -187,7 +187,7 @@ 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",
> > > @@ -207,17 +207,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,
> > >  };
> > >  
> > > @@ -233,8 +233,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,
> > > @@ -244,9 +244,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,
> > > @@ -255,7 +255,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, "pcommit", "clflushopt",
> > >      "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> > > @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
> > >      .set   = x86_set_hv_spinlocks,
> > >  };
> > >  
> > > -/* Convert all '_' in a feature string option name to '-', to make feature
> > > - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> > > +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> > > + * with old feature names that used "_" instead of "-".
> > >   */
> > >  static inline void feat2prop(char *s)
> > >  {
> > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >      while (featurestr) {
> > >          char *val;  
> > I'd place a single feat2prop() here
> > and delete it from other call sites in this function.  
> 
> A previous version of this patch had it. But it would change the
> property value too, not just the property name (breaking stuff
> like "model-id=some_string").
>
it's bug in feat2prop(), which probably should be fixed there,
so it would do what comment above it says. Or as alternative:


diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ca2a893..e46e4c3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
-        char *val;
+        char *val = strchr(featurestr, '=');
+        if (val) {
+            *val = 0; 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);
+        } else if (val) {
             if (!strcmp(featurestr, "xlevel")) {
                 char *err;
                 char num[32];
@@ -2000,7 +2002,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) {

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Eduardo Habkost]

On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
> On Tue, 24 May 2016 09:34:05 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> > > On Fri,  6 May 2016 15:11:31 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > > -/* Convert all '_' in a feature string option name to '-', to make feature
> > > > - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> > > > +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> > > > + * with old feature names that used "_" instead of "-".
> > > >   */
> > > >  static inline void feat2prop(char *s)
> > > >  {
> > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > > >      while (featurestr) {
> > > >          char *val;  
> > > I'd place a single feat2prop() here
> > > and delete it from other call sites in this function.  
> > 
> > A previous version of this patch had it. But it would change the
> > property value too, not just the property name (breaking stuff
> > like "model-id=some_string").
> >
> it's bug in feat2prop(), which probably should be fixed there,
> so it would do what comment above it says. Or as alternative:

The comment above it doesn't say anything about stopping at a '='
delimiter. I always expected it to just replace "_" with "-" in a
null-terminated string.

(I am not completely against making it stop at '=', but I believe
your suggestion below sounds better).

> 
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ca2a893..e46e4c3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
> -        char *val;
> +        char *val = strchr(featurestr, '=');
> +        if (val) {
> +            *val = 0; val++;
> +        }
> +        feat2prop(featurestr);

This would make "+feature=FOO" treated as a valid option, and it
isn't. It would keep the existing behavior if we did this:

-          if (featurestr[0] == '+') {
+          if (featurestr[0] == '+' && !val) {
               add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
-          } else if (featurestr[0] == '-') {
+          if (featurestr[0] == '+' && !val) {
               add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);

In either case, I prefer to get this optimization reviewed as a
separate patch. Can you send it as a follow-up?

> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> -            feat2prop(featurestr);
> +        } else if (val) {
>              if (!strcmp(featurestr, "xlevel")) {
>                  char *err;
>                  char num[32];
> @@ -2000,7 +2002,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) {
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

[Igor Mammedov]

On Fri, 27 May 2016 17:32:34 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
> > On Tue, 24 May 2016 09:34:05 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  6 May 2016 15:11:31 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> [...]
> > > > > -/* Convert all '_' in a feature string option name to '-', to make feature
> > > > > - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> > > > > +/* Convert all '_' in a feature string option name to '-', to keep compatibility
> > > > > + * with old feature names that used "_" instead of "-".
> > > > >   */
> > > > >  static inline void feat2prop(char *s)
> > > > >  {
> > > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > > > >      while (featurestr) {
> > > > >          char *val;    
> > > > I'd place a single feat2prop() here
> > > > and delete it from other call sites in this function.    
> > > 
> > > A previous version of this patch had it. But it would change the
> > > property value too, not just the property name (breaking stuff
> > > like "model-id=some_string").
> > >  
> > it's bug in feat2prop(), which probably should be fixed there,
> > so it would do what comment above it says. Or as alternative:  
> 
> The comment above it doesn't say anything about stopping at a '='
> delimiter. I always expected it to just replace "_" with "-" in a
> null-terminated string.
> 
> (I am not completely against making it stop at '=', but I believe
> your suggestion below sounds better).
> 
> > 
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ca2a893..e46e4c3 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features
> >      featurestr = features ? strtok(features, ",") : NULL;
> >  
> >      while (featurestr) {
> > -        char *val;
> > +        char *val = strchr(featurestr, '=');
> > +        if (val) {
> > +            *val = 0; val++;
> > +        }
> > +        feat2prop(featurestr);  
> 
> This would make "+feature=FOO" treated as a valid option, and it
> isn't. It would keep the existing behavior if we did this:
> 
> -          if (featurestr[0] == '+') {
> +          if (featurestr[0] == '+' && !val) {
>                add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> -          } else if (featurestr[0] == '-') {
> +          if (featurestr[0] == '+' && !val) {
>                add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> 
> In either case, I prefer to get this optimization reviewed as a
> separate patch. Can you send it as a follow-up?
sure

> 
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > -            feat2prop(featurestr);
> > +        } else if (val) {
> >              if (!strcmp(featurestr, "xlevel")) {
> >                  char *err;
> >                  char num[32];
> > @@ -2000,7 +2002,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) {
> > 
> >   
> 

[Qemu-devel] [PATCH 9/9] target-i386: Return runnability information on query-cpu-definitions

[Eduardo Habkost]

Fill the "runnable" and "unavailable-features" fields on the x86
implementation of query-cpu-definitions.

Example command output:

  {
      "return": [
          { "runnable": true, "name": "host"},
          { "runnable": true, "name": "qemu64"},
          { "runnable": true, "name": "qemu32"},
          { "runnable": false, "name": "phenom",
            "unavailable-features": ["npt", "sse4a", "3dnow", "3dnowext", "fxsr-opt", "mmxext"] },
          { "runnable": true, "name": "pentium3"},
          { "runnable": true, "name": "pentium2"},
          { "runnable": true, "name": "pentium"},
          { "runnable": true, "name": "n270"},
          { "runnable": true, "name": "kvm64"},
          { "runnable": true, "name": "kvm32"},
          { "runnable": true, "name": "coreduo"},
          { "runnable": true, "name": "core2duo"},
          { "runnable": false, "name": "athlon",
            "unavailable-features": ["3dnow", "3dnowext", "mmxext"] },
          { "runnable": true, "name": "Westmere"},
          { "runnable": true, "name": "SandyBridge"},
          { "runnable": true, "name": "Penryn"},
          { "runnable": false, "name": "Opteron_G5",
            "unavailable-features": ["tbm", "fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"] },
          { "runnable": false, "name": "Opteron_G4",
            "unavailable-features": ["fma4", "xop", "3dnowprefetch", "misalignsse", "sse4a"] },
          { "runnable": false, "name": "Opteron_G3",
            "unavailable-features": ["misalignsse", "sse4a"] },
          { "runnable": true, "name": "Opteron_G2"},
          { "runnable": true, "name": "Opteron_G1"},
          { "runnable": true, "name": "Nehalem"},
          { "runnable": true, "name": "IvyBridge"},
          { "runnable": false, "name": "Haswell",
            "unavailable-features": ["rtm", "hle"] },
          { "runnable": true, "name": "Haswell-noTSX"},
          { "runnable": true, "name": "Conroe"},
          { "runnable": false, "name": "Broadwell",
            "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed", "rtm", "hle"] },
          { "runnable": false, "name": "Broadwell-noTSX",
            "unavailable-features": ["3dnowprefetch", "smap", "adx", "rdseed"] },
          { "runnable": true, "name": "486"}
      ]
  }

Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: libvir-list@redhat.com
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e7365d1..f6f0f83 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2041,6 +2041,41 @@ static void x86_cpu_report_filtered_features(X86CPU *cpu)
     }
 }
 
+static bool x86_cpu_class_is_runnable(X86CPUClass *xcc, strList **missing_feats)
+{
+    X86CPU *xc;
+    bool r;
+    FeatureWord w;
+
+    if (xcc->kvm_required && !kvm_enabled()) {
+        return false;
+    }
+
+    xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
+    r = !x86_cpu_filter_features(xc);
+    if (!r) {
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            FeatureWordInfo *fi = &feature_word_info[w];
+            uint32_t filtered = xc->filtered_features[w];
+            int i;
+            for (i = 0; i < 32; i++) {
+                if (filtered & (1UL << i)) {
+                    char **parts = g_strsplit(fi->feat_names[i], "|", 2);
+                    strList *new = g_new0(strList, 1);
+                    new->value = g_strdup(parts[0]);
+                    feat2prop(new->value);
+                    new->next = *missing_feats;
+                    *missing_feats = new;
+                    g_strfreev(parts);
+                }
+            }
+        }
+    }
+
+    object_unref(OBJECT(xc));
+    return r;
+}
+
 /* Print all cpuid feature names in featureset
  */
 static void listflags(FILE *f, fprintf_function print, const char **featureset)
@@ -2133,6 +2168,11 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 
     info = g_malloc0(sizeof(*info));
     info->name = x86_cpu_class_get_model_name(cc);
+    info->has_runnable = true;
+    info->runnable = x86_cpu_class_is_runnable(cc, &info->unavailable_features);
+    if (!info->runnable) {
+        info->has_unavailable_features = true;
+    }
 
     entry = g_malloc0(sizeof(*entry));
     entry->value = info;
-- 
2.5.5

Re: [Qemu-devel] [PATCH 0/9] Add runnability info to query-cpu-definitions

[David Hildenbrand]

> This series extends query-cpu-definitions to include two extra
> fields: "runnable", and "unavailable-features".
> 
> This will return information based on the current machine and
> accelerator only. In the future we may extend these mechanisms to
> allow querying other machines and other accelerators without
> restarting QEMU, but it will require some reorganization of
> QEMU's main code.

Hi Eduardo,

just as discussed during/after the last kvm forum offline, we are currently
working on a new set of qmp functions to keep all the cpu model details
in QEMU and not have to replicate them in libvirt and to have a nice way
to query the host cpu model.

We already have a prototype running, just need to clarify some s390x details
(feature set of the CPU generations) before we can send out an RFC. I could,
however, send out just the interface changes + details on the concept upfront,
if anybody is interested.

In the current implementation:
- s390x cpu models will be independent of the QEMU machine, so for s390x the
  runnability information will only depend on the accelerator (host cpu model).
  However, for compatibility machines, cpu models will be disabled, and we won't
  therefore have any runnability information.
- we will not touch query-cpu-definitions
- the new interfaces will also make use of the current accelerator and the
  current machine, so the context of these functions are the same.
  
Adding functionality to query for other machines is not necessary for s390x.
Other accelerators don't really make sense to me, let's keep it simple here.

So this change is fine from my point of view. It doesn't contradict to out
concept.

David