[Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Eduardo Habkost]

This series implements basic infrastructure for CPU model
versioning, as discussed before[1][2][3].  This will finally
allow us to update CPU models in ways that introduce new software
or hardware requirements.

My original plan was to use "query-cpu-model-expansion
mode=static" to resolve aliases, but I dropped that plan because
it would increase complexity for management software a lot.
static CPU models are documented as not being affected by the
machine type and accelerator at all, which would make the
versioned CPU models very inconvenient to use in the command
line.  e.g.: users would be forced to replace:

  -cpu Haswell

with:

  -cpu Haswell-4.1,+2apic,+monitor,+kvmclock,+kvm-nopiodelay,+kvm-asyncpf,+kvm-steal-time,+kvm-pv-eoi,+kvmclock-stable-bit,+x2apic,-acpi,-monitor,-svm

In the end, making the versioned CPU models static is not a
requirement at all: what we really need is to drop the
runnability guarantees from unversioned CPU model names, and
require management software to resolve the unversioned alias
before saving the VM configuration.

Guest ABI compatibility and live migration guarantees are going
to be kept: unversioned CPU models will still be usable with live
migration.  Only runnability guarantees when updating the machine
type will be dropped.  This means unversioned CPU models are
still reported as migration-safe in query-cpu-definitions.

The last patch in the series demonstrates how the new feature can
be used to update a CPU model: it adds a Cascadelake-Server-4.1.1
CPU model, including "arch-capabilities=on" and "stepping=5".
Unfortunately we can't enable arch-capabilities in the -4.1
version of Cascadelake-Server because it would break our existing
runnability guarantees.

[1] https://www.mail-archive.com/libvir-list@redhat.com/msg167342.html
[2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg590034.html
[3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg611244.html

---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Pavel Hrdina <phrdina@redhat.com>
Cc: Jiri Denemark <jdenemar@redhat.com>
Cc: "Hu, Robert" <robert.hu@intel.com>
Cc: Tao Xu <tao3.xu@intel.com>
Cc: Richard Henderson <rth@twiddle.net>

Eduardo Habkost (6):
  i386: Add x-force-features option for testing
  i386: Remove unused host_cpudef variable
  qmp: Add "alias-of" field to query-cpu-definitions
  i386: Infrastructure for versioned CPU models
  docs: Deprecate CPU model runnability guarantees
  i386: Add Cascadelake-Server-4.1.1 CPU model

 qapi/target.json                           |   9 +-
 include/hw/i386/pc.h                       |   3 +
 target/i386/cpu-qom.h                      |  10 +-
 target/i386/cpu.h                          |  16 ++
 hw/i386/pc.c                               |   3 +
 hw/i386/pc_piix.c                          |   4 +
 hw/i386/pc_q35.c                           |   4 +
 target/i386/cpu.c                          | 188 +++++++++++++++++----
 qemu-deprecated.texi                       |  19 +++
 tests/acceptance/x86_cpu_model_versions.py | 173 +++++++++++++++++++
 10 files changed, 388 insertions(+), 41 deletions(-)
 create mode 100644 tests/acceptance/x86_cpu_model_versions.py

-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 1/6] i386: Add x-force-features option for testing

[Eduardo Habkost]

Add a new option that can be used to disable feature flag
filtering.  This will allow CPU model compatibility test cases to
work without host hardware dependencies.

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

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index df99d70c43..25544fdaaa 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1414,6 +1414,12 @@ struct X86CPU {
 
     bool check_cpuid;
     bool enforce_cpuid;
+    /*
+     * Force features to be enabled even if the host doesn't support them.
+     * This is dangerous and should be done only for testing CPUID
+     * compatibility.
+     */
+    bool force_features;
     bool expose_kvm;
     bool expose_tcg;
     bool migratable;
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index f8d8f779c1..1bad957f6e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5140,8 +5140,11 @@ static int x86_cpu_filter_features(X86CPU *cpu)
         uint32_t host_feat =
             x86_cpu_get_supported_feature_word(w, false);
         uint32_t requested_features = env->features[w];
-        env->features[w] &= host_feat;
-        cpu->filtered_features[w] = requested_features & ~env->features[w];
+        uint32_t available_features = requested_features & host_feat;
+        if (!cpu->force_features) {
+            env->features[w] = available_features;
+        }
+        cpu->filtered_features[w] = requested_features & ~available_features;
         if (cpu->filtered_features[w]) {
             rv = 1;
         }
@@ -5866,6 +5869,7 @@ static Property x86_cpu_properties[] = {
 
     DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true),
     DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false),
+    DEFINE_PROP_BOOL("x-force-features", X86CPU, force_features, false),
     DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true),
     DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
     DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 2/6] i386: Remove unused host_cpudef variable

[Eduardo Habkost]

The variable is completely unused, probably a leftover from
previous code clean up.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1bad957f6e..cf03dc786e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3133,14 +3133,8 @@ static void max_x86_cpu_initfn(Object *obj)
         char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
         char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
         int family, model, stepping;
-        X86CPUDefinition host_cpudef = { };
-        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
-
-        host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-        x86_cpu_vendor_words2str(host_cpudef.vendor, ebx, edx, ecx);
 
         host_vendor_fms(vendor, &family, &model, &stepping);
-
         cpu_x86_fill_model_id(model_id);
 
         object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 2/6] i386: Remove unused host_cpudef variable

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> The variable is completely unused, probably a leftover from
> previous code clean up.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  target/i386/cpu.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 1bad957f6e..cf03dc786e 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3133,14 +3133,8 @@ static void max_x86_cpu_initfn(Object *obj)
>          char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
>          char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
>          int family, model, stepping;
> -        X86CPUDefinition host_cpudef = { };
> -        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> -
> -        host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -        x86_cpu_vendor_words2str(host_cpudef.vendor, ebx, edx, ecx);
>  
>          host_vendor_fms(vendor, &family, &model, &stepping);
> -
>          cpu_x86_fill_model_id(model_id);
>  
>          object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [PATCH 3/6] qmp: Add "alias-of" field to query-cpu-definitions

[Eduardo Habkost]

Management software will be expected to resolve CPU model name
aliases using the new field.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
---
 qapi/target.json | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qapi/target.json b/qapi/target.json
index 1d4d54b600..0197c7962c 100644
--- a/qapi/target.json
+++ b/qapi/target.json
@@ -475,6 +475,12 @@
 #            to introspect properties configurable using -cpu or -global.
 #            (since 2.9)
 #
+# @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.
+#            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)
+#
 # @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 there's no known
@@ -498,7 +504,8 @@
             '*migration-safe': 'bool',
             'static': 'bool',
             '*unavailable-features': [ 'str' ],
-            'typename': 'str' },
+            'typename': 'str',
+            '*alias-of' : 'str' },
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
 
 ##
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 3/6] qmp: Add "alias-of" field to query-cpu-definitions

[Daniel P. Berrangé]

On Tue, Jun 25, 2019 at 02:00:05AM -0300, Eduardo Habkost wrote:
> Management software will be expected to resolve CPU model name
> aliases using the new field.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> ---
>  qapi/target.json | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/target.json b/qapi/target.json
> index 1d4d54b600..0197c7962c 100644
> --- a/qapi/target.json
> +++ b/qapi/target.json
> @@ -475,6 +475,12 @@
>  #            to introspect properties configurable using -cpu or -global.
>  #            (since 2.9)
>  #
> +# @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.
> +#            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)
> +#
>  # @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 there's no known
> @@ -498,7 +504,8 @@
>              '*migration-safe': 'bool',
>              'static': 'bool',
>              '*unavailable-features': [ 'str' ],
> -            'typename': 'str' },
> +            'typename': 'str',
> +            '*alias-of' : 'str' },
>    'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }

IIUC, this means that data for a "Haswell" CPU model will now report
"alias-of": "Haswell-NNN"  (for some arbitrary NNN which may change
at will in any release).


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: [Qemu-devel] [PATCH 3/6] qmp: Add "alias-of" field to query-cpu-definitions

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:15:33PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:05AM -0300, Eduardo Habkost wrote:
> > Management software will be expected to resolve CPU model name
> > aliases using the new field.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: Eric Blake <eblake@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > ---
> >  qapi/target.json | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/target.json b/qapi/target.json
> > index 1d4d54b600..0197c7962c 100644
> > --- a/qapi/target.json
> > +++ b/qapi/target.json
> > @@ -475,6 +475,12 @@
> >  #            to introspect properties configurable using -cpu or -global.
> >  #            (since 2.9)
> >  #
> > +# @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.
> > +#            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)
> > +#
> >  # @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 there's no known
> > @@ -498,7 +504,8 @@
> >              '*migration-safe': 'bool',
> >              'static': 'bool',
> >              '*unavailable-features': [ 'str' ],
> > -            'typename': 'str' },
> > +            'typename': 'str',
> > +            '*alias-of' : 'str' },
> >    'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
> 
> IIUC, this means that data for a "Haswell" CPU model will now report
> "alias-of": "Haswell-NNN"  (for some arbitrary NNN which may change
> at will in any release).

That's correct.

-- 
Eduardo

[Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

Base code for versioned CPU models.  This will register a "-4.1"
version of all existing CPU models, and make the unversioned CPU
models be an alias for the -4.1 versions on the pc-*-4.1 machine
types.

On older machine types, the unversioned CPU models will keep the
old behavior.  This way, management software can use old machine
types while resolving aliases if compatibility with older QEMU
versions is required.

Using "-machine none", the unversioned CPU models will be aliases
to the latest CPU model version.

Includes a test case to ensure that:
old machine types won't report any alias to versioned CPU models;
"pc-*-4.1" will return aliases to -4.1 CPU models;
and "-machine none" will report aliases to some versioned CPU model.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
---
 include/hw/i386/pc.h                       |   3 +
 target/i386/cpu-qom.h                      |  10 +-
 target/i386/cpu.h                          |  10 ++
 hw/i386/pc.c                               |   3 +
 hw/i386/pc_piix.c                          |   4 +
 hw/i386/pc_q35.c                           |   4 +
 target/i386/cpu.c                          | 159 +++++++++++++++++----
 tests/acceptance/x86_cpu_model_versions.py | 102 +++++++++++++
 8 files changed, 263 insertions(+), 32 deletions(-)
 create mode 100644 tests/acceptance/x86_cpu_model_versions.py

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index c54cc54a47..d2e2ed072f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -107,6 +107,9 @@ typedef struct PCMachineClass {
 
     /* Compat options: */
 
+    /* Default CPU model version.  See x86_cpu_set_default_version(). */
+    const char *default_cpu_version;
+
     /* ACPI compat: */
     bool has_acpi_build;
     bool rsdp_in_ram;
diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
index 22f95eb3a4..1a52f02a4c 100644
--- a/target/i386/cpu-qom.h
+++ b/target/i386/cpu-qom.h
@@ -36,13 +36,7 @@
 #define X86_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
 
-/**
- * X86CPUDefinition:
- *
- * CPU model definition data that was not converted to QOM per-subclass
- * property defaults yet.
- */
-typedef struct X86CPUDefinition X86CPUDefinition;
+typedef struct X86CPUModel X86CPUModel;
 
 /**
  * X86CPUClass:
@@ -64,7 +58,7 @@ typedef struct X86CPUClass {
     /* CPU definition, automatically loaded by instance_init if not NULL.
      * Should be eventually replaced by subclass-specific property defaults.
      */
-    X86CPUDefinition *cpu_def;
+    X86CPUModel *model;
 
     bool host_cpuid_required;
     int ordering;
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 25544fdaaa..800bee3c6a 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1925,6 +1925,16 @@ void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
  */
 void x86_cpu_change_kvm_default(const char *prop, const char *value);
 
+/*
+ * Set default CPU model version for all CPU models
+ *
+ * If set to NULL, the old unversioned CPU models will be used by default.
+ *
+ * If non-NULL, the unversioned CPU models will be aliases to the
+ * corresponding version.
+ */
+void x86_cpu_set_default_version(const char *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 e96360b47a..d2852a77f8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1561,6 +1561,9 @@ 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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c07c4a5b38..9de86c71bd 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -430,9 +430,11 @@ static void pc_i440fx_machine_options(MachineClass *m)
 
 static void pc_i440fx_4_1_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
+    pcmc->default_cpu_version = "4.1";
 }
 
 DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
@@ -440,9 +442,11 @@ DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
 
 static void pc_i440fx_4_0_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_4_1_machine_options(m);
     m->alias = NULL;
     m->is_default = 0;
+    pcmc->default_cpu_version = NULL;
     compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
     compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 57232aed6b..7755d60167 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -367,8 +367,10 @@ static void pc_q35_machine_options(MachineClass *m)
 
 static void pc_q35_4_1_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
     m->alias = "q35";
+    pcmc->default_cpu_version = "4.1";
 }
 
 DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
@@ -376,8 +378,10 @@ DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
 
 static void pc_q35_4_0_1_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_4_1_machine_options(m);
     m->alias = NULL;
+    pcmc->default_cpu_version = NULL;
     /*
      * This is the default machine for the 4.0-stable branch. It is basically
      * a 4.0 that doesn't use split irqchip by default. It MUST hence apply the
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index cf03dc786e..121f568954 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1432,7 +1432,17 @@ static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
                      strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
 }
 
-struct X86CPUDefinition {
+typedef struct PropValue {
+    const char *prop, *value;
+} PropValue;
+
+typedef struct X86CPUVersionDefinition {
+    const char *name;
+    PropValue *props;
+} X86CPUVersionDefinition;
+
+/* Base definition for a CPU model */
+typedef struct X86CPUDefinition {
     const char *name;
     uint32_t level;
     uint32_t xlevel;
@@ -1444,8 +1454,32 @@ struct X86CPUDefinition {
     FeatureWordArray features;
     const char *model_id;
     CPUCaches *cache_info;
+    /*
+     * Definitions for alternative versions of CPU model.
+     * List is terminated by item with name==NULL.
+     * If NULL, base_cpu_versions will be used instead.
+     */
+    const X86CPUVersionDefinition *versions;
+} X86CPUDefinition;
+
+/* CPU model, which might include a specific CPU model version */
+struct X86CPUModel {
+    /* Base CPU definition */
+    X86CPUDefinition *cpudef;
+
+    /*
+     * CPU model version.  If NULL, version will be chosen depending on current
+     * machine.
+     */
+    const char *version;
 };
 
+static char *x86_cpu_versioned_model_name(X86CPUDefinition *cpudef,
+                                          const char *version)
+{
+    return g_strdup_printf("%s-%s", cpudef->name, version);
+}
+
 static CPUCaches epyc_cache_info = {
     .l1d_cache = &(CPUCacheInfo) {
         .type = DATA_CACHE,
@@ -3010,10 +3044,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
     },
 };
 
-typedef struct PropValue {
-    const char *prop, *value;
-} PropValue;
-
 /* KVM-specific features that are automatically added/removed
  * from all CPU models when KVM is enabled.
  */
@@ -3039,6 +3069,19 @@ static PropValue tcg_default_props[] = {
 };
 
 
+/* List of CPU model versions used when X86CPUDefinition::versions is NULL */
+static const X86CPUVersionDefinition base_cpu_versions[] = {
+    { "4.1" },
+    { /* end of list */ },
+};
+
+static const char *default_cpu_version = "4.1";
+
+void x86_cpu_set_default_version(const char *version)
+{
+    default_cpu_version = version;
+}
+
 void x86_cpu_change_kvm_default(const char *prop, const char *value)
 {
     PropValue *pv;
@@ -3116,8 +3159,6 @@ static void max_x86_cpu_class_init(ObjectClass *oc, void *data)
     dc->props = max_x86_cpu_properties;
 }
 
-static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp);
-
 static void max_x86_cpu_initfn(Object *obj)
 {
     X86CPU *cpu = X86_CPU(obj);
@@ -3771,8 +3812,8 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
     X86CPUClass *cc = X86_CPU_CLASS(oc);
     char *name = x86_cpu_class_get_model_name(cc);
     const char *desc = cc->model_description;
-    if (!desc && cc->cpu_def) {
-        desc = cc->cpu_def->model_id;
+    if (!desc && cc->model) {
+        desc = cc->model->cpudef->model_id;
     }
 
     qemu_printf("x86 %-20s  %-48s\n", name, desc);
@@ -3825,6 +3866,11 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
     info->migration_safe = cc->migration_safe;
     info->has_migration_safe = true;
     info->q_static = cc->static_model;
+    if (cc->model && !cc->model->version && default_cpu_version) {
+        info->has_alias_of = true;
+        info->alias_of = x86_cpu_versioned_model_name(cc->model->cpudef,
+                                                      default_cpu_version);
+    }
 
     entry = g_malloc0(sizeof(*entry));
     entry->value = info;
@@ -3898,10 +3944,38 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
     }
 }
 
+static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition *def)
+{
+    return def->versions ?: base_cpu_versions;
+}
+
+static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUDefinition *def,
+                                        const char *version)
+{
+    const X86CPUVersionDefinition *vdef;
+
+    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
+        PropValue *p;
+
+        for (p = vdef->props; p && p->prop; p++) {
+            object_property_parse(OBJECT(cpu), p->value, p->prop,
+                                  &error_abort);
+        }
+
+        if (!strcmp(vdef->name, version)) {
+            break;
+        }
+    }
+
+    /* If we reached the end of the list, version string was invalid */
+    assert(vdef->name);
+}
+
 /* Load data from X86CPUDefinition into a X86CPU object
  */
-static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
+static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
 {
+    X86CPUDefinition *def = model->cpudef;
     CPUX86State *env = &cpu->env;
     const char *vendor;
     char host_vendor[CPUID_VENDOR_SZ + 1];
@@ -3958,11 +4032,16 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
     object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
 
+    if (model->version) {
+        x86_cpu_apply_version_props(cpu, def, model->version);
+    } else if (default_cpu_version) {
+        x86_cpu_apply_version_props(cpu, def, default_cpu_version);
+    }
 }
 
 #ifndef CONFIG_USER_ONLY
 /* Return a QDict containing keys for all properties that can be included
- * in static expansion of CPU models. All properties set by x86_cpu_load_def()
+ * in static expansion of CPU models. All properties set by x86_cpu_load_model()
  * must be included in the dictionary.
  */
 static QDict *x86_cpu_static_props(void)
@@ -4176,23 +4255,44 @@ static gchar *x86_gdb_arch_name(CPUState *cs)
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
 {
-    X86CPUDefinition *cpudef = data;
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
 
-    xcc->cpu_def = cpudef;
+    xcc->model = data;
     xcc->migration_safe = true;
 }
 
-static void x86_register_cpudef_type(X86CPUDefinition *def)
+static char *x86_cpu_model_type_name(X86CPUModel *model)
 {
-    char *typename = x86_cpu_type_name(def->name);
+    if (model->version) {
+        char *name = x86_cpu_versioned_model_name(model->cpudef,
+                                                  model->version);
+        char *r = x86_cpu_type_name(name);
+        g_free(name);
+        return r;
+    } else {
+        return x86_cpu_type_name(model->cpudef->name);
+    }
+}
+
+static void x86_register_cpu_model_type(X86CPUModel *model)
+{
+    char *typename = x86_cpu_model_type_name(model);
     TypeInfo ti = {
         .name = typename,
         .parent = TYPE_X86_CPU,
         .class_init = x86_cpu_cpudef_class_init,
-        .class_data = def,
+        .class_data = model,
     };
 
+    type_register(&ti);
+    g_free(typename);
+}
+
+static void x86_register_cpudef_types(X86CPUDefinition *def)
+{
+    X86CPUModel *m;
+    const X86CPUVersionDefinition *vdef;
+
     /* AMD aliases are handled at runtime based on CPUID vendor, so
      * they shouldn't be set on the CPU model table.
      */
@@ -4200,9 +4300,20 @@ static void x86_register_cpudef_type(X86CPUDefinition *def)
     /* catch mistakes instead of silently truncating model_id when too long */
     assert(def->model_id && strlen(def->model_id) <= 48);
 
+    /* Unversioned model: */
+    m = g_new0(X86CPUModel, 1);
+    m->cpudef = def;
+    x86_register_cpu_model_type(m);
+
+    /* Versioned models: */
+
+    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
+        X86CPUModel *m = g_new0(X86CPUModel, 1);
+        m->cpudef = def;
+        m->version = vdef->name;
+        x86_register_cpu_model_type(m);
+    }
 
-    type_register(&ti);
-    g_free(typename);
 }
 
 #if !defined(CONFIG_USER_ONLY)
@@ -4989,7 +5100,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
  * involved in setting up CPUID data are:
  *
  * 1) Loading CPU model definition (X86CPUDefinition). This is
- *    implemented by x86_cpu_load_def() and should be completely
+ *    implemented by x86_cpu_load_model() and should be completely
  *    transparent, as it is done automatically by instance_init.
  *    No code should need to look at X86CPUDefinition structs
  *    outside instance_init.
@@ -5306,7 +5417,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
     /* Cache information initialization */
     if (!cpu->legacy_cache) {
-        if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
+        if (!xcc->model || !xcc->model->cpudef->cache_info) {
             char *name = x86_cpu_class_get_model_name(xcc);
             error_setg(errp,
                        "CPU model '%s' doesn't support legacy-cache=off", name);
@@ -5314,7 +5425,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
             return;
         }
         env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
-            *xcc->cpu_def->cache_info;
+            *xcc->model->cpudef->cache_info;
     } else {
         /* Build legacy cache information */
         env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
@@ -5671,8 +5782,8 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_alias(obj, "sse4_1", obj, "sse4.1", &error_abort);
     object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort);
 
-    if (xcc->cpu_def) {
-        x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
+    if (xcc->model) {
+        x86_cpu_load_model(cpu, xcc->model, &error_abort);
     }
 }
 
@@ -6009,7 +6120,7 @@ static void x86_cpu_register_types(void)
 
     type_register_static(&x86_cpu_type_info);
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
-        x86_register_cpudef_type(&builtin_x86_defs[i]);
+        x86_register_cpudef_types(&builtin_x86_defs[i]);
     }
     type_register_static(&max_x86_cpu_type_info);
     type_register_static(&x86_base_cpu_type_info);
diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
new file mode 100644
index 0000000000..c0660a552f
--- /dev/null
+++ b/tests/acceptance/x86_cpu_model_versions.py
@@ -0,0 +1,102 @@
+#!/usr/bin/env python
+#
+# Basic validation of x86 versioned CPU models and CPU model aliases
+#
+#  Copyright (c) 2019 Red Hat Inc
+#
+# Author:
+#  Eduardo Habkost <ehabkost@redhat.com>
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, see <http://www.gnu.org/licenses/>.
+#
+
+
+import avocado_qemu
+
+def get_cpu_prop(vm, prop):
+    cpu_path = vm.command('query-cpus')[0].get('qom_path')
+    return vm.command('qom-get', path=cpu_path, property=prop)
+
+class X86CPUModelAliases(avocado_qemu.Test):
+    """
+    Validation of PC CPU model versions and CPU model aliases
+
+    :avocado: tags=arch:x86_64
+    """
+    def test_4_0_alias(self):
+        """Check if pc-*-4.0 unversioned CPU model won't be an alias"""
+        # pc-*-4.0 won't expose non-versioned CPU models as aliases
+        # We do this to help management software to keep compatibility
+        # with older QEMU versions that didn't have the versioned CPU model
+        self.vm.add_args('-S')
+        self.vm.set_machine('pc-i440fx-4.0')
+        self.vm.launch()
+
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
+
+        self.assertFalse(cpus['Cascadelake-Server']['static'],
+                         'unversioned Cascadelake-Server CPU model must not be static')
+        self.assertNotIn('alias-of', cpus['Cascadelake-Server'],
+                         'Cascadelake-Server must not be an alias')
+
+        self.assertFalse(cpus['qemu64']['static'],
+                         'unversioned qemu64 CPU model must not be static')
+        self.assertNotIn('alias-of', cpus['qemu64'],
+                         'qemu64 must not be an alias')
+        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
+                         'qemu64-4.1 must not be an alias')
+
+    def test_4_1_alias(self):
+        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
+        self.vm.add_args('-S')
+        self.vm.set_machine('pc-i440fx-4.1')
+        self.vm.launch()
+
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
+
+        self.assertFalse(cpus['Cascadelake-Server']['static'],
+                         'unversioned Cascadelake-Server CPU model must not be static')
+        self.assertEquals(cpus['Cascadelake-Server'].get('alias-of'), 'Cascadelake-Server-4.1',
+                          'Cascadelake-Server must be an alias of Cascadelake-Server-4.1')
+        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
+                         'Cascadelake-Server-4.1 must not be an alias')
+
+        self.assertFalse(cpus['qemu64']['static'],
+                         'unversioned qemu64 CPU model must not be static')
+        self.assertEquals(cpus['qemu64'].get('alias-of'), 'qemu64-4.1',
+                          'qemu64 must be an alias of qemu64-4.1')
+        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
+                         'qemu64-4.1 must not be an alias')
+
+    def test_none_alias(self):
+        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
+        self.vm.add_args('-S')
+        self.vm.set_machine('none')
+        self.vm.launch()
+
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
+
+        self.assertFalse(cpus['Cascadelake-Server']['static'],
+                         'unversioned Cascadelake-Server CPU model must not be static')
+        self.assertTrue(cpus['Cascadelake-Server']['alias-of'].startswith('Cascadelake-Server-'),
+                          'Cascadelake-Server must be an alias of versioned CPU model')
+        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
+                         'Cascadelake-Server-4.1 must not be an alias')
+
+        self.assertFalse(cpus['qemu64']['static'],
+                         'unversioned qemu64 CPU model must not be static')
+        self.assertTrue(cpus['qemu64']['alias-of'].startswith('qemu64-'),
+                          'qemu64 must be an alias of versioned CPU model')
+        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
+                         'qemu64-4.1 must not be an alias')
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> Base code for versioned CPU models.  This will register a "-4.1"
> version of all existing CPU models, and make the unversioned CPU
> models be an alias for the -4.1 versions on the pc-*-4.1 machine
> types.
> 
> On older machine types, the unversioned CPU models will keep the
> old behavior.  This way, management software can use old machine
> types while resolving aliases if compatibility with older QEMU
> versions is required.
> 
> Using "-machine none", the unversioned CPU models will be aliases
> to the latest CPU model version.
> 
> Includes a test case to ensure that:
> old machine types won't report any alias to versioned CPU models;
> "pc-*-4.1" will return aliases to -4.1 CPU models;
> and "-machine none" will report aliases to some versioned CPU model.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

What happens when we add the next new CPU model?  So say in 4.2 we add
a new CPU, does that default to being newcpu-4.2 ?

Dave

> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> ---
>  include/hw/i386/pc.h                       |   3 +
>  target/i386/cpu-qom.h                      |  10 +-
>  target/i386/cpu.h                          |  10 ++
>  hw/i386/pc.c                               |   3 +
>  hw/i386/pc_piix.c                          |   4 +
>  hw/i386/pc_q35.c                           |   4 +
>  target/i386/cpu.c                          | 159 +++++++++++++++++----
>  tests/acceptance/x86_cpu_model_versions.py | 102 +++++++++++++
>  8 files changed, 263 insertions(+), 32 deletions(-)
>  create mode 100644 tests/acceptance/x86_cpu_model_versions.py
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index c54cc54a47..d2e2ed072f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -107,6 +107,9 @@ typedef struct PCMachineClass {
>  
>      /* Compat options: */
>  
> +    /* Default CPU model version.  See x86_cpu_set_default_version(). */
> +    const char *default_cpu_version;
> +
>      /* ACPI compat: */
>      bool has_acpi_build;
>      bool rsdp_in_ram;
> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
> index 22f95eb3a4..1a52f02a4c 100644
> --- a/target/i386/cpu-qom.h
> +++ b/target/i386/cpu-qom.h
> @@ -36,13 +36,7 @@
>  #define X86_CPU_GET_CLASS(obj) \
>      OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
>  
> -/**
> - * X86CPUDefinition:
> - *
> - * CPU model definition data that was not converted to QOM per-subclass
> - * property defaults yet.
> - */
> -typedef struct X86CPUDefinition X86CPUDefinition;
> +typedef struct X86CPUModel X86CPUModel;
>  
>  /**
>   * X86CPUClass:
> @@ -64,7 +58,7 @@ typedef struct X86CPUClass {
>      /* CPU definition, automatically loaded by instance_init if not NULL.
>       * Should be eventually replaced by subclass-specific property defaults.
>       */
> -    X86CPUDefinition *cpu_def;
> +    X86CPUModel *model;
>  
>      bool host_cpuid_required;
>      int ordering;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 25544fdaaa..800bee3c6a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1925,6 +1925,16 @@ void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>   */
>  void x86_cpu_change_kvm_default(const char *prop, const char *value);
>  
> +/*
> + * Set default CPU model version for all CPU models
> + *
> + * If set to NULL, the old unversioned CPU models will be used by default.
> + *
> + * If non-NULL, the unversioned CPU models will be aliases to the
> + * corresponding version.
> + */
> +void x86_cpu_set_default_version(const char *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 e96360b47a..d2852a77f8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1561,6 +1561,9 @@ 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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c07c4a5b38..9de86c71bd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -430,9 +430,11 @@ static void pc_i440fx_machine_options(MachineClass *m)
>  
>  static void pc_i440fx_4_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
> +    pcmc->default_cpu_version = "4.1";
>  }
>  
>  DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
> @@ -440,9 +442,11 @@ DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
>  
>  static void pc_i440fx_4_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_4_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = 0;
> +    pcmc->default_cpu_version = NULL;
>      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 57232aed6b..7755d60167 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -367,8 +367,10 @@ static void pc_q35_machine_options(MachineClass *m)
>  
>  static void pc_q35_4_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_machine_options(m);
>      m->alias = "q35";
> +    pcmc->default_cpu_version = "4.1";
>  }
>  
>  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> @@ -376,8 +378,10 @@ DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
>  
>  static void pc_q35_4_0_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_4_1_machine_options(m);
>      m->alias = NULL;
> +    pcmc->default_cpu_version = NULL;
>      /*
>       * This is the default machine for the 4.0-stable branch. It is basically
>       * a 4.0 that doesn't use split irqchip by default. It MUST hence apply the
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index cf03dc786e..121f568954 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1432,7 +1432,17 @@ static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
>                       strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
>  }
>  
> -struct X86CPUDefinition {
> +typedef struct PropValue {
> +    const char *prop, *value;
> +} PropValue;
> +
> +typedef struct X86CPUVersionDefinition {
> +    const char *name;
> +    PropValue *props;
> +} X86CPUVersionDefinition;
> +
> +/* Base definition for a CPU model */
> +typedef struct X86CPUDefinition {
>      const char *name;
>      uint32_t level;
>      uint32_t xlevel;
> @@ -1444,8 +1454,32 @@ struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> +    /*
> +     * Definitions for alternative versions of CPU model.
> +     * List is terminated by item with name==NULL.
> +     * If NULL, base_cpu_versions will be used instead.
> +     */
> +    const X86CPUVersionDefinition *versions;
> +} X86CPUDefinition;
> +
> +/* CPU model, which might include a specific CPU model version */
> +struct X86CPUModel {
> +    /* Base CPU definition */
> +    X86CPUDefinition *cpudef;
> +
> +    /*
> +     * CPU model version.  If NULL, version will be chosen depending on current
> +     * machine.
> +     */
> +    const char *version;
>  };
>  
> +static char *x86_cpu_versioned_model_name(X86CPUDefinition *cpudef,
> +                                          const char *version)
> +{
> +    return g_strdup_printf("%s-%s", cpudef->name, version);
> +}
> +
>  static CPUCaches epyc_cache_info = {
>      .l1d_cache = &(CPUCacheInfo) {
>          .type = DATA_CACHE,
> @@ -3010,10 +3044,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>      },
>  };
>  
> -typedef struct PropValue {
> -    const char *prop, *value;
> -} PropValue;
> -
>  /* KVM-specific features that are automatically added/removed
>   * from all CPU models when KVM is enabled.
>   */
> @@ -3039,6 +3069,19 @@ static PropValue tcg_default_props[] = {
>  };
>  
>  
> +/* List of CPU model versions used when X86CPUDefinition::versions is NULL */
> +static const X86CPUVersionDefinition base_cpu_versions[] = {
> +    { "4.1" },
> +    { /* end of list */ },
> +};
> +
> +static const char *default_cpu_version = "4.1";
> +
> +void x86_cpu_set_default_version(const char *version)
> +{
> +    default_cpu_version = version;
> +}
> +
>  void x86_cpu_change_kvm_default(const char *prop, const char *value)
>  {
>      PropValue *pv;
> @@ -3116,8 +3159,6 @@ static void max_x86_cpu_class_init(ObjectClass *oc, void *data)
>      dc->props = max_x86_cpu_properties;
>  }
>  
> -static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp);
> -
>  static void max_x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> @@ -3771,8 +3812,8 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
>      X86CPUClass *cc = X86_CPU_CLASS(oc);
>      char *name = x86_cpu_class_get_model_name(cc);
>      const char *desc = cc->model_description;
> -    if (!desc && cc->cpu_def) {
> -        desc = cc->cpu_def->model_id;
> +    if (!desc && cc->model) {
> +        desc = cc->model->cpudef->model_id;
>      }
>  
>      qemu_printf("x86 %-20s  %-48s\n", name, desc);
> @@ -3825,6 +3866,11 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
>      info->migration_safe = cc->migration_safe;
>      info->has_migration_safe = true;
>      info->q_static = cc->static_model;
> +    if (cc->model && !cc->model->version && default_cpu_version) {
> +        info->has_alias_of = true;
> +        info->alias_of = x86_cpu_versioned_model_name(cc->model->cpudef,
> +                                                      default_cpu_version);
> +    }
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;
> @@ -3898,10 +3944,38 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
>      }
>  }
>  
> +static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition *def)
> +{
> +    return def->versions ?: base_cpu_versions;
> +}
> +
> +static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUDefinition *def,
> +                                        const char *version)
> +{
> +    const X86CPUVersionDefinition *vdef;
> +
> +    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
> +        PropValue *p;
> +
> +        for (p = vdef->props; p && p->prop; p++) {
> +            object_property_parse(OBJECT(cpu), p->value, p->prop,
> +                                  &error_abort);
> +        }
> +
> +        if (!strcmp(vdef->name, version)) {
> +            break;
> +        }
> +    }
> +
> +    /* If we reached the end of the list, version string was invalid */
> +    assert(vdef->name);
> +}
> +
>  /* Load data from X86CPUDefinition into a X86CPU object
>   */
> -static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
> +static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
>  {
> +    X86CPUDefinition *def = model->cpudef;
>      CPUX86State *env = &cpu->env;
>      const char *vendor;
>      char host_vendor[CPUID_VENDOR_SZ + 1];
> @@ -3958,11 +4032,16 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  
>      object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
>  
> +    if (model->version) {
> +        x86_cpu_apply_version_props(cpu, def, model->version);
> +    } else if (default_cpu_version) {
> +        x86_cpu_apply_version_props(cpu, def, default_cpu_version);
> +    }
>  }
>  
>  #ifndef CONFIG_USER_ONLY
>  /* Return a QDict containing keys for all properties that can be included
> - * in static expansion of CPU models. All properties set by x86_cpu_load_def()
> + * in static expansion of CPU models. All properties set by x86_cpu_load_model()
>   * must be included in the dictionary.
>   */
>  static QDict *x86_cpu_static_props(void)
> @@ -4176,23 +4255,44 @@ static gchar *x86_gdb_arch_name(CPUState *cs)
>  
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
> -    X86CPUDefinition *cpudef = data;
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
>  
> -    xcc->cpu_def = cpudef;
> +    xcc->model = data;
>      xcc->migration_safe = true;
>  }
>  
> -static void x86_register_cpudef_type(X86CPUDefinition *def)
> +static char *x86_cpu_model_type_name(X86CPUModel *model)
>  {
> -    char *typename = x86_cpu_type_name(def->name);
> +    if (model->version) {
> +        char *name = x86_cpu_versioned_model_name(model->cpudef,
> +                                                  model->version);
> +        char *r = x86_cpu_type_name(name);
> +        g_free(name);
> +        return r;
> +    } else {
> +        return x86_cpu_type_name(model->cpudef->name);
> +    }
> +}
> +
> +static void x86_register_cpu_model_type(X86CPUModel *model)
> +{
> +    char *typename = x86_cpu_model_type_name(model);
>      TypeInfo ti = {
>          .name = typename,
>          .parent = TYPE_X86_CPU,
>          .class_init = x86_cpu_cpudef_class_init,
> -        .class_data = def,
> +        .class_data = model,
>      };
>  
> +    type_register(&ti);
> +    g_free(typename);
> +}
> +
> +static void x86_register_cpudef_types(X86CPUDefinition *def)
> +{
> +    X86CPUModel *m;
> +    const X86CPUVersionDefinition *vdef;
> +
>      /* AMD aliases are handled at runtime based on CPUID vendor, so
>       * they shouldn't be set on the CPU model table.
>       */
> @@ -4200,9 +4300,20 @@ static void x86_register_cpudef_type(X86CPUDefinition *def)
>      /* catch mistakes instead of silently truncating model_id when too long */
>      assert(def->model_id && strlen(def->model_id) <= 48);
>  
> +    /* Unversioned model: */
> +    m = g_new0(X86CPUModel, 1);
> +    m->cpudef = def;
> +    x86_register_cpu_model_type(m);
> +
> +    /* Versioned models: */
> +
> +    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
> +        X86CPUModel *m = g_new0(X86CPUModel, 1);
> +        m->cpudef = def;
> +        m->version = vdef->name;
> +        x86_register_cpu_model_type(m);
> +    }
>  
> -    type_register(&ti);
> -    g_free(typename);
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -4989,7 +5100,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>   * involved in setting up CPUID data are:
>   *
>   * 1) Loading CPU model definition (X86CPUDefinition). This is
> - *    implemented by x86_cpu_load_def() and should be completely
> + *    implemented by x86_cpu_load_model() and should be completely
>   *    transparent, as it is done automatically by instance_init.
>   *    No code should need to look at X86CPUDefinition structs
>   *    outside instance_init.
> @@ -5306,7 +5417,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      /* Cache information initialization */
>      if (!cpu->legacy_cache) {
> -        if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
> +        if (!xcc->model || !xcc->model->cpudef->cache_info) {
>              char *name = x86_cpu_class_get_model_name(xcc);
>              error_setg(errp,
>                         "CPU model '%s' doesn't support legacy-cache=off", name);
> @@ -5314,7 +5425,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>              return;
>          }
>          env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
> -            *xcc->cpu_def->cache_info;
> +            *xcc->model->cpudef->cache_info;
>      } else {
>          /* Build legacy cache information */
>          env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
> @@ -5671,8 +5782,8 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1", &error_abort);
>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort);
>  
> -    if (xcc->cpu_def) {
> -        x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
> +    if (xcc->model) {
> +        x86_cpu_load_model(cpu, xcc->model, &error_abort);
>      }
>  }
>  
> @@ -6009,7 +6120,7 @@ static void x86_cpu_register_types(void)
>  
>      type_register_static(&x86_cpu_type_info);
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> -        x86_register_cpudef_type(&builtin_x86_defs[i]);
> +        x86_register_cpudef_types(&builtin_x86_defs[i]);
>      }
>      type_register_static(&max_x86_cpu_type_info);
>      type_register_static(&x86_base_cpu_type_info);
> diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
> new file mode 100644
> index 0000000000..c0660a552f
> --- /dev/null
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -0,0 +1,102 @@
> +#!/usr/bin/env python
> +#
> +# Basic validation of x86 versioned CPU models and CPU model aliases
> +#
> +#  Copyright (c) 2019 Red Hat Inc
> +#
> +# Author:
> +#  Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +
> +import avocado_qemu
> +
> +def get_cpu_prop(vm, prop):
> +    cpu_path = vm.command('query-cpus')[0].get('qom_path')
> +    return vm.command('qom-get', path=cpu_path, property=prop)
> +
> +class X86CPUModelAliases(avocado_qemu.Test):
> +    """
> +    Validation of PC CPU model versions and CPU model aliases
> +
> +    :avocado: tags=arch:x86_64
> +    """
> +    def test_4_0_alias(self):
> +        """Check if pc-*-4.0 unversioned CPU model won't be an alias"""
> +        # pc-*-4.0 won't expose non-versioned CPU models as aliases
> +        # We do this to help management software to keep compatibility
> +        # with older QEMU versions that didn't have the versioned CPU model
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('pc-i440fx-4.0')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server'],
> +                         'Cascadelake-Server must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertNotIn('alias-of', cpus['qemu64'],
> +                         'qemu64 must not be an alias')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> +
> +    def test_4_1_alias(self):
> +        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('pc-i440fx-4.1')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertEquals(cpus['Cascadelake-Server'].get('alias-of'), 'Cascadelake-Server-4.1',
> +                          'Cascadelake-Server must be an alias of Cascadelake-Server-4.1')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
> +                         'Cascadelake-Server-4.1 must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertEquals(cpus['qemu64'].get('alias-of'), 'qemu64-4.1',
> +                          'qemu64 must be an alias of qemu64-4.1')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> +
> +    def test_none_alias(self):
> +        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertTrue(cpus['Cascadelake-Server']['alias-of'].startswith('Cascadelake-Server-'),
> +                          'Cascadelake-Server must be an alias of versioned CPU model')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
> +                         'Cascadelake-Server-4.1 must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertTrue(cpus['qemu64']['alias-of'].startswith('qemu64-'),
> +                          'qemu64 must be an alias of versioned CPU model')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> > 
> > On older machine types, the unversioned CPU models will keep the
> > old behavior.  This way, management software can use old machine
> > types while resolving aliases if compatibility with older QEMU
> > versions is required.
> > 
> > Using "-machine none", the unversioned CPU models will be aliases
> > to the latest CPU model version.
> > 
> > Includes a test case to ensure that:
> > old machine types won't report any alias to versioned CPU models;
> > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > and "-machine none" will report aliases to some versioned CPU model.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> What happens when we add the next new CPU model?  So say in 4.2 we add
> a new CPU, does that default to being newcpu-4.2 ?

We can choose between providing old versions of the CPU model
retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
only "NewModel-4.2".

The question is: if we provide only "NewModel-4.2", what should
be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > Base code for versioned CPU models.  This will register a "-4.1"
> > > version of all existing CPU models, and make the unversioned CPU
> > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > types.
> > > 
> > > On older machine types, the unversioned CPU models will keep the
> > > old behavior.  This way, management software can use old machine
> > > types while resolving aliases if compatibility with older QEMU
> > > versions is required.
> > > 
> > > Using "-machine none", the unversioned CPU models will be aliases
> > > to the latest CPU model version.
> > > 
> > > Includes a test case to ensure that:
> > > old machine types won't report any alias to versioned CPU models;
> > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > and "-machine none" will report aliases to some versioned CPU model.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > 
> > What happens when we add the next new CPU model?  So say in 4.2 we add
> > a new CPU, does that default to being newcpu-4.2 ?
> 
> We can choose between providing old versions of the CPU model
> retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> only "NewModel-4.2".
> 
> The question is: if we provide only "NewModel-4.2", what should
> be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

Perhaps the existing CPUs and the first instance of a new CPU
we should use something non-numeric, e.g. 'orig' rather than 4.1;
we only go numeric when we cause a divergence.

Dave

> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > version of all existing CPU models, and make the unversioned CPU
> > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > types.
> > > > 
> > > > On older machine types, the unversioned CPU models will keep the
> > > > old behavior.  This way, management software can use old machine
> > > > types while resolving aliases if compatibility with older QEMU
> > > > versions is required.
> > > > 
> > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > to the latest CPU model version.
> > > > 
> > > > Includes a test case to ensure that:
> > > > old machine types won't report any alias to versioned CPU models;
> > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > 
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > 
> > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > a new CPU, does that default to being newcpu-4.2 ?
> > 
> > We can choose between providing old versions of the CPU model
> > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > only "NewModel-4.2".
> > 
> > The question is: if we provide only "NewModel-4.2", what should
> > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> 
> Perhaps the existing CPUs and the first instance of a new CPU
> we should use something non-numeric, e.g. 'orig' rather than 4.1;
> we only go numeric when we cause a divergence.

What would be the advantage of a non-numeric version identifier?
I believe it would be more confusing to have (e.g.)
["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
["NewModel-4.2", "NewModel-4.3"].

However, you have another interesting point: should we introduce
-4.2 versions of all CPU models in QEMU 4.2, or only for the ones
that actually changed?  I think I prefer consistency, even if it
means making the list of CPU models larger.

What do others think?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > > version of all existing CPU models, and make the unversioned CPU
> > > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > > types.
> > > > > 
> > > > > On older machine types, the unversioned CPU models will keep the
> > > > > old behavior.  This way, management software can use old machine
> > > > > types while resolving aliases if compatibility with older QEMU
> > > > > versions is required.
> > > > > 
> > > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > > to the latest CPU model version.
> > > > > 
> > > > > Includes a test case to ensure that:
> > > > > old machine types won't report any alias to versioned CPU models;
> > > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > > 
> > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > 
> > > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > > a new CPU, does that default to being newcpu-4.2 ?
> > > 
> > > We can choose between providing old versions of the CPU model
> > > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > > only "NewModel-4.2".
> > > 
> > > The question is: if we provide only "NewModel-4.2", what should
> > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> > 
> > Perhaps the existing CPUs and the first instance of a new CPU
> > we should use something non-numeric, e.g. 'orig' rather than 4.1;
> > we only go numeric when we cause a divergence.
> 
> What would be the advantage of a non-numeric version identifier?
> I believe it would be more confusing to have (e.g.)
> ["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
> ["NewModel-4.2", "NewModel-4.3"].

To my mind it answers your question:
> > > The question is: if we provide only "NewModel-4.2", what should
> > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?

NewModel-orig doesn't look weird in pc-i440fx-4.1

Dave


> However, you have another interesting point: should we introduce
> -4.2 versions of all CPU models in QEMU 4.2, or only for the ones
> that actually changed?  I think I prefer consistency, even if it
> means making the list of CPU models larger.
> 
> What do others think?
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 03:56:51PM +0100, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Tue, Jun 25, 2019 at 03:32:16PM +0100, Dr. David Alan Gilbert wrote:
> > > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > > On Tue, Jun 25, 2019 at 10:32:01AM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > > > > Base code for versioned CPU models.  This will register a "-4.1"
> > > > > > version of all existing CPU models, and make the unversioned CPU
> > > > > > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > > > > > types.
> > > > > > 
> > > > > > On older machine types, the unversioned CPU models will keep the
> > > > > > old behavior.  This way, management software can use old machine
> > > > > > types while resolving aliases if compatibility with older QEMU
> > > > > > versions is required.
> > > > > > 
> > > > > > Using "-machine none", the unversioned CPU models will be aliases
> > > > > > to the latest CPU model version.
> > > > > > 
> > > > > > Includes a test case to ensure that:
> > > > > > old machine types won't report any alias to versioned CPU models;
> > > > > > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > > > > > and "-machine none" will report aliases to some versioned CPU model.
> > > > > > 
> > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > 
> > > > > What happens when we add the next new CPU model?  So say in 4.2 we add
> > > > > a new CPU, does that default to being newcpu-4.2 ?
> > > > 
> > > > We can choose between providing old versions of the CPU model
> > > > retroactively ("NewModel-4.1" and "NewModel-4.2"), or providing
> > > > only "NewModel-4.2".
> > > > 
> > > > The question is: if we provide only "NewModel-4.2", what should
> > > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> > > 
> > > Perhaps the existing CPUs and the first instance of a new CPU
> > > we should use something non-numeric, e.g. 'orig' rather than 4.1;
> > > we only go numeric when we cause a divergence.
> > 
> > What would be the advantage of a non-numeric version identifier?
> > I believe it would be more confusing to have (e.g.)
> > ["NewModel-orig", "NewModel-4.3"] in QEMU 4.3 instead of
> > ["NewModel-4.2", "NewModel-4.3"].
> 
> To my mind it answers your question:
> > > > The question is: if we provide only "NewModel-4.2", what should
> > > > be the behavior of "-machine pc-i440fx-4.1 -cpu NewModel"?
> 
> NewModel-orig doesn't look weird in pc-i440fx-4.1

It doesn't look weird in the surface, but it doesn't change the
fact that "NewModel-orig" is the version of NewModel that was
shipped in in QEMU 4.2.  Why would we want to hide that fact?

What's the advantage of:

  "-machine pc-i440fx-4.1 -cpu NewModel" will use "NewModel-orig",
  which is the version of NewModel added in QEMU 4.2.

in relation to:

  "-machine pc-i440fx-4.1 -cpu NewModel" will use "NewModel-4.2",
  which is the version of NewModel added in QEMU 4.2.


-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Daniel P. Berrangé]

On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> Base code for versioned CPU models.  This will register a "-4.1"
> version of all existing CPU models, and make the unversioned CPU
> models be an alias for the -4.1 versions on the pc-*-4.1 machine
> types.
> 
> On older machine types, the unversioned CPU models will keep the
> old behavior.  This way, management software can use old machine
> types while resolving aliases if compatibility with older QEMU
> versions is required.
> 
> Using "-machine none", the unversioned CPU models will be aliases
> to the latest CPU model version.
> 
> Includes a test case to ensure that:
> old machine types won't report any alias to versioned CPU models;
> "pc-*-4.1" will return aliases to -4.1 CPU models;
> and "-machine none" will report aliases to some versioned CPU model.

I'm wondering about the of tieing CPU versions to the release version
number and whether its a good idea or not ?

Could there be a reason for us to introduce 2 or more variants
of a CPU in the same release & would that be a problem if we needed
todo it ?

Consider if we did not have a Broadwell CPU model defined yet
and we were adding it at the same time as Spectre came out. We
might have needed to add "Broadwell-NN" and "Broadwell-MM" one
with "spec-ctrl" and one without, in order to ensure runability
on hosts with & without the microcode upgrade.  "Broadwell" alias
would resolve to either the NN or MM variant according to what
the current host supported.

One way to cope with that would have been to add a 3rd digit
after the version number. eg a Broadwell-4.1.1 and Broadwell-4.1.2

An alternative could consider using a plain counter for the CPU
versions eg Broadwell-1, Broadwell-2, etc.... ?


If we want to backport the newly added CPU model variants to
exist branches, plain counters don't have the unsightly mismatch.
eg we'd backport Broadwell-3 to QEMU 3.1, not Broadwell-4.1 to
QEMU 3.1.  This isn't a functional problem, just something looking
a bit odd.


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: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:26:03PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> > 
> > On older machine types, the unversioned CPU models will keep the
> > old behavior.  This way, management software can use old machine
> > types while resolving aliases if compatibility with older QEMU
> > versions is required.
> > 
> > Using "-machine none", the unversioned CPU models will be aliases
> > to the latest CPU model version.
> > 
> > Includes a test case to ensure that:
> > old machine types won't report any alias to versioned CPU models;
> > "pc-*-4.1" will return aliases to -4.1 CPU models;
> > and "-machine none" will report aliases to some versioned CPU model.
> 
> I'm wondering about the of tieing CPU versions to the release version
> number and whether its a good idea or not ?
> 
> Could there be a reason for us to introduce 2 or more variants
> of a CPU in the same release & would that be a problem if we needed
> todo it ?

I don't see a problem, we can use 3-digit versions that won't be
enabled by any machine type by default.

> 
> Consider if we did not have a Broadwell CPU model defined yet
> and we were adding it at the same time as Spectre came out. We
> might have needed to add "Broadwell-NN" and "Broadwell-MM" one
> with "spec-ctrl" and one without, in order to ensure runability
> on hosts with & without the microcode upgrade.  "Broadwell" alias
> would resolve to either the NN or MM variant according to what
> the current host supported.
> 
> One way to cope with that would have been to add a 3rd digit
> after the version number. eg a Broadwell-4.1.1 and Broadwell-4.1.2

That's exactly what I did for Cascadelake-Server, see patch 6/6.

> 
> An alternative could consider using a plain counter for the CPU
> versions eg Broadwell-1, Broadwell-2, etc.... ?

This is possible too.  It would require a more complex mapping
between machine types and CPU model versions, though.  Maybe this
is worth the extra complexity because it would make the external
interfaces simpler.

> 
> 
> If we want to backport the newly added CPU model variants to
> exist branches, plain counters don't have the unsightly mismatch.
> eg we'd backport Broadwell-3 to QEMU 3.1, not Broadwell-4.1 to
> QEMU 3.1.  This isn't a functional problem, just something looking
> a bit odd.

I think I'm liking this approach.  If we're untying CPU models
from machine types, let's do it all the way.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Daniel P. Berrangé]

On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> Base code for versioned CPU models.  This will register a "-4.1"
> version of all existing CPU models, and make the unversioned CPU
> models be an alias for the -4.1 versions on the pc-*-4.1 machine
> types.

Currently we have some CPUs that I'd consider historical "mistakes"
due to fact that versioning didn't previously exist.

eg

   Haswell
   Haswell-noTSX
   Haswell-noTSX-IBRS

IIUC this patch adds

  Haswell            alias-of Haswell-4.1
  Haswell-noTSX      alias-of Haswell-noTSX-4.1
  Haswell-noTSX-IBRS alias-of Haswell-noTSX-IBRS-4.1

I'm thinking we should instead be merging all these haswell variants


  Haswell            alias-of Haswell-4.1.1
  Haswell-noTSX      alias-of Haswell-4.1.2
  Haswell-noTSX-IBRS alias-of Haswell-4.1.3

Or if we used the simple counter versioning

  Haswell            alias-of Haswell-1
  Haswell-noTSX      alias-of Haswell-2
  Haswell-noTSX-IBRS alias-of Haswell-3

Likewise for the other named CPUs with wierd variants.

This would effectively be "deprecating" the noTSX and IBRS variants
in favour of using the versioning approach

> 
> On older machine types, the unversioned CPU models will keep the
> old behavior.  This way, management software can use old machine
> types while resolving aliases if compatibility with older QEMU
> versions is required.
> 
> Using "-machine none", the unversioned CPU models will be aliases
> to the latest CPU model version.
> 
> Includes a test case to ensure that:
> old machine types won't report any alias to versioned CPU models;
> "pc-*-4.1" will return aliases to -4.1 CPU models;
> and "-machine none" will report aliases to some versioned CPU model.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> ---
>  include/hw/i386/pc.h                       |   3 +
>  target/i386/cpu-qom.h                      |  10 +-
>  target/i386/cpu.h                          |  10 ++
>  hw/i386/pc.c                               |   3 +
>  hw/i386/pc_piix.c                          |   4 +
>  hw/i386/pc_q35.c                           |   4 +
>  target/i386/cpu.c                          | 159 +++++++++++++++++----
>  tests/acceptance/x86_cpu_model_versions.py | 102 +++++++++++++
>  8 files changed, 263 insertions(+), 32 deletions(-)
>  create mode 100644 tests/acceptance/x86_cpu_model_versions.py
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index c54cc54a47..d2e2ed072f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -107,6 +107,9 @@ typedef struct PCMachineClass {
>  
>      /* Compat options: */
>  
> +    /* Default CPU model version.  See x86_cpu_set_default_version(). */
> +    const char *default_cpu_version;
> +
>      /* ACPI compat: */
>      bool has_acpi_build;
>      bool rsdp_in_ram;
> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
> index 22f95eb3a4..1a52f02a4c 100644
> --- a/target/i386/cpu-qom.h
> +++ b/target/i386/cpu-qom.h
> @@ -36,13 +36,7 @@
>  #define X86_CPU_GET_CLASS(obj) \
>      OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU)
>  
> -/**
> - * X86CPUDefinition:
> - *
> - * CPU model definition data that was not converted to QOM per-subclass
> - * property defaults yet.
> - */
> -typedef struct X86CPUDefinition X86CPUDefinition;
> +typedef struct X86CPUModel X86CPUModel;
>  
>  /**
>   * X86CPUClass:
> @@ -64,7 +58,7 @@ typedef struct X86CPUClass {
>      /* CPU definition, automatically loaded by instance_init if not NULL.
>       * Should be eventually replaced by subclass-specific property defaults.
>       */
> -    X86CPUDefinition *cpu_def;
> +    X86CPUModel *model;
>  
>      bool host_cpuid_required;
>      int ordering;
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 25544fdaaa..800bee3c6a 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1925,6 +1925,16 @@ void apic_handle_tpr_access_report(DeviceState *d, target_ulong ip,
>   */
>  void x86_cpu_change_kvm_default(const char *prop, const char *value);
>  
> +/*
> + * Set default CPU model version for all CPU models
> + *
> + * If set to NULL, the old unversioned CPU models will be used by default.
> + *
> + * If non-NULL, the unversioned CPU models will be aliases to the
> + * corresponding version.
> + */
> +void x86_cpu_set_default_version(const char *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 e96360b47a..d2852a77f8 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1561,6 +1561,9 @@ 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/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c07c4a5b38..9de86c71bd 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -430,9 +430,11 @@ static void pc_i440fx_machine_options(MachineClass *m)
>  
>  static void pc_i440fx_4_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
> +    pcmc->default_cpu_version = "4.1";
>  }
>  
>  DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
> @@ -440,9 +442,11 @@ DEFINE_I440FX_MACHINE(v4_1, "pc-i440fx-4.1", NULL,
>  
>  static void pc_i440fx_4_0_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_4_1_machine_options(m);
>      m->alias = NULL;
>      m->is_default = 0;
> +    pcmc->default_cpu_version = NULL;
>      compat_props_add(m->compat_props, hw_compat_4_0, hw_compat_4_0_len);
>      compat_props_add(m->compat_props, pc_compat_4_0, pc_compat_4_0_len);
>  }
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 57232aed6b..7755d60167 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -367,8 +367,10 @@ static void pc_q35_machine_options(MachineClass *m)
>  
>  static void pc_q35_4_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_machine_options(m);
>      m->alias = "q35";
> +    pcmc->default_cpu_version = "4.1";
>  }
>  
>  DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
> @@ -376,8 +378,10 @@ DEFINE_Q35_MACHINE(v4_1, "pc-q35-4.1", NULL,
>  
>  static void pc_q35_4_0_1_machine_options(MachineClass *m)
>  {
> +    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_4_1_machine_options(m);
>      m->alias = NULL;
> +    pcmc->default_cpu_version = NULL;
>      /*
>       * This is the default machine for the 4.0-stable branch. It is basically
>       * a 4.0 that doesn't use split irqchip by default. It MUST hence apply the
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index cf03dc786e..121f568954 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1432,7 +1432,17 @@ static char *x86_cpu_class_get_model_name(X86CPUClass *cc)
>                       strlen(class_name) - strlen(X86_CPU_TYPE_SUFFIX));
>  }
>  
> -struct X86CPUDefinition {
> +typedef struct PropValue {
> +    const char *prop, *value;
> +} PropValue;
> +
> +typedef struct X86CPUVersionDefinition {
> +    const char *name;
> +    PropValue *props;
> +} X86CPUVersionDefinition;
> +
> +/* Base definition for a CPU model */
> +typedef struct X86CPUDefinition {
>      const char *name;
>      uint32_t level;
>      uint32_t xlevel;
> @@ -1444,8 +1454,32 @@ struct X86CPUDefinition {
>      FeatureWordArray features;
>      const char *model_id;
>      CPUCaches *cache_info;
> +    /*
> +     * Definitions for alternative versions of CPU model.
> +     * List is terminated by item with name==NULL.
> +     * If NULL, base_cpu_versions will be used instead.
> +     */
> +    const X86CPUVersionDefinition *versions;
> +} X86CPUDefinition;
> +
> +/* CPU model, which might include a specific CPU model version */
> +struct X86CPUModel {
> +    /* Base CPU definition */
> +    X86CPUDefinition *cpudef;
> +
> +    /*
> +     * CPU model version.  If NULL, version will be chosen depending on current
> +     * machine.
> +     */
> +    const char *version;
>  };
>  
> +static char *x86_cpu_versioned_model_name(X86CPUDefinition *cpudef,
> +                                          const char *version)
> +{
> +    return g_strdup_printf("%s-%s", cpudef->name, version);
> +}
> +
>  static CPUCaches epyc_cache_info = {
>      .l1d_cache = &(CPUCacheInfo) {
>          .type = DATA_CACHE,
> @@ -3010,10 +3044,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>      },
>  };
>  
> -typedef struct PropValue {
> -    const char *prop, *value;
> -} PropValue;
> -
>  /* KVM-specific features that are automatically added/removed
>   * from all CPU models when KVM is enabled.
>   */
> @@ -3039,6 +3069,19 @@ static PropValue tcg_default_props[] = {
>  };
>  
>  
> +/* List of CPU model versions used when X86CPUDefinition::versions is NULL */
> +static const X86CPUVersionDefinition base_cpu_versions[] = {
> +    { "4.1" },
> +    { /* end of list */ },
> +};
> +
> +static const char *default_cpu_version = "4.1";
> +
> +void x86_cpu_set_default_version(const char *version)
> +{
> +    default_cpu_version = version;
> +}
> +
>  void x86_cpu_change_kvm_default(const char *prop, const char *value)
>  {
>      PropValue *pv;
> @@ -3116,8 +3159,6 @@ static void max_x86_cpu_class_init(ObjectClass *oc, void *data)
>      dc->props = max_x86_cpu_properties;
>  }
>  
> -static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp);
> -
>  static void max_x86_cpu_initfn(Object *obj)
>  {
>      X86CPU *cpu = X86_CPU(obj);
> @@ -3771,8 +3812,8 @@ static void x86_cpu_list_entry(gpointer data, gpointer user_data)
>      X86CPUClass *cc = X86_CPU_CLASS(oc);
>      char *name = x86_cpu_class_get_model_name(cc);
>      const char *desc = cc->model_description;
> -    if (!desc && cc->cpu_def) {
> -        desc = cc->cpu_def->model_id;
> +    if (!desc && cc->model) {
> +        desc = cc->model->cpudef->model_id;
>      }
>  
>      qemu_printf("x86 %-20s  %-48s\n", name, desc);
> @@ -3825,6 +3866,11 @@ static void x86_cpu_definition_entry(gpointer data, gpointer user_data)
>      info->migration_safe = cc->migration_safe;
>      info->has_migration_safe = true;
>      info->q_static = cc->static_model;
> +    if (cc->model && !cc->model->version && default_cpu_version) {
> +        info->has_alias_of = true;
> +        info->alias_of = x86_cpu_versioned_model_name(cc->model->cpudef,
> +                                                      default_cpu_version);
> +    }
>  
>      entry = g_malloc0(sizeof(*entry));
>      entry->value = info;
> @@ -3898,10 +3944,38 @@ static void x86_cpu_apply_props(X86CPU *cpu, PropValue *props)
>      }
>  }
>  
> +static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition *def)
> +{
> +    return def->versions ?: base_cpu_versions;
> +}
> +
> +static void x86_cpu_apply_version_props(X86CPU *cpu, X86CPUDefinition *def,
> +                                        const char *version)
> +{
> +    const X86CPUVersionDefinition *vdef;
> +
> +    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
> +        PropValue *p;
> +
> +        for (p = vdef->props; p && p->prop; p++) {
> +            object_property_parse(OBJECT(cpu), p->value, p->prop,
> +                                  &error_abort);
> +        }
> +
> +        if (!strcmp(vdef->name, version)) {
> +            break;
> +        }
> +    }
> +
> +    /* If we reached the end of the list, version string was invalid */
> +    assert(vdef->name);
> +}
> +
>  /* Load data from X86CPUDefinition into a X86CPU object
>   */
> -static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
> +static void x86_cpu_load_model(X86CPU *cpu, X86CPUModel *model, Error **errp)
>  {
> +    X86CPUDefinition *def = model->cpudef;
>      CPUX86State *env = &cpu->env;
>      const char *vendor;
>      char host_vendor[CPUID_VENDOR_SZ + 1];
> @@ -3958,11 +4032,16 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  
>      object_property_set_str(OBJECT(cpu), vendor, "vendor", errp);
>  
> +    if (model->version) {
> +        x86_cpu_apply_version_props(cpu, def, model->version);
> +    } else if (default_cpu_version) {
> +        x86_cpu_apply_version_props(cpu, def, default_cpu_version);
> +    }
>  }
>  
>  #ifndef CONFIG_USER_ONLY
>  /* Return a QDict containing keys for all properties that can be included
> - * in static expansion of CPU models. All properties set by x86_cpu_load_def()
> + * in static expansion of CPU models. All properties set by x86_cpu_load_model()
>   * must be included in the dictionary.
>   */
>  static QDict *x86_cpu_static_props(void)
> @@ -4176,23 +4255,44 @@ static gchar *x86_gdb_arch_name(CPUState *cs)
>  
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
> -    X86CPUDefinition *cpudef = data;
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
>  
> -    xcc->cpu_def = cpudef;
> +    xcc->model = data;
>      xcc->migration_safe = true;
>  }
>  
> -static void x86_register_cpudef_type(X86CPUDefinition *def)
> +static char *x86_cpu_model_type_name(X86CPUModel *model)
>  {
> -    char *typename = x86_cpu_type_name(def->name);
> +    if (model->version) {
> +        char *name = x86_cpu_versioned_model_name(model->cpudef,
> +                                                  model->version);
> +        char *r = x86_cpu_type_name(name);
> +        g_free(name);
> +        return r;
> +    } else {
> +        return x86_cpu_type_name(model->cpudef->name);
> +    }
> +}
> +
> +static void x86_register_cpu_model_type(X86CPUModel *model)
> +{
> +    char *typename = x86_cpu_model_type_name(model);
>      TypeInfo ti = {
>          .name = typename,
>          .parent = TYPE_X86_CPU,
>          .class_init = x86_cpu_cpudef_class_init,
> -        .class_data = def,
> +        .class_data = model,
>      };
>  
> +    type_register(&ti);
> +    g_free(typename);
> +}
> +
> +static void x86_register_cpudef_types(X86CPUDefinition *def)
> +{
> +    X86CPUModel *m;
> +    const X86CPUVersionDefinition *vdef;
> +
>      /* AMD aliases are handled at runtime based on CPUID vendor, so
>       * they shouldn't be set on the CPU model table.
>       */
> @@ -4200,9 +4300,20 @@ static void x86_register_cpudef_type(X86CPUDefinition *def)
>      /* catch mistakes instead of silently truncating model_id when too long */
>      assert(def->model_id && strlen(def->model_id) <= 48);
>  
> +    /* Unversioned model: */
> +    m = g_new0(X86CPUModel, 1);
> +    m->cpudef = def;
> +    x86_register_cpu_model_type(m);
> +
> +    /* Versioned models: */
> +
> +    for (vdef = x86_cpu_def_get_versions(def); vdef->name; vdef++) {
> +        X86CPUModel *m = g_new0(X86CPUModel, 1);
> +        m->cpudef = def;
> +        m->version = vdef->name;
> +        x86_register_cpu_model_type(m);
> +    }
>  
> -    type_register(&ti);
> -    g_free(typename);
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -4989,7 +5100,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
>   * involved in setting up CPUID data are:
>   *
>   * 1) Loading CPU model definition (X86CPUDefinition). This is
> - *    implemented by x86_cpu_load_def() and should be completely
> + *    implemented by x86_cpu_load_model() and should be completely
>   *    transparent, as it is done automatically by instance_init.
>   *    No code should need to look at X86CPUDefinition structs
>   *    outside instance_init.
> @@ -5306,7 +5417,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      /* Cache information initialization */
>      if (!cpu->legacy_cache) {
> -        if (!xcc->cpu_def || !xcc->cpu_def->cache_info) {
> +        if (!xcc->model || !xcc->model->cpudef->cache_info) {
>              char *name = x86_cpu_class_get_model_name(xcc);
>              error_setg(errp,
>                         "CPU model '%s' doesn't support legacy-cache=off", name);
> @@ -5314,7 +5425,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>              return;
>          }
>          env->cache_info_cpuid2 = env->cache_info_cpuid4 = env->cache_info_amd =
> -            *xcc->cpu_def->cache_info;
> +            *xcc->model->cpudef->cache_info;
>      } else {
>          /* Build legacy cache information */
>          env->cache_info_cpuid2.l1d_cache = &legacy_l1d_cache;
> @@ -5671,8 +5782,8 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_alias(obj, "sse4_1", obj, "sse4.1", &error_abort);
>      object_property_add_alias(obj, "sse4_2", obj, "sse4.2", &error_abort);
>  
> -    if (xcc->cpu_def) {
> -        x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
> +    if (xcc->model) {
> +        x86_cpu_load_model(cpu, xcc->model, &error_abort);
>      }
>  }
>  
> @@ -6009,7 +6120,7 @@ static void x86_cpu_register_types(void)
>  
>      type_register_static(&x86_cpu_type_info);
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> -        x86_register_cpudef_type(&builtin_x86_defs[i]);
> +        x86_register_cpudef_types(&builtin_x86_defs[i]);
>      }
>      type_register_static(&max_x86_cpu_type_info);
>      type_register_static(&x86_base_cpu_type_info);
> diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
> new file mode 100644
> index 0000000000..c0660a552f
> --- /dev/null
> +++ b/tests/acceptance/x86_cpu_model_versions.py
> @@ -0,0 +1,102 @@
> +#!/usr/bin/env python
> +#
> +# Basic validation of x86 versioned CPU models and CPU model aliases
> +#
> +#  Copyright (c) 2019 Red Hat Inc
> +#
> +# Author:
> +#  Eduardo Habkost <ehabkost@redhat.com>
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +
> +import avocado_qemu
> +
> +def get_cpu_prop(vm, prop):
> +    cpu_path = vm.command('query-cpus')[0].get('qom_path')
> +    return vm.command('qom-get', path=cpu_path, property=prop)
> +
> +class X86CPUModelAliases(avocado_qemu.Test):
> +    """
> +    Validation of PC CPU model versions and CPU model aliases
> +
> +    :avocado: tags=arch:x86_64
> +    """
> +    def test_4_0_alias(self):
> +        """Check if pc-*-4.0 unversioned CPU model won't be an alias"""
> +        # pc-*-4.0 won't expose non-versioned CPU models as aliases
> +        # We do this to help management software to keep compatibility
> +        # with older QEMU versions that didn't have the versioned CPU model
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('pc-i440fx-4.0')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server'],
> +                         'Cascadelake-Server must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertNotIn('alias-of', cpus['qemu64'],
> +                         'qemu64 must not be an alias')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> +
> +    def test_4_1_alias(self):
> +        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('pc-i440fx-4.1')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertEquals(cpus['Cascadelake-Server'].get('alias-of'), 'Cascadelake-Server-4.1',
> +                          'Cascadelake-Server must be an alias of Cascadelake-Server-4.1')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
> +                         'Cascadelake-Server-4.1 must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertEquals(cpus['qemu64'].get('alias-of'), 'qemu64-4.1',
> +                          'qemu64 must be an alias of qemu64-4.1')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> +
> +    def test_none_alias(self):
> +        """Check if unversioned CPU model is an alias pointing to 4.1 version"""
> +        self.vm.add_args('-S')
> +        self.vm.set_machine('none')
> +        self.vm.launch()
> +
> +        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))
> +
> +        self.assertFalse(cpus['Cascadelake-Server']['static'],
> +                         'unversioned Cascadelake-Server CPU model must not be static')
> +        self.assertTrue(cpus['Cascadelake-Server']['alias-of'].startswith('Cascadelake-Server-'),
> +                          'Cascadelake-Server must be an alias of versioned CPU model')
> +        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
> +                         'Cascadelake-Server-4.1 must not be an alias')
> +
> +        self.assertFalse(cpus['qemu64']['static'],
> +                         'unversioned qemu64 CPU model must not be static')
> +        self.assertTrue(cpus['qemu64']['alias-of'].startswith('qemu64-'),
> +                          'qemu64 must be an alias of versioned CPU model')
> +        self.assertNotIn('alias-of', cpus['qemu64-4.1'],
> +                         'qemu64-4.1 must not be an alias')
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 

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: [Qemu-devel] [PATCH 4/6] i386: Infrastructure for versioned CPU models

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 07:08:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:06AM -0300, Eduardo Habkost wrote:
> > Base code for versioned CPU models.  This will register a "-4.1"
> > version of all existing CPU models, and make the unversioned CPU
> > models be an alias for the -4.1 versions on the pc-*-4.1 machine
> > types.
> 
> Currently we have some CPUs that I'd consider historical "mistakes"
> due to fact that versioning didn't previously exist.
> 
> eg
> 
>    Haswell
>    Haswell-noTSX
>    Haswell-noTSX-IBRS
> 
> IIUC this patch adds
> 
>   Haswell            alias-of Haswell-4.1
>   Haswell-noTSX      alias-of Haswell-noTSX-4.1
>   Haswell-noTSX-IBRS alias-of Haswell-noTSX-IBRS-4.1
> 
> I'm thinking we should instead be merging all these haswell variants
> 
> 
>   Haswell            alias-of Haswell-4.1.1
>   Haswell-noTSX      alias-of Haswell-4.1.2
>   Haswell-noTSX-IBRS alias-of Haswell-4.1.3
> 
> Or if we used the simple counter versioning
> 
>   Haswell            alias-of Haswell-1
>   Haswell-noTSX      alias-of Haswell-2
>   Haswell-noTSX-IBRS alias-of Haswell-3
> 
> Likewise for the other named CPUs with wierd variants.
> 
> This would effectively be "deprecating" the noTSX and IBRS variants
> in favour of using the versioning approach

Sounds good.  I will do it.

-- 
Eduardo

[Qemu-devel] [PATCH 5/6] docs: Deprecate CPU model runnability guarantees

[Eduardo Habkost]

Document that CPU model runnability guarantees won't apply to
unversioned CPU models anymore.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: libvir-list@redhat.com
---
 qemu-deprecated.texi | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index df04f2840b..5fafb62a6d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -243,3 +243,22 @@ Note that if you are exposing the export via /dev/nbd0, it is easier
 to just export the entire image and then mount only /dev/nbd0p1 than
 it is to reinvoke @command{qemu-nbd -c /dev/nbd0} limited to just a
 subset of the image.
+
+@section Backwards compatibility
+
+@subsection Runnability guarantee of CPU models (since 4.1.0)
+
+Previous versions of QEMU never changed existing CPU models in
+ways that introduced additional host software or hardware
+requirements to the VM.  This allowed management software to
+safely change the machine type of an existing VM without
+introducing new requirements ("runnability guarantee").  This
+prevented CPU models from being updated to include CPU
+vulnerability mitigations, leaving guests vulnerable in the
+default configuration.
+
+The CPU model runnability guarantee won't apply anymore to
+existing CPU models.  Management software that needs runnability
+guarantees must resolve the CPU model aliases using te
+``alias-of'' field returned by the ``query-cpu-definitions'' QMP
+command.
-- 
2.18.0.rc1.1.g3f1ff2140

[Qemu-devel] [PATCH 6/6] i386: Add Cascadelake-Server-4.1.1 CPU model

[Eduardo Habkost]

Add new version of Cascadelake-Server CPU model, setting
stepping=5 and enabling the IA32_ARCH_CAPABILITIES MSR.

The new feature will introduce a new host software requirement,
breaking our CPU model runnability promises.  This means we can't
enable the new CPU model version by default in QEMU 4.1, because
management software isn't ready yet to resolve CPU model aliases.
This is why the feature is being enabled in a
Cascadelake-Server-4.1.1 CPU model instead of
Cascadelake-Server-4.1.

Includes a test case to ensure the right combinations of
machine-type + CPU model + command-line feature flags will work
as expected.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Cc: "Hu, Robert" <robert.hu@intel.com>
Cc: Tao Xu <tao3.xu@intel.com>
Cc: jingqi.liu@intel.com,
Cc: "Lai, Paul C" <paul.c.lai@intel.com>
---
 target/i386/cpu.c                          | 15 +++++
 tests/acceptance/x86_cpu_model_versions.py | 71 ++++++++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 121f568954..8edae04161 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2607,6 +2607,21 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_6_EAX_ARAT,
         .xlevel = 0x80000008,
         .model_id = "Intel Xeon Processor (Cascadelake)",
+        .versions = (X86CPUVersionDefinition[]) {
+            /*
+             * 4.1 won't have arch-capabilities enabled yet, to not break
+             * older management software
+             */
+            { .name = "4.1" },
+            { .name = "4.1.1",
+              .props = (PropValue[]) {
+                  { "stepping", "5" },
+                  { "arch-capabilities", "on" },
+                  { /* end of list */ },
+              },
+            },
+            { /* end of list */ },
+        }
     },
     {
         .name = "Icelake-Client",
diff --git a/tests/acceptance/x86_cpu_model_versions.py b/tests/acceptance/x86_cpu_model_versions.py
index c0660a552f..127239e2a1 100644
--- a/tests/acceptance/x86_cpu_model_versions.py
+++ b/tests/acceptance/x86_cpu_model_versions.py
@@ -49,6 +49,8 @@ class X86CPUModelAliases(avocado_qemu.Test):
                          'unversioned Cascadelake-Server CPU model must not be static')
         self.assertNotIn('alias-of', cpus['Cascadelake-Server'],
                          'Cascadelake-Server must not be an alias')
+        self.assertNotIn('alias-of', cpus['Cascadelake-Server-4.1'],
+                         'Cascadelake-Server-4.1 must not be an alias')
 
         self.assertFalse(cpus['qemu64']['static'],
                          'unversioned qemu64 CPU model must not be static')
@@ -100,3 +102,72 @@ class X86CPUModelAliases(avocado_qemu.Test):
                           'qemu64 must be an alias of versioned CPU model')
         self.assertNotIn('alias-of', cpus['qemu64-4.1'],
                          'qemu64-4.1 must not be an alias')
+
+    def test_Cascadelake_arch_capabilities_result(self):
+        # machine-type only:
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.1')
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')
+        vm.launch()
+        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.1 + Cascadelake-Server should not have arch-capabilities')
+
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.0')
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')
+        vm.launch()
+        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.0 + Cascadelake-Server should not have arch-capabilities')
+
+        # command line must override machine-type if CPU model is not versioned:
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.0')
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,+arch-capabilities')
+        vm.launch()
+        self.assertTrue(get_cpu_prop(vm, 'arch-capabilities'),
+                        'pc-i440fx-4.0 + Cascadelake-Server,+arch-capabilities should have arch-capabilities')
+
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.1')
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,-arch-capabilities')
+        vm.launch()
+        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.1 + Cascadelake-Server,-arch-capabilities should not have arch-capabilities')
+
+        # versioned CPU model overrides machine-type:
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.0')
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1,x-force-features=on,check=off,enforce=off')
+        vm.launch()
+        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should not have arch-capabilities')
+
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.0')
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off')
+        vm.launch()
+        self.assertTrue(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should have arch-capabilities')
+
+        # command line must override machine-type and versioned CPU model:
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.0')
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,+arch-capabilities')
+        vm.launch()
+        self.assertTrue(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.0 + Cascadelake-Server-4.1,+arch-capabilities should have arch-capabilities')
+
+        vm = self.get_vm()
+        vm.add_args('-S')
+        vm.set_machine('pc-i440fx-4.1')
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off,-arch-capabilities')
+        vm.launch()
+        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1.1,-arch-capabilities should not have arch-capabilities')
-- 
2.18.0.rc1.1.g3f1ff2140

Re: [Qemu-devel] [PATCH 6/6] i386: Add Cascadelake-Server-4.1.1 CPU model

[Tao Xu]

On 6/25/2019 1:00 PM, Eduardo Habkost wrote:
> Add new version of Cascadelake-Server CPU model, setting
> stepping=5 and enabling the IA32_ARCH_CAPABILITIES MSR.
> 
> The new feature will introduce a new host software requirement,
> breaking our CPU model runnability promises.  This means we can't
> enable the new CPU model version by default in QEMU 4.1, because
> management software isn't ready yet to resolve CPU model aliases.
> This is why the feature is being enabled in a
> Cascadelake-Server-4.1.1 CPU model instead of
> Cascadelake-Server-4.1.
> 
> Includes a test case to ensure the right combinations of
> machine-type + CPU model + command-line feature flags will work
> as expected.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Cc: "Hu, Robert" <robert.hu@intel.com>
> Cc: Tao Xu <tao3.xu@intel.com>
> Cc: jingqi.liu@intel.com,
> Cc: "Lai, Paul C" <paul.c.lai@intel.com>
> ---
>   target/i386/cpu.c                          | 15 +++++
>   tests/acceptance/x86_cpu_model_versions.py | 71 ++++++++++++++++++++++
>   2 files changed, 86 insertions(+)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 121f568954..8edae04161 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2607,6 +2607,21 @@ static X86CPUDefinition builtin_x86_defs[] = {
...
> +        # versioned CPU model overrides machine-type:
> +        vm = self.get_vm()
> +        vm.add_args('-S')
> +        vm.set_machine('pc-i440fx-4.0')
> +        vm.add_args('-cpu', 'Cascadelake-Server-4.1,x-force-features=on,check=off,enforce=off')
> +        vm.launch()
> +        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
> +                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should not have arch-capabilities')
> +
> +        vm = self.get_vm()
> +        vm.add_args('-S')
> +        vm.set_machine('pc-i440fx-4.0')
> +        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off')
> +        vm.launch()
> +        self.assertTrue(get_cpu_prop(vm, 'arch-capabilities'),
> +                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should have arch-capabilities')
Hi Eduardo

Could me ask why here the error show "Cascadelake-Server-4.1" instead of 
"Cascadelake-Server-4.1.1"?

And will you add the MSR_ARCH_CAP_RDCL_NO, MSR_ARCH_CAP_IBRS_ALL, and 
MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY into .features[FEAT_ARCH_CAPABILITIES]?

Tao

Re: [Qemu-devel] [PATCH 6/6] i386: Add Cascadelake-Server-4.1.1 CPU model

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 01:35:17PM +0800, Tao Xu wrote:
> On 6/25/2019 1:00 PM, Eduardo Habkost wrote:
> > Add new version of Cascadelake-Server CPU model, setting
> > stepping=5 and enabling the IA32_ARCH_CAPABILITIES MSR.
> > 
> > The new feature will introduce a new host software requirement,
> > breaking our CPU model runnability promises.  This means we can't
> > enable the new CPU model version by default in QEMU 4.1, because
> > management software isn't ready yet to resolve CPU model aliases.
> > This is why the feature is being enabled in a
> > Cascadelake-Server-4.1.1 CPU model instead of
> > Cascadelake-Server-4.1.
> > 
> > Includes a test case to ensure the right combinations of
> > machine-type + CPU model + command-line feature flags will work
> > as expected.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Cc: "Hu, Robert" <robert.hu@intel.com>
> > Cc: Tao Xu <tao3.xu@intel.com>
> > Cc: jingqi.liu@intel.com,
> > Cc: "Lai, Paul C" <paul.c.lai@intel.com>
> > ---
> >   target/i386/cpu.c                          | 15 +++++
> >   tests/acceptance/x86_cpu_model_versions.py | 71 ++++++++++++++++++++++
> >   2 files changed, 86 insertions(+)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 121f568954..8edae04161 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2607,6 +2607,21 @@ static X86CPUDefinition builtin_x86_defs[] = {
> ...
> > +        # versioned CPU model overrides machine-type:
> > +        vm = self.get_vm()
> > +        vm.add_args('-S')
> > +        vm.set_machine('pc-i440fx-4.0')
> > +        vm.add_args('-cpu', 'Cascadelake-Server-4.1,x-force-features=on,check=off,enforce=off')
> > +        vm.launch()
> > +        self.assertFalse(get_cpu_prop(vm, 'arch-capabilities'),
> > +                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should not have arch-capabilities')
> > +
> > +        vm = self.get_vm()
> > +        vm.add_args('-S')
> > +        vm.set_machine('pc-i440fx-4.0')
> > +        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off')
> > +        vm.launch()
> > +        self.assertTrue(get_cpu_prop(vm, 'arch-capabilities'),
> > +                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should have arch-capabilities')
> Hi Eduardo
> 
> Could me ask why here the error show "Cascadelake-Server-4.1" instead of
> "Cascadelake-Server-4.1.1"?

My mistake.  Will fix it.

> 
> And will you add the MSR_ARCH_CAP_RDCL_NO, MSR_ARCH_CAP_IBRS_ALL, and
> MSR_ARCH_CAP_SKIP_L1DFL_VMENTRY into .features[FEAT_ARCH_CAPABILITIES]?

I will do it.  I don't know why I didn't notice them in the "Add
some MSR based features on Cascadelake-Server CPU model" patches
you sent.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[no-reply]

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



Hi,

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

Message-id: 20190625050008.12789-1-ehabkost@redhat.com
Type: series
Subject: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

=== 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 ===

From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190625050008.12789-1-ehabkost@redhat.com -> patchew/20190625050008.12789-1-ehabkost@redhat.com
Switched to a new branch 'test'
fa60fbbfe4 i386: Add Cascadelake-Server-4.1.1 CPU model
41aac3f41c docs: Deprecate CPU model runnability guarantees
7702d3e8ca i386: Infrastructure for versioned CPU models
3fe33acf1b qmp: Add "alias-of" field to query-cpu-definitions
582f02c5f5 i386: Remove unused host_cpudef variable
be6cf76099 i386: Add x-force-features option for testing

=== OUTPUT BEGIN ===
1/6 Checking commit be6cf760996c (i386: Add x-force-features option for testing)
2/6 Checking commit 582f02c5f580 (i386: Remove unused host_cpudef variable)
3/6 Checking commit 3fe33acf1b30 (qmp: Add "alias-of" field to query-cpu-definitions)
4/6 Checking commit 7702d3e8ca63 (i386: Infrastructure for versioned CPU models)
WARNING: Block comments use a leading /* on a separate line
#212: FILE: target/i386/cpu.c:3075:
+    { /* end of list */ },

WARNING: line over 80 characters
#261: FILE: target/i386/cpu.c:3987:
+static const X86CPUVersionDefinition *x86_cpu_def_get_versions(X86CPUDefinition *def)

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#457: 
new file mode 100644

WARNING: line over 80 characters
#507: FILE: tests/acceptance/x86_cpu_model_versions.py:46:
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))

WARNING: line over 80 characters
#510: FILE: tests/acceptance/x86_cpu_model_versions.py:49:
+                         'unversioned Cascadelake-Server CPU model must not be static')

WARNING: line over 80 characters
#527: FILE: tests/acceptance/x86_cpu_model_versions.py:66:
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))

WARNING: line over 80 characters
#530: FILE: tests/acceptance/x86_cpu_model_versions.py:69:
+                         'unversioned Cascadelake-Server CPU model must not be static')

ERROR: line over 90 characters
#531: FILE: tests/acceptance/x86_cpu_model_versions.py:70:
+        self.assertEquals(cpus['Cascadelake-Server'].get('alias-of'), 'Cascadelake-Server-4.1',

WARNING: line over 80 characters
#532: FILE: tests/acceptance/x86_cpu_model_versions.py:71:
+                          'Cascadelake-Server must be an alias of Cascadelake-Server-4.1')

WARNING: line over 80 characters
#549: FILE: tests/acceptance/x86_cpu_model_versions.py:88:
+        cpus = dict((m['name'], m) for m in self.vm.command('query-cpu-definitions'))

WARNING: line over 80 characters
#552: FILE: tests/acceptance/x86_cpu_model_versions.py:91:
+                         'unversioned Cascadelake-Server CPU model must not be static')

ERROR: line over 90 characters
#553: FILE: tests/acceptance/x86_cpu_model_versions.py:92:
+        self.assertTrue(cpus['Cascadelake-Server']['alias-of'].startswith('Cascadelake-Server-'),

WARNING: line over 80 characters
#554: FILE: tests/acceptance/x86_cpu_model_versions.py:93:
+                          'Cascadelake-Server must be an alias of versioned CPU model')

total: 2 errors, 11 warnings, 477 lines checked

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

5/6 Checking commit 41aac3f41c03 (docs: Deprecate CPU model runnability guarantees)
6/6 Checking commit fa60fbbfe411 (i386: Add Cascadelake-Server-4.1.1 CPU model)
WARNING: Block comments use a leading /* on a separate line
#43: FILE: target/i386/cpu.c:2620:
+                  { /* end of list */ },

WARNING: Block comments use a leading /* on a separate line
#46: FILE: target/i386/cpu.c:2623:
+            { /* end of list */ },

ERROR: line over 90 characters
#74: FILE: tests/acceptance/x86_cpu_model_versions.py:111:
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')

ERROR: line over 90 characters
#77: FILE: tests/acceptance/x86_cpu_model_versions.py:114:
+                         'pc-i440fx-4.1 + Cascadelake-Server should not have arch-capabilities')

ERROR: line over 90 characters
#82: FILE: tests/acceptance/x86_cpu_model_versions.py:119:
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off')

ERROR: line over 90 characters
#85: FILE: tests/acceptance/x86_cpu_model_versions.py:122:
+                         'pc-i440fx-4.0 + Cascadelake-Server should not have arch-capabilities')

ERROR: line over 90 characters
#91: FILE: tests/acceptance/x86_cpu_model_versions.py:128:
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,+arch-capabilities')

ERROR: line over 90 characters
#94: FILE: tests/acceptance/x86_cpu_model_versions.py:131:
+                        'pc-i440fx-4.0 + Cascadelake-Server,+arch-capabilities should have arch-capabilities')

ERROR: line over 90 characters
#99: FILE: tests/acceptance/x86_cpu_model_versions.py:136:
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,-arch-capabilities')

ERROR: line over 90 characters
#102: FILE: tests/acceptance/x86_cpu_model_versions.py:139:
+                         'pc-i440fx-4.1 + Cascadelake-Server,-arch-capabilities should not have arch-capabilities')

ERROR: line over 90 characters
#108: FILE: tests/acceptance/x86_cpu_model_versions.py:145:
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1,x-force-features=on,check=off,enforce=off')

ERROR: line over 90 characters
#111: FILE: tests/acceptance/x86_cpu_model_versions.py:148:
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should not have arch-capabilities')

ERROR: line over 90 characters
#116: FILE: tests/acceptance/x86_cpu_model_versions.py:153:
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off')

ERROR: line over 90 characters
#119: FILE: tests/acceptance/x86_cpu_model_versions.py:156:
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1 should have arch-capabilities')

ERROR: line over 90 characters
#125: FILE: tests/acceptance/x86_cpu_model_versions.py:162:
+        vm.add_args('-cpu', 'Cascadelake-Server,x-force-features=on,check=off,enforce=off,+arch-capabilities')

ERROR: line over 90 characters
#128: FILE: tests/acceptance/x86_cpu_model_versions.py:165:
+                         'pc-i440fx-4.0 + Cascadelake-Server-4.1,+arch-capabilities should have arch-capabilities')

ERROR: line over 90 characters
#133: FILE: tests/acceptance/x86_cpu_model_versions.py:170:
+        vm.add_args('-cpu', 'Cascadelake-Server-4.1.1,x-force-features=on,check=off,enforce=off,-arch-capabilities')

ERROR: line over 90 characters
#136: FILE: tests/acceptance/x86_cpu_model_versions.py:173:
+                         'pc-i440fx-4.1 + Cascadelake-Server-4.1.1,-arch-capabilities should not have arch-capabilities')

total: 16 errors, 2 warnings, 101 lines checked

Patch 6/6 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/20190625050008.12789-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

Re: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Daniel P. Berrangé]

On Tue, Jun 25, 2019 at 02:00:02AM -0300, Eduardo Habkost wrote:
> This series implements basic infrastructure for CPU model
> versioning, as discussed before[1][2][3].  This will finally
> allow us to update CPU models in ways that introduce new software
> or hardware requirements.
> 
> My original plan was to use "query-cpu-model-expansion
> mode=static" to resolve aliases, but I dropped that plan because
> it would increase complexity for management software a lot.
> static CPU models are documented as not being affected by the
> machine type and accelerator at all, which would make the
> versioned CPU models very inconvenient to use in the command
> line.  e.g.: users would be forced to replace:
> 
>   -cpu Haswell
> 
> with:
> 
>   -cpu Haswell-4.1,+2apic,+monitor,+kvmclock,+kvm-nopiodelay,+kvm-asyncpf,+kvm-steal-time,+kvm-pv-eoi,+kvmclock-stable-bit,+x2apic,-acpi,-monitor,-svm
> 
> In the end, making the versioned CPU models static is not a
> requirement at all: what we really need is to drop the
> runnability guarantees from unversioned CPU model names, and
> require management software to resolve the unversioned alias
> before saving the VM configuration.
> 
> Guest ABI compatibility and live migration guarantees are going
> to be kept: unversioned CPU models will still be usable with live
> migration.  Only runnability guarantees when updating the machine
> type will be dropped.  This means unversioned CPU models are
> still reported as migration-safe in query-cpu-definitions.

I'm having a little bit of a hard time parsing the overall behaviour
from the above description. Let me outline the examples so you can
confirm if I've got it right.

Lets assume there is a VM configured using "Haswell"

Today a mgmt app would pass the CPU model name in to QEMU as is,
and thus we get

  $QEMU -machine pc-i440fx-4.0 -cpu Haswell ...more args...

The semantics of "Haswell" here is going to vary according to
what the machine type pc-i440fx-4.0 customizes wrt Haswell.

If the config later has the machine type changed to pc-i440fx-4.1
the semantics of Haswell might change in an arbitrary way. It
might even become unrunnable on the current hardware.

With the new versioned machine types, the VM config of "Haswell"
would be resolved into some arbitrary versioned machine type
"Haswell-NNN" and thus the mgmt app would launch

  $QEMU -machine pc-i440fx-4.0 -cpu Haswell-NNN ...more args...

The semantics of "Haswell-NNN" will never change no matter what
the machine type does.

The end user has the option to explicitly give "Haswell-NNN" to
the mgmt app if they know they want that particular variant, and
in this case the mgmt app won't need to resolve anything (or at
least the process of trying to resolve it won't change it).


> 
> The last patch in the series demonstrates how the new feature can
> be used to update a CPU model: it adds a Cascadelake-Server-4.1.1
> CPU model, including "arch-capabilities=on" and "stepping=5".
> Unfortunately we can't enable arch-capabilities in the -4.1
> version of Cascadelake-Server because it would break our existing
> runnability guarantees.
> 
> [1] https://www.mail-archive.com/libvir-list@redhat.com/msg167342.html
> [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg590034.html
> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg611244.html
> 
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Pavel Hrdina <phrdina@redhat.com>
> Cc: Jiri Denemark <jdenemar@redhat.com>
> Cc: "Hu, Robert" <robert.hu@intel.com>
> Cc: Tao Xu <tao3.xu@intel.com>
> Cc: Richard Henderson <rth@twiddle.net>
> 
> Eduardo Habkost (6):
>   i386: Add x-force-features option for testing
>   i386: Remove unused host_cpudef variable
>   qmp: Add "alias-of" field to query-cpu-definitions
>   i386: Infrastructure for versioned CPU models
>   docs: Deprecate CPU model runnability guarantees
>   i386: Add Cascadelake-Server-4.1.1 CPU model
> 
>  qapi/target.json                           |   9 +-
>  include/hw/i386/pc.h                       |   3 +
>  target/i386/cpu-qom.h                      |  10 +-
>  target/i386/cpu.h                          |  16 ++
>  hw/i386/pc.c                               |   3 +
>  hw/i386/pc_piix.c                          |   4 +
>  hw/i386/pc_q35.c                           |   4 +
>  target/i386/cpu.c                          | 188 +++++++++++++++++----
>  qemu-deprecated.texi                       |  19 +++
>  tests/acceptance/x86_cpu_model_versions.py | 173 +++++++++++++++++++
>  10 files changed, 388 insertions(+), 41 deletions(-)
>  create mode 100644 tests/acceptance/x86_cpu_model_versions.py
> 
> -- 
> 2.18.0.rc1.1.g3f1ff2140
> 

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: [Qemu-devel] [PATCH 0/6] x86 CPU model versioning

[Eduardo Habkost]

On Tue, Jun 25, 2019 at 05:13:57PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 25, 2019 at 02:00:02AM -0300, Eduardo Habkost wrote:
> > This series implements basic infrastructure for CPU model
> > versioning, as discussed before[1][2][3].  This will finally
> > allow us to update CPU models in ways that introduce new software
> > or hardware requirements.
> > 
> > My original plan was to use "query-cpu-model-expansion
> > mode=static" to resolve aliases, but I dropped that plan because
> > it would increase complexity for management software a lot.
> > static CPU models are documented as not being affected by the
> > machine type and accelerator at all, which would make the
> > versioned CPU models very inconvenient to use in the command
> > line.  e.g.: users would be forced to replace:
> > 
> >   -cpu Haswell
> > 
> > with:
> > 
> >   -cpu Haswell-4.1,+2apic,+monitor,+kvmclock,+kvm-nopiodelay,+kvm-asyncpf,+kvm-steal-time,+kvm-pv-eoi,+kvmclock-stable-bit,+x2apic,-acpi,-monitor,-svm
> > 
> > In the end, making the versioned CPU models static is not a
> > requirement at all: what we really need is to drop the
> > runnability guarantees from unversioned CPU model names, and
> > require management software to resolve the unversioned alias
> > before saving the VM configuration.
> > 
> > Guest ABI compatibility and live migration guarantees are going
> > to be kept: unversioned CPU models will still be usable with live
> > migration.  Only runnability guarantees when updating the machine
> > type will be dropped.  This means unversioned CPU models are
> > still reported as migration-safe in query-cpu-definitions.
> 
> I'm having a little bit of a hard time parsing the overall behaviour
> from the above description. Let me outline the examples so you can
> confirm if I've got it right.
> 
> Lets assume there is a VM configured using "Haswell"
> 
> Today a mgmt app would pass the CPU model name in to QEMU as is,
> and thus we get
> 
>   $QEMU -machine pc-i440fx-4.0 -cpu Haswell ...more args...
> 
> The semantics of "Haswell" here is going to vary according to
> what the machine type pc-i440fx-4.0 customizes wrt Haswell.
> 
> If the config later has the machine type changed to pc-i440fx-4.1
> the semantics of Haswell might change in an arbitrary way. It
> might even become unrunnable on the current hardware.

If we follow our deprecation policy, this shouldn't happen until
pc-*-4.3.  On pc-*-4.1 and pc-*-4.2, I expect "-cpu Haswell" to
stay runnable if it's already runnable with pc-*-4.0.  This will
give libvirt and other management software some time to adapt.

CPU model changes that don't affect runnability, though, are
already allowed and will still be allowed.


> 
> With the new versioned machine types, the VM config of "Haswell"
> would be resolved into some arbitrary versioned machine type
> "Haswell-NNN" and thus the mgmt app would launch
> 
>   $QEMU -machine pc-i440fx-4.0 -cpu Haswell-NNN ...more args...
> 
> The semantics of "Haswell-NNN" will never change no matter what
> the machine type does.

Exactly.

> 
> The end user has the option to explicitly give "Haswell-NNN" to
> the mgmt app if they know they want that particular variant, and
> in this case the mgmt app won't need to resolve anything (or at
> least the process of trying to resolve it won't change it).

Yes.  Management software should see that "Haswell-NNN" has no
"alias-of" field, so it doesn't need to be changed.

-- 
Eduardo