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 <ehabk...@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'))
>
>
>
