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

[Gavin Shan]

On 9/11/23 19:43, Philippe Mathieu-Daudé wrote:
> On 11/9/23 01:28, Gavin Shan wrote:
> > 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.

CPU_RESOLVING_TYPE and CPUClass::cpu_resolving_type is duplicate to
each other. There are two options I can figure out to avoid the
duplication.

(a) move cpu_class_by_name() from hw/core/cpu-common.c to cpu.c, so that
    CPU_RESOLVING_TYPE can be seen. cpu.c::list_cpus() is the example.

(b) remove hw/core/cpu-common.c::cpu_calss_by_name() and squeeze its
    logic to cpu.c::parse_cpu_option() since there are not too much
    users for it. target/arm and target/s390 needs some tweaks so that
    hw/core/cpu-common.c::cpu_calss_by_name() can be removed.

    [gshan@gshan q]$ git grep \ cpu_class_by_name\(
    cpu.c:    oc = cpu_class_by_name(CPU_RESOLVING_TYPE, model_pieces[0]);
    target/arm/arm-qmp-cmds.c:    oc = cpu_class_by_name(TYPE_ARM_CPU, 
model->name);
    target/s390x/cpu_models_sysemu.c:    oc = cpu_class_by_name(TYPE_S390_CPU, 
info->name);

When option (b) is taken, this series to have the checks against @oc
in hw/core/cpu-common.c::cpu_calss_by_name() becomes non-sense. Instead,
we need the same (and complete) checks in CPUClass::class_by_name() for
individual targets. Further more, an inline helper can be provided to do
the check in CPUClass::class_by_name() for individual targets.

   include/hw/core/cpu.h

   static inline bool cpu_class_is_valid(ObjectClass *oc, const char *parent)
   {
       if (!object_class_dynamic_cast(oc, parent) ||
           object_class_is_abstract(oc)) {
           return false;
       }

       return true;
   }

Thanks,
Gavin