| Message ID | 20201118102936.25569-9-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup | expand |
On 11/18/20 11:29 AM, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also defer the registration of the cpu models, this is not true anymore, so my commit message here needs fixing. > in order to make them subclasses of a per-accel cpu type. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > accel/kvm/kvm-all.c | 11 +++++++++-- > accel/qtest/qtest.c | 10 +++++++++- > accel/tcg/tcg-all.c | 11 +++++++++-- > accel/xen/xen-all.c | 12 +++++++++--- > include/qemu/module.h | 2 ++ > softmmu/vl.c | 6 ++++++ > target/i386/hax/hax-all.c | 12 +++++++++--- > target/i386/hvf/hvf.c | 10 +++++++++- > target/i386/whpx/whpx-all.c | 11 +++++++++-- > 9 files changed, 71 insertions(+), 14 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 9ef5daf4c5..509b249f52 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -2251,8 +2251,6 @@ static int kvm_init(MachineState *ms) > ret = ram_block_discard_disable(true); > assert(!ret); > } > - > - cpus_register_accel(&kvm_cpus); > return 0; > > err: > @@ -3236,3 +3234,12 @@ 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/qtest/qtest.c b/accel/qtest/qtest.c > index b282cea5cf..8d14059e32 100644 > --- a/accel/qtest/qtest.c > +++ b/accel/qtest/qtest.c > @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { > > static int qtest_init_accel(MachineState *ms) > { > - cpus_register_accel(&qtest_cpus); > return 0; > } > > @@ -58,3 +57,12 @@ static void qtest_type_init(void) > } > > 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-all.c b/accel/tcg/tcg-all.c > index fa1208158f..9ffedc8151 100644 > --- a/accel/tcg/tcg-all.c > +++ b/accel/tcg/tcg-all.c > @@ -104,8 +104,6 @@ static int tcg_init(MachineState *ms) > > tcg_exec_init(s->tb_size * 1024 * 1024); > mttcg_enabled = s->mttcg_enabled; > - cpus_register_accel(&tcg_cpus); > - > return 0; > } > > @@ -201,3 +199,12 @@ static void register_accel_types(void) > } > > type_init(register_accel_types); > + > +static void tcg_accel_cpu_init(void) > +{ > + if (tcg_enabled()) { > + cpus_register_accel(&tcg_cpus); > + } > +} > + > +accel_cpu_init(tcg_accel_cpu_init); > diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c > index 878a4089d9..6932a9f364 100644 > --- a/accel/xen/xen-all.c > +++ b/accel/xen/xen-all.c > @@ -185,9 +185,6 @@ static int xen_init(MachineState *ms) > * opt out of system RAM being allocated by generic code > */ > mc->default_ram_id = NULL; > - > - cpus_register_accel(&xen_cpus); > - > return 0; > } > > @@ -228,3 +225,12 @@ static void xen_type_init(void) > } > > 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/qemu/module.h b/include/qemu/module.h > index 944d403cbd..485eda986a 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -44,6 +44,7 @@ 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, > @@ -54,6 +55,7 @@ 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/softmmu/vl.c b/softmmu/vl.c > index e6e0ad5a92..df4bed056a 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -4173,6 +4173,12 @@ 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/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c > index fecfe8cd6e..3bada019f5 100644 > --- a/target/i386/hax/hax-all.c > +++ b/target/i386/hax/hax-all.c > @@ -364,9 +364,6 @@ static int hax_accel_init(MachineState *ms) > !ret ? "working" : "not working", > !ret ? "fast virt" : "emulation"); > } > - if (ret == 0) { > - cpus_register_accel(&hax_cpus); > - } > return ret; > } > > @@ -1141,3 +1138,12 @@ 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/hvf/hvf.c b/target/i386/hvf/hvf.c > index ed9356565c..249b77797f 100644 > --- a/target/i386/hvf/hvf.c > +++ b/target/i386/hvf/hvf.c > @@ -887,7 +887,6 @@ static int hvf_accel_init(MachineState *ms) > > hvf_state = s; > memory_listener_register(&hvf_memory_listener, &address_space_memory); > - cpus_register_accel(&hvf_cpus); > return 0; > } > > @@ -911,3 +910,12 @@ 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 f4f3e33eac..2e715e2bc6 100644 > --- a/target/i386/whpx/whpx-all.c > +++ b/target/i386/whpx/whpx-all.c > @@ -1642,8 +1642,6 @@ static int whpx_accel_init(MachineState *ms) > > whpx_memory_init(); > > - cpus_register_accel(&whpx_cpus); > - > printf("Windows Hypervisor Platform accelerator is operational\n"); > return 0; > > @@ -1713,3 +1711,12 @@ 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); >
On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also defer the registration of the cpu models, > in order to make them subclasses of a per-accel cpu type. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- [...] > + /* > + * accelerator has been chosen and initialized, now it is time to > + * register the cpu accel interface. > + */ > + module_call_init(MODULE_INIT_ACCEL_CPU); I don't get why we would use a new module initialization level for this. If the accelerator object was already created, we can just ask the existing accel object to do whatever initialization step is necessary. e.g. we can add a AccelClass.cpu_accel_ops field, and call: cpus_register_accel(current_machine->accelerator->cpu_accel_ops);
On 11/18/20 1:48 PM, Eduardo Habkost wrote: > On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote: >> apply this to the registration of the cpus accel interfaces, >> >> but this will be also in preparation for later use of this >> new module init step to also defer the registration of the cpu models, >> in order to make them subclasses of a per-accel cpu type. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- > [...] >> + /* >> + * accelerator has been chosen and initialized, now it is time to >> + * register the cpu accel interface. >> + */ >> + module_call_init(MODULE_INIT_ACCEL_CPU); > > I don't get why we would use a new module initialization level To have a clear point in time after which all accelerator interface initialization is done. It avoids to have to hunt down the registration points spread around the code base. I'd turn it around, why not? > for this. If the accelerator object was already created, we can > just ask the existing accel object to do whatever initialization > step is necessary. > > e.g. we can add a AccelClass.cpu_accel_ops field, and call: > > cpus_register_accel(current_machine->accelerator->cpu_accel_ops); > _When_ this is done is the question, in my view, where the call to the registration is placed. After adding additonal operations that have to be done at "accelerator-chosen" time, it becomes more and more difficult to trace them around the codebase. Thanks, Claudio
On 18/11/20 14:48, Claudio Fontana wrote: > On 11/18/20 1:48 PM, Eduardo Habkost wrote: >> On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote: >>> apply this to the registration of the cpus accel interfaces, >>> >>> but this will be also in preparation for later use of this >>> new module init step to also defer the registration of the cpu models, >>> in order to make them subclasses of a per-accel cpu type. >>> >>> Signed-off-by: Claudio Fontana <cfontana@suse.de> >>> --- >> [...] >>> + /* >>> + * accelerator has been chosen and initialized, now it is time to >>> + * register the cpu accel interface. >>> + */ >>> + module_call_init(MODULE_INIT_ACCEL_CPU); >> >> I don't get why we would use a new module initialization level > > To have a clear point in time after which all accelerator interface initialization is done. > It avoids to have to hunt down the registration points spread around the code base. > I'd turn it around, why not? I see two disadvantages: 1) you have to hunt down accel_cpu_inits instead of looking at accelerator classes. :) 2) all callbacks have an "if (*_enabled())" around the actual meat. Another related issue is that usually the module_call_init are unconditional. I think the idea of using module_call_init is good however. What about: static void kvm_cpu_accel_init(void) { x86_cpu_accel_init(&kvm_cpu_accel); } static void kvm_cpu_accel_register(void) { accel_register_call(TYPE_KVM, kvm_cpu_accel_init); } accel_cpu_init(kvm_cpu_accel_register); ... void accel_register_call(const char *qom_type, void (*fn)(void)) { AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type)); acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn); } void accel_do_call(void *data, void *unused) { void (*fn)(void) = data; data(); } int accel_init_machine(AccelState *accel, MachineState *ms) { ... if (ret < 0) { ms->accelerator = NULL; *(acc->allowed) = false; object_unref(OBJECT(accel)); } else { object_set_accelerator_compat_props(acc->compat_props); g_slist_foreach(acc->setup_calls, accel_do_call, NULL); } return ret; } where the module_call_init would be right after MODULE_INIT_QOM Paolo >> for this. If the accelerator object was already created, we can >> just ask the existing accel object to do whatever initialization >> step is necessary. >> >> e.g. we can add a AccelClass.cpu_accel_ops field, and call: >> >> cpus_register_accel(current_machine->accelerator->cpu_accel_ops); >> > > _When_ this is done is the question, in my view, where the call to the registration is placed. > > After adding additonal operations that have to be done at "accelerator-chosen" time, it becomes more and more difficult to trace them around the codebase. > > Thanks, > > Claudio > > > >
On Wed, Nov 18, 2020 at 03:05:42PM +0100, Paolo Bonzini wrote: > On 18/11/20 14:48, Claudio Fontana wrote: > > On 11/18/20 1:48 PM, Eduardo Habkost wrote: > > > On Wed, Nov 18, 2020 at 11:29:35AM +0100, Claudio Fontana wrote: > > > > apply this to the registration of the cpus accel interfaces, > > > > > > > > but this will be also in preparation for later use of this > > > > new module init step to also defer the registration of the cpu models, > > > > in order to make them subclasses of a per-accel cpu type. > > > > > > > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > > > > --- > > > [...] > > > > + /* > > > > + * accelerator has been chosen and initialized, now it is time to > > > > + * register the cpu accel interface. > > > > + */ > > > > + module_call_init(MODULE_INIT_ACCEL_CPU); > > > > > > I don't get why we would use a new module initialization level > > > > To have a clear point in time after which all accelerator interface initialization is done. > > It avoids to have to hunt down the registration points spread around the code base. > > I'd turn it around, why not? > > I see two disadvantages: > > 1) you have to hunt down accel_cpu_inits instead of looking at accelerator > classes. :) > > 2) all callbacks have an "if (*_enabled())" around the actual meat. Another > related issue is that usually the module_call_init are unconditional. > > I think the idea of using module_call_init is good however. What about: > > static void kvm_cpu_accel_init(void) > { > x86_cpu_accel_init(&kvm_cpu_accel); What do you expect x86_cpu_accel_init() to do? > } > > static void kvm_cpu_accel_register(void) > { > accel_register_call(TYPE_KVM, kvm_cpu_accel_init); > } > accel_cpu_init(kvm_cpu_accel_register); > > ... > > void > accel_register_call(const char *qom_type, void (*fn)(void)) > { > AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type)); > > acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn); > } > > void > accel_do_call(void *data, void *unused) > { > void (*fn)(void) = data; > > data(); > } > > int accel_init_machine(AccelState *accel, MachineState *ms) > { > ... > if (ret < 0) { > ms->accelerator = NULL; > *(acc->allowed) = false; > object_unref(OBJECT(accel)); > } else { > object_set_accelerator_compat_props(acc->compat_props); > g_slist_foreach(acc->setup_calls, accel_do_call, NULL); Why all this extra complexity if you can simply do: ACCEL_GET_CLASS(acc)->finish_arch_specific_init(); ? > } > return ret; > } > > where the module_call_init would be right after MODULE_INIT_QOM > > Paolo > > > > for this. If the accelerator object was already created, we can > > > just ask the existing accel object to do whatever initialization > > > step is necessary. > > > > > > e.g. we can add a AccelClass.cpu_accel_ops field, and call: > > > > > > cpus_register_accel(current_machine->accelerator->cpu_accel_ops); > > > > > > > _When_ this is done is the question, in my view, where the call to the registration is placed. > > > > After adding additonal operations that have to be done at > > "accelerator-chosen" time, it becomes more and more difficult > > to trace them around the codebase. I don't understand why a separate module init level is necessary here. Making sure module_call_init() is called at the correct moment is not easier or safer than just making sure accel_init_machine() (or another init function you create) is called at the correct moment.
On 18/11/20 15:36, Eduardo Habkost wrote: > On Wed, Nov 18, 2020 at 03:05:42PM +0100, Paolo Bonzini wrote: >> On 18/11/20 14:48, Claudio Fontana wrote: >>> On 11/18/20 1:48 PM, Eduardo Habkost wrote: >>>> I don't get why we would use a new module initialization level >>> >>> To have a clear point in time after which all accelerator interface initialization is done. >>> It avoids to have to hunt down the registration points spread around the code base. >>> I'd turn it around, why not? >> >> I see two disadvantages: >> >> 1) you have to hunt down accel_cpu_inits instead of looking at accelerator >> classes. :) >> >> 2) all callbacks have an "if (*_enabled())" around the actual meat. Another >> related issue is that usually the module_call_init are unconditional. >> >> I think the idea of using module_call_init is good however. What about: >> >> static void kvm_cpu_accel_init(void) >> { >> x86_cpu_accel_init(&kvm_cpu_accel); > > What do you expect x86_cpu_accel_init() to do? I don't know, the same that it was doing in Claudio's patches. :) He had if (kvm_enabled()) { x86_cpu_accel_init(&kvm_cpu_accel); } and I'm calling only the function that is registered on the enabled accelerator. > I don't understand why a separate module init level is necessary > here. Because you must call accel_register_call after the TYPE_KVM type has been registered, or object_class_by_name fails: void accel_register_call(const char *qom_type, void (*fn)(void)) { AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type)); acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn); } The alternative is to store the (type, function) tuple directly, with the type as a string. Then you can just use type_init. > Making sure module_call_init() is called at the correct moment is > not easier or safer than just making sure accel_init_machine() > (or another init function you create) is called at the correct > moment. Since there is a way to do it without a new level, that would of course be fine for me too. Let me explain however why I think Claudio's design had module_call_init() misplaced and what the fundamental difference is. The basic phases in qemu_init() are: - initialize stuff - parse command line - create machine - create accelerator - initialize machine - create devices - start with a mess of other object creation sprinkled between the various phases (but we don't care about those). What I object to, is calling module_call_init() after the "initialize stuff" phase. Claudio was using it to call the function directly, so it had to be exactly at "create accelerator". This is different from all other module_call_init() calls, which are done very early. With the implementation I sketched, accel_register_call must still be done after type_init, so there's still an ordering constraint, but all it's doing is registering a callback in the "initialize stuff" phase. Thanks, Paolo
On Wed, Nov 18, 2020 at 03:51:44PM +0100, Paolo Bonzini wrote: > On 18/11/20 15:36, Eduardo Habkost wrote: > > On Wed, Nov 18, 2020 at 03:05:42PM +0100, Paolo Bonzini wrote: > > > On 18/11/20 14:48, Claudio Fontana wrote: > > > > On 11/18/20 1:48 PM, Eduardo Habkost wrote: > > > > > I don't get why we would use a new module initialization level > > > > > > > > To have a clear point in time after which all accelerator interface initialization is done. > > > > It avoids to have to hunt down the registration points spread around the code base. > > > > I'd turn it around, why not? > > > > > > I see two disadvantages: > > > > > > 1) you have to hunt down accel_cpu_inits instead of looking at accelerator > > > classes. :) > > > > > > 2) all callbacks have an "if (*_enabled())" around the actual meat. Another > > > related issue is that usually the module_call_init are unconditional. > > > > > > I think the idea of using module_call_init is good however. What about: > > > > > > static void kvm_cpu_accel_init(void) > > > { > > > x86_cpu_accel_init(&kvm_cpu_accel); > > > > What do you expect x86_cpu_accel_init() to do? > > I don't know, the same that it was doing in Claudio's patches. :) > > He had > > if (kvm_enabled()) { > x86_cpu_accel_init(&kvm_cpu_accel); > } > > and I'm calling only the function that is registered on the enabled > accelerator. > > > I don't understand why a separate module init level is necessary > > here. > > Because you must call accel_register_call after the TYPE_KVM type has been > registered, or object_class_by_name fails: > > void > accel_register_call(const char *qom_type, void (*fn)(void)) > { > AccelClass *acc = ACCEL_CLASS(object_class_by_name(qom_type)); > > acc->setup_calls = g_slist_append(acc->setup_calls, (void *)fn); > } > > The alternative is to store the (type, function) tuple directly, with the > type as a string. Then you can just use type_init. Right. Let's build on top of that: Another alternative would be to store a (type, X86CPUAccel) tuple directly, with the type as string. This would save the extra indirection of the x86_cpu_accel_init() call. It turns out we already have a mechanism to register and store (type, StructContainingFunctionPointers) tuples at initialization time: QOM. X86CPUAccel can become X86CPUAccelClass, and be registered as a QOM type. It could be a subtype of TYPE_ACCEL or not, it shouldn't matter. I remember this was suggested in a previous thread, but I don't remember if there were any objections. > > > Making sure module_call_init() is called at the correct moment is > > not easier or safer than just making sure accel_init_machine() > > (or another init function you create) is called at the correct > > moment. > > Since there is a way to do it without a new level, that would of course be > fine for me too. Let me explain however why I think Claudio's design had > module_call_init() misplaced and what the fundamental difference is. The > basic phases in qemu_init() are: > > - initialize stuff > - parse command line > - create machine > - create accelerator > - initialize machine > - create devices > - start > > with a mess of other object creation sprinkled between the various phases > (but we don't care about those). > > What I object to, is calling module_call_init() after the "initialize stuff" > phase. Claudio was using it to call the function directly, so it had to be > exactly at "create accelerator". This is different from all other > module_call_init() calls, which are done very early. I agree. > > With the implementation I sketched, accel_register_call must still be done > after type_init, so there's still an ordering constraint, but all it's doing > is registering a callback in the "initialize stuff" phase. Makes sense, if we really want to introduce a new accel_register_call() abstraction. I don't think we need it, though.
Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > > > The alternative is to store the (type, function) tuple directly, with the > > type as a string. Then you can just use type_init. > > Right. Let's build on top of that: > > Another alternative would be to store a (type, X86CPUAccel) tuple > directly, with the type as string. This would save the extra > indirection of the x86_cpu_accel_init() call. > > It turns out we already have a mechanism to register and store > (type, StructContainingFunctionPointers) tuples at initialization > time: QOM. > > X86CPUAccel can become X86CPUAccelClass, and be registered as a > QOM type. It could be a subtype of TYPE_ACCEL or not, it > shouldn't matter. > It would be a weird type that isn't instantiated, and/or that does nothing but monkey patching other classes. I don't think it's a good fit. Yet another possibility is to use GHashTable. It is limited to one value per key, but it's enough if everything is kept local to {hw,target}/i386. If needed a new function pointer can be added to MachineClass, implemented in X86MachineState (where the GHashTable would also be) and called in accel.c. Paolo Paolo > I remember this was suggested in a previous thread, but I don't > remember if there were any objections. > > > > > > Making sure module_call_init() is called at the correct moment is > > > not easier or safer than just making sure accel_init_machine() > > > (or another init function you create) is called at the correct > > > moment. > > > > Since there is a way to do it without a new level, that would of course > be > > fine for me too. Let me explain however why I think Claudio's design had > > module_call_init() misplaced and what the fundamental difference is. The > > basic phases in qemu_init() are: > > > > - initialize stuff > > - parse command line > > - create machine > > - create accelerator > > - initialize machine > > - create devices > > - start > > > > with a mess of other object creation sprinkled between the various phases > > (but we don't care about those). > > > > What I object to, is calling module_call_init() after the "initialize > stuff" > > phase. Claudio was using it to call the function directly, so it had to > be > > exactly at "create accelerator". This is different from all other > > module_call_init() calls, which are done very early. > > I agree. > > > > > With the implementation I sketched, accel_register_call must still be > done > > after type_init, so there's still an ordering constraint, but all it's > doing > > is registering a callback in the "initialize stuff" phase. > > Makes sense, if we really want to introduce a new accel_register_call() > abstraction. I don't think we need it, though. > > -- > Eduardo > >
On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote: > Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > > > > > > The alternative is to store the (type, function) tuple directly, with the > > > type as a string. Then you can just use type_init. > > > > Right. Let's build on top of that: > > > > Another alternative would be to store a (type, X86CPUAccel) tuple > > directly, with the type as string. This would save the extra > > indirection of the x86_cpu_accel_init() call. > > > > It turns out we already have a mechanism to register and store > > (type, StructContainingFunctionPointers) tuples at initialization > > time: QOM. > > > > X86CPUAccel can become X86CPUAccelClass, and be registered as a > > QOM type. It could be a subtype of TYPE_ACCEL or not, it > > shouldn't matter. > > > > It would be a weird type that isn't instantiated, and/or that does nothing > but monkey patching other classes. I don't think it's a good fit. The whole point of this would be to avoid monkey patching other classes. Why wouldn't we instantiate it? There's a huge number of static variables in target/i386/kvm.c that could be moved to that object. Sounds like a perfect fit for me. I won't try to stop you if you really want to invent a brand new (name => CollectionOfFunctionPointers) registry, but it seems unnecessary. > > Yet another possibility is to use GHashTable. It is limited to one value > per key, but it's enough if everything is kept local to {hw,target}/i386. > If needed a new function pointer can be added to MachineClass, implemented > in X86MachineState (where the GHashTable would also be) and called in > accel.c. > > Paolo > > Paolo > > > > I remember this was suggested in a previous thread, but I don't > > remember if there were any objections. > > > > > > > > > Making sure module_call_init() is called at the correct moment is > > > > not easier or safer than just making sure accel_init_machine() > > > > (or another init function you create) is called at the correct > > > > moment. > > > > > > Since there is a way to do it without a new level, that would of course > > be > > > fine for me too. Let me explain however why I think Claudio's design had > > > module_call_init() misplaced and what the fundamental difference is. The > > > basic phases in qemu_init() are: > > > > > > - initialize stuff > > > - parse command line > > > - create machine > > > - create accelerator > > > - initialize machine > > > - create devices > > > - start > > > > > > with a mess of other object creation sprinkled between the various phases > > > (but we don't care about those). > > > > > > What I object to, is calling module_call_init() after the "initialize > > stuff" > > > phase. Claudio was using it to call the function directly, so it had to > > be > > > exactly at "create accelerator". This is different from all other > > > module_call_init() calls, which are done very early. > > > > I agree. > > > > > > > > With the implementation I sketched, accel_register_call must still be > > done > > > after type_init, so there's still an ordering constraint, but all it's > > doing > > > is registering a callback in the "initialize stuff" phase. > > > > Makes sense, if we really want to introduce a new accel_register_call() > > abstraction. I don't think we need it, though. > > > > -- > > Eduardo > > > >
Il mer 18 nov 2020, 17:11 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote: > > Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabkost@redhat.com> ha > scritto: > > > > > > > > > The alternative is to store the (type, function) tuple directly, > with the > > > > type as a string. Then you can just use type_init. > > > > > > Right. Let's build on top of that: > > > > > > Another alternative would be to store a (type, X86CPUAccel) tuple > > > directly, with the type as string. This would save the extra > > > indirection of the x86_cpu_accel_init() call. > > > > > > It turns out we already have a mechanism to register and store > > > (type, StructContainingFunctionPointers) tuples at initialization > > > time: QOM. > > > > > > X86CPUAccel can become X86CPUAccelClass, and be registered as a > > > QOM type. It could be a subtype of TYPE_ACCEL or not, it > > > shouldn't matter. > > > > > > > It would be a weird type that isn't instantiated, and/or that does > nothing > > but monkey patching other classes. I don't think it's a good fit. > > The whole point of this would be to avoid monkey patching other > classes. > Adding a layer of indirect calls is not very different from monkey patching though. 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. 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. Why wouldn't we instantiate it? There's a huge number of static > variables in target/i386/kvm.c that could be moved to that > object. Sounds like a perfect fit for me. > Most of those are properties of the running kernel so there's no need to move them inside an object. Paolo I won't try to stop you if you really want to invent a brand new > (name => CollectionOfFunctionPointers) registry, but it seems > unnecessary. > > > > > Yet another possibility is to use GHashTable. It is limited to one value > > per key, but it's enough if everything is kept local to {hw,target}/i386. > > If needed a new function pointer can be added to MachineClass, > implemented > > in X86MachineState (where the GHashTable would also be) and called in > > accel.c. > > > > Paolo > > > > Paolo > > > > > > > I remember this was suggested in a previous thread, but I don't > > > remember if there were any objections. > > > > > > > > > > > > Making sure module_call_init() is called at the correct moment is > > > > > not easier or safer than just making sure accel_init_machine() > > > > > (or another init function you create) is called at the correct > > > > > moment. > > > > > > > > Since there is a way to do it without a new level, that would of > course > > > be > > > > fine for me too. Let me explain however why I think Claudio's > design had > > > > module_call_init() misplaced and what the fundamental difference > is. The > > > > basic phases in qemu_init() are: > > > > > > > > - initialize stuff > > > > - parse command line > > > > - create machine > > > > - create accelerator > > > > - initialize machine > > > > - create devices > > > > - start > > > > > > > > with a mess of other object creation sprinkled between the various > phases > > > > (but we don't care about those). > > > > > > > > What I object to, is calling module_call_init() after the "initialize > > > stuff" > > > > phase. Claudio was using it to call the function directly, so it > had to > > > be > > > > exactly at "create accelerator". This is different from all other > > > > module_call_init() calls, which are done very early. > > > > > > I agree. > > > > > > > > > > > With the implementation I sketched, accel_register_call must still be > > > done > > > > after type_init, so there's still an ordering constraint, but all > it's > > > doing > > > > is registering a callback in the "initialize stuff" phase. > > > > > > Makes sense, if we really want to introduce a new accel_register_call() > > > abstraction. I don't think we need it, though. > > > > > > -- > > > Eduardo > > > > > > > > -- > Eduardo > >
On Wed, Nov 18, 2020 at 05:22:46PM +0100, Paolo Bonzini wrote: > Il mer 18 nov 2020, 17:11 Eduardo Habkost <ehabkost@redhat.com> ha scritto: > > > On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote: > > > Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabkost@redhat.com> ha > > scritto: > > > > > > > > > > > > The alternative is to store the (type, function) tuple directly, > > with the > > > > > type as a string. Then you can just use type_init. > > > > > > > > Right. Let's build on top of that: > > > > > > > > Another alternative would be to store a (type, X86CPUAccel) tuple > > > > directly, with the type as string. This would save the extra > > > > indirection of the x86_cpu_accel_init() call. > > > > > > > > It turns out we already have a mechanism to register and store > > > > (type, StructContainingFunctionPointers) tuples at initialization > > > > time: QOM. > > > > > > > > X86CPUAccel can become X86CPUAccelClass, and be registered as a > > > > QOM type. It could be a subtype of TYPE_ACCEL or not, it > > > > shouldn't matter. > > > > > > > > > > It would be a weird type that isn't instantiated, and/or that does > > nothing > > > but monkey patching other classes. I don't think it's a good fit. > > > > The whole point of this would be to avoid monkey patching other > > classes. > > > > 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. > > 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? > > Why wouldn't we instantiate it? There's a huge number of static > > variables in target/i386/kvm.c that could be moved to that > > object. Sounds like a perfect fit for me. > > > > Most of those are properties of the running kernel so there's no need to > move them inside an object. There's no need, correct. Some consistency would be nice, though. All kernel capabilities in kvm-all.c are saved in KVMState. > > Paolo > > I won't try to stop you if you really want to invent a brand new > > (name => CollectionOfFunctionPointers) registry, but it seems > > unnecessary. > > > > > > > > Yet another possibility is to use GHashTable. It is limited to one value > > > per key, but it's enough if everything is kept local to {hw,target}/i386. > > > If needed a new function pointer can be added to MachineClass, > > implemented > > > in X86MachineState (where the GHashTable would also be) and called in > > > accel.c. > > > > > > Paolo > > > > > > Paolo > > > > > > > > > > I remember this was suggested in a previous thread, but I don't > > > > remember if there were any objections. > > > > > > > > > > > > > > > Making sure module_call_init() is called at the correct moment is > > > > > > not easier or safer than just making sure accel_init_machine() > > > > > > (or another init function you create) is called at the correct > > > > > > moment. > > > > > > > > > > Since there is a way to do it without a new level, that would of > > course > > > > be > > > > > fine for me too. Let me explain however why I think Claudio's > > design had > > > > > module_call_init() misplaced and what the fundamental difference > > is. The > > > > > basic phases in qemu_init() are: > > > > > > > > > > - initialize stuff > > > > > - parse command line > > > > > - create machine > > > > > - create accelerator > > > > > - initialize machine > > > > > - create devices > > > > > - start > > > > > > > > > > with a mess of other object creation sprinkled between the various > > phases > > > > > (but we don't care about those). > > > > > > > > > > What I object to, is calling module_call_init() after the "initialize > > > > stuff" > > > > > phase. Claudio was using it to call the function directly, so it > > had to > > > > be > > > > > exactly at "create accelerator". This is different from all other > > > > > module_call_init() calls, which are done very early. > > > > > > > > I agree. > > > > > > > > > > > > > > With the implementation I sketched, accel_register_call must still be > > > > done > > > > > after type_init, so there's still an ordering constraint, but all > > it's > > > > doing > > > > > is registering a callback in the "initialize stuff" phase. > > > > > > > > Makes sense, if we really want to introduce a new accel_register_call() > > > > abstraction. I don't think we need it, though. > > > > > > > > -- > > > > Eduardo > > > > > > > > > > > > -- > > Eduardo > > > >
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. 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. >> 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. 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. Paolo
On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: > On 18/11/20 18:30, Eduardo Habkost wrote: > > > Adding a layer of indirect calls is not very different from monkey patching > > > though. > > > > I'm a little bothered by monkey patching, but I'm more > > bothered by having to: > > > > (1) register (module_init()) a function (kvm_cpu_accel_register()) that > > (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that > > (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that > > (4) will be saved in multiple QOM classes, so that > > (5) we will call the right X86CPUClass.accel method at the right moment > > (common_class_init(), instance_init(), realizefn()), > > where: > > step 4 must be done before any CPU object is created > > (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn > > will be silently ignored), and > > step 3 must be done after all QOM types were registered. > > > > > You also have to consider that accel currently does not exist in usermode > > > emulators, so that's an issue too. I would rather get a simple change in > > > quickly, instead of designing the perfect class hierarchy. > > > > It doesn't have to be perfect. I agree that simple is better. > > > > To me, registering a QOM type and looking it up when necessary is > > simpler than the above. Even if it's a weird class having no > > object instances. It probably could be an interface type. > > Registering a QOM type still has quite some boilerplate. [...] We're working on that. :) > [...] Also registering a > QOM type has a public side effect (shows up in qom-list-types). In general > I don't look at QOM unless I want its property mechanism, but maybe that's > just me. We have lots of internal-use-only types returned by qom-list-types, I don't think it's a big deal. > > > > Perhaps another idea would be to allow adding interfaces to classes > > > *separately from the registration of the types*. Then we can use it to add > > > SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and > > > add the accel object to usermode emulators. > > > > I'm not sure I follow. What do you mean by bare bones accel > > class, and when exactly would you add the new interfaces to the > > classes? > > A bare bones accel class would not have init_machine and setup_post methods; > those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have > properties (such as tb-size for TCG) and would be able to register compat > properties. Oh, I think I see. This could save us having a lot of parallel type hierarchies. > > Where would I add it, I don't know. It could be a simple public wrapper > around type_initialize_interface() if we add a new MODULE_INIT_* phase after > QOM. > > Or without adding a new phase, it could be a class_type->array of > (interface_type, init_fn) hash table. type_initialize would look up the > class_type by name, add the interfaces would to the class with > type_initialize_interface, and then call the init_fn to fill in the vtable. That sounds nice. I don't think Claudio's cleanup should be blocked until this new mechanism is ready, though. We don't really need the type representing X86CPUAccel to be a subtype of TYPE_ACCEL or an interface implemented by current_machine->accelerator, in the first version. We just need a simple way for the CPU initialization code to find the correct X86CPUAccel struct. While we don't have the new mechanism, it can be just a: object_class_by_name("%s-x86-cpu-accel" % (accel->name)) call. Below is a rough draft of what I mean. There's still lots of room for cleaning it up (especially getting rid of the X86CPUClass.common_class_init and X86CPUClass.accel fields). git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- diff --git a/include/qemu/module.h b/include/qemu/module.h index 485eda986a..944d403cbd 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -44,7 +44,6 @@ typedef enum { MODULE_INIT_BLOCK, MODULE_INIT_OPTS, MODULE_INIT_QOM, - MODULE_INIT_ACCEL_CPU, MODULE_INIT_TRACE, MODULE_INIT_XEN_BACKEND, MODULE_INIT_LIBQOS, @@ -55,7 +54,6 @@ typedef enum { #define block_init(function) module_init(function, MODULE_INIT_BLOCK) #define opts_init(function) module_init(function, MODULE_INIT_OPTS) #define type_init(function) module_init(function, MODULE_INIT_QOM) -#define accel_cpu_init(function) module_init(function, MODULE_INIT_ACCEL_CPU) #define trace_init(function) module_init(function, MODULE_INIT_TRACE) #define xen_backend_init(function) module_init(function, \ MODULE_INIT_XEN_BACKEND) diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h index 032169ccd3..14491297bb 100644 --- a/include/sysemu/cpus.h +++ b/include/sysemu/cpus.h @@ -25,6 +25,9 @@ typedef struct CpuAccelOps { /* register accel-specific cpus interface implementation */ void cpus_register_accel(const CpuAccelOps *i); +/* Call arch-specific accel initialization */ +void cpu_accel_arch_init(const char *accel_name); + /* Create a dummy vcpu for CpuAccelOps->create_vcpu_thread */ void dummy_start_vcpu_thread(CPUState *); diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h index 79fcbd3b9b..eafd86dc22 100644 --- a/target/i386/cpu-qom.h +++ b/target/i386/cpu-qom.h @@ -34,7 +34,7 @@ OBJECT_DECLARE_TYPE(X86CPU, X86CPUClass, X86_CPU) typedef struct X86CPUModel X86CPUModel; -typedef struct X86CPUAccel X86CPUAccel; +typedef struct X86CPUAccelInterface X86CPUAccelInterface; /** * X86CPUClass: @@ -71,13 +71,11 @@ struct X86CPUClass { DeviceUnrealize parent_unrealize; DeviceReset parent_reset; - const X86CPUAccel *accel; + const X86CPUAccelInterface *accel; }; /** - * X86CPUAccel: - * @name: string name of the X86 CPU Accelerator - * + * X86CPUAccelInterface: * @common_class_init: initializer for the common cpu * @instance_init: cpu instance initialization * @realizefn: realize function, called first in x86 cpu realize @@ -85,14 +83,16 @@ struct X86CPUClass { * X86 CPU accelerator-specific CPU initializations */ -struct X86CPUAccel { - const char *name; - +struct X86CPUAccelInterface { + ObjectClass parent_class; void (*common_class_init)(X86CPUClass *xcc); void (*instance_init)(X86CPU *cpu); void (*realizefn)(X86CPU *cpu, Error **errp); }; -void x86_cpu_accel_init(const X86CPUAccel *accel); +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); + +#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'))
On 11/18/20 11:07 PM, Eduardo Habkost wrote: > On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: >> On 18/11/20 18:30, Eduardo Habkost wrote: >>>> Adding a layer of indirect calls is not very different from monkey patching >>>> though. >>> >>> I'm a little bothered by monkey patching, but I'm more >>> bothered by having to: >>> >>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that >>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that >>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that >>> (4) will be saved in multiple QOM classes, so that >>> (5) we will call the right X86CPUClass.accel method at the right moment >>> (common_class_init(), instance_init(), realizefn()), >>> where: >>> step 4 must be done before any CPU object is created >>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn >>> will be silently ignored), and >>> step 3 must be done after all QOM types were registered. >>> >>>> You also have to consider that accel currently does not exist in usermode >>>> emulators, so that's an issue too. I would rather get a simple change in >>>> quickly, instead of designing the perfect class hierarchy. >>> >>> It doesn't have to be perfect. I agree that simple is better. >>> >>> To me, registering a QOM type and looking it up when necessary is >>> simpler than the above. Even if it's a weird class having no >>> object instances. It probably could be an interface type. >> >> Registering a QOM type still has quite some boilerplate. [...] > > We're working on that. :) > >> [...] Also registering a >> QOM type has a public side effect (shows up in qom-list-types). In general >> I don't look at QOM unless I want its property mechanism, but maybe that's >> just me. > > We have lots of internal-use-only types returned by > qom-list-types, I don't think it's a big deal. > >> >>>> Perhaps another idea would be to allow adding interfaces to classes >>>> *separately from the registration of the types*. Then we can use it to add >>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and >>>> add the accel object to usermode emulators. >>> >>> I'm not sure I follow. What do you mean by bare bones accel >>> class, and when exactly would you add the new interfaces to the >>> classes? >> >> A bare bones accel class would not have init_machine and setup_post methods; >> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have >> properties (such as tb-size for TCG) and would be able to register compat >> properties. > > Oh, I think I see. This could save us having a lot of parallel type > hierarchies. > >> >> Where would I add it, I don't know. It could be a simple public wrapper >> around type_initialize_interface() if we add a new MODULE_INIT_* phase after >> QOM. >> >> Or without adding a new phase, it could be a class_type->array of >> (interface_type, init_fn) hash table. type_initialize would look up the >> class_type by name, add the interfaces would to the class with >> type_initialize_interface, and then call the init_fn to fill in the vtable. > > That sounds nice. I don't think Claudio's cleanup should be > blocked until this new mechanism is ready, though. > > We don't really need the type representing X86CPUAccel to be a > subtype of TYPE_ACCEL or an interface implemented by > current_machine->accelerator, in the first version. We just need > a simple way for the CPU initialization code to find the correct > X86CPUAccel struct. > > While we don't have the new mechanism, it can be just a: > object_class_by_name("%s-x86-cpu-accel" % (accel->name)) > call. > > Below is a rough draft of what I mean. There's still lots of > room for cleaning it up (especially getting rid of the > X86CPUClass.common_class_init and X86CPUClass.accel fields). > > git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > diff --git a/include/qemu/module.h b/include/qemu/module.h > index 485eda986a..944d403cbd 100644 > --- a/include/qemu/module.h > +++ b/include/qemu/module.h > @@ -44,7 +44,6 @@ typedef enum { > MODULE_INIT_BLOCK, > MODULE_INIT_OPTS, > MODULE_INIT_QOM, > - MODULE_INIT_ACCEL_CPU, > MODULE_INIT_TRACE, > MODULE_INIT_XEN_BACKEND, > MODULE_INIT_LIBQOS, > @@ -55,7 +54,6 @@ typedef enum { > #define block_init(function) module_init(function, MODULE_INIT_BLOCK) > #define opts_init(function) module_init(function, MODULE_INIT_OPTS) > #define type_init(function) module_init(function, MODULE_INIT_QOM) > -#define accel_cpu_init(function) module_init(function, MODULE_INIT_ACCEL_CPU) > #define trace_init(function) module_init(function, MODULE_INIT_TRACE) > #define xen_backend_init(function) module_init(function, \ > MODULE_INIT_XEN_BACKEND) > diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h > index 032169ccd3..14491297bb 100644 > --- a/include/sysemu/cpus.h > +++ b/include/sysemu/cpus.h > @@ -25,6 +25,9 @@ typedef struct CpuAccelOps { > /* register accel-specific cpus interface implementation */ > void cpus_register_accel(const CpuAccelOps *i); > > +/* Call arch-specific accel initialization */ > +void cpu_accel_arch_init(const char *accel_name); > + > /* Create a dummy vcpu for CpuAccelOps->create_vcpu_thread */ > void dummy_start_vcpu_thread(CPUState *); > > diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h > index 79fcbd3b9b..eafd86dc22 100644 > --- a/target/i386/cpu-qom.h > +++ b/target/i386/cpu-qom.h > @@ -34,7 +34,7 @@ OBJECT_DECLARE_TYPE(X86CPU, X86CPUClass, > X86_CPU) > > typedef struct X86CPUModel X86CPUModel; > -typedef struct X86CPUAccel X86CPUAccel; > +typedef struct X86CPUAccelInterface X86CPUAccelInterface; > > /** > * X86CPUClass: > @@ -71,13 +71,11 @@ struct X86CPUClass { > DeviceUnrealize parent_unrealize; > DeviceReset parent_reset; > > - const X86CPUAccel *accel; > + const X86CPUAccelInterface *accel; > }; > > /** > - * X86CPUAccel: > - * @name: string name of the X86 CPU Accelerator > - * > + * X86CPUAccelInterface: > * @common_class_init: initializer for the common cpu > * @instance_init: cpu instance initialization > * @realizefn: realize function, called first in x86 cpu realize > @@ -85,14 +83,16 @@ struct X86CPUClass { > * X86 CPU accelerator-specific CPU initializations > */ > > -struct X86CPUAccel { > - const char *name; > - > +struct X86CPUAccelInterface { > + ObjectClass parent_class; > void (*common_class_init)(X86CPUClass *xcc); > void (*instance_init)(X86CPU *cpu); > void (*realizefn)(X86CPU *cpu, Error **errp); > }; > > -void x86_cpu_accel_init(const X86CPUAccel *accel); > +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" > +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); I am not exactly sure what precisely you are doing here, I get the general intention to use the existing interface-related "stuff" in QOM, but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? Thanks, Claudio > + > +#define X86_CPU_ACCEL_NAME(acc) (acc "-x86-cpu-accel") > > #endif > diff --git a/bsd-user/main.c b/bsd-user/main.c > index 9f88ae952a..6107c8ca24 100644 > --- a/bsd-user/main.c > +++ b/bsd-user/main.c > @@ -909,7 +909,8 @@ int main(int argc, char **argv) > > /* init tcg before creating CPUs and to get qemu_host_page_size */ > tcg_exec_init(0); > - module_call_init(MODULE_INIT_ACCEL_CPU); > + cpu_accel_arch_init("tcg"); > + > > cpu_type = parse_cpu_option(cpu_model); > cpu = cpu_create(cpu_type); > diff --git a/linux-user/main.c b/linux-user/main.c > index a745901d86..c36564fd61 100644 > --- a/linux-user/main.c > +++ b/linux-user/main.c > @@ -704,7 +704,7 @@ int main(int argc, char **argv, char **envp) > > /* init tcg before creating CPUs and to get qemu_host_page_size */ > tcg_exec_init(0); > - module_call_init(MODULE_INIT_ACCEL_CPU); > + cpu_accel_arch_init("tcg"); > > cpu = cpu_create(cpu_type); > env = cpu->env_ptr; > diff --git a/softmmu/vl.c b/softmmu/vl.c > index df4bed056a..b90d107475 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2744,6 +2744,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp) > return 0; > } > > + cpu_accel_arch_init(acc); > return 1; > } > > @@ -4173,12 +4174,6 @@ void qemu_init(int argc, char **argv, char **envp) > */ > configure_accelerators(argv[0]); > > - /* > - * accelerator has been chosen and initialized, now it is time to > - * register the cpu accel interface. > - */ > - module_call_init(MODULE_INIT_ACCEL_CPU); > - > /* > * Beware, QOM objects created before this point miss global and > * compat properties. > diff --git a/stubs/cpu_accel_arch_init.c b/stubs/cpu_accel_arch_init.c > new file mode 100644 > index 0000000000..b80cbdd847 > --- /dev/null > +++ b/stubs/cpu_accel_arch_init.c > @@ -0,0 +1,6 @@ > +#include "qemu/osdep.h" > +#include "sysemu/cpus.h" > + > +void cpu_accel_arch_init(const char *accel_name) > +{ > +} > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b53e958926..b91e0b44ca 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -7041,6 +7041,12 @@ static const TypeInfo x86_base_cpu_type_info = { > .class_init = x86_cpu_base_class_init, > }; > > +static const TypeInfo x86_cpu_accel_type_info = { > + .name = TYPE_X86_CPU_ACCEL, > + .parent = TYPE_INTERFACE, > + .class_size = sizeof(X86CPUAccelInterface), > +}; > + > static void x86_cpu_register_types(void) > { > int i; > @@ -7051,6 +7057,7 @@ static void x86_cpu_register_types(void) > } > type_register_static(&max_x86_cpu_type_info); > type_register_static(&x86_base_cpu_type_info); > + type_register_static(&x86_cpu_accel_type_info); > } > > type_init(x86_cpu_register_types) > @@ -7058,13 +7065,22 @@ type_init(x86_cpu_register_types) > static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque) > { > X86CPUClass *xcc = X86_CPU_CLASS(klass); > - const X86CPUAccel **accel = opaque; > + const X86CPUAccelInterface **accel = opaque; > > xcc->accel = *accel; > xcc->accel->common_class_init(xcc); > } > > -void x86_cpu_accel_init(const X86CPUAccel *accel) > +static void x86_cpu_accel_init(const X86CPUAccelInterface *accel) > { > object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, &accel); > } > + > +void cpu_accel_arch_init(const char *accel_name) > +{ > + g_autofree char *cpu_accel_name = > + g_strdup_printf(X86_CPU_ACCEL_NAME("%s"), accel_name); > + X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(object_class_by_name(cpu_accel_name)); > + assert(acc); > + x86_cpu_accel_init(acc); > +} > diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c > index 29e672191f..358351018f 100644 > --- a/target/i386/hvf/cpu.c > +++ b/target/i386/hvf/cpu.c > @@ -46,19 +46,23 @@ static void hvf_cpu_instance_init(X86CPU *cpu) > } > } > > -static const X86CPUAccel hvf_cpu_accel = { > - .name = TYPE_X86_CPU "-hvf", > +static void hvf_cpu_accel_interface_init(ObjectClass *oc, void *data) > +{ > + X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc); > + acc->realizefn = host_cpu_realizefn; > + acc->common_class_init = hvf_cpu_common_class_init; > + acc->instance_init = hvf_cpu_instance_init; > +}; > > - .realizefn = host_cpu_realizefn, > - .common_class_init = hvf_cpu_common_class_init, > - .instance_init = hvf_cpu_instance_init, > +static const TypeInfo hvf_cpu_accel_type_info = { > + .name = X86_CPU_ACCEL_NAME("hvf"), > + .parent = TYPE_X86_CPU_ACCEL, > + .class_init = hvf_cpu_accel_interface_init, > }; > > static void hvf_cpu_accel_init(void) > { > - if (hvf_enabled()) { > - x86_cpu_accel_init(&hvf_cpu_accel); > - } > + type_register_static(&hvf_cpu_accel_type_info); > } > > -accel_cpu_init(hvf_cpu_accel_init); > +type_init(hvf_cpu_accel_init); > diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c > index 76982865eb..b6a1a4d200 100644 > --- a/target/i386/kvm/cpu.c > +++ b/target/i386/kvm/cpu.c > @@ -128,18 +128,23 @@ static void kvm_cpu_instance_init(X86CPU *cpu) > } > } > > -static const X86CPUAccel kvm_cpu_accel = { > - .name = TYPE_X86_CPU "-kvm", > +static void kvm_cpu_accel_interface_init(ObjectClass *oc, void *data) > +{ > + X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc); > + acc->realizefn = kvm_cpu_realizefn; > + acc->common_class_init = kvm_cpu_common_class_init; > + acc->instance_init = kvm_cpu_instance_init; > +}; > > - .realizefn = kvm_cpu_realizefn, > - .common_class_init = kvm_cpu_common_class_init, > - .instance_init = kvm_cpu_instance_init, > +static const TypeInfo kvm_cpu_accel_type_info = { > + .name = X86_CPU_ACCEL_NAME("kvm"), > + .parent = TYPE_X86_CPU_ACCEL, > + .class_init = kvm_cpu_accel_interface_init, > }; > > static void kvm_cpu_accel_init(void) > { > - if (kvm_enabled()) { > - x86_cpu_accel_init(&kvm_cpu_accel); > - } > + type_register_static(&kvm_cpu_accel_type_info); > } > -accel_cpu_init(kvm_cpu_accel_init); > + > +type_init(kvm_cpu_accel_init); > diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c > index 25cf4cfb46..0321688cd3 100644 > --- a/target/i386/tcg/cpu.c > +++ b/target/i386/tcg/cpu.c > @@ -150,19 +150,23 @@ static void tcg_cpu_instance_init(X86CPU *cpu) > x86_cpu_apply_props(cpu, tcg_default_props); > } > > -static const X86CPUAccel tcg_cpu_accel = { > - .name = TYPE_X86_CPU "-tcg", > +static void tcg_cpu_accel_interface_init(ObjectClass *oc, void *data) > +{ > + X86CPUAccelInterface *acc = X86_CPU_ACCEL_CLASS(oc); > + acc->realizefn = tcg_cpu_realizefn; > + acc->common_class_init = tcg_cpu_common_class_init; > + acc->instance_init = tcg_cpu_instance_init; > +}; > > - .realizefn = tcg_cpu_realizefn, > - .common_class_init = tcg_cpu_common_class_init, > - .instance_init = tcg_cpu_instance_init, > +static const TypeInfo tcg_cpu_accel_type_info = { > + .name = X86_CPU_ACCEL_NAME("tcg"), > + .parent = TYPE_X86_CPU_ACCEL, > + .class_init = tcg_cpu_accel_interface_init, > }; > > static void tcg_cpu_accel_init(void) > { > - if (tcg_enabled()) { > - x86_cpu_accel_init(&tcg_cpu_accel); > - } > + type_register_static(&tcg_cpu_accel_type_info); > } > > -accel_cpu_init(tcg_cpu_accel_init); > +type_init(tcg_cpu_accel_init); > diff --git a/stubs/meson.build b/stubs/meson.build > index 82b7ba60ab..1d66de1fae 100644 > --- a/stubs/meson.build > +++ b/stubs/meson.build > @@ -1,4 +1,5 @@ > stub_ss.add(files('arch_type.c')) > +stub_ss.add(files('cpu_accel_arch_init.c')) > stub_ss.add(files('bdrv-next-monitor-owned.c')) > stub_ss.add(files('blk-commit-all.c')) > stub_ss.add(files('blk-exp-close-all.c')) > > >
On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: > On 11/18/20 11:07 PM, Eduardo Habkost wrote: > > On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: > >> On 18/11/20 18:30, Eduardo Habkost wrote: > >>>> Adding a layer of indirect calls is not very different from monkey patching > >>>> though. > >>> > >>> I'm a little bothered by monkey patching, but I'm more > >>> bothered by having to: > >>> > >>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that > >>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that > >>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that > >>> (4) will be saved in multiple QOM classes, so that > >>> (5) we will call the right X86CPUClass.accel method at the right moment > >>> (common_class_init(), instance_init(), realizefn()), > >>> where: > >>> step 4 must be done before any CPU object is created > >>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn > >>> will be silently ignored), and > >>> step 3 must be done after all QOM types were registered. > >>> > >>>> You also have to consider that accel currently does not exist in usermode > >>>> emulators, so that's an issue too. I would rather get a simple change in > >>>> quickly, instead of designing the perfect class hierarchy. > >>> > >>> It doesn't have to be perfect. I agree that simple is better. > >>> > >>> To me, registering a QOM type and looking it up when necessary is > >>> simpler than the above. Even if it's a weird class having no > >>> object instances. It probably could be an interface type. > >> > >> Registering a QOM type still has quite some boilerplate. [...] > > > > We're working on that. :) > > > >> [...] Also registering a > >> QOM type has a public side effect (shows up in qom-list-types). In general > >> I don't look at QOM unless I want its property mechanism, but maybe that's > >> just me. > > > > We have lots of internal-use-only types returned by > > qom-list-types, I don't think it's a big deal. > > > >> > >>>> Perhaps another idea would be to allow adding interfaces to classes > >>>> *separately from the registration of the types*. Then we can use it to add > >>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and > >>>> add the accel object to usermode emulators. > >>> > >>> I'm not sure I follow. What do you mean by bare bones accel > >>> class, and when exactly would you add the new interfaces to the > >>> classes? > >> > >> A bare bones accel class would not have init_machine and setup_post methods; > >> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have > >> properties (such as tb-size for TCG) and would be able to register compat > >> properties. [1] > > > > Oh, I think I see. This could save us having a lot of parallel type > > hierarchies. > > > >> > >> Where would I add it, I don't know. It could be a simple public wrapper > >> around type_initialize_interface() if we add a new MODULE_INIT_* phase after > >> QOM. > >> > >> Or without adding a new phase, it could be a class_type->array of > >> (interface_type, init_fn) hash table. type_initialize would look up the > >> class_type by name, add the interfaces would to the class with > >> type_initialize_interface, and then call the init_fn to fill in the vtable. > > > > That sounds nice. I don't think Claudio's cleanup should be > > blocked until this new mechanism is ready, though. > > > > We don't really need the type representing X86CPUAccel to be a > > subtype of TYPE_ACCEL or an interface implemented by > > current_machine->accelerator, in the first version. We just need > > a simple way for the CPU initialization code to find the correct > > X86CPUAccel struct. > > > > While we don't have the new mechanism, it can be just a: > > object_class_by_name("%s-x86-cpu-accel" % (accel->name)) > > call. > > > > Below is a rough draft of what I mean. There's still lots of > > room for cleaning it up (especially getting rid of the > > X86CPUClass.common_class_init and X86CPUClass.accel fields). > > > > git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> [...] > > /** > > - * X86CPUAccel: > > - * @name: string name of the X86 CPU Accelerator > > - * > > + * X86CPUAccelInterface: > > * @common_class_init: initializer for the common cpu > > * @instance_init: cpu instance initialization > > * @realizefn: realize function, called first in x86 cpu realize > > @@ -85,14 +83,16 @@ struct X86CPUClass { > > * X86 CPU accelerator-specific CPU initializations > > */ > > > > -struct X86CPUAccel { > > - const char *name; > > - > > +struct X86CPUAccelInterface { > > + ObjectClass parent_class; > > void (*common_class_init)(X86CPUClass *xcc); > > void (*instance_init)(X86CPU *cpu); > > void (*realizefn)(X86CPU *cpu, Error **errp); > > }; > > > > -void x86_cpu_accel_init(const X86CPUAccel *accel); > > +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" > > +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); > > > I am not exactly sure what precisely you are doing here, > > I get the general intention to use the existing interface-related "stuff" in QOM, > but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. See the git URL I sent above, for other related changes: https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > > Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? An interface is declared in a very similar way, but instance_size and the instance type cast macro is a bit different (because instances of interface types are never created directly). For the draft we have here, it shouldn't make any difference if you use OBJECT_DECLARE_TYPE, because the instance type cast macros are left unused. Normally the use case for interfaces is not like what I did here. Interfaces are usually attached to other classes (to declare that object instances of that class implement the methods of that interface). Using interfaces would be just an intermediate step to the solution Paolo was mentioning (dynamically adding interface to classes, see [1] above).
On 11/20/20 6:19 PM, Eduardo Habkost wrote: > On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: >> On 11/18/20 11:07 PM, Eduardo Habkost wrote: >>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: >>>> On 18/11/20 18:30, Eduardo Habkost wrote: >>>>>> Adding a layer of indirect calls is not very different from monkey patching >>>>>> though. >>>>> >>>>> I'm a little bothered by monkey patching, but I'm more >>>>> bothered by having to: >>>>> >>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that >>>>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that >>>>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that >>>>> (4) will be saved in multiple QOM classes, so that >>>>> (5) we will call the right X86CPUClass.accel method at the right moment >>>>> (common_class_init(), instance_init(), realizefn()), >>>>> where: >>>>> step 4 must be done before any CPU object is created >>>>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn >>>>> will be silently ignored), and >>>>> step 3 must be done after all QOM types were registered. >>>>> >>>>>> You also have to consider that accel currently does not exist in usermode >>>>>> emulators, so that's an issue too. I would rather get a simple change in >>>>>> quickly, instead of designing the perfect class hierarchy. >>>>> >>>>> It doesn't have to be perfect. I agree that simple is better. >>>>> >>>>> To me, registering a QOM type and looking it up when necessary is >>>>> simpler than the above. Even if it's a weird class having no >>>>> object instances. It probably could be an interface type. >>>> >>>> Registering a QOM type still has quite some boilerplate. [...] >>> >>> We're working on that. :) >>> >>>> [...] Also registering a >>>> QOM type has a public side effect (shows up in qom-list-types). In general >>>> I don't look at QOM unless I want its property mechanism, but maybe that's >>>> just me. >>> >>> We have lots of internal-use-only types returned by >>> qom-list-types, I don't think it's a big deal. >>> >>>> >>>>>> Perhaps another idea would be to allow adding interfaces to classes >>>>>> *separately from the registration of the types*. Then we can use it to add >>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and >>>>>> add the accel object to usermode emulators. >>>>> >>>>> I'm not sure I follow. What do you mean by bare bones accel >>>>> class, and when exactly would you add the new interfaces to the >>>>> classes? >>>> >>>> A bare bones accel class would not have init_machine and setup_post methods; >>>> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have >>>> properties (such as tb-size for TCG) and would be able to register compat >>>> properties. > > [1] > >>> >>> Oh, I think I see. This could save us having a lot of parallel type >>> hierarchies. >>> >>>> >>>> Where would I add it, I don't know. It could be a simple public wrapper >>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase after >>>> QOM. >>>> >>>> Or without adding a new phase, it could be a class_type->array of >>>> (interface_type, init_fn) hash table. type_initialize would look up the >>>> class_type by name, add the interfaces would to the class with >>>> type_initialize_interface, and then call the init_fn to fill in the vtable. >>> >>> That sounds nice. I don't think Claudio's cleanup should be >>> blocked until this new mechanism is ready, though. >>> >>> We don't really need the type representing X86CPUAccel to be a >>> subtype of TYPE_ACCEL or an interface implemented by >>> current_machine->accelerator, in the first version. We just need >>> a simple way for the CPU initialization code to find the correct >>> X86CPUAccel struct. >>> >>> While we don't have the new mechanism, it can be just a: >>> object_class_by_name("%s-x86-cpu-accel" % (accel->name)) >>> call. >>> >>> Below is a rough draft of what I mean. There's still lots of >>> room for cleaning it up (especially getting rid of the >>> X86CPUClass.common_class_init and X86CPUClass.accel fields). >>> >>> git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > [...] >>> /** >>> - * X86CPUAccel: >>> - * @name: string name of the X86 CPU Accelerator >>> - * >>> + * X86CPUAccelInterface: >>> * @common_class_init: initializer for the common cpu >>> * @instance_init: cpu instance initialization >>> * @realizefn: realize function, called first in x86 cpu realize >>> @@ -85,14 +83,16 @@ struct X86CPUClass { >>> * X86 CPU accelerator-specific CPU initializations >>> */ >>> >>> -struct X86CPUAccel { >>> - const char *name; >>> - >>> +struct X86CPUAccelInterface { >>> + ObjectClass parent_class; >>> void (*common_class_init)(X86CPUClass *xcc); >>> void (*instance_init)(X86CPU *cpu); >>> void (*realizefn)(X86CPU *cpu, Error **errp); >>> }; >>> >>> -void x86_cpu_accel_init(const X86CPUAccel *accel); >>> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" >>> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); >> >> >> I am not exactly sure what precisely you are doing here, >> >> I get the general intention to use the existing interface-related "stuff" in QOM, >> but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. > > See the git URL I sent above, for other related changes: > > https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel Aaah I missed this, there are quite a few more changes there; for me it's great if you take it from there, I see you are developing a solution on top of the previous series. > >> >> Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? > > An interface is declared in a very similar way, but instance_size > and the instance type cast macro is a bit different (because > instances of interface types are never created directly). > > For the draft we have here, it shouldn't make any difference if > you use OBJECT_DECLARE_TYPE, because the instance type cast > macros are left unused. > > Normally the use case for interfaces is not like what I did here. > Interfaces are usually attached to other classes (to declare that > object instances of that class implement the methods of that > interface). Using interfaces would be just an intermediate step > to the solution Paolo was mentioning (dynamically adding > interface to classes, see [1] above). > Makes sense to me, let me know how you guys would like to proceed from here. The thing I am still uncertain about, looking at your tree at: https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel is the removal of MODULE_INIT_ACCEL_CPU, it would be way simpler to understand I think, both for CpuAccelOps and X86CPUAccel, and is actualy in my view a perfect fit for the problem that module_call_init is supposed to solve. But, my 2c of course, Ciao, Claudio
On Fri, Nov 20, 2020 at 06:41:35PM +0100, Claudio Fontana wrote: > On 11/20/20 6:19 PM, Eduardo Habkost wrote: > > On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: > >> On 11/18/20 11:07 PM, Eduardo Habkost wrote: > >>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: > >>>> On 18/11/20 18:30, Eduardo Habkost wrote: > >>>>>> Adding a layer of indirect calls is not very different from monkey patching > >>>>>> though. > >>>>> > >>>>> I'm a little bothered by monkey patching, but I'm more > >>>>> bothered by having to: > >>>>> > >>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that > >>>>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that > >>>>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that > >>>>> (4) will be saved in multiple QOM classes, so that > >>>>> (5) we will call the right X86CPUClass.accel method at the right moment > >>>>> (common_class_init(), instance_init(), realizefn()), > >>>>> where: > >>>>> step 4 must be done before any CPU object is created > >>>>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn > >>>>> will be silently ignored), and > >>>>> step 3 must be done after all QOM types were registered. > >>>>> > >>>>>> You also have to consider that accel currently does not exist in usermode > >>>>>> emulators, so that's an issue too. I would rather get a simple change in > >>>>>> quickly, instead of designing the perfect class hierarchy. > >>>>> > >>>>> It doesn't have to be perfect. I agree that simple is better. > >>>>> > >>>>> To me, registering a QOM type and looking it up when necessary is > >>>>> simpler than the above. Even if it's a weird class having no > >>>>> object instances. It probably could be an interface type. > >>>> > >>>> Registering a QOM type still has quite some boilerplate. [...] > >>> > >>> We're working on that. :) > >>> > >>>> [...] Also registering a > >>>> QOM type has a public side effect (shows up in qom-list-types). In general > >>>> I don't look at QOM unless I want its property mechanism, but maybe that's > >>>> just me. > >>> > >>> We have lots of internal-use-only types returned by > >>> qom-list-types, I don't think it's a big deal. > >>> > >>>> > >>>>>> Perhaps another idea would be to allow adding interfaces to classes > >>>>>> *separately from the registration of the types*. Then we can use it to add > >>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and > >>>>>> add the accel object to usermode emulators. > >>>>> > >>>>> I'm not sure I follow. What do you mean by bare bones accel > >>>>> class, and when exactly would you add the new interfaces to the > >>>>> classes? > >>>> > >>>> A bare bones accel class would not have init_machine and setup_post methods; > >>>> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have > >>>> properties (such as tb-size for TCG) and would be able to register compat > >>>> properties. > > > > [1] > > > >>> > >>> Oh, I think I see. This could save us having a lot of parallel type > >>> hierarchies. > >>> > >>>> > >>>> Where would I add it, I don't know. It could be a simple public wrapper > >>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase after > >>>> QOM. > >>>> > >>>> Or without adding a new phase, it could be a class_type->array of > >>>> (interface_type, init_fn) hash table. type_initialize would look up the > >>>> class_type by name, add the interfaces would to the class with > >>>> type_initialize_interface, and then call the init_fn to fill in the vtable. > >>> > >>> That sounds nice. I don't think Claudio's cleanup should be > >>> blocked until this new mechanism is ready, though. > >>> > >>> We don't really need the type representing X86CPUAccel to be a > >>> subtype of TYPE_ACCEL or an interface implemented by > >>> current_machine->accelerator, in the first version. We just need > >>> a simple way for the CPU initialization code to find the correct > >>> X86CPUAccel struct. > >>> > >>> While we don't have the new mechanism, it can be just a: > >>> object_class_by_name("%s-x86-cpu-accel" % (accel->name)) > >>> call. > >>> > >>> Below is a rough draft of what I mean. There's still lots of > >>> room for cleaning it up (especially getting rid of the > >>> X86CPUClass.common_class_init and X86CPUClass.accel fields). > >>> > >>> git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > >>> > >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > [...] > >>> /** > >>> - * X86CPUAccel: > >>> - * @name: string name of the X86 CPU Accelerator > >>> - * > >>> + * X86CPUAccelInterface: > >>> * @common_class_init: initializer for the common cpu > >>> * @instance_init: cpu instance initialization > >>> * @realizefn: realize function, called first in x86 cpu realize > >>> @@ -85,14 +83,16 @@ struct X86CPUClass { > >>> * X86 CPU accelerator-specific CPU initializations > >>> */ > >>> > >>> -struct X86CPUAccel { > >>> - const char *name; > >>> - > >>> +struct X86CPUAccelInterface { > >>> + ObjectClass parent_class; > >>> void (*common_class_init)(X86CPUClass *xcc); > >>> void (*instance_init)(X86CPU *cpu); > >>> void (*realizefn)(X86CPU *cpu, Error **errp); > >>> }; > >>> > >>> -void x86_cpu_accel_init(const X86CPUAccel *accel); > >>> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" > >>> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); > >> > >> > >> I am not exactly sure what precisely you are doing here, > >> > >> I get the general intention to use the existing interface-related "stuff" in QOM, > >> but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. > > > > See the git URL I sent above, for other related changes: > > > > https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > > > Aaah I missed this, there are quite a few more changes there; > > for me it's great if you take it from there, I see you are > developing a solution on top of the previous series. I'm a bit busy with other stuff, so I'm probably not going to be able to make sure the patches are in a good shape to be submitted soon. I don't want to impose any obstacles for the work you are doing, either. Please consider the patch I sent (and the git tree above) as just an example of a possible solution to the two issues Paolo raised at https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com > > > > > >> > >> Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? > > > > An interface is declared in a very similar way, but instance_size > > and the instance type cast macro is a bit different (because > > instances of interface types are never created directly). > > > > For the draft we have here, it shouldn't make any difference if > > you use OBJECT_DECLARE_TYPE, because the instance type cast > > macros are left unused. > > > > Normally the use case for interfaces is not like what I did here. > > Interfaces are usually attached to other classes (to declare that > > object instances of that class implement the methods of that > > interface). Using interfaces would be just an intermediate step > > to the solution Paolo was mentioning (dynamically adding > > interface to classes, see [1] above). > > > > Makes sense to me, > let me know how you guys would like to proceed from here. > To me, the main issue (also raised by Paolo above) is the fact that you are doing *_enabled() checks in the module init functions. Every single use case we have for module init functions today is for unconditionally registering code or data structures provided by a code module (config group names, QOM types, block backends, multifd methods, etc.), and none of them depend on runtime options (like machine or accelerator options). The x86_cpu_accel_init() calls, on the other hand, are not module initialization, but just one additional step of machine/accelerator/cpu initialization. > The thing I am still uncertain about, looking at your tree at: > > https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel > > is the removal of MODULE_INIT_ACCEL_CPU, it would be way simpler to understand I think, > both for CpuAccelOps and X86CPUAccel, and is actualy in my view a perfect fit for > the problem that module_call_init is supposed to solve. That was one of my goals. My first goal was the removal of the (hvm|kvm|tcg)_enabled() checks in the accel init functions. My secondary goal (and a side effect of the first goal) was making MODULE_INIT_ACCEL_CPU unnecessary. If we are not trying to remove the *_enabled() checks in the accel init functions (nor trying to make MODULE_INIT_ACCEL_CPU unnecessary), my suggestion of using QOM doesn't make things simpler. Let's hear what Paolo thinks. If you want to proceed with the accel_register_call() solution suggested by Paolo, that's OK to me. I just don't think we really need it, because QOM already solves the problem for us.
On 11/20/20 7:09 PM, Eduardo Habkost wrote: > On Fri, Nov 20, 2020 at 06:41:35PM +0100, Claudio Fontana wrote: >> On 11/20/20 6:19 PM, Eduardo Habkost wrote: >>> On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: >>>> On 11/18/20 11:07 PM, Eduardo Habkost wrote: >>>>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: >>>>>> On 18/11/20 18:30, Eduardo Habkost wrote: >>>>>>>> Adding a layer of indirect calls is not very different from monkey patching >>>>>>>> though. >>>>>>> >>>>>>> I'm a little bothered by monkey patching, but I'm more >>>>>>> bothered by having to: >>>>>>> >>>>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that >>>>>>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that >>>>>>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that >>>>>>> (4) will be saved in multiple QOM classes, so that >>>>>>> (5) we will call the right X86CPUClass.accel method at the right moment >>>>>>> (common_class_init(), instance_init(), realizefn()), >>>>>>> where: >>>>>>> step 4 must be done before any CPU object is created >>>>>>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn >>>>>>> will be silently ignored), and >>>>>>> step 3 must be done after all QOM types were registered. >>>>>>> >>>>>>>> You also have to consider that accel currently does not exist in usermode >>>>>>>> emulators, so that's an issue too. I would rather get a simple change in >>>>>>>> quickly, instead of designing the perfect class hierarchy. >>>>>>> >>>>>>> It doesn't have to be perfect. I agree that simple is better. >>>>>>> >>>>>>> To me, registering a QOM type and looking it up when necessary is >>>>>>> simpler than the above. Even if it's a weird class having no >>>>>>> object instances. It probably could be an interface type. >>>>>> >>>>>> Registering a QOM type still has quite some boilerplate. [...] >>>>> >>>>> We're working on that. :) >>>>> >>>>>> [...] Also registering a >>>>>> QOM type has a public side effect (shows up in qom-list-types). In general >>>>>> I don't look at QOM unless I want its property mechanism, but maybe that's >>>>>> just me. >>>>> >>>>> We have lots of internal-use-only types returned by >>>>> qom-list-types, I don't think it's a big deal. >>>>> >>>>>> >>>>>>>> Perhaps another idea would be to allow adding interfaces to classes >>>>>>>> *separately from the registration of the types*. Then we can use it to add >>>>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and >>>>>>>> add the accel object to usermode emulators. >>>>>>> >>>>>>> I'm not sure I follow. What do you mean by bare bones accel >>>>>>> class, and when exactly would you add the new interfaces to the >>>>>>> classes? >>>>>> >>>>>> A bare bones accel class would not have init_machine and setup_post methods; >>>>>> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have >>>>>> properties (such as tb-size for TCG) and would be able to register compat >>>>>> properties. >>> >>> [1] >>> >>>>> >>>>> Oh, I think I see. This could save us having a lot of parallel type >>>>> hierarchies. >>>>> >>>>>> >>>>>> Where would I add it, I don't know. It could be a simple public wrapper >>>>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase after >>>>>> QOM. >>>>>> >>>>>> Or without adding a new phase, it could be a class_type->array of >>>>>> (interface_type, init_fn) hash table. type_initialize would look up the >>>>>> class_type by name, add the interfaces would to the class with >>>>>> type_initialize_interface, and then call the init_fn to fill in the vtable. >>>>> >>>>> That sounds nice. I don't think Claudio's cleanup should be >>>>> blocked until this new mechanism is ready, though. >>>>> >>>>> We don't really need the type representing X86CPUAccel to be a >>>>> subtype of TYPE_ACCEL or an interface implemented by >>>>> current_machine->accelerator, in the first version. We just need >>>>> a simple way for the CPU initialization code to find the correct >>>>> X86CPUAccel struct. >>>>> >>>>> While we don't have the new mechanism, it can be just a: >>>>> object_class_by_name("%s-x86-cpu-accel" % (accel->name)) >>>>> call. >>>>> >>>>> Below is a rough draft of what I mean. There's still lots of >>>>> room for cleaning it up (especially getting rid of the >>>>> X86CPUClass.common_class_init and X86CPUClass.accel fields). >>>>> >>>>> git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >>>>> >>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> [...] >>>>> /** >>>>> - * X86CPUAccel: >>>>> - * @name: string name of the X86 CPU Accelerator >>>>> - * >>>>> + * X86CPUAccelInterface: >>>>> * @common_class_init: initializer for the common cpu >>>>> * @instance_init: cpu instance initialization >>>>> * @realizefn: realize function, called first in x86 cpu realize >>>>> @@ -85,14 +83,16 @@ struct X86CPUClass { >>>>> * X86 CPU accelerator-specific CPU initializations >>>>> */ >>>>> >>>>> -struct X86CPUAccel { >>>>> - const char *name; >>>>> - >>>>> +struct X86CPUAccelInterface { >>>>> + ObjectClass parent_class; >>>>> void (*common_class_init)(X86CPUClass *xcc); >>>>> void (*instance_init)(X86CPU *cpu); >>>>> void (*realizefn)(X86CPU *cpu, Error **errp); >>>>> }; >>>>> >>>>> -void x86_cpu_accel_init(const X86CPUAccel *accel); >>>>> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" >>>>> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); >>>> >>>> >>>> I am not exactly sure what precisely you are doing here, >>>> >>>> I get the general intention to use the existing interface-related "stuff" in QOM, >>>> but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. >>> >>> See the git URL I sent above, for other related changes: >>> >>> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >> >> >> Aaah I missed this, there are quite a few more changes there; >> >> for me it's great if you take it from there, I see you are >> developing a solution on top of the previous series. > > I'm a bit busy with other stuff, so I'm probably not going to be > able to make sure the patches are in a good shape to be submitted > soon. > > I don't want to impose any obstacles for the work you are doing, > either. Please consider the patch I sent (and the git tree > above) as just an example of a possible solution to the two issues > Paolo raised at https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com Ok, thanks for the clarification, will take those two issues up in my next attempt. > >> >> >>> >>>> >>>> Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? >>> >>> An interface is declared in a very similar way, but instance_size >>> and the instance type cast macro is a bit different (because >>> instances of interface types are never created directly). >>> >>> For the draft we have here, it shouldn't make any difference if >>> you use OBJECT_DECLARE_TYPE, because the instance type cast >>> macros are left unused. >>> >>> Normally the use case for interfaces is not like what I did here. >>> Interfaces are usually attached to other classes (to declare that >>> object instances of that class implement the methods of that >>> interface). Using interfaces would be just an intermediate step >>> to the solution Paolo was mentioning (dynamically adding >>> interface to classes, see [1] above). >>> >> >> Makes sense to me, >> let me know how you guys would like to proceed from here. >> > > To me, the main issue (also raised by Paolo above) is the fact > that you are doing *_enabled() checks in the module init > functions. Every single use case we have for module init > functions today is for unconditionally registering code or data > structures provided by a code module (config group names, QOM > types, block backends, multifd methods, etc.), and none of them > depend on runtime options (like machine or accelerator options). Ok, no _enabled() checks, got it. Just to note, since _enabled() has multiple meanings depending on configuration (CONFIG_KVM), the _enabled() used in kvm/cpu.c has actually the meaning of: if (accelerator_finally_chosen == KVM). But we can refactor this implicit check out, and make it instead a accel_cpu_init("kvm"), like you suggest, so that the ifs disappear. > > The x86_cpu_accel_init() calls, on the other hand, are not module > initialization, but just one additional step of > machine/accelerator/cpu initialization. > > >> The thing I am still uncertain about, looking at your tree at: >> >> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >> >> is the removal of MODULE_INIT_ACCEL_CPU, it would be way simpler to understand I think, >> both for CpuAccelOps and X86CPUAccel, and is actualy in my view a perfect fit for >> the problem that module_call_init is supposed to solve. > > That was one of my goals. My first goal was the removal of the > (hvm|kvm|tcg)_enabled() checks in the accel init functions. My > secondary goal (and a side effect of the first goal) was making > MODULE_INIT_ACCEL_CPU unnecessary. > > If we are not trying to remove the *_enabled() checks in the > accel init functions (nor trying to make MODULE_INIT_ACCEL_CPU > unnecessary), my suggestion of using QOM doesn't make things > simpler. > > Let's hear what Paolo thinks. > > If you want to proceed with the accel_register_call() solution > suggested by Paolo, that's OK to me. I just don't think we > really need it, because QOM already solves the problem for us. > Ok, thanks for all the input, will need some time to process, thanks, Claudio
On 11/23/20 10:29 AM, Claudio Fontana wrote: > On 11/20/20 7:09 PM, Eduardo Habkost wrote: >> On Fri, Nov 20, 2020 at 06:41:35PM +0100, Claudio Fontana wrote: >>> On 11/20/20 6:19 PM, Eduardo Habkost wrote: >>>> On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote: >>>>> On 11/18/20 11:07 PM, Eduardo Habkost wrote: >>>>>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote: >>>>>>> On 18/11/20 18:30, Eduardo Habkost wrote: >>>>>>>>> Adding a layer of indirect calls is not very different from monkey patching >>>>>>>>> though. >>>>>>>> >>>>>>>> I'm a little bothered by monkey patching, but I'm more >>>>>>>> bothered by having to: >>>>>>>> >>>>>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that >>>>>>>> (2) register (accel_register_call()) a function (kvm_cpu_accel_init()) that >>>>>>>> (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel kvm_cpu_accel) that >>>>>>>> (4) will be saved in multiple QOM classes, so that >>>>>>>> (5) we will call the right X86CPUClass.accel method at the right moment >>>>>>>> (common_class_init(), instance_init(), realizefn()), >>>>>>>> where: >>>>>>>> step 4 must be done before any CPU object is created >>>>>>>> (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn >>>>>>>> will be silently ignored), and >>>>>>>> step 3 must be done after all QOM types were registered. >>>>>>>> >>>>>>>>> You also have to consider that accel currently does not exist in usermode >>>>>>>>> emulators, so that's an issue too. I would rather get a simple change in >>>>>>>>> quickly, instead of designing the perfect class hierarchy. >>>>>>>> >>>>>>>> It doesn't have to be perfect. I agree that simple is better. >>>>>>>> >>>>>>>> To me, registering a QOM type and looking it up when necessary is >>>>>>>> simpler than the above. Even if it's a weird class having no >>>>>>>> object instances. It probably could be an interface type. >>>>>>> >>>>>>> Registering a QOM type still has quite some boilerplate. [...] >>>>>> >>>>>> We're working on that. :) >>>>>> >>>>>>> [...] Also registering a >>>>>>> QOM type has a public side effect (shows up in qom-list-types). In general >>>>>>> I don't look at QOM unless I want its property mechanism, but maybe that's >>>>>>> just me. >>>>>> >>>>>> We have lots of internal-use-only types returned by >>>>>> qom-list-types, I don't think it's a big deal. >>>>>> >>>>>>> >>>>>>>>> Perhaps another idea would be to allow adding interfaces to classes >>>>>>>>> *separately from the registration of the types*. Then we can use it to add >>>>>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, and >>>>>>>>> add the accel object to usermode emulators. >>>>>>>> >>>>>>>> I'm not sure I follow. What do you mean by bare bones accel >>>>>>>> class, and when exactly would you add the new interfaces to the >>>>>>>> classes? >>>>>>> >>>>>>> A bare bones accel class would not have init_machine and setup_post methods; >>>>>>> those would be in a TYPE_SOFTMMU_ACCEL interface. It would still have >>>>>>> properties (such as tb-size for TCG) and would be able to register compat >>>>>>> properties. >>>> >>>> [1] >>>> >>>>>> >>>>>> Oh, I think I see. This could save us having a lot of parallel type >>>>>> hierarchies. >>>>>> >>>>>>> >>>>>>> Where would I add it, I don't know. It could be a simple public wrapper >>>>>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase after >>>>>>> QOM. >>>>>>> >>>>>>> Or without adding a new phase, it could be a class_type->array of >>>>>>> (interface_type, init_fn) hash table. type_initialize would look up the >>>>>>> class_type by name, add the interfaces would to the class with >>>>>>> type_initialize_interface, and then call the init_fn to fill in the vtable. >>>>>> >>>>>> That sounds nice. I don't think Claudio's cleanup should be >>>>>> blocked until this new mechanism is ready, though. >>>>>> >>>>>> We don't really need the type representing X86CPUAccel to be a >>>>>> subtype of TYPE_ACCEL or an interface implemented by >>>>>> current_machine->accelerator, in the first version. We just need >>>>>> a simple way for the CPU initialization code to find the correct >>>>>> X86CPUAccel struct. >>>>>> >>>>>> While we don't have the new mechanism, it can be just a: >>>>>> object_class_by_name("%s-x86-cpu-accel" % (accel->name)) >>>>>> call. >>>>>> >>>>>> Below is a rough draft of what I mean. There's still lots of >>>>>> room for cleaning it up (especially getting rid of the >>>>>> X86CPUClass.common_class_init and X86CPUClass.accel fields). >>>>>> >>>>>> git tree at https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >>>>>> >>>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>> [...] >>>>>> /** >>>>>> - * X86CPUAccel: >>>>>> - * @name: string name of the X86 CPU Accelerator >>>>>> - * >>>>>> + * X86CPUAccelInterface: >>>>>> * @common_class_init: initializer for the common cpu >>>>>> * @instance_init: cpu instance initialization >>>>>> * @realizefn: realize function, called first in x86 cpu realize >>>>>> @@ -85,14 +83,16 @@ struct X86CPUClass { >>>>>> * X86 CPU accelerator-specific CPU initializations >>>>>> */ >>>>>> >>>>>> -struct X86CPUAccel { >>>>>> - const char *name; >>>>>> - >>>>>> +struct X86CPUAccelInterface { >>>>>> + ObjectClass parent_class; >>>>>> void (*common_class_init)(X86CPUClass *xcc); >>>>>> void (*instance_init)(X86CPU *cpu); >>>>>> void (*realizefn)(X86CPU *cpu, Error **errp); >>>>>> }; >>>>>> >>>>>> -void x86_cpu_accel_init(const X86CPUAccel *accel); >>>>>> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel" >>>>>> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, X86_CPU_ACCEL); >>>>> >>>>> >>>>> I am not exactly sure what precisely you are doing here, >>>>> >>>>> I get the general intention to use the existing interface-related "stuff" in QOM, >>>>> but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem like the other boilerplate used for interfaces. >>>> >>>> See the git URL I sent above, for other related changes: >>>> >>>> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >>> >>> >>> Aaah I missed this, there are quite a few more changes there; >>> >>> for me it's great if you take it from there, I see you are >>> developing a solution on top of the previous series. >> >> I'm a bit busy with other stuff, so I'm probably not going to be >> able to make sure the patches are in a good shape to be submitted >> soon. >> >> I don't want to impose any obstacles for the work you are doing, >> either. Please consider the patch I sent (and the git tree >> above) as just an example of a possible solution to the two issues >> Paolo raised at https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com > > > Ok, thanks for the clarification, > will take those two issues up in my next attempt. > > >> >>> >>> >>>> >>>>> >>>>> Could you clarify what happens here? Should we just use a normal object, call it "Interface" and call it a day? >>>> >>>> An interface is declared in a very similar way, but instance_size >>>> and the instance type cast macro is a bit different (because >>>> instances of interface types are never created directly). >>>> >>>> For the draft we have here, it shouldn't make any difference if >>>> you use OBJECT_DECLARE_TYPE, because the instance type cast >>>> macros are left unused. >>>> >>>> Normally the use case for interfaces is not like what I did here. >>>> Interfaces are usually attached to other classes (to declare that >>>> object instances of that class implement the methods of that >>>> interface). Using interfaces would be just an intermediate step >>>> to the solution Paolo was mentioning (dynamically adding >>>> interface to classes, see [1] above). >>>> >>> >>> Makes sense to me, >>> let me know how you guys would like to proceed from here. >>> >> >> To me, the main issue (also raised by Paolo above) is the fact >> that you are doing *_enabled() checks in the module init >> functions. Every single use case we have for module init >> functions today is for unconditionally registering code or data >> structures provided by a code module (config group names, QOM >> types, block backends, multifd methods, etc.), and none of them >> depend on runtime options (like machine or accelerator options). > > > Ok, no _enabled() checks, got it. > > Just to note, since _enabled() has multiple meanings depending on configuration (CONFIG_KVM), > the _enabled() used in kvm/cpu.c has actually the meaning of: > > if (accelerator_finally_chosen == KVM). > > But we can refactor this implicit check out, and make it instead a > > accel_cpu_init("kvm"), like you suggest, so that the ifs disappear. > > > >> >> The x86_cpu_accel_init() calls, on the other hand, are not module >> initialization, but just one additional step of >> machine/accelerator/cpu initialization. >> >> >>> The thing I am still uncertain about, looking at your tree at: >>> >>> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel >>> >>> is the removal of MODULE_INIT_ACCEL_CPU, it would be way simpler to understand I think, >>> both for CpuAccelOps and X86CPUAccel, and is actualy in my view a perfect fit for >>> the problem that module_call_init is supposed to solve. >> >> That was one of my goals. My first goal was the removal of the >> (hvm|kvm|tcg)_enabled() checks in the accel init functions. My >> secondary goal (and a side effect of the first goal) was making >> MODULE_INIT_ACCEL_CPU unnecessary. >> >> If we are not trying to remove the *_enabled() checks in the >> accel init functions (nor trying to make MODULE_INIT_ACCEL_CPU >> unnecessary), my suggestion of using QOM doesn't make things >> simpler. >> >> Let's hear what Paolo thinks. >> >> If you want to proceed with the accel_register_call() solution >> suggested by Paolo, that's OK to me. I just don't think we >> really need it, because QOM already solves the problem for us. >> > > Ok, thanks for all the input, will need some time to process, > > thanks, > > Claudio > One idea that came to mind is, why not extend accel.h to user mode? It already contains #ifndef CONFIG_USER_ONLY parts, so maybe it was meant to be used by both, and just happened to end up confined to include/softmmu ? Basically I was thinking, we could have an AccelState and an AccelClass for user mode as well (without bringing in the whole machine thing), and from there we could use current_accel() to build up the right name for the chosen accelerator? Ciao, Claudio
On 23/11/20 10:55, Claudio Fontana wrote: > One idea that came to mind is, why not extend accel.h to user mode? > > It already contains > > #ifndef CONFIG_USER_ONLY > > parts, so maybe it was meant to be used by both, and just happened to > end up confined to include/softmmu ? > > Basically I was thinking, we could have an AccelState and an > AccelClass for user mode as well (without bringing in the whole > machine thing), and from there we could use current_accel() to build > up the right name for the chosen accelerator? Yes, extending the accelerator class to usermode emulation is certainly a good idea. Paolo
On 11/23/20 2:18 PM, Paolo Bonzini wrote: > On 23/11/20 10:55, Claudio Fontana wrote: >> One idea that came to mind is, why not extend accel.h to user mode? >> >> It already contains >> >> #ifndef CONFIG_USER_ONLY >> >> parts, so maybe it was meant to be used by both, and just happened to >> end up confined to include/softmmu ? >> >> Basically I was thinking, we could have an AccelState and an >> AccelClass for user mode as well (without bringing in the whole >> machine thing), and from there we could use current_accel() to build >> up the right name for the chosen accelerator? > > Yes, extending the accelerator class to usermode emulation is certainly > a good idea. > > Paolo > Thanks, I'll work on this option. Btw considering that CpusAccel for tcg is actually three different interfaces (for mttcg, for icount, and plain RR), it will be tough to, in the stated objective, "remove all conditionals", even after removing the tcg_enabled(). I wonder how you see this issue (patches for 3 TCG split are in Richard's queue atm). static void tcg_accel_cpu_init(void) { if (tcg_enabled()) { TCGState *s = TCG_STATE(current_accel()); if (s->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); } } } Ciao, Claudio
On 23/11/20 16:02, Claudio Fontana wrote: > Thanks, I'll work on this option. > > Btw considering that CpusAccel for tcg is actually three different interfaces (for mttcg, for icount, and plain RR), > it will be tough to, in the stated objective, "remove all conditionals", even after removing the tcg_enabled(). I'm not sure removing all conditionals is a goal in and of itself, but of course keeping the conditionals more local should be a good. Paolo > I wonder how you see this issue (patches for 3 TCG split are in Richard's queue atm). > > static void tcg_accel_cpu_init(void) > { > if (tcg_enabled()) { > TCGState *s = TCG_STATE(current_accel()); > > if (s->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); > } > }
On Mon, Nov 23, 2020 at 04:02:24PM +0100, Claudio Fontana wrote: > On 11/23/20 2:18 PM, Paolo Bonzini wrote: > > On 23/11/20 10:55, Claudio Fontana wrote: > >> One idea that came to mind is, why not extend accel.h to user mode? > >> > >> It already contains > >> > >> #ifndef CONFIG_USER_ONLY > >> > >> parts, so maybe it was meant to be used by both, and just happened to > >> end up confined to include/softmmu ? > >> > >> Basically I was thinking, we could have an AccelState and an > >> AccelClass for user mode as well (without bringing in the whole > >> machine thing), and from there we could use current_accel() to build > >> up the right name for the chosen accelerator? > > > > Yes, extending the accelerator class to usermode emulation is certainly > > a good idea. > > > > Paolo > > > > Thanks, I'll work on this option. > > Btw considering that CpusAccel for tcg is actually three different interfaces (for mttcg, for icount, and plain RR), > it will be tough to, in the stated objective, "remove all conditionals", even after removing the tcg_enabled(). The main issue were the conditionals inside module init function. They are completely OK inside accel-specific methods. > > I wonder how you see this issue (patches for 3 TCG split are in Richard's queue atm). > > static void tcg_accel_cpu_init(void) > { > if (tcg_enabled()) { > TCGState *s = TCG_STATE(current_accel()); > > if (s->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); > } > } > } Probably what we are missing here is a non-softmmu-specific AccelClass.init() method?
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 9ef5daf4c5..509b249f52 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2251,8 +2251,6 @@ static int kvm_init(MachineState *ms) ret = ram_block_discard_disable(true); assert(!ret); } - - cpus_register_accel(&kvm_cpus); return 0; err: @@ -3236,3 +3234,12 @@ 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/qtest/qtest.c b/accel/qtest/qtest.c index b282cea5cf..8d14059e32 100644 --- a/accel/qtest/qtest.c +++ b/accel/qtest/qtest.c @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { static int qtest_init_accel(MachineState *ms) { - cpus_register_accel(&qtest_cpus); return 0; } @@ -58,3 +57,12 @@ static void qtest_type_init(void) } 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-all.c b/accel/tcg/tcg-all.c index fa1208158f..9ffedc8151 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -104,8 +104,6 @@ static int tcg_init(MachineState *ms) tcg_exec_init(s->tb_size * 1024 * 1024); mttcg_enabled = s->mttcg_enabled; - cpus_register_accel(&tcg_cpus); - return 0; } @@ -201,3 +199,12 @@ static void register_accel_types(void) } type_init(register_accel_types); + +static void tcg_accel_cpu_init(void) +{ + if (tcg_enabled()) { + cpus_register_accel(&tcg_cpus); + } +} + +accel_cpu_init(tcg_accel_cpu_init); diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index 878a4089d9..6932a9f364 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -185,9 +185,6 @@ static int xen_init(MachineState *ms) * opt out of system RAM being allocated by generic code */ mc->default_ram_id = NULL; - - cpus_register_accel(&xen_cpus); - return 0; } @@ -228,3 +225,12 @@ static void xen_type_init(void) } 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/qemu/module.h b/include/qemu/module.h index 944d403cbd..485eda986a 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -44,6 +44,7 @@ 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, @@ -54,6 +55,7 @@ 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/softmmu/vl.c b/softmmu/vl.c index e6e0ad5a92..df4bed056a 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -4173,6 +4173,12 @@ 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/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c index fecfe8cd6e..3bada019f5 100644 --- a/target/i386/hax/hax-all.c +++ b/target/i386/hax/hax-all.c @@ -364,9 +364,6 @@ static int hax_accel_init(MachineState *ms) !ret ? "working" : "not working", !ret ? "fast virt" : "emulation"); } - if (ret == 0) { - cpus_register_accel(&hax_cpus); - } return ret; } @@ -1141,3 +1138,12 @@ 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/hvf/hvf.c b/target/i386/hvf/hvf.c index ed9356565c..249b77797f 100644 --- a/target/i386/hvf/hvf.c +++ b/target/i386/hvf/hvf.c @@ -887,7 +887,6 @@ static int hvf_accel_init(MachineState *ms) hvf_state = s; memory_listener_register(&hvf_memory_listener, &address_space_memory); - cpus_register_accel(&hvf_cpus); return 0; } @@ -911,3 +910,12 @@ 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 f4f3e33eac..2e715e2bc6 100644 --- a/target/i386/whpx/whpx-all.c +++ b/target/i386/whpx/whpx-all.c @@ -1642,8 +1642,6 @@ static int whpx_accel_init(MachineState *ms) whpx_memory_init(); - cpus_register_accel(&whpx_cpus); - printf("Windows Hypervisor Platform accelerator is operational\n"); return 0; @@ -1713,3 +1711,12 @@ 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);
apply this to the registration of the cpus accel interfaces, but this will be also in preparation for later use of this new module init step to also defer the registration of the cpu models, in order to make them subclasses of a per-accel cpu type. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- accel/kvm/kvm-all.c | 11 +++++++++-- accel/qtest/qtest.c | 10 +++++++++- accel/tcg/tcg-all.c | 11 +++++++++-- accel/xen/xen-all.c | 12 +++++++++--- include/qemu/module.h | 2 ++ softmmu/vl.c | 6 ++++++ target/i386/hax/hax-all.c | 12 +++++++++--- target/i386/hvf/hvf.c | 10 +++++++++- target/i386/whpx/whpx-all.c | 11 +++++++++-- 9 files changed, 71 insertions(+), 14 deletions(-)