| Message ID | 20201124162210.8796-13-cfontana@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | i386 cleanup | expand |
On Tue, Nov 24, 2020 at 05:22:10PM +0100, Claudio Fontana wrote: > Signed-off-by: Claudio Fontana <cfontana@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. TCG makes it a bit more complicated because there's a different set of methods chosen for TYPE_TCG_ACCEL depending on the configuration. This could be solved by patching AccelClass at init time, or by moving the method pointers to AccelState. 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 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? > + > + 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(). > + 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. > + 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. > + > + /* > + * 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); } > + } 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(). > +} > + > +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 >
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 <cfontana@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
On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: [...] > >> + } > > > > 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.. > I think this is a key point we need to sort out. What do you mean by "link between all operations done at accel-chosen time" and why that's important? accel_init_machine() has 2-3 lines of code with side effects. It calls AccelClass.init_machine(), which may may have hundreds of lines of code. accel_setup_post() has one additional method call, which is triggered at a slightly different moment. You are using MODULE_INIT_ACCEL for 2 additional lines of code: - the cpus_register_accel() call - the x86_cpu_accel_init() call What makes those 2 lines of code so special, to make them deserve a completely new mechanism to trigger them, instead of using trivial function calls inside a accel_init() function?
On 11/24/20 8:27 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: > [...] >>>> + } >>> >>> 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.. >> > > I think this is a key point we need to sort out. > > What do you mean by "link between all operations done at > accel-chosen time" and why that's important? For understanding by a reader that tries to figure this out, (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). And it could be that the high level plan to make accelerators fully dynamically loadable plugins in the future also conditioned me to want to have a very clear demarcation line around the choice of the accelerator. > > accel_init_machine() has 2-3 lines of code with side effects. It > calls AccelClass.init_machine(), which may may have hundreds of could we initialize also all arch-dependent stuff in here? > lines of code. accel_setup_post() has one additional method > call, which is triggered at a slightly different moment. > > You are using MODULE_INIT_ACCEL for 2 additional lines of code: > - the cpus_register_accel() call > - the x86_cpu_accel_init() call > > What makes those 2 lines of code so special, to make them deserve > a completely new mechanism to trigger them, instead of using > trivial function calls inside a accel_init() function? > ...can we do also the x86_cpu_accel_init inside accel_init()? In any case I'll try also the alternative, it would be nice if I could bring everything together under the same roof, and easily discoverable, both the arch-specific steps that we need to do at accel-chosen time and the non-arch-specific steps. Hope this helps clarifying where I am coming from, but I am open to have my mind changed, also trying the alternatives you propose here could help me see first hand how things play out. Thanks! Claudio
On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: > On 11/24/20 8:27 PM, Eduardo Habkost wrote: > > On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: > > [...] > >>>> + } > >>> > >>> 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.. > >> > > > > I think this is a key point we need to sort out. > > > > What do you mean by "link between all operations done at > > accel-chosen time" and why that's important? > > > For understanding by a reader that tries to figure this out, > (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU) indirection makes this easier to figure out than just looking at a accel_init() function that makes regular function calls? > > And it could be that the high level plan to make accelerators fully dynamically loadable plugins in the future > also conditioned me to want to have a very clear demarcation line around the choice of the accelerator. We have dynamically loadable modules for other QOM types, already, and they just use type_init(). I don't see why an extra module_init() type makes this easier. > > > > > > accel_init_machine() has 2-3 lines of code with side effects. It > > calls AccelClass.init_machine(), which may may have hundreds of > > > could we initialize also all arch-dependent stuff in here? You can, if you use a wrapper + stub, like arch_cpu_accel_init(). > > > > lines of code. accel_setup_post() has one additional method > > call, which is triggered at a slightly different moment. > > > > You are using MODULE_INIT_ACCEL for 2 additional lines of code: > > - the cpus_register_accel() call > > - the x86_cpu_accel_init() call > > > > What makes those 2 lines of code so special, to make them deserve > > a completely new mechanism to trigger them, instead of using > > trivial function calls inside a accel_init() function? > > > > ...can we do also the x86_cpu_accel_init inside accel_init()? > > > In any case I'll try also the alternative, it would be nice if I could bring everything together under the same roof, > and easily discoverable, both the arch-specific steps that we need to do at accel-chosen time and the non-arch-specific steps. One way to bring everything together under the same roof is to call everything inside a accel_init() function. > > Hope this helps clarifying where I am coming from, > but I am open to have my mind changed, also trying the alternatives you propose here could help me see first hand how things play out. Thanks!
On 11/24/20 9:34 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: >> On 11/24/20 8:27 PM, Eduardo Habkost wrote: >>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: >>> [...] >>>>>> + } >>>>> >>>>> 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.. >>>> >>> >>> I think this is a key point we need to sort out. >>> >>> What do you mean by "link between all operations done at >>> accel-chosen time" and why that's important? >> >> >> For understanding by a reader that tries to figure this out, >> (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). > > Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU) > indirection makes this easier to figure out than just looking at > a accel_init() function that makes regular function calls? I agree, if we accomplish a single accel_init() call that does everything (accelerator initialization and arch independent ops initialization and arch dependent specialization of the x86 cpu), that would be the best outcome in my view also. > > >> >> And it could be that the high level plan to make accelerators fully dynamically loadable plugins in the future >> also conditioned me to want to have a very clear demarcation line around the choice of the accelerator. > > We have dynamically loadable modules for other QOM types, > already, and they just use type_init(). I don't see why an extra > module_init() type makes this easier. > >> >> >>> >>> accel_init_machine() has 2-3 lines of code with side effects. It >>> calls AccelClass.init_machine(), which may may have hundreds of >> >> >> could we initialize also all arch-dependent stuff in here? > > You can, if you use a wrapper + stub, like arch_cpu_accel_init(). > As mentioned elsewhere, I'll try to avoid stubs. One is too many I think in the codebase (well one is probably ok, but you get what I mean, I don't like their proliferation). > >> >> >>> lines of code. accel_setup_post() has one additional method >>> call, which is triggered at a slightly different moment. >>> >>> You are using MODULE_INIT_ACCEL for 2 additional lines of code: >>> - the cpus_register_accel() call >>> - the x86_cpu_accel_init() call >>> >>> What makes those 2 lines of code so special, to make them deserve >>> a completely new mechanism to trigger them, instead of using >>> trivial function calls inside a accel_init() function? >>> >> >> ...can we do also the x86_cpu_accel_init inside accel_init()? >> >> >> In any case I'll try also the alternative, it would be nice if I could bring everything together under the same roof, >> and easily discoverable, both the arch-specific steps that we need to do at accel-chosen time and the non-arch-specific steps. > > One way to bring everything together under the same roof is to > call everything inside a accel_init() function. Will try! > > >> >> Hope this helps clarifying where I am coming from, >> but I am open to have my mind changed, also trying the alternatives you propose here could help me see first hand how things play out. > > Thanks! > Thanks, Ciao, Claudio
On 11/25/20 10:32 AM, Claudio Fontana wrote: > On 11/24/20 9:34 PM, Eduardo Habkost wrote: >> On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: >>> On 11/24/20 8:27 PM, Eduardo Habkost wrote: >>>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: >>>> [...] >>>>>>> + } >>>>>> >>>>>> 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(). On this one I see an issue: the *-user_main() would still need an ac->machine_init() call to initialize tcg itself, currently the accelerator initialization is put into ac->machine_init (tcg_init, kvm_init, xen_init, etc). Or are you proposing to move tcg initialization away from the current ->machine_init(), into the new ac->init called by accel_init()? This would make tcg even more different from the other accelerators. Or are you proposing for all accelerators to separate the initialization of the accelerator itself from the machine state input, leading to, for example, separating kvm-all.c kvm_init() into two functions, one which takes the input from MachineState and puts it into the AccelState, and another one which actually then initializes kvm proper? And same for all accels? In my view we could still do in *-user main.c, ac = ACCEL_GET_CLASS(current_accel()) ac->machine_init(NULL); ac->init_cpu_interfaces(ac); to solve this, or something like that, but also the option of fixing all accelerators to separate the gathering of the input from the MachineState to the actual accelerator initialization is a possibility to me. Ciao, Claudio >>>>> >>>>> >>>>> >>>>> 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.. >>>>> >>>> >>>> I think this is a key point we need to sort out. >>>> >>>> What do you mean by "link between all operations done at >>>> accel-chosen time" and why that's important? >>> >>> >>> For understanding by a reader that tries to figure this out, >>> (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). >> >> Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU) >> indirection makes this easier to figure out than just looking at >> a accel_init() function that makes regular function calls? > > > I agree, if we accomplish a single accel_init() call that does everything (accelerator initialization and arch independent ops initialization and arch dependent specialization of the x86 cpu), > that would be the best outcome in my view also. > > >> >> >>> >>> And it could be that the high level plan to make accelerators fully dynamically loadable plugins in the future >>> also conditioned me to want to have a very clear demarcation line around the choice of the accelerator. >> >> We have dynamically loadable modules for other QOM types, >> already, and they just use type_init(). I don't see why an extra >> module_init() type makes this easier. >> >>> >>> >>>> >>>> accel_init_machine() has 2-3 lines of code with side effects. It >>>> calls AccelClass.init_machine(), which may may have hundreds of >>> >>> >>> could we initialize also all arch-dependent stuff in here? >> >> You can, if you use a wrapper + stub, like arch_cpu_accel_init(). >> > > As mentioned elsewhere, I'll try to avoid stubs. One is too many I think in the codebase (well one is probably ok, but you get what I mean, I don't like their proliferation). > >> >>> >>> >>>> lines of code. accel_setup_post() has one additional method >>>> call, which is triggered at a slightly different moment. >>>> >>>> You are using MODULE_INIT_ACCEL for 2 additional lines of code: >>>> - the cpus_register_accel() call >>>> - the x86_cpu_accel_init() call >>>> >>>> What makes those 2 lines of code so special, to make them deserve >>>> a completely new mechanism to trigger them, instead of using >>>> trivial function calls inside a accel_init() function? >>>> >>> >>> ...can we do also the x86_cpu_accel_init inside accel_init()? >>> >>> >>> In any case I'll try also the alternative, it would be nice if I could bring everything together under the same roof, >>> and easily discoverable, both the arch-specific steps that we need to do at accel-chosen time and the non-arch-specific steps. >> >> One way to bring everything together under the same roof is to >> call everything inside a accel_init() function. > > Will try! > > >> >> >>> >>> Hope this helps clarifying where I am coming from, >>> but I am open to have my mind changed, also trying the alternatives you propose here could help me see first hand how things play out. >> >> Thanks! >> > > Thanks, > > Ciao, > > Claudio >
On Wed, Nov 25, 2020 at 12:48:22PM +0100, Claudio Fontana wrote: > On 11/25/20 10:32 AM, Claudio Fontana wrote: > > On 11/24/20 9:34 PM, Eduardo Habkost wrote: > >> On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: > >>> On 11/24/20 8:27 PM, Eduardo Habkost wrote: > >>>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: > >>>> [...] > >>>>>>> + } > >>>>>> > >>>>>> 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(). > > > On this one I see an issue: > > the *-user_main() would still need an ac->machine_init() call to initialize tcg itself, > currently the accelerator initialization is put into ac->machine_init > > (tcg_init, kvm_init, xen_init, etc). > > Or are you proposing to move tcg initialization away from the current ->machine_init(), > into the new ac->init called by accel_init()? Yes. Anything that requires MachineState (and is softmmu-specific) would go to ->machine_init(). Anything that is not softmmu-specific would go to ->init(). > > This would make tcg even more different from the other accelerators. That's true, but isn't this only because TCG is the only one that really needs it? > > Or are you proposing for all accelerators to separate the initialization of the accelerator itself > from the machine state input, leading to, for example, separating kvm-all.c kvm_init() into two > functions, one which takes the input from MachineState and puts it into the AccelState, and > another one which actually then initializes kvm proper? And same for all accels? That would be possible (and maybe a good idea), but not necessary to make it work. > > In my view we could still do in *-user main.c, > > ac = ACCEL_GET_CLASS(current_accel()) > ac->machine_init(NULL); > ac->init_cpu_interfaces(ac); That would work too. I would implement it as an accel_init(NULL) call, however, to avoid duplicating the code from accel_init_machine(). Calling ->machine_init(NULL) is just a bit surprising because of the name (calling machine_init() when there's no machine), and because we know most accelerators will crash if getting a NULL argument. Anyway, the split between ->machine_init() and ->init() is just a suggestion. Keeping a single init method that accepts a NULL MachineState* as argument is not my favourite option, but it works. Whatever you choose, my only ask is to document clearly the expectations and requirements of the AccelClass methods you are using. > > to solve this, or something like that, but also the option of fixing all accelerators to separate > the gathering of the input from the MachineState to the actual accelerator initialization is > a possibility to me. > > Ciao, > > Claudio Thank you very much for your patience! I think we're going on the right direction.
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); - .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; }; +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); diff --git a/accel/kvm/kvm-cpus.h b/accel/kvm/kvm-cpus.h index 3df732b816..bf0bd1bee4 100644 --- a/accel/kvm/kvm-cpus.h +++ b/accel/kvm/kvm-cpus.h @@ -12,8 +12,6 @@ #include "sysemu/cpus.h" -extern const CpusAccel kvm_cpus; - int kvm_init_vcpu(CPUState *cpu, Error **errp); int kvm_cpu_exec(CPUState *cpu); void kvm_destroy_vcpu(CPUState *cpu); diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c index 482f89729f..8bf51689bc 100644 --- a/accel/qtest/qtest.c +++ b/accel/qtest/qtest.c @@ -25,11 +25,6 @@ #include "qemu/main-loop.h" #include "hw/core/cpu.h" -const CpusAccel qtest_cpus = { - .create_vcpu_thread = dummy_start_vcpu_thread, - .get_virtual_clock = qtest_get_virtual_clock, -}; - static int qtest_init_accel(MachineState *ms) { return 0; @@ -51,18 +46,26 @@ static const TypeInfo qtest_accel_type = { .class_init = qtest_accel_class_init, }; +static void qtest_cpus_class_init(ObjectClass *oc, void *data) +{ + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); + + ops->create_vcpu_thread = dummy_start_vcpu_thread; + ops->get_virtual_clock = qtest_get_virtual_clock; +}; + +static const TypeInfo qtest_cpus_type_info = { + .name = CPUS_ACCEL_TYPE_NAME("qtest"), + + .parent = TYPE_CPUS_ACCEL_OPS, + .class_init = qtest_cpus_class_init, + .abstract = true, +}; + static void qtest_type_init(void) { type_register_static(&qtest_accel_type); + type_register_static(&qtest_cpus_type_info); } type_init(qtest_type_init); - -static void qtest_accel_cpu_init(void) -{ - if (qtest_enabled()) { - cpus_register_accel(&qtest_cpus); - } -} - -accel_cpu_init(qtest_accel_cpu_init); diff --git a/accel/tcg/tcg-cpus-icount.c b/accel/tcg/tcg-cpus-icount.c index 9f45432275..5445b4d545 100644 --- a/accel/tcg/tcg-cpus-icount.c +++ b/accel/tcg/tcg-cpus-icount.c @@ -125,7 +125,7 @@ void icount_process_data(CPUState *cpu) replay_mutex_unlock(); } -static void icount_handle_interrupt(CPUState *cpu, int mask) +void icount_handle_interrupt(CPUState *cpu, int mask) { int old_mask = cpu->interrupt_request; @@ -136,12 +136,3 @@ static void icount_handle_interrupt(CPUState *cpu, int mask) cpu_abort(cpu, "Raised interrupt while not in I/O function"); } } - -const CpusAccel tcg_cpus_icount = { - .create_vcpu_thread = rr_start_vcpu_thread, - .kick_vcpu_thread = rr_kick_vcpu_thread, - - .handle_interrupt = icount_handle_interrupt, - .get_virtual_clock = icount_get, - .get_elapsed_ticks = icount_get, -}; diff --git a/accel/tcg/tcg-cpus-icount.h b/accel/tcg/tcg-cpus-icount.h index b695939dfa..d884aa2aaa 100644 --- a/accel/tcg/tcg-cpus-icount.h +++ b/accel/tcg/tcg-cpus-icount.h @@ -14,4 +14,6 @@ void icount_handle_deadline(void); void icount_prepare_for_run(CPUState *cpu); void icount_process_data(CPUState *cpu); +void icount_handle_interrupt(CPUState *cpu, int mask); + #endif /* TCG_CPUS_ICOUNT_H */ diff --git a/accel/tcg/tcg-cpus-mttcg.c b/accel/tcg/tcg-cpus-mttcg.c index 9c3767d260..dabf5ed42e 100644 --- a/accel/tcg/tcg-cpus-mttcg.c +++ b/accel/tcg/tcg-cpus-mttcg.c @@ -33,6 +33,7 @@ #include "hw/boards.h" #include "tcg-cpus.h" +#include "tcg-cpus-mttcg.h" /* * In the multi-threaded case each vCPU has its own thread. The TLS @@ -103,12 +104,12 @@ static void *mttcg_cpu_thread_fn(void *arg) return NULL; } -static void mttcg_kick_vcpu_thread(CPUState *cpu) +void mttcg_kick_vcpu_thread(CPUState *cpu) { cpu_exit(cpu); } -static void mttcg_start_vcpu_thread(CPUState *cpu) +void mttcg_start_vcpu_thread(CPUState *cpu) { char thread_name[VCPU_THREAD_NAME_SIZE]; @@ -131,10 +132,3 @@ static void mttcg_start_vcpu_thread(CPUState *cpu) cpu->hThread = qemu_thread_get_handle(cpu->thread); #endif } - -const CpusAccel tcg_cpus_mttcg = { - .create_vcpu_thread = mttcg_start_vcpu_thread, - .kick_vcpu_thread = mttcg_kick_vcpu_thread, - - .handle_interrupt = tcg_cpus_handle_interrupt, -}; diff --git a/accel/tcg/tcg-cpus-mttcg.h b/accel/tcg/tcg-cpus-mttcg.h new file mode 100644 index 0000000000..0af91dd3b3 --- /dev/null +++ b/accel/tcg/tcg-cpus-mttcg.h @@ -0,0 +1,19 @@ +/* + * QEMU TCG Multi Threaded vCPUs implementation + * + * Copyright 2020 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 TCG_CPUS_MTTCG_H +#define TCG_CPUS_MTTCG_H + +/* kick MTTCG vCPU thread */ +void mttcg_kick_vcpu_thread(CPUState *cpu); + +/* start an mttcg vCPU thread */ +void mttcg_start_vcpu_thread(CPUState *cpu); + +#endif /* TCG_CPUS_MTTCG_H */ diff --git a/accel/tcg/tcg-cpus-rr.c b/accel/tcg/tcg-cpus-rr.c index 0181d2e4eb..802c57bb60 100644 --- a/accel/tcg/tcg-cpus-rr.c +++ b/accel/tcg/tcg-cpus-rr.c @@ -296,10 +296,3 @@ void rr_start_vcpu_thread(CPUState *cpu) cpu->created = true; } } - -const CpusAccel tcg_cpus_rr = { - .create_vcpu_thread = rr_start_vcpu_thread, - .kick_vcpu_thread = rr_kick_vcpu_thread, - - .handle_interrupt = tcg_cpus_handle_interrupt, -}; diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c index c9e662f06e..4f3a50af2e 100644 --- a/accel/tcg/tcg-cpus.c +++ b/accel/tcg/tcg-cpus.c @@ -35,6 +35,9 @@ #include "hw/boards.h" #include "tcg-cpus.h" +#include "tcg-cpus-mttcg.h" +#include "tcg-cpus-rr.h" +#include "tcg-cpus-icount.h" /* common functionality among all TCG variants */ @@ -81,17 +84,42 @@ void tcg_cpus_handle_interrupt(CPUState *cpu, int mask) } } -static void tcg_accel_cpu_init(void) +static void tcg_cpus_accel_chosen_init(CpusAccelOps *ops) { - if (tcg_enabled()) { - if (qemu_tcg_mttcg_enabled()) { - cpus_register_accel(&tcg_cpus_mttcg); - } else if (icount_enabled()) { - cpus_register_accel(&tcg_cpus_icount); - } else { - cpus_register_accel(&tcg_cpus_rr); - } + if (qemu_tcg_mttcg_enabled()) { + ops->create_vcpu_thread = mttcg_start_vcpu_thread; + ops->kick_vcpu_thread = mttcg_kick_vcpu_thread; + ops->handle_interrupt = tcg_cpus_handle_interrupt; + + } else if (icount_enabled()) { + ops->create_vcpu_thread = rr_start_vcpu_thread; + ops->kick_vcpu_thread = rr_kick_vcpu_thread; + ops->handle_interrupt = icount_handle_interrupt; + ops->get_virtual_clock = icount_get; + ops->get_elapsed_ticks = icount_get; + + } else { + ops->create_vcpu_thread = rr_start_vcpu_thread; + ops->kick_vcpu_thread = rr_kick_vcpu_thread; + ops->handle_interrupt = tcg_cpus_handle_interrupt; } } -accel_cpu_init(tcg_accel_cpu_init); +static void tcg_cpus_class_init(ObjectClass *oc, void *data) +{ + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); + + ops->accel_chosen_init = tcg_cpus_accel_chosen_init; +}; +static const TypeInfo tcg_cpus_type_info = { + .name = CPUS_ACCEL_TYPE_NAME("tcg"), + + .parent = TYPE_CPUS_ACCEL_OPS, + .class_init = tcg_cpus_class_init, + .abstract = true, +}; +static void tcg_cpus_register_types(void) +{ + type_register_static(&tcg_cpus_type_info); +} +type_init(tcg_cpus_register_types); diff --git a/accel/tcg/tcg-cpus.h b/accel/tcg/tcg-cpus.h index d6893a32f8..923cbace12 100644 --- a/accel/tcg/tcg-cpus.h +++ b/accel/tcg/tcg-cpus.h @@ -14,10 +14,6 @@ #include "sysemu/cpus.h" -extern const CpusAccel tcg_cpus_mttcg; -extern const CpusAccel tcg_cpus_icount; -extern const CpusAccel tcg_cpus_rr; - void tcg_cpus_destroy(CPUState *cpu); int tcg_cpus_exec(CPUState *cpu); void tcg_cpus_handle_interrupt(CPUState *cpu, int mask); diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index be09b6ec22..976c7806d0 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -154,10 +154,6 @@ static void xen_setup_post(MachineState *ms, AccelState *accel) } } -const CpusAccel xen_cpus = { - .create_vcpu_thread = dummy_start_vcpu_thread, -}; - static int xen_init(MachineState *ms) { MachineClass *mc = MACHINE_GET_CLASS(ms); @@ -219,18 +215,23 @@ static const TypeInfo xen_accel_type = { .class_init = xen_accel_class_init, }; +static void xen_cpus_class_init(ObjectClass *oc, void *data) +{ + CpusAccelOps *ops = CPUS_ACCEL_OPS_CLASS(oc); + + ops->create_vcpu_thread = dummy_start_vcpu_thread; +}; +static const TypeInfo xen_cpus_type_info = { + .name = CPUS_ACCEL_TYPE_NAME("xen"), + + .parent = TYPE_CPUS_ACCEL_OPS, + .class_init = xen_cpus_class_init, + .abstract = true, +}; + static void xen_type_init(void) { type_register_static(&xen_accel_type); + type_register_static(&xen_cpus_type_info); } - type_init(xen_type_init); - -static void xen_accel_cpu_init(void) -{ - if (xen_enabled()) { - cpus_register_accel(&xen_cpus); - } -} - -accel_cpu_init(xen_accel_cpu_init); diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h index e8156728c6..9b0c5eadf3 100644 --- a/include/sysemu/cpus.h +++ b/include/sysemu/cpus.h @@ -1,14 +1,39 @@ +/* + * CPUS module (softmmu/cpus.c) Accelerator Ops + * + * Copyright 2020 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 QEMU_CPUS_H #define QEMU_CPUS_H #include "qemu/timer.h" +#include "qom/object.h" + +/* accel/dummy-cpus.c */ + +/* Create a dummy vcpu for CpusAccelOps->create_vcpu_thread */ +void dummy_start_vcpu_thread(CPUState *); /* cpus.c */ -/* CPU execution threads */ +#define TYPE_CPUS_ACCEL_OPS "accel-ops" +#define CPUS_ACCEL_TYPE_NAME(name) (name "-" TYPE_CPUS_ACCEL_OPS) -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); + + 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; - -/* register accel-specific cpus interface implementation */ -void cpus_register_accel(const CpusAccel *i); - -/* Create a dummy vcpu for CpusAccel->create_vcpu_thread */ -void dummy_start_vcpu_thread(CPUState *); +}; /* interface available for cpus accelerator threads */ diff --git a/softmmu/cpus.c b/softmmu/cpus.c index e46ac68ad0..2d2386900a 100644 --- a/softmmu/cpus.c +++ b/softmmu/cpus.c @@ -127,7 +127,7 @@ void hw_error(const char *fmt, ...) /* * The chosen accelerator is supposed to register this. */ -static const CpusAccel *cpus_accel; +static CpusAccelOps *cpus_accel; void cpu_synchronize_all_states(void) { @@ -593,13 +593,6 @@ void cpu_remove_sync(CPUState *cpu) qemu_mutex_lock_iothread(); } -void cpus_register_accel(const CpusAccel *ca) -{ - assert(ca != NULL); - assert(ca->create_vcpu_thread != NULL); /* mandatory */ - cpus_accel = ca; -} - void qemu_init_vcpu(CPUState *cpu) { MachineState *ms = MACHINE(qdev_get_machine()); @@ -617,7 +610,7 @@ void qemu_init_vcpu(CPUState *cpu) cpu_address_space_init(cpu, 0, "cpu-memory", cpu->memory); } - /* accelerators all implement the CpusAccel interface */ + /* accelerators all implement the CpusAccelOps */ g_assert(cpus_accel != NULL && cpus_accel->create_vcpu_thread != NULL); cpus_accel->create_vcpu_thread(cpu); @@ -797,3 +790,43 @@ void qmp_inject_nmi(Error **errp) nmi_monitor_handle(monitor_get_cpu_index(monitor_cur()), errp); } +static const TypeInfo cpus_accel_type_info = { + .name = TYPE_CPUS_ACCEL_OPS, + .parent = TYPE_OBJECT, + .abstract = true, + .class_size = sizeof(CpusAccelOps), +}; +static void cpus_register_types(void) +{ + type_register_static(&cpus_accel_type_info); +} +type_init(cpus_register_types); + +static void cpus_accel_ops_init(void) +{ + const char *ac_name; + ObjectClass *ac; + char *ops_name; + ObjectClass *ops; + + ac = object_get_class(OBJECT(current_accel())); + g_assert(ac != NULL); + 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); + + /* + * 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); + } +} + +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);
Signed-off-by: Claudio Fontana <cfontana@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