qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Robert Hoo
Subject: Re: [PATCH v2 1/2] Introduce (x86) CPU model deprecation API
Date: Tue, 15 Sep 2020 10:56:06 +0800

On Mon, 2020-09-14 at 13:38 +0000, Eduardo Habkost wrote:
> On Mon, Sep 14, 2020 at 06:50:09PM +0800, Robert Hoo wrote:
> > On Fri, 2020-09-11 at 10:00 -0400, Eduardo Habkost wrote:
> > > On Fri, Sep 11, 2020 at 02:22:51PM +0800, Robert Hoo wrote:
> > > > On Wed, 2020-09-09 at 14:15 -0400, Eduardo Habkost wrote:
> 
> [...]
> > > > > > +static void x86_cpu_deprecation_check(ObjectClass *oc)
> > > > > > +{
> > > > > > +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > > > +    X86CPUVersion effective_version;
> > > > > > +    const X86CPUVersionDefinition *vdef;
> > > > > > +
> > > > > > +    if (xcc->model == NULL) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    if (xcc->model->version == CPU_VERSION_LEGACY) {
> > > > > > +        /* Treat legacy version as v1 */
> > > > > > +        effective_version = 1;
> > > > > > +    } else {
> > > > > > +        effective_version =
> > > > > > x86_cpu_model_resolve_version(xcc-
> > > > > > > model);
> > > > > > 
> > > > > > +    }
> > > > > > +
> > > > > > +    vdef = xcc->model->cpudef->versions;
> > > > > > +
> > > > > > +    if (vdef == NULL) {
> > > > > > +        return;
> > > > > > +    } else {
> > > > > > +        if (vdef[effective_version - 1].deprecated) {
> > > > > > +            warn_report("Effective CPU model '%s' -- %s",
> > > > > > +                    x86_cpu_versioned_model_name(xcc-
> > > > > > >model-
> > > > > > > cpudef,\
> > > > > > 
> > > > > > +                                                effective_
> > > > > > vers
> > > > > > ion)
> > > > > > ,
> > > > > > +                    vdef[effective_version - 1].note);
> > > > > > +        }
> > > > > > +    }
> > > > > 
> > > > > Why do we need this extra logic?  Isn't it simpler to just
> > > > > add a
> > > > > bool CPUClass::deprecated field, and set:
> > > > > 
> > > > >    cpu->deprecated = model->deprecated;
> > > > > 
> > > > > inside x86_cpu_cpudef_class_init()?
> > > > > 
> > > > 
> > > > All these are to fulfill the target you expected earlier:
> > > > 
> > > > "We need a proper CPU model deprecation API.  Deprecation info
> > > > should appear on query-cpu-definitions and should trigger a
> > > > warning when using the CPU model."
> > > > 
> > > > So I think each deprecated model shall have its own deprecation
> > > > message, e.g. by which version it's going to be deprecation,
> > > > etc.
> > > 
> > > There's nothing x86-specific about having deprecated CPU models,
> > > so I don't understand the reason for the x86-specific hook.
> > > 
> > > If the .note field is the reason you added the arch-specific
> > > hook, you can just add a CPUClass::deprecation_note field and
> > > make the feature generic.
> > > 
> > 
> > I tend to agree with you on this generalization requirement.
> > 
> > But then I find it still has some tricky thing, perhaps that's why
> > I
> > defined this x86 target specific hook:
> > 
> > 1) The versioned CPU model is x86 specific (at least at present)
> 
> I don't see why this would be an obstacle.  You just need to set
> CPUClass::deprecated and/or CPUClass::deprecation_note in the
> x86-specific class_init code.
> 
> > 
> > 2) Each x86 cpudef CPU model has 1 unversioned cpu_model_type then
> > its
> > versioned cpu_model_types. Refer to code in
> > x86_register_cpudef_types(). The unversioned model won't be marked
> > deprecated as it is unkown when registered. In
> > machine_run_board_init(), the cpu_model being checked is the
> > unversioned one, if I set -cpu to its general unversioned model.
> > In short, the unversioned cpudef CPU model would escape the
> > deprecation
> > check.
> 
> Why is that a problem?  If, for example, Model-v1 is deprecated
> and Model-v2 is not deprecated, we must never tell the user that
> "-cpu Model" is deprecated.  

Yes, that's why I cannot mark the unversioned one deprecated or not in
its init.

> Even if some machine types resolve
> "-cpu Model" to Model-v1.
> 
That's what I concerned. Say, if I named "-cpu Icelake-Client" and it's
resolved to Icelake-CPU-v1 (deprecated), shouldn't we warn user?
> --
> Eduardo
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]