| Message ID | 20190625050008.12789-5-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86 CPU model versioning | expand |
* 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
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 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
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 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
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.
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
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.
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
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.
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')
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