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

[Gavin Shan]

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