| Message ID | 1591843676-102054-1-git-send-email-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] Introduce (x86) CPU model deprecation API | expand |
Hi, Ping for comments:) On Thu, 2020-06-11 at 10:47 +0800, Robert Hoo wrote: > Complement versioned CPU model framework with the ability of marking > some > versions deprecated. When that CPU model is chosen, get some warning. > The > warning message is customized, e.g. telling in which future QEMU > version will > it be obsoleted. > The deprecation message will also appear by x86_cpu_list_entry(), > e.g. '-cpu > help'. > QMP 'query-cpu-definitions' will also return a bool value indicating > the > deprecation status. > > Changes in v2: > Move deprecation check from parse_cpu_option() to > machine_run_board_init(), so > that it can cover implicit cpu_type assignment cases. > Add qapi new member documentation. Thanks Eric for comment and > guidance on qapi. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > hw/core/machine.c | 11 +++++++++-- > include/hw/core/cpu.h | 1 + > qapi/machine-target.json | 7 ++++++- > target/i386/cpu.c | 45 > +++++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 59 insertions(+), 5 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index bb3a7b1..9318964 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1083,6 +1083,8 @@ MemoryRegion > *machine_consume_memdev(MachineState *machine, > void machine_run_board_init(MachineState *machine) > { > MachineClass *machine_class = MACHINE_GET_CLASS(machine); > + ObjectClass *oc = object_class_by_name(machine->cpu_type); > + CPUClass *cc; > > if (machine->ram_memdev_id) { > Object *o; > @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState > *machine) > * specified a CPU with -cpu check here that the user CPU is > supported. > */ > if (machine_class->valid_cpu_types && machine->cpu_type) { > - ObjectClass *class = object_class_by_name(machine- > >cpu_type); > int i; > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > - if (object_class_dynamic_cast(class, > + if (object_class_dynamic_cast(oc, > machine_class- > >valid_cpu_types[i])) { > /* The user specificed CPU is in the valid field, we > are > * good to go. > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState > *machine) > } > } > > + /* Check if CPU type is deprecated and warn if so */ > + cc = CPU_CLASS(oc); > + if (cc->deprecation_check) { > + cc->deprecation_check(oc); > + } > + > machine_class->init(machine); > } > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 497600c..1ca47dc 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -218,6 +218,7 @@ typedef struct CPUClass { > void (*disas_set_info)(CPUState *cpu, disassemble_info *info); > vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, > int len); > void (*tcg_initialize)(void); > + void (*deprecation_check)(ObjectClass *oc); > > /* Keep non-pointer data at the end to minimize holes. */ > int gdb_num_core_regs; > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index f2c8294..c24f506 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -285,6 +285,10 @@ > # in the VM configuration, because aliases may stop being > # migration-safe in the future (since 4.1) > # > +# @deprecated: If true, this CPU model is deprecated and may be > removed in > +# in some future version of QEMU according to the QEMU > deprecation > +# policy. (since 5.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 > @@ -309,7 +313,8 @@ > 'static': 'bool', > '*unavailable-features': [ 'str' ], > 'typename': 'str', > - '*alias-of' : 'str' }, > + '*alias-of' : 'str', > + 'deprecated' : 'bool' }, > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || > defined(TARGET_I386) || defined(TARGET_S390X) || > defined(TARGET_MIPS)' } > > ## > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index ba05da3..0d8638a 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { > const char *alias; > const char *note; > PropValue *props; > + bool deprecated; > } X86CPUVersionDefinition; > > /* Base definition for a CPU model */ > @@ -1638,6 +1639,11 @@ struct X86CPUModel { > * This matters only for "-cpu help" and query-cpu-definitions > */ > bool is_alias; > + /* > + * If true, this model is deprecated, and may be removed in the > future. > + * Trying to use it now will cause a warning. > + */ > + bool deprecated; > }; > > /* Get full model name for CPU version */ > @@ -4128,8 +4134,7 @@ static X86CPUVersion > x86_cpu_model_resolve_version(const X86CPUModel *model) > X86CPUVersion v = model->version; > if (v == CPU_VERSION_AUTO) { > v = default_cpu_version; > - } > - if (v == CPU_VERSION_LATEST) { > + } else if (v == CPU_VERSION_LATEST) { > return x86_cpu_model_last_version(model); > } > return v; > @@ -4975,6 +4980,7 @@ 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; > + info->deprecated = cc->model ? cc->model->deprecated : false; > /* > * Old machine types won't report aliases, so that alias > translation > * doesn't break compatibility with previous QEMU versions. > @@ -5411,6 +5417,7 @@ static void > x86_register_cpudef_types(X86CPUDefinition *def) > m->cpudef = def; > m->version = vdef->version; > m->note = vdef->note; > + m->deprecated = vdef->deprecated; > x86_register_cpu_model_type(name, m); > > if (vdef->alias) { > @@ -5418,6 +5425,8 @@ static void > x86_register_cpudef_types(X86CPUDefinition *def) > am->cpudef = def; > am->version = vdef->version; > am->is_alias = true; > + am->note = vdef->note; > + am->deprecated = vdef->deprecated; > x86_register_cpu_model_type(vdef->alias, am); > } > } > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { > DEFINE_PROP_END_OF_LIST() > }; > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > +{ > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > + X86CPUVersion effective_version; > + const X86CPUVersionDefinition *vdef; > + > + if (xcc->model == NULL) { > + return; > + } > + > + if (xcc->model->version == CPU_VERSION_LEGACY) { > + /* Treat legacy version as v1 */ > + effective_version = 1; > + } else { > + effective_version = x86_cpu_model_resolve_version(xcc- > >model); > + } > + > + vdef = xcc->model->cpudef->versions; > + > + if (vdef == NULL) { > + return; > + } else { > + if (vdef[effective_version - 1].deprecated) { > + warn_report("Effective CPU model '%s' -- %s", > + x86_cpu_versioned_model_name(xcc->model- > >cpudef,\ > + effective_version), > + vdef[effective_version - 1].note); > + } > + } > +} > + > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > { > X86CPUClass *xcc = X86_CPU_CLASS(oc); > @@ -7291,6 +7331,7 @@ static void > x86_cpu_common_class_init(ObjectClass *oc, void *data) > cc->tlb_fill = x86_cpu_tlb_fill; > #endif > cc->disas_set_info = x86_disas_set_info; > + cc->deprecation_check = x86_cpu_deprecation_check; > > dc->user_creatable = true; > }
Hi, Thanks for the patch, and sorry for taking so long to review this. I'm finally getting to the patches that were postponed to 5.2. Comments and questions below: On Thu, Jun 11, 2020 at 10:47:55AM +0800, Robert Hoo wrote: > Complement versioned CPU model framework with the ability of marking some > versions deprecated. When that CPU model is chosen, get some warning. The > warning message is customized, e.g. telling in which future QEMU version will > it be obsoleted. > The deprecation message will also appear by x86_cpu_list_entry(), e.g. '-cpu > help'. > QMP 'query-cpu-definitions' will also return a bool value indicating the > deprecation status. > > Changes in v2: > Move deprecation check from parse_cpu_option() to machine_run_board_init(), so > that it can cover implicit cpu_type assignment cases. > Add qapi new member documentation. Thanks Eric for comment and guidance on qapi. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > hw/core/machine.c | 11 +++++++++-- > include/hw/core/cpu.h | 1 + > qapi/machine-target.json | 7 ++++++- > target/i386/cpu.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- > 4 files changed, 59 insertions(+), 5 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index bb3a7b1..9318964 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -1083,6 +1083,8 @@ MemoryRegion *machine_consume_memdev(MachineState *machine, > void machine_run_board_init(MachineState *machine) > { > MachineClass *machine_class = MACHINE_GET_CLASS(machine); > + ObjectClass *oc = object_class_by_name(machine->cpu_type); > + CPUClass *cc; > > if (machine->ram_memdev_id) { > Object *o; > @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState *machine) > * specified a CPU with -cpu check here that the user CPU is supported. > */ > if (machine_class->valid_cpu_types && machine->cpu_type) { > - ObjectClass *class = object_class_by_name(machine->cpu_type); > int i; > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > - if (object_class_dynamic_cast(class, > + if (object_class_dynamic_cast(oc, > machine_class->valid_cpu_types[i])) { > /* The user specificed CPU is in the valid field, we are > * good to go. > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState *machine) > } > } > > + /* Check if CPU type is deprecated and warn if so */ > + cc = CPU_CLASS(oc); > + if (cc->deprecation_check) { > + cc->deprecation_check(oc); > + } Why do we need target-specific code here? A CPUClass::deprecated field would be much simpler. > + > machine_class->init(machine); > } > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 497600c..1ca47dc 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -218,6 +218,7 @@ typedef struct CPUClass { > void (*disas_set_info)(CPUState *cpu, disassemble_info *info); > vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len); > void (*tcg_initialize)(void); > + void (*deprecation_check)(ObjectClass *oc); > > /* Keep non-pointer data at the end to minimize holes. */ > int gdb_num_core_regs; > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index f2c8294..c24f506 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -285,6 +285,10 @@ > # in the VM configuration, because aliases may stop being > # migration-safe in the future (since 4.1) > # > +# @deprecated: If true, this CPU model is deprecated and may be removed in > +# in some future version of QEMU according to the QEMU deprecation > +# policy. (since 5.1) Next version needs to say "since 5.2". > +# > # @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 > @@ -309,7 +313,8 @@ > 'static': 'bool', > '*unavailable-features': [ 'str' ], > 'typename': 'str', > - '*alias-of' : 'str' }, > + '*alias-of' : 'str', > + 'deprecated' : 'bool' }, > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' } > > ## > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index ba05da3..0d8638a 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { > const char *alias; > const char *note; > PropValue *props; > + bool deprecated; > } X86CPUVersionDefinition; > > /* Base definition for a CPU model */ > @@ -1638,6 +1639,11 @@ struct X86CPUModel { > * This matters only for "-cpu help" and query-cpu-definitions > */ > bool is_alias; > + /* > + * If true, this model is deprecated, and may be removed in the future. > + * Trying to use it now will cause a warning. > + */ > + bool deprecated; > }; > > /* Get full model name for CPU version */ > @@ -4128,8 +4134,7 @@ static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model) > X86CPUVersion v = model->version; > if (v == CPU_VERSION_AUTO) { > v = default_cpu_version; > - } > - if (v == CPU_VERSION_LATEST) { > + } else if (v == CPU_VERSION_LATEST) { Why is this change necessary? > return x86_cpu_model_last_version(model); > } > return v; > @@ -4975,6 +4980,7 @@ 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; > + info->deprecated = cc->model ? cc->model->deprecated : false; > /* > * Old machine types won't report aliases, so that alias translation > * doesn't break compatibility with previous QEMU versions. > @@ -5411,6 +5417,7 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) > m->cpudef = def; > m->version = vdef->version; > m->note = vdef->note; > + m->deprecated = vdef->deprecated; > x86_register_cpu_model_type(name, m); > > if (vdef->alias) { > @@ -5418,6 +5425,8 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) > am->cpudef = def; > am->version = vdef->version; > am->is_alias = true; > + am->note = vdef->note; Is this extra line related to the deprecation feature? It doesn't seem related, and it doesn't seem necessary as the `note` field is already ignored for alias CPU models. > + am->deprecated = vdef->deprecated; > x86_register_cpu_model_type(vdef->alias, am); > } > } > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { > DEFINE_PROP_END_OF_LIST() > }; > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > +{ > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > + X86CPUVersion effective_version; > + const X86CPUVersionDefinition *vdef; > + > + if (xcc->model == NULL) { > + return; > + } > + > + if (xcc->model->version == CPU_VERSION_LEGACY) { > + /* Treat legacy version as v1 */ > + effective_version = 1; > + } else { > + effective_version = x86_cpu_model_resolve_version(xcc->model); > + } > + > + vdef = xcc->model->cpudef->versions; > + > + if (vdef == NULL) { > + return; > + } else { > + if (vdef[effective_version - 1].deprecated) { > + warn_report("Effective CPU model '%s' -- %s", > + x86_cpu_versioned_model_name(xcc->model->cpudef,\ > + effective_version), > + vdef[effective_version - 1].note); > + } > + } Why do we need this extra logic? Isn't it simpler to just add a bool CPUClass::deprecated field, and set: cpu->deprecated = model->deprecated; inside x86_cpu_cpudef_class_init()? > +} > + > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > { > X86CPUClass *xcc = X86_CPU_CLASS(oc); > @@ -7291,6 +7331,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > cc->tlb_fill = x86_cpu_tlb_fill; > #endif > cc->disas_set_info = x86_disas_set_info; > + cc->deprecation_check = x86_cpu_deprecation_check; > > dc->user_creatable = true; > } > -- > 1.8.3.1 >
On 9/9/20 8:15 PM, Eduardo Habkost wrote: > Hi, > > Thanks for the patch, and sorry for taking so long to review > this. I'm finally getting to the patches that were postponed to > 5.2. Sorry I missed that patch too. Why restrict this to CPUClass, and not provide this feature to all ObjectClass? > > Comments and questions below: > > On Thu, Jun 11, 2020 at 10:47:55AM +0800, Robert Hoo wrote: >> Complement versioned CPU model framework with the ability of marking some >> versions deprecated. When that CPU model is chosen, get some warning. The >> warning message is customized, e.g. telling in which future QEMU version will >> it be obsoleted. >> The deprecation message will also appear by x86_cpu_list_entry(), e.g. '-cpu >> help'. >> QMP 'query-cpu-definitions' will also return a bool value indicating the >> deprecation status. >> >> Changes in v2: >> Move deprecation check from parse_cpu_option() to machine_run_board_init(), so >> that it can cover implicit cpu_type assignment cases. >> Add qapi new member documentation. Thanks Eric for comment and guidance on qapi. >> >> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> >> --- >> hw/core/machine.c | 11 +++++++++-- >> include/hw/core/cpu.h | 1 + >> qapi/machine-target.json | 7 ++++++- >> target/i386/cpu.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- >> 4 files changed, 59 insertions(+), 5 deletions(-) >> >> diff --git a/hw/core/machine.c b/hw/core/machine.c >> index bb3a7b1..9318964 100644 >> --- a/hw/core/machine.c >> +++ b/hw/core/machine.c >> @@ -1083,6 +1083,8 @@ MemoryRegion *machine_consume_memdev(MachineState *machine, >> void machine_run_board_init(MachineState *machine) >> { >> MachineClass *machine_class = MACHINE_GET_CLASS(machine); >> + ObjectClass *oc = object_class_by_name(machine->cpu_type); >> + CPUClass *cc; >> >> if (machine->ram_memdev_id) { >> Object *o; >> @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState *machine) >> * specified a CPU with -cpu check here that the user CPU is supported. >> */ >> if (machine_class->valid_cpu_types && machine->cpu_type) { >> - ObjectClass *class = object_class_by_name(machine->cpu_type); >> int i; >> >> for (i = 0; machine_class->valid_cpu_types[i]; i++) { >> - if (object_class_dynamic_cast(class, >> + if (object_class_dynamic_cast(oc, >> machine_class->valid_cpu_types[i])) { >> /* The user specificed CPU is in the valid field, we are >> * good to go. >> @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState *machine) >> } >> } >> >> + /* Check if CPU type is deprecated and warn if so */ >> + cc = CPU_CLASS(oc); >> + if (cc->deprecation_check) { >> + cc->deprecation_check(oc); >> + } > > Why do we need target-specific code here? A CPUClass::deprecated > field would be much simpler. > >> + >> machine_class->init(machine); >> } >> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index 497600c..1ca47dc 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -218,6 +218,7 @@ typedef struct CPUClass { >> void (*disas_set_info)(CPUState *cpu, disassemble_info *info); >> vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len); >> void (*tcg_initialize)(void); >> + void (*deprecation_check)(ObjectClass *oc); >> >> /* Keep non-pointer data at the end to minimize holes. */ >> int gdb_num_core_regs; >> diff --git a/qapi/machine-target.json b/qapi/machine-target.json >> index f2c8294..c24f506 100644 >> --- a/qapi/machine-target.json >> +++ b/qapi/machine-target.json >> @@ -285,6 +285,10 @@ >> # in the VM configuration, because aliases may stop being >> # migration-safe in the future (since 4.1) >> # >> +# @deprecated: If true, this CPU model is deprecated and may be removed in >> +# in some future version of QEMU according to the QEMU deprecation >> +# policy. (since 5.1) > > Next version needs to say "since 5.2". > >> +# >> # @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 >> @@ -309,7 +313,8 @@ >> 'static': 'bool', >> '*unavailable-features': [ 'str' ], >> 'typename': 'str', >> - '*alias-of' : 'str' }, >> + '*alias-of' : 'str', >> + 'deprecated' : 'bool' }, >> 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' } >> >> ## >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index ba05da3..0d8638a 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { >> const char *alias; >> const char *note; >> PropValue *props; >> + bool deprecated; >> } X86CPUVersionDefinition; >> >> /* Base definition for a CPU model */ >> @@ -1638,6 +1639,11 @@ struct X86CPUModel { >> * This matters only for "-cpu help" and query-cpu-definitions >> */ >> bool is_alias; >> + /* >> + * If true, this model is deprecated, and may be removed in the future. >> + * Trying to use it now will cause a warning. >> + */ >> + bool deprecated; >> }; >> >> /* Get full model name for CPU version */ >> @@ -4128,8 +4134,7 @@ static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model) >> X86CPUVersion v = model->version; >> if (v == CPU_VERSION_AUTO) { >> v = default_cpu_version; >> - } >> - if (v == CPU_VERSION_LATEST) { >> + } else if (v == CPU_VERSION_LATEST) { > > Why is this change necessary? > >> return x86_cpu_model_last_version(model); >> } >> return v; >> @@ -4975,6 +4980,7 @@ 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; >> + info->deprecated = cc->model ? cc->model->deprecated : false; >> /* >> * Old machine types won't report aliases, so that alias translation >> * doesn't break compatibility with previous QEMU versions. >> @@ -5411,6 +5417,7 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) >> m->cpudef = def; >> m->version = vdef->version; >> m->note = vdef->note; >> + m->deprecated = vdef->deprecated; >> x86_register_cpu_model_type(name, m); >> >> if (vdef->alias) { >> @@ -5418,6 +5425,8 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) >> am->cpudef = def; >> am->version = vdef->version; >> am->is_alias = true; >> + am->note = vdef->note; > > Is this extra line related to the deprecation feature? > > It doesn't seem related, and it doesn't seem necessary as the > `note` field is already ignored for alias CPU models. > >> + am->deprecated = vdef->deprecated; >> x86_register_cpu_model_type(vdef->alias, am); >> } >> } >> @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { >> DEFINE_PROP_END_OF_LIST() >> }; >> >> +static void x86_cpu_deprecation_check(ObjectClass *oc) >> +{ >> + X86CPUClass *xcc = X86_CPU_CLASS(oc); >> + X86CPUVersion effective_version; >> + const X86CPUVersionDefinition *vdef; >> + >> + if (xcc->model == NULL) { >> + return; >> + } >> + >> + if (xcc->model->version == CPU_VERSION_LEGACY) { >> + /* Treat legacy version as v1 */ >> + effective_version = 1; >> + } else { >> + effective_version = x86_cpu_model_resolve_version(xcc->model); >> + } >> + >> + vdef = xcc->model->cpudef->versions; >> + >> + if (vdef == NULL) { >> + return; >> + } else { >> + if (vdef[effective_version - 1].deprecated) { >> + warn_report("Effective CPU model '%s' -- %s", >> + x86_cpu_versioned_model_name(xcc->model->cpudef,\ >> + effective_version), >> + vdef[effective_version - 1].note); >> + } >> + } > > Why do we need this extra logic? Isn't it simpler to just add a > bool CPUClass::deprecated field, and set: > > cpu->deprecated = model->deprecated; > > inside x86_cpu_cpudef_class_init()? > >> +} >> + >> static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >> { >> X86CPUClass *xcc = X86_CPU_CLASS(oc); >> @@ -7291,6 +7331,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >> cc->tlb_fill = x86_cpu_tlb_fill; >> #endif >> cc->disas_set_info = x86_disas_set_info; >> + cc->deprecation_check = x86_cpu_deprecation_check; >> >> dc->user_creatable = true; >> } >> -- >> 1.8.3.1 >> >
On Thu, Sep 10, 2020 at 07:29:03AM +0200, Philippe Mathieu-Daudé wrote: > On 9/9/20 8:15 PM, Eduardo Habkost wrote: > > Hi, > > > > Thanks for the patch, and sorry for taking so long to review > > this. I'm finally getting to the patches that were postponed to > > 5.2. > > Sorry I missed that patch too. > > Why restrict this to CPUClass, and not provide this feature to > all ObjectClass? We could do it, but the field would be left unused for anything that's not a CPU model or machine type, as there are no other QMP interfaces for querying deprecation status yet. A QMP interface for querying deprecation status of device types in general would be useful, though.
On 9/10/20 9:12 PM, Eduardo Habkost wrote: > On Thu, Sep 10, 2020 at 07:29:03AM +0200, Philippe Mathieu-Daudé wrote: >> On 9/9/20 8:15 PM, Eduardo Habkost wrote: >>> Hi, >>> >>> Thanks for the patch, and sorry for taking so long to review >>> this. I'm finally getting to the patches that were postponed to >>> 5.2. >> >> Sorry I missed that patch too. >> >> Why restrict this to CPUClass, and not provide this feature to >> all ObjectClass? > > We could do it, but the field would be left unused for anything > that's not a CPU model or machine type, as there are no other QMP > interfaces for querying deprecation status yet. > > A QMP interface for querying deprecation status of device types > in general would be useful, though. OK we can move that later, thanks.
On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote: > Hi, > > Thanks for the patch, and sorry for taking so long to review > this. I'm finally getting to the patches that were postponed to > 5.2. > > Comments and questions below: > > On Thu, Jun 11, 2020 at 10:47:55AM +0800, Robert Hoo wrote: > > Complement versioned CPU model framework with the ability of > > marking some > > versions deprecated. When that CPU model is chosen, get some > > warning. The > > warning message is customized, e.g. telling in which future QEMU > > version will > > it be obsoleted. > > The deprecation message will also appear by x86_cpu_list_entry(), > > e.g. '-cpu > > help'. > > QMP 'query-cpu-definitions' will also return a bool value > > indicating the > > deprecation status. > > > > Changes in v2: > > Move deprecation check from parse_cpu_option() to > > machine_run_board_init(), so > > that it can cover implicit cpu_type assignment cases. > > Add qapi new member documentation. Thanks Eric for comment and > > guidance on qapi. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > hw/core/machine.c | 11 +++++++++-- > > include/hw/core/cpu.h | 1 + > > qapi/machine-target.json | 7 ++++++- > > target/i386/cpu.c | 45 > > +++++++++++++++++++++++++++++++++++++++++++-- > > 4 files changed, 59 insertions(+), 5 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index bb3a7b1..9318964 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -1083,6 +1083,8 @@ MemoryRegion > > *machine_consume_memdev(MachineState *machine, > > void machine_run_board_init(MachineState *machine) > > { > > MachineClass *machine_class = MACHINE_GET_CLASS(machine); > > + ObjectClass *oc = object_class_by_name(machine->cpu_type); > > + CPUClass *cc; > > > > if (machine->ram_memdev_id) { > > Object *o; > > @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState > > *machine) > > * specified a CPU with -cpu check here that the user CPU is > > supported. > > */ > > if (machine_class->valid_cpu_types && machine->cpu_type) { > > - ObjectClass *class = object_class_by_name(machine- > > >cpu_type); > > int i; > > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > > - if (object_class_dynamic_cast(class, > > + if (object_class_dynamic_cast(oc, > > machine_class- > > >valid_cpu_types[i])) { > > /* The user specificed CPU is in the valid field, > > we are > > * good to go. > > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState > > *machine) > > } > > } > > > > + /* Check if CPU type is deprecated and warn if so */ > > + cc = CPU_CLASS(oc); > > + if (cc->deprecation_check) { > > + cc->deprecation_check(oc); > > + } > > Why do we need target-specific code here? A CPUClass::deprecated > field would be much simpler. > Because the Warning message composing is target-specific, using X86CPUVersionDefinition.note. Other targets can have their own warning message composing approaches. > > + > > machine_class->init(machine); > > } > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > index 497600c..1ca47dc 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -218,6 +218,7 @@ typedef struct CPUClass { > > void (*disas_set_info)(CPUState *cpu, disassemble_info *info); > > vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, > > int len); > > void (*tcg_initialize)(void); > > + void (*deprecation_check)(ObjectClass *oc); > > > > /* Keep non-pointer data at the end to minimize holes. */ > > int gdb_num_core_regs; > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > > index f2c8294..c24f506 100644 > > --- a/qapi/machine-target.json > > +++ b/qapi/machine-target.json > > @@ -285,6 +285,10 @@ > > # in the VM configuration, because aliases may stop > > being > > # migration-safe in the future (since 4.1) > > # > > +# @deprecated: If true, this CPU model is deprecated and may be > > removed in > > +# in some future version of QEMU according to the > > QEMU deprecation > > +# policy. (since 5.1) > > Next version needs to say "since 5.2". Sure. > > > +# > > # @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 > > @@ -309,7 +313,8 @@ > > 'static': 'bool', > > '*unavailable-features': [ 'str' ], > > 'typename': 'str', > > - '*alias-of' : 'str' }, > > + '*alias-of' : 'str', > > + 'deprecated' : 'bool' }, > > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || > > defined(TARGET_I386) || defined(TARGET_S390X) || > > defined(TARGET_MIPS)' } > > > > ## > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index ba05da3..0d8638a 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { > > const char *alias; > > const char *note; > > PropValue *props; > > + bool deprecated; > > } X86CPUVersionDefinition; > > > > /* Base definition for a CPU model */ > > @@ -1638,6 +1639,11 @@ struct X86CPUModel { > > * This matters only for "-cpu help" and query-cpu-definitions > > */ > > bool is_alias; > > + /* > > + * If true, this model is deprecated, and may be removed in > > the future. > > + * Trying to use it now will cause a warning. > > + */ > > + bool deprecated; > > }; > > > > /* Get full model name for CPU version */ > > @@ -4128,8 +4134,7 @@ static X86CPUVersion > > x86_cpu_model_resolve_version(const X86CPUModel *model) > > X86CPUVersion v = model->version; > > if (v == CPU_VERSION_AUTO) { > > v = default_cpu_version; > > - } > > - if (v == CPU_VERSION_LATEST) { > > + } else if (v == CPU_VERSION_LATEST) { > > Why is this change necessary? Just kind of compulsion of avoiding unnecessary if() :-). 'v' can only be one of CPU_VERSION_AUTO and CPU_VERSION_LATEST, unnecessary to judge twice. > > > return x86_cpu_model_last_version(model); > > } > > return v; > > @@ -4975,6 +4980,7 @@ 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; > > + info->deprecated = cc->model ? cc->model->deprecated : false; > > /* > > * Old machine types won't report aliases, so that alias > > translation > > * doesn't break compatibility with previous QEMU versions. > > @@ -5411,6 +5417,7 @@ static void > > x86_register_cpudef_types(X86CPUDefinition *def) > > m->cpudef = def; > > m->version = vdef->version; > > m->note = vdef->note; > > + m->deprecated = vdef->deprecated; > > x86_register_cpu_model_type(name, m); > > > > if (vdef->alias) { > > @@ -5418,6 +5425,8 @@ static void > > x86_register_cpudef_types(X86CPUDefinition *def) > > am->cpudef = def; > > am->version = vdef->version; > > am->is_alias = true; > > + am->note = vdef->note; > > Is this extra line related to the deprecation feature? > > It doesn't seem related, and it doesn't seem necessary as the > `note` field is already ignored for alias CPU models. Because it is unused by other features, I use it to store model specific deprecation message. > > > + am->deprecated = vdef->deprecated; > > x86_register_cpu_model_type(vdef->alias, am); > > } > > } > > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { > > DEFINE_PROP_END_OF_LIST() > > }; > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > > +{ > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > + X86CPUVersion effective_version; > > + const X86CPUVersionDefinition *vdef; > > + > > + if (xcc->model == NULL) { > > + return; > > + } > > + > > + if (xcc->model->version == CPU_VERSION_LEGACY) { > > + /* Treat legacy version as v1 */ > > + effective_version = 1; > > + } else { > > + effective_version = x86_cpu_model_resolve_version(xcc- > > >model); > > + } > > + > > + vdef = xcc->model->cpudef->versions; > > + > > + if (vdef == NULL) { > > + return; > > + } else { > > + if (vdef[effective_version - 1].deprecated) { > > + warn_report("Effective CPU model '%s' -- %s", > > + x86_cpu_versioned_model_name(xcc->model- > > >cpudef,\ > > + effective_version) > > , > > + vdef[effective_version - 1].note); > > + } > > + } > > Why do we need this extra logic? Isn't it simpler to just add a > bool CPUClass::deprecated field, and set: > > cpu->deprecated = model->deprecated; > > inside x86_cpu_cpudef_class_init()? > All these are to fulfill the target you expected earlier: "We need a proper CPU model deprecation API. Deprecation info should appear on query-cpu-definitions and should trigger a warning when using the CPU model." So I think each deprecated model shall have its own deprecation message, e.g. by which version it's going to be deprecation, etc. > > +} > > + > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > { > > X86CPUClass *xcc = X86_CPU_CLASS(oc); > > @@ -7291,6 +7331,7 @@ static void > > x86_cpu_common_class_init(ObjectClass *oc, void *data) > > cc->tlb_fill = x86_cpu_tlb_fill; > > #endif > > cc->disas_set_info = x86_disas_set_info; > > + cc->deprecation_check = x86_cpu_deprecation_check; > > > > dc->user_creatable = true; > > } > > -- > > 1.8.3.1 > > > >
On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote: > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote: > > Hi, > > > > Thanks for the patch, and sorry for taking so long to review > > this. I'm finally getting to the patches that were postponed to > > 5.2. > > > > Comments and questions below: > > > > On Thu, Jun 11, 2020 at 10:47:55AM +0800, Robert Hoo wrote: > > > Complement versioned CPU model framework with the ability of > > > marking some > > > versions deprecated. When that CPU model is chosen, get some > > > warning. The > > > warning message is customized, e.g. telling in which future QEMU > > > version will > > > it be obsoleted. > > > The deprecation message will also appear by x86_cpu_list_entry(), > > > e.g. '-cpu > > > help'. > > > QMP 'query-cpu-definitions' will also return a bool value > > > indicating the > > > deprecation status. > > > > > > Changes in v2: > > > Move deprecation check from parse_cpu_option() to > > > machine_run_board_init(), so > > > that it can cover implicit cpu_type assignment cases. > > > Add qapi new member documentation. Thanks Eric for comment and > > > guidance on qapi. > > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > > --- > > > hw/core/machine.c | 11 +++++++++-- > > > include/hw/core/cpu.h | 1 + > > > qapi/machine-target.json | 7 ++++++- > > > target/i386/cpu.c | 45 > > > +++++++++++++++++++++++++++++++++++++++++++-- > > > 4 files changed, 59 insertions(+), 5 deletions(-) > > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > > index bb3a7b1..9318964 100644 > > > --- a/hw/core/machine.c > > > +++ b/hw/core/machine.c > > > @@ -1083,6 +1083,8 @@ MemoryRegion > > > *machine_consume_memdev(MachineState *machine, > > > void machine_run_board_init(MachineState *machine) > > > { > > > MachineClass *machine_class = MACHINE_GET_CLASS(machine); > > > + ObjectClass *oc = object_class_by_name(machine->cpu_type); > > > + CPUClass *cc; > > > > > > if (machine->ram_memdev_id) { > > > Object *o; > > > @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState > > > *machine) > > > * specified a CPU with -cpu check here that the user CPU is > > > supported. > > > */ > > > if (machine_class->valid_cpu_types && machine->cpu_type) { > > > - ObjectClass *class = object_class_by_name(machine- > > > >cpu_type); > > > int i; > > > > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) { > > > - if (object_class_dynamic_cast(class, > > > + if (object_class_dynamic_cast(oc, > > > machine_class- > > > >valid_cpu_types[i])) { > > > /* The user specificed CPU is in the valid field, > > > we are > > > * good to go. > > > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState > > > *machine) > > > } > > > } > > > > > > + /* Check if CPU type is deprecated and warn if so */ > > > + cc = CPU_CLASS(oc); > > > + if (cc->deprecation_check) { > > > + cc->deprecation_check(oc); > > > + } > > > > Why do we need target-specific code here? A CPUClass::deprecated > > field would be much simpler. > > > Because the Warning message composing is target-specific, using > X86CPUVersionDefinition.note. > Other targets can have their own warning message composing approaches. I think I understand what you were trying to do, but having each target with a different warning message would be a bad thing, not a desirable feature. The warning message can be generic. > > > > + > > > machine_class->init(machine); > > > } > > > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > > index 497600c..1ca47dc 100644 > > > --- a/include/hw/core/cpu.h > > > +++ b/include/hw/core/cpu.h > > > @@ -218,6 +218,7 @@ typedef struct CPUClass { > > > void (*disas_set_info)(CPUState *cpu, disassemble_info *info); > > > vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, > > > int len); > > > void (*tcg_initialize)(void); > > > + void (*deprecation_check)(ObjectClass *oc); > > > > > > /* Keep non-pointer data at the end to minimize holes. */ > > > int gdb_num_core_regs; > > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > > > index f2c8294..c24f506 100644 > > > --- a/qapi/machine-target.json > > > +++ b/qapi/machine-target.json > > > @@ -285,6 +285,10 @@ > > > # in the VM configuration, because aliases may stop > > > being > > > # migration-safe in the future (since 4.1) > > > # > > > +# @deprecated: If true, this CPU model is deprecated and may be > > > removed in > > > +# in some future version of QEMU according to the > > > QEMU deprecation > > > +# policy. (since 5.1) > > > > Next version needs to say "since 5.2". > > Sure. > > > > > +# > > > # @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 > > > @@ -309,7 +313,8 @@ > > > 'static': 'bool', > > > '*unavailable-features': [ 'str' ], > > > 'typename': 'str', > > > - '*alias-of' : 'str' }, > > > + '*alias-of' : 'str', > > > + 'deprecated' : 'bool' }, > > > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || > > > defined(TARGET_I386) || defined(TARGET_S390X) || > > > defined(TARGET_MIPS)' } > > > > > > ## > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index ba05da3..0d8638a 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { > > > const char *alias; > > > const char *note; > > > PropValue *props; > > > + bool deprecated; > > > } X86CPUVersionDefinition; > > > > > > /* Base definition for a CPU model */ > > > @@ -1638,6 +1639,11 @@ struct X86CPUModel { > > > * This matters only for "-cpu help" and query-cpu-definitions > > > */ > > > bool is_alias; > > > + /* > > > + * If true, this model is deprecated, and may be removed in > > > the future. > > > + * Trying to use it now will cause a warning. > > > + */ > > > + bool deprecated; > > > }; > > > > > > /* Get full model name for CPU version */ > > > @@ -4128,8 +4134,7 @@ static X86CPUVersion > > > x86_cpu_model_resolve_version(const X86CPUModel *model) > > > X86CPUVersion v = model->version; > > > if (v == CPU_VERSION_AUTO) { > > > v = default_cpu_version; > > > - } > > > - if (v == CPU_VERSION_LATEST) { > > > + } else if (v == CPU_VERSION_LATEST) { > > > > Why is this change necessary? > > Just kind of compulsion of avoiding unnecessary if() :-). 'v' can only > be one of CPU_VERSION_AUTO and CPU_VERSION_LATEST, unnecessary to judge > twice. I think this breaks the case where default_cpu_version is set to CPU_VERSION_LATEST > > > > > return x86_cpu_model_last_version(model); > > > } > > > return v; > > > @@ -4975,6 +4980,7 @@ 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; > > > + info->deprecated = cc->model ? cc->model->deprecated : false; > > > /* > > > * Old machine types won't report aliases, so that alias > > > translation > > > * doesn't break compatibility with previous QEMU versions. > > > @@ -5411,6 +5417,7 @@ static void > > > x86_register_cpudef_types(X86CPUDefinition *def) > > > m->cpudef = def; > > > m->version = vdef->version; > > > m->note = vdef->note; > > > + m->deprecated = vdef->deprecated; > > > x86_register_cpu_model_type(name, m); > > > > > > if (vdef->alias) { > > > @@ -5418,6 +5425,8 @@ static void > > > x86_register_cpudef_types(X86CPUDefinition *def) > > > am->cpudef = def; > > > am->version = vdef->version; > > > am->is_alias = true; > > > + am->note = vdef->note; > > > > Is this extra line related to the deprecation feature? > > > > It doesn't seem related, and it doesn't seem necessary as the > > `note` field is already ignored for alias CPU models. > > Because it is unused by other features, I use it to store model > specific deprecation message. > > > > > + am->deprecated = vdef->deprecated; > > > x86_register_cpu_model_type(vdef->alias, am); > > > } > > > } > > > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { > > > DEFINE_PROP_END_OF_LIST() > > > }; > > > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > > > +{ > > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > + X86CPUVersion effective_version; > > > + const X86CPUVersionDefinition *vdef; > > > + > > > + if (xcc->model == NULL) { > > > + return; > > > + } > > > + > > > + if (xcc->model->version == CPU_VERSION_LEGACY) { > > > + /* Treat legacy version as v1 */ > > > + effective_version = 1; > > > + } else { > > > + effective_version = x86_cpu_model_resolve_version(xcc- > > > >model); > > > + } > > > + > > > + vdef = xcc->model->cpudef->versions; > > > + > > > + if (vdef == NULL) { > > > + return; > > > + } else { > > > + if (vdef[effective_version - 1].deprecated) { > > > + warn_report("Effective CPU model '%s' -- %s", > > > + x86_cpu_versioned_model_name(xcc->model- > > > >cpudef,\ > > > + effective_version) > > > , > > > + vdef[effective_version - 1].note); > > > + } > > > + } > > > > Why do we need this extra logic? Isn't it simpler to just add a > > bool CPUClass::deprecated field, and set: > > > > cpu->deprecated = model->deprecated; > > > > inside x86_cpu_cpudef_class_init()? > > > All these are to fulfill the target you expected earlier: > > "We need a proper CPU model deprecation API. Deprecation info > should appear on query-cpu-definitions and should trigger a > warning when using the CPU model." > > So I think each deprecated model shall have its own deprecation > message, e.g. by which version it's going to be deprecation, etc. There's nothing x86-specific about having deprecated CPU models, so I don't understand the reason for the x86-specific hook. If the .note field is the reason you added the arch-specific hook, you can just add a CPUClass::deprecation_note field and make the feature generic. > > > > +} > > > + > > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > { > > > X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > @@ -7291,6 +7331,7 @@ static void > > > x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > cc->tlb_fill = x86_cpu_tlb_fill; > > > #endif > > > cc->disas_set_info = x86_disas_set_info; > > > + cc->deprecation_check = x86_cpu_deprecation_check; > > > > > > dc->user_creatable = true; > > > } > > > -- > > > 1.8.3.1 > > > > > > > >
On Fri, 2020-09-11 at 10:00 -0400, Eduardo Habkost wrote: > On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote: > > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote: > > > Hi, > > > > > > > > > > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState > > > > *machine) > > > > } > > > > } > > > > > > > > + /* Check if CPU type is deprecated and warn if so */ > > > > + cc = CPU_CLASS(oc); > > > > + if (cc->deprecation_check) { > > > > + cc->deprecation_check(oc); > > > > + } > > > > > > Why do we need target-specific code here? A CPUClass::deprecated > > > field would be much simpler. > > > > > > > Because the Warning message composing is target-specific, using > > X86CPUVersionDefinition.note. > > Other targets can have their own warning message composing > > approaches. > > I think I understand what you were trying to do, but having each > target with a different warning message would be a bad thing, not > a desirable feature. The warning message can be generic. > > > > > > > + > > > > machine_class->init(machine); > > > > } > > > > > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > > > index 497600c..1ca47dc 100644 > > > > --- a/include/hw/core/cpu.h > > > > +++ b/include/hw/core/cpu.h > > > > @@ -218,6 +218,7 @@ typedef struct CPUClass { > > > > void (*disas_set_info)(CPUState *cpu, disassemble_info > > > > *info); > > > > vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr > > > > addr, > > > > int len); > > > > void (*tcg_initialize)(void); > > > > + void (*deprecation_check)(ObjectClass *oc); > > > > > > > > /* Keep non-pointer data at the end to minimize holes. */ > > > > int gdb_num_core_regs; > > > > diff --git a/qapi/machine-target.json b/qapi/machine- > > > > target.json > > > > index f2c8294..c24f506 100644 > > > > --- a/qapi/machine-target.json > > > > +++ b/qapi/machine-target.json > > > > @@ -285,6 +285,10 @@ > > > > # in the VM configuration, because aliases may stop > > > > being > > > > # migration-safe in the future (since 4.1) > > > > # > > > > +# @deprecated: If true, this CPU model is deprecated and may > > > > be > > > > removed in > > > > +# in some future version of QEMU according to the > > > > QEMU deprecation > > > > +# policy. (since 5.1) > > > > > > Next version needs to say "since 5.2". > > > > Sure. > > > > > > > > > > > > > > > /* Get full model name for CPU version */ > > > > @@ -4128,8 +4134,7 @@ static X86CPUVersion > > > > x86_cpu_model_resolve_version(const X86CPUModel *model) > > > > X86CPUVersion v = model->version; > > > > if (v == CPU_VERSION_AUTO) { > > > > v = default_cpu_version; > > > > - } > > > > - if (v == CPU_VERSION_LATEST) { > > > > + } else if (v == CPU_VERSION_LATEST) { > > > > > > Why is this change necessary? > > > > Just kind of compulsion of avoiding unnecessary if() :-). 'v' can > > only > > be one of CPU_VERSION_AUTO and CPU_VERSION_LATEST, unnecessary to > > judge > > twice. > > I think this breaks the case where default_cpu_version is set to > CPU_VERSION_LATEST > OK, understand. > > > > > > > return x86_cpu_model_last_version(model); > > > > } > > > > return v; > > > > @@ -4975,6 +4980,7 @@ 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; > > > > + info->deprecated = cc->model ? cc->model->deprecated : > > > > false; > > > > /* > > > > * Old machine types won't report aliases, so that alias > > > > translation > > > > * doesn't break compatibility with previous QEMU > > > > versions. > > > > @@ -5411,6 +5417,7 @@ static void > > > > x86_register_cpudef_types(X86CPUDefinition *def) > > > > m->cpudef = def; > > > > m->version = vdef->version; > > > > m->note = vdef->note; > > > > + m->deprecated = vdef->deprecated; > > > > x86_register_cpu_model_type(name, m); > > > > > > > > if (vdef->alias) { > > > > @@ -5418,6 +5425,8 @@ static void > > > > x86_register_cpudef_types(X86CPUDefinition *def) > > > > am->cpudef = def; > > > > am->version = vdef->version; > > > > am->is_alias = true; > > > > + am->note = vdef->note; > > > > > > Is this extra line related to the deprecation feature? > > > > > > It doesn't seem related, and it doesn't seem necessary as the > > > `note` field is already ignored for alias CPU models. > > > > Because it is unused by other features, I use it to store model > > specific deprecation message. > > > > > > > + am->deprecated = vdef->deprecated; > > > > x86_register_cpu_model_type(vdef->alias, am); > > > > } > > > > } > > > > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { > > > > DEFINE_PROP_END_OF_LIST() > > > > }; > > > > > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > > > > +{ > > > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > > + X86CPUVersion effective_version; > > > > + const X86CPUVersionDefinition *vdef; > > > > + > > > > + if (xcc->model == NULL) { > > > > + return; > > > > + } > > > > + > > > > + if (xcc->model->version == CPU_VERSION_LEGACY) { > > > > + /* Treat legacy version as v1 */ > > > > + effective_version = 1; > > > > + } else { > > > > + effective_version = x86_cpu_model_resolve_version(xcc- > > > > > model); > > > > > > > > + } > > > > + > > > > + vdef = xcc->model->cpudef->versions; > > > > + > > > > + if (vdef == NULL) { > > > > + return; > > > > + } else { > > > > + if (vdef[effective_version - 1].deprecated) { > > > > + warn_report("Effective CPU model '%s' -- %s", > > > > + x86_cpu_versioned_model_name(xcc->model- > > > > > cpudef,\ > > > > > > > > + effective_vers > > > > ion) > > > > , > > > > + vdef[effective_version - 1].note); > > > > + } > > > > + } > > > > > > Why do we need this extra logic? Isn't it simpler to just add a > > > bool CPUClass::deprecated field, and set: > > > > > > cpu->deprecated = model->deprecated; > > > > > > inside x86_cpu_cpudef_class_init()? > > > > > > > All these are to fulfill the target you expected earlier: > > > > "We need a proper CPU model deprecation API. Deprecation info > > should appear on query-cpu-definitions and should trigger a > > warning when using the CPU model." > > > > So I think each deprecated model shall have its own deprecation > > message, e.g. by which version it's going to be deprecation, etc. > > There's nothing x86-specific about having deprecated CPU models, > so I don't understand the reason for the x86-specific hook. > > If the .note field is the reason you added the arch-specific > hook, you can just add a CPUClass::deprecation_note field and > make the feature generic. > I tend to agree with you on this generalization requirement. But then I find it still has some tricky thing, perhaps that's why I defined this x86 target specific hook: 1) The versioned CPU model is x86 specific (at least at present) 2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then its versioned cpu_model_types. Refer to code in x86_register_cpudef_types(). The unversioned model won't be marked deprecated as it is unkown when registered. In machine_run_board_init(), the cpu_model being checked is the unversioned one, if I set -cpu to its general unversioned model. In short, the unversioned cpudef CPU model would escape the deprecation check. > > > > > > +} > > > > + > > > > static void x86_cpu_common_class_init(ObjectClass *oc, void > > > > *data) > > > > { > > > > X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > > @@ -7291,6 +7331,7 @@ static void > > > > x86_cpu_common_class_init(ObjectClass *oc, void *data) > > > > cc->tlb_fill = x86_cpu_tlb_fill; > > > > #endif > > > > cc->disas_set_info = x86_disas_set_info; > > > > + cc->deprecation_check = x86_cpu_deprecation_check; > > > > > > > > dc->user_creatable = true; > > > > } > > > > -- > > > > 1.8.3.1 > > > > > > > > > > > >
On Mon, Sep 14, 2020 at 06:50:09PM +0800, Robert Hoo wrote: > On Fri, 2020-09-11 at 10:00 -0400, Eduardo Habkost wrote: > > On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote: > > > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote: [...] > > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > > > > > +{ > > > > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > > > + X86CPUVersion effective_version; > > > > > + const X86CPUVersionDefinition *vdef; > > > > > + > > > > > + if (xcc->model == NULL) { > > > > > + return; > > > > > + } > > > > > + > > > > > + if (xcc->model->version == CPU_VERSION_LEGACY) { > > > > > + /* Treat legacy version as v1 */ > > > > > + effective_version = 1; > > > > > + } else { > > > > > + effective_version = x86_cpu_model_resolve_version(xcc- > > > > > > model); > > > > > > > > > > + } > > > > > + > > > > > + vdef = xcc->model->cpudef->versions; > > > > > + > > > > > + if (vdef == NULL) { > > > > > + return; > > > > > + } else { > > > > > + if (vdef[effective_version - 1].deprecated) { > > > > > + warn_report("Effective CPU model '%s' -- %s", > > > > > + x86_cpu_versioned_model_name(xcc->model- > > > > > > cpudef,\ > > > > > > > > > > + effective_vers > > > > > ion) > > > > > , > > > > > + vdef[effective_version - 1].note); > > > > > + } > > > > > + } > > > > > > > > Why do we need this extra logic? Isn't it simpler to just add a > > > > bool CPUClass::deprecated field, and set: > > > > > > > > cpu->deprecated = model->deprecated; > > > > > > > > inside x86_cpu_cpudef_class_init()? > > > > > > > > > > All these are to fulfill the target you expected earlier: > > > > > > "We need a proper CPU model deprecation API. Deprecation info > > > should appear on query-cpu-definitions and should trigger a > > > warning when using the CPU model." > > > > > > So I think each deprecated model shall have its own deprecation > > > message, e.g. by which version it's going to be deprecation, etc. > > > > There's nothing x86-specific about having deprecated CPU models, > > so I don't understand the reason for the x86-specific hook. > > > > If the .note field is the reason you added the arch-specific > > hook, you can just add a CPUClass::deprecation_note field and > > make the feature generic. > > > I tend to agree with you on this generalization requirement. > > But then I find it still has some tricky thing, perhaps that's why I > defined this x86 target specific hook: > > 1) The versioned CPU model is x86 specific (at least at present) I don't see why this would be an obstacle. You just need to set CPUClass::deprecated and/or CPUClass::deprecation_note in the x86-specific class_init code. > > 2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then its > versioned cpu_model_types. Refer to code in > x86_register_cpudef_types(). The unversioned model won't be marked > deprecated as it is unkown when registered. In > machine_run_board_init(), the cpu_model being checked is the > unversioned one, if I set -cpu to its general unversioned model. > In short, the unversioned cpudef CPU model would escape the deprecation > check. Why is that a problem? If, for example, Model-v1 is deprecated and Model-v2 is not deprecated, we must never tell the user that "-cpu Model" is deprecated. Even if some machine types resolve "-cpu Model" to Model-v1.
On Mon, 2020-09-14 at 13:38 +0000, Eduardo Habkost wrote: > On Mon, Sep 14, 2020 at 06:50:09PM +0800, Robert Hoo wrote: > > On Fri, 2020-09-11 at 10:00 -0400, Eduardo Habkost wrote: > > > On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote: > > > > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote: > > [...] > > > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc) > > > > > > +{ > > > > > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > > > > > + X86CPUVersion effective_version; > > > > > > + const X86CPUVersionDefinition *vdef; > > > > > > + > > > > > > + if (xcc->model == NULL) { > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + if (xcc->model->version == CPU_VERSION_LEGACY) { > > > > > > + /* Treat legacy version as v1 */ > > > > > > + effective_version = 1; > > > > > > + } else { > > > > > > + effective_version = > > > > > > x86_cpu_model_resolve_version(xcc- > > > > > > > model); > > > > > > > > > > > > + } > > > > > > + > > > > > > + vdef = xcc->model->cpudef->versions; > > > > > > + > > > > > > + if (vdef == NULL) { > > > > > > + return; > > > > > > + } else { > > > > > > + if (vdef[effective_version - 1].deprecated) { > > > > > > + warn_report("Effective CPU model '%s' -- %s", > > > > > > + x86_cpu_versioned_model_name(xcc- > > > > > > >model- > > > > > > > cpudef,\ > > > > > > > > > > > > + effective_ > > > > > > vers > > > > > > ion) > > > > > > , > > > > > > + vdef[effective_version - 1].note); > > > > > > + } > > > > > > + } > > > > > > > > > > Why do we need this extra logic? Isn't it simpler to just > > > > > add a > > > > > bool CPUClass::deprecated field, and set: > > > > > > > > > > cpu->deprecated = model->deprecated; > > > > > > > > > > inside x86_cpu_cpudef_class_init()? > > > > > > > > > > > > > All these are to fulfill the target you expected earlier: > > > > > > > > "We need a proper CPU model deprecation API. Deprecation info > > > > should appear on query-cpu-definitions and should trigger a > > > > warning when using the CPU model." > > > > > > > > So I think each deprecated model shall have its own deprecation > > > > message, e.g. by which version it's going to be deprecation, > > > > etc. > > > > > > There's nothing x86-specific about having deprecated CPU models, > > > so I don't understand the reason for the x86-specific hook. > > > > > > If the .note field is the reason you added the arch-specific > > > hook, you can just add a CPUClass::deprecation_note field and > > > make the feature generic. > > > > > > > I tend to agree with you on this generalization requirement. > > > > But then I find it still has some tricky thing, perhaps that's why > > I > > defined this x86 target specific hook: > > > > 1) The versioned CPU model is x86 specific (at least at present) > > I don't see why this would be an obstacle. You just need to set > CPUClass::deprecated and/or CPUClass::deprecation_note in the > x86-specific class_init code. > > > > > 2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then > > its > > versioned cpu_model_types. Refer to code in > > x86_register_cpudef_types(). The unversioned model won't be marked > > deprecated as it is unkown when registered. In > > machine_run_board_init(), the cpu_model being checked is the > > unversioned one, if I set -cpu to its general unversioned model. > > In short, the unversioned cpudef CPU model would escape the > > deprecation > > check. > > Why is that a problem? If, for example, Model-v1 is deprecated > and Model-v2 is not deprecated, we must never tell the user that > "-cpu Model" is deprecated. Yes, that's why I cannot mark the unversioned one deprecated or not in its init. > Even if some machine types resolve > "-cpu Model" to Model-v1. > That's what I concerned. Say, if I named "-cpu Icelake-Client" and it's resolved to Icelake-CPU-v1 (deprecated), shouldn't we warn user? > -- > Eduardo >
On Tue, Sep 15, 2020 at 10:56:06AM +0800, Robert Hoo wrote: > On Mon, 2020-09-14 at 13:38 +0000, Eduardo Habkost wrote: > > On Mon, Sep 14, 2020 at 06:50:09PM +0800, Robert Hoo wrote: [...] > > > 2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then > > > its > > > versioned cpu_model_types. Refer to code in > > > x86_register_cpudef_types(). The unversioned model won't be marked > > > deprecated as it is unkown when registered. In > > > machine_run_board_init(), the cpu_model being checked is the > > > unversioned one, if I set -cpu to its general unversioned model. > > > In short, the unversioned cpudef CPU model would escape the > > > deprecation > > > check. > > > > Why is that a problem? If, for example, Model-v1 is deprecated > > and Model-v2 is not deprecated, we must never tell the user that > > "-cpu Model" is deprecated. > > Yes, that's why I cannot mark the unversioned one deprecated or not in > its init. > > > Even if some machine types resolve > > "-cpu Model" to Model-v1. > > > That's what I concerned. Say, if I named "-cpu Icelake-Client" and it's > resolved to Icelake-CPU-v1 (deprecated), shouldn't we warn user? For Icelake-Client, we want to make all versions of Icelake-Client deprecated, so "Icelake-Client" can and should be marked as deprecated at class_init time. I don't think we need to support a use case where "Model" is not deprecated bu "Model-v1" is.
diff --git a/hw/core/machine.c b/hw/core/machine.c index bb3a7b1..9318964 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1083,6 +1083,8 @@ MemoryRegion *machine_consume_memdev(MachineState *machine, void machine_run_board_init(MachineState *machine) { MachineClass *machine_class = MACHINE_GET_CLASS(machine); + ObjectClass *oc = object_class_by_name(machine->cpu_type); + CPUClass *cc; if (machine->ram_memdev_id) { Object *o; @@ -1102,11 +1104,10 @@ void machine_run_board_init(MachineState *machine) * specified a CPU with -cpu check here that the user CPU is supported. */ if (machine_class->valid_cpu_types && machine->cpu_type) { - ObjectClass *class = object_class_by_name(machine->cpu_type); int i; for (i = 0; machine_class->valid_cpu_types[i]; i++) { - if (object_class_dynamic_cast(class, + if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) { /* The user specificed CPU is in the valid field, we are * good to go. @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState *machine) } } + /* Check if CPU type is deprecated and warn if so */ + cc = CPU_CLASS(oc); + if (cc->deprecation_check) { + cc->deprecation_check(oc); + } + machine_class->init(machine); } diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 497600c..1ca47dc 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -218,6 +218,7 @@ typedef struct CPUClass { void (*disas_set_info)(CPUState *cpu, disassemble_info *info); vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len); void (*tcg_initialize)(void); + void (*deprecation_check)(ObjectClass *oc); /* Keep non-pointer data at the end to minimize holes. */ int gdb_num_core_regs; diff --git a/qapi/machine-target.json b/qapi/machine-target.json index f2c8294..c24f506 100644 --- a/qapi/machine-target.json +++ b/qapi/machine-target.json @@ -285,6 +285,10 @@ # in the VM configuration, because aliases may stop being # migration-safe in the future (since 4.1) # +# @deprecated: If true, this CPU model is deprecated and may be removed in +# in some future version of QEMU according to the QEMU deprecation +# policy. (since 5.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 @@ -309,7 +313,8 @@ 'static': 'bool', '*unavailable-features': [ 'str' ], 'typename': 'str', - '*alias-of' : 'str' }, + '*alias-of' : 'str', + 'deprecated' : 'bool' }, 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' } ## diff --git a/target/i386/cpu.c b/target/i386/cpu.c index ba05da3..0d8638a 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -1599,6 +1599,7 @@ typedef struct X86CPUVersionDefinition { const char *alias; const char *note; PropValue *props; + bool deprecated; } X86CPUVersionDefinition; /* Base definition for a CPU model */ @@ -1638,6 +1639,11 @@ struct X86CPUModel { * This matters only for "-cpu help" and query-cpu-definitions */ bool is_alias; + /* + * If true, this model is deprecated, and may be removed in the future. + * Trying to use it now will cause a warning. + */ + bool deprecated; }; /* Get full model name for CPU version */ @@ -4128,8 +4134,7 @@ static X86CPUVersion x86_cpu_model_resolve_version(const X86CPUModel *model) X86CPUVersion v = model->version; if (v == CPU_VERSION_AUTO) { v = default_cpu_version; - } - if (v == CPU_VERSION_LATEST) { + } else if (v == CPU_VERSION_LATEST) { return x86_cpu_model_last_version(model); } return v; @@ -4975,6 +4980,7 @@ 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; + info->deprecated = cc->model ? cc->model->deprecated : false; /* * Old machine types won't report aliases, so that alias translation * doesn't break compatibility with previous QEMU versions. @@ -5411,6 +5417,7 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) m->cpudef = def; m->version = vdef->version; m->note = vdef->note; + m->deprecated = vdef->deprecated; x86_register_cpu_model_type(name, m); if (vdef->alias) { @@ -5418,6 +5425,8 @@ static void x86_register_cpudef_types(X86CPUDefinition *def) am->cpudef = def; am->version = vdef->version; am->is_alias = true; + am->note = vdef->note; + am->deprecated = vdef->deprecated; x86_register_cpu_model_type(vdef->alias, am); } } @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_END_OF_LIST() }; +static void x86_cpu_deprecation_check(ObjectClass *oc) +{ + X86CPUClass *xcc = X86_CPU_CLASS(oc); + X86CPUVersion effective_version; + const X86CPUVersionDefinition *vdef; + + if (xcc->model == NULL) { + return; + } + + if (xcc->model->version == CPU_VERSION_LEGACY) { + /* Treat legacy version as v1 */ + effective_version = 1; + } else { + effective_version = x86_cpu_model_resolve_version(xcc->model); + } + + vdef = xcc->model->cpudef->versions; + + if (vdef == NULL) { + return; + } else { + if (vdef[effective_version - 1].deprecated) { + warn_report("Effective CPU model '%s' -- %s", + x86_cpu_versioned_model_name(xcc->model->cpudef,\ + effective_version), + vdef[effective_version - 1].note); + } + } +} + static void x86_cpu_common_class_init(ObjectClass *oc, void *data) { X86CPUClass *xcc = X86_CPU_CLASS(oc); @@ -7291,6 +7331,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) cc->tlb_fill = x86_cpu_tlb_fill; #endif cc->disas_set_info = x86_disas_set_info; + cc->deprecation_check = x86_cpu_deprecation_check; dc->user_creatable = true; }
Complement versioned CPU model framework with the ability of marking some versions deprecated. When that CPU model is chosen, get some warning. The warning message is customized, e.g. telling in which future QEMU version will it be obsoleted. The deprecation message will also appear by x86_cpu_list_entry(), e.g. '-cpu help'. QMP 'query-cpu-definitions' will also return a bool value indicating the deprecation status. Changes in v2: Move deprecation check from parse_cpu_option() to machine_run_board_init(), so that it can cover implicit cpu_type assignment cases. Add qapi new member documentation. Thanks Eric for comment and guidance on qapi. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- hw/core/machine.c | 11 +++++++++-- include/hw/core/cpu.h | 1 + qapi/machine-target.json | 7 ++++++- target/i386/cpu.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 59 insertions(+), 5 deletions(-)