On 11/24/20 6:48 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 05:22:10PM +0100, Claudio Fontana wrote: >> Signed-off-by: Claudio Fontana <cfont...@suse.de> >> --- >> accel/kvm/kvm-all.c | 9 ------- >> accel/kvm/kvm-cpus.c | 26 +++++++++++++----- >> accel/kvm/kvm-cpus.h | 2 -- >> accel/qtest/qtest.c | 31 ++++++++++++---------- >> accel/tcg/tcg-cpus-icount.c | 11 +------- >> accel/tcg/tcg-cpus-icount.h | 2 ++ >> accel/tcg/tcg-cpus-mttcg.c | 12 +++------ >> accel/tcg/tcg-cpus-mttcg.h | 19 ++++++++++++++ >> accel/tcg/tcg-cpus-rr.c | 7 ----- >> accel/tcg/tcg-cpus.c | 48 ++++++++++++++++++++++++++------- >> accel/tcg/tcg-cpus.h | 4 --- >> accel/xen/xen-all.c | 29 ++++++++++---------- >> include/sysemu/cpus.h | 39 ++++++++++++++++++++------- >> softmmu/cpus.c | 51 +++++++++++++++++++++++++++++------- >> target/i386/hax/hax-all.c | 9 ------- >> target/i386/hax/hax-cpus.c | 29 +++++++++++++++----- >> target/i386/hax/hax-cpus.h | 2 -- >> target/i386/hvf/hvf-cpus.c | 27 ++++++++++++++----- >> target/i386/hvf/hvf-cpus.h | 2 -- >> target/i386/hvf/hvf.c | 9 ------- >> target/i386/whpx/whpx-all.c | 9 ------- >> target/i386/whpx/whpx-cpus.c | 29 +++++++++++++++----- >> target/i386/whpx/whpx-cpus.h | 2 -- >> 23 files changed, 251 insertions(+), 157 deletions(-) >> create mode 100644 accel/tcg/tcg-cpus-mttcg.h >> >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> index 509b249f52..33156cc4c7 100644 >> --- a/accel/kvm/kvm-all.c >> +++ b/accel/kvm/kvm-all.c >> @@ -3234,12 +3234,3 @@ static void kvm_type_init(void) >> } >> >> type_init(kvm_type_init); >> - >> -static void kvm_accel_cpu_init(void) >> -{ >> - if (kvm_enabled()) { >> - cpus_register_accel(&kvm_cpus); >> - } >> -} >> - >> -accel_cpu_init(kvm_accel_cpu_init); >> diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-cpus.c >> index d809b1e74c..33dc8e737a 100644 >> --- a/accel/kvm/kvm-cpus.c >> +++ b/accel/kvm/kvm-cpus.c >> @@ -74,11 +74,25 @@ static void kvm_start_vcpu_thread(CPUState *cpu) >> cpu, QEMU_THREAD_JOINABLE); >> } >> >> -const CpusAccel kvm_cpus = { >> - .create_vcpu_thread = kvm_start_vcpu_thread, >> +static void kvm_cpus_class_init(ObjectClass *oc, void *data) >> +{ >> + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); > > Why do you need a separate QOM type hierarchy instead of just > doing this inside AccelClass methods and/or existing accel > class_init functions? > >> >> - .synchronize_post_reset = kvm_cpu_synchronize_post_reset, >> - .synchronize_post_init = kvm_cpu_synchronize_post_init, >> - .synchronize_state = kvm_cpu_synchronize_state, >> - .synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm, >> + ops->create_vcpu_thread = kvm_start_vcpu_thread; >> + ops->synchronize_post_reset = kvm_cpu_synchronize_post_reset; >> + ops->synchronize_post_init = kvm_cpu_synchronize_post_init; >> + ops->synchronize_state = kvm_cpu_synchronize_state; >> + ops->synchronize_pre_loadvm = kvm_cpu_synchronize_pre_loadvm; > > All of these could be AccelClass fields.
Makes sense, and I like also the idea below (to have a pointer from the AccelClass to the Ops). I'll give both a try. > > TCG makes it a bit more complicated because there's a different > set of methods chosen for TYPE_TCG_ACCEL depending on the > configuration. Right, that was a bit painful, > This could be solved by patching AccelClass at > init time, or by moving the method pointers to AccelState. Ok I'll experiment here. > > Alternatively, if you still want to keep the > CpusAccel/CpusAccelOps struct, that's OK. You can just add a > `CpusAccel *cpu_accel_ops` field to AccelClass or AccelState. No > need for a separate QOM hierarchy, either. > > If you _really_ want a separate TYPE_CPU_ACCEL_OPS QOM type, you No, I do not think I really need a separate QOM type. > can still have it. But it can be just an interface implemented > by each accel subclass, instead of requiring a separate > CPUS_ACCEL_TYPE_NAME(...) type to be registered for each > accelerator. (I don't see why you would want it, though.) > > >> }; >> +static const TypeInfo kvm_cpus_type_info = { >> + .name = CPUS_ACCEL_TYPE_NAME("kvm"), >> + >> + .parent = TYPE_CPUS_ACCEL_OPS, >> + .class_init = kvm_cpus_class_init, >> + .abstract = true, >> +}; >> +static void kvm_cpus_register_types(void) >> +{ >> + type_register_static(&kvm_cpus_type_info); >> +} >> +type_init(kvm_cpus_register_types); > [...] >> -typedef struct CpusAccel { >> - void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY */ >> +typedef struct CpusAccelOps CpusAccelOps; >> +DECLARE_CLASS_CHECKERS(CpusAccelOps, CPUS_ACCEL_OPS, TYPE_CPUS_ACCEL_OPS) >> + >> +struct CpusAccelOps { >> + ObjectClass parent_class; >> + >> + /* initialization function called when accel is chosen */ >> + void (*accel_chosen_init)(CpusAccelOps *ops); > > This can be an AccelClass method too. What about just naming it > AccelClass.init? yes, .init is fine for me > >> + >> + void (*create_vcpu_thread)(CPUState *cpu); /* MANDATORY NON-NULL */ >> void (*kick_vcpu_thread)(CPUState *cpu); >> >> void (*synchronize_post_reset)(CPUState *cpu); >> @@ -20,13 +45,7 @@ typedef struct CpusAccel { >> >> int64_t (*get_virtual_clock)(void); >> int64_t (*get_elapsed_ticks)(void); >> -} CpusAccel; >> - > [...] >> + >> +static void cpus_accel_ops_init(void) >> +{ > > If we move the fields above part of AccelClass, this could be > called accel_init(). *nod* > >> + const char *ac_name; >> + ObjectClass *ac; >> + char *ops_name; >> + ObjectClass *ops; >> + >> + ac = object_get_class(OBJECT(current_accel())); >> + g_assert(ac != NULL); > > If you call this function directly from accel_init_machine(), > bsd-user:main(), and linux-user:main(), you can get AccelState* > as argument, and the dependency on current_accel() becomes > explicit instead of implicit. That I must say is a good point. > >> + ac_name = object_class_get_name(ac); >> + g_assert(ac_name != NULL); >> + >> + ops_name = g_strdup_printf("%s-ops", ac_name); >> + ops = object_class_by_name(ops_name); >> + g_free(ops_name); > > If we make the fields above part of AccelClass, you don't need > this lookup trick. Yes, I will remove this QOM hierarchy completely. > >> + >> + /* >> + * all accelerators need to define ops, providing at least a mandatory >> + * non-NULL create_vcpu_thread operation. >> + */ >> + g_assert(ops != NULL); >> + cpus_accel = CPUS_ACCEL_OPS_CLASS(ops); >> + if (cpus_accel->accel_chosen_init) { >> + cpus_accel->accel_chosen_init(cpus_accel); > > If we move CpusAccelOps.accel_chosen_init to AccelClass.init, > this would be just: > > AccelClass *acc = ACCEL_GET_CLASS(accel); > if (acc->init) { > acc->init(acc); > } > nice... >> + } > > Additionally, if you call arch_cpu_accel_init() here, you won't > need MODULE_INIT_ACCEL_CPU anymore. The > > module_call_init(MODULE_INIT_ACCEL_CPU) > > call with implicit dependencies on runtime state inside vl.c and > *-user/main.c becomes a trivial: > > accel_init(accel) > > call in accel_init_machine() and *-user:main(). I do need a separate thing for the arch cpu accel specialization though, without MODULE_INIT_ACCEL_CPU that link between all operations done at accel-chosen time is missing.. > >> +} >> + >> +accel_cpu_init(cpus_accel_ops_init); >> diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c >> index 77c365311c..ec3c426223 100644 >> --- a/target/i386/hax/hax-all.c >> +++ b/target/i386/hax/hax-all.c >> @@ -1138,12 +1138,3 @@ static void hax_type_init(void) >> } >> >> type_init(hax_type_init); >> - >> -static void hax_accel_cpu_init(void) >> -{ >> - if (hax_enabled()) { >> - cpus_register_accel(&hax_cpus); >> - } >> -} >> - >> -accel_cpu_init(hax_accel_cpu_init); >> diff --git a/target/i386/hax/hax-cpus.c b/target/i386/hax/hax-cpus.c >> index f72c85bd49..171b5ac1e6 100644 >> --- a/target/i386/hax/hax-cpus.c >> +++ b/target/i386/hax/hax-cpus.c >> @@ -74,12 +74,27 @@ static void hax_start_vcpu_thread(CPUState *cpu) >> #endif >> } >> >> -const CpusAccel hax_cpus = { >> - .create_vcpu_thread = hax_start_vcpu_thread, >> - .kick_vcpu_thread = hax_kick_vcpu_thread, >> +static void hax_cpus_class_init(ObjectClass *oc, void *data) >> +{ >> + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); >> >> - .synchronize_post_reset = hax_cpu_synchronize_post_reset, >> - .synchronize_post_init = hax_cpu_synchronize_post_init, >> - .synchronize_state = hax_cpu_synchronize_state, >> - .synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm, >> + ops->create_vcpu_thread = hax_start_vcpu_thread; >> + ops->kick_vcpu_thread = hax_kick_vcpu_thread; >> + >> + ops->synchronize_post_reset = hax_cpu_synchronize_post_reset; >> + ops->synchronize_post_init = hax_cpu_synchronize_post_init; >> + ops->synchronize_state = hax_cpu_synchronize_state; >> + ops->synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm; >> +}; >> +static const TypeInfo hax_cpus_type_info = { >> + .name = CPUS_ACCEL_TYPE_NAME("hax"), >> + >> + .parent = TYPE_CPUS_ACCEL_OPS, >> + .class_init = hax_cpus_class_init, >> + .abstract = true, >> }; >> +static void hax_cpus_register_types(void) >> +{ >> + type_register_static(&hax_cpus_type_info); >> +} >> +type_init(hax_cpus_register_types); >> diff --git a/target/i386/hax/hax-cpus.h b/target/i386/hax/hax-cpus.h >> index ee8ab7a631..c7698519cd 100644 >> --- a/target/i386/hax/hax-cpus.h >> +++ b/target/i386/hax/hax-cpus.h >> @@ -12,8 +12,6 @@ >> >> #include "sysemu/cpus.h" >> >> -extern const CpusAccel hax_cpus; >> - >> #include "hax-interface.h" >> #include "hax-i386.h" >> >> diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-cpus.c >> index 817b3d7452..124662de58 100644 >> --- a/target/i386/hvf/hvf-cpus.c >> +++ b/target/i386/hvf/hvf-cpus.c >> @@ -121,11 +121,26 @@ static void hvf_start_vcpu_thread(CPUState *cpu) >> cpu, QEMU_THREAD_JOINABLE); >> } >> >> -const CpusAccel hvf_cpus = { >> - .create_vcpu_thread = hvf_start_vcpu_thread, >> +static void hvf_cpus_class_init(ObjectClass *oc, void *data) >> +{ >> + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); >> >> - .synchronize_post_reset = hvf_cpu_synchronize_post_reset, >> - .synchronize_post_init = hvf_cpu_synchronize_post_init, >> - .synchronize_state = hvf_cpu_synchronize_state, >> - .synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm, >> + ops->create_vcpu_thread = hvf_start_vcpu_thread; >> + >> + ops->synchronize_post_reset = hvf_cpu_synchronize_post_reset; >> + ops->synchronize_post_init = hvf_cpu_synchronize_post_init; >> + ops->synchronize_state = hvf_cpu_synchronize_state; >> + ops->synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm; >> +}; >> +static const TypeInfo hvf_cpus_type_info = { >> + .name = CPUS_ACCEL_TYPE_NAME("hvf"), >> + >> + .parent = TYPE_CPUS_ACCEL_OPS, >> + .class_init = hvf_cpus_class_init, >> + .abstract = true, >> }; >> +static void hvf_cpus_register_types(void) >> +{ >> + type_register_static(&hvf_cpus_type_info); >> +} >> +type_init(hvf_cpus_register_types); >> diff --git a/target/i386/hvf/hvf-cpus.h b/target/i386/hvf/hvf-cpus.h >> index ced31b82c0..8f992da168 100644 >> --- a/target/i386/hvf/hvf-cpus.h >> +++ b/target/i386/hvf/hvf-cpus.h >> @@ -12,8 +12,6 @@ >> >> #include "sysemu/cpus.h" >> >> -extern const CpusAccel hvf_cpus; >> - >> int hvf_init_vcpu(CPUState *); >> int hvf_vcpu_exec(CPUState *); >> void hvf_cpu_synchronize_state(CPUState *); >> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c >> index 58794c35ae..bd94bb5243 100644 >> --- a/target/i386/hvf/hvf.c >> +++ b/target/i386/hvf/hvf.c >> @@ -910,12 +910,3 @@ static void hvf_type_init(void) >> } >> >> type_init(hvf_type_init); >> - >> -static void hvf_accel_cpu_init(void) >> -{ >> - if (hvf_enabled()) { >> - cpus_register_accel(&hvf_cpus); >> - } >> -} >> - >> -accel_cpu_init(hvf_accel_cpu_init); >> diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c >> index 097d6f5e60..90adae9af7 100644 >> --- a/target/i386/whpx/whpx-all.c >> +++ b/target/i386/whpx/whpx-all.c >> @@ -1711,12 +1711,3 @@ error: >> } >> >> type_init(whpx_type_init); >> - >> -static void whpx_accel_cpu_init(void) >> -{ >> - if (whpx_enabled()) { >> - cpus_register_accel(&whpx_cpus); >> - } >> -} >> - >> -accel_cpu_init(whpx_accel_cpu_init); >> diff --git a/target/i386/whpx/whpx-cpus.c b/target/i386/whpx/whpx-cpus.c >> index d9bd5a2d36..1e736a50b0 100644 >> --- a/target/i386/whpx/whpx-cpus.c >> +++ b/target/i386/whpx/whpx-cpus.c >> @@ -85,12 +85,27 @@ static void whpx_kick_vcpu_thread(CPUState *cpu) >> } >> } >> >> -const CpusAccel whpx_cpus = { >> - .create_vcpu_thread = whpx_start_vcpu_thread, >> - .kick_vcpu_thread = whpx_kick_vcpu_thread, >> +static void whpx_cpus_class_init(ObjectClass *oc, void *data) >> +{ >> + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); >> >> - .synchronize_post_reset = whpx_cpu_synchronize_post_reset, >> - .synchronize_post_init = whpx_cpu_synchronize_post_init, >> - .synchronize_state = whpx_cpu_synchronize_state, >> - .synchronize_pre_loadvm = whpx_cpu_synchronize_pre_loadvm, >> + ops->create_vcpu_thread = whpx_start_vcpu_thread; >> + ops->kick_vcpu_thread = whpx_kick_vcpu_thread; >> + >> + ops->synchronize_post_reset = whpx_cpu_synchronize_post_reset; >> + ops->synchronize_post_init = whpx_cpu_synchronize_post_init; >> + ops->synchronize_state = whpx_cpu_synchronize_state; >> + ops->synchronize_pre_loadvm = whpx_cpu_synchronize_pre_loadvm; >> +}; >> +static const TypeInfo whpx_cpus_type_info = { >> + .name = CPUS_ACCEL_TYPE_NAME("whpx"), >> + >> + .parent = TYPE_CPUS_ACCEL_OPS, >> + .class_init = whpx_cpus_class_init, >> + .abstract = true, >> }; >> +static void whpx_cpus_register_types(void) >> +{ >> + type_register_static(&whpx_cpus_type_info); >> +} >> +type_init(whpx_cpus_register_types); >> diff --git a/target/i386/whpx/whpx-cpus.h b/target/i386/whpx/whpx-cpus.h >> index bdb367d1d0..2dee6d61ea 100644 >> --- a/target/i386/whpx/whpx-cpus.h >> +++ b/target/i386/whpx/whpx-cpus.h >> @@ -12,8 +12,6 @@ >> >> #include "sysemu/cpus.h" >> >> -extern const CpusAccel whpx_cpus; >> - >> int whpx_init_vcpu(CPUState *cpu); >> int whpx_vcpu_exec(CPUState *cpu); >> void whpx_destroy_vcpu(CPUState *cpu); >> -- >> 2.26.2 >> > Thanks a lot for reading all this! Back to the drawing board.. Ciao, Claudio