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

[Paolo Bonzini <pbonzini@redhat.com>]

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