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

[Gavin Shan <gshan@redhat.com>]

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. 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;
> diff --git a/target/alpha/cpu.c b/target/alpha/cpu.c
> index 351ee2e9f2..0ddea8004c 100644
> --- a/target/alpha/cpu.c
> +++ b/target/alpha/cpu.c
> @@ -254,6 +254,7 @@ static void alpha_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_realize(dc, alpha_cpu_realizefn,
>                                       &acc->parent_realize);
>   
> +    cc->cpu_resolving_type = TYPE_ALPHA_CPU;
>       cc->class_by_name = alpha_cpu_class_by_name;
>       cc->has_work = alpha_cpu_has_work;
>       cc->dump_state = alpha_cpu_dump_state;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 42e29816cc..9e51bde170 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2377,6 +2377,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, arm_cpu_reset_hold, NULL,
>                                          &acc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_ARM_CPU;
>       cc->class_by_name = arm_cpu_class_by_name;
>       cc->has_work = arm_cpu_has_work;
>       cc->dump_state = arm_cpu_dump_state;
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 4b255eade1..f6004169ac 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -233,6 +233,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, avr_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_AVR_CPU;
>       cc->class_by_name = avr_cpu_class_by_name;
>   
>       cc->has_work = avr_cpu_has_work;
> diff --git a/target/cris/cpu.c b/target/cris/cpu.c
> index 115f6e2ea2..adde4f599d 100644
> --- a/target/cris/cpu.c
> +++ b/target/cris/cpu.c
> @@ -314,6 +314,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, cris_cpu_reset_hold, NULL,
>                                          &ccc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_CRIS_CPU;
>       cc->class_by_name = cris_cpu_class_by_name;
>       cc->has_work = cris_cpu_has_work;
>       cc->dump_state = cris_cpu_dump_state;
> diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
> index 5e301327d3..2d4fed838d 100644
> --- a/target/hexagon/cpu.c
> +++ b/target/hexagon/cpu.c
> @@ -381,6 +381,7 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, hexagon_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_HEXAGON_CPU;
>       cc->class_by_name = hexagon_cpu_class_by_name;
>       cc->has_work = hexagon_cpu_has_work;
>       cc->dump_state = hexagon_dump_state;
> diff --git a/target/hppa/cpu.c b/target/hppa/cpu.c
> index 11022f9c99..47950a15ae 100644
> --- a/target/hppa/cpu.c
> +++ b/target/hppa/cpu.c
> @@ -192,6 +192,7 @@ static void hppa_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_realize(dc, hppa_cpu_realizefn,
>                                       &acc->parent_realize);
>   
> +    cc->cpu_resolving_type = TYPE_HPPA_CPU;
>       cc->class_by_name = hppa_cpu_class_by_name;
>       cc->has_work = hppa_cpu_has_work;
>       cc->dump_state = hppa_cpu_dump_state;
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 00f913b638..9979464420 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7951,6 +7951,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>                                          &xcc->parent_phases);
>       cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP;
>   
> +    cc->cpu_resolving_type = TYPE_X86_CPU;
>       cc->class_by_name = x86_cpu_class_by_name;
>       cc->parse_features = x86_cpu_parse_featurestr;
>       cc->has_work = x86_cpu_has_work;
> diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> index fe2e5ecc46..189dfd32d1 100644
> --- a/target/loongarch/cpu.c
> +++ b/target/loongarch/cpu.c
> @@ -743,6 +743,7 @@ static void loongarch_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, loongarch_cpu_reset_hold, NULL,
>                                          &lacc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_LOONGARCH_CPU;
>       cc->class_by_name = loongarch_cpu_class_by_name;
>       cc->has_work = loongarch_cpu_has_work;
>       cc->dump_state = loongarch_cpu_dump_state;
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index 004f3d6265..bd7bb103d7 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -558,6 +558,7 @@ static void m68k_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, m68k_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_M68K_CPU;
>       cc->class_by_name = m68k_cpu_class_by_name;
>       cc->has_work = m68k_cpu_has_work;
>       cc->dump_state = m68k_cpu_dump_state;
> diff --git a/target/microblaze/cpu.c b/target/microblaze/cpu.c
> index 03c2c4db1f..bb5f2c1f00 100644
> --- a/target/microblaze/cpu.c
> +++ b/target/microblaze/cpu.c
> @@ -414,6 +414,7 @@ static void mb_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, mb_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_MICROBLAZE_CPU;
>       cc->class_by_name = mb_cpu_class_by_name;
>       cc->has_work = mb_cpu_has_work;
>   
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 63da1948fd..649147df2e 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -578,6 +578,7 @@ static void mips_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, mips_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_MIPS_CPU;
>       cc->class_by_name = mips_cpu_class_by_name;
>       cc->has_work = mips_cpu_has_work;
>       cc->dump_state = mips_cpu_dump_state;
> diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
> index bc5cbf81c2..fc7c6a83ee 100644
> --- a/target/nios2/cpu.c
> +++ b/target/nios2/cpu.c
> @@ -381,6 +381,7 @@ static void nios2_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, nios2_cpu_reset_hold, NULL,
>                                          &ncc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_NIOS2_CPU;
>       cc->class_by_name = nios2_cpu_class_by_name;
>       cc->has_work = nios2_cpu_has_work;
>       cc->dump_state = nios2_cpu_dump_state;
> diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c
> index 3bbbcc4e63..5e1e0576e0 100644
> --- a/target/openrisc/cpu.c
> +++ b/target/openrisc/cpu.c
> @@ -243,6 +243,7 @@ static void openrisc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, openrisc_cpu_reset_hold, NULL,
>                                          &occ->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_OPENRISC_CPU;
>       cc->class_by_name = openrisc_cpu_class_by_name;
>       cc->has_work = openrisc_cpu_has_work;
>       cc->dump_state = openrisc_cpu_dump_state;
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 02b7aad9b0..bc106d01a2 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -7357,6 +7357,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, ppc_cpu_reset_hold, NULL,
>                                          &pcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_POWERPC_CPU;
>       cc->class_by_name = ppc_cpu_class_by_name;
>       cc->has_work = ppc_cpu_has_work;
>       cc->dump_state = ppc_cpu_dump_state;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 17b00eb7c0..e8f04ef82b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2130,6 +2130,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>       resettable_class_set_parent_phases(rc, NULL, riscv_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_RISCV_CPU;
>       cc->class_by_name = riscv_cpu_class_by_name;
>       cc->has_work = riscv_cpu_has_work;
>       cc->dump_state = riscv_cpu_dump_state;
> diff --git a/target/rx/cpu.c b/target/rx/cpu.c
> index c98034540d..2a6df418a8 100644
> --- a/target/rx/cpu.c
> +++ b/target/rx/cpu.c
> @@ -222,6 +222,7 @@ static void rx_cpu_class_init(ObjectClass *klass, void *data)
>       resettable_class_set_parent_phases(rc, NULL, rx_cpu_reset_hold, NULL,
>                                          &rcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_RX_CPU;
>       cc->class_by_name = rx_cpu_class_by_name;
>       cc->has_work = rx_cpu_has_work;
>       cc->dump_state = rx_cpu_dump_state;
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index df167493c3..bcba466bb4 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -336,6 +336,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>       device_class_set_parent_reset(dc, s390_cpu_reset_full, &scc->parent_reset);
>   
>       scc->reset = s390_cpu_reset;
> +    cc->cpu_resolving_type = TYPE_S390_CPU;
>       cc->class_by_name = s390_cpu_class_by_name,
>       cc->has_work = s390_cpu_has_work;
>       cc->dump_state = s390_cpu_dump_state;
> diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c
> index bc112776fc..17c87f15f2 100644
> --- a/target/sh4/cpu.c
> +++ b/target/sh4/cpu.c
> @@ -283,6 +283,7 @@ static void superh_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, superh_cpu_reset_hold, NULL,
>                                          &scc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_SUPERH_CPU;
>       cc->class_by_name = superh_cpu_class_by_name;
>       cc->has_work = superh_cpu_has_work;
>       cc->dump_state = superh_cpu_dump_state;
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index 130ab8f578..e41a9f4ee2 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -902,6 +902,7 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, sparc_cpu_reset_hold, NULL,
>                                          &scc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_SPARC_CPU;
>       cc->class_by_name = sparc_cpu_class_by_name;
>       cc->parse_features = sparc_cpu_parse_features;
>       cc->has_work = sparc_cpu_has_work;
> diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c
> index a2381b0dc1..ffe5158786 100644
> --- a/target/tricore/cpu.c
> +++ b/target/tricore/cpu.c
> @@ -202,6 +202,7 @@ static void tricore_cpu_class_init(ObjectClass *c, void *data)
>   
>       resettable_class_set_parent_phases(rc, NULL, tricore_cpu_reset_hold, NULL,
>                                          &mcc->parent_phases);
> +    cc->cpu_resolving_type = TYPE_TRICORE_CPU;
>       cc->class_by_name = tricore_cpu_class_by_name;
>       cc->has_work = tricore_cpu_has_work;
>   
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index a31825a2b5..13bed05d0c 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -252,6 +252,7 @@ static void xtensa_cpu_class_init(ObjectClass *oc, void *data)
>       resettable_class_set_parent_phases(rc, NULL, xtensa_cpu_reset_hold, NULL,
>                                          &xcc->parent_phases);
>   
> +    cc->cpu_resolving_type = TYPE_XTENSA_CPU;
>       cc->class_by_name = xtensa_cpu_class_by_name;
>       cc->has_work = xtensa_cpu_has_work;
>       cc->dump_state = xtensa_cpu_dump_state;