* [PATCH v2 2/2] Mark Icelake-Client CPU models deprecated
2020-06-11 2:47 [PATCH v2 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
@ 2020-06-11 2:47 ` Robert Hoo
2020-06-28 11:30 ` [PATCH v2 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
2020-09-09 18:15 ` Eduardo Habkost
2 siblings, 0 replies; 13+ messages in thread
From: Robert Hoo @ 2020-06-11 2:47 UTC (permalink / raw)
To: pbonzini, rth, ehabkost, eblake, armbru
Cc: robert.hu, Robert Hoo, xiaoyao.li, qemu-devel, chenyi.qiang
Going to obsolete Icelake-Client CPU models in the future.
(No changes in v2)
Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
target/i386/cpu.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 0d8638a..47a11b5 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3350,7 +3350,12 @@ static X86CPUDefinition builtin_x86_defs[] = {
.xlevel = 0x80000008,
.model_id = "Intel Core Processor (Icelake)",
.versions = (X86CPUVersionDefinition[]) {
- { .version = 1 },
+ {
+ .version = 1,
+ .deprecated = true,
+ .note = "Deprecated. Will be obsoleted in v5.1. Please use "
+ "'Icelake-Server-v1' CPU model",
+ },
{
.version = 2,
.alias = "Icelake-Client-noTSX",
@@ -3359,6 +3364,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
{ "rtm", "off" },
{ /* end of list */ }
},
+ .deprecated = true,
+ .note = "Deprecated. Will be obsoleted in v5.1. Please use "
+ "'Icelake-Server-v2' CPU model",
},
{ /* end of list */ }
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-06-11 2:47 [PATCH v2 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
2020-06-11 2:47 ` [PATCH v2 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
@ 2020-06-28 11:30 ` Robert Hoo
2020-09-09 18:15 ` Eduardo Habkost
2 siblings, 0 replies; 13+ messages in thread
From: Robert Hoo @ 2020-06-28 11:30 UTC (permalink / raw)
To: pbonzini, rth, ehabkost, eblake, armbru
Cc: robert.hu, xiaoyao.li, qemu-devel, chenyi.qiang
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;
> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-06-11 2:47 [PATCH v2 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
2020-06-11 2:47 ` [PATCH v2 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
2020-06-28 11:30 ` [PATCH v2 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
@ 2020-09-09 18:15 ` Eduardo Habkost
2020-09-10 5:29 ` Philippe Mathieu-Daudé
2020-09-11 6:22 ` Robert Hoo
2 siblings, 2 replies; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-09 18:15 UTC (permalink / raw)
To: Robert Hoo
Cc: xiaoyao.li, qemu-devel, armbru, robert.hu, chenyi.qiang, pbonzini, rth
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
>
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-09 18:15 ` Eduardo Habkost
@ 2020-09-10 5:29 ` Philippe Mathieu-Daudé
2020-09-10 19:12 ` Eduardo Habkost
2020-09-11 6:22 ` Robert Hoo
1 sibling, 1 reply; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-10 5:29 UTC (permalink / raw)
To: Eduardo Habkost, Robert Hoo
Cc: armbru, xiaoyao.li, chenyi.qiang, qemu-devel, pbonzini, robert.hu, rth
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
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-10 5:29 ` Philippe Mathieu-Daudé
@ 2020-09-10 19:12 ` Eduardo Habkost
2020-09-10 19:42 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-10 19:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, xiaoyao.li, chenyi.qiang, armbru, pbonzini,
robert.hu, Robert Hoo, rth
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.
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-10 19:12 ` Eduardo Habkost
@ 2020-09-10 19:42 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 13+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-10 19:42 UTC (permalink / raw)
To: Eduardo Habkost
Cc: qemu-devel, xiaoyao.li, chenyi.qiang, armbru, pbonzini,
robert.hu, Robert Hoo, rth
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-09 18:15 ` Eduardo Habkost
2020-09-10 5:29 ` Philippe Mathieu-Daudé
@ 2020-09-11 6:22 ` Robert Hoo
2020-09-11 14:00 ` Eduardo Habkost
1 sibling, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-11 6:22 UTC (permalink / raw)
To: Eduardo Habkost
Cc: xiaoyao.li, qemu-devel, armbru, robert.hu, chenyi.qiang, pbonzini, rth
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
> >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-11 6:22 ` Robert Hoo
@ 2020-09-11 14:00 ` Eduardo Habkost
2020-09-14 10:50 ` Robert Hoo
0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-11 14:00 UTC (permalink / raw)
To: Robert Hoo
Cc: xiaoyao.li, qemu-devel, armbru, robert.hu, chenyi.qiang, pbonzini, rth
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
> > >
> >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-11 14:00 ` Eduardo Habkost
@ 2020-09-14 10:50 ` Robert Hoo
2020-09-14 13:38 ` Eduardo Habkost
0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-14 10:50 UTC (permalink / raw)
To: Eduardo Habkost
Cc: xiaoyao.li, qemu-devel, armbru, robert.hu, chenyi.qiang, pbonzini, rth
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
> > > >
> > >
> > >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-14 10:50 ` Robert Hoo
@ 2020-09-14 13:38 ` Eduardo Habkost
2020-09-15 2:56 ` Robert Hoo
0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-14 13:38 UTC (permalink / raw)
To: Robert Hoo
Cc: xiaoyao.li, qemu-devel, armbru, robert.hu, chenyi.qiang, pbonzini, rth
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.
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-14 13:38 ` Eduardo Habkost
@ 2020-09-15 2:56 ` Robert Hoo
2020-09-15 14:06 ` Eduardo Habkost
0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-15 2:56 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Li, Xiaoyao, qemu-devel, armbru, Hu, Robert, Qiang, Chenyi,
pbonzini, rth
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
2020-09-15 2:56 ` Robert Hoo
@ 2020-09-15 14:06 ` Eduardo Habkost
0 siblings, 0 replies; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-15 14:06 UTC (permalink / raw)
To: Robert Hoo
Cc: Li, Xiaoyao, qemu-devel, armbru, Hu, Robert, Qiang, Chenyi,
pbonzini, rth
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.
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread