Re: [PATCH v4 03/33] cpu: Call object_class_dynamic_cast() once in cpu_class_by_name()

[Gavin Shan]

On 11/2/23 13:52, Richard Henderson wrote:
> On 11/1/23 17:24, Gavin Shan wrote:
> > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> > For all targets, the CPU class returned from CPUClass::class_by_name()
> > and object_class_dynamic_cast(oc, CPU_RESOLVING_TYPE) need to be
> > compatible. Lets apply the check in cpu_class_by_name() for once,
> > instead of having the check in CPUClass::class_by_name() for individual
> > target. In order to make CPU_RESOLVING_TYPE visible to cpu_class_by_name(),
> > the helper has to be moved to cpu-target.c
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   cpu-target.c           | 15 +++++++++++++++
> >   hw/core/cpu-common.c   | 14 --------------
> >   target/arm/cpu.c       |  4 +---
> >   target/avr/cpu.c       |  8 +-------
> >   target/cris/cpu.c      |  4 +---
> >   target/hexagon/cpu.c   |  4 +---
> >   target/loongarch/cpu.c |  8 +-------
> >   target/m68k/cpu.c      |  4 +---
> >   target/openrisc/cpu.c  |  4 +---
> >   target/riscv/cpu.c     |  4 +---
> >   target/tricore/cpu.c   |  4 +---
> >   target/xtensa/cpu.c    |  4 +---
> >   12 files changed, 25 insertions(+), 52 deletions(-)
> >
> > diff --git a/cpu-target.c b/cpu-target.c
> > index 79363ae370..876b498233 100644
> > --- a/cpu-target.c
> > +++ b/cpu-target.c
> > @@ -250,6 +250,21 @@ void cpu_exec_initfn(CPUState *cpu)
> >   #endif
> >   }
> > +ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
> > +{
> > +    CPUClass *cc = CPU_CLASS(object_class_by_name(typename));
> > +    ObjectClass *oc;
> > +
> > +    assert(cpu_model && cc->class_by_name);
> > +    oc = cc->class_by_name(cpu_model);
> > +    if (oc && !object_class_is_abstract(oc) &&
> > +        object_class_dynamic_cast(oc, CPU_RESOLVING_TYPE)) {
> > +        return oc;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >   const char *parse_cpu_option(const char *cpu_option)
> >   {
> >       ObjectClass *oc;
> > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > index bca0323e9f..1024de53bb 100644
> > --- a/hw/core/cpu-common.c
> > +++ b/hw/core/cpu-common.c
> > @@ -147,20 +147,6 @@ static bool cpu_common_has_work(CPUState *cs)
> >       return false;
> >   }
> > -ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
> > -{
> > -    CPUClass *cc = CPU_CLASS(object_class_by_name(typename));
> > -    ObjectClass *oc;
> > -
> > -    assert(cpu_model && cc->class_by_name);
> > -    oc = cc->class_by_name(cpu_model);
> > -    if (oc && !object_class_is_abstract(oc)) {
> > -        return oc;
> > -    }
> > -
> > -    return NULL;
> > -}
>
> This move is unnecessary: typename == CPU_RESOLVING_TYPE.
>
> Indeed, since you've grabbed class_by_name from CPUClass, it is completely 
> logical that that's the base class against which we should verify.

Good point. I just realized typename == CPU_RESOLVING_TYPE, so the movement
isn't needed at all.

[gshan@gshan q]$ git grep \ cpu_class_by_name\(
cpu-target.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);

[gshan@gshan q]$ git grep 'define CPU_RESOLVING_TYPE' target/arm/*
target/arm/cpu.h:#define CPU_RESOLVING_TYPE TYPE_ARM_CPU
[gshan@gshan q]$ git grep 'define CPU_RESOLVING_TYPE' target/s390x/*
target/s390x/cpu.h:#define CPU_RESOLVING_TYPE TYPE_S390_CPU


Thanks,
Gavin