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

[Paolo Bonzini <pbonzini@redhat.com>]

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
> 
> 
> 
>