On Tue, Nov 24, 2020 at 07:29:50PM +0100, Claudio Fontana wrote: > On 11/24/20 6:08 PM, Eduardo Habkost wrote: > > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote: > >> apply this to the registration of the cpus accel interfaces, > >> > >> but this will be also in preparation for later use of this > >> new module init step to also register per-accel x86 cpu type > >> interfaces. > >> > >> Signed-off-by: Claudio Fontana <cfont...@suse.de> > >> --- > > [...] > >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c > >> index b4e731cb2b..482f89729f 100644 > >> --- a/accel/qtest/qtest.c > >> +++ b/accel/qtest/qtest.c > >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { > >> > >> static int qtest_init_accel(MachineState *ms) > >> { > >> - cpus_register_accel(&qtest_cpus); > >> return 0; > >> } > >> > >> @@ -58,3 +57,12 @@ static void qtest_type_init(void) > >> } > >> > >> type_init(qtest_type_init); > >> + > >> +static void qtest_accel_cpu_init(void) > >> +{ > >> + if (qtest_enabled()) { > >> + cpus_register_accel(&qtest_cpus); > >> + } > >> +} > >> + > >> +accel_cpu_init(qtest_accel_cpu_init); > > > > I don't understand why this (and the similar changes on other > > accelerators) is an improvement. > > > > You are replacing a trivial AccelClass-specific init method with > > a module_init() function that has a hidden dependency on runtime > > state. > > > > Not a big advantage I agree, > I think however there is one, in using the existing framework that exists, > for the purposes that it was built for. > > As I understand it, the global module init framework is supposed to mark the > major initialization steps, > and this seems to fit the bill.
That seems to be the main source of disagreement. I don't agree that's the purpose of module_init(). The module init framework is used to unconditionally register module-provided entities like option names, QOM types, block drivers, trace events, etc. The entities registered by module init functions represent a passive dynamically loadable piece of code. module_init() was never used for initialization of machines, devices, CPUs, or other runtime state. We don't have MODULE_INIT_MONITOR, MODULE_INIT_OBJECTS, MODULE_INIT_MACHINE, MODULE_INIT_CPUS, MODULE_INIT_DEVICES, etc. And I'm not convinced we should, because it would hide dependencies between initialization steps. It would force us to make initialization functions affect and depend on global state. I believe this: int main() { result_of_A = init_A(input_for_A); result_of_B = init_B(input_for_B); result_of_C = init_C(input_for_C); } is clearer and more maintainable than: int main() { module_init_call(MODULE_INIT_A); /* result_of_A hidden in global state */ module_init_call(MODULE_INIT_B); /* result_of_B hidden in global state */ module_init_call(MODULE_INIT_C); /* result_of_C hidden in global state */ } > > The "hidden" dependency on the fact that accels need to be initialized at > that time, is not hidden at all I think, > it is what this module init step is all about. > > It is explicitly meaning, "_now that the current accelerator is chosen_, > perform these initializations". > > But, as you mentioned elsewhere, I will in the meantime anyway squash these > things so they do not start fragmented at all, and centralize immediately. Agreed. We still need to sort out the disagreement above, or we'll spend a lot of energy arguing about code. -- Eduardo