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

[Claudio Fontana <cfontana@suse.de>]

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