Re: [PATCH 0/3] hw/arm/virt: Use generic CPU invalidation

[Gavin Shan]

On 7/14/23 10:51, Gavin Shan wrote:
> On 7/14/23 02:29, Philippe Mathieu-Daudé wrote:
> > On 13/7/23 14:34, Gavin Shan wrote:
> > > On 7/13/23 21:52, Marcin Juszkiewicz wrote:
> > > > W dniu 13.07.2023 o 13:44, Peter Maydell pisze:
> > > >
> > > > > I see this isn't a change in this patch, but given that
> > > > > what the user specifies is not "cortex-a8-arm-cpu" but
> > > > > "cortex-a8", why do we include the "-arm-cpu" suffix in
> > > > > the error messages? It's not valid syntax to say
> > > > > "-cpu cortex-a8-arm-cpu", so it's a bit misleading...
> > > >
> > > > Internally those cpu names are "max-{TYPE_ARM_CPU}" and similar for other 
> > > > architectures.
> > > >
> > > > I like the change but it (IMHO) needs to cut "-{TYPE_*_CPU}" string from names:
> > > >
> > > > 13:37 marcin@applejack:qemu$ ./build/aarch64-softmmu/qemu-system-aarch64 -M 
> > > > virt -cpu cortex-r5
> > > > qemu-system-aarch64: Invalid CPU type: cortex-r5-arm-cpu
> > > > The valid types are: cortex-a7-arm-cpu, cortex-a15-arm-cpu, cortex-a35-arm-cpu, 
> > > > cortex-a55-arm-cpu, cortex-a72-arm-cpu, cortex-a76-arm-cpu, a64fx-arm-cpu, 
> > > > neoverse-n1-arm-cpu, neoverse-v1-arm-cpu, cortex-a53-arm-cpu, 
> > > > cortex-a57-arm-cpu, host-arm-cpu, max-arm-cpu
> > > >
> > > > 13:37 marcin@applejack:qemu$ ./build/aarch64-softmmu/qemu-system-aarch64 -M 
> > > > virt -cpu cortex-a57-arm-cpu
> > > > qemu-system-aarch64: unable to find CPU model 'cortex-a57-arm-cpu'
> > >
> > > The suffix of CPU types are provided in hw/arm/virt.c::valid_cpu_types in 
> > > PATCH[2].
> > > In the generic validation, the complete CPU type is used. The error message also
> > > have complete CPU type there.
> >
> > In some places (arm_cpu_list_entry, arm_cpu_add_definition) we use:
> >
> >    g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU))
> >
> > Maybe extract as a helper? cpu_typename_name()? :)
>
> Yeah, it's definitely a good idea. The helper is needed by all architectures,
> not ARM alone. The following CPU types don't have explicit definition of
> XXXX_CPU_TYPE_SUFFIX. We need take "-" TYPE_CPU as the suffix.
>
>      target/microblaze/cpu.c  TYPE_MICROBLAZE_CPU
>      target/hppa/cpu.c        TYPE_HPPA_CPU
>      target/nios2/cpu.c       TYPE_NIOS2_CPU
>
>      target/microblaze/cpu-qom.h:#define TYPE_MICROBLAZE_CPU "microblaze-cpu"
>      target/hppa/cpu-qom.h:      #define TYPE_HPPA_CPU       "hppa-cpu"
>      target/nios2/cpu.h:         #define TYPE_NIOS2_CPU      "nios2-cpu"
>
> I think the function name can be cpu_model_name() since we have called it
> as 'model' in cpu.c::parse_cpu_option(). Something like below. Please let
> me know if you have more comments.
>
>      target/xxxx/cpu.h
>      -----------------
>
>      static inline char *cpu_model_name(const char *typename)
>      {
>          return g_strndup(typename, strlen(typename) - 
> strlen(TYPE_XXX_CPU_SUFFIX));
>      }

I found the generic CPU type invalidation in hw/core/machine.c can't see 
functions
from target/xxx/, including cpu_model_name(). In order to call this function 
from
hw/core/machine.c, we need transit in cpu.c

    include/exec/cpu-common.h
    -------------------------
    char *cpu_get_model_name(const char *name);
    void list_cpus(void);
    
    cpu.c
    -----
    char *cpu_get_model_name(const char *name)
    {
       return cpu_model_name(name);
    }

With above hunk of changes, cpu_get_model_name() can be called in 
hw/core/machine.c,
to extract the CPU model name from the CPU type name.

Thanks,
Gavin