Re: [PULL 46/46] accel: introduce AccelCPUClass extending CPUClass

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Claudio,

+Eduardo/Paolo

On 2/5/21 11:56 PM, Richard Henderson wrote:
> From: Claudio Fontana <cfontana@suse.de>
> 
> add a new optional interface to CPUClass, which allows accelerators
> to extend the CPUClass with additional accelerator-specific
> initializations.
> 
> This will allow to separate the target cpu code that is specific
> to each accelerator, and register it automatically with object
> hierarchy lookup depending on accelerator code availability,
> as part of the accel_init_interfaces() initialization step.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> Message-Id: <20210204163931.7358-19-cfontana@suse.de>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/hw/core/accel-cpu.h | 38 ++++++++++++++++++++++++++++++++
>  include/hw/core/cpu.h       |  4 ++++
>  accel/accel-common.c        | 44 +++++++++++++++++++++++++++++++++++++
>  MAINTAINERS                 |  1 +
>  4 files changed, 87 insertions(+)
>  create mode 100644 include/hw/core/accel-cpu.h
> 
> diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h
> new file mode 100644
> index 0000000000..24a6697412
> --- /dev/null
> +++ b/include/hw/core/accel-cpu.h
> @@ -0,0 +1,38 @@
> +/*
> + * Accelerator interface, specializes CPUClass
> + * This header is used only by target-specific code.
> + *
> + * Copyright 2021 SUSE LLC
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef ACCEL_CPU_H
> +#define ACCEL_CPU_H
> +
> +/*
> + * This header is used to define new accelerator-specific target-specific
> + * accelerator cpu subclasses.
> + * It uses CPU_RESOLVING_TYPE, so this is clearly target-specific.
> + *
> + * Do not try to use for any other purpose than the implementation of new
> + * subclasses in target/, or the accel implementation itself in accel/
> + */
> +
> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
> +typedef struct AccelCPUClass AccelCPUClass;
> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
> +
> +typedef struct AccelCPUClass {
> +    /*< private >*/
> +    ObjectClass parent_class;
> +    /*< public >*/
> +
> +    void (*cpu_class_init)(CPUClass *cc);
> +    void (*cpu_instance_init)(CPUState *cpu);
> +    void (*cpu_realizefn)(CPUState *cpu, Error **errp);

We could use a const pointer to const AccelCPUOps.

> +} AccelCPUClass;
> +
> +#endif /* ACCEL_CPU_H */
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 4f6c6b18c9..38d813c389 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -79,6 +79,9 @@ struct TranslationBlock;
>  /* see tcg-cpu-ops.h */
>  struct TCGCPUOps;
>  
> +/* see accel-cpu.h */
> +struct AccelCPUClass;
> +
>  /**
>   * CPUClass:
>   * @class_by_name: Callback to map -cpu command line model name to an
> @@ -187,6 +190,7 @@ struct CPUClass {
>      /* Keep non-pointer data at the end to minimize holes.  */
>      int gdb_num_core_regs;
>      bool gdb_stop_before_watchpoint;
> +    struct AccelCPUClass *accel_cpu;

(so here AccelCPUOps)

>  
>      /* when TCG is not available, this pointer is NULL */
>      struct TCGCPUOps *tcg_ops;
> diff --git a/accel/accel-common.c b/accel/accel-common.c
> index 6b59873419..9901b0531c 100644
> --- a/accel/accel-common.c
> +++ b/accel/accel-common.c
> @@ -26,6 +26,9 @@
>  #include "qemu/osdep.h"
>  #include "qemu/accel.h"
>  
> +#include "cpu.h"
> +#include "hw/core/accel-cpu.h"
> +
>  #ifndef CONFIG_USER_ONLY
>  #include "accel-softmmu.h"
>  #endif /* !CONFIG_USER_ONLY */
> @@ -46,16 +49,57 @@ AccelClass *accel_find(const char *opt_name)
>      return ac;
>  }
>  
> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
> +{
> +    CPUClass *cc = CPU_CLASS(klass);
> +    AccelCPUClass *accel_cpu = opaque;
> +
> +    cc->accel_cpu = accel_cpu;
> +    if (accel_cpu->cpu_class_init) {
> +        accel_cpu->cpu_class_init(cc);
> +    }
> +}
> +
> +/* initialize the arch-specific accel CpuClass interfaces */
> +static void accel_init_cpu_interfaces(AccelClass *ac)
> +{
> +    const char *ac_name; /* AccelClass name */
> +    char *acc_name;      /* AccelCPUClass name */
> +    ObjectClass *acc;    /* AccelCPUClass */
> +
> +    ac_name = object_class_get_name(OBJECT_CLASS(ac));
> +    g_assert(ac_name != NULL);
> +
> +    acc_name = g_strdup_printf("%s-%s", ac_name, CPU_RESOLVING_TYPE);
> +    acc = object_class_by_name(acc_name);
> +    g_free(acc_name);
> +
> +    if (acc) {
> +        object_class_foreach(accel_init_cpu_int_aux,
> +                             CPU_RESOLVING_TYPE, false, acc);
> +    }
> +}
> +
>  void accel_init_interfaces(AccelClass *ac)
>  {
>  #ifndef CONFIG_USER_ONLY
>      accel_init_ops_interfaces(ac);
>  #endif /* !CONFIG_USER_ONLY */
> +
> +    accel_init_cpu_interfaces(ac);
>  }
>  
> +static const TypeInfo accel_cpu_type = {
> +    .name = TYPE_ACCEL_CPU,
> +    .parent = TYPE_OBJECT,
> +    .abstract = true,

I'm not convince the abstract QOM parent has to be
per target. All methods in AccelCPUClass use pointers,
so we should be fine with opaque CPU* pointer declarations,
and have one common type for all targets.

> +    .class_size = sizeof(AccelCPUClass),
> +};
> +
>  static void register_accel_types(void)
>  {
>      type_register_static(&accel_type);
> +    type_register_static(&accel_cpu_type);
>  }