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

[Robert Hoo]

On Fri, 2020-09-11 at 10:00 -0400, Eduardo Habkost wrote:
> On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote:
> > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote:
> > > Hi,
> > > 
> > > 
> > > > @@ -1129,6 +1130,12 @@ void machine_run_board_init(MachineState
> > > > *machine)
> > > >          }
> > > >      }
> > > >  
> > > > +    /* Check if CPU type is deprecated and warn if so */
> > > > +    cc = CPU_CLASS(oc);
> > > > +    if (cc->deprecation_check) {
> > > > +        cc->deprecation_check(oc);
> > > > +    }
> > > 
> > > Why do we need target-specific code here?  A CPUClass::deprecated
> > > field would be much simpler.
> > > 
> > 
> > Because the Warning message composing is target-specific, using
> > X86CPUVersionDefinition.note.
> > Other targets can have their own warning message composing
> > approaches.
> 
> I think I understand what you were trying to do, but having each
> target with a different warning message would be a bad thing, not
> a desirable feature.  The warning message can be generic.
> 
> > 
> > > > +
> > > >      machine_class->init(machine);
> > > >  }
> > > >  
> > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > > index 497600c..1ca47dc 100644
> > > > --- a/include/hw/core/cpu.h
> > > > +++ b/include/hw/core/cpu.h
> > > > @@ -218,6 +218,7 @@ typedef struct CPUClass {
> > > >      void (*disas_set_info)(CPUState *cpu, disassemble_info
> > > > *info);
> > > >      vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr
> > > > addr,
> > > > int len);
> > > >      void (*tcg_initialize)(void);
> > > > +    void (*deprecation_check)(ObjectClass *oc);
> > > >  
> > > >      /* Keep non-pointer data at the end to minimize holes.  */
> > > >      int gdb_num_core_regs;
> > > > diff --git a/qapi/machine-target.json b/qapi/machine-
> > > > target.json
> > > > index f2c8294..c24f506 100644
> > > > --- a/qapi/machine-target.json
> > > > +++ b/qapi/machine-target.json
> > > > @@ -285,6 +285,10 @@
> > > >  #            in the VM configuration, because aliases may stop
> > > > being
> > > >  #            migration-safe in the future (since 4.1)
> > > >  #
> > > > +# @deprecated: If true, this CPU model is deprecated and may
> > > > be
> > > > removed in
> > > > +#              in some future version of QEMU according to the
> > > > QEMU deprecation
> > > > +#              policy. (since 5.1)
> > > 
> > > Next version needs to say "since 5.2".
> > 
> > Sure.
> > > 
> > > > 
> > > >  
> > > >  /* Get full model name for CPU version */
> > > > @@ -4128,8 +4134,7 @@ static X86CPUVersion
> > > > x86_cpu_model_resolve_version(const X86CPUModel *model)
> > > >      X86CPUVersion v = model->version;
> > > >      if (v == CPU_VERSION_AUTO) {
> > > >          v = default_cpu_version;
> > > > -    }
> > > > -    if (v == CPU_VERSION_LATEST) {
> > > > +    } else if (v == CPU_VERSION_LATEST) {
> > > 
> > > Why is this change necessary?
> > 
> > Just kind of compulsion of avoiding unnecessary if() :-). 'v' can
> > only
> > be one of CPU_VERSION_AUTO and CPU_VERSION_LATEST, unnecessary to
> > judge
> > twice.
> 
> I think this breaks the case where default_cpu_version is set to
> CPU_VERSION_LATEST
> 
OK, understand.
> > > 
> > > >          return x86_cpu_model_last_version(model);
> > > >      }
> > > >      return v;
> > > > @@ -4975,6 +4980,7 @@ static void
> > > > x86_cpu_definition_entry(gpointer
> > > > data, gpointer user_data)
> > > >      info->migration_safe = cc->migration_safe;
> > > >      info->has_migration_safe = true;
> > > >      info->q_static = cc->static_model;
> > > > +    info->deprecated = cc->model ? cc->model->deprecated :
> > > > false;
> > > >      /*
> > > >       * Old machine types won't report aliases, so that alias
> > > > translation
> > > >       * doesn't break compatibility with previous QEMU
> > > > versions.
> > > > @@ -5411,6 +5417,7 @@ static void
> > > > x86_register_cpudef_types(X86CPUDefinition *def)
> > > >          m->cpudef = def;
> > > >          m->version = vdef->version;
> > > >          m->note = vdef->note;
> > > > +        m->deprecated = vdef->deprecated;
> > > >          x86_register_cpu_model_type(name, m);
> > > >  
> > > >          if (vdef->alias) {
> > > > @@ -5418,6 +5425,8 @@ static void
> > > > x86_register_cpudef_types(X86CPUDefinition *def)
> > > >              am->cpudef = def;
> > > >              am->version = vdef->version;
> > > >              am->is_alias = true;
> > > > +            am->note = vdef->note;
> > > 
> > > Is this extra line related to the deprecation feature?
> > > 
> > > It doesn't seem related, and it doesn't seem necessary as the
> > > `note` field is already ignored for alias CPU models.
> > 
> > Because it is unused by other features, I use it to store model
> > specific deprecation message.
> > > 
> > > > +            am->deprecated = vdef->deprecated;
> > > >              x86_register_cpu_model_type(vdef->alias, am);
> > > >          }
> > > >      }
> > > > @@ -7233,6 +7242,37 @@ static Property x86_cpu_properties[] = {
> > > >      DEFINE_PROP_END_OF_LIST()
> > > >  };
> > > >  
> > > > +static void x86_cpu_deprecation_check(ObjectClass *oc)
> > > > +{
> > > > +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > +    X86CPUVersion effective_version;
> > > > +    const X86CPUVersionDefinition *vdef;
> > > > +
> > > > +    if (xcc->model == NULL) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (xcc->model->version == CPU_VERSION_LEGACY) {
> > > > +        /* Treat legacy version as v1 */
> > > > +        effective_version = 1;
> > > > +    } else {
> > > > +        effective_version = x86_cpu_model_resolve_version(xcc-
> > > > > model);
> > > > 
> > > > +    }
> > > > +
> > > > +    vdef = xcc->model->cpudef->versions;
> > > > +
> > > > +    if (vdef == NULL) {
> > > > +        return;
> > > > +    } else {
> > > > +        if (vdef[effective_version - 1].deprecated) {
> > > > +            warn_report("Effective CPU model '%s' -- %s",
> > > > +                    x86_cpu_versioned_model_name(xcc->model-
> > > > > cpudef,\
> > > > 
> > > > +                                                effective_vers
> > > > ion)
> > > > ,
> > > > +                    vdef[effective_version - 1].note);
> > > > +        }
> > > > +    }
> > > 
> > > Why do we need this extra logic?  Isn't it simpler to just add a
> > > bool CPUClass::deprecated field, and set:
> > > 
> > >    cpu->deprecated = model->deprecated;
> > > 
> > > inside x86_cpu_cpudef_class_init()?
> > > 
> > 
> > All these are to fulfill the target you expected earlier:
> > 
> > "We need a proper CPU model deprecation API.  Deprecation info
> > should appear on query-cpu-definitions and should trigger a
> > warning when using the CPU model."
> > 
> > So I think each deprecated model shall have its own deprecation
> > message, e.g. by which version it's going to be deprecation, etc.
> 
> There's nothing x86-specific about having deprecated CPU models,
> so I don't understand the reason for the x86-specific hook.
> 
> If the .note field is the reason you added the arch-specific
> hook, you can just add a CPUClass::deprecation_note field and
> make the feature generic.
> 
I tend to agree with you on this generalization requirement.

But then I find it still has some tricky thing, perhaps that's why I
defined this x86 target specific hook:

1) The versioned CPU model is x86 specific (at least at present)

2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then its
versioned cpu_model_types. Refer to code in
x86_register_cpudef_types(). The unversioned model won't be marked
deprecated as it is unkown when registered. In
machine_run_board_init(), the cpu_model being checked is the
unversioned one, if I set -cpu to its general unversioned model.
In short, the unversioned cpudef CPU model would escape the deprecation
check.

> > 
> > > > +}
> > > > +
> > > >  static void x86_cpu_common_class_init(ObjectClass *oc, void
> > > > *data)
> > > >  {
> > > >      X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > @@ -7291,6 +7331,7 @@ static void
> > > > x86_cpu_common_class_init(ObjectClass *oc, void *data)
> > > >      cc->tlb_fill = x86_cpu_tlb_fill;
> > > >  #endif
> > > >      cc->disas_set_info = x86_disas_set_info;
> > > > +    cc->deprecation_check = x86_cpu_deprecation_check;
> > > >  
> > > >      dc->user_creatable = true;
> > > >  }
> > > > -- 
> > > > 1.8.3.1
> > > > 
> > > 
> > > 
> 
>