Re: [Qemu-devel] [PATCH 20/24] machine: drop MachineState::cpu_model

[Igor Mammedov]

On Fri, 19 Jan 2018 11:14:30 -0200
Eduardo Habkost <address@hidden> wrote:

> On Fri, Jan 19, 2018 at 11:14:39AM +0100, Igor Mammedov wrote:
> > On Thu, 18 Jan 2018 17:18:09 -0200
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Thu, Jan 18, 2018 at 11:10:35AM +0100, Igor Mammedov wrote:
> > > > On Wed, 17 Jan 2018 23:48:46 -0200
> > > > Eduardo Habkost <address@hidden> wrote:
> > > >   
> > > > > On Wed, Jan 17, 2018 at 04:43:32PM +0100, Igor Mammedov wrote:  
> > > > > > The last user of it was machine type 'none', which used field
> > > > > > to create CPU id user requested it on CLI with -cpu option.
> > [...]
> > 
> > > It looks like default_cpu_type is being overloaded for two
> > > different roles: 1) specifying the default CPU type; 2) finding
> > > the arch-specific class to be used to parse -cpu.
> > > 
> > > In the case of null-machine, these two roles conflict with each
> > > other.  I believe we can find other solutions instead of this
> > > hack that involves lying on MachineClass::default_cpu_type (and
> > > then having to work around the lie on machine_none_init()).
> > > 
> > > I see multiple options: adding a new MachineClass field for that
> > > (e.g.  resolving_cpu_type, which defaults to default_cpu_type if
> > > NULL); moving the CPU parsing code to arch_init.c (so it could
> > > use CPU_RESOLVING_TYPE or something similar); adding a optional
> > > MachineClass::parse_cpu_model hook.  We could even try to get rid
> > > of CPUClass::parse_features completely
> > Adding hooks just for the sake on null-machine seems to be overkill,
> > I'd go for arch_init.c but it won't work for linux-user, how about
> > exec.c as following:
> > 
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 93bd546..0185589 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -661,8 +661,7 @@ ObjectClass *cpu_class_by_name(const char *typename, 
> > const char *cpu_model);
> > [...]
> > 
> > diff --git a/exec.c b/exec.c
> > index d28fc0c..4543f06 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -817,6 +817,29 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> >  #endif
> >  }
> >  
> > +const char *parse_cpu_model(const char *cpu_model)
> > +{
> > +    ObjectClass *oc;
> > +    CPUClass *cc;
> > +    gchar **model_pieces;
> > +    const char *cpu_type;
> > +
> > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > +
> > +    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
> > +    if (oc == NULL) {
> > +        error_report("unable to find CPU model '%s'", model_pieces[0]);
> > +        g_strfreev(model_pieces);
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> > +    cpu_type = object_class_get_name(oc);
> > +    cc = CPU_CLASS(oc);
> > +    cc->parse_features(cpu_type, model_pieces[1], &error_fatal);
> > +    g_strfreev(model_pieces);
> > +    return cpu_type;
> > +}
> 
> Sounds good to me.  Only two comments:
> 
> This looks like duplication of cpu_parse_cpu_model().  Should
> this function body be replaced with:
>   cpu_parse_cpu_model(CPU_RESOLVING_TYPE, cpu_model)
it's cpu_parse_cpu_model() which is moved to exec.c
and first typename argument is replaced by inline CPU_RESOLVING_TYPE


> I would move this to arch_init.c, because that's where existing
> target-dependent initialization code lives.
arch_init.c doesn't fit linux-user, it has only sys emulation code
while exec.c is used by both and still target depended so CPU_RESOLVING_TYPE
could be used there

> 
> 
> > [...]
>