Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU

[Eduardo Habkost <ehabkost@redhat.com>]

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.

-- 
Eduardo