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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 11 Sep 2023 11:43:00 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> 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.

Well cpu model resolving ain't going to work with heterogeneous env.
But otherwise it's argument towards dropping CPU model and use
cpu types directly (then we can get rid all this resolving nonsense).

Though for Gavin's purposes, given existing cpu type naming convention
it's sufficient to just cut of the last 2 '-' from typename to get
cpumodel (target independent and no need for resolving type). 


> 
> > 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;  
> 
>