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 <cfont...@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. -- Eduardo