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

[Eduardo Habkost]

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