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); + +#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')) -- Eduardo