Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model

[Igor Mammedov]

On Mon, 5 Feb 2018 20:42:05 -0200
Eduardo Habkost <address@hidden> wrote:

> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> > On Mon, 5 Feb 2018 11:54:01 -0200
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 2 Feb 2018 16:23:26 -0200
> > > > Eduardo Habkost <address@hidden> wrote:
> > > >     
> > > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:    
> > > > > > As cpu_type is not a user visible string let's convert the
> > > > > > valid_cpu_types to compare against cpu_model instead. This way we 
> > > > > > have a
> > > > > > user friendly string to report back.
> > > > > > 
> > > > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > > > reverted and we should use cpu_type instead.
> > > > > > 
> > > > > > Signed-off-by: Alistair Francis <address@hidden>
> > > > > > ---
> > > > > > 
> > > > > >  hw/core/machine.c | 11 +++++------
> > > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index cdc1163dc6..de5bac1c84 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState 
> > > > > > *machine)
> > > > > >      /* If the machine supports the valid_cpu_types check and the 
> > > > > > user
> > > > > >       * 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);
> > > > > > +    if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > > >          int i;
> > > > > >  
> > > > > >          for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > > > -            if (object_class_dynamic_cast(class,
> > > > > > -                                          
> > > > > > machine_class->valid_cpu_types[i])) {
> > > > > > +            if (!strcmp(machine->cpu_model,
> > > > > > +                        machine_class->valid_cpu_types[i])) {      
> > > > > 
> > > > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > > > new semantics clearer.
> > > > > 
> > > > > Anyway, I have bad and good news:
> > > > > 
> > > > > The bad news is Igor already sent patches last week that remove
> > > > > MachineState::cpu_model, so this conflicts with his series.  Now
> > > > > parse_cpu_model() will be the only place where the original CPU model 
> > > > > name is
> > > > > available, but the function needs to work on *-user too.  See:
> > > > > "[PATCH v3 23/25] Use cpu_create(type) instead of 
> > > > > cpu_init(cpu_model)".
> > > > > 
> > > > > The good news is that I think we can fix this very easily if
> > > > > validation is done at the same place where parse_cpu_model() is
> > > > > called.  e.g.:
> > > > > 
> > > > >     current_machine->cpu_type = machine_class->default_cpu_type;
> > > > >     if (cpu_model) {
> > > > >         current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > > > 
> > > > >         if (machine_class->valid_cpu_models) {
> > > > >             ObjectClass *class = 
> > > > > object_class_by_name(machine->cpu_type);
> > > > >             int i;
> > > > > 
> > > > >             for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > > >                 const char *valid_model = 
> > > > > machine_class->valid_cpu_models[i];
> > > > >                 ObjectClass *valid_class = 
> > > > > cpu_class_by_name(machine->cpu_type, valid_model);
> > > > >                 if (object_class_dynamic_cast(class,
> > > > >                                               
> > > > > object_class_get_name(valid_class))) {
> > > > >                      /* Valid CPU type, we're good to go */
> > > > >                      break;
> > > > >                 }
> > > > >             }
> > > > >             if (!machine_class->valid_cpu_models[i]) {
> > > > >                 error_report("Invalid CPU model: %s", cpu_model);
> > > > >                 error_printf("The valid CPU models are: %s",
> > > > >                              machine_class->valid_cpu_models[0]);
> > > > >                 for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > > >                     error_printf(", %s", 
> > > > > machine_class->valid_cpu_models[i]);
> > > > >                 }
> > > > >                 error_printf("\n");
> > > > >                 exit(1);
> > > > >             }
> > > > >         }
> > > > >     }
> > > > > 
> > > > > This can be done inside main(), or moved inside
> > > > > machine_run_board_init() if main() pass cpu_model as argument to
> > > > > the function.
> > > > > 
> > > > > On either case, I think it's a good idea to do validation and
> > > > > printing of error messages closer to the code that parses the
> > > > > command-line options.  This way we separate parsing/validation
> > > > > from initialization.    
> > > > I agree it's better like you suggest as at least it prevents
> > > > ms->cpu_model creeping back into boards code.
> > > > 
> > > > But I still dislike (hate) an idea of new code adding non
> > > > canonized cpu_model strings back in the boards code.
> > > > It's just a matter of time when someone would use them
> > > > and cpu_model parsing will creep back into boards.
> > > > 
> > > > It would be much better to if we add 
> > > >    char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > > > callback and let machines in this patchset to set it,
> > > > something along following lines which is not much of
> > > > refactoring and allows for gradual conversion:
> > > > 
> > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > index 9631670..85cca84 100644
> > > > --- a/target/arm/cpu.h
> > > > +++ b/target/arm/cpu.h
> > > > @@ -2885,4 +2885,6 @@ static inline void 
> > > > *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > > >      return cpu->el_change_hook_opaque;
> > > >  }
> > > >  
> > > > +char *arm_cpu_name_by_type_name(const char *typename);
> > > > +
> > > >  #endif
> > > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > > > index f936017..ae6adb7 100644
> > > > --- a/hw/arm/netduino2.c
> > > > +++ b/hw/arm/netduino2.c
> > > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > > >      mc->desc = "Netduino 2 Machine";
> > > >      mc->init = netduino2_init;
> > > >      mc->ignore_memory_transaction_failures = true;
> > > > +    mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:    
> > > 
> > > I really don't want to introduce a new arch-specific hook just
> > > for that.  We should move CPU type lookup logic to common code
> > > and make it unnecessary to write new hooks.  
> > unfortunately cpu_model (cpu name part) is target specific
> > and it's translation to type and back is target specific mayhem.  
> 
> Why can't the model<->type translation be represented as data?
> We could have simple cpu_type_name_suffix + an alias table.
> 
> We have at least 4 arches that return a constant at
> class_by_name.  We have at least 10 arches that simply add a
> suffix to the CPU model name.  We must make them use common code
> instead of requiring them to implement yet another hook[1].
True, some of them could use generic hook and reduce
code duplication greatly, we should do it regardless of whether
table or target specific func approach is used.

> In addition to the ones above, we have 3 that seem to just need
> an alias table (cris, superh, alpha).  ppc can probably also use
> an alias table for the ppc_cpu_class_by_pvr() stuff.  sparc just
> needs whitespaces translated to '-' (sparc), which can be done
> using an alias table.
> 
> In the end I couldn't find any example that can't be represented
> by a suffix + alias table.

Table based approach is possible but it won't be as simple
as you've just pictured it.

From what I recall from cpu_class_by_name cleanups table should be able
to describe cases like (sometimes combination of them):
   * 1:1 mapping - where cpu_model == cpu_type
   * cpu_model <==> cpu_model + suffix  - most common usecase
   * cpu_model <==> prefix cpu_model  - riscv patches on list are trying to add 
such cpu types
   * NULL => some_fixed type
   * case (in) sensitive flag
   * garbage => some_fixed type
   * substitutions
   * aliases (sometimes dynamic depending on --enable-kvm (PPC))
Maybe something else.

We can think about it at leisure but I can't say if new approach
complexity it's worth of the effort.
 
It would be nice see impl, but it's a lot of refactoring that's
clearly out of scope of this series.
I'd prefer small incremental refactoring (if possible) that
won't scare people of and easy to review vs a huge one.

> > So I'd prefer having both back and forth functions together in
> > one place. And common code to call them when necessary.
> > 
> > We could do global cpu_name_by_type_name() instead of hook,
> > which I'd prefer even more but then conversion can't be done
> > only for one target but rather for all targets at once.  
> 
> I don't mind letting a few targets override default behavior with
> a hook if really necessary, but I have a problem with requiring
> all targets to implement what's basically the same boilerplate
> code to add/remove a string suffix and translating aliases.
it could be generic helper if target does the same plus
not mandatory at that (in case target/board doesn't care
about valid cpus).

> > > I agree it would be better if we had a cpu_name_by_type_name()
> > > function, but I would like to have it implemented cleanly.  
> > In some cases(targets) it can be common helper, but in other
> > cases it's not so.
> > My suggestion though allows to do gradual conversion and
> > avoid putting cpu_model names back in board's code (which I just manged to 
> > remove).
> > Once all targets converted and relevant code is isolated
> > we can attempt to generalize it if it's possible or at least
> > make of it global per target helper to get rid of
> > temporary machine hook.
> > 
> > (seeing this series reposted with cpu_model names in boards code,
> > it doesn't looks like author would like to implement tree-wide
> > generalization first)  
> 
> Well, if nobody is willing to generalize all target-specific code
> right now, I don't see the harm in having cpu_model-based tables
> in a few boards in the meantime (as this patch series does).  But
> I do see harm in requiring all our 20 targets to implement yet
> another hook and increasing the costs of cleaning up the mess
> later.
If we use MachineClass hook then it might be done per target
on demand, so no one would require that every target should
implement it.
Also there could be a generic helper for targets that do the same.
Machine which needs to enable valid_cpus, will have to use generic
hook impl or provide target specific if it's special case.

Though I do see harm in adding cpu_model tables in boards code
vs target specific hooks on demand as that will be copy-pasted
in other boards afterwards (number of which is bigger compared
to targets count) and ultimately it would duplicate cpu_name
strings in every board vs hook approach where cpu_model could
be calculated from cpu_type by a function (generic or
target specific).

Good thing about hook is that it's non intrusive and
isolates(consolidates) existing cpu_type -> cpu_model
conversion in multiple places into one place.
Then later it would be easier to generalize if someone
decides to do it.