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

[Paolo Bonzini]

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