Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU

[Claudio Fontana]

On 11/18/20 11:07 PM, Eduardo Habkost wrote:
> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote:
>> On 18/11/20 18:30, Eduardo Habkost wrote:
>>>> Adding a layer of indirect calls is not very different from monkey patching
>>>> though.
>>>
>>> I'm a little bothered by monkey patching, but I'm more
>>> bothered by having to:
>>>
>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that
>>>    (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) 
>>> that
>>>      (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel 
>>> kvm_cpu_accel) that
>>>        (4) will be saved in multiple QOM classes, so that
>>>          (5) we will call the right X86CPUClass.accel method at the right 
>>> moment
>>>              (common_class_init(), instance_init(), realizefn()),
>>> where:
>>>    step 4 must be done before any CPU object is created
>>>      (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn
>>>       will be silently ignored), and
>>>    step 3 must be done after all QOM types were registered.
>>>
>>>> You also have to consider that accel currently does not exist in usermode
>>>> emulators, so that's an issue too. I would rather get a simple change in
>>>> quickly, instead of designing the perfect class hierarchy.
>>>
>>> It doesn't have to be perfect.  I agree that simple is better.
>>>
>>> To me, registering a QOM type and looking it up when necessary is
>>> simpler than the above.  Even if it's a weird class having no
>>> object instances.  It probably could be an interface type.
>>
>> Registering a QOM type still has quite some boilerplate.  [...]
> 
> We're working on that.  :)
> 
>>                                                    [...]  Also registering a
>> QOM type has a public side effect (shows up in qom-list-types).  In general
>> I don't look at QOM unless I want its property mechanism, but maybe that's
>> just me.
> 
> We have lots of internal-use-only types returned by
> qom-list-types, I don't think it's a big deal.
> 
>>
>>>> Perhaps another idea would be to allow adding interfaces to classes
>>>> *separately from the registration of the types*. Then we can use it to add
>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and
>>>> add the accel object to usermode emulators.
>>>
>>> I'm not sure I follow.  What do you mean by bare bones accel
>>> class, and when exactly would you add the new interfaces to the
>>> classes?
>>
>> A bare bones accel class would not have init_machine and setup_post methods;
>> those would be in a TYPE_SOFTMMU_ACCEL interface.  It would still have
>> properties (such as tb-size for TCG) and would be able to register compat
>> properties.
> 
> Oh, I think I see.  This could save us having a lot of parallel type
> hierarchies.
> 
>>
>> Where would I add it, I don't know.  It could be a simple public wrapper
>> around type_initialize_interface() if we add a new MODULE_INIT_* phase after
>> QOM.
>>
>> Or without adding a new phase, it could be a class_type->array of
>> (interface_type, init_fn) hash table.  type_initialize would look up the
>> class_type by name, add the interfaces would to the class with
>> type_initialize_interface, and then call the init_fn to fill in the vtable.
> 
> That sounds nice.  I don't think Claudio's cleanup should be
> blocked until this new mechanism is ready, though.
> 
> We don't really need the type representing X86CPUAccel to be a
> subtype of TYPE_ACCEL or an interface implemented by
> current_machine->accelerator, in the first version.  We just need
> a simple way for the CPU initialization code to find the correct
> X86CPUAccel struct.
> 
> While we don't have the new mechanism, it can be just a:
>   object_class_by_name("%s-x86-cpu-accel" % (accel->name))
> call.
> 
> Below is a rough draft of what I mean.  There's still lots of
> room for cleaning it up (especially getting rid of the
> X86CPUClass.common_class_init and X86CPUClass.accel fields).
> 
> git tree at 
> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 485eda986a..944d403cbd 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -44,7 +44,6 @@ typedef enum {
>      MODULE_INIT_BLOCK,
>      MODULE_INIT_OPTS,
>      MODULE_INIT_QOM,
> -    MODULE_INIT_ACCEL_CPU,
>      MODULE_INIT_TRACE,
>      MODULE_INIT_XEN_BACKEND,
>      MODULE_INIT_LIBQOS,
> @@ -55,7 +54,6 @@ typedef enum {
>  #define block_init(function) module_init(function, MODULE_INIT_BLOCK)
>  #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
>  #define type_init(function) module_init(function, MODULE_INIT_QOM)
> -#define accel_cpu_init(function) module_init(function, MODULE_INIT_ACCEL_CPU)
>  #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
>  #define xen_backend_init(function) module_init(function, \
>                                                 MODULE_INIT_XEN_BACKEND)
> diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
> index 032169ccd3..14491297bb 100644
> --- a/include/sysemu/cpus.h
> +++ b/include/sysemu/cpus.h
> @@ -25,6 +25,9 @@ typedef struct CpuAccelOps {
>  /* register accel-specific cpus interface implementation */
>  void cpus_register_accel(const CpuAccelOps *i);
>  
> +/* Call arch-specific accel initialization */
> +void cpu_accel_arch_init(const char *accel_name);
> +
>  /* Create a dummy vcpu for CpuAccelOps->create_vcpu_thread */
>  void dummy_start_vcpu_thread(CPUState *);
>  
> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
> index 79fcbd3b9b..eafd86dc22 100644
> --- a/target/i386/cpu-qom.h
> +++ b/target/i386/cpu-qom.h
> @@ -34,7 +34,7 @@ OBJECT_DECLARE_TYPE(X86CPU, X86CPUClass,
>                      X86_CPU)
>  
>  typedef struct X86CPUModel X86CPUModel;
> -typedef struct X86CPUAccel X86CPUAccel;
> +typedef struct X86CPUAccelInterface X86CPUAccelInterface;
>  
>  /**
>   * X86CPUClass:
> @@ -71,13 +71,11 @@ struct X86CPUClass {
>      DeviceUnrealize parent_unrealize;
>      DeviceReset parent_reset;
>  
> -    const X86CPUAccel *accel;
> +    const X86CPUAccelInterface *accel;
>  };
>  
>  /**
> - * X86CPUAccel:
> - * @name: string name of the X86 CPU Accelerator
> - *
> + * X86CPUAccelInterface:
>   * @common_class_init: initializer for the common cpu
>   * @instance_init: cpu instance initialization
>   * @realizefn: realize function, called first in x86 cpu realize
> @@ -85,14 +83,16 @@ struct X86CPUClass {
>   * X86 CPU accelerator-specific CPU initializations
>   */
>  
> -struct X86CPUAccel {
> -    const char *name;
> -
> +struct X86CPUAccelInterface {
> +    ObjectClass parent_class;
>      void (*common_class_init)(X86CPUClass *xcc);
>      void (*instance_init)(X86CPU *cpu);
>      void (*realizefn)(X86CPU *cpu, Error **errp);
>  };
>  
> -void x86_cpu_accel_init(const X86CPUAccel *accel);
> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel"
> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL);


I am not exactly sure what precisely you are doing here,

I get the general intention to use the existing interface-related "stuff" in 
QOM,
but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like 
the other boilerplate used for interfaces.

Could you clarify what happens here? Should we just use a normal object, call 
it "Interface" and call it a day?

Thanks,

Claudio


> +
> +#define X86_CPU_ACCEL_NAME(acc) (acc "-x86-cpu-accel")
>  
>  #endif
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 9f88ae952a..6107c8ca24 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -909,7 +909,8 @@ int main(int argc, char **argv)
>  
>      /* init tcg before creating CPUs and to get qemu_host_page_size */
>      tcg_exec_init(0);
> -    module_call_init(MODULE_INIT_ACCEL_CPU);
> +    cpu_accel_arch_init("tcg");
> +
>  
>      cpu_type = parse_cpu_option(cpu_model);
>      cpu = cpu_create(cpu_type);
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a745901d86..c36564fd61 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -704,7 +704,7 @@ int main(int argc, char **argv, char **envp)
>  
>      /* init tcg before creating CPUs and to get qemu_host_page_size */
>      tcg_exec_init(0);
> -    module_call_init(MODULE_INIT_ACCEL_CPU);
> +    cpu_accel_arch_init("tcg");
>  
>      cpu = cpu_create(cpu_type);
>      env = cpu->env_ptr;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index df4bed056a..b90d107475 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2744,6 +2744,7 @@ static int do_configure_accelerator(void *opaque, 
> QemuOpts *opts, Error **errp)
>          return 0;
>      }
>  
> +    cpu_accel_arch_init(acc);
>      return 1;
>  }
>  
> @@ -4173,12 +4174,6 @@ void qemu_init(int argc, char **argv, char **envp)
>       */
>      configure_accelerators(argv[0]);
>  
> -    /*
> -     * accelerator has been chosen and initialized, now it is time to
> -     * register the cpu accel interface.
> -     */
> -    module_call_init(MODULE_INIT_ACCEL_CPU);
> -
>      /*
>       * Beware, QOM objects created before this point miss global and
>       * compat properties.
> diff --git a/stubs/cpu_accel_arch_init.c b/stubs/cpu_accel_arch_init.c
> new file mode 100644
> index 0000000000..b80cbdd847
> --- /dev/null
> +++ b/stubs/cpu_accel_arch_init.c
> @@ -0,0 +1,6 @@
> +#include "qemu/osdep.h"
> +#include "sysemu/cpus.h"
> +
> +void cpu_accel_arch_init(const char *accel_name)
> +{
> +}
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b53e958926..b91e0b44ca 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7041,6 +7041,12 @@ static const TypeInfo x86_base_cpu_type_info = {
>          .class_init = x86_cpu_base_class_init,
>  };
>  
> +static const TypeInfo x86_cpu_accel_type_info = {
> +    .name = TYPE_X86_CPU_ACCEL,
> +    .parent = TYPE_INTERFACE,
> +    .class_size = sizeof(X86CPUAccelInterface),
> +};
> +
>  static void x86_cpu_register_types(void)
>  {
>      int i;
> @@ -7051,6 +7057,7 @@ static void x86_cpu_register_types(void)
>      }
>      type_register_static(&max_x86_cpu_type_info);
>      type_register_static(&x86_base_cpu_type_info);
> +    type_register_static(&x86_cpu_accel_type_info);
>  }
>  
>  type_init(x86_cpu_register_types)
> @@ -7058,13 +7065,22 @@ type_init(x86_cpu_register_types)
>  static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
>  {
>      X86CPUClass *xcc = X86_CPU_CLASS(klass);
> -    const X86CPUAccel **accel = opaque;
> +    const X86CPUAccelInterface **accel = opaque;
>  
>      xcc->accel = *accel;
>      xcc->accel->common_class_init(xcc);
>  }
>  
> -void x86_cpu_accel_init(const X86CPUAccel *accel)
> +static void x86_cpu_accel_init(const X86CPUAccelInterface *accel)
>  {
>      object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, 
> &accel);
>  }
> +
> +void cpu_accel_arch_init(const char *accel_name)
> +{
> +    g_autofree char *cpu_accel_name =
> +        g_strdup_printf(X86_CPU_ACCEL_NAME("%s"), accel_name);
> +    X86CPUAccelInterface *acc = 
> X86_CPU_ACCEL_CLASS(object_class_by_name(cpu_accel_name));
> +    assert(acc);
> +    x86_cpu_accel_init(acc);
> +}
> diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c
> index 29e672191f..358351018f 100644
> --- a/target/i386/hvf/cpu.c
> +++ b/target/i386/hvf/cpu.c
> @@ -46,19 +46,23 @@ static void hvf_cpu_instance_init(X86CPU *cpu)
>      }
>  }
>  
> -static const X86CPUAccel hvf_cpu_accel = {
> -    .name = TYPE_X86_CPU "-hvf",
> +static void hvf_cpu_accel_interface_init(ObjectClass *oc, void *data)
> +{
> +    X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc);
> +    acc->realizefn = host_cpu_realizefn;
> +    acc->common_class_init = hvf_cpu_common_class_init;
> +    acc->instance_init = hvf_cpu_instance_init;
> +};
>  
> -    .realizefn = host_cpu_realizefn,
> -    .common_class_init = hvf_cpu_common_class_init,
> -    .instance_init = hvf_cpu_instance_init,
> +static const TypeInfo hvf_cpu_accel_type_info = {
> +    .name = X86_CPU_ACCEL_NAME("hvf"),
> +    .parent = TYPE_X86_CPU_ACCEL,
> +    .class_init = hvf_cpu_accel_interface_init,
>  };
>  
>  static void hvf_cpu_accel_init(void)
>  {
> -    if (hvf_enabled()) {
> -        x86_cpu_accel_init(&hvf_cpu_accel);
> -    }
> +    type_register_static(&hvf_cpu_accel_type_info);
>  }
>  
> -accel_cpu_init(hvf_cpu_accel_init);
> +type_init(hvf_cpu_accel_init);
> diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c
> index 76982865eb..b6a1a4d200 100644
> --- a/target/i386/kvm/cpu.c
> +++ b/target/i386/kvm/cpu.c
> @@ -128,18 +128,23 @@ static void kvm_cpu_instance_init(X86CPU *cpu)
>      }
>  }
>  
> -static const X86CPUAccel kvm_cpu_accel = {
> -    .name = TYPE_X86_CPU "-kvm",
> +static void kvm_cpu_accel_interface_init(ObjectClass *oc, void *data)
> +{
> +    X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc);
> +    acc->realizefn = kvm_cpu_realizefn;
> +    acc->common_class_init = kvm_cpu_common_class_init;
> +    acc->instance_init = kvm_cpu_instance_init;
> +};
>  
> -    .realizefn = kvm_cpu_realizefn,
> -    .common_class_init = kvm_cpu_common_class_init,
> -    .instance_init = kvm_cpu_instance_init,
> +static const TypeInfo kvm_cpu_accel_type_info = {
> +    .name = X86_CPU_ACCEL_NAME("kvm"),
> +    .parent = TYPE_X86_CPU_ACCEL,
> +    .class_init = kvm_cpu_accel_interface_init,
>  };
>  
>  static void kvm_cpu_accel_init(void)
>  {
> -    if (kvm_enabled()) {
> -        x86_cpu_accel_init(&kvm_cpu_accel);
> -    }
> +    type_register_static(&kvm_cpu_accel_type_info);
>  }
> -accel_cpu_init(kvm_cpu_accel_init);
> +
> +type_init(kvm_cpu_accel_init);
> diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c
> index 25cf4cfb46..0321688cd3 100644
> --- a/target/i386/tcg/cpu.c
> +++ b/target/i386/tcg/cpu.c
> @@ -150,19 +150,23 @@ static void tcg_cpu_instance_init(X86CPU *cpu)
>      x86_cpu_apply_props(cpu, tcg_default_props);
>  }
>  
> -static const X86CPUAccel tcg_cpu_accel = {
> -    .name = TYPE_X86_CPU "-tcg",
> +static void tcg_cpu_accel_interface_init(ObjectClass *oc, void *data)
> +{
> +    X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc);
> +    acc->realizefn = tcg_cpu_realizefn;
> +    acc->common_class_init = tcg_cpu_common_class_init;
> +    acc->instance_init = tcg_cpu_instance_init;
> +};
>  
> -    .realizefn = tcg_cpu_realizefn,
> -    .common_class_init = tcg_cpu_common_class_init,
> -    .instance_init = tcg_cpu_instance_init,
> +static const TypeInfo tcg_cpu_accel_type_info = {
> +    .name = X86_CPU_ACCEL_NAME("tcg"),
> +    .parent = TYPE_X86_CPU_ACCEL,
> +    .class_init = tcg_cpu_accel_interface_init,
>  };
>  
>  static void tcg_cpu_accel_init(void)
>  {
> -    if (tcg_enabled()) {
> -        x86_cpu_accel_init(&tcg_cpu_accel);
> -    }
> +    type_register_static(&tcg_cpu_accel_type_info);
>  }
>  
> -accel_cpu_init(tcg_cpu_accel_init);
> +type_init(tcg_cpu_accel_init);
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 82b7ba60ab..1d66de1fae 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -1,4 +1,5 @@
>  stub_ss.add(files('arch_type.c'))
> +stub_ss.add(files('cpu_accel_arch_init.c'))
>  stub_ss.add(files('bdrv-next-monitor-owned.c'))
>  stub_ss.add(files('blk-commit-all.c'))
>  stub_ss.add(files('blk-exp-close-all.c'))
> 
> 
>