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

[Eduardo Habkost]

On Wed, Nov 18, 2020 at 03:51:44PM +0100, Paolo Bonzini wrote:
> 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.

Right.  Let's build on top of that:

Another alternative would be to store a (type, X86CPUAccel) tuple
directly, with the type as string.  This would save the extra
indirection of the x86_cpu_accel_init() call.

It turns out we already have a mechanism to register and store
(type, StructContainingFunctionPointers) tuples at initialization
time: QOM.

X86CPUAccel can become X86CPUAccelClass, and be registered as a
QOM type.  It could be a subtype of TYPE_ACCEL or not, it
shouldn't matter.

I remember this was suggested in a previous thread, but I don't
remember if there were any objections.

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

I agree.

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

Makes sense, if we really want to introduce a new accel_register_call()
abstraction.  I don't think we need it, though.

-- 
Eduardo