Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps

[Eduardo Habkost]

On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote:
> On 11/24/20 8:27 PM, Eduardo Habkost wrote:
> > On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote:
> > [...]
> >>>> +    }
> >>>
> >>> Additionally, if you call arch_cpu_accel_init() here, you won't
> >>> need MODULE_INIT_ACCEL_CPU anymore.  The
> >>>
> >>>   module_call_init(MODULE_INIT_ACCEL_CPU)
> >>>
> >>> call with implicit dependencies on runtime state inside vl.c and
> >>> *-user/main.c becomes a trivial:
> >>>
> >>>   accel_init(accel)
> >>>
> >>> call in accel_init_machine() and *-user:main().
> >>
> >>
> >>
> >> I do need a separate thing for the arch cpu accel specialization though,
> >> without MODULE_INIT_ACCEL_CPU that link between all operations done at 
> >> accel-chosen time is missing..
> >>
> > 
> > I think this is a key point we need to sort out.
> > 
> > What do you mean by "link between all operations done at
> > accel-chosen time" and why that's important?
> 
> 
> For understanding by a reader that tries to figure this out,
> (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread).

Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU)
indirection makes this easier to figure out than just looking at
a accel_init() function that makes regular function calls?


> 
> And it could be that the high level plan to make accelerators fully 
> dynamically loadable plugins in the future
> also conditioned me to want to have a very clear demarcation line around the 
> choice of the accelerator.

We have dynamically loadable modules for other QOM types,
already, and they just use type_init().  I don't see why an extra
module_init() type makes this easier.

> 
> 
> > 
> > accel_init_machine() has 2-3 lines of code with side effects.  It
> > calls AccelClass.init_machine(), which may may have hundreds of
> 
> 
> could we initialize also all arch-dependent stuff in here?

You can, if you use a wrapper + stub, like arch_cpu_accel_init().


> 
> 
> > lines of code.  accel_setup_post() has one additional method
> > call, which is triggered at a slightly different moment.
> > 
> > You are using MODULE_INIT_ACCEL for 2 additional lines of code:
> > - the cpus_register_accel() call
> > - the x86_cpu_accel_init() call
> > 
> > What makes those 2 lines of code so special, to make them deserve
> > a completely new mechanism to trigger them, instead of using
> > trivial function calls inside a accel_init() function?
> > 
> 
> ...can we do also the x86_cpu_accel_init inside accel_init()?
> 
> 
> In any case I'll try also the alternative, it would be nice if I could bring 
> everything together under the same roof,
> and easily discoverable, both the arch-specific steps that we need to do at 
> accel-chosen time and the non-arch-specific steps.

One way to bring everything together under the same roof is to
call everything inside a accel_init() function.


> 
> Hope this helps clarifying where I am coming from,
> but I am open to have my mind changed, also trying the alternatives you 
> propose here could help me see first hand how things play out.

Thanks!

-- 
Eduardo