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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 17 Jun 2019 17:15:21 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 6/17/19 5:01 PM, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 07:09:59 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >   
> >> Hi Igor, Eduardo,
> >>
> >> On 2/6/18 3:43 PM, Igor Mammedov wrote:  
> >>> On Mon, 5 Feb 2018 20:42:05 -0200
> >>> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <alistair.francis@xilinx.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>  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.    
> >>
> >> I wonder how you want to proceed with this series, the first patch got
> >> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
> >> seems now not very complete/usable.
> >>
> >> Rebasing this series, i.e. with this snippet:
> >>  
> >> -- >8 --    
> >> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >> index f57fc38f92..cca4ec6648 100644
> >> --- a/hw/arm/netduino2.c
> >> +++ b/hw/arm/netduino2.c
> >> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
> >>      DeviceState *dev;
> >>
> >>      dev = qdev_create(NULL, TYPE_STM32F205_SOC);
> >> -    qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
> >> +    qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
> >>      object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
> >>
> >>      armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> >> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
> >>
> >>  static void netduino2_machine_init(MachineClass *mc)
> >>  {
> >> +    static const char *valid_cpus[] = {
> >> +        ARM_CPU_TYPE_NAME("cortex-m3"),
> >> +        ARM_CPU_TYPE_NAME("cortex-m4"),
> >> +        NULL
> >> +    };
> >>      mc->desc = "Netduino 2 Machine";
> >>      mc->init = netduino2_init;
> >> +    mc->valid_cpu_types = valid_cpus;
> >>      mc->ignore_memory_transaction_failures = true;
> >>  }
> >> ---
> >>
> >> We get cpu names with suffix:
> >>
> >>   $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
> >>   qemu-system-arm: Invalid CPU type: arm926-arm-cpu
> >>   The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
> >>
> >> I understand you won't want a global cpu_name_by_type_name, how do you
> >> want to do then?
> >>
> >> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> >> Then we could have generic machine code to parse the names.  
> > It would work only for some cases,
> > problem is that we have a zoo of naming schemes.
> > Considering that cpus models are used widely we probably can't
> > deprecate it outright (for versioned machine types).  
> > 
> > Instead of wasting resources on translating cpu-type => 'cpu-name',
> > (hook or lookup tables) how about simplifying code and making all
> > boards accept full typenames?
> > 
> > It could be handled in generic way and then printing error with
> > full type names would be acceptable since user would be able to feed it
> > to -cpu.
> > 
> > '-cpu help' - would need some work to display types as well (also could be generic)
> > 
> > Perhaps with it we could deprecate cpu_models for non versioned
> > machines/targets, which in most cases would allow us to drop special
> > suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
> > is already existing and keep exiting translation routines (hooks) only
> > for versioned machine types as necessary evil.  
Maybe we don't even need to keep it versioned machine types, considering
change mapping is static and doesn't influence ABI/migration.
Management interface could just translate cpu_model to type name
for new QEMU.

That would allow us to remove quite a bit target specific code
that deals with all permutations of CPU model and replace a bunch of
'-cpu help' target specific handlers with a generic enumeration
of built-in CPU types.


> Yes. Eduardo and you should write some lines to explain this, and then
> we will follow :)
Unfortunately I don't recall details anymore. One could check out all
implementations of class_by_name callbacks to find out current state.


> I feel concerned because:
> 1/ Alistair series is very helpful to new users, and
> 2/ As the RX architecture series showed, today it is not very clear how
> to correctly use cpu_models.
That's why I'm pushing for supporting only type names whenever possible
so there  won't be confusion anymore. Using types is consistent with
-device and some QMP interfaces, enforces strict error checking so
user would get error on nonsense input.


> Eduardo is working on a series, I'll wait for his work.
> 
> Regards,
> 
> Phil.