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

[Gavin Shan <gshan@redhat.com>]

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().


>> 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