qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
@ 2020-09-16  8:37 Robert Hoo
  2020-09-16  8:37 ` [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
  2020-09-17 18:18 ` [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Eduardo Habkost
  0 siblings, 2 replies; 13+ messages in thread
From: Robert Hoo @ 2020-09-16  8:37 UTC (permalink / raw)
  To: ehabkost, eblake, pbonzini, rth, armbru; +Cc: robert.hu, qemu-devel, Robert Hoo

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.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
Changelog
v3:
Make the deprecation implementation CPUClass generic.

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.

 hw/core/machine.c        | 12 ++++++++++--
 include/hw/core/cpu.h    |  6 ++++++
 qapi/machine-target.json |  7 ++++++-
 target/i386/cpu.c        | 29 +++++++++++++++++++++++------
 4 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index ea26d61..b41f88d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1095,6 +1095,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;
@@ -1114,11 +1116,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.
@@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState *machine)
         }
     }
 
+    /* Check if CPU type is deprecated and warn if so */
+    cc = CPU_CLASS(oc);
+    if (cc->deprecated) {
+        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
+                    cc->deprecation_note);
+    }
+
     machine_class->init(machine);
 }
 
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 99dc33f..c4b17c8 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -155,6 +155,10 @@ struct TranslationBlock;
  * @disas_set_info: Setup architecture specific components of disassembly info
  * @adjust_watchpoint_address: Perform a target-specific adjustment to an
  * address before attempting to match it against watchpoints.
+ * @deprecated: True if this CPU model is deprecated (going to be removed in
+ *              near future).
+ * @deprecation_note: Message about the deprecation. e.g. Since which version
+ *                    will it be obsoleted.
  *
  * Represents a CPU family or model.
  */
@@ -221,6 +225,8 @@ struct CPUClass {
     vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
     void (*tcg_initialize)(void);
 
+    bool deprecated;
+    const char *deprecation_note;
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 698850c..fec3bb8 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -286,6 +286,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.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
@@ -310,7 +314,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 49d8958..9cb82b7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
     const char *alias;
     const char *note;
     PropValue *props;
+    bool       deprecated;
 } X86CPUVersionDefinition;
 
 /* Base definition for a CPU model */
@@ -1751,6 +1752,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 */
@@ -5096,6 +5102,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.
@@ -5486,9 +5493,12 @@ static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
 {
     X86CPUModel *model = data;
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
+    CPUClass *cc = CPU_CLASS(oc);
 
     xcc->model = model;
     xcc->migration_safe = true;
+    cc->deprecated = model->deprecated;
+    cc->deprecation_note = g_strdup(model->note);
 }
 
 static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
@@ -5524,21 +5534,28 @@ static void x86_register_cpudef_types(X86CPUDefinition *def)
     x86_register_cpu_model_type(def->name, m);
 
     /* Versioned models: */
-
     for (vdef = x86_cpu_def_get_versions(def); vdef->version; vdef++) {
-        X86CPUModel *m = g_new0(X86CPUModel, 1);
+        X86CPUModel *vm = g_new0(X86CPUModel, 1);
         g_autofree char *name =
             x86_cpu_versioned_model_name(def, vdef->version);
-        m->cpudef = def;
-        m->version = vdef->version;
-        m->note = vdef->note;
-        x86_register_cpu_model_type(name, m);
+        vm->cpudef = def;
+        vm->version = vdef->version;
+        vm->note = vdef->note;
+        vm->deprecated = vdef->deprecated;
+        /* If Model-v1 is deprecated, Model is deprecated. */
+        if (vdef->version == 1 && vdef->deprecated == true) {
+            m->deprecated = true;
+            m->note = vdef->note;
+        }
+        x86_register_cpu_model_type(name, vm);
 
         if (vdef->alias) {
             X86CPUModel *am = g_new0(X86CPUModel, 1);
             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);
         }
     }
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-16  8:37 [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
@ 2020-09-16  8:37 ` Robert Hoo
  2020-09-17 18:01   ` Eduardo Habkost
  2020-09-17 18:18 ` [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Eduardo Habkost
  1 sibling, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-16  8:37 UTC (permalink / raw)
  To: ehabkost, eblake, pbonzini, rth, armbru; +Cc: robert.hu, qemu-devel, Robert Hoo

Going to obsolete Icelake-Client CPU models in the future.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
Change log
v3:
Obsolete in v5.2 --> v5.3.

 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 9cb82b7..15c1c00 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3467,7 +3467,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.3. Please use "
+                        "'Icelake-Server-v1' CPU model",
+            },
             {
                 .version = 2,
                 .note = "no TSX",
@@ -3477,6 +3482,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
                     { "rtm", "off" },
                     { /* end of list */ }
                 },
+                .deprecated = true,
+                .note = "Deprecated. Will be obsoleted in v5.3. 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 v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-16  8:37 ` [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
@ 2020-09-17 18:01   ` Eduardo Habkost
  2020-09-18  2:18     ` Robert Hoo
  0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-17 18:01 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Wed, Sep 16, 2020 at 04:37:14PM +0800, Robert Hoo wrote:
> Going to obsolete Icelake-Client CPU models in the future.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
> Change log
> v3:
> Obsolete in v5.2 --> v5.3.
> 
>  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 9cb82b7..15c1c00 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -3467,7 +3467,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.3. Please use "
> +                        "'Icelake-Server-v1' CPU model",

What's the difference between "deprecated" and "obsoleted"?

> +            },
>              {
>                  .version = 2,
>                  .note = "no TSX",
> @@ -3477,6 +3482,9 @@ static X86CPUDefinition builtin_x86_defs[] = {
>                      { "rtm", "off" },
>                      { /* end of list */ }
>                  },
> +                .deprecated = true,
> +                .note = "Deprecated. Will be obsoleted in v5.3. Please use "
> +                        "'Icelake-Server-v2' CPU model",
>              },
>              { /* end of list */ }
>          }
> -- 
> 1.8.3.1
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-16  8:37 [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
  2020-09-16  8:37 ` [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
@ 2020-09-17 18:18 ` Eduardo Habkost
  2020-09-18  5:48   ` Robert Hoo
  1 sibling, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-17 18:18 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

I think the patch looks better now, thanks!

Just a few minor questions and suggestions:

On Wed, Sep 16, 2020 at 04:37:13PM +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.
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
> Changelog
> v3:
> Make the deprecation implementation CPUClass generic.
> 
> v2:
> Move deprecation check from parse_cpu_option() to machine_run_board_init(), so
> that it can cover implicit cpu_type assignment cases.

What do you mean by implicit cpu_type assignment cases?

Doing it inside parse_cpu_option() like you did in v1 will make
the deprecation warnings appear for *-user too (which is a good
thing).

> Add qapi new member documentation. Thanks Eric for comment and guidance on qapi.
> 
>  hw/core/machine.c        | 12 ++++++++++--
>  include/hw/core/cpu.h    |  6 ++++++
>  qapi/machine-target.json |  7 ++++++-
>  target/i386/cpu.c        | 29 +++++++++++++++++++++++------
>  4 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index ea26d61..b41f88d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1095,6 +1095,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;
> @@ -1114,11 +1116,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.
> @@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState *machine)
>          }
>      }
>  
> +    /* Check if CPU type is deprecated and warn if so */
> +    cc = CPU_CLASS(oc);
> +    if (cc->deprecated) {
> +        warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
> +                    cc->deprecation_note);
> +    }
> +
>      machine_class->init(machine);
>  }
>  
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 99dc33f..c4b17c8 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -155,6 +155,10 @@ struct TranslationBlock;
>   * @disas_set_info: Setup architecture specific components of disassembly info
>   * @adjust_watchpoint_address: Perform a target-specific adjustment to an
>   * address before attempting to match it against watchpoints.
> + * @deprecated: True if this CPU model is deprecated (going to be removed in
> + *              near future).
> + * @deprecation_note: Message about the deprecation. e.g. Since which version
> + *                    will it be obsoleted.

We don't need two separate fields if (deprecation_note != NULL)
means the CPU model is deprecated.  This is how it was
implemented in MachineClass (see MachineClass::deprecation_reason).

>   *
>   * Represents a CPU family or model.
>   */
> @@ -221,6 +225,8 @@ struct CPUClass {
>      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
>      void (*tcg_initialize)(void);
>  
> +    bool deprecated;
> +    const char *deprecation_note;
>      /* Keep non-pointer data at the end to minimize holes.  */
>      int gdb_num_core_regs;
>      bool gdb_stop_before_watchpoint;
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 698850c..fec3bb8 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -286,6 +286,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.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
> @@ -310,7 +314,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 49d8958..9cb82b7 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
>      const char *alias;
>      const char *note;
>      PropValue *props;
> +    bool       deprecated;
>  } X86CPUVersionDefinition;
>  
>  /* Base definition for a CPU model */
> @@ -1751,6 +1752,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 */
> @@ -5096,6 +5102,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.
> @@ -5486,9 +5493,12 @@ static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>  {
>      X86CPUModel *model = data;
>      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(oc);
>  
>      xcc->model = model;
>      xcc->migration_safe = true;
> +    cc->deprecated = model->deprecated;
> +    cc->deprecation_note = g_strdup(model->note);

The meaning of cc->deprecation_note and model->note is a bit
ambiguous here.  model->note is not necessarily a deprecation
note, but its contents are being copied unconditionally to
cc->deprecation_note.

What about setting cc->deprecation_note only if and only if the
model is deprecated?


>  }
>  
>  static void x86_register_cpu_model_type(const char *name, X86CPUModel *model)
> @@ -5524,21 +5534,28 @@ static void x86_register_cpudef_types(X86CPUDefinition *def)
>      x86_register_cpu_model_type(def->name, m);
>  
>      /* Versioned models: */
> -
>      for (vdef = x86_cpu_def_get_versions(def); vdef->version; vdef++) {
> -        X86CPUModel *m = g_new0(X86CPUModel, 1);
> +        X86CPUModel *vm = g_new0(X86CPUModel, 1);
>          g_autofree char *name =
>              x86_cpu_versioned_model_name(def, vdef->version);
> -        m->cpudef = def;
> -        m->version = vdef->version;
> -        m->note = vdef->note;
> -        x86_register_cpu_model_type(name, m);
> +        vm->cpudef = def;
> +        vm->version = vdef->version;
> +        vm->note = vdef->note;
> +        vm->deprecated = vdef->deprecated;
> +        /* If Model-v1 is deprecated, Model is deprecated. */
> +        if (vdef->version == 1 && vdef->deprecated == true) {
> +            m->deprecated = true;
> +            m->note = vdef->note;

I'm not sure this rule will always apply.  If we deprecate
"Model-v1" but not "Model-v2", we probably don't want "Model" to
be deprecated too.

Considering that we don't have cases where specific versions are
being deprecated, what about making it as simple as possible and
just add a new X86CPUDefinition::deprecation_note field?  No need
for any new fields in X86CPUModel (model->cpudef->deprecation_note
can be used directly), no need for two new fields in CPUClass
(just deprecation_note).

If one day we decide to deprecate just some versions of a CPU
model, we can extend X86CPUVersionDefinition in the future.

> +        }
> +        x86_register_cpu_model_type(name, vm);
>  
>          if (vdef->alias) {
>              X86CPUModel *am = g_new0(X86CPUModel, 1);
>              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);
>          }
>      }
> -- 
> 1.8.3.1
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-17 18:01   ` Eduardo Habkost
@ 2020-09-18  2:18     ` Robert Hoo
  2020-09-18  4:20       ` Eduardo Habkost
  0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-18  2:18 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Thu, 2020-09-17 at 14:01 -0400, Eduardo Habkost wrote:
> On Wed, Sep 16, 2020 at 04:37:14PM +0800, Robert Hoo wrote:
> > Going to obsolete Icelake-Client CPU models in the future.
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> > Change log
> > v3:
> > Obsolete in v5.2 --> v5.3.
> > 
> >  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 9cb82b7..15c1c00 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -3467,7 +3467,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.3.
> > Please use "
> > +                        "'Icelake-Server-v1' CPU model",
> 
> What's the difference between "deprecated" and "obsoleted"?
> 
Forgive my non-native understanding on English word:-D
Here is my understanding:
'Deprecate' is to express strong disapproval on the usage; but, can
still be used if user insists.
'Obsolete' means not usable anymore.

You can feel free to reword the note words.
Perhaps substitute 'removed' for 'obsolete' will be better.

> > +            },
> >              {
> >                  .version = 2,
> >                  .note = "no TSX",
> > @@ -3477,6 +3482,9 @@ static X86CPUDefinition builtin_x86_defs[] =
> > {
> >                      { "rtm", "off" },
> >                      { /* end of list */ }
> >                  },
> > +                .deprecated = true,
> > +                .note = "Deprecated. Will be obsoleted in v5.3.
> > Please use "
> > +                        "'Icelake-Server-v2' CPU model",
> >              },
> >              { /* end of list */ }
> >          }
> > -- 
> > 1.8.3.1
> > 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-18  2:18     ` Robert Hoo
@ 2020-09-18  4:20       ` Eduardo Habkost
  2020-09-21  7:45         ` Robert Hoo
  0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-18  4:20 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Fri, Sep 18, 2020 at 10:18:56AM +0800, Robert Hoo wrote:
> On Thu, 2020-09-17 at 14:01 -0400, Eduardo Habkost wrote:
> > On Wed, Sep 16, 2020 at 04:37:14PM +0800, Robert Hoo wrote:
> > > Going to obsolete Icelake-Client CPU models in the future.
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > ---
> > > Change log
> > > v3:
> > > Obsolete in v5.2 --> v5.3.
> > > 
> > >  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 9cb82b7..15c1c00 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -3467,7 +3467,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.3.
> > > Please use "
> > > +                        "'Icelake-Server-v1' CPU model",
> > 
> > What's the difference between "deprecated" and "obsoleted"?
> > 
> Forgive my non-native understanding on English word:-D

No problem!  I'm not a native speaker either.  :-)

> Here is my understanding:
> 'Deprecate' is to express strong disapproval on the usage; but, can
> still be used if user insists.
> 'Obsolete' means not usable anymore.
> 
> You can feel free to reword the note words.
> Perhaps substitute 'removed' for 'obsolete' will be better.

"Removed" would be clearer, yes.  It's probably better to not
mention the exact version, and just say it will be removed in
the future.

Or maybe just make the message shorter and set deprecation_note
to "Please use Icelake-Server instead".  The details can be
documented in docs/system/deprecated.rst.

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-17 18:18 ` [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Eduardo Habkost
@ 2020-09-18  5:48   ` Robert Hoo
  2020-09-18 16:42     ` Eduardo Habkost
  0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-18  5:48 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Thu, 2020-09-17 at 14:18 -0400, Eduardo Habkost wrote:
> I think the patch looks better now, thanks!
> 
> Just a few minor questions and suggestions:
> 
> On Wed, Sep 16, 2020 at 04:37:13PM +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.
> > 
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> > Changelog
> > v3:
> > Make the deprecation implementation CPUClass generic.
> > 
> > v2:
> > Move deprecation check from parse_cpu_option() to
> > machine_run_board_init(), so
> > that it can cover implicit cpu_type assignment cases.
> 
> What do you mean by implicit cpu_type assignment cases?

Means the case that user doesn't explicitly assign '-cpu xxx' but
implicitly inherits machine's default cpu type.
vl.c
    /* parse features once if machine provides default cpu_type */
    current_machine->cpu_type = machine_class->default_cpu_type;
    if (cpu_option) {
        current_machine->cpu_type = parse_cpu_option(cpu_option);
    }
> 
> Doing it inside parse_cpu_option() like you did in v1 will make
> the deprecation warnings appear for *-user too (which is a good
> thing).
> 
So you changed your mind;-)

Could you shed more details? I don't get this point. What do you mean 
"*-user"?

> > Add qapi new member documentation. Thanks Eric for comment and
> > guidance on qapi.
> > 
> >  hw/core/machine.c        | 12 ++++++++++--
> >  include/hw/core/cpu.h    |  6 ++++++
> >  qapi/machine-target.json |  7 ++++++-
> >  target/i386/cpu.c        | 29 +++++++++++++++++++++++------
> >  4 files changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index ea26d61..b41f88d 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -1095,6 +1095,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;
> > @@ -1114,11 +1116,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.
> > @@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState
> > *machine)
> >          }
> >      }
> >  
> > +    /* Check if CPU type is deprecated and warn if so */
> > +    cc = CPU_CLASS(oc);
> > +    if (cc->deprecated) {
> > +        warn_report("CPU model %s is deprecated -- %s", machine-
> > >cpu_type,
> > +                    cc->deprecation_note);
> > +    }
> > +
> >      machine_class->init(machine);
> >  }
> >  
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 99dc33f..c4b17c8 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -155,6 +155,10 @@ struct TranslationBlock;
> >   * @disas_set_info: Setup architecture specific components of
> > disassembly info
> >   * @adjust_watchpoint_address: Perform a target-specific
> > adjustment to an
> >   * address before attempting to match it against watchpoints.
> > + * @deprecated: True if this CPU model is deprecated (going to be
> > removed in
> > + *              near future).
> > + * @deprecation_note: Message about the deprecation. e.g. Since
> > which version
> > + *                    will it be obsoleted.
> 
> We don't need two separate fields if (deprecation_note != NULL)
> means the CPU model is deprecated.  This is how it was
> implemented in MachineClass (see MachineClass::deprecation_reason).
> 

Agree.
I think such applies to X86CPUModel::deprecated and X86CPUModel::note
as well; and rename X86CPUModel::note --> deprecation_note. How do you
like this?

> >   *
> >   * Represents a CPU family or model.
> >   */
> > @@ -221,6 +225,8 @@ struct CPUClass {
> >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr,
> > int len);
> >      void (*tcg_initialize)(void);
> >  
> > +    bool deprecated;
> > +    const char *deprecation_note;
> >      /* Keep non-pointer data at the end to minimize holes.  */
> >      int gdb_num_core_regs;
> >      bool gdb_stop_before_watchpoint;
> > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > index 698850c..fec3bb8 100644
> > --- a/qapi/machine-target.json
> > +++ b/qapi/machine-target.json
> > @@ -286,6 +286,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.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
> > @@ -310,7 +314,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 49d8958..9cb82b7 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
> >      const char *alias;
> >      const char *note;
> >      PropValue *props;
> > +    bool       deprecated;
> >  } X86CPUVersionDefinition;
> >  
> >  /* Base definition for a CPU model */
> > @@ -1751,6 +1752,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 */
> > @@ -5096,6 +5102,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.
> > @@ -5486,9 +5493,12 @@ static void
> > x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> >  {
> >      X86CPUModel *model = data;
> >      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > +    CPUClass *cc = CPU_CLASS(oc);
> >  
> >      xcc->model = model;
> >      xcc->migration_safe = true;
> > +    cc->deprecated = model->deprecated;
> > +    cc->deprecation_note = g_strdup(model->note);
> 
> The meaning of cc->deprecation_note and model->note is a bit
> ambiguous here.  model->note is not necessarily a deprecation
> note, but its contents are being copied unconditionally to
> cc->deprecation_note.
> 
> What about setting cc->deprecation_note only if and only if the
> model is deprecated?
> 
Agree. Since X86CPUModel::note is actually unused by anything now,
going to rename it to deprecation_note as aforementioned.

A side concern,
cc->deprecation_note = g_strdup(model->note);

I don's see where free the cc object. Assuming the dup'ed string, as
well as object, will last through QEMU process life time, freed
implicitly as this process gone. Is my understanding right?
> 
> >  }
> >  
> >  static void x86_register_cpu_model_type(const char *name,
> > X86CPUModel *model)
> > @@ -5524,21 +5534,28 @@ static void
> > x86_register_cpudef_types(X86CPUDefinition *def)
> >      x86_register_cpu_model_type(def->name, m);
> >  
> >      /* Versioned models: */
> > -
> >      for (vdef = x86_cpu_def_get_versions(def); vdef->version;
> > vdef++) {
> > -        X86CPUModel *m = g_new0(X86CPUModel, 1);
> > +        X86CPUModel *vm = g_new0(X86CPUModel, 1);
> >          g_autofree char *name =
> >              x86_cpu_versioned_model_name(def, vdef->version);
> > -        m->cpudef = def;
> > -        m->version = vdef->version;
> > -        m->note = vdef->note;
> > -        x86_register_cpu_model_type(name, m);
> > +        vm->cpudef = def;
> > +        vm->version = vdef->version;
> > +        vm->note = vdef->note;
> > +        vm->deprecated = vdef->deprecated;
> > +        /* If Model-v1 is deprecated, Model is deprecated. */
> > +        if (vdef->version == 1 && vdef->deprecated == true) {
> > +            m->deprecated = true;
> > +            m->note = vdef->note;
> 
> I'm not sure this rule will always apply.  If we deprecate
> "Model-v1" but not "Model-v2", we probably don't want "Model" to
> be deprecated too.

Agree
> 
> Considering that we don't have cases where specific versions are
> being deprecated, what about making it as simple as possible and
> just add a new X86CPUDefinition::deprecation_note field?  No need
> for any new fields in X86CPUModel (model->cpudef->deprecation_note
> can be used directly), no need for two new fields in CPUClass
> (just deprecation_note).

How about eliminating the unversioned CPU model? Then we can still have
deprecation_note in X86CPUModel, which looks more natural to me than in
X86CPUDefition.
For anyway, the unversioned CPU model will eventually be instantiated
to its some versioned CPU model. It's like a virtual class.

> 
> If one day we decide to deprecate just some versions of a CPU
> model, we can extend X86CPUVersionDefinition in the future.
> 
> > +        }
> > +        x86_register_cpu_model_type(name, vm);
> >  
> >          if (vdef->alias) {
> >              X86CPUModel *am = g_new0(X86CPUModel, 1);
> >              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);
> >          }
> >      }
> > -- 
> > 1.8.3.1
> > 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-18  5:48   ` Robert Hoo
@ 2020-09-18 16:42     ` Eduardo Habkost
  2020-09-19  3:22       ` Robert Hoo
  0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-18 16:42 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Fri, Sep 18, 2020 at 01:48:18PM +0800, Robert Hoo wrote:
> On Thu, 2020-09-17 at 14:18 -0400, Eduardo Habkost wrote:
> > I think the patch looks better now, thanks!
> > 
> > Just a few minor questions and suggestions:
> > 
> > On Wed, Sep 16, 2020 at 04:37:13PM +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.
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > ---
> > > Changelog
> > > v3:
> > > Make the deprecation implementation CPUClass generic.
> > > 
> > > v2:
> > > Move deprecation check from parse_cpu_option() to
> > > machine_run_board_init(), so
> > > that it can cover implicit cpu_type assignment cases.
> > 
> > What do you mean by implicit cpu_type assignment cases?
> 
> Means the case that user doesn't explicitly assign '-cpu xxx' but
> implicitly inherits machine's default cpu type.
> vl.c
>     /* parse features once if machine provides default cpu_type */
>     current_machine->cpu_type = machine_class->default_cpu_type;
>     if (cpu_option) {
>         current_machine->cpu_type = parse_cpu_option(cpu_option);
>     }

We probably would never deprecate CPU models that are still used
by default, so this is not an issue.

> > 
> > Doing it inside parse_cpu_option() like you did in v1 will make
> > the deprecation warnings appear for *-user too (which is a good
> > thing).
> > 
> So you changed your mind;-)

Sorry, I don't remember suggesting this.  I couldn't find any
reply from myself in v1, and v2 already had the code moved to
machine_run_board_init().

Note that doing the check in machine_run_board_init() is not
necessarily a problem, I'm just trying to understand why the code
was moved from parse_cpu_option() to machine_run_board_init()
between v1 and v2.


> 
> Could you shed more details? I don't get this point. What do you mean 
> "*-user"?

I mean QEMU user mode emulator, bsd-user and linux-user (e.g. the
qemu-x86_64 binary).  They call parse_cpu_option() as well.

> 
> > > Add qapi new member documentation. Thanks Eric for comment and
> > > guidance on qapi.
> > > 
> > >  hw/core/machine.c        | 12 ++++++++++--
> > >  include/hw/core/cpu.h    |  6 ++++++
> > >  qapi/machine-target.json |  7 ++++++-
> > >  target/i386/cpu.c        | 29 +++++++++++++++++++++++------
> > >  4 files changed, 45 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index ea26d61..b41f88d 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -1095,6 +1095,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;
> > > @@ -1114,11 +1116,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.
> > > @@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState
> > > *machine)
> > >          }
> > >      }
> > >  
> > > +    /* Check if CPU type is deprecated and warn if so */
> > > +    cc = CPU_CLASS(oc);
> > > +    if (cc->deprecated) {
> > > +        warn_report("CPU model %s is deprecated -- %s", machine-
> > > >cpu_type,
> > > +                    cc->deprecation_note);
> > > +    }
> > > +
> > >      machine_class->init(machine);
> > >  }
> > >  
> > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > index 99dc33f..c4b17c8 100644
> > > --- a/include/hw/core/cpu.h
> > > +++ b/include/hw/core/cpu.h
> > > @@ -155,6 +155,10 @@ struct TranslationBlock;
> > >   * @disas_set_info: Setup architecture specific components of
> > > disassembly info
> > >   * @adjust_watchpoint_address: Perform a target-specific
> > > adjustment to an
> > >   * address before attempting to match it against watchpoints.
> > > + * @deprecated: True if this CPU model is deprecated (going to be
> > > removed in
> > > + *              near future).
> > > + * @deprecation_note: Message about the deprecation. e.g. Since
> > > which version
> > > + *                    will it be obsoleted.
> > 
> > We don't need two separate fields if (deprecation_note != NULL)
> > means the CPU model is deprecated.  This is how it was
> > implemented in MachineClass (see MachineClass::deprecation_reason).
> > 
> 
> Agree.
> I think such applies to X86CPUModel::deprecated and X86CPUModel::note
> as well; and rename X86CPUModel::note --> deprecation_note. How do you
> like this?

Sound good!

> 
> > >   *
> > >   * Represents a CPU family or model.
> > >   */
> > > @@ -221,6 +225,8 @@ struct CPUClass {
> > >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr,
> > > int len);
> > >      void (*tcg_initialize)(void);
> > >  
> > > +    bool deprecated;
> > > +    const char *deprecation_note;
> > >      /* Keep non-pointer data at the end to minimize holes.  */
> > >      int gdb_num_core_regs;
> > >      bool gdb_stop_before_watchpoint;
> > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> > > index 698850c..fec3bb8 100644
> > > --- a/qapi/machine-target.json
> > > +++ b/qapi/machine-target.json
> > > @@ -286,6 +286,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.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
> > > @@ -310,7 +314,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 49d8958..9cb82b7 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
> > >      const char *alias;
> > >      const char *note;
> > >      PropValue *props;
> > > +    bool       deprecated;
> > >  } X86CPUVersionDefinition;
> > >  
> > >  /* Base definition for a CPU model */
> > > @@ -1751,6 +1752,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 */
> > > @@ -5096,6 +5102,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.
> > > @@ -5486,9 +5493,12 @@ static void
> > > x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> > >  {
> > >      X86CPUModel *model = data;
> > >      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > +    CPUClass *cc = CPU_CLASS(oc);
> > >  
> > >      xcc->model = model;
> > >      xcc->migration_safe = true;
> > > +    cc->deprecated = model->deprecated;
> > > +    cc->deprecation_note = g_strdup(model->note);
> > 
> > The meaning of cc->deprecation_note and model->note is a bit
> > ambiguous here.  model->note is not necessarily a deprecation
> > note, but its contents are being copied unconditionally to
> > cc->deprecation_note.
> > 
> > What about setting cc->deprecation_note only if and only if the
> > model is deprecated?
> > 
> Agree. Since X86CPUModel::note is actually unused by anything now,
> going to rename it to deprecation_note as aforementioned.

.note is used by all the CPU models that set .note.  See for
example:

x86 Cascadelake-Server-v1  Intel Xeon Processor (Cascadelake)
x86 Cascadelake-Server-v2  Intel Xeon Processor (Cascadelake) [ARCH_CAPABILITIES]
x86 Cascadelake-Server-v3  Intel Xeon Processor (Cascadelake) [ARCH_CAPABILITIES, no TSX]
x86 Cascadelake-Server-v4  Intel Xeon Processor (Cascadelake) [ARCH_CAPABILITIES, no TSX]


> 
> A side concern,
> cc->deprecation_note = g_strdup(model->note);
> 
> I don's see where free the cc object. Assuming the dup'ed string, as
> well as object, will last through QEMU process life time, freed
> implicitly as this process gone. Is my understanding right?

We never free QOM class structs (like CPUClass), so that's OK.

> > 
> > >  }
> > >  
> > >  static void x86_register_cpu_model_type(const char *name,
> > > X86CPUModel *model)
> > > @@ -5524,21 +5534,28 @@ static void
> > > x86_register_cpudef_types(X86CPUDefinition *def)
> > >      x86_register_cpu_model_type(def->name, m);
> > >  
> > >      /* Versioned models: */
> > > -
> > >      for (vdef = x86_cpu_def_get_versions(def); vdef->version;
> > > vdef++) {
> > > -        X86CPUModel *m = g_new0(X86CPUModel, 1);
> > > +        X86CPUModel *vm = g_new0(X86CPUModel, 1);
> > >          g_autofree char *name =
> > >              x86_cpu_versioned_model_name(def, vdef->version);
> > > -        m->cpudef = def;
> > > -        m->version = vdef->version;
> > > -        m->note = vdef->note;
> > > -        x86_register_cpu_model_type(name, m);
> > > +        vm->cpudef = def;
> > > +        vm->version = vdef->version;
> > > +        vm->note = vdef->note;
> > > +        vm->deprecated = vdef->deprecated;
> > > +        /* If Model-v1 is deprecated, Model is deprecated. */
> > > +        if (vdef->version == 1 && vdef->deprecated == true) {
> > > +            m->deprecated = true;
> > > +            m->note = vdef->note;
> > 
> > I'm not sure this rule will always apply.  If we deprecate
> > "Model-v1" but not "Model-v2", we probably don't want "Model" to
> > be deprecated too.
> 
> Agree
> > 
> > Considering that we don't have cases where specific versions are
> > being deprecated, what about making it as simple as possible and
> > just add a new X86CPUDefinition::deprecation_note field?  No need
> > for any new fields in X86CPUModel (model->cpudef->deprecation_note
> > can be used directly), no need for two new fields in CPUClass
> > (just deprecation_note).
> 
> How about eliminating the unversioned CPU model? Then we can still have
> deprecation_note in X86CPUModel, which looks more natural to me than in
> X86CPUDefition.
> For anyway, the unversioned CPU model will eventually be instantiated
> to its some versioned CPU model. It's like a virtual class.

What do you mean by eliminating the unversioned CPU model?  Keep
in mind that we need "-cpu Model" to keep working, because of
compatibility and also because "-cpu Model" is more convenient.

There are ways the representation of CPU models + aliases +
versions can be improved, but I assume you wouldn't want a large
refactor of the CPU model/version table to get in the way of a
simple feature.

> 
> > 
> > If one day we decide to deprecate just some versions of a CPU
> > model, we can extend X86CPUVersionDefinition in the future.
> > 
> > > +        }
> > > +        x86_register_cpu_model_type(name, vm);
> > >  
> > >          if (vdef->alias) {
> > >              X86CPUModel *am = g_new0(X86CPUModel, 1);
> > >              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);
> > >          }
> > >      }
> > > -- 
> > > 1.8.3.1
> > > 
> > 
> > 
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-18 16:42     ` Eduardo Habkost
@ 2020-09-19  3:22       ` Robert Hoo
  2020-09-21 15:37         ` Eduardo Habkost
  0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-19  3:22 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, armbru, Hu, Robert, pbonzini, rth

On Fri, 2020-09-18 at 16:42 +0000, Eduardo Habkost wrote:
> ...
> > > > ---
> > > > Changelog
> > > > v3:
> > > > Make the deprecation implementation CPUClass generic.
> > > > 
> > > > v2:
> > > > Move deprecation check from parse_cpu_option() to
> > > > machine_run_board_init(), so
> > > > that it can cover implicit cpu_type assignment cases.
> > > 
> > > What do you mean by implicit cpu_type assignment cases?
> > 
> > Means the case that user doesn't explicitly assign '-cpu xxx' but
> > implicitly inherits machine's default cpu type.
> > vl.c
> >     /* parse features once if machine provides default cpu_type */
> >     current_machine->cpu_type = machine_class->default_cpu_type;
> >     if (cpu_option) {
> >         current_machine->cpu_type = parse_cpu_option(cpu_option);
> >     }
> 
> We probably would never deprecate CPU models that are still used
> by default, so this is not an issue.

Understand.
Even though less possible, I think it is still doable, say, firstly
switch the default model to the other one, then deprecate it.
> 
> > > 
> > > Doing it inside parse_cpu_option() like you did in v1 will make
> > > the deprecation warnings appear for *-user too (which is a good
> > > thing).
> > > 
> > 
> > So you changed your mind;-)
> 
> Sorry, I don't remember suggesting this.  I couldn't find any
> reply from myself in v1, and v2 already had the code moved to
> machine_run_board_init().
> 
> Note that doing the check in machine_run_board_init() is not
> necessarily a problem, I'm just trying to understand why the code
> was moved from parse_cpu_option() to machine_run_board_init()
> between v1 and v2.
> 
Sorry, I just searched my Linux mailbox, cannot find previous thread
either. But in my memory, it was your suggestion:) and I prefer this
than doing it in parse_cpu_option() as well, as parse_cpu_option() is
early time, the passed in cpu_model could be not concrete yet. See my
below idea of making unversioned cpu_model virtual.
> > 
> > Could you shed more details? I don't get this point. What do you
> > mean
> > "*-user"?
> 
> I mean QEMU user mode emulator, bsd-user and linux-user (e.g. the
> qemu-x86_64 binary).  They call parse_cpu_option() as well.
> 
Oh, I'm not familiar with these user modes yet. How about leave them
alone at this moment? let's cover most cases, i.e. based on machine
type, first. Then in the future we consider how to expand this to *-
user cases.
> > 
> > > > Add qapi new member documentation. Thanks Eric for comment and
> > > > guidance on qapi.
> > > > 
> > > >  hw/core/machine.c        | 12 ++++++++++--
> > > >  include/hw/core/cpu.h    |  6 ++++++
> > > >  qapi/machine-target.json |  7 ++++++-
> > > >  target/i386/cpu.c        | 29 +++++++++++++++++++++++------
> > > >  4 files changed, 45 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index ea26d61..b41f88d 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -1095,6 +1095,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;
> > > > @@ -1114,11 +1116,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.
> > > > @@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState
> > > > *machine)
> > > >          }
> > > >      }
> > > > 
> > > > +    /* Check if CPU type is deprecated and warn if so */
> > > > +    cc = CPU_CLASS(oc);
> > > > +    if (cc->deprecated) {
> > > > +        warn_report("CPU model %s is deprecated -- %s",
> > > > machine-
> > > > > cpu_type,
> > > > 
> > > > +                    cc->deprecation_note);
> > > > +    }
> > > > +
> > > >      machine_class->init(machine);
> > > >  }
> > > > 
> > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > > index 99dc33f..c4b17c8 100644
> > > > --- a/include/hw/core/cpu.h
> > > > +++ b/include/hw/core/cpu.h
> > > > @@ -155,6 +155,10 @@ struct TranslationBlock;
> > > >   * @disas_set_info: Setup architecture specific components of
> > > > disassembly info
> > > >   * @adjust_watchpoint_address: Perform a target-specific
> > > > adjustment to an
> > > >   * address before attempting to match it against watchpoints.
> > > > + * @deprecated: True if this CPU model is deprecated (going to
> > > > be
> > > > removed in
> > > > + *              near future).
> > > > + * @deprecation_note: Message about the deprecation. e.g.
> > > > Since
> > > > which version
> > > > + *                    will it be obsoleted.
> > > 
> > > We don't need two separate fields if (deprecation_note != NULL)
> > > means the CPU model is deprecated.  This is how it was
> > > implemented in MachineClass (see
> > > MachineClass::deprecation_reason).
> > > 
> > 
> > Agree.
> > I think such applies to X86CPUModel::deprecated and
> > X86CPUModel::note
> > as well; and rename X86CPUModel::note --> deprecation_note. How do
> > you
> > like this?
> 
> Sound good!
> 
> > 
> > > >   *
> > > >   * Represents a CPU family or model.
> > > >   */
> > > > @@ -221,6 +225,8 @@ struct CPUClass {
> > > >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr
> > > > addr,
> > > > int len);
> > > >      void (*tcg_initialize)(void);
> > > > 
> > > > +    bool deprecated;
> > > > +    const char *deprecation_note;
> > > >      /* Keep non-pointer data at the end to minimize holes.  */
> > > >      int gdb_num_core_regs;
> > > >      bool gdb_stop_before_watchpoint;
> > > > diff --git a/qapi/machine-target.json b/qapi/machine-
> > > > target.json
> > > > index 698850c..fec3bb8 100644
> > > > --- a/qapi/machine-target.json
> > > > +++ b/qapi/machine-target.json
> > > > @@ -286,6 +286,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.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
> > > > @@ -310,7 +314,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 49d8958..9cb82b7 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
> > > >      const char *alias;
> > > >      const char *note;
> > > >      PropValue *props;
> > > > +    bool       deprecated;
> > > >  } X86CPUVersionDefinition;
> > > > 
> > > >  /* Base definition for a CPU model */
> > > > @@ -1751,6 +1752,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 */
> > > > @@ -5096,6 +5102,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.
> > > > @@ -5486,9 +5493,12 @@ static void
> > > > x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> > > >  {
> > > >      X86CPUModel *model = data;
> > > >      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > +    CPUClass *cc = CPU_CLASS(oc);
> > > > 
> > > >      xcc->model = model;
> > > >      xcc->migration_safe = true;
> > > > +    cc->deprecated = model->deprecated;
> > > > +    cc->deprecation_note = g_strdup(model->note);
> > > 
> > > The meaning of cc->deprecation_note and model->note is a bit
> > > ambiguous here.  model->note is not necessarily a deprecation
> > > note, but its contents are being copied unconditionally to
> > > cc->deprecation_note.
> > > 
> > > What about setting cc->deprecation_note only if and only if the
> > > model is deprecated?
> > > 
> > 
> > Agree. Since X86CPUModel::note is actually unused by anything now,
> > going to rename it to deprecation_note as aforementioned.
> 
> .note is used by all the CPU models that set .note.  See for
> example:
> 
> x86 Cascadelake-Server-v1  Intel Xeon Processor (Cascadelake)
> x86 Cascadelake-Server-v2  Intel Xeon Processor (Cascadelake)
> [ARCH_CAPABILITIES]
> x86 Cascadelake-Server-v3  Intel Xeon Processor (Cascadelake)
> [ARCH_CAPABILITIES, no TSX]
> x86 Cascadelake-Server-v4  Intel Xeon Processor (Cascadelake)
> [ARCH_CAPABILITIES, no TSX]
> 
OK, then I'll add another deprecation_note.
> 
> > 
> > A side concern,
> > cc->deprecation_note = g_strdup(model->note);
> > 
> > I don's see where free the cc object. Assuming the dup'ed string,
> > as
> > well as object, will last through QEMU process life time, freed
> > implicitly as this process gone. Is my understanding right?
> 
> We never free QOM class structs (like CPUClass), so that's OK.
> 
> > > 
> > > >  }
> > > > 
> > > >  static void x86_register_cpu_model_type(const char *name,
> > > > X86CPUModel *model)
> > > > @@ -5524,21 +5534,28 @@ static void
> > > > x86_register_cpudef_types(X86CPUDefinition *def)
> > > >      x86_register_cpu_model_type(def->name, m);
> > > > 
> > > >      /* Versioned models: */
> > > > -
> > > >      for (vdef = x86_cpu_def_get_versions(def); vdef->version;
> > > > vdef++) {
> > > > -        X86CPUModel *m = g_new0(X86CPUModel, 1);
> > > > +        X86CPUModel *vm = g_new0(X86CPUModel, 1);
> > > >          g_autofree char *name =
> > > >              x86_cpu_versioned_model_name(def, vdef->version);
> > > > -        m->cpudef = def;
> > > > -        m->version = vdef->version;
> > > > -        m->note = vdef->note;
> > > > -        x86_register_cpu_model_type(name, m);
> > > > +        vm->cpudef = def;
> > > > +        vm->version = vdef->version;
> > > > +        vm->note = vdef->note;
> > > > +        vm->deprecated = vdef->deprecated;
> > > > +        /* If Model-v1 is deprecated, Model is deprecated. */
> > > > +        if (vdef->version == 1 && vdef->deprecated == true) {
> > > > +            m->deprecated = true;
> > > > +            m->note = vdef->note;
> > > 
> > > I'm not sure this rule will always apply.  If we deprecate
> > > "Model-v1" but not "Model-v2", we probably don't want "Model" to
> > > be deprecated too.
> > 
> > Agree
> > > 
> > > Considering that we don't have cases where specific versions are
> > > being deprecated, what about making it as simple as possible and
> > > just add a new X86CPUDefinition::deprecation_note field?  No need
> > > for any new fields in X86CPUModel (model->cpudef-
> > > >deprecation_note
> > > can be used directly), no need for two new fields in CPUClass
> > > (just deprecation_note).
> > 
> > How about eliminating the unversioned CPU model? Then we can still
> > have
> > deprecation_note in X86CPUModel, which looks more natural to me
> > than in
> > X86CPUDefition.
> > For anyway, the unversioned CPU model will eventually be
> > instantiated
> > to its some versioned CPU model. It's like a virtual class.
> 
> What do you mean by eliminating the unversioned CPU model?  Keep
> in mind that we need "-cpu Model" to keep working, because of
> compatibility and also because "-cpu Model" is more convenient.
> 
Yes, keeping "-cpu Model" usage as before is my thought as well. I mean
making it virtual, no real CPU model associate with it, but parse it to
some concrete version when CPUModel initializes.

> There are ways the representation of CPU models + aliases +
> versions can be improved, but I assume you wouldn't want a large
> refactor of the CPU model/version table to get in the way of a
> simple feature.
> 
Yes. Trying as less refactor as possible. I think my changes even
cannot be called refactor at all. :)
My idea is to make unversioned CPU model virtual. I did some patch,
doable:
1) in x86_register_cpudef_types(), don't register cpu_model type for
the unversioned 'Model'.
2) in x86_cpu_class_by_name(), check passed-in cpu_model param
versioned or not, if the virtual unversioned 'Model', parse it to some
concrete version by global default_cpu_version designation.

So, user can still use '-cpu Model' as before, no interface changes.
And no nature changes:
1) in current code, even legacy 'Model', you have created a v1 model
for it. i.e., every virtual 'Model' already has at least one concrete
versioned one.
2) in current code, x86_cpu_model_resolve_version() is called when
x86_cpu_load_model(). x86_cpu_model_resolve_version() actually does
kind of work that concreting unversioned Model to its versioned one, by
global default_cpu_version designation. Same as my aforementioned
above.

Would you like me to integrate this implementation in v4? to have a
look.
> > 
> > > 
> > > If one day we decide to deprecate just some versions of a CPU
> > > model, we can extend X86CPUVersionDefinition in the future.
> > > 
> > > > +        }
> > > > +        x86_register_cpu_model_type(name, vm);
> > > > 
> > > >          if (vdef->alias) {
> > > >              X86CPUModel *am = g_new0(X86CPUModel, 1);
> > > >              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);
> > > >          }
> > > >      }
> > > > --
> > > > 1.8.3.1
> > > > 
> > > 
> > > 
> 
> --
> Eduardo
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-18  4:20       ` Eduardo Habkost
@ 2020-09-21  7:45         ` Robert Hoo
  2020-09-21 15:25           ` Eduardo Habkost
  0 siblings, 1 reply; 13+ messages in thread
From: Robert Hoo @ 2020-09-21  7:45 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Fri, 2020-09-18 at 00:20 -0400, Eduardo Habkost wrote:
> On Fri, Sep 18, 2020 at 10:18:56AM +0800, Robert Hoo wrote:
> > On Thu, 2020-09-17 at 14:01 -0400, Eduardo Habkost wrote:
> > > On Wed, Sep 16, 2020 at 04:37:14PM +0800, Robert Hoo wrote:
> > > > Going to obsolete Icelake-Client CPU models in the future.
> > > > 
> > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > > ---
> > > > Change log
> > > > v3:
> > > > Obsolete in v5.2 --> v5.3.
> > > > 
> > > >  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 9cb82b7..15c1c00 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -3467,7 +3467,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.3.
> > > > Please use "
> > > > +                        "'Icelake-Server-v1' CPU model",
> > > 
> > > What's the difference between "deprecated" and "obsoleted"?
> > > 
> > 
> > Forgive my non-native understanding on English word:-D
> 
> No problem!  I'm not a native speaker either.  :-)
> 
> > Here is my understanding:
> > 'Deprecate' is to express strong disapproval on the usage; but, can
> > still be used if user insists.
> > 'Obsolete' means not usable anymore.
> > 
> > You can feel free to reword the note words.
> > Perhaps substitute 'removed' for 'obsolete' will be better.
> 
> "Removed" would be clearer, yes.  It's probably better to not
> mention the exact version, and just say it will be removed in
> the future.

Then I would tend to agree with your suggestion of no specific
'deprecation_note' at all; instead, a general warning message "will be
removed in the future" in machine_run_board_init().
> 
> Or maybe just make the message shorter and set deprecation_note
> to "Please use Icelake-Server instead".  The details can be
> documented in docs/system/deprecated.rst.
> 
Prefer documenting detail and model specific deprecation plan in
docs/system/deprecated.rst.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated
  2020-09-21  7:45         ` Robert Hoo
@ 2020-09-21 15:25           ` Eduardo Habkost
  0 siblings, 0 replies; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-21 15:25 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, robert.hu, pbonzini, rth

On Mon, Sep 21, 2020 at 03:45:12PM +0800, Robert Hoo wrote:
> On Fri, 2020-09-18 at 00:20 -0400, Eduardo Habkost wrote:
> > On Fri, Sep 18, 2020 at 10:18:56AM +0800, Robert Hoo wrote:
> > > On Thu, 2020-09-17 at 14:01 -0400, Eduardo Habkost wrote:
> > > > On Wed, Sep 16, 2020 at 04:37:14PM +0800, Robert Hoo wrote:
> > > > > Going to obsolete Icelake-Client CPU models in the future.
> > > > > 
> > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > > > ---
> > > > > Change log
> > > > > v3:
> > > > > Obsolete in v5.2 --> v5.3.
> > > > > 
> > > > >  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 9cb82b7..15c1c00 100644
> > > > > --- a/target/i386/cpu.c
> > > > > +++ b/target/i386/cpu.c
> > > > > @@ -3467,7 +3467,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.3.
> > > > > Please use "
> > > > > +                        "'Icelake-Server-v1' CPU model",
> > > > 
> > > > What's the difference between "deprecated" and "obsoleted"?
> > > > 
> > > 
> > > Forgive my non-native understanding on English word:-D
> > 
> > No problem!  I'm not a native speaker either.  :-)
> > 
> > > Here is my understanding:
> > > 'Deprecate' is to express strong disapproval on the usage; but, can
> > > still be used if user insists.
> > > 'Obsolete' means not usable anymore.
> > > 
> > > You can feel free to reword the note words.
> > > Perhaps substitute 'removed' for 'obsolete' will be better.
> > 
> > "Removed" would be clearer, yes.  It's probably better to not
> > mention the exact version, and just say it will be removed in
> > the future.
> 
> Then I would tend to agree with your suggestion of no specific
> 'deprecation_note' at all; instead, a general warning message "will be
> removed in the future" in machine_run_board_init().

That would work too, but having "please use Icelake-Server
instead" would reduce user confusion.  It would also be
consistent with how MachineClass does it, and make it easier to
combine those two APIs together in the future.

It's not a hard requirement, though.  I'm OK if you want to keep
it as simple as possible.

> > 
> > Or maybe just make the message shorter and set deprecation_note
> > to "Please use Icelake-Server instead".  The details can be
> > documented in docs/system/deprecated.rst.
> > 
> Prefer documenting detail and model specific deprecation plan in
> docs/system/deprecated.rst.
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-19  3:22       ` Robert Hoo
@ 2020-09-21 15:37         ` Eduardo Habkost
  2020-09-22  5:07           ` Robert Hoo
  0 siblings, 1 reply; 13+ messages in thread
From: Eduardo Habkost @ 2020-09-21 15:37 UTC (permalink / raw)
  To: Robert Hoo; +Cc: qemu-devel, armbru, Hu, Robert, pbonzini, rth

On Sat, Sep 19, 2020 at 11:22:12AM +0800, Robert Hoo wrote:
> On Fri, 2020-09-18 at 16:42 +0000, Eduardo Habkost wrote:
> > ...
> > > > > ---
> > > > > Changelog
> > > > > v3:
> > > > > Make the deprecation implementation CPUClass generic.
> > > > > 
> > > > > v2:
> > > > > Move deprecation check from parse_cpu_option() to
> > > > > machine_run_board_init(), so
> > > > > that it can cover implicit cpu_type assignment cases.
> > > > 
> > > > What do you mean by implicit cpu_type assignment cases?
> > > 
> > > Means the case that user doesn't explicitly assign '-cpu xxx' but
> > > implicitly inherits machine's default cpu type.
> > > vl.c
> > >     /* parse features once if machine provides default cpu_type */
> > >     current_machine->cpu_type = machine_class->default_cpu_type;
> > >     if (cpu_option) {
> > >         current_machine->cpu_type = parse_cpu_option(cpu_option);
> > >     }
> > 
> > We probably would never deprecate CPU models that are still used
> > by default, so this is not an issue.
> 
> Understand.
> Even though less possible, I think it is still doable, say, firstly
> switch the default model to the other one, then deprecate it.

Right.  I mean it's OK to not print warnings if default_cpu_type
is deprecated.

> > 
> > > > 
> > > > Doing it inside parse_cpu_option() like you did in v1 will make
> > > > the deprecation warnings appear for *-user too (which is a good
> > > > thing).
> > > > 
> > > 
> > > So you changed your mind;-)
> > 
> > Sorry, I don't remember suggesting this.  I couldn't find any
> > reply from myself in v1, and v2 already had the code moved to
> > machine_run_board_init().
> > 
> > Note that doing the check in machine_run_board_init() is not
> > necessarily a problem, I'm just trying to understand why the code
> > was moved from parse_cpu_option() to machine_run_board_init()
> > between v1 and v2.
> > 
> Sorry, I just searched my Linux mailbox, cannot find previous thread
> either. But in my memory, it was your suggestion:) and I prefer this
> than doing it in parse_cpu_option() as well, as parse_cpu_option() is
> early time, the passed in cpu_model could be not concrete yet. See my
> below idea of making unversioned cpu_model virtual.

That's OK to me.

> > > 
> > > Could you shed more details? I don't get this point. What do you
> > > mean
> > > "*-user"?
> > 
> > I mean QEMU user mode emulator, bsd-user and linux-user (e.g. the
> > qemu-x86_64 binary).  They call parse_cpu_option() as well.
> > 
> Oh, I'm not familiar with these user modes yet. How about leave them
> alone at this moment? let's cover most cases, i.e. based on machine
> type, first. Then in the future we consider how to expand this to *-
> user cases.

No problem to me.

> > > 
> > > > > Add qapi new member documentation. Thanks Eric for comment and
> > > > > guidance on qapi.
> > > > > 
> > > > >  hw/core/machine.c        | 12 ++++++++++--
> > > > >  include/hw/core/cpu.h    |  6 ++++++
> > > > >  qapi/machine-target.json |  7 ++++++-
> > > > >  target/i386/cpu.c        | 29 +++++++++++++++++++++++------
> > > > >  4 files changed, 45 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index ea26d61..b41f88d 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -1095,6 +1095,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;
> > > > > @@ -1114,11 +1116,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.
> > > > > @@ -1141,6 +1142,13 @@ void machine_run_board_init(MachineState
> > > > > *machine)
> > > > >          }
> > > > >      }
> > > > > 
> > > > > +    /* Check if CPU type is deprecated and warn if so */
> > > > > +    cc = CPU_CLASS(oc);
> > > > > +    if (cc->deprecated) {
> > > > > +        warn_report("CPU model %s is deprecated -- %s",
> > > > > machine-
> > > > > > cpu_type,
> > > > > 
> > > > > +                    cc->deprecation_note);
> > > > > +    }
> > > > > +
> > > > >      machine_class->init(machine);
> > > > >  }
> > > > > 
> > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > > > index 99dc33f..c4b17c8 100644
> > > > > --- a/include/hw/core/cpu.h
> > > > > +++ b/include/hw/core/cpu.h
> > > > > @@ -155,6 +155,10 @@ struct TranslationBlock;
> > > > >   * @disas_set_info: Setup architecture specific components of
> > > > > disassembly info
> > > > >   * @adjust_watchpoint_address: Perform a target-specific
> > > > > adjustment to an
> > > > >   * address before attempting to match it against watchpoints.
> > > > > + * @deprecated: True if this CPU model is deprecated (going to
> > > > > be
> > > > > removed in
> > > > > + *              near future).
> > > > > + * @deprecation_note: Message about the deprecation. e.g.
> > > > > Since
> > > > > which version
> > > > > + *                    will it be obsoleted.
> > > > 
> > > > We don't need two separate fields if (deprecation_note != NULL)
> > > > means the CPU model is deprecated.  This is how it was
> > > > implemented in MachineClass (see
> > > > MachineClass::deprecation_reason).
> > > > 
> > > 
> > > Agree.
> > > I think such applies to X86CPUModel::deprecated and
> > > X86CPUModel::note
> > > as well; and rename X86CPUModel::note --> deprecation_note. How do
> > > you
> > > like this?
> > 
> > Sound good!
> > 
> > > 
> > > > >   *
> > > > >   * Represents a CPU family or model.
> > > > >   */
> > > > > @@ -221,6 +225,8 @@ struct CPUClass {
> > > > >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr
> > > > > addr,
> > > > > int len);
> > > > >      void (*tcg_initialize)(void);
> > > > > 
> > > > > +    bool deprecated;
> > > > > +    const char *deprecation_note;
> > > > >      /* Keep non-pointer data at the end to minimize holes.  */
> > > > >      int gdb_num_core_regs;
> > > > >      bool gdb_stop_before_watchpoint;
> > > > > diff --git a/qapi/machine-target.json b/qapi/machine-
> > > > > target.json
> > > > > index 698850c..fec3bb8 100644
> > > > > --- a/qapi/machine-target.json
> > > > > +++ b/qapi/machine-target.json
> > > > > @@ -286,6 +286,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.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
> > > > > @@ -310,7 +314,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 49d8958..9cb82b7 100644
> > > > > --- a/target/i386/cpu.c
> > > > > +++ b/target/i386/cpu.c
> > > > > @@ -1716,6 +1716,7 @@ typedef struct X86CPUVersionDefinition {
> > > > >      const char *alias;
> > > > >      const char *note;
> > > > >      PropValue *props;
> > > > > +    bool       deprecated;
> > > > >  } X86CPUVersionDefinition;
> > > > > 
> > > > >  /* Base definition for a CPU model */
> > > > > @@ -1751,6 +1752,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 */
> > > > > @@ -5096,6 +5102,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.
> > > > > @@ -5486,9 +5493,12 @@ static void
> > > > > x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> > > > >  {
> > > > >      X86CPUModel *model = data;
> > > > >      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > > +    CPUClass *cc = CPU_CLASS(oc);
> > > > > 
> > > > >      xcc->model = model;
> > > > >      xcc->migration_safe = true;
> > > > > +    cc->deprecated = model->deprecated;
> > > > > +    cc->deprecation_note = g_strdup(model->note);
> > > > 
> > > > The meaning of cc->deprecation_note and model->note is a bit
> > > > ambiguous here.  model->note is not necessarily a deprecation
> > > > note, but its contents are being copied unconditionally to
> > > > cc->deprecation_note.
> > > > 
> > > > What about setting cc->deprecation_note only if and only if the
> > > > model is deprecated?
> > > > 
> > > 
> > > Agree. Since X86CPUModel::note is actually unused by anything now,
> > > going to rename it to deprecation_note as aforementioned.
> > 
> > .note is used by all the CPU models that set .note.  See for
> > example:
> > 
> > x86 Cascadelake-Server-v1  Intel Xeon Processor (Cascadelake)
> > x86 Cascadelake-Server-v2  Intel Xeon Processor (Cascadelake)
> > [ARCH_CAPABILITIES]
> > x86 Cascadelake-Server-v3  Intel Xeon Processor (Cascadelake)
> > [ARCH_CAPABILITIES, no TSX]
> > x86 Cascadelake-Server-v4  Intel Xeon Processor (Cascadelake)
> > [ARCH_CAPABILITIES, no TSX]
> > 
> OK, then I'll add another deprecation_note.
> > 
> > > 
> > > A side concern,
> > > cc->deprecation_note = g_strdup(model->note);
> > > 
> > > I don's see where free the cc object. Assuming the dup'ed string,
> > > as
> > > well as object, will last through QEMU process life time, freed
> > > implicitly as this process gone. Is my understanding right?
> > 
> > We never free QOM class structs (like CPUClass), so that's OK.
> > 
> > > > 
> > > > >  }
> > > > > 
> > > > >  static void x86_register_cpu_model_type(const char *name,
> > > > > X86CPUModel *model)
> > > > > @@ -5524,21 +5534,28 @@ static void
> > > > > x86_register_cpudef_types(X86CPUDefinition *def)
> > > > >      x86_register_cpu_model_type(def->name, m);
> > > > > 
> > > > >      /* Versioned models: */
> > > > > -
> > > > >      for (vdef = x86_cpu_def_get_versions(def); vdef->version;
> > > > > vdef++) {
> > > > > -        X86CPUModel *m = g_new0(X86CPUModel, 1);
> > > > > +        X86CPUModel *vm = g_new0(X86CPUModel, 1);
> > > > >          g_autofree char *name =
> > > > >              x86_cpu_versioned_model_name(def, vdef->version);
> > > > > -        m->cpudef = def;
> > > > > -        m->version = vdef->version;
> > > > > -        m->note = vdef->note;
> > > > > -        x86_register_cpu_model_type(name, m);
> > > > > +        vm->cpudef = def;
> > > > > +        vm->version = vdef->version;
> > > > > +        vm->note = vdef->note;
> > > > > +        vm->deprecated = vdef->deprecated;
> > > > > +        /* If Model-v1 is deprecated, Model is deprecated. */
> > > > > +        if (vdef->version == 1 && vdef->deprecated == true) {
> > > > > +            m->deprecated = true;
> > > > > +            m->note = vdef->note;
> > > > 
> > > > I'm not sure this rule will always apply.  If we deprecate
> > > > "Model-v1" but not "Model-v2", we probably don't want "Model" to
> > > > be deprecated too.
> > > 
> > > Agree
> > > > 
> > > > Considering that we don't have cases where specific versions are
> > > > being deprecated, what about making it as simple as possible and
> > > > just add a new X86CPUDefinition::deprecation_note field?  No need
> > > > for any new fields in X86CPUModel (model->cpudef-
> > > > >deprecation_note
> > > > can be used directly), no need for two new fields in CPUClass
> > > > (just deprecation_note).
> > > 
> > > How about eliminating the unversioned CPU model? Then we can still
> > > have
> > > deprecation_note in X86CPUModel, which looks more natural to me
> > > than in
> > > X86CPUDefition.
> > > For anyway, the unversioned CPU model will eventually be
> > > instantiated
> > > to its some versioned CPU model. It's like a virtual class.
> > 
> > What do you mean by eliminating the unversioned CPU model?  Keep
> > in mind that we need "-cpu Model" to keep working, because of
> > compatibility and also because "-cpu Model" is more convenient.
> > 
> Yes, keeping "-cpu Model" usage as before is my thought as well. I mean
> making it virtual, no real CPU model associate with it, but parse it to
> some concrete version when CPUModel initializes.
> 
> > There are ways the representation of CPU models + aliases +
> > versions can be improved, but I assume you wouldn't want a large
> > refactor of the CPU model/version table to get in the way of a
> > simple feature.
> > 
> Yes. Trying as less refactor as possible. I think my changes even
> cannot be called refactor at all. :)
> My idea is to make unversioned CPU model virtual. I did some patch,
> doable:
> 1) in x86_register_cpudef_types(), don't register cpu_model type for
> the unversioned 'Model'.
> 2) in x86_cpu_class_by_name(), check passed-in cpu_model param
> versioned or not, if the virtual unversioned 'Model', parse it to some
> concrete version by global default_cpu_version designation.
> 
> So, user can still use '-cpu Model' as before, no interface changes.
> And no nature changes:
> 1) in current code, even legacy 'Model', you have created a v1 model
> for it. i.e., every virtual 'Model' already has at least one concrete
> versioned one.
> 2) in current code, x86_cpu_model_resolve_version() is called when
> x86_cpu_load_model(). x86_cpu_model_resolve_version() actually does
> kind of work that concreting unversioned Model to its versioned one, by
> global default_cpu_version designation. Same as my aforementioned
> above.
> 
> Would you like me to integrate this implementation in v4? to have a
> look.

I think we might give this a try, but I wouldn't like to have
your model deprecation series delayed because of this.  Some
obstacles I expect to see:

Right now the code assumes a 1:1 mapping between CPU model and
QOM class.  We even have a `typename` field returned by
query-cpu-definitions.

It would also become an obstacle for removing the existing
arch-specific class_by_name methods and do the model->class
mapping based solely on a string template.  See this thread for a
glimpse on what we have been trying to do:
https://lore.kernel.org/qemu-devel/877eb173a3.fsf@dusky.pond.sub.org/

I'm not saying we shouldn't do what you suggest, but it would add
a lot of complexity to your CPU model deprecation work (that's
very close to be ready to be merged).

> > > 
> > > > 
> > > > If one day we decide to deprecate just some versions of a CPU
> > > > model, we can extend X86CPUVersionDefinition in the future.
> > > > 
> > > > > +        }
> > > > > +        x86_register_cpu_model_type(name, vm);
> > > > > 
> > > > >          if (vdef->alias) {
> > > > >              X86CPUModel *am = g_new0(X86CPUModel, 1);
> > > > >              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);
> > > > >          }
> > > > >      }
> > > > > --
> > > > > 1.8.3.1
> > > > > 
> > > > 
> > > > 
> > 
> > --
> > Eduardo
> > 
> 

-- 
Eduardo



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API
  2020-09-21 15:37         ` Eduardo Habkost
@ 2020-09-22  5:07           ` Robert Hoo
  0 siblings, 0 replies; 13+ messages in thread
From: Robert Hoo @ 2020-09-22  5:07 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, armbru, Hu, Robert, pbonzini, rth

On Mon, 2020-09-21 at 11:37 -0400, Eduardo Habkost wrote:
> > 
...
> > Yes. Trying as less refactor as possible. I think my changes even
> > cannot be called refactor at all. :)
> > My idea is to make unversioned CPU model virtual. I did some patch,
> > doable:
> > 1) in x86_register_cpudef_types(), don't register cpu_model type
> > for
> > the unversioned 'Model'.
> > 2) in x86_cpu_class_by_name(), check passed-in cpu_model param
> > versioned or not, if the virtual unversioned 'Model', parse it to
> > some
> > concrete version by global default_cpu_version designation.
> > 
> > So, user can still use '-cpu Model' as before, no interface
> > changes.
> > And no nature changes:
> > 1) in current code, even legacy 'Model', you have created a v1
> > model
> > for it. i.e., every virtual 'Model' already has at least one
> > concrete
> > versioned one.
> > 2) in current code, x86_cpu_model_resolve_version() is called when
> > x86_cpu_load_model(). x86_cpu_model_resolve_version() actually does
> > kind of work that concreting unversioned Model to its versioned
> > one, by
> > global default_cpu_version designation. Same as my aforementioned
> > above.
> > 
> > Would you like me to integrate this implementation in v4? to have a
> > look.
> 
> I think we might give this a try, but I wouldn't like to have
> your model deprecation series delayed because of this.  Some
> obstacles I expect to see:
> 
> Right now the code assumes a 1:1 mapping between CPU model and
> QOM class.  We even have a `typename` field returned by
> query-cpu-definitions.
> 
> It would also become an obstacle for removing the existing
> arch-specific class_by_name methods and do the model->class
> mapping based solely on a string template.  See this thread for a
> glimpse on what we have been trying to do:
> https://lore.kernel.org/qemu-devel/877eb173a3.fsf@dusky.pond.sub.org/
> 
> I'm not saying we shouldn't do what you suggest, but it would add
> a lot of complexity to your CPU model deprecation work (that's
> very close to be ready to be merged).
> 
OK, I'm going to send v4 without this.
Then send this in a separate patch for discussion.
> > > > 
> > > --
> > > Eduardo
> > > 
> 
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-09-22  5:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16  8:37 [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Robert Hoo
2020-09-16  8:37 ` [PATCH v3 2/2] Mark Icelake-Client CPU models deprecated Robert Hoo
2020-09-17 18:01   ` Eduardo Habkost
2020-09-18  2:18     ` Robert Hoo
2020-09-18  4:20       ` Eduardo Habkost
2020-09-21  7:45         ` Robert Hoo
2020-09-21 15:25           ` Eduardo Habkost
2020-09-17 18:18 ` [PATCH v3 1/2] Introduce (x86) CPU model deprecation API Eduardo Habkost
2020-09-18  5:48   ` Robert Hoo
2020-09-18 16:42     ` Eduardo Habkost
2020-09-19  3:22       ` Robert Hoo
2020-09-21 15:37         ` Eduardo Habkost
2020-09-22  5:07           ` Robert Hoo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).