On 11/24/20 9:34 PM, Eduardo Habkost wrote: > 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?
I agree, if we accomplish a single accel_init() call that does everything (accelerator initialization and arch independent ops initialization and arch dependent specialization of the x86 cpu), that would be the best outcome in my view also. > > >> >> 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(). > As mentioned elsewhere, I'll try to avoid stubs. One is too many I think in the codebase (well one is probably ok, but you get what I mean, I don't like their proliferation). > >> >> >>> 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. Will try! > > >> >> 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! > Thanks, Ciao, Claudio