Re: [PATCH v2 3/8] machine: Print supported CPU models instead of typenames

[Igor Mammedov]

On Wed, 30 Aug 2023 17:34:12 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Igor,
> 
> On 8/29/23 19:03, Igor Mammedov wrote:
> > On Tue, 29 Aug 2023 16:28:45 +1000
> > Gavin Shan <gshan@redhat.com> wrote:  
> >> On 8/29/23 00:46, Igor Mammedov wrote:  
> >>> On Mon, 31 Jul 2023 15:07:30 +1000
> >>> Gavin Shan <gshan@redhat.com> wrote:  
> >>>> On 7/27/23 19:00, Igor Mammedov wrote:  
> >>>>> On Thu, 27 Jul 2023 15:16:18 +1000
> >>>>> Gavin Shan <gshan@redhat.com> wrote:
> >>>>>         
> >>>>>> On 7/27/23 09:08, Richard Henderson wrote:  
> >>>>>>> On 7/25/23 17:32, Gavin Shan wrote:  
> >>>>>>>> -static const char *q800_machine_valid_cpu_types[] = {
> >>>>>>>> +static const char * const q800_machine_valid_cpu_types[] = {
> >>>>>>>>          M68K_CPU_TYPE_NAME("m68040"),
> >>>>>>>>          NULL
> >>>>>>>>      };
> >>>>>>>> +static const char * const q800_machine_valid_cpu_models[] = {
> >>>>>>>> +    "m68040",
> >>>>>>>> +    NULL
> >>>>>>>> +};  
> >>>>>>>
> >>>>>>> I really don't like this replication.
> >>>>>>>            
> >>>>>>
> >>>>>> Right, it's going to be lots of replications, but gives much 
> >>>>>> flexibility.
> >>>>>> There are 21 targets and we don't have fixed pattern for the mapping 
> >>>>>> between
> >>>>>> CPU model name and CPU typename. I'm summarizing the used patterns 
> >>>>>> like below.
> >>>>>>
> >>>>>>       1 All CPU model names are mappinged to fixed CPU typename;  
> >>>>>
> >>>>>            plainly spelled it would be: cpu_model name ignored and
> >>>>>            a cpu type is returned anyways.
> >>>>>
> >>>>> I'd make this hard error right away, as "junk in => error out"
> >>>>> it's clearly user error. I think we don't even have to follow
> >>>>> deprecation process for that.
> >>>>>         
> >>>>
> >>>> Right, It's not expected behavior to map ambiguous CPU model names to
> >>>> the fixed CPU typename.  
> >>>
> >>> to be nice we can deprecate those and then later remove.
> >>> (while deprecating make those targets accept typenames)
> >>>      
> >>
> >> Lets put it aside for now and revisit it later, so that we can focus on
> >> the conversion from the CPU type name to the CPU model name for now.
> >>  
> >>>>     
> >>>>>>       2 CPU model name is same to CPU typename;
> >>>>>>       3 CPU model name is alias to CPU typename;
> >>>>>>       4 CPU model name is prefix of CPU typename;  
> >>>>>
> >>>>> and some more:
> >>>>>        5. cpu model names aren't names at all sometimes, and some other
> >>>>>           CPU property is used. (ppc)
> >>>>> This one I'd prefer to get rid of and ppc handling more consistent
> >>>>> with other targets, which would need PPC folks to persuaded to drop
> >>>>> PVR lookup.
> >>>>>         
> >>>>
> >>>> I put this into class 3, meaning the PVRs are regarded as aliases to CPU
> >>>> typenames.  
> >>>
> >>> with PPC using 'true' aliases, -cpu input is lost after it's translated 
> >>> into typename.
> >>> (same for alpha)
> >>>
> >>> it also adds an extra fun with 'max' cpu model but that boils down to 
> >>> above statement.
> >>> (same for
> >>>     * sh4
> >>>     * cris(in user mode only, but you are making sysemu extension, so it 
> >>> doesn't count)
> >>> )
> >>> For this class of aliases reverse translation won't yield the same
> >>> result as used -cpu. The only option you have is to store -cpu cpu_model
> >>> somewhere (use qemu_opts??, and then fetch it later to print in error 
> >>> message)
> >>>
> >>> x86 has 'aliases' as well, but in reality it creates distinct cpu types
> >>> for each 'alias', so it's possible to do reverse translation.
> >>>      
> >>
> >> It's true that '-cpu input' gets lost in these cases. However, the CPU type
> >> name instead of the CPU model name is printed in the error message when the
> >> CPU type is validated in hw/core/machine.c::machine_run_board_init(). It 
> >> looks
> >> good to me to print the CPU type name instead of the model name there.  
> > 
> > It's the same confusing whether it's type or cpumodel it it doesn't match
> > user provided value.
> >   
> 
> I tend to agree that it's misleading to print the CPU type name in the
> error message in hw/core/machine.c::machine_run_board_init(), where the CPU
> type is validated. qemu_opts may be too heavy for this. It eventually turns
> to a machine's property if @machine_opts_dict is the best place to store
> '-cpu input'. Besides, it doesn't fit for another case very well, where
> current_machine->cpu_type = machine_class->default_cpu_type if '-cpu input'
> isn't provided by user.
> 
> For simplicity, how about to add MachineState::cpu_model? It's initialized to
> cpu_model_from_type(machine_class->default_cpu_type) in qemu_init(), or
> g_strdump(model_pieces[0) in parse_cpu_option().

I'd prefer not bringing cpu_model back to device models
(Machine in this case) after getting rid of it.


> >> Another error message is printed when the CPU model specified in '-cpu 
> >> input'
> >> isn't valid. The CPU model has been printed and it looks good either.
> >>
> >>     # qemu-system-aarch64 -M virt -cpu aaa
> >>     qemu-system-aarch64: unable to find CPU model 'aaa'
> >>
> >> Are there other cases I missed where we need to print the CPU model name, 
> >> which
> >> is specified by user through '-cpu input'?
> >>  
> >>>>>>
> >>>>>>       Target         Categories    suffix-of-CPU-typename
> >>>>>>       -------------------------------------------------------
> >>>>>>       alpha          -234          -alpha-cpu
> >>>>>>       arm            ---4          -arm-cpu
> >>>>>>       avr            -2--
> >>>>>>       cris           --34          -cris-cpu
> >>>>>>       hexagon        ---4          -hexagon-cpu
> >>>>>>       hppa           1---
> >>>>>>       i386           ---4          -i386-cpu
> >>>>>>       loongarch      -2-4          -loongarch-cpu
> >>>>>>       m68k           ---4          -m68k-cpu
> >>>>>>       microblaze     1---
> >>>>>>       mips           ---4          -mips64-cpu  -mips-cpu
> >>>>>>       nios2          1---
> >>>>>>       openrisc       ---4          -or1k-cpu
> >>>>>>       ppc            --34          -powerpc64-cpu  -powerpc-cpu
> >>>>>>       riscv          ---4          -riscv-cpu
> >>>>>>       rx             -2-4          -rx-cpu
> >>>>>>       s390x          ---4          -s390x-cpu
> >>>>>>       sh4            --34          -superh-cpu
> >>>>>>       sparc          -2--  
> >>>
> >>> it's case 4
> >>>      
> >>
> >> Yes.
> >>  
> >>>>>>       tricore        ---4          -tricore-cpu
> >>>>>>       xtensa         ---4          -xtensa-cpu
> >>>>>>
> >>>>>> There are several options as below. Please let me know which one or 
> >>>>>> something
> >>>>>> else is the best.
> >>>>>>
> >>>>>> (a) Keep what we have and use mc->valid_{cpu_types, cpu_models}[] to 
> >>>>>> track
> >>>>>> the valid CPU typenames and CPU model names.
> >>>>>>
> >>>>>> (b) Introduce CPUClass::model_name_by_typename(). Every target has 
> >>>>>> their own
> >>>>>> implementation to convert CPU typename to CPU model name. The CPU 
> >>>>>> model name
> >>>>>> is parsed from mc->valid_cpu_types[i].
> >>>>>>
> >>>>>>         char *CPUClass::model_by_typename(const char *typename);
> >>>>>>
> >>>>>> (c) As we discussed before, use mc->valid_cpu_type_suffix and 
> >>>>>> mc->valid_cpu_models
> >>>>>> because the CPU type check is currently needed by target 
> >>>>>> arm/m68k/riscv where we
> >>>>>> do have fixed pattern to convert CPU model names to CPU typenames. The 
> >>>>>> CPU typename
> >>>>>> is comprised of CPU model name and suffix. However, it won't be 
> >>>>>> working when the CPU
> >>>>>> type check is required by other target where we have patterns other 
> >>>>>> than this.  
> >>>>>
> >>>>> none of above is really good, that's why I was objecting to introducing
> >>>>> reverse type->name mapping. That ends up with increased amount junk,
> >>>>> and it's not because your patches are bad, but because you are trying
> >>>>> to deal with cpu model names (which is a historically evolved mess).
> >>>>> The best from engineering POV would be replacing CPU models with
> >>>>> type names.
> >>>>>
> >>>>> Even though it's a bit radical, I very much prefer replacing
> >>>>> cpu_model names with '-cpu type'usage directly. Making it
> >>>>> consistent with -device/other interfaces and coincidentally that
> >>>>> obsoletes need in reverse name mapping.
> >>>>>
> >>>>> It's painful for end users who will need to change configs/scripts,
> >>>>> but it's one time job. Additionally from QEMU pov, codebase
> >>>>> will be less messy => more maintainable which benefits not only
> >>>>> developers but end-users in the end.
> >>>>>         
> >>>>
> >>>> I have to clarify the type->model mapping has been existing since the
> >>>> model->type mapping was introduced with the help of CPU_RESOLVING_TYPE.
> >>>> I mean the logic has been existing since the existence of 
> >>>> CPU_RESOLVING_TYPE,
> >>>> even the code wasn't there.
> >>>>
> >>>> I'm not sure about the idea to switch to '-cpu <cpu-type-name>' since
> >>>> it was rejected by Peter Maydell before. Hope Peter can double confirm
> >>>> for this. For me, the shorter name is beneficial. For example, users
> >>>> needn't to have '-cpu host-arm-cpu' for '-cpu host'.
> >>>>
> >>>>     
> >>>>> [rant:
> >>>>> It's the same story repeating over and over, when it comes to
> >>>>> changing QEMU CLI, which hits resistance wall. But with QEMU
> >>>>> deprecation process we've changed CLI behavior before,
> >>>>> despite of that world didn't cease to exist and users
> >>>>> have adapted to new QEMU and arguably QEMU became a tiny
> >>>>> bit more maintainable since we don't have to deal some
> >>>>> legacy behavior.
> >>>>> ]
> >>>>>         
> >>>>
> >>>> I need more context about 'deprecation process' here. My understanding
> >>>> is both CPU typename and model name will be accepted for a fixed period
> >>>> of time. However, a warning message will be given to indicate that the
> >>>> model name will be obsoleted soon. Eventually, we switch to CPU typename
> >>>> completely. Please correct me if there are anything wrong.  
> >>>
> >>> yep, that's the gist of deprecation in this case.
> >>>      
> >>
> >> Ok. Thanks for your confirm.
> >>       
> >>>>> Another idea back in the days was (as a compromise),
> >>>>>     1. keep using keep valid_cpu_types
> >>>>>     2. instead of introducing yet another way to do reverse mapping,
> >>>>>        clean/generalize/make it work everywhere list_cpus (which
> >>>>>        already does that mapping) and then use that to do your thing.
> >>>>>        It will have drawbacks you've listed above, but hopefully
> >>>>>        that will clean up and reuse existing list_cpus.
> >>>>>        (only this time, I'd build it around  query-cpu-model-expansion,
> >>>>>         which output is used by generic list_cpus)
> >>>>>        [and here I'm asking to rewrite directly unrelated QEMU part yet 
> >>>>> again]
> >>>>>         
> >>>>
> >>>> I'm afraid that list_cpus() is hard to be reused. All available CPU 
> >>>> model names
> >>>> are listed by list_cpus(). mc->valid_cpu_types[] are just part of them 
> >>>> and variable
> >>>> on basis of boards. Generally speaking, we need a function to do reverse 
> >>>> things
> >>>> as to CPUClass::class_by_name(). So I would suggest to introduce 
> >>>> CPUClass::model_from_type(),
> >>>> as below. Could you please suggest if it sounds reasonable to you?
> >>>>
> >>>> - CPUClass::class_by_name() is modified to
> >>>>      char *CPUClass::model_to_type(const char *model)
> >>>>
> >>>> - char *CPUClass::type_to_model(const char *type)
> >>>>
> >>>> - CPUClass::type_to_model() is used in cpu_list() for every target when 
> >>>> CPU
> >>>>      model name, fetched from CPU type name, is printed in 
> >>>> xxx_cpu_list_entry()
> >>>>
> >>>> - CPUClass::type_to_model() is reused in hw/core/machine.c to get the CPU
> >>>>      model name from CPU type names in mc->valid_cpu_types[].  
> >>>
> >>> instead of per target hooks (which are atm mostly open-coded in several 
> >>> places)
> >>> how about adding generic handler for cases 2,4:
> >>>     cpu_type_to_model(typename)
> >>>        cpu_suffix = re'-*-cpu'
> >>>        if (class_exists(typename - cpu_suffix))
> >>>            return typename - cpu_suffix
> >>>        else if (class_exists(typename))
> >>>            return typename
> >>>        explode
> >>>
> >>> that should work for translating valid_cpus typenames to cpumodel names
> >>> and once that in place cleanup all open-coded translations with it 
> >>> tree-wide
> >>>
> >>> you can find those easily by:
> >>> git grep _CPU_TYPE_SUFFIX
> >>> git grep query_cpu_definitions
> >>>      
> >>
> >> Thanks for the advice. I think it's enough for now since the CPU type
> >> invalidation is currently done for arm/mips/riscv for now. On these
> >> targets, the CPU type name is always the combination of the CPU model
> >> name and suffix. I will add helper qemu/cpu.c::cpu_model_by_name()  
> > 
> > cpu_model_from_type() would be describe what function does better.
> >   
> 
> Agreed, thanks.
> 
> >> as you suggested. Note that, the suffix can be gained by ("-" 
> >> CPU_RESOLVING_TYPE)
> >>
> >> Yes, the newly added helper cpu_model_by_name() needs to be applied
> >> to targets where query_cpu_definitions and cpu_list are defined.  
> 
> Thanks,
> Gavin
>