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

[Gavin Shan <gshan@redhat.com>]

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.

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

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


> 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[].

Thanks,
Gavin