On Wed, Nov 25, 2020 at 12:48:22PM +0100, Claudio Fontana wrote: > On 11/25/20 10:32 AM, Claudio Fontana wrote: > > 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(). > > > On this one I see an issue: > > the *-user_main() would still need an ac->machine_init() call to initialize > tcg itself, > currently the accelerator initialization is put into ac->machine_init > > (tcg_init, kvm_init, xen_init, etc). > > Or are you proposing to move tcg initialization away from the current > ->machine_init(), > into the new ac->init called by accel_init()?
Yes. Anything that requires MachineState (and is softmmu-specific) would go to ->machine_init(). Anything that is not softmmu-specific would go to ->init(). > > This would make tcg even more different from the other accelerators. That's true, but isn't this only because TCG is the only one that really needs it? > > Or are you proposing for all accelerators to separate the initialization of > the accelerator itself > from the machine state input, leading to, for example, separating kvm-all.c > kvm_init() into two > functions, one which takes the input from MachineState and puts it into the > AccelState, and > another one which actually then initializes kvm proper? And same for all > accels? That would be possible (and maybe a good idea), but not necessary to make it work. > > In my view we could still do in *-user main.c, > > ac = ACCEL_GET_CLASS(current_accel()) > ac->machine_init(NULL); > ac->init_cpu_interfaces(ac); That would work too. I would implement it as an accel_init(NULL) call, however, to avoid duplicating the code from accel_init_machine(). Calling ->machine_init(NULL) is just a bit surprising because of the name (calling machine_init() when there's no machine), and because we know most accelerators will crash if getting a NULL argument. Anyway, the split between ->machine_init() and ->init() is just a suggestion. Keeping a single init method that accepts a NULL MachineState* as argument is not my favourite option, but it works. Whatever you choose, my only ask is to document clearly the expectations and requirements of the AccelClass methods you are using. > > to solve this, or something like that, but also the option of fixing all > accelerators to separate > the gathering of the input from the MachineState to the actual accelerator > initialization is > a possibility to me. > > Ciao, > > Claudio Thank you very much for your patience! I think we're going on the right direction. -- Eduardo
