Re: [PATCH 3/4] hw/cpu: Introduce CPUClass::cpu_resolving_type field

[Philippe Mathieu-Daudé]

On 11/9/23 01:28, Gavin Shan wrote:
> Hi Philippe,
>
> On 9/8/23 21:22, Philippe Mathieu-Daudé wrote:
> > Add a field to return the QOM type name of a CPU class.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   include/hw/core/cpu.h   | 2 ++
> >   hw/core/cpu-common.c    | 2 +-
> >   target/alpha/cpu.c      | 1 +
> >   target/arm/cpu.c        | 1 +
> >   target/avr/cpu.c        | 1 +
> >   target/cris/cpu.c       | 1 +
> >   target/hexagon/cpu.c    | 1 +
> >   target/hppa/cpu.c       | 1 +
> >   target/i386/cpu.c       | 1 +
> >   target/loongarch/cpu.c  | 1 +
> >   target/m68k/cpu.c       | 1 +
> >   target/microblaze/cpu.c | 1 +
> >   target/mips/cpu.c       | 1 +
> >   target/nios2/cpu.c      | 1 +
> >   target/openrisc/cpu.c   | 1 +
> >   target/ppc/cpu_init.c   | 1 +
> >   target/riscv/cpu.c      | 1 +
> >   target/rx/cpu.c         | 1 +
> >   target/s390x/cpu.c      | 1 +
> >   target/sh4/cpu.c        | 1 +
> >   target/sparc/cpu.c      | 1 +
> >   target/tricore/cpu.c    | 1 +
> >   target/xtensa/cpu.c     | 1 +
> >   23 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 129d179937..e469efd409 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -100,6 +100,7 @@ struct SysemuCPUOps;
> >   /**
> >    * CPUClass:
> > + * @cpu_resolving_type: CPU QOM type name
> >    * @class_by_name: Callback to map -cpu command line model name to an
> >    *                 instantiatable CPU type.
> >    * @parse_features: Callback to parse command line arguments.
> > @@ -148,6 +149,7 @@ struct CPUClass {
> >       DeviceClass parent_class;
> >       /*< public >*/
> > +    const char *cpu_resolving_type;
> >       ObjectClass *(*class_by_name)(const char *cpu_model);
> >       void (*parse_features)(const char *typename, char *str, Error 
> > **errp);
>
> The question is why not use CPU_RESOLVING_TYPE directly? It seems 
> CPU_RESOLVING_TYPE
> is exactly what you want here.

Unfortunately CPU_RESOLVING_TYPE is target-specific, we want
hw/core/cpu-common.c to be target-agnostic (build once for all
targets). This is particularly important in the context of
heterogeneous QEMU, where a single binary will be able to create
CPUs from different targets.

> Besides, I guess the changes can be 
> squeezed into two
> patches (commits) as below:
>
> PATCH[1] target/alpha: Tidy up alpha_cpu_class_by_name()
> PATCH[2] Move the checks (oc == NULL || object_class_is_abstract() || 
> !object_class_dynamic_cast())
>           from individual targets to 
> hw/core/cpu-common.c::cpu_class_by_name()
>
> I rebase my series of '[PATCH v3 00/32] Unified CPU type check' on top 
> of yours. Please
> let me know if I need to include your patch into my v4 series for 
> review. In that case,
> I can include your patches with above changes applied.
>
> Thanks,
> Gavin
>
> > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > index c6a0c9390c..2d24261a6a 100644
> > --- a/hw/core/cpu-common.c
> > +++ b/hw/core/cpu-common.c
> > @@ -155,7 +155,7 @@ ObjectClass *cpu_class_by_name(const char 
> > *typename, const char *cpu_model)
> >       assert(cpu_model);
> >       oc = object_class_by_name(typename);
> >       cc = CPU_CLASS(oc);
> > -    assert(cc->class_by_name);
> > +    assert(cc->cpu_resolving_type && cc->class_by_name);
> >       oc = cc->class_by_name(cpu_model);
> >       if (oc == NULL || object_class_is_abstract(oc)) {
> >           return NULL;