Re: [PATCH v3 1/2] Introduce (x86) CPU model deprecation API

[Eduardo Habkost]

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