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