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

[Paolo Bonzini]

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