[PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Eduardo Habkost]

We had introduced versioned CPU models in QEMU 4.1, including a
method for querying for CPU model versions using
query-cpu-definitions.  This only has one problem: fetching CPU
alias information for multiple machine types required restarting
QEMU for each machine being queried.

This series adds a new `machine` parameter to
query-cpu-definitions, that can be used to query CPU model alias
information for multiple machines without restarting QEMU.

Eduardo Habkost (7):
  i386: Use g_autofree at x86_cpu_list_entry()
  i386: Add default_version parameter to CPU version functions
  i386: Don't use default_cpu_version at "-cpu help"
  machine: machine_find_class() function
  i386: Remove x86_cpu_set_default_version() function
  i386: Don't use default_cpu_version() inside query-cpu-definitions
  cpu: Add `machine` parameter to query-cpu-definitions

 qapi/machine-target.json                   | 14 +++-
 include/hw/boards.h                        |  1 +
 include/hw/i386/pc.h                       |  5 +-
 target/i386/cpu.h                          |  6 --
 hw/core/machine.c                          | 16 ++++
 hw/i386/pc.c                               |  3 -
 target/arm/helper.c                        |  4 +-
 target/i386/cpu.c                          | 93 +++++++++++++++-------
 target/mips/helper.c                       |  4 +-
 target/ppc/translate_init.inc.c            |  4 +-
 target/s390x/cpu_models.c                  |  4 +-
 vl.c                                       | 17 +---
 tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++
 13 files changed, 154 insertions(+), 59 deletions(-)

-- 
2.21.0

[PATCH 1/7] i386: Use g_autofree at x86_cpu_list_entry()

[Eduardo Habkost]

Make the code shorter and simpler.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0de8a22e1e..5b7c5b1177 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3976,9 +3976,9 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
 {
     ObjectClass *oc = data;
     X86CPUClass *cc = X86_CPU_CLASS(oc);
-    char *name = x86_cpu_class_get_model_name(cc);
-    char *desc = g_strdup(cc->model_description);
-    char *alias_of = x86_cpu_class_get_alias_of(cc);
+    g_autofree char *name = x86_cpu_class_get_model_name(cc);
+    g_autofree char *desc = g_strdup(cc->model_description);
+    g_autofree char *alias_of = x86_cpu_class_get_alias_of(cc);
 
     if (!desc && alias_of) {
         if (cc->model && cc->model->version == CPU_VERSION_AUTO) {
@@ -3992,9 +3992,6 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
     }
 
     qemu_printf("x86 %-20s  %-48s\n", name, desc);
-    g_free(name);
-    g_free(desc);
-    g_free(alias_of);
 }
 
 /* list available CPU models and flags */
-- 
2.21.0

[PATCH 2/7] i386: Add default_version parameter to CPU version functions

[Eduardo Habkost]

Not all CPU version lookup code will use default_cpu_version:
we'll change query-cpu-definitions to optionally get a machine
type argument.  Make CPU version resolving functions get an
explicit default_version argument.

All callers are being changed to use default_cpu_version as
argument, so no behavior is being changed yet.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5b7c5b1177..843f8c4b68 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3187,11 +3187,12 @@ static X86CPUVersion x86_cpu_model_last_version(const X86CPUModel *model)
 }
 
 /* Return the actual version being used for a specific CPU model */
-static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model)
+static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model,
+                                                   X86CPUVersion default_version)
 {
     X86CPUVersion v = model->version;
     if (v == CPU_VERSION_AUTO) {
-        v = default_cpu_version;
+        v = default_version;
     }
     if (v == CPU_VERSION_LATEST) {
         return x86_cpu_model_last_version(model);
@@ -3958,14 +3959,15 @@ static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
     return r;
 }
 
-static char *x86_cpu_class_get_alias_of(X86CPUClass *cc)
+static char *x86_cpu_class_get_alias_of(X86CPUClass *cc,
+                                        X86CPUVersion default_version)
 {
     X86CPUVersion version;
 
     if (!cc->model || !cc->model->is_alias) {
         return NULL;
     }
-    version = x86_cpu_model_resolve_version(cc->model);
+    version = x86_cpu_model_resolve_version(cc->model, default_version);
     if (version <= 0) {
         return NULL;
     }
@@ -3978,7 +3980,7 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
     X86CPUClass *cc = X86_CPU_CLASS(oc);
     g_autofree char *name = x86_cpu_class_get_model_name(cc);
     g_autofree char *desc = g_strdup(cc->model_description);
-    g_autofree char *alias_of = x86_cpu_class_get_alias_of(cc);
+    g_autofree char *alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version);
 
     if (!desc && alias_of) {
         if (cc->model && cc->model->version == CPU_VERSION_AUTO) {
@@ -4045,7 +4047,7 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
      * doesn't break compatibility with previous QEMU versions.
      */
     if (default_cpu_version != CPU_VERSION_LEGACY) {
-        info->alias_of = x86_cpu_class_get_alias_of(cc);
+        info->alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version);
         info->has_alias_of = !!info->alias_of;
     }
 
@@ -4116,7 +4118,7 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model)
 {
     const X86CPUVersionDefinition *vdef;
-    X86CPUVersion version = x86_cpu_model_resolve_version(model);
+    X86CPUVersion version = x86_cpu_model_resolve_version(model, default_cpu_version);
 
     if (version == CPU_VERSION_LEGACY) {
         return;
-- 
2.21.0

[PATCH 3/7] i386: Don't use default_cpu_version at "-cpu help"

[Eduardo Habkost]

The output of "-cpu help" doesn't change depending on the machine
type, already.  We can remove usage of default_cpu_version and
keep output exactly the same.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 843f8c4b68..8cecc669b3 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3980,10 +3980,11 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
     X86CPUClass *cc = X86_CPU_CLASS(oc);
     g_autofree char *name = x86_cpu_class_get_model_name(cc);
     g_autofree char *desc = g_strdup(cc->model_description);
-    g_autofree char *alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version);
 
-    if (!desc && alias_of) {
-        if (cc->model && cc->model->version == CPU_VERSION_AUTO) {
+    if (!desc && cc->model && cc->model->is_alias) {
+        g_autofree char *alias_of =
+            x86_cpu_class_get_alias_of(cc, CPU_VERSION_AUTO);
+        if (!alias_of) {
             desc = g_strdup("(alias configured by machine type)");
         } else {
             desc = g_strdup_printf("(alias of %s)", alias_of);
-- 
2.21.0

[PATCH 4/7] machine: machine_find_class() function

[Eduardo Habkost]

Move find_machine() from vl.c to core/machine.c and rename it to
machine_find_class(), so it can be reused by other code.

The function won't reuse the results of the previous
object_class_get_list() call like it did in vl.c, but this
shouldn't be a problem because the function is expected to be
called only once during regular QEMU usage.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/boards.h |  1 +
 hw/core/machine.c   | 16 ++++++++++++++++
 vl.c                | 17 +----------------
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087f34..0ab7138c63 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -76,6 +76,7 @@ void machine_set_cpu_numa_node(MachineState *machine,
                                Error **errp);
 
 void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
+MachineClass *machine_find_class(const char *name);
 
 
 /**
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8..53dae1cd08 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1143,6 +1143,22 @@ void machine_run_board_init(MachineState *machine)
     machine_class->init(machine);
 }
 
+MachineClass *machine_find_class(const char *name)
+{
+    g_autoptr(GSList) machines = object_class_get_list(TYPE_MACHINE, false);
+    GSList *el;
+
+    for (el = machines; el; el = el->next) {
+        MachineClass *mc = el->data;
+
+        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
+            return mc;
+        }
+    }
+
+    return NULL;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/vl.c b/vl.c
index 4489cfb2bb..8901455ee7 100644
--- a/vl.c
+++ b/vl.c
@@ -1306,21 +1306,6 @@ static int usb_parse(const char *cmdline)
 
 MachineState *current_machine;
 
-static MachineClass *find_machine(const char *name, GSList *machines)
-{
-    GSList *el;
-
-    for (el = machines; el; el = el->next) {
-        MachineClass *mc = el->data;
-
-        if (!strcmp(mc->name, name) || !g_strcmp0(mc->alias, name)) {
-            return mc;
-        }
-    }
-
-    return NULL;
-}
-
 static MachineClass *find_default_machine(GSList *machines)
 {
     GSList *el;
@@ -2485,7 +2470,7 @@ static MachineClass *machine_parse(const char *name, GSList *machines)
         exit(0);
     }
 
-    mc = find_machine(name, machines);
+    mc = machine_find_class(name);
     if (!mc) {
         error_report("unsupported machine type");
         error_printf("Use -machine help to list supported machines\n");
-- 
2.21.0

[PATCH 5/7] i386: Remove x86_cpu_set_default_version() function

[Eduardo Habkost]

We will introduce code that will return machine-type-specific
from other machines (not the current one), so we'll need a helper
for getting the default CPU version from a machine class.

With the new helper, we don't need the machine init function to
call x86_cpu_set_default_version() anymore: we can just look at
the machine class of the current machine.  Replace the
default_cpu_version static variable with a default_cpu_version()
function that will look at qdev_get_machine().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/pc.h |  5 ++++-
 target/i386/cpu.h    |  6 ------
 hw/i386/pc.c         |  3 ---
 target/i386/cpu.c    | 28 ++++++++++++++++++++--------
 4 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 37bfd95113..00ac726ebc 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -113,7 +113,10 @@ typedef struct PCMachineClass {
 
     /* Compat options: */
 
-    /* Default CPU model version.  See x86_cpu_set_default_version(). */
+    /*
+     * Default CPU model version for CPU models having
+     * version == CPU_VERSION_AUTO.
+     */
     int default_cpu_version;
 
     /* ACPI compat: */
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index cedb5bc205..aa17c79b43 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2168,12 +2168,6 @@ void x86_cpu_change_kvm_default(const char *prop, const char *value);
 
 typedef int X86CPUVersion;
 
-/*
- * Set default CPU model version for CPU models having
- * version == CPU_VERSION_AUTO.
- */
-void x86_cpu_set_default_version(X86CPUVersion version);
-
 /* Return name of 32-bit register, from a R_* constant */
 const char *get_register_name_32(unsigned int reg);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4b1904237e..64ec995172 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1503,9 +1503,6 @@ void pc_cpus_init(PCMachineState *pcms)
     const CPUArchIdList *possible_cpus;
     MachineState *ms = MACHINE(pcms);
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
-    PCMachineClass *pcmc = PC_MACHINE_CLASS(mc);
-
-    x86_cpu_set_default_version(pcmc->default_cpu_version);
 
     /* Calculates the limit to CPU APIC ID values
      *
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 8cecc669b3..5dbd379331 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -57,6 +57,7 @@
 #include "hw/xen/xen.h"
 #include "hw/i386/apic_internal.h"
 #include "hw/boards.h"
+#include "hw/i386/pc.h"
 #endif
 
 #include "disas/capstone.h"
@@ -3165,14 +3166,25 @@ static PropValue tcg_default_props[] = {
 };
 
 
-X86CPUVersion default_cpu_version = CPU_VERSION_LATEST;
+#ifdef CONFIG_USER_ONLY
+static X86CPUVersion default_cpu_version(void)
+{
+    return CPU_VERSION_LATEST;
+}
+#else
+static X86CPUVersion default_cpu_version_for_machine(MachineClass *mc)
+{
+    PCMachineClass *pcmc =
+        (PCMachineClass *)object_class_dynamic_cast(OBJECT_CLASS(mc), TYPE_PC_MACHINE);
+    return pcmc ? pcmc->default_cpu_version : CPU_VERSION_LATEST;
+}
 
-void x86_cpu_set_default_version(X86CPUVersion version)
+static X86CPUVersion default_cpu_version(void)
 {
-    /* Translating CPU_VERSION_AUTO to CPU_VERSION_AUTO doesn't make sense */
-    assert(version != CPU_VERSION_AUTO);
-    default_cpu_version = version;
+    return default_cpu_version_for_machine(MACHINE_GET_CLASS(qdev_get_machine()));
 }
+#endif
+
 
 static X86CPUVersion x86_cpu_model_last_version(const X86CPUModel *model)
 {
@@ -4047,8 +4059,8 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
      * Old machine types won't report aliases, so that alias translation
      * doesn't break compatibility with previous QEMU versions.
      */
-    if (default_cpu_version != CPU_VERSION_LEGACY) {
-        info->alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version);
+    if (default_cpu_version() != CPU_VERSION_LEGACY) {
+        info->alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version());
         info->has_alias_of = !!info->alias_of;
     }
 
@@ -4119,7 +4131,7 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
 static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUModel *model)
 {
     const X86CPUVersionDefinition *vdef;
-    X86CPUVersion version = x86_cpu_model_resolve_version(model, default_cpu_version);
+    X86CPUVersion version = x86_cpu_model_resolve_version(model, default_cpu_version());
 
     if (version == CPU_VERSION_LEGACY) {
         return;
-- 
2.21.0

[PATCH 6/7] i386: Don't use default_cpu_version() inside query-cpu-definitions

[Eduardo Habkost]

We will change query-cpu-definitions to have a new `machine`
parameter.  Make the machine-specific parts of that code explicit
instead of calling default_cpu_version(), so we can change it to
use the new parameter later.

As the code now has a dependency on MachineClass, wrap it inside
a !CONFIG_USER_ONLY ifdef.  The function was never used by
*-user, anyway.

This patch shouldn't introduce any behavior change.  Results of
query-cpu-definition will be exactly the same.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 5dbd379331..67d1eca4ed 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3877,6 +3877,7 @@ static void x86_cpu_get_unavailable_features(Object *obj, Visitor *v,
     visit_type_strList(v, "unavailable-features", &result, errp);
 }
 
+#ifndef CONFIG_USER_ONLY
 /* Check for missing features that may prevent the CPU class from
  * running using the current machine and accelerator.
  */
@@ -3914,6 +3915,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
 
     object_unref(OBJECT(xc));
 }
+#endif
 
 /* Print all cpuid feature names in featureset
  */
@@ -4039,11 +4041,17 @@ void x86_cpu_list(void)
     g_list_free(names);
 }
 
+#ifndef CONFIG_USER_ONLY
+typedef struct X86CPUDefinitionArgs {
+    CpuDefinitionInfoList *cpu_list;
+    X86CPUVersion default_version;
+} X86CPUDefinitionArgs;
+
 static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
 {
     ObjectClass *oc = data;
     X86CPUClass *cc = X86_CPU_CLASS(oc);
-    CpuDefinitionInfoList **cpu_list = user_data;
+    X86CPUDefinitionArgs *args = user_data;
     CpuDefinitionInfoList *entry;
     CpuDefinitionInfo *info;
 
@@ -4059,25 +4067,30 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
      * Old machine types won't report aliases, so that alias translation
      * doesn't break compatibility with previous QEMU versions.
      */
-    if (default_cpu_version() != CPU_VERSION_LEGACY) {
-        info->alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version());
+    if (args->default_version != CPU_VERSION_LEGACY) {
+        info->alias_of = x86_cpu_class_get_alias_of(cc, args->default_version);
         info->has_alias_of = !!info->alias_of;
     }
 
     entry = g_malloc0(sizeof(*entry));
     entry->value = info;
-    entry->next = *cpu_list;
-    *cpu_list = entry;
+    entry->next = args->cpu_list;
+    args->cpu_list = entry;
 }
 
 CpuDefinitionInfoList *qmp_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);
+    X86CPUDefinitionArgs args = { .cpu_list = NULL };
+    GSList *list;
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    args.default_version = default_cpu_version_for_machine(mc);
+    list = get_sorted_cpu_model_list();
+    g_slist_foreach(list, x86_cpu_definition_entry, &args);
     g_slist_free(list);
-    return cpu_list;
+    return args.cpu_list;
 }
+#endif
 
 static uint64_t x86_cpu_get_supported_feature_word(FeatureWord w,
                                                    bool migratable_only)
-- 
2.21.0

[PATCH 7/7] cpu: Add `machine` parameter to query-cpu-definitions

[Eduardo Habkost]

The new parameter can be used by management software to query for
CPU model alias information for multiple machines without
restarting QEMU.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qapi/machine-target.json                   | 14 +++++++-
 target/arm/helper.c                        |  4 ++-
 target/i386/cpu.c                          | 16 +++++++--
 target/mips/helper.c                       |  4 ++-
 target/ppc/translate_init.inc.c            |  4 ++-
 target/s390x/cpu_models.c                  |  4 ++-
 tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++++++++++++++
 7 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 55310a6aa2..7bff3811fe 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -281,6 +281,10 @@
 #
 # @alias-of: Name of CPU model this model is an alias for.  The target of the
 #            CPU model alias may change depending on the machine type.
+#            If the @machine argument was provided to query-cpu-definitions,
+#            alias information that machine type will be returned.
+#            If @machine is not provided, alias information for
+#            the current machine will be returned.
 #            Management software is supposed to translate CPU model aliases
 #            in the VM configuration, because aliases may stop being
 #            migration-safe in the future (since 4.1)
@@ -317,9 +321,17 @@
 #
 # Return a list of supported virtual CPU definitions
 #
+# @machine: Name of machine type.  The command returns some data
+#           that machine-specific.  This overrides the machine type
+#           used to look up that information.  This can be used,
+#           for example, to query machine-specific CPU model aliases
+#           without restarting QEMU (since 4.2)
+#
 # Returns: a list of CpuDefInfo
 #
 # Since: 1.2.0
 ##
-{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
+{ 'command': 'query-cpu-definitions',
+  'data': { '*machine': 'str' },
+  'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0d9a2d2ab7..96f9fe7fff 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6965,7 +6965,9 @@ static void arm_cpu_add_definition(gpointer data, gpointer user_data)
     *cpu_list = entry;
 }
 
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
+                                                 const char *machine,
+                                                 Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
     GSList *list;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 67d1eca4ed..ae633793ed 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4078,11 +4078,23 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
     args->cpu_list = entry;
 }
 
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
+                                                 const char *machine,
+                                                 Error **errp)
 {
     X86CPUDefinitionArgs args = { .cpu_list = NULL };
     GSList *list;
-    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+    MachineClass *mc;
+
+    if (!has_machine) {
+        mc = MACHINE_GET_CLASS(qdev_get_machine());
+    } else {
+        mc = machine_find_class(machine);
+        if (!mc) {
+            error_setg(errp, "Machine type '%s' not found", machine);
+            return NULL;
+        }
+    }
 
     args.default_version = default_cpu_version_for_machine(mc);
     list = get_sorted_cpu_model_list();
diff --git a/target/mips/helper.c b/target/mips/helper.c
index a2b6459b05..a73c767462 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -1481,7 +1481,9 @@ static void mips_cpu_add_definition(gpointer data, gpointer user_data)
     *cpu_list = entry;
 }
 
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
+                                                 const char *machine,
+                                                 Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
     GSList *list;
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index ba726dec4d..4493309c4c 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10350,7 +10350,9 @@ static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
     *first = entry;
 }
 
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
+                                                 const char *machine,
+                                                 Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
     GSList *list;
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2e15..e40f1f6bec 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -456,7 +456,9 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
     *cpu_list = entry;
 }
 
-CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
+CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
+                                                 const char *machine,
+                                                 Error **errp)
 {
     struct CpuDefinitionInfoListData list_data = {
         .list = NULL,
diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
index 5fc9ca4bc6..79c5250243 100644
--- a/tests/acceptance/x86_cpu_model_versions.py
+++ b/tests/acceptance/x86_cpu_model_versions.py
@@ -234,6 +234,48 @@ class X86CPUModelAliases(avocado_qemu.Test):
 
         self.validate_aliases(cpus)
 
+    def test_machine_arg_none(self):
+        """Check if unversioned CPU model is an alias pointing to right version"""
+        vm1 = self.get_vm()
+        vm1.add_args('-S')
+        vm1.set_machine('pc-i440fx-4.0')
+        vm1.launch()
+        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='none'))
+        vm1.shutdown()
+
+        vm2 = self.get_vm()
+        vm2.add_args('-S')
+        vm2.set_machine('none')
+        vm2.launch()
+        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))
+        vm1.shutdown()
+
+        self.assertEquals(cpus1, cpus2)
+
+    def test_machine_arg_4_1(self):
+        """Check if unversioned CPU model is an alias pointing to right version"""
+        vm1 = self.get_vm()
+        vm1.add_args('-S')
+        vm1.set_machine('pc-i440fx-4.0')
+        vm1.launch()
+        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='pc-i440fx-4.1'))
+        vm1.shutdown()
+
+        vm2 = self.get_vm()
+        vm2.add_args('-S')
+        vm2.set_machine('pc-i440fx-4.1')
+        vm2.launch()
+        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))
+        vm1.shutdown()
+
+        self.assertEquals(cpus1, cpus2)
+
+    def test_invalid_machine(self):
+        self.vm.add_args('-S')
+        self.vm.launch()
+        r = self.vm.qmp('query-cpu-definitions', machine='invalid-machine-123')
+        self.assertIn('error', r)
+
 
 class CascadelakeArchCapabilities(avocado_qemu.Test):
     """
-- 
2.21.0

Re: [PATCH 7/7] cpu: Add `machine` parameter to query-cpu-definitions

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> The new parameter can be used by management software to query for
> CPU model alias information for multiple machines without
> restarting QEMU.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qapi/machine-target.json                   | 14 +++++++-
>  target/arm/helper.c                        |  4 ++-
>  target/i386/cpu.c                          | 16 +++++++--
>  target/mips/helper.c                       |  4 ++-
>  target/ppc/translate_init.inc.c            |  4 ++-
>  target/s390x/cpu_models.c                  |  4 ++-
>  tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++++++++++++++
>  7 files changed, 81 insertions(+), 7 deletions(-)
>
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 55310a6aa2..7bff3811fe 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -281,6 +281,10 @@

This is CpuDefinitionInfo.

>  #
>  # @alias-of: Name of CPU model this model is an alias for.  The target of the
>  #            CPU model alias may change depending on the machine type.
> +#            If the @machine argument was provided to query-cpu-definitions,
> +#            alias information that machine type will be returned.

"for that machine type"

> +#            If @machine is not provided, alias information for
> +#            the current machine will be returned.
>  #            Management software is supposed to translate CPU model aliases
>  #            in the VM configuration, because aliases may stop being
>  #            migration-safe in the future (since 4.1)
> @@ -317,9 +321,17 @@
   ##
   # @query-cpu-definitions:
>  #
>  # Return a list of supported virtual CPU definitions
>  #
> +# @machine: Name of machine type.  The command returns some data
> +#           that machine-specific.  This overrides the machine type

"that is machine-specific"

> +#           used to look up that information.  This can be used,
> +#           for example, to query machine-specific CPU model aliases
> +#           without restarting QEMU (since 4.2)
> +#
>  # Returns: a list of CpuDefInfo
>  #
>  # Since: 1.2.0
>  ##
> -{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> +{ 'command': 'query-cpu-definitions',
> +  'data': { '*machine': 'str' },
> +  'returns': ['CpuDefinitionInfo'],
>    'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }

I'm afraid the doc comment is less than clear.  Before I can suggest
improvements, I have questions.

Looks like @alias-of is the only part of the return value that changes
when I re-run query-cpu-definitions with another @machine argument.
Correct?

How is this going to be used?  Will management software run
query-cpu-definitions for each machine type returned by query-machines?
Or just for a few of them?

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0d9a2d2ab7..96f9fe7fff 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6965,7 +6965,9 @@ static void arm_cpu_add_definition(gpointer data, gpointer user_data)
>      *cpu_list = entry;
>  }
>  
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> +                                                 const char *machine,
> +                                                 Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
>      GSList *list;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 67d1eca4ed..ae633793ed 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -4078,11 +4078,23 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
>      args->cpu_list = entry;
>  }
>  
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> +                                                 const char *machine,
> +                                                 Error **errp)
>  {
>      X86CPUDefinitionArgs args = { .cpu_list = NULL };
>      GSList *list;
> -    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    MachineClass *mc;
> +
> +    if (!has_machine) {
> +        mc = MACHINE_GET_CLASS(qdev_get_machine());
> +    } else {
> +        mc = machine_find_class(machine);
> +        if (!mc) {
> +            error_setg(errp, "Machine type '%s' not found", machine);
> +            return NULL;
> +        }
> +    }
>  
>      args.default_version = default_cpu_version_for_machine(mc);
>      list = get_sorted_cpu_model_list();
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index a2b6459b05..a73c767462 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -1481,7 +1481,9 @@ static void mips_cpu_add_definition(gpointer data, gpointer user_data)
>      *cpu_list = entry;
>  }
>  
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> +                                                 const char *machine,
> +                                                 Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
>      GSList *list;
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d..4493309c4c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10350,7 +10350,9 @@ static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
>      *first = entry;
>  }
>  
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> +                                                 const char *machine,
> +                                                 Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
>      GSList *list;
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 7e92fb2e15..e40f1f6bec 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -456,7 +456,9 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
>      *cpu_list = entry;
>  }
>  
> -CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> +CpuDefinitionInfoList *qmp_query_cpu_definitions(bool has_machine,
> +                                                 const char *machine,
> +                                                 Error **errp)
>  {
>      struct CpuDefinitionInfoListData list_data = {
>          .list = NULL,

Should the commit message explain why all implementations but one ignore
@machine?

> diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
> index 5fc9ca4bc6..79c5250243 100644
> --- a/tests/acceptance/x86_cpu_model_versions.py
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -234,6 +234,48 @@ class X86CPUModelAliases(avocado_qemu.Test):
>  
>          self.validate_aliases(cpus)
>  
> +    def test_machine_arg_none(self):
> +        """Check if unversioned CPU model is an alias pointing to right version"""
> +        vm1 = self.get_vm()
> +        vm1.add_args('-S')
> +        vm1.set_machine('pc-i440fx-4.0')
> +        vm1.launch()
> +        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='none'))
> +        vm1.shutdown()
> +
> +        vm2 = self.get_vm()
> +        vm2.add_args('-S')
> +        vm2.set_machine('none')
> +        vm2.launch()
> +        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))
> +        vm1.shutdown()
> +
> +        self.assertEquals(cpus1, cpus2)
> +
> +    def test_machine_arg_4_1(self):
> +        """Check if unversioned CPU model is an alias pointing to right version"""
> +        vm1 = self.get_vm()
> +        vm1.add_args('-S')
> +        vm1.set_machine('pc-i440fx-4.0')
> +        vm1.launch()
> +        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='pc-i440fx-4.1'))
> +        vm1.shutdown()
> +
> +        vm2 = self.get_vm()
> +        vm2.add_args('-S')
> +        vm2.set_machine('pc-i440fx-4.1')
> +        vm2.launch()
> +        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))
> +        vm1.shutdown()
> +
> +        self.assertEquals(cpus1, cpus2)
> +
> +    def test_invalid_machine(self):
> +        self.vm.add_args('-S')
> +        self.vm.launch()
> +        r = self.vm.qmp('query-cpu-definitions', machine='invalid-machine-123')
> +        self.assertIn('error', r)
> +
>  
>  class CascadelakeArchCapabilities(avocado_qemu.Test):
>      """

Tests look good to me.

I admit to being confused on when to use the tests/acceptance/ framework
and when to use qtest.

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[David Hildenbrand]

On 25.10.19 04:25, Eduardo Habkost wrote:
> We had introduced versioned CPU models in QEMU 4.1, including a
> method for querying for CPU model versions using

Interesting, I was not aware of that.

On s390x, we somewhat have versioned CPU models, but we decided against 
giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it 
didn't really seem to be necessary. (and we often implement/add features 
for older CPU models, there is a lot of fluctuation) Actually, you would 
have had to add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" 
to model the features you actually see in all the different virtual 
environments ("what a CPU looks like"). Not to talk about QEMU versions 
in addition to that. So we decided to always model what you would see 
under LPAR and are able to specify for a KVM guest. So you can use "z13" 
in an up-to-date LPAR environment, but not in a z/VM environment (you 
would have to disable features).

Each (!base) CPU model has a specific feature set per machine. Libvirt 
uses query-cpu-model-expansion() to convert this model+machine to a 
machine-independent representation. That representation is sufficient 
for all use cases we were aware of (esp. "virsh domcapabilities", the 
host CPU model, migration).

While s390x has versioned CPU models, we have no explicit way of 
specifying them for older machines, besides going over 
query-cpu-model-expansion and using expanded "base model + features".

I can see that this might make sense on x86-64, where you only have 
maybe 3 versions of a CPU (e.g., the one Intel messed up first - 
Haswell, the one Intel messed up next - Haswell-noTSX, and the one that 
Intel eventually did right - Haswell-noTSX-IBRS) and you might want to 
specify "Haswell" vs. "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But 
actually, you will always want to go for "Haswell-noTSX-IBRS", because 
you can expect to run in updated environments if I am not wrong, 
everything else are corner cases.

Of course, versioned CPU model are neat to avoid "base model + list of 
features", but at least for expanding the host model on s390x, it is not 
really helpful. When migrating, the model expansion does the trick.

I haven't looked into details of "how to specify or model versions". 
Maybe IBM wants to look into creating versions for all the old models we 
had. But again, not sure if that is of any help for s390x. CCing IBM.

> query-cpu-definitions.  This only has one problem: fetching CPU
> alias information for multiple machine types required restarting
> QEMU for each machine being queried.
> 
> This series adds a new `machine` parameter to
> query-cpu-definitions, that can be used to query CPU model alias
> information for multiple machines without restarting QEMU.
> 
> Eduardo Habkost (7):
>    i386: Use g_autofree at x86_cpu_list_entry()
>    i386: Add default_version parameter to CPU version functions
>    i386: Don't use default_cpu_version at "-cpu help"
>    machine: machine_find_class() function
>    i386: Remove x86_cpu_set_default_version() function
>    i386: Don't use default_cpu_version() inside query-cpu-definitions
>    cpu: Add `machine` parameter to query-cpu-definitions
> 
>   qapi/machine-target.json                   | 14 +++-
>   include/hw/boards.h                        |  1 +
>   include/hw/i386/pc.h                       |  5 +-
>   target/i386/cpu.h                          |  6 --
>   hw/core/machine.c                          | 16 ++++
>   hw/i386/pc.c                               |  3 -
>   target/arm/helper.c                        |  4 +-
>   target/i386/cpu.c                          | 93 +++++++++++++++-------
>   target/mips/helper.c                       |  4 +-
>   target/ppc/translate_init.inc.c            |  4 +-
>   target/s390x/cpu_models.c                  |  4 +-
>   vl.c                                       | 17 +---
>   tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++
>   13 files changed, 154 insertions(+), 59 deletions(-)
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Christian Borntraeger]

On 25.10.19 09:17, David Hildenbrand wrote:
> On 25.10.19 04:25, Eduardo Habkost wrote:
>> We had introduced versioned CPU models in QEMU 4.1, including a
>> method for querying for CPU model versions using
> 
> Interesting, I was not aware of that.
> 
> On s390x, we somewhat have versioned CPU models, but we decided against giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't really seem to be necessary. (and we often implement/add features for older CPU models, there is a lot of fluctuation) Actually, you would have had to add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the features you actually see in all the different virtual environments ("what a CPU looks like"). Not to talk about QEMU versions in addition to that. So we decided to always model what you would see under LPAR and are able to specify for a KVM guest. So you can use "z13" in an up-to-date LPAR environment, but not in a z/VM environment (you would have to disable features).
> 
> Each (!base) CPU model has a specific feature set per machine. Libvirt uses query-cpu-model-expansion() to convert this model+machine to a machine-independent representation. That representation is sufficient for all use cases we were aware of (esp. "virsh domcapabilities", the host CPU model, migration).
> 
> While s390x has versioned CPU models, we have no explicit way of specifying them for older machines, besides going over query-cpu-model-expansion and using expanded "base model + features".
> 
> I can see that this might make sense on x86-64, where you only have maybe 3 versions of a CPU (e.g., the one Intel messed up first - Haswell, the one Intel messed up next - Haswell-noTSX, and the one that Intel eventually did right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs. "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want to go for "Haswell-noTSX-IBRS", because you can expect to run in updated environments if I am not wrong, everything else are corner cases.
> 
> Of course, versioned CPU model are neat to avoid "base model + list of features", but at least for expanding the host model on s390x, it is not really helpful. When migrating, the model expansion does the trick.
> 
> I haven't looked into details of "how to specify or model versions". Maybe IBM wants to look into creating versions for all the old models we had. But again, not sure if that is of any help for s390x. CCing IBM.

I agree that this does not look very helpful. 
Especially as several things depend on the kernel version a QEMU version is
not sufficient to be guarantee construction success.
So we would need something like z14-qemu4.0-kernel-5.2-suse-flavour-onLPAR

Instead we do check if we can construct an equivalent model on the migration target.
And that model is precise. We do even have versions.
Right now with QEMU on s390  our models are versioned in a way that we fence of
facilities for old machine versions.

For example
-machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
and 
-machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
As migration does always require the tuple of machine and cpu this is save. I fail
to see what the benefit of an explicit z14-3.1 would be.

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[David Hildenbrand]

On 25.10.19 09:55, Christian Borntraeger wrote:
> 
> 
> On 25.10.19 09:17, David Hildenbrand wrote:
>> On 25.10.19 04:25, Eduardo Habkost wrote:
>>> We had introduced versioned CPU models in QEMU 4.1, including a
>>> method for querying for CPU model versions using
>>
>> Interesting, I was not aware of that.
>>
>> On s390x, we somewhat have versioned CPU models, but we decided against giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't really seem to be necessary. (and we often implement/add features for older CPU models, there is a lot of fluctuation) Actually, you would have had to add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the features you actually see in all the different virtual environments ("what a CPU looks like"). Not to talk about QEMU versions in addition to that. So we decided to always model what you would see under LPAR and are able to specify for a KVM guest. So you can use "z13" in an up-to-date LPAR environment, but not in a z/VM environment (you would have to disable features).
>>
>> Each (!base) CPU model has a specific feature set per machine. Libvirt uses query-cpu-model-expansion() to convert this model+machine to a machine-independent representation. That representation is sufficient for all use cases we were aware of (esp. "virsh domcapabilities", the host CPU model, migration).
>>
>> While s390x has versioned CPU models, we have no explicit way of specifying them for older machines, besides going over query-cpu-model-expansion and using expanded "base model + features".
>>
>> I can see that this might make sense on x86-64, where you only have maybe 3 versions of a CPU (e.g., the one Intel messed up first - Haswell, the one Intel messed up next - Haswell-noTSX, and the one that Intel eventually did right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs. "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want to go for "Haswell-noTSX-IBRS", because you can expect to run in updated environments if I am not wrong, everything else are corner cases.
>>
>> Of course, versioned CPU model are neat to avoid "base model + list of features", but at least for expanding the host model on s390x, it is not really helpful. When migrating, the model expansion does the trick.
>>
>> I haven't looked into details of "how to specify or model versions". Maybe IBM wants to look into creating versions for all the old models we had. But again, not sure if that is of any help for s390x. CCing IBM.
> 
> I agree that this does not look very helpful.
> Especially as several things depend on the kernel version a QEMU version is
> not sufficient to be guarantee construction success.
> So we would need something like z14-qemu4.0-kernel-5.2-suse-flavour-onLPAR
> 
> Instead we do check if we can construct an equivalent model on the migration target.
> And that model is precise. We do even have versions.
> Right now with QEMU on s390  our models are versioned in a way that we fence of
> facilities for old machine versions.
> 
> For example
> -machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
> and
> -machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
> As migration does always require the tuple of machine and cpu this is save. I fail
> to see what the benefit of an explicit z14-3.1 would be.
> 

AFAIKS the only real benefit of versioned CPU models is that you can add 
new CPU model versions without new QEMU version.

Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how 
versioned CPU model were implemented) on any QEMU machine. Which is the 
same as telling your customer "please use z13,featX=on" in case you have 
a good reason to not use the host model (along with baselining) but use 
an explicit model.

If you can change the default model of QEMU machines, you can automate 
this process. I am pretty sure this is a corner case, though (e.g., 
IBRS). Usually you have a new QEMU machine and can simply enable the new 
feature from that point on.

-- 

Thanks,

David / dhildenb

Re: [PATCH 7/7] cpu: Add `machine` parameter to query-cpu-definitions

[Eduardo Habkost]

CCing libvir-list.

On Fri, Oct 25, 2019 at 08:36:59AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> > The new parameter can be used by management software to query for
> > CPU model alias information for multiple machines without
> > restarting QEMU.
[...]
> > @@ -317,9 +321,17 @@
>    ##
>    # @query-cpu-definitions:
> >  #
> >  # Return a list of supported virtual CPU definitions
> >  #
> > +# @machine: Name of machine type.  The command returns some data
> > +#           that machine-specific.  This overrides the machine type
> 
> "that is machine-specific"
> 
> > +#           used to look up that information.  This can be used,
> > +#           for example, to query machine-specific CPU model aliases
> > +#           without restarting QEMU (since 4.2)
> > +#
> >  # Returns: a list of CpuDefInfo
> >  #
> >  # Since: 1.2.0
> >  ##
> > -{ 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> > +{ 'command': 'query-cpu-definitions',
> > +  'data': { '*machine': 'str' },
> > +  'returns': ['CpuDefinitionInfo'],
> >    'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> 
> I'm afraid the doc comment is less than clear.  Before I can suggest
> improvements, I have questions.
> 
> Looks like @alias-of is the only part of the return value that changes
> when I re-run query-cpu-definitions with another @machine argument.
> Correct?

Yes.

> 
> How is this going to be used?  Will management software run
> query-cpu-definitions for each machine type returned by query-machines?
> Or just for a few of them?

I don't know.  I'll let Jiri and other libvirt developers answer
that.

-- 
Eduardo

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Eduardo Habkost]

CCing danpb, libvir-list, mskrivanek.

On Fri, Oct 25, 2019 at 09:17:46AM +0200, David Hildenbrand wrote:
> On 25.10.19 04:25, Eduardo Habkost wrote:
> > We had introduced versioned CPU models in QEMU 4.1, including a
> > method for querying for CPU model versions using
> 
> Interesting, I was not aware of that.
> 
> On s390x, we somewhat have versioned CPU models, but we decided against
> giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't
> really seem to be necessary. (and we often implement/add features for older
> CPU models, there is a lot of fluctuation) Actually, you would have had to
> add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the
> features you actually see in all the different virtual environments ("what a
> CPU looks like"). Not to talk about QEMU versions in addition to that. So we
> decided to always model what you would see under LPAR and are able to
> specify for a KVM guest. So you can use "z13" in an up-to-date LPAR
> environment, but not in a z/VM environment (you would have to disable
> features).
> 
> Each (!base) CPU model has a specific feature set per machine. Libvirt uses
> query-cpu-model-expansion() to convert this model+machine to a
> machine-independent representation. That representation is sufficient for
> all use cases we were aware of (esp. "virsh domcapabilities", the host CPU
> model, migration).
> 
> While s390x has versioned CPU models, we have no explicit way of specifying
> them for older machines, besides going over query-cpu-model-expansion and
> using expanded "base model + features".
> 
> I can see that this might make sense on x86-64, where you only have maybe 3
> versions of a CPU (e.g., the one Intel messed up first - Haswell, the one
> Intel messed up next - Haswell-noTSX, and the one that Intel eventually did
> right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs.
> "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want
> to go for "Haswell-noTSX-IBRS", because you can expect to run in updated
> environments if I am not wrong, everything else are corner cases.
> 
> Of course, versioned CPU model are neat to avoid "base model + list of
> features", but at least for expanding the host model on s390x, it is not
> really helpful. When migrating, the model expansion does the trick.
> 
> I haven't looked into details of "how to specify or model versions". Maybe
> IBM wants to look into creating versions for all the old models we had. But
> again, not sure if that is of any help for s390x. CCing IBM.

I'm not sure yet if there are the x86-specific goals and
constraints that would make it difficult to follow the same
approach followed by s390x.  I have the impression we do,
but I need to think more carefully about it.

Let's discuss that during KVM Forum?

The two main goals of versioned CPU models in x86 are:
1) Decoupling CPU model updates from machine-types (users should be
   able to update a CPU model definition without changing machine
   type).
2) Letting management software automate CPU model updates.
   Normally this is necessary when bare metal microcode or
   software updates add/remove features from CPUs.  Sometimes the
   Virtual CPU model update needs to be performed before the host
   updates are applied (if features are being removed).

The main constraint that makes it difficult to address the above
without a new API is:
A) Every CPU model in x86 except "host" is already expected to
   be migration-safe (I don't know how this compares to s390x).


> 
> > query-cpu-definitions.  This only has one problem: fetching CPU
> > alias information for multiple machine types required restarting
> > QEMU for each machine being queried.
> > 
> > This series adds a new `machine` parameter to
> > query-cpu-definitions, that can be used to query CPU model alias
> > information for multiple machines without restarting QEMU.
> > 
> > Eduardo Habkost (7):
> >    i386: Use g_autofree at x86_cpu_list_entry()
> >    i386: Add default_version parameter to CPU version functions
> >    i386: Don't use default_cpu_version at "-cpu help"
> >    machine: machine_find_class() function
> >    i386: Remove x86_cpu_set_default_version() function
> >    i386: Don't use default_cpu_version() inside query-cpu-definitions
> >    cpu: Add `machine` parameter to query-cpu-definitions
> > 
> >   qapi/machine-target.json                   | 14 +++-
> >   include/hw/boards.h                        |  1 +
> >   include/hw/i386/pc.h                       |  5 +-
> >   target/i386/cpu.h                          |  6 --
> >   hw/core/machine.c                          | 16 ++++
> >   hw/i386/pc.c                               |  3 -
> >   target/arm/helper.c                        |  4 +-
> >   target/i386/cpu.c                          | 93 +++++++++++++++-------
> >   target/mips/helper.c                       |  4 +-
> >   target/ppc/translate_init.inc.c            |  4 +-
> >   target/s390x/cpu_models.c                  |  4 +-
> >   vl.c                                       | 17 +---
> >   tests/acceptance/x86_cpu_model_versions.py | 42 ++++++++++
> >   13 files changed, 154 insertions(+), 59 deletions(-)
> > 
> 
> 
> -- 
> 
> Thanks,
> 
> David / dhildenb

-- 
Eduardo

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Eduardo Habkost]

CCing mskrivanek, danpb, libvir-list.

On Fri, Oct 25, 2019 at 10:02:29AM +0200, David Hildenbrand wrote:
> On 25.10.19 09:55, Christian Borntraeger wrote:
> > 
> > 
> > On 25.10.19 09:17, David Hildenbrand wrote:
> > > On 25.10.19 04:25, Eduardo Habkost wrote:
> > > > We had introduced versioned CPU models in QEMU 4.1, including a
> > > > method for querying for CPU model versions using
> > > 
> > > Interesting, I was not aware of that.
> > > 
> > > On s390x, we somewhat have versioned CPU models, but we decided against giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't really seem to be necessary. (and we often implement/add features for older CPU models, there is a lot of fluctuation) Actually, you would have had to add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the features you actually see in all the different virtual environments ("what a CPU looks like"). Not to talk about QEMU versions in addition to that. So we decided to always model what you would see under LPAR and are able to specify for a KVM guest. So you can use "z13" in an up-to-date LPAR environment, but not in a z/VM environment (you would have to disable features).
> > > 
> > > Each (!base) CPU model has a specific feature set per machine. Libvirt uses query-cpu-model-expansion() to convert this model+machine to a machine-independent representation. That representation is sufficient for all use cases we were aware of (esp. "virsh domcapabilities", the host CPU model, migration).
> > > 
> > > While s390x has versioned CPU models, we have no explicit way of specifying them for older machines, besides going over query-cpu-model-expansion and using expanded "base model + features".
> > > 
> > > I can see that this might make sense on x86-64, where you only have maybe 3 versions of a CPU (e.g., the one Intel messed up first - Haswell, the one Intel messed up next - Haswell-noTSX, and the one that Intel eventually did right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs. "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want to go for "Haswell-noTSX-IBRS", because you can expect to run in updated environments if I am not wrong, everything else are corner cases.
> > > 
> > > Of course, versioned CPU model are neat to avoid "base model + list of features", but at least for expanding the host model on s390x, it is not really helpful. When migrating, the model expansion does the trick.
> > > 
> > > I haven't looked into details of "how to specify or model versions". Maybe IBM wants to look into creating versions for all the old models we had. But again, not sure if that is of any help for s390x. CCing IBM.
> > 
> > I agree that this does not look very helpful.
> > Especially as several things depend on the kernel version a QEMU version is
> > not sufficient to be guarantee construction success.
> > So we would need something like z14-qemu4.0-kernel-5.2-suse-flavour-onLPAR
> > 
> > Instead we do check if we can construct an equivalent model on the migration target.
> > And that model is precise. We do even have versions.
> > Right now with QEMU on s390  our models are versioned in a way that we fence of
> > facilities for old machine versions.
> > 
> > For example
> > -machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
> > and
> > -machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
> > As migration does always require the tuple of machine and cpu this is save. I fail
> > to see what the benefit of an explicit z14-3.1 would be.
> > 
> 
> AFAIKS the only real benefit of versioned CPU models is that you can add new
> CPU model versions without new QEMU version.
> 
> Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how
> versioned CPU model were implemented) on any QEMU machine. Which is the same
> as telling your customer "please use z13,featX=on" in case you have a good
> reason to not use the host model (along with baselining) but use an explicit
> model.

Exactly.  oVirt, specifically, don't use host-model.

> 
> If you can change the default model of QEMU machines, you can automate this
> process. I am pretty sure this is a corner case, though (e.g., IBRS).
> Usually you have a new QEMU machine and can simply enable the new feature
> from that point on.

When -noTSX happened, we thought it was a corner case.  Then we
had -IBRS & -IBPB.  Then we had SSBD (with no new CPU models).
Then we had MD_CLEAR (with no new CPU models).  It's now very
easy to get an insecure VM created if you are not using
host-model.

-- 
Eduardo

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Daniel P. Berrangé]

On Fri, Oct 25, 2019 at 10:02:29AM +0200, David Hildenbrand wrote:
> On 25.10.19 09:55, Christian Borntraeger wrote:
> > 
> > 
> > On 25.10.19 09:17, David Hildenbrand wrote:
> > > On 25.10.19 04:25, Eduardo Habkost wrote:
> > > > We had introduced versioned CPU models in QEMU 4.1, including a
> > > > method for querying for CPU model versions using
> > > 
> > > Interesting, I was not aware of that.
> > > 
> > > On s390x, we somewhat have versioned CPU models, but we decided against giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't really seem to be necessary. (and we often implement/add features for older CPU models, there is a lot of fluctuation) Actually, you would have had to add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the features you actually see in all the different virtual environments ("what a CPU looks like"). Not to talk about QEMU versions in addition to that. So we decided to always model what you would see under LPAR and are able to specify for a KVM guest. So you can use "z13" in an up-to-date LPAR environment, but not in a z/VM environment (you would have to disable features).
> > > 
> > > Each (!base) CPU model has a specific feature set per machine. Libvirt uses query-cpu-model-expansion() to convert this model+machine to a machine-independent representation. That representation is sufficient for all use cases we were aware of (esp. "virsh domcapabilities", the host CPU model, migration).
> > > 
> > > While s390x has versioned CPU models, we have no explicit way of specifying them for older machines, besides going over query-cpu-model-expansion and using expanded "base model + features".
> > > 
> > > I can see that this might make sense on x86-64, where you only have maybe 3 versions of a CPU (e.g., the one Intel messed up first - Haswell, the one Intel messed up next - Haswell-noTSX, and the one that Intel eventually did right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs. "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want to go for "Haswell-noTSX-IBRS", because you can expect to run in updated environments if I am not wrong, everything else are corner cases.
> > > 
> > > Of course, versioned CPU model are neat to avoid "base model + list of features", but at least for expanding the host model on s390x, it is not really helpful. When migrating, the model expansion does the trick.
> > > 
> > > I haven't looked into details of "how to specify or model versions". Maybe IBM wants to look into creating versions for all the old models we had. But again, not sure if that is of any help for s390x. CCing IBM.
> > 
> > I agree that this does not look very helpful.
> > Especially as several things depend on the kernel version a QEMU version is
> > not sufficient to be guarantee construction success.
> > So we would need something like z14-qemu4.0-kernel-5.2-suse-flavour-onLPAR
> > 
> > Instead we do check if we can construct an equivalent model on the migration target.
> > And that model is precise. We do even have versions.
> > Right now with QEMU on s390  our models are versioned in a way that we fence of
> > facilities for old machine versions.
> > 
> > For example
> > -machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
> > and
> > -machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
> > As migration does always require the tuple of machine and cpu this is save. I fail
> > to see what the benefit of an explicit z14-3.1 would be.
> > 
> 
> AFAIKS the only real benefit of versioned CPU models is that you can add new
> CPU model versions without new QEMU version.

This is very important for backporting CPU security fixes to existing QEMU
releases. 

> 
> Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how
> versioned CPU model were implemented) on any QEMU machine. Which is the same
> as telling your customer "please use z13,featX=on" in case you have a good
> reason to not use the host model (along with baselining) but use an explicit
> model.
> 
> If you can change the default model of QEMU machines, you can automate this
> process. I am pretty sure this is a corner case, though (e.g., IBRS).
> Usually you have a new QEMU machine and can simply enable the new feature
> from that point on.

There are now 4 Haswell variants, only some of which are runnable
on any given host, depending on what microcode the user has installed
or what particular Haswell silicon SKU the user purchased. Given the
frequency of new CPU flaws arrived since the first Meltdown/Spectre,
this isn't a corner case, at least for the x86 world & Intel in
particular. Other arches/vendors haven't been quite so badly affected
in this way.

If we tied each new Haswell variant to a machine type, then users would
be blocked from consuming a new machine type depending on runnability of
the CPU model. This is not at all desirable, as mgmt apps now have complex
rules on what machine type they can use.

When dealing with backporting patches for new CPU hardware flaws, the
new CPU features are backported to many old QEMU versions. The new
machine types are not backportable.

Both these called for making CPU versioning independant of machine
type versioning.

Essentially the goal with CPU versioning is that the user can request
a bare "Haswell" and libvirt (or the mgmt app) will automatically
expand this to the best Haswell version that the host is able to
support with its CPUs / microcode / BIOS config combination.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[David Hildenbrand]

On 25.10.19 15:38, Eduardo Habkost wrote:
> CCing danpb, libvir-list, mskrivanek.
> 
> On Fri, Oct 25, 2019 at 09:17:46AM +0200, David Hildenbrand wrote:
>> On 25.10.19 04:25, Eduardo Habkost wrote:
>>> We had introduced versioned CPU models in QEMU 4.1, including a
>>> method for querying for CPU model versions using
>>
>> Interesting, I was not aware of that.
>>
>> On s390x, we somewhat have versioned CPU models, but we decided against
>> giving them explicit names (e.g., z13-v1 or z13-4.1.0), because it didn't
>> really seem to be necessary. (and we often implement/add features for older
>> CPU models, there is a lot of fluctuation) Actually, you would have had to
>> add "z13-z/VM-x.x.x" or "z13-LPAR-x.x.x" or "z13-KVM-x.x.x" to model the
>> features you actually see in all the different virtual environments ("what a
>> CPU looks like"). Not to talk about QEMU versions in addition to that. So we
>> decided to always model what you would see under LPAR and are able to
>> specify for a KVM guest. So you can use "z13" in an up-to-date LPAR
>> environment, but not in a z/VM environment (you would have to disable
>> features).
>>
>> Each (!base) CPU model has a specific feature set per machine. Libvirt uses
>> query-cpu-model-expansion() to convert this model+machine to a
>> machine-independent representation. That representation is sufficient for
>> all use cases we were aware of (esp. "virsh domcapabilities", the host CPU
>> model, migration).
>>
>> While s390x has versioned CPU models, we have no explicit way of specifying
>> them for older machines, besides going over query-cpu-model-expansion and
>> using expanded "base model + features".
>>
>> I can see that this might make sense on x86-64, where you only have maybe 3
>> versions of a CPU (e.g., the one Intel messed up first - Haswell, the one
>> Intel messed up next - Haswell-noTSX, and the one that Intel eventually did
>> right - Haswell-noTSX-IBRS) and you might want to specify "Haswell" vs.
>> "Haswell-IBRS" vs. "Haswell-noTSX-IBRS". But actually, you will always want
>> to go for "Haswell-noTSX-IBRS", because you can expect to run in updated
>> environments if I am not wrong, everything else are corner cases.
>>
>> Of course, versioned CPU model are neat to avoid "base model + list of
>> features", but at least for expanding the host model on s390x, it is not
>> really helpful. When migrating, the model expansion does the trick.
>>
>> I haven't looked into details of "how to specify or model versions". Maybe
>> IBM wants to look into creating versions for all the old models we had. But
>> again, not sure if that is of any help for s390x. CCing IBM.
> 
> I'm not sure yet if there are the x86-specific goals and
> constraints that would make it difficult to follow the same
> approach followed by s390x.  I have the impression we do,
> but I need to think more carefully about it.
> 
> Let's discuss that during KVM Forum?

I won't be attending KVM Forum this year, but Christian should be around.

> 
> The two main goals of versioned CPU models in x86 are:
> 1) Decoupling CPU model updates from machine-types (users should be
>     able to update a CPU model definition without changing machine
>     type).
> 2) Letting management software automate CPU model updates.
>     Normally this is necessary when bare metal microcode or
>     software updates add/remove features from CPUs.  Sometimes the
>     Virtual CPU model update needs to be performed before the host
>     updates are applied (if features are being removed)

We have 2) on s390x after a QEMU machine update. You need a QEMU update 
usually either way to support the new CPU feature. But I can see how 
decoupling the CPU model definition from the machine makes this easier. 
It does introduce complexity. It applies only when running older QEMU 
machines, or if we have to update a CPU model before we get a new QEMU 
machine.

But we do have versioned CPU models already, so the discussion is 
already over :)

> 
> The main constraint that makes it difficult to address the above
> without a new API is:
> A) Every CPU model in x86 except "host" is already expected to
>     be migration-safe (I don't know how this compares to s390x).

I would describe that not a bug but a feature :) It does come with the 
price that only using the newest QEMU machine results in the newest CPU 
model, that part I understand.

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[David Hildenbrand]

>>> For example
>>> -machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
>>> and
>>> -machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
>>> As migration does always require the tuple of machine and cpu this is save. I fail
>>> to see what the benefit of an explicit z14-3.1 would be.
>>>
>>
>> AFAIKS the only real benefit of versioned CPU models is that you can add new
>> CPU model versions without new QEMU version.
> 
> This is very important for backporting CPU security fixes to existing QEMU
> releases.

I'd say it's not really relevant for backporting per se. It's relevant 
for automatically enabling security fixes when not using the host model. 
That part I understand. Less likely to make mistakes when explicitly 
specifying CPU models.

I once was told that if a user actually specified an explicit CPU model 
in the libvirt XML ("haswell-whatever"), you should not go ahead and 
make any later changes to that model (guest ABI should not change when 
you update/restart the guest ...). So this only applies when creating 
new guests? Or will you change existing model definitions implicitly?

> 
>>
>> Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how
>> versioned CPU model were implemented) on any QEMU machine. Which is the same
>> as telling your customer "please use z13,featX=on" in case you have a good
>> reason to not use the host model (along with baselining) but use an explicit
>> model.
>>
>> If you can change the default model of QEMU machines, you can automate this
>> process. I am pretty sure this is a corner case, though (e.g., IBRS).
>> Usually you have a new QEMU machine and can simply enable the new feature
>> from that point on.
> 
> There are now 4 Haswell variants, only some of which are runnable
> on any given host, depending on what microcode the user has installed
> or what particular Haswell silicon SKU the user purchased. Given the
> frequency of new CPU flaws arrived since the first Meltdown/Spectre,
> this isn't a corner case, at least for the x86 world & Intel in
> particular. Other arches/vendors haven't been quite so badly affected
> in this way.

On s390x you can assume that such firmware/microcode updates will be on 
any machine after some time. That is a big difference to x86-64 AFAIK.

> 
> If we tied each new Haswell variant to a machine type, then users would
> be blocked from consuming a new machine type depending on runnability of
> the CPU model. This is not at all desirable, as mgmt apps now have complex
> rules on what machine type they can use.

So you actually want different CPU variants, which you have already, 
just in a different form. (e.g., "haswell" will be mapped to 
"haswell-whatever", just differently via versions)

> 
> When dealing with backporting patches for new CPU hardware flaws, the
> new CPU features are backported to many old QEMU versions. The new
> machine types are not backportable.

That part I understand.

> 
> Both these called for making CPU versioning independant of machine
> type versioning.
> 
> Essentially the goal with CPU versioning is that the user can request
> a bare "Haswell" and libvirt (or the mgmt app) will automatically
> expand this to the best Haswell version that the host is able to
> support with its CPUs / microcode / BIOS config combination.


So if I do a "-cpu haswell -M whatever-machine", as far as I understood 
reading this,  I get the "default CPU model alias for that QEMU machine" 
and *not* the "best Haswell version that the host is able to support".

Or does the default actually also depend on the current host?

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[Daniel P. Berrangé]

On Fri, Oct 25, 2019 at 04:23:59PM +0200, David Hildenbrand wrote:
> > > > For example
> > > > -machine s390-virtio-ccw-3.1 -cpu z14 will not have the multiple epoch facility
> > > > and
> > > > -machine s390-virtio-ccw-4.0 -cpu z14 will have the multiple epoch facility.
> > > > As migration does always require the tuple of machine and cpu this is save. I fail
> > > > to see what the benefit of an explicit z14-3.1 would be.
> > > > 
> > > 
> > > AFAIKS the only real benefit of versioned CPU models is that you can add new
> > > CPU model versions without new QEMU version.
> > 
> > This is very important for backporting CPU security fixes to existing QEMU
> > releases.
> 
> I'd say it's not really relevant for backporting per se. It's relevant for
> automatically enabling security fixes when not using the host model. That
> part I understand. Less likely to make mistakes when explicitly specifying
> CPU models.
> 
> I once was told that if a user actually specified an explicit CPU model in
> the libvirt XML ("haswell-whatever"), you should not go ahead and make any
> later changes to that model (guest ABI should not change when you
> update/restart the guest ...). So this only applies when creating new
> guests? Or will you change existing model definitions implicitly?

Libvirt will only ever expand a bare CPU model at time it first parses
the XML. So if a mgmt app defines a new persistent guest in libvirt, the
CPU is expanded them and remains unchanged thereafter, in order to preserve
ABI compat.

If using transient guests its different as libvirt doesn't store the config
in disk when the guest isn't running. So mgmt apps using transient guests
are responsible  for picking a explicit versioned model themselves if they
need stable ABI.

> > > Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how
> > > versioned CPU model were implemented) on any QEMU machine. Which is the same
> > > as telling your customer "please use z13,featX=on" in case you have a good
> > > reason to not use the host model (along with baselining) but use an explicit
> > > model.
> > > 
> > > If you can change the default model of QEMU machines, you can automate this
> > > process. I am pretty sure this is a corner case, though (e.g., IBRS).
> > > Usually you have a new QEMU machine and can simply enable the new feature
> > > from that point on.
> > 
> > There are now 4 Haswell variants, only some of which are runnable
> > on any given host, depending on what microcode the user has installed
> > or what particular Haswell silicon SKU the user purchased. Given the
> > frequency of new CPU flaws arrived since the first Meltdown/Spectre,
> > this isn't a corner case, at least for the x86 world & Intel in
> > particular. Other arches/vendors haven't been quite so badly affected
> > in this way.
> 
> On s390x you can assume that such firmware/microcode updates will be on any
> machine after some time. That is a big difference to x86-64 AFAIK.

I don't know s390x much, but can we really assume that users promptly
install firmware updates, any better than users do for x86 or other
arch. IME corporate beaurcracy can drag out time to update arbitrarily
long.

> > If we tied each new Haswell variant to a machine type, then users would
> > be blocked from consuming a new machine type depending on runnability of
> > the CPU model. This is not at all desirable, as mgmt apps now have complex
> > rules on what machine type they can use.
> 
> So you actually want different CPU variants, which you have already, just in
> a different form. (e.g., "haswell" will be mapped to "haswell-whatever",
> just differently via versions)

Yes, you can think of "Haswell", "Haswell-noTSX", "Haswell-noTSX-IBRS"
as all being versions of the same thing. There was never any explicit
association or naming though. So what's changing is that we're defining
a sane naming scheme for the variants of each model so we don't end
up with   "Haswell-noTSX-IBRS-SSBD-MDS-WHATEVER-NEXT-INTEL-FLAW-IS",
and we declaring that a bare "Haswell" will expand to some "best"
version depending on machine type (but also selectable by mgmt app
above).

> > Both these called for making CPU versioning independant of machine
> > type versioning.
> > 
> > Essentially the goal with CPU versioning is that the user can request
> > a bare "Haswell" and libvirt (or the mgmt app) will automatically
> > expand this to the best Haswell version that the host is able to
> > support with its CPUs / microcode / BIOS config combination.
> 
> 
> So if I do a "-cpu haswell -M whatever-machine", as far as I understood
> reading this,  I get the "default CPU model alias for that QEMU machine" and
> *not* the "best Haswell version that the host is able to support".
> 
> Or does the default actually also depend on the current host?

At the QEMU level "haswell" will expand to a particular CPU version
per machine type. So yes, at the QEMU level machine types might have
a dependancy on the host.

Above QEMU though, libvirt/mgmt apps can be more dynamic in how they
expand a bare "haswell" to take account of what the host supports.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[David Hildenbrand]

>> I once was told that if a user actually specified an explicit CPU model in
>> the libvirt XML ("haswell-whatever"), you should not go ahead and make any
>> later changes to that model (guest ABI should not change when you
>> update/restart the guest ...). So this only applies when creating new
>> guests? Or will you change existing model definitions implicitly?
> 
> Libvirt will only ever expand a bare CPU model at time it first parses
> the XML. So if a mgmt app defines a new persistent guest in libvirt, the
> CPU is expanded them and remains unchanged thereafter, in order to preserve
> ABI compat.

Okay, perfect.

> 
> If using transient guests its different as libvirt doesn't store the config
> in disk when the guest isn't running. So mgmt apps using transient guests
> are responsible  for picking a explicit versioned model themselves if they
> need stable ABI.

That makes sense.

> 
>>>> Then you can specify "-cpu z13-vX" or "-cpu z13 -cpuv X" (no idea how
>>>> versioned CPU model were implemented) on any QEMU machine. Which is the same
>>>> as telling your customer "please use z13,featX=on" in case you have a good
>>>> reason to not use the host model (along with baselining) but use an explicit
>>>> model.
>>>>
>>>> If you can change the default model of QEMU machines, you can automate this
>>>> process. I am pretty sure this is a corner case, though (e.g., IBRS).
>>>> Usually you have a new QEMU machine and can simply enable the new feature
>>>> from that point on.
>>>
>>> There are now 4 Haswell variants, only some of which are runnable
>>> on any given host, depending on what microcode the user has installed
>>> or what particular Haswell silicon SKU the user purchased. Given the
>>> frequency of new CPU flaws arrived since the first Meltdown/Spectre,
>>> this isn't a corner case, at least for the x86 world & Intel in
>>> particular. Other arches/vendors haven't been quite so badly affected
>>> in this way.
>>
>> On s390x you can assume that such firmware/microcode updates will be on any
>> machine after some time. That is a big difference to x86-64 AFAIK.
> 
> I don't know s390x much, but can we really assume that users promptly
> install firmware updates, any better than users do for x86 or other
> arch. IME corporate beaurcracy can drag out time to update arbitrarily
> long.

That's what you get when you pay premium prices for premium support :D

The real issue when it comes to CPU models on s390x is the variance of 
features of a specific model across environments (especially different 
hypervisors).

>>> If we tied each new Haswell variant to a machine type, then users would
>>> be blocked from consuming a new machine type depending on runnability of
>>> the CPU model. This is not at all desirable, as mgmt apps now have complex
>>> rules on what machine type they can use.
>>
>> So you actually want different CPU variants, which you have already, just in
>> a different form. (e.g., "haswell" will be mapped to "haswell-whatever",
>> just differently via versions)
> 
> Yes, you can think of "Haswell", "Haswell-noTSX", "Haswell-noTSX-IBRS"
> as all being versions of the same thing. There was never any explicit
> association or naming though. So what's changing is that we're defining
> a sane naming scheme for the variants of each model so we don't end
> up with   "Haswell-noTSX-IBRS-SSBD-MDS-WHATEVER-NEXT-INTEL-FLAW-IS",
> and we declaring that a bare "Haswell" will expand to some "best"
> version depending on machine type (but also selectable by mgmt app
> above).

I mean, all you really want is a way to specify "give me the best 
haswell you can do with this accelerator", which *could* map to "-cpu 
haswell,tsx=off,ibrs=on,ssbf=on" ... but also something else on the HW.

I really don't see why we need versioned CPU models for that, all you 
want to do is apply delta updates to the initial model if possible on 
the current accelerator. Just like HW does. See below for a simpler 
approach.

> 
>>> Both these called for making CPU versioning independant of machine
>>> type versioning.
>>>
>>> Essentially the goal with CPU versioning is that the user can request
>>> a bare "Haswell" and libvirt (or the mgmt app) will automatically
>>> expand this to the best Haswell version that the host is able to
>>> support with its CPUs / microcode / BIOS config combination.
>>
>>
>> So if I do a "-cpu haswell -M whatever-machine", as far as I understood
>> reading this,  I get the "default CPU model alias for that QEMU machine" and
>> *not* the "best Haswell version that the host is able to support".
>>
>> Or does the default actually also depend on the current host?
> 
> At the QEMU level "haswell" will expand to a particular CPU version
> per machine type. So yes, at the QEMU level machine types might have
> a dependancy on the host.
> 
> Above QEMU though, libvirt/mgmt apps can be more dynamic in how they
> expand a bare "haswell" to take account of what the host supports.

Let me propose something *much* simpler which would work just fine on 
s390x, and I assume on x86-64 as well.

On s390x we e.g. have the two models:
- "z14-base"
  -> minimum feature set we expect to have in every environment
  -> QEMU version/machine independent, will never change
- "z14"
  -> "default model", can change between QEMU machines
  -> migration-safe

Now, internally we have in addition something that matches:
- "z14-max"
  -> all possible CPU features valid for this model
  -> Includes e.g., nested virt features not in the "z14" model
  -> Can and will change between QEMU versions

Of course we also have:
- "max"
  -> "all features supported by the accelerator in the current host"

What we really want is:
- "z14-best"
  -> "best features for this model supported by the accelerator in the
      current host"

The trick is that *usually* :
	"z14-best" = baseline("z14-max", "max")

Minor exceptions can be easily added (e.g., always disable a certain 
feature). So, what I would be proposing for s390x (and also x86-64, but 
I know that they have more legacy handling) is simply implementing and 
exposing all -best models.


Why is this good in general?

1. No semantic changes of existing models. What was migration safe
    remains migration safe.
2. No CPU versions, less complexity.
3. No changes in the tool stack. Implement it in QEMU and it will be
    supported on every layer. Simply specify/expand "z14-best" and you
    get something that will run and make use of the best (on s390x
    usually all) features available that are valid for this model.

Why is this good for s390x?

1. As discussed, versioning all the different flavors we have is
    not feasible, nor practicable.

2. Features that are typically not around (especially, nested virt
    features) will be enabled similar to the host model when around.


I think I can hack a prototype of that in a couple of hours.

-- 

Thanks,

David / dhildenb

Re: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191025022553.25298-1-ehabkost@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/7] i386: Add `machine` parameter to query-cpu-definitions
Type: series
Message-id: 20191025022553.25298-1-ehabkost@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
80774f3 cpu: Add `machine` parameter to query-cpu-definitions
a94469e i386: Don't use default_cpu_version() inside query-cpu-definitions
9c82004 i386: Remove x86_cpu_set_default_version() function
c529624 machine: machine_find_class() function
f79edfc i386: Don't use default_cpu_version at "-cpu help"
0106983 i386: Add default_version parameter to CPU version functions
d6a172b i386: Use g_autofree at x86_cpu_list_entry()

=== OUTPUT BEGIN ===
1/7 Checking commit d6a172b05619 (i386: Use g_autofree at x86_cpu_list_entry())
2/7 Checking commit 0106983c7b3c (i386: Add default_version parameter to CPU version functions)
WARNING: line over 80 characters
#28: FILE: target/i386/cpu.c:3191:
+                                                   X86CPUVersion default_version)

WARNING: line over 80 characters
#60: FILE: target/i386/cpu.c:3983:
+    g_autofree char *alias_of = x86_cpu_class_get_alias_of(cc, default_cpu_version);

WARNING: line over 80 characters
#78: FILE: target/i386/cpu.c:4121:
+    X86CPUVersion version = x86_cpu_model_resolve_version(model, default_cpu_version);

total: 0 errors, 3 warnings, 55 lines checked

Patch 2/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/7 Checking commit f79edfcd6195 (i386: Don't use default_cpu_version at "-cpu help")
4/7 Checking commit c529624d287b (machine: machine_find_class() function)
5/7 Checking commit 9c820045c733 (i386: Remove x86_cpu_set_default_version() function)
WARNING: line over 80 characters
#81: FILE: target/i386/cpu.c:3178:
+        (PCMachineClass *)object_class_dynamic_cast(OBJECT_CLASS(mc), TYPE_PC_MACHINE);

WARNING: line over 80 characters
#87: FILE: target/i386/cpu.c:3184:
+    return default_cpu_version_for_machine(MACHINE_GET_CLASS(qdev_get_machine()));

WARNING: line over 80 characters
#110: FILE: target/i386/cpu.c:4134:
+    X86CPUVersion version = x86_cpu_model_resolve_version(model, default_cpu_version());

total: 0 errors, 3 warnings, 88 lines checked

Patch 5/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/7 Checking commit a94469ea9b83 (i386: Don't use default_cpu_version() inside query-cpu-definitions)
7/7 Checking commit 80774f3866be (cpu: Add `machine` parameter to query-cpu-definitions)
WARNING: line over 80 characters
#147: FILE: tests/acceptance/x86_cpu_model_versions.py:238:
+        """Check if unversioned CPU model is an alias pointing to right version"""

ERROR: line over 90 characters
#152: FILE: tests/acceptance/x86_cpu_model_versions.py:243:
+        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='none'))

ERROR: line over 90 characters
#159: FILE: tests/acceptance/x86_cpu_model_versions.py:250:
+        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))

WARNING: line over 80 characters
#165: FILE: tests/acceptance/x86_cpu_model_versions.py:256:
+        """Check if unversioned CPU model is an alias pointing to right version"""

ERROR: line over 90 characters
#170: FILE: tests/acceptance/x86_cpu_model_versions.py:261:
+        cpus1 = dict((m['name'], m.get('alias-of')) for m in vm1.command('query-cpu-definitions', machine='pc-i440fx-4.1'))

ERROR: line over 90 characters
#177: FILE: tests/acceptance/x86_cpu_model_versions.py:268:
+        cpus2 = dict((m['name'], m.get('alias-of')) for m in vm2.command('query-cpu-definitions'))

total: 4 errors, 2 warnings, 141 lines checked

Patch 7/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191025022553.25298-1-ehabkost@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com